ingest new Propolis VM creation API #7211
Conversation
There was a problem hiding this comment.
Making notes of a couple of things I saw while writing the PR description.
I also need to check that disk snapshots against a running instance work as intended, since that API also changed (I've tested this on the Propolis side but need to make sure it works here too).
gjcolombo
force-pushed
the
gjcolombo/ingest-propolis-ensure-api
branch
from
222ff66 to
dd376b0
Compare
I did this by spinning up a dev cluster, creating an instance and logging into it, touching some files there, and taking a snapshot of the image's disk while it was still running. The Propolis logs showed it servicing the snapshot request, and afterward I had a usable snapshot that I could use to create a new image/disk/instance that contains the files I'd touched pre-snapshot. |
gjcolombo
force-pushed
the
gjcolombo/ingest-propolis-ensure-api
branch
from
dd376b0 to
17e1ac0
Compare
|
Now that all the relevant Propolis changes have merged, I've rebased this to the latest main and updated the pointee Propolis commit to I also tested somewhat more rigorously that similarly-configured VMs have the same guest-visible topology and IDs before and after this change by spinning up dev clusters with and without the changes and doing the following:
Next I need to recheck disk snapshots and VCR replacement--if both of those work as intended I'll open this up for review. Detailed outputWith the changes: Without: |
|
I've also tested the following:
Looking into the integration test failures now. |
|
The most recent integration test failure (in |
gjcolombo
force-pushed
the
gjcolombo/ingest-propolis-ensure-api
branch
from
a01c51d to
4292e00
Compare
| @@ -152,9 +184,6 @@ pub struct VmmUnregisterResponse { | |||
| /// migration. | |||
| #[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] | |||
| pub struct InstanceMigrationTargetParams { | |||
| /// The Propolis ID of the migration source. | |||
| pub src_propolis_id: Uuid, | |||
There was a problem hiding this comment.
It seems like a mistake to lose this, from the perspective of wishing we had sent the expected UUID of the Downstairs we're connecting to from the Upstairs, and the bugs not having it has caused.
There was a problem hiding this comment.
Can you elaborate on which bugs these are?
I'm trying to think about what kinds of bugs this would prevent in this path. The main one I can think of is a sort of bug in which we intended to migrate from VMM A, but for some reason we passed VMM B's server address to the ensure request. It seems like it would be hard to do that up in Nexus given where all this information comes from.
I guess we could forestall this by having the source Propolis check that its ID matches the one the target supplied. This would still not prevent a different bug, though, in which an instance is currently running in VMM A, but somehow Nexus reads the VMM record for VMM B and passes its ID and address to this request--in that case the migration will be accepted but we'll still end up migrating from the wrong server.
In any event: I'm not sure I can address this directly in this PR, because the corresponding parameter is also gone from the Propolis API and migration protocol. But if it can effectively prevent a bug I'm all for adding it back.
There was a problem hiding this comment.
#6353 is a manifestation of the bug I'm talking about: an Upstairs constructed by a Pantry panicked when one of the regions it was connecting to did not match the others in the region set. The only information the Upstairs receives about Downstairs is SocketAddrs to connect to:
targets: [
addr1, // downstairs A
addr2, // downstairs B
addr3 // downstairs C
]
When the Crucible agent creates and destroys Downstairs, it will aggressively reuse what ports are assigned to new Downstairs. If Nexus destroys Downstairs C, then creates a new Downstairs D (without doing anything else with the Agent), then Downstairs D will bind to the same port that Downstairs C did. If that happens then the targets array that the Upstairs was sent is instantly invalid, and constructing it will either 1) lead to a panic because the Downstairs D region is a different size than A and B, or 2) not panic because Downstairs D is the same size as A and B, but it will be blank, leading to other issues.
Without any additional information in the targets array, the Upstairs has to panic: it would be invalid to continue and operate on regions with mismatched sizes. Adding the expected UUID would give the Upstairs the option to bail instead:
targets: [
{ uuidA, addr1 },
{ uuidB, addr2 },
{ uuidC, addr3 }
]
During the negotiation with the downstairs it would be able to compare the expected UUID in the targets array with what the Downstairs it's talking to is reporting, and fail to connect instead of panicking.
nexus/src/app/background/tasks/region_snapshot_replacement_step.rs
Outdated
Show resolved
Hide resolved
| @@ -152,9 +184,6 @@ pub struct VmmUnregisterResponse { | |||
| /// migration. | |||
| #[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] | |||
| pub struct InstanceMigrationTargetParams { | |||
| /// The Propolis ID of the migration source. | |||
| pub src_propolis_id: Uuid, | |||
There was a problem hiding this comment.
Can you elaborate on which bugs these are?
I'm trying to think about what kinds of bugs this would prevent in this path. The main one I can think of is a sort of bug in which we intended to migrate from VMM A, but for some reason we passed VMM B's server address to the ensure request. It seems like it would be hard to do that up in Nexus given where all this information comes from.
I guess we could forestall this by having the source Propolis check that its ID matches the one the target supplied. This would still not prevent a different bug, though, in which an instance is currently running in VMM A, but somehow Nexus reads the VMM record for VMM B and passes its ID and address to this request--in that case the migration will be accepted but we'll still end up migrating from the wrong server.
In any event: I'm not sure I can address this directly in this PR, because the corresponding parameter is also gone from the Propolis API and migration protocol. But if it can effectively prevent a bug I'm all for adding it back.
nexus/src/app/background/tasks/region_snapshot_replacement_step.rs
Outdated
Show resolved
Hide resolved
This ends up touching a substantial number of files because of the way Nexus accessed Crucible client types: - Nexus imported Crucible types from the generated sled agent client - Sled agent's code included generated Crucible types from the Propolis client - The Propolis client generates Crucible types because the Propolis server API used a DiskRequest type that referred to the Crucible client library's VolumeConstructionRequest and CrucibleOpts types The Propolis server API no longer has a DiskRequest type, so generated Propolis clients no longer include the Crucible types, and neither do the generated sled agent clients. Deriving Crucible types from the Propolis client is useful because it ensures that Nexus's conception of a VolumeConstructionRequest is the same as Propolis's. To maintain this property, the Propolis client depends on the crucible-client-types crate and re-exports types from it; the sled agent client then re-exports from the Propolis client library. The catch is that Progenitor-generated versions of these types use Strings for socket addresses, while the native types use native SocketAddrs. This requires a fair bit of munging in the tests to remove some conversions between strings and socket addresses.
gjcolombo
force-pushed
the
gjcolombo/ingest-propolis-ensure-api
branch
from
97d1e40 to
f0c692d
Compare
|
@iximeow Thanks for the review! I think I've addressed all your comments in f0c692d. I've also retested on a dev cluster that instance creation/start and disk snapshot seem to work (to try to gain some confidence that I didn't bungle any SocketAddr conversions or boot order designations). I'm planning to merge this if/when CI comes back green. |
Update Omicron to use the new Propolis VM creation API defined in oxidecomputer/propolis#813 and oxidecomputer/propolis#816. This API requires clients to pass instance specs to create new VMs and component replacement lists to migrate existing VMs. Construct these in sled agent for now; in the future this logic can move to Nexus and become part of a robust virtual platform abstraction. For now the goal is just to keep everything working for existing VMs while adapting to the new Propolis API.
Slightly adjust the sled agent instance APIs so that Nexus specifies disks and boot orderings using sled-agent-defined types and not re-exported Propolis types.
Finally, adapt Nexus to the fact that Crucible's
VolumeConstructionRequestandCrucibleOptstypes no longer appear in Propolis's generated client (and so don't appear in sled agent's client either). Instead, thepropolis-clientcrate re-exports these types directly from itscrucible-client-typesdependency. For the most part, this involves updatingusedirectives and storingSocketAddrs in their natively typed form instead of converting them to and from strings.Tests: cargo nextest, plus ad hoc testing in a dev cluster as described in the PR comments.