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

feat: add launch VM button #865

Merged
merged 6 commits into from
Oct 21, 2024
Merged

feat: add launch VM button #865

merged 6 commits into from
Oct 21, 2024

Conversation

cdrage
Copy link
Collaborator

@cdrage cdrage commented Sep 27, 2024

feat: add launch VM button

What does this PR do?

  • Using QEMU we add a feature to "Launch VM" in the background
  • Uses websockets, qemu as well as our xterm.js library to achieve this
  • Launches in "snapshot" mode so no data is written to .raw file so the
    file can be easily re-used

Screenshot / video of UI

Screen.Recording.2024-09-27.at.11.05.00.AM.mov

What issues does this PR fix or reference?

Closes #813

How to test this PR?

  1. Be on macOS silicon
  2. brew install qemu
  3. Build a bootc container image
  4. Press launch VM button in actions bar

Signed-off-by: Charlie Drage charlie@charliedrage.com

@cdrage cdrage requested a review from a team as a code owner September 27, 2024 15:06
@cdrage cdrage requested review from dgolovin, jeffmaury and lstocchi and removed request for a team September 27, 2024 15:06
@cdrage cdrage changed the title feat: add launch VM button [WIP / no review] feat: add launch VM button Sep 27, 2024
@cdrage cdrage marked this pull request as draft September 27, 2024 15:06
@cdrage cdrage force-pushed the add-terminal branch 2 times, most recently from c1e9589 to 9dbe951 Compare September 27, 2024 15:07
@cdrage
Copy link
Collaborator Author

cdrage commented Sep 27, 2024

This is an initial implementation and meant for experimenting as parts have not been finalized yet / further discussion needed.

Feel free to test it out @deboer-tim @slemeur

We can further discuss this during the week!

@cdrage cdrage force-pushed the add-terminal branch 2 times, most recently from 00a2992 to 017082e Compare September 27, 2024 15:11
@cdrage cdrage force-pushed the add-terminal branch 6 times, most recently from 7f889c4 to b38fa33 Compare September 30, 2024 19:05
@cdrage cdrage changed the title [WIP / no review] feat: add launch VM button [WIP] feat: add launch VM button Sep 30, 2024
@deboer-tim
Copy link
Collaborator

Comments from my initial testing:

  • That's a lot of text on the page. I wonder if we can detect if qemu is installed and give shorter messages, shorten text, or say there are caveats and move those to docs?
  • Do we hide the page for non-raw disk images? Feels like it might be annoying to always have it there when you can't use it, but is also good advertising.
  • I get errors like Error stopping VM: sh: line 0: kill: (97942) - No such process in the log, even before I've tried to start a VM.
  • Woo hoo! It worked perfectly, and was so nice to see this function (again).
  • The Connection opened label is maybe correct, but I think something like ' VM running' might be more intuitive.
  • I wanted to check something from the image I was using... unfortunate that you can't pop away for a minute.
  • I know leaving the page will stop it, but I felt like I should have a Stop button too. Or conversely, if I accidentally click out before stopping it might be nice to have a warning?

@cdrage
Copy link
Collaborator Author

cdrage commented Oct 1, 2024

Comments from my initial testing:

  • That's a lot of text on the page. I wonder if we can detect if qemu is installed and give shorter messages, shorten text, or say there are caveats and move those to docs?

Agreed, let's launch it and maybe just link to docs near the status icon to give more information?

  • Do we hide the page for non-raw disk images? Feels like it might be annoying to always have it there when you can't use it, but is also good advertising.

I don't think we should hide, but maybe an empty page saying "must build raw image, no raw image found"?

  • I get errors like Error stopping VM: sh: line 0: kill: (97942) - No such process in the log, even before I've tried to start a VM.

Intentional! Sorry!

  • Woo hoo! It worked perfectly, and was so nice to see this function (again).
  • The Connection opened label is maybe correct, but I think something like ' VM running' might be more intuitive.

Will fix.

  • I wanted to check something from the image I was using... unfortunate that you can't pop away for a minute.

