untangle migration and instance spec versioning (+ configurable NVMe VWC bit)#1178
untangle migration and instance spec versioning (+ configurable NVMe VWC bit)#1178iximeow wants to merge 10 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has checked 415 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 314 | 1 | 100 | 0 |
Click to see the invalid file list
- crates/propolis-api-types-versions/src/nvme_write_cache/components/devices.rs
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
| // variants here, plus uses in `VersionedInstanceSpec::from_spec` and | ||
| // `VersionedInstanceSpec::into_amended_spec`. shrimple as that. | ||
| #[derive(Deserialize, Serialize, Debug)] | ||
| pub(crate) enum VersionedInstanceSpec { |
There was a problem hiding this comment.
this file and the functions on this enum are really the ones I'd like to hear from folks about, re. the intersection of API versioning and migration. if you are looking at this and are at all unsure how you'd add a new version, please shout.
| #[derive(Deserialize, Serialize, Debug)] | ||
| pub(crate) enum VersionedInstanceSpec { | ||
| V1(v1::instance_spec::InstanceSpec), | ||
| V2(v2::instance_spec::InstanceSpec), |
There was a problem hiding this comment.
(I need to figure out if v2 should or shouldn't be here. there's an InstanceSpec, so I think so, but my notes say that it maybe didn't end up in the API as such so we never actually have to express this rather than v1 or v3?)
| MigrateError::InstanceSpecsIncompatible(msg) | ||
| } | ||
|
|
||
| let amended_spec = match self { |
There was a problem hiding this comment.
this match is kind of atrocious and I don't like what the future holds for it. mayyyyybe we can get a way with a per-API-version amend() that takes the replacements list and does the needful per-InstanceSpec? that at least means this grows only by a few lines per version with each api_spec_*.rs locked in amber.
| } | ||
| } | ||
|
|
||
| impl TryFrom<Spec> for v3::instance_spec::InstanceSpec { |
There was a problem hiding this comment.
I don't love having so much duplicate glue for each rev of InstanceSpec/Component, but I think it's the least bad option.. open to ideas
1 participant
this is something of an unfortunate two-fer of a change, in that this gets #1136 in service of making #1170 straightforward. I'd started by wanting to plumb the volatile write cache bit out to the
propolis-serverAPI, but I really couldn't stomach playing games with that bit in aFromwhich really should be a lossless conversion, so this evolved into not having incorrectFrombetweenv1::instance_spec::InstanceSpecandpropolis-server'sSpec. at that point fixing instance versioning as handled by migration was trivial, so I did that here too.in-all I think these together help explain why there are some mighty funky-looking names and impls in here: changing the structure of an existing device definition pushes the types and conversions in a way that wasn't super obvious to me at the outset, so reviewing this minus the new
v6API would have been pretty confusing at least to me.along the way I've taken this as an excuse to write down a bunch of thoughts about this approach vs some alternatives in
bin/propolis-server/src/lib/migrate/types.rs.the whole structure here could use proptests around spec transformation actually being lossless. this doesn't get any of the testing bits out of the way in PHD (though now we can use new features in phd and migrate them successfully!). and this is still a draft because I've missed some obvious plumbing as it relates to getting
has_write_cachethroughpropolis-cliand into anInstanceSpec. but if anyone wants to take an early look, here it is!