Add skeleton support for VNC server. #113

jordanhendricks · 2022-04-12T02:26:49Z

Summary

This PR implements prototype-level support in propolis-server for connecting to a propolis VM over VNC. Specifically:

propolis-server now has a vnc server listening on port 5900 (the bulk of the work for the server implemented in the rfb crate)
one can connect to the port with a VNC client prior to starting the guest and see a blank white screen
after the guest is started, the contents of the VM framebuffer should be visible in a VNC client (noVNC and tigervnc both seem to work on linux)

See the "Examples" section below for what this looks like in noVNC.

There's a lot left to implement. Broadly, this includes:

support for input devices (the keyboard and mouse events are sent to the vnc server, but propolis does not yet have device support here)
more encoding support. Currently the only encoding supported is Raw.
a fair amount of additional protocol support in the rfb crate (such as using the encodings, color format, and pixel format requested by the client)
addition of vnc support in propolis-standalone

Still, I am making this PR now in the interest of merging often and making review easier as this project is ongoing and has a lot of moving parts.

Overview of Changes

Addition of the rfb crate: This crate implements a server-side implementation of RFB, the protocol that VNC uses. Consumers use the crate by implementing the rfb::server::Server trait, which for now only requires a method to provide framebuffer data.
The new PropolisVncServer struct implements rfb::server::Server, which calls into the guest memory if the guest has been initialized (otherwise, it provides a white screen). A handle to this server is hung off the server Context that each dropshot endpoint gets. The propolis RamFb device notifies PropolisVncServer of updates when it's updated via fwcfg_rw.
I plumbed the RamFb entity ID through the intializer code so that its notifier function could be updated when an instance is created (instance_ensure).
The vnc server is created, kicked off, and awaited in server/src/main.rs.

Examples

Example Uninitialized Framebuffer (guest not started)

Here's an example of connecting without a guest running. I chose to make the screen white instead of black to differentiate between the framebuffer not being sent at all (which renders as black on some clients).

Example Initialized Framebuffer (guest started)

Here's a low-ish quality gif demonstrating what it looks like to connect to propolis-server over VNC, then create a VM and run it. (It looks better on a real client, I promise).

A couple of things to note here:

the client here (noVNC) leaves the old white background when the screen resolution changes. If I refresh the client this goes away. I'm not sure if this a bug on my end or the client; in any event I should probably figure out how to fix it.
The serial console prompt at the end only happens if I run the serial command on the CLI. I'm not sure why this is, or if we should expect to see the serial console here by default (I would think so).

pfmooney

I have a few comments, but am also inclined to land it sooner rather than later, and iterate after that.

pfmooney · 2022-04-13T17:02:27Z

server/src/lib/vnc.rs

+}
+
+#[derive(Clone)]
+pub struct PropolisVncServer {


style nit: probably don't need 'propolis' in the name

I just called it that because there's another type called rfb::VncServer and I found it confusing. I would prefer to name it not just VncServer but am down to rename it otherwise.

pfmooney · 2022-04-13T17:08:57Z

server/src/lib/vnc.rs

+                // Show a white screen if the guest isn't ready yet.
+                const LEN: usize = 1024 * 768 * 4;
+                let mut pixels = vec![0u8; LEN];
+                for i in 0..LEN {
+                    pixels[i] = 0xff;
+                }
+                let r = Rectangle::new(
+                    0,
+                    0,
+                    1024,
+                    768,
+                    Box::new(RawEncoding::new(pixels)),
+                );
+                FramebufferUpdate::new(vec![r])


It seems like the framebuffer-not-yet-initialized behavior, complete with white screen (or whatever Please Stand By graphics we want) should probably be centralized?

Took a gander at doing this in ddc78a6.

Ooops, I missed one place where the width/height was hardcoded. Let me fix in a follow up.

Finished this in dbb4f6a

pfmooney · 2022-04-13T17:11:00Z

server/src/lib/vnc.rs

+
+#[async_trait]
+impl Server for PropolisVncServer {
+    async fn get_framebuffer_update(&self) -> FramebufferUpdate {


Long-term structure question: It looks like this is mostly a 'pull' model from the RFB crate? Seems like we might want at least some 'push' portions of it, like when the ramfb config is updated from inside the guest (potentially requiring an explicit change-of-res message in the proto).

Yes, that's right. I structured it that way because the RFC describes the protocol as client-driven, so the server isn't supposed to send messages that aren't requested (like framebuffer updates) after the initial handshake. That said, I think where we've landed is that we're going to want to control the client for product quality purposes, so that's definitely something we can do. (It also seems to be something that the clients I've tested with might handle already -- even before I wrote code to process client messages they seemed to accept unprompted framebuffer updates, but I would need to do more testing to be sure.)

pfmooney · 2022-04-13T17:12:41Z

server/src/main.rs

+        // XXX: vncviewer won't work without offering VncAuth, even though it doesn't ask to use
+        // it.


Can probably be a "durable" (sans XXX) comment, considering the value

Fair. I don't think I care too much about vncviewer as a client (it seems to be buggy, or at least behaves differently from others when processing framebuffer updates), but it has the nice quality of very verbose error messages, which has helped me debug protocol errors on my end. VNCAuth isn't implemented yet (and maybe never will be) so it probably should be removed in that case.

Addressed in ddc78a6

jordanhendricks requested a review from pfmooney April 12, 2022 02:26

pfmooney reviewed Apr 13, 2022

View reviewed changes

Add skeleton support for VNC server.

a57c3ca

jordanhendricks force-pushed the jhendricks/initial-vnc branch from dbb4f6a to a57c3ca Compare April 14, 2022 18:06

jordanhendricks merged commit ac50018 into master Apr 14, 2022

jordanhendricks deleted the jhendricks/initial-vnc branch April 14, 2022 18:09

jordanhendricks mentioned this pull request Apr 15, 2022

Fix failing integration test due to changed server::Context constructor. #115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skeleton support for VNC server. #113

Add skeleton support for VNC server. #113

jordanhendricks commented Apr 12, 2022 •

edited

Loading

pfmooney left a comment

pfmooney Apr 13, 2022

jordanhendricks Apr 13, 2022

pfmooney Apr 13, 2022

jordanhendricks Apr 13, 2022

jordanhendricks Apr 13, 2022

jordanhendricks Apr 13, 2022

pfmooney Apr 13, 2022

jordanhendricks Apr 13, 2022

pfmooney Apr 13, 2022

jordanhendricks Apr 13, 2022

jordanhendricks Apr 13, 2022

		// XXX: vncviewer won't work without offering VncAuth, even though it doesn't ask to use
		// it.

Add skeleton support for VNC server. #113

Add skeleton support for VNC server. #113

Conversation

jordanhendricks commented Apr 12, 2022 • edited Loading

Summary

Overview of Changes

Examples

Example Uninitialized Framebuffer (guest not started)

Example Initialized Framebuffer (guest started)

pfmooney 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

jordanhendricks commented Apr 12, 2022 •

edited

Loading