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

jakecorrenti · 2023-06-06T21:08:47Z

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 · 2023-06-06T21:08:59Z

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cmd/vfkit/main.go

gbraad · 2023-06-07T12:46:58Z

/ok-to-test

baude · 2023-06-07T12:58:37Z

pkg/config/virtio.go

@@ -11,6 +11,27 @@ import (
 // The VirtioDevice interface is an interface which is implemented by all virtio devices.
 type VirtioDevice VMComponent

+const (


maybe it makes sense to break this group of devices into something like graphics.go or display.go. Dealer's choice.

cmd/vfkit/main.go

pkg/config/virtio.go

pkg/vf/virtio.go

pkg/config/virtio.go

baude · 2023-06-07T13:10:03Z

@jakecorrenti nice work! and speedy too!

baude · 2023-06-08T03:36:01Z

LGTM

cfergeau

Thanks a lot for your PR, this is going to be a useful addition to vfkit!
I'm only half way through the review, but here are my first round of comments.

pkg/config/config.go

pkg/config/virtio.go

pkg/vf/virtio.go

pkg/config/virtio.go

gbraad · 2023-06-09T01:16:37Z

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 · 2023-06-09T08:20:05Z

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 · 2023-06-09T10:22:28Z

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 · 2023-06-09T12:02:14Z

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 · 2023-06-09T12:03:57Z

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 · 2023-06-09T15:37:30Z

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

cfergeau

A bunch of additional comments now that I could look at the virtio-gpu code and test it, but overall looks good, thanks again!

cfergeau · 2023-06-09T15:58:05Z

cmd/vfkit/main.go

+	if opts.UseGUI {
+		for _, gpuDev := range vmConfig.VirtioGPUDevices() {
+			if gpuDev.Device == config.VirtioGPUDisplayDevice {
+				gpuDev.UsesGUI = true


I think in an earlier iteration you had a gui attribute which could be set on each virtio-gpu device, but now it seems to be gone?
The gui attribute on each gpu dev was fine with me (or did I make a comment about it?). It's also fine with me if gui is unconditionally enabled for all devices, but if you do it this way, I'd move the attribute to something higher level (vmConfig maybe?).

I don't think we'll want multiple virtio-gpu devices used for display very often, if you want multiple heads on your VM, I think it will be through one device + multiple scanouts. Not something to worry about at this point anyway :)

I didn't make any changes to the VirtioGPU type. The UsesGUI attribute is still there for each Virtio-gpu device, and is automatically set to false. It can only be set to true if the user provides the --gui flag which is initially checked here.

pkg/config/virtio.go

cfergeau · 2023-06-09T16:08:24Z

cmd/vfkit/main.go

+
+	if opts.UseGUI {
+		for _, gpuDev := range vmConfig.VirtioGPUDevices() {
+			if gpuDev.Device == config.VirtioGPUDisplayDevice {


I don't understand why this Device field is needed? It seems the code would work equally well if the field was removed?

My intent behind it was to leave it open to the possibility that additional graphics devices may be supported. But I can remove it and make the assumption that the only graphics device that will be added to the VM will be the display

But I can remove it and make the assumption that the only graphics device that will be added to the VM will be the display

I'd make that assumption for now and remove display. When we have a clear use case for multiple GPUs, some dispaly, some non-display, it should be easier to design the cmdline we want to use for that.

pkg/config/virtio.go

cmd/vfkit/main.go

cfergeau

Apart from this display comment, should be good to go in, thanks for the changes you've made so far!

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: ```bash $ ./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 ``` Signed-off-by: Jake Correnti <[email protected]>

openshift-ci · 2023-06-14T13:20:22Z

[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~~ [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from cfergeau and praveenkumar June 6, 2023 21:08

openshift-ci bot added the needs-ok-to-test label Jun 6, 2023

gbraad reviewed Jun 7, 2023

View reviewed changes

cmd/vfkit/main.go Outdated Show resolved Hide resolved

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Jun 7, 2023