Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle error so that it is shown in both page and task manager #247

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lstocchi
Copy link
Contributor

@lstocchi lstocchi commented Aug 2, 2024

This PR refactors a bit the start action so that the error, if any, is propagated properly in the logger and caller.

Starting from the resources page
start_error_1

Create and start the VM
start_error_2

@lstocchi lstocchi marked this pull request as ready for review August 2, 2024 13:40
@lstocchi lstocchi requested review from jeffmaury and dgolovin August 2, 2024 13:40
@lstocchi lstocchi linked an issue Aug 2, 2024 that may be closed by this pull request
Copy link
Collaborator

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not work for me seems the VM is created but not started

crc

So this may be due to another issue: when testing it prompted me to update CRC which I did so I am missing the latest Microshift bundle so crc start should fail

@jeffmaury
Copy link
Collaborator

But seems this PR is breaking the following workflow:

  • crc cleanup
  • launch Podman Desktop
  • go to the dashboard and select initialize

@lstocchi
Copy link
Contributor Author

But seems this PR is breaking the following workflow:

  • crc cleanup
  • launch Podman Desktop
  • go to the dashboard and select initialize

what do you mean by breaking? The initialize do nothing?

@lstocchi
Copy link
Contributor Author

Ok, got it. I didn't update the initialize command

@lstocchi
Copy link
Contributor Author

But seems this PR is breaking the following workflow:

  • crc cleanup
  • launch Podman Desktop
  • go to the dashboard and select initialize

When initializing, should the older crc machine gets removed? Should we ask the user?
Bc you update crc, then you should download the bundle, then you hsould create a ne vm

@jeffmaury
Copy link
Collaborator

But seems this PR is breaking the following workflow:

  • crc cleanup
  • launch Podman Desktop
  • go to the dashboard and select initialize

When initializing, should the older crc machine gets removed? Should we ask the user? Bc you update crc, then you should download the bundle, then you hsould create a ne vm

I would say yes but could be a follow up PR as it's not related to this PR

@lstocchi lstocchi force-pushed the i243_1 branch 3 times, most recently from a490ca0 to b256c1f Compare August 13, 2024 16:02
@lstocchi
Copy link
Contributor Author

I think i fixed the initialize but there is an old issue about the spinner

@jeffmaury
Copy link
Collaborator

I think i fixed the initialize but there is an old issue about the spinner

I still have an issue as it never ends (opposed to main) and I am never asked to logon to SSO (if I do crc cleanup before)

@lstocchi
Copy link
Contributor Author

@jeffmaury updated. The SSO will be asked when starting the VM. The initialize + start action is broken because of podman-desktop/podman-desktop#8514 so i think it never worked.

You can use the initialize and then go to the resources page.

src/extension.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
@dgolovin
Copy link
Contributor

@jeffmaury @lstocchi podman-desktop/podman-desktop#8514 should be fixed, because even if we fix everything on Openshift Local side initial experience is still going to be broken.

@jeffmaury
Copy link
Collaborator

I can't make this PR to work. If I run crc cleanup before and try Initialize and start, it works on main but never ends with this PR:

image

@lstocchi
Copy link
Contributor Author

lstocchi commented Aug 21, 2024

I can't make this PR to work. If I run crc cleanup before and try Initialize and start, it works on main but never ends with this PR:

@jeffmaury The initialize + start action is broken because of podman-desktop/podman-desktop#8514 so i think it never worked.

Edit: or if it worked it was using a broken logic bc the start should happen during the starting phase, so most probably it was started during the configurephase which is not correct, bc otherwise there is no difference between initialize and initialize + start

@lstocchi lstocchi requested a review from dgolovin August 21, 2024 08:25
@lstocchi
Copy link
Contributor Author

@jeffmaury @dgolovin i wonder if we can release without this PR. After all it just improves the error handling using the logger/taskManager but it was not there in previous releases, so there will be no regression. So we can merge the missing logic in desktop and then merge this PR and have a working solution next time

@jeffmaury
Copy link
Collaborator

I can't make this PR to work. If I run crc cleanup before and try Initialize and start, it works on main but never ends with this PR:

@jeffmaury The initialize + start action is broken because of podman-desktop/podman-desktop#8514 so i think it never worked.

Edit: or if it worked it was using a broken logic bc the start should happen during the starting phase, so most probably it was started during the configurephase which is not correct, bc otherwise there is no difference between initialize and initialize + start

Why does it work in main?

@lstocchi
Copy link
Contributor Author

Why does it work in main?

Bc in main the code is this
https://github.com/crc-org/crc-extension/pull/247/files#diff-04bba6a35cad1c794cbbe677678a51de13441b7a6ee8592b7b50be1f05c6f626L225

the initialize function calls the createVM which checks if crc needs to be setup (and it executes the setup if needed) and then start the vm. So technically it performs a setup + start which is wrong. The initialize action should only execute the setup action.

If you choose the initialize + start action what happens in podman desktop is that it calls the initialize method of the extension and then the start function in the registered provider lifecycle.

But in crc this never happens.

If you execute the initialize or initialize + start, the crc workflow will be the same.

Maybe it works but with an incorrect logic.

@jeffmaury
Copy link
Collaborator

Why does it work in main?

Bc in main the code is this https://github.com/crc-org/crc-extension/pull/247/files#diff-04bba6a35cad1c794cbbe677678a51de13441b7a6ee8592b7b50be1f05c6f626L225

the initialize function calls the createVM which checks if crc needs to be setup (and it executes the setup if needed) and then start the vm. So technically it performs a setup + start which is wrong. The initialize action should only execute the setup action.

If you choose the initialize + start action what happens in podman desktop is that it calls the initialize method of the extension and then the start function in the registered provider lifecycle.

But in crc this never happens.

If you execute the initialize or initialize + start, the crc workflow will be the same.

Maybe it works but with an incorrect logic.

I think this PR addresses 2 different concerns: refactoring the initialize and start and handle errors during start. So I think either we delay this PR after the release or we split it in 2 different PRs

@lstocchi
Copy link
Contributor Author

@jeffmaury @dgolovin rebased. With the new desktop, the initialize and start does not fail anymore. Please give it a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VM start failure is not reported in UI
3 participants