I agree :( I think we should have that in a follow up issue / PR though.

  • I know leaving the page will stop it, but I felt like I should have a Stop button too. Or conversely, if I accidentally click out before stopping it might be nice to have a warning?

@cdrage
Copy link
Collaborator Author

cdrage commented Oct 1, 2024

Screen.Recording.2024-10-01.at.12.56.43.PM.mov

@deboer-tim Ready for another review! I have done the following:

  • No more button to launch VM, it will insta-launch
  • Added prereq checks for mac + QEMU
  • Removed the error logging (except for when a VM is shut down, still working on that).
  • Fixed connection closed / VM status labels

Feel free to test it out and let me know if this works and I can begin adding some unit tests so we can get this initial POC in.

@cdrage cdrage force-pushed the add-terminal branch 2 times, most recently from 73cc1d8 to a570f03 Compare October 1, 2024 17:06
@cdrage cdrage changed the title [WIP] feat: add launch VM button feat: add launch VM button Oct 1, 2024
@cdrage cdrage marked this pull request as ready for review October 1, 2024 19:14
@cdrage
Copy link
Collaborator Author

cdrage commented Oct 1, 2024

Ready for review with the exception of unit tests being added later.

This is marked as experimental (as per the tab) and only shown for macOS users.

Comment on lines 39 to 40
// Before launching, make sure that we stop any previously running VM's and ignore any errors when stopping
//await stopVM();
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

benoitf
benoitf previously requested changes Oct 3, 2024
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

need to revisit the design to be more pluggable

also probably needs to have a Class/instance/state per runner as here we're passing tons of useless parameters

@cdrage
Copy link
Collaborator Author

cdrage commented Oct 3, 2024

need to revisit the design to be more pluggable

also probably needs to have a Class/instance/state per runner as here we're passing tons of useless parameters

ACK, I'll refactor the launch-vm.ts part to make it more pluggable for future implementations (linux, windows, etc.)

@cdrage cdrage force-pushed the add-terminal branch 2 times, most recently from d9d4f4f to 99c3d3f Compare October 7, 2024 13:30
@cdrage cdrage requested a review from benoitf October 7, 2024 14:19
Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

As before, testing worked without any issues.

I don't love auto-launching a 'large' VM, but that's fine for first pass. The main drawback is the VM getting reset when you leave the tab, but again that is expected for this first step.

@@ -21,6 +21,7 @@ import type { ImageInfo, ImageInspectInfo, ManifestInspectInfo } from '@podman-d

export abstract class BootcApi {
abstract checkPrereqs(): Promise<string | undefined>;
abstract checkVMLaunchPrereqs(folder: string, architecture: string): Promise<string | undefined>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both backend and frontend know about list of BootcBuildInfos (i.e. disk images), but frontend grabs folder and arch from one of these to check if it can be launched, then to actually launchVM(). Seems like we could use the BootBuildInfo or its id directly instead? i.e. can I launch disk image X / run disk image Y.

packages/backend/src/vm-manager.ts Outdated Show resolved Hide resolved
@cdrage cdrage force-pushed the add-terminal branch 2 times, most recently from 561f0d5 to b314111 Compare October 16, 2024 18:11
@cdrage
Copy link
Collaborator Author

cdrage commented Oct 16, 2024

@deboer-tim updated with a new commit!

Let me know if that works and I can start on tests 👍

@deboer-tim
Copy link
Collaborator

Thanks for the changes! I was also suggesting instead of:
abstract launchVM(folder: string, architecture: string): Promise<void>;
API could be:
abstract launchVM(diskImageId: string): Promise<void>;
but please go ahead with wrapping up tests.

@cdrage
Copy link
Collaborator Author

cdrage commented Oct 16, 2024

Thanks for the changes! I was also suggesting instead of: abstract launchVM(folder: string, architecture: string): Promise<void>; API could be: abstract launchVM(diskImageId: string): Promise<void>; but please go ahead with wrapping up tests.

Done! I use BuildImageInfo for that section now because who knows what other information we may be passing in the future now.

@cdrage cdrage force-pushed the add-terminal branch 4 times, most recently from 464623c to 89f5a7a Compare October 17, 2024 18:23
### What does this PR do?

* Using QEMU we add a feature to "Launch VM" in the background
* Uses websockets, qemu as well as our xterm.js library to achieve this
* Launches in "snapshot" mode so no data is written to .raw file so the
  file can be easily re-used

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#813

### How to test this PR?

<!-- Please explain steps to reproduce -->

1. Be on macOS silicon
2. `brew install qemu`
3. Build a bootc container image
4. Press launch VM button in actions bar

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Collaborator Author

cdrage commented Oct 17, 2024

Tests added and ready for review 👍

@benoitf @deboer-tim

Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

Thanks for all the improvements, @cdrage. One minor url change noted but otherwise let's get this in and get some feedback!

@@ -17,7 +20,20 @@ async function deleteBuild(): Promise<void> {
async function gotoLogs(): Promise<void> {
router.goto(`/disk-image/${btoa(object.id)}/build`);
}

async function gotoVM(): Promise<void> {
router.goto(`/details/${btoa(object.id)}/vm`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to be updated to /disk-image/ after the nav change.

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Collaborator Author

cdrage commented Oct 21, 2024

@deboer-tim Updated and confirm link works now!

Agreed, let's get this in even if it's in nightly and get some feedback :)

Screen.Recording.2024-10-21.at.9.34.34.AM.mov

@cdrage cdrage enabled auto-merge (squash) October 21, 2024 13:36
@cdrage cdrage dismissed benoitf’s stale review October 21, 2024 13:51

dismissing as changes were made based on review + two other reviews of tim and axel

@cdrage cdrage merged commit 0bdb5a5 into containers:main Oct 21, 2024
6 checks passed
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.

Add initial "launch VM" implementation with terminal
5 participants