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

Add migration endpoints to propolis-server #69

Merged
merged 23 commits into from
Dec 1, 2021
Merged

Add migration endpoints to propolis-server #69

merged 23 commits into from
Dec 1, 2021

Conversation

luqmana
Copy link
Contributor

@luqmana luqmana commented Nov 16, 2021

This adds the endpoints described in RFD 71 along with a first pass for some of the setup.

New Endpoints:

  1. /instances/{src-uuid}/migrate/start — This is the endpoint called by the new (destination) propolis-server instance when its ready to begin the migration process. This request requires no body as we'll perform an HTTP upgrade to reuse the underlying TCP socket as a bidirectional channel to perform the migration over.
  2. /instances/{src-uuid}/migrate/status — This endpoint allows querying either the source or destination on the current progress of a migration.

Modified Endpoints:

  1. /instances/{dst-uuid} — This is the existing "ensure" endpoint, which has been extended with an optional blob describing a source VM from which a new VM will be populated.

) -> Result<(), MigrateError> {
info!(log, "Enter Migrate Task");

// TODO: actual migration protocol, for now just send some stuff back and forth
Copy link
Contributor Author

@luqmana luqmana Nov 16, 2021

Choose a reason for hiding this comment

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

cc @dancrossnyc this would be where we plug in the tokio codec Framed stuff to wrap this Upgraded object on the source side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool; thanks.

let mut conn = hyper::upgrade::on(res).await?;

// Good to go, ready to migrate from the source via `conn`
// TODO: wrap in a tokio codec::Framed or such
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dancrossnyc and here for the destination side.

@pfmooney
Copy link
Collaborator

I assume a5bdf81 is due to the stabilization of the asm stuff in the latest toolchain? Perhaps land that (now) on its own with a little note in README about the MSRV requirement?

@luqmana
Copy link
Contributor Author

luqmana commented Nov 16, 2021

I assume a5bdf81 is due to the stabilization of the asm stuff in the latest toolchain? Perhaps land that (now) on its own with a little note in README about the MSRV requirement?

Oh, no we already have a top-level rust-toolchain.toml which is set to nightly so that hasn't changed. I was just annoyed needed to change multiple other files while testing something earlier. But yup, I'll pull that outta this PR.

@luqmana luqmana force-pushed the live-migrate branch 2 times, most recently from 888fcf0 to c4047a3 Compare November 16, 2021 22:23
// Now, we spawn a new task to handle the actual migration over the upgraded socket
let task = tokio::spawn(async move {
let conn = match upgrade.await {
Ok(upgraded) => upgraded,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we perhaps call this conn or socket or something? I probably just need more caffeine, but I was thinking that this referred to the upgrade case for LM. :-D

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 already do have a conn there but I just added a comment to make it more clear. Let me know if that's enough.

@pfmooney
Copy link
Collaborator

pfmooney commented Dec 1, 2021

I see no reason why we should get this landed, provided it'll make it easier to integrate with the other pending bits for LM.

/instances/{dst-uuid} — This is the existing "ensure" endpoint, which has been extended with an optional blob describing a source VM from which a new VM will be populated.

An item for later follow-up, but we might end up wanting a separate endpoint for creating an instance for the purpose of "rehydration" during a migration. The expectations there might be different enough to split those cases up. shrug

Thanks for putting this all together.

@dancrossnyc
Copy link
Contributor

@pfmooney I think there was a typo in your comment, "I see no reason why we shouldn't get this landed" ?

@luqmana luqmana merged commit 8ce5aeb into master Dec 1, 2021
@pfmooney
Copy link
Collaborator

pfmooney commented Dec 1, 2021

@pfmooney I think there was a typo in your comment, "I see no reason why we shouldn't get this landed" ?

And this is why I shouldn't be up so late.

@dancrossnyc
Copy link
Contributor

And this is why I shouldn't be up so late.

:-) The meaning was clear from context, though.

@dancrossnyc dancrossnyc deleted the live-migrate branch December 1, 2021 16:30
luqmana added a commit to oxidecomputer/omicron that referenced this pull request Jan 25, 2022
Building on the live migration work in propolis (oxidecomputer/propolis#69), this adds the nexus endpoint to trigger a migration of an instance.
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.

3 participants