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: move migration compat checks to propolis-server #766

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

gjcolombo
Copy link
Contributor

(Part 8/9 of the instance spec rework; see #735.)

Transfer Propolis's component-level live migration compatibility checks from the API types crate to propolis-server. This is the last big chunk of application code that lives in the api-types crate. This is not straight-up code movement because the internal Spec type doesn't have the same notion of "device" and "backend" collections that the wire type has. This means that the hierarchy of errors in the new propolis-server compat.rs is different (but hopefully simpler) than what was in the old propolis-api-types migration.rs. The actual compatibility rules should be the same as they were before, however, and I've tried to bring over wholesale the api-types crate's compatibility unit tests.

Also, adjust how compatibility checks work in the migration protocol. Before, the migration preamble sent a DeviceSpecV0 and a list of backend component names; the destination checked that the devices were compatible and that the backend name sets matched (but, owing to the absence of the actual backend descriptors, did not check that the backends themselves were migration-compatible). With this change, the preamble contains the source's entire InstanceSpecV0, though the compatibility checks are still (currently) limited to the VM's devices. This breaks the protocol's dependency on the notion that instance specs necessarily have collections of "devices" and "backends" as opposed to simply having a set of components. This is a prerequisite for the next PR in this series, which removes this distinction from the InstanceSpecV0 type itself.

This is a migration protocol-breaking change, so failures in the migrate-from-base tests are expected.

Tests: cargo test, PHD.

@gjcolombo gjcolombo requested review from hawkw and iximeow September 17, 2024 22:58
// 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doc comment doesn't need to be 500 words or anything but I could probably elaborate a little bit here

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 to me --- removing the distinction between "backends" and "devices" from the spec types is a nice simplification. I quite like the new compatibility errors, too: the messages seem a bit more helpful than the prior ones. I picked a few very minor nits but there's nothing terribly controversial here IMO.

bin/propolis-server/src/lib/migrate/compat.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/migrate/compat.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/migrate/compat.rs Show resolved Hide resolved
bin/propolis-server/src/lib/migrate/compat.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/migrate/compat.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/migrate/compat.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/spec/mod.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/migrate/compat.rs Outdated Show resolved Hide resolved
@gjcolombo gjcolombo merged commit 516dabe into master Sep 18, 2024
10 of 11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/instance-spec-rework-8 branch September 18, 2024 18:50
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.

2 participants