api: one ensure API to rule them all #813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hawkw
approved these changes
Nov 20, 2024
gjcolombo
force-pushed
the
gjcolombo/death-of-a-config-file
branch
from
986264a to
8c0f774
Compare
gjcolombo
force-pushed
the
gjcolombo/one-ensure-api
branch
from
91023e4 to
d8967c7
Compare
gjcolombo
force-pushed
the
gjcolombo/death-of-a-config-file
branch
from
8c0f774 to
fb4b081
Compare
Remove the existing instance ensure APIs and collapse them into a single API that takes a set of instance properties and a mechanism for creating the instance, which can be either creating a new VM from a spec or initializing as a live migration target. In the latter case, the client can also supply a list of instance spec components that the client wishes to override the components in the source's spec. This gives Omicron a way to replace backend information (Crucible generation number, host VNIC names) that changes over live migration. Remove all of the instance ensure types that supported the old ensure interface. One notable consequence of this is that the `DiskRequest` type also caused propolis-server's versions of Crucible's `VolumeConstructionRequest` and `CrucibleOpts` types to appear in the generated client. Omicron code tends to pull these types in from propolis-client (often transitively through sled-agent-client). This is a useful way of making sure Omicron components serialize/deserialize VCRs the same way Propolis does, so preserve it by re-exporting these types from propolis-client. Remove the parts of the mock server that validated disk, NIC, and cloud-init volume requests. The Omicron tests that use the mock server primarily check that its serial console interface works as expected and don't heavily exercise these checks. (Omicron CI's end-to-end test will fail if sled-agent mishandles the Propolis ensure API.) Finally, remove a few other unused types from the propolis_api_types crate. Tests: cargo test, PHD, driving ad hoc VMs with propolis-cli.
gjcolombo
force-pushed
the
gjcolombo/one-ensure-api
branch
from
a92ffd7 to
7a9bbc0
Compare
hawkw
approved these changes
Dec 18, 2024
crates/propolis-api-types/src/lib.rs
Outdated
Comment on lines
64
to
73
| /// - Crucible disks: After migrating, the target Propolis presents itself as a | ||
| /// new client of the Crucible downstairs servers backing the VM's disks. | ||
| /// Crucible requires the target to present a newer client generation number | ||
| /// to allow the target to connect. In a full Oxide deployment, these numbers | ||
| /// are managed by the control plane (i.e. it is not safe for Propolis to | ||
| /// manage these values directly--new Crucible volume connection information | ||
| /// must always come from Nexus). | ||
| /// - Virtio network devices: Each virtio NIC in the guest needs to bind to a | ||
| /// named VNIC object on the host. These names can change when a VM migrates | ||
| /// from host to host. |
gjcolombo
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Dec 20, 2024
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 `VolumeConstructionRequest` and `CrucibleOpts` types no longer appear in Propolis's generated client (and so don't appear in sled agent's client either). Instead, the `propolis-client` crate re-exports these types directly from its `crucible-client-types` dependency. For the most part, this involves updating `use` directives and storing `SocketAddr`s 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stacked on #810. Part of #804.
Remove the existing instance ensure APIs and collapse them into a single API that takes a set of instance properties and a mechanism for creating the instance, which can be either creating a new VM from a spec or initializing as a live migration target. In the latter case, the client can also supply a list of instance spec components that the client wishes to override the components in the source's spec. This gives Omicron a way to replace backend information (Crucible generation number, host VNIC names) that changes over live migration.
Remove all of the instance ensure types that supported the old ensure interface. One notable consequence of this is that the
DiskRequesttype also caused propolis-server's versions of Crucible'sVolumeConstructionRequestandCrucibleOptstypes to appear in the generated client. Omicron code tends to pull these types in from propolis-client (often transitively through sled-agent-client). This is a useful way of making sure Omicron components serialize/deserialize VCRs the same way Propolis does, so preserve it by re-exporting these types from propolis-client.Remove the parts of the mock server that validated disk, NIC, and cloud-init volume requests. The Omicron tests that use the mock server primarily check that its serial console interface works as expected and don't heavily exercise these checks. (Omicron CI's end-to-end test will fail if sled-agent mishandles the Propolis ensure API.)
Finally, remove a few other unused types from the propolis_api_types crate.
Tests: cargo test, PHD, driving ad hoc VMs with propolis-cli.