VNC screenshot support on the API and on virtctl #8465

rmohr · 2022-09-13T16:28:57Z

What this PR does / why we need it:

Add a GET subresource endpoint virtualmachineinstances/<name>/vnc/screenshot which allows taking VNC screenshots in PNG format in a server-side fashion.

virtctl support is also included:

Support saving VNC screenshots in PNG format to files:

virctl vnc screenshot testvmi -f screenshot.png

Support piping VNC screenshots in PNG format:

virtctl vnc screenshot testvmi -f - | display

Support waking up the VNC screen when e.g. a screensaver hides the actual screen:

virtcl vnc screenshot testvmi --move-cursor -f screenshot.png

This allows addressing the following use-cases:

Allow users discovering bluescreens, crashing and stuck VMs while not giving them interactive access to vnc and console.
Allow UIs to give a quick debug-view on VMs without having to open heavy-weight VNC connections
Allow scripts collecting screenshot for automatic tasks
Allow debugging VMs when websocket is disabled due to integration constraints
Allow easier debuging of VMs when no interactive display is available and port-forwarding is difficult
Allow users to inspect the display, if they have no local VNC client available which we support

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Add a vnc screenshot REST endpoint and a "virtctl vnc screenshot" command for UI and script integration

rmohr · 2022-09-13T18:37:09Z

/cc @0xFelix

regarding to the virtctl changes. Would be great if you could to a review.

rmohr · 2022-09-15T07:19:22Z

/test pull-kubevirt-goveralls

vasiliy-ul

Nice feature. Looks good for me in general, I just left a couple of minor comments inline.

vasiliy-ul · 2022-09-16T10:45:34Z

pkg/virt-api/rest/vnc.go

+	rects := msg.(*vnc.FramebufferUpdateMessage).Rectangles
+
+	w := int(rects[0].Width)
+	h := int(rects[0].Height)


Is it worth checking len(rects) > 0 before doing rect[0]?

vasiliy-ul · 2022-09-16T10:52:23Z

pkg/virt-api/rest/vnc.go

+	_ = c.PointerEvent(0, 0, 0)
+	_ = c.PointerEvent(0, 1, 1)


This looks like a side effect (potentially unwanted). Maybe better to make it optional somehow (e.g. via an additional argument)?

Added a --move-cursor arguement and a moveCursor query parameter. Defaults to not moving.

vasiliy-ul · 2022-09-16T11:05:54Z

pkg/virtctl/vnc/screenshot/screenshot.go

