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

server: introduce a serial console backend #830

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

gjcolombo
Copy link
Contributor

Rework parts of propolis-server's serial console logic to achieve the following goals:

  • Make the serial console history buffer (which is transferred in the live migration protocol) use the versioned payload helpers supplied by the main Propolis library.
  • Provide the primitives necessary to implement the policies described in RFD 491: it should be possible to distinguish read-write and read-only clients of a serial console and apply different policies (around the number of clients and what happens if they fall behind when reading) to each of them.
  • Simplify the logic for dealing with multiple serial console clients to avoid cancellation safety issues.

To do this, split the existing serial console task into two layers. The ConsoleBackend layer deals with I/O to and from a Propolis character device. The SerialConsoleManager layer creates a task for each websocket connection to a serial device and registers it as a client of the relevant ConsoleBackend. The backend layer allows the manager to declare that specific clients have read-only access to the console and to decide what happens if a client becomes unable to accept new bytes from a client; the manager implements the policies that determine how many clients can exist concurrently and what backend policies apply to them.

Update the serial API endpoint to allow users to request a writable connection to the console. Add parameters to the serial console helper types in propolis-client that govern how this is set. This will require a small update to Nexus the next time Omicron picks up a Propolis build to add the extra parameter. End users don't see this aspect of the serial API (Nexus serves as a proxy for connections to VMs' serial consoles), so this should be a non-breaking change for control plane API users.

propolis-cli serial defaults to a writable connection to match previous behavior, but it now supports a --readonly flag to create read-only connections.

As currently written, this PR implements the "only one read-write client at a time" policy described in RFD 491. I can easily disable this part of the change if we don't want to do this yet (should just need to remove a few lines from the console manager's client-attach function).

Tests: cargo test, PHD, ad hoc testing with local propolis-server processes. This change changes the order of messages sent in the migration protocol and so will break the migrate-from-base tests; this remains OK for now since live migration isn't being used in production yet.

Fixes #650. Related to #614.

@gjcolombo gjcolombo requested review from lifning and pfmooney January 10, 2025 20:36
@hawkw hawkw self-requested a review January 16, 2025 21:04
bin/propolis-server/src/lib/serial/backend.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/backend.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/backend.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/backend.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/backend.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/mod.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/mod.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/serial/mod.rs Outdated Show resolved Hide resolved
@gjcolombo
Copy link
Contributor Author

Thanks as always for the thorough review, @hawkw! I'm going to rebase this as-is onto #834 and then will start tightening things up.

@gjcolombo gjcolombo force-pushed the gjcolombo/serial-console-task-manager branch from 7f2707e to 8f98714 Compare January 16, 2025 23:56
Instead of pushing all serial console websocket connections onto a
single task, create one task per websocket connection and add a serial
console backend that brokers traffic between the websocket tasks and the
underlying character device.

Each connection to the console backend has an independent handle that
has its own permissions (read-write or read-only) and rules for what to
do if a byte from the guest can't immediately be written to a client
task (block or disconnect the client). These tools are sufficient to
implement the serial console access policies described in RFD 491.

This change doesn't build on its own; additional work is needed to hook
the new backend up to the server APIs.
This plumbs the backend everywhere it needs to be, but import and export
aren't quite set up yet.
@gjcolombo gjcolombo force-pushed the gjcolombo/serial-console-task-manager branch from 8f98714 to 5b4e8c9 Compare January 17, 2025 18:10
@gjcolombo
Copy link
Contributor Author

@hawkw: I think I've addressed all your feedback in the newest round of commits. The new changes use tokio::io::simplex (thank you for suggesting this!), but this required some architectural adjustments to account for differences between a WriteHalf and an mpsc::Sender (the former isn't Clone, and writes won't return an error if the reader is dropped). For the most part, what this amounts to is that the ConsoleBackend's read task no longer reads the backend's client table directly; instead the backend feeds it a set of Reader objects, and readers and their clients share a watch so they can mutually shut down without having to hold references to each other.

There's a new theory statement in serial/backend.rs that tries to explain this a bit more. Let me know what you think.

@gjcolombo gjcolombo requested a review from hawkw January 22, 2025 01:23
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.

server serial task's send-to-listeners future is not cancel-safe
2 participants