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

instance spec rework: remove most dependencies on InstanceSpecV0 from propolis-server #757

Merged
merged 11 commits into from
Sep 11, 2024

Conversation

gjcolombo
Copy link
Contributor

(Part 6/X in the instance spec rework; see #735.)

Define propolis-server's internal representation of a VM component manifest (the Spec type) and switch almost everyone in the server over to it. (The exception is the live migration preamble; this will be dealt with in a future PR that moves migration compatibility checks from api-types to server.)

  • Define the internal spec::Spec type and some helper types, like internal representations of a Disk and a Nic.
    • This PR starts to show some of the benefits of this series of changes (finally!): now a regular guest VNIC, which is only ever supposed to have a viona backend, is guaranteed by construction to have one, so the machine initializer doesn't need to handle the "oops wait you specified an invalid backend type here" case.
  • Add conversion functions to/from the InstanceSpecV0 public-API type. Converting to a versioned spec is infallible, but conversion from a versioned spec (which can be well-formed but not valid) is fallible.
  • Represent Falcon devices with a little more care:
    • The parsed SoftNPU device collection now uses Options for components (the main PCI device, the P9 device, and the P9 filesystem adapter) with a maximum cardinality of 1.
    • When enabling SoftNPU, explicitly denote in the instance spec that SoftNPU expects to use COM4 instead of excluding it from the spec entirely.

Known warts in this change that are on deck to be fixed later in this series:

  • In the new internal spec type, serial devices don't have names yet (they still use port numbers as a key and convert these to implicit names).
  • The new Disk and Nic types duplicate backend names: both the types themselves and their device_spec members contain backend_name fields, and these need to be identical or hilarity will ensue. Subsequent changes will remove the backend_name field from these types and just rely on the backend name that's in the device spec.

Tests: cargo test, PHD. (It probably wouldn't hurt for me to run this in an Omicron dev cluster, either. I can give that a try before merging.)

Implement conversions between InstanceSpecV0 and the internal spec type
and start converting uses of the former to the latter.

The notable exception is machine initialization, which still relies on
an InstanceSpecV0.
bin/propolis-server/src/lib/spec/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Overall, this looks great. I had some very minor nits, but none of them are major issues!

bin/propolis-server/src/lib/initializer.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/initializer.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/initializer.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/initializer.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/api_spec_v0.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/api_spec_v0.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/api_spec_v0.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/api_spec_v0.rs Outdated Show resolved Hide resolved
@gjcolombo gjcolombo merged commit eaef107 into master Sep 11, 2024
11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/instance-spec-rework-6 branch September 11, 2024 21:04
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