+		if _, err := os.Stdout.Write(screenshot); err != nil {
+			return fmt.Errorf("failed to write image to stdout: %v", err)
+		}
+	} else if err := os.WriteFile(fileName, screenshot, os.ModePerm); err != nil {


https://pkg.go.dev/io/fs#ModePerm:

ModePerm FileMode = 0777 // Unix permission bits

0777 seems too permissive. Maybe 0600?

A common default would be 0644 IMO. But actually os.ModePerm should be alright, if os.WriteFile respects umask (which it should, see here).

Yes, still let me go with 0644, it will still make prper use of umask and is much more appropriate. Thanks.

vasiliy-ul · 2022-09-16T11:13:26Z

pkg/virtctl/vnc/screenshot/screenshot.go

+	// setup connection with VM
+	screenshot, err := virtCli.VirtualMachineInstance(namespace).Screenshot(vmi)
+	if err != nil {
+		return fmt.Errorf("Can't access VMI %s: %v: %s", vmi, err, string(screenshot))


I think if you just convert []byte as string(screenshot) it will print garbage and potentially unprintable or special characters. I would personally omit this part, but alternatively it can be encoded to e.g. base64

The main issue is that the error is very unspecific and the body contains the actual error (a general problem in our webhooks I think). Maybe I can find a solution to add it to the actual error.

So maybe I misunderstood it, but I got the impression that screenshot will contain only the PNG bits, no?
Or do you mean that in the error case, it will include the rest response?

yes, if there is a server error the err.Error() string just contains .e.g 500 internal error, and the actual reason for the error is in the body. That does not look right, due to potential issues, like you describe, but it is in virtctl at this moment the only way to print the actual reason for the error.

And depending on when it fails, it could indeed add some png parts already, resulting to ugly output. I will try to see if I can fix this the right way.

Done. Fixed by properly retrieving the error from the body in client-go which has the code to properly detect if a StatusError is sent in the body instead of the actual requested payload.

vasiliy-ul · 2022-09-16T11:17:17Z

tests/vnc_test.go

+			cmd := clientcmd.NewVirtctlCommand("vnc", "screenshot", "--namespace", vmi.Namespace, "--file", filePath, vmi.Name)
+			Expect(cmd.Execute()).To(Succeed())
+
+			f, err := os.Open(filePath)


Probably defer f.close() is needed here?

vasiliy-ul · 2022-09-16T11:19:52Z

tests/vnc_test.go

+			Expect(img.Bounds().Size().X).To(BeNumerically("==", 720))
+			Expect(img.Bounds().Size().Y).To(BeNumerically("==", 400))


Just to confirm: the framebuffer size 720x400 is specific to that particular VM image that is run in the test, right?

It is the default resolution of the qemu boot screen. Since the VM we start has no actual VM image, it should be stable to use, because there boots no OS which can change it.

0xFelix

Looks quite good! Added some comments.

0xFelix · 2022-09-19T09:00:24Z

pkg/virt-api/rest/vnc.go

+	activeConnectionMetric := apimetrics.NewActiveVNCConnection(request.PathParameter("namespace"), request.PathParameter("name"))
+	defer activeConnectionMetric.Dec()
+
+	ch := make(chan vnc.ServerMessage)


Put this above line 68?

0xFelix · 2022-09-19T09:02:19Z

pkg/virt-api/rest/vnc.go

+
+	// Then send a buffer update request
+	err = c.FramebufferUpdateRequest(false, 0, 0, c.FrameBufferWidth, c.FrameBufferHeight)
+


Remove new line?

0xFelix · 2022-09-19T09:15:50Z

pkg/virtctl/vnc/screenshot/screenshot.go

+			return c.Run(cmd, args)
+		},
+	}
+	cmd.Flags().StringVarP(&fileName, "file", "f", "", "where to store the VNC screenshot in PNG format. User '-' for stdout")


I think output or o would make it more clear that this is the output of the command. The flag file is often used as an input.

I would prefer to keep f and file because o is prominently used in kubectl for the output format (-o wide, ...), so I would it expect to be used for the image type if we would for instance start supporting different image formats (but no plans to do so right now).

Ok, then let's keep it.

0xFelix · 2022-09-19T09:17:30Z

pkg/virtctl/vnc/screenshot/screenshot.go

+	"kubevirt.io/kubevirt/pkg/virtctl/templates"
+)
+
+var fileName string


Can't fileName be part of the Screenshot struct instead of a global variable?

Definitely. Done.

0xFelix · 2022-09-19T09:19:17Z

pkg/virtctl/vnc/screenshot/screenshot.go

+		return err
+	}
+
+	vmi := args[0]


Put this above line 63?

0xFelix · 2022-09-19T09:24:59Z

pkg/virtctl/vnc/screenshot/screenshot.go

+		if _, err := os.Stdout.Write(screenshot); err != nil {
+			return fmt.Errorf("failed to write image to stdout: %v", err)
+		}
+	} else if err := os.WriteFile(fileName, screenshot, os.ModePerm); err != nil {


A common default would be 0644 IMO. But actually os.ModePerm should be alright, if os.WriteFile respects umask (which it should, see here).

0xFelix · 2022-09-19T09:30:19Z

staging/src/kubevirt.io/client-go/kubecli/websocket.go

@@ -131,3 +131,8 @@ func convert(err error) error {
 	}
 	return err
 }
+
+type readWriteCloser struct {


Is this actually used?

No. Removed. 👍

0xFelix · 2022-09-19T09:31:30Z

staging/src/kubevirt.io/client-go/kubecli/streamer.go

@@ -53,3 +53,10 @@ func (c *wsConn) SetDeadline(t time.Time) error {
 	}
 	return c.Conn.SetReadDeadline(t)
 }
