Skip to content

Commit

Permalink
instance spec rework: flatten InstanceSpecV0 (#767)
Browse files Browse the repository at this point in the history
Remove the complex hierarchy of component types from InstanceSpecV0 and
instead turn V0 specs into a Board and a bag of Components. This makes it
much easier to define new component types flexibly:

- New components all go into the same collection: no more deciding if
  you've defined a device or a backend or deciding what to do if you have
  something in between.
- New components always appear in a map and so always have names (their
  keys). A bonus is that having only one component map prevents keys from
  being reused in multiple maps.
- Because new components don't need fields, there's no need to remember to
  tag component fields with serde's default or skip_serializing_if
  attributes (though this may still be needed for component fields).
- It's also much harder to get painted into a backwards-compatibility
  corner. Suppose you define a new component, add a field for it, and make
  the field an Option. If you later want to support having more than one of
  that component, you need to add a new spec field with higher maximum
  cardinality to avoid breaking old specs. Now you have two fields for the
  same kind of component. Yuck.

The downside of all this is that there are now more wire specs that type
check but that the server should reject; for example, you can now have a
wire spec with multiple panic devices. The previous changes in this series
mitigate this by converting wire specs into more rigorously organized
internal specs (which may enforce e.g. cardinality limits) before
propolis-server will actually use them.

Now that wire specs have a much simpler structure, the spec builder in the
propolis-client lib pulls very little weight, so remove it. Incoming wire
specs need to be validated on the server in any case, and two different
server versions may disagree on whether a particular wire spec is
acceptable (if one has features the other does not), so it's hard to have a
single builder that checks invariants for all relevant server versions.
Clients who want to have a mistake-catching/invariant-protecting builder
are, of course, still free to define their own (and those who might not
want one, like PHD, don't need to pull one in that they won't use).

Finally, remove the #[cfg(feature = "falcon")] guards from the api-types
crate. Servers built without Falcon support will reject specs that contain
Falcon components. The downside to this is that users of generated clients
who never intend to access a Falcon server (e.g. sled agent) will start to
see these types. The upside is that this means that propolis-server's
OpenAPI definition no longer varies with its feature set, which means we no
longer need propolis-server-falcon.json (which is easy to forget to update)
or the Falcon variant of the generated client (whose Progenitor directives
had to be manually kept in sync with the non-Falcon client).

This is (once again) a migration protocol-breaking change, so the
migrate-from-base tests are (once again) expected to fail.

Tests: cargo test, PHD.
  • Loading branch information
gjcolombo authored Oct 1, 2024
1 parent 7e2a3ca commit f255595
Show file tree
Hide file tree
Showing 26 changed files with 915 additions and 2,998 deletions.
2 changes: 1 addition & 1 deletion bin/propolis-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,4 @@ default = []
omicron-build = ["propolis/omicron-build"]

# Falcon builds require corresponding bits turned on in the dependency libs
falcon = ["propolis/falcon", "propolis_api_types/falcon"]
falcon = ["propolis/falcon"]
6 changes: 3 additions & 3 deletions bin/propolis-server/src/lib/initializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1009,9 +1009,9 @@ impl<'a> MachineInitializer<'a> {
info!(
self.log,
"Generating bootorder with order: {:?}",
self.spec.boot_order.as_ref()
self.spec.boot_settings.as_ref()
);
let Some(boot_names) = self.spec.boot_order.as_ref() else {
let Some(boot_names) = self.spec.boot_settings.as_ref() else {
return Ok(None);
};

Expand All @@ -1033,7 +1033,7 @@ impl<'a> MachineInitializer<'a> {
bdf
};

for boot_entry in boot_names.iter() {
for boot_entry in boot_names.order.iter() {
// Theoretically we could support booting from network devices by
// matching them here and adding their PCI paths, but exactly what
// would happen is ill-understood. So, only check disks here.
Expand Down
3 changes: 2 additions & 1 deletion bin/propolis-server/src/lib/migrate/compat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

//! Checks for compatibility of two instance specs.
//! Associated functions for the [`crate::spec::Spec`] type that determine
//! whether two specs describe migration-compatible VMs.
use std::collections::HashMap;

Expand Down
8 changes: 5 additions & 3 deletions bin/propolis-server/src/lib/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,11 @@ fn instance_spec_from_request(
}

if let Some(boot_settings) = request.boot_settings.as_ref() {
for item in boot_settings.order.iter() {
spec_builder.add_boot_option(item)?;
}
let order = boot_settings.order.clone();
spec_builder.add_boot_order(
"boot-settings".to_string(),
order.into_iter().map(Into::into),
)?;
}

if let Some(base64) = &request.cloud_init_bytes {
Expand Down
Loading

0 comments on commit f255595

Please sign in to comment.