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

Expose propolis' serial console channel in nexus #1766

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

lifning
Copy link
Contributor

@lifning lifning commented Sep 30, 2022

  • Includes updates to progenitor and reqwest 0.11.12 for client support for Dropshot websocket channels.
  • Literally only passes through the websocket, does not yet include the serial output buffering done by its parent GET endpoint. (This will be moved from sled-agent to propolis-server in forthcoming PRs)
  • Begins migrating sled-agent's propolis-client usage to the new progenitor-generated version (Dropshot-based websocket endpoint & Add Progenitor-based client propolis#206)
  • Adds support for ensuring a websocket endpoint gets upgraded in nexus integration tests (i.e. test_unauthorized).

I'll update how-to-run.adoc after either oxidecomputer/cli#268 lands (or if we implement it in oxide-sdk-rust instead)

Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet, thanks lif! a couple small comments, but i'm inclined to land things so we can finish the handmade->generated client migration

/// Connect to an instance's serial console
#[channel {
protocol = WEBSOCKETS,
path = "/organizations/{organization_name}/projects/{project_name}/instances/{instance_name}/serial-console/stream",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea we'll have both the streaming and non-streaming endpoint in the limit? I assume the latter for the buffer playback behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the streaming endpoint will eventually get params to start from an arbitrary byte offset (within the buffer) as well once i've moved the longer buffering into propolis-server, at which point the main reasons to keep the non-streaming one around would be

  • wanting to separate out permission to read it vs. read/write
  • wanting to provide an API for accessing it that doesn't require websocket support

}
}
}
// pass along Close messages
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also try to gracefully send a close to the other side if one unexpectedly drops?

sled-agent/Cargo.toml Outdated Show resolved Hide resolved
sled-agent/Cargo.toml Outdated Show resolved Hide resolved
@@ -25,7 +25,7 @@ use uuid::Uuid;

use super::sled_agent::SledAgent;

use propolis_client::api::VolumeConstructionRequest;
use crucible_client_types::VolumeConstructionRequest;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the generated client not re-export types it uses?

Copy link
Contributor Author

@lifning lifning Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of a rough edge i think: progenitor re-declares a copy of them based on the openapi spec it's given (which is of course generated using the derive(JsonSchema) on the original crucible_client_types types). unless there's a fancy feature of progenitor i'm missing, this means we end up needing boilerplate to convert between the two

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right i know you added the From impls for the generated-migration feature but I wonder if its worth just having in general too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i get the distinct feeling it might be, yeah.

- Includes updates to progenitor and reqwest 0.11.12 for client support
  for Dropshot websocket channels.
- Literally only passes through the websocket, does not yet include the
  serial output buffering done by its parent GET endpoint.
- Adds support for ensuring a websocket endpoint gets upgraded in nexus
  integration tests (i.e. test_unauthorized).
- Begins migrating sled-agent's propolis-client usage to the new
  progenitor-generated version (oxidecomputer/propolis#206)
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.

None yet

2 participants