+
+func NewWebsocketStreamer(conn *websocket.Conn, done chan struct{}) *wsStreamer {


Is this actually used?

Yes, in rest/vnc.go

Allow virt-api websocket dialers to return the virt-handler websocket connection itself or the underlying connection based on the need. Signed-off-by: Roman Mohr <rmohr@google.com>

Make use of the more flexible virt-handler dialers to directly get access to the websocket connection and use the websocket stream functionality to connect to vnc-go. Signed-off-by: Roman Mohr <rmohr@google.com>

Signed-off-by: Roman Mohr <rmohr@google.com>

Support saving VNC screenshots in PNG format to files: virctl vnc screenshot testvmi -f screenshot.png Support piping VNC screenshots in PNG format: virtctl vnc screenshot testvmi -f - | display Signed-off-by: Roman Mohr <rmohr@google.com>

Start a VM without a boot disk and take a screenshot. Due to not having a boot disk, the default resolution is known and we can check in the e2e test for it. Signed-off-by: Roman Mohr <rmohr@google.com>

rmohr

Thanks for the reviews! PTAL.

rmohr · 2022-09-20T07:35:48Z

pkg/virtctl/vnc/screenshot/screenshot.go

+			return c.Run(cmd, args)
+		},
+	}
+	cmd.Flags().StringVarP(&fileName, "file", "f", "", "where to store the VNC screenshot in PNG format. User '-' for stdout")


I would prefer to keep f and file because o is prominently used in kubectl for the output format (-o wide, ...), so I would it expect to be used for the image type if we would for instance start supporting different image formats (but no plans to do so right now).

rmohr · 2022-09-20T07:36:11Z

pkg/virtctl/vnc/screenshot/screenshot.go

+	"kubevirt.io/kubevirt/pkg/virtctl/templates"
+)
+
+var fileName string


Definitely. Done.

rmohr · 2022-09-20T07:36:31Z

pkg/virtctl/vnc/screenshot/screenshot.go

+		return err
+	}
+
+	vmi := args[0]


rmohr · 2022-09-20T07:36:39Z

pkg/virtctl/vnc/screenshot/screenshot.go

+	// setup connection with VM
+	screenshot, err := virtCli.VirtualMachineInstance(namespace).Screenshot(vmi)
+	if err != nil {
+		return fmt.Errorf("Can't access VMI %s: %v: %s", vmi, err, string(screenshot))


Done. Fixed by properly retrieving the error from the body in client-go which has the code to properly detect if a StatusError is sent in the body instead of the actual requested payload.

rmohr · 2022-09-20T07:37:56Z

pkg/virtctl/vnc/screenshot/screenshot.go

+		if _, err := os.Stdout.Write(screenshot); err != nil {
+			return fmt.Errorf("failed to write image to stdout: %v", err)
+		}
+	} else if err := os.WriteFile(fileName, screenshot, os.ModePerm); err != nil {


Yes, still let me go with 0644, it will still make prper use of umask and is much more appropriate. Thanks.

rmohr · 2022-09-20T08:20:41Z

pkg/virt-api/rest/vnc.go

+	rects := msg.(*vnc.FramebufferUpdateMessage).Rectangles
+
+	w := int(rects[0].Width)
+	h := int(rects[0].Height)


rmohr · 2022-09-20T08:20:56Z

staging/src/kubevirt.io/client-go/kubecli/websocket.go

@@ -131,3 +131,8 @@ func convert(err error) error {
 	}
 	return err
 }
+
+type readWriteCloser struct {


No. Removed. 👍

rmohr · 2022-09-20T08:21:03Z

staging/src/kubevirt.io/client-go/kubecli/streamer.go

@@ -53,3 +53,10 @@ func (c *wsConn) SetDeadline(t time.Time) error {
 	}
 	return c.Conn.SetReadDeadline(t)
 }
