crc-org / vfkit

Apache License 2.0
123 stars 24 forks source link

Add support for a GUI, virtio-input and virtio-gpu devices #44

Closed jakecorrenti closed 1 year ago

jakecorrenti commented 1 year ago

Adds command line support for the user to add virtio-gpu and virtio-input devices which also ties into having a good experience with the newly added gui compatability.

Adds command line support for the user to start a graphical application window when their virtual machine starts through the --gui flag (assuming the user added a virtio-gpu device as well).

Adds some mutual exclusion during creation of the virtual machine configuration. Without calling runtime.LockOSThread(), starting the virtual machine subsequently starting a graphic application window would result in a SIGILL error. Alongside calling runtime.LockOSThread(), parallelized the running of the graphic application window and checking the status of the virtual machine. Otherwise, the two wouldn't be able to run simultaneously.

These new features can be testing using a command similar to this:

$ ./out/vfkit --cpus 2 --memory 2048 \
--bootloader efi,variable-store=/Users/jakecorrenti/efi-variable-store,create \
--device virtio-serial,stdio \
--device virtio-fs,sharedDir=/Users/jakecorrenti,mountTag=dir0 \
--device virtio-blk,path=/Users/jakecorrenti/Downloads/Fedora-Server-dvd-x86_64-38-1.6.iso \
--device virtio-net,nat,mac=72:20:43:d4:38:62 --device virtio-input,keyboard --device virtio-input,pointing --device virtio-gpu --gui
openshift-ci[bot] commented 1 year ago

Hi @jakecorrenti. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
gbraad commented 1 year ago

/ok-to-test

baude commented 1 year ago

@jakecorrenti nice work! and speedy too!

baude commented 1 year ago

LGTM

gbraad commented 1 year ago

please be aware that the git title is too long and gets broken up. best to just write 'gui' instead of the full 'graphical user interface' as I care more about the addition of the last words: virtio-gpu devices

cfergeau commented 1 year ago

please be aware that the git title is too long and gets broken up. best to just write 'gui' instead of the full 'graphical user interface' as I care more about the addition of the last words: virtio-gpu devices

Since you bring this up, I would have preferred (at least) 3 separate commits, each doing one specific thing, "add virtio-input support", "add virtio-gpu support", "add gui support". This also helps with the shortlog length ;)

gbraad commented 1 year ago

I would have preferred (at least) 3 separate commits,

Didn't want to be so picky at first, but the description talks about the "I added", "I also added", which refers to these, right. Prefer not to see 'I' in a description either. More like 'This adds'


Let's say, for future commits to separate them?

jakecorrenti commented 1 year ago

I would have preferred (at least) 3 separate commits,

Didn't want to be so picky at first, but the description talks about the "I added", "I also added", which refers to these, right. Prefer not to see 'I' in a description either. More like 'This adds'

Let's say, for future commits to separate them?

Fixed. I fixed the PR info but forgot to actually change the commit. Also, feel free to be picky. I want this to be a quality contribution

jakecorrenti commented 1 year ago

please be aware that the git title is too long and gets broken up. best to just write 'gui' instead of the full 'graphical user interface' as I care more about the addition of the last words: virtio-gpu devices

Since you bring this up, I would have preferred (at least) 3 separate commits, each doing one specific thing, "add virtio-input support", "add virtio-gpu support", "add gui support". This also helps with the shortlog length ;)

I can definitely see this being better, I had the habit of squashing everything. I think it would be beneficial to have some sort of CONTRIBUTING.md file to have a location for contributors to see what you want for a PR so you don't have to make the same change requests.

gbraad commented 1 year ago

we prefer small(er) commits as we have been in situations where bisect was required.

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, gbraad

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/crc-org/vfkit/blob/main/OWNERS)~~ [cfergeau] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment