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: flatten InstanceSpecV0 #767

Merged
merged 7 commits into from
Oct 1, 2024

Conversation

gjcolombo
Copy link
Contributor

(9/9 in the instance spec rework; see #735.)

Finally, the point of all the prior refactoring PRs!

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.

Closes #735.

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 good to me --- I did try to search for specific nits to pick and didn't really find any. It's a bit of a shame to do more of the instance-spec validation at runtime, but I agree that it's the right path forward given everything you wrote in the commit message.

Comment on lines +186 to +196
ComponentV0::CrucibleStorageBackend(_)
| ComponentV0::FileStorageBackend(_)
| ComponentV0::BlobStorageBackend(_) => {
Copy link
Member

Choose a reason for hiding this comment

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

i kinda wondered if it was worth having functions like ComponentV0::is_storage_backend() -> bool but, upon further consideration, we would only ever call them in one place, so probably not...

@gjcolombo
Copy link
Contributor Author

Thanks for the review, @hawkw! I'm going to hold this until #756 lands to save us from having to spend time rebasing that change onto this one.

@gjcolombo gjcolombo force-pushed the gjcolombo/instance-spec-rework-9 branch from 7ef5e19 to 0bdabd8 Compare September 30, 2024 21:06
@gjcolombo
Copy link
Contributor Author

In addition to failing CI, my resolution of the conflicts from my force-push (to rebase on top of #756) is not as clean as it probably could be. I'll push another commit or two soon to clean things up.

Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

i forgot how much nicer components were going to be once #756 landed! two minor quibbles about boot settings though :)

bin/propolis-server/src/lib/spec/builder.rs Outdated Show resolved Hide resolved
@gjcolombo gjcolombo merged commit f255595 into master Oct 1, 2024
10 of 11 checks passed
@gjcolombo gjcolombo deleted the gjcolombo/instance-spec-rework-9 branch October 1, 2024 03:35
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.

instance specs could be improved
3 participants