+
+func NewWebsocketStreamer(conn *websocket.Conn, done chan struct{}) *wsStreamer {


Yes, in rest/vnc.go

rmohr · 2022-09-20T08:21:38Z

pkg/virt-api/rest/vnc.go

+	_ = c.PointerEvent(0, 0, 0)
+	_ = c.PointerEvent(0, 1, 1)


Added a --move-cursor arguement and a moveCursor query parameter. Defaults to not moving.

rmohr · 2022-09-20T08:21:42Z

pkg/virt-api/rest/vnc.go

+	activeConnectionMetric := apimetrics.NewActiveVNCConnection(request.PathParameter("namespace"), request.PathParameter("name"))
+	defer activeConnectionMetric.Dec()
+
+	ch := make(chan vnc.ServerMessage)


rmohr

Thanks for the reviews! PTAL.

vasiliy-ul

Looks good 👍

0xFelix · 2022-09-21T12:07:34Z

Looks good to me!

/lgtm

rmohr · 2022-09-21T15:56:23Z

/retest

rmohr · 2022-09-26T07:08:22Z

thanks everyone for the reviews!

/approve

kubevirt-bot · 2022-09-26T07:08:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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

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

kubevirt-commenter-bot · 2022-09-26T12:28:01Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Sep 13, 2022

kubevirt-bot requested review from EdDev and kbidarkar September 13, 2022 16:29

rmohr removed request for kbidarkar and EdDev September 13, 2022 16:31

rmohr force-pushed the screenshot branch from 23a3b7f to 2cf0161 Compare September 13, 2022 17:10

rmohr changed the title ~~Screenshot~~ VNC screenshot support on the API and on virtctl Sep 13, 2022

kubevirt-bot requested a review from 0xFelix September 13, 2022 18:37

rmohr force-pushed the screenshot branch from 2cf0161 to 0f328da Compare September 14, 2022 12:11

vasiliy-ul reviewed Sep 16, 2022

View reviewed changes

0xFelix reviewed Sep 19, 2022

View reviewed changes

rmohr added 7 commits September 20, 2022 10:30

Rework virt-api dialers to be more flexible

74165c2

Allow virt-api websocket dialers to return the virt-handler websocket connection itself or the underlying connection based on the need. Signed-off-by: Roman Mohr <rmohr@google.com>

Add screenshot endpoint to virt-api

5e872d0

Make use of the more flexible virt-handler dialers to directly get access to the websocket connection and use the websocket stream functionality to connect to vnc-go. Signed-off-by: Roman Mohr <rmohr@google.com>

Add RBAC permissions for edit and admin roles

5ae5f4c

Signed-off-by: Roman Mohr <rmohr@google.com>

Add Screenshot method to client-go

750889e

Signed-off-by: Roman Mohr <rmohr@google.com>

Run make generate

2950851

Signed-off-by: Roman Mohr <rmohr@google.com>

Add virtctl vnc screenshot command

fa325ec

Support saving VNC screenshots in PNG format to files: virctl vnc screenshot testvmi -f screenshot.png Support piping VNC screenshots in PNG format: virtctl vnc screenshot testvmi -f - | display Signed-off-by: Roman Mohr <rmohr@google.com>

Add VNC screenshot e2e test

982e0f9

Start a VM without a boot disk and take a screenshot. Due to not having a boot disk, the default resolution is known and we can check in the e2e test for it. Signed-off-by: Roman Mohr <rmohr@google.com>

rmohr force-pushed the screenshot branch from 0f328da to 982e0f9 Compare September 20, 2022 08:37

rmohr commented Sep 20, 2022

View reviewed changes

vasiliy-ul approved these changes Sep 20, 2022

View reviewed changes

kubevirt-bot assigned vasiliy-ul Sep 20, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2022

kubevirt-bot assigned 0xFelix Sep 21, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2022

kubevirt-bot merged commit 16a9313 into kubevirt:main Sep 26, 2022

		Expect(img.Bounds().Size().X).To(BeNumerically("==", 720))
		Expect(img.Bounds().Size().Y).To(BeNumerically("==", 400))


		// Then send a buffer update request
		err = c.FramebufferUpdateRequest(false, 0, 0, c.FrameBufferWidth, c.FrameBufferHeight)

VNC screenshot support on the API and on virtctl #8465

VNC screenshot support on the API and on virtctl #8465

Conversation

rmohr commented Sep 13, 2022 • edited

rmohr commented Sep 13, 2022

rmohr commented Sep 15, 2022

vasiliy-ul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr left a comment

Choose a reason for hiding this comment

vasiliy-ul left a comment

Choose a reason for hiding this comment

0xFelix commented Sep 21, 2022

rmohr commented Sep 21, 2022

rmohr commented Sep 26, 2022

kubevirt-bot commented Sep 26, 2022

kubevirt-commenter-bot commented Sep 26, 2022

rmohr commented Sep 13, 2022 •

edited