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 20 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.
| let v3_spec: v3::instance_spec::InstanceSpec = | ||
| val.try_into().unwrap_or_else(|e| { | ||
| unreachable!( | ||
| "Converting to Spec without v6 bits to v3 failed: {e}. \ | ||
| This is currently impossible. When Spec to \ | ||
| v6::instance_spec::InstanceSpec becomes fallible, \ | ||
| this should `?`." | ||
| ); | ||
| }); | ||
|
|
||
| let mut spec: v6::instance_spec::InstanceSpec = v3_spec.into(); | ||
|
|
||
| // Inserts a component entry into the supplied map, asserting first that | ||
| // the supplied key is not present in that map. | ||
| // | ||
| // This assertion is valid because internal instance specs should assign | ||
| // a unique name to each component they describe. The spec builder | ||
| // upholds this invariant at spec creation time. | ||
| #[track_caller] | ||
| fn insert_component( | ||
| spec: &mut v6::instance_spec::InstanceSpec, | ||
| key: SpecKey, | ||
| val: v6::instance_spec::Component, | ||
| ) { | ||
| assert!( | ||
| !spec.components.contains_key(&key), | ||
| "component name {} already exists in output spec", | ||
| &key | ||
| ); | ||
| spec.components.insert(key, val); | ||
| } |
There was a problem hiding this comment.
so.. InstanceSpec->Spec conversions work out kinda nicely with the approach of "remove whatever's new, convert it to the previous version, then in v1 produce a SpecBuilder we tack stuff onto in each revision".
this is me trying out the reverse for Spec->InstanceSpec and I'm really not sure how I feel about it. for v1, v2, and v3 this means the TryFrom<Spec> for vWhatever::instance_spec::InstanceSpec is legible, and I really like that! but for the latest version, where the conversion is realistically infallible, there's the really unsettling unreachable!() that will become reachable in a future v7 I'm sure.
there's also insert_component which I don't see a way to not have copies of for each version that touches components. since SpecBuilder is kinda the GCD of all instance specs we can just use it. but here we're editing some specific InstanceSpec with that spec's Component, so I think we end up having either this here, or
impl InstanceSpec {
fn insert_component(&mut self, key, component) -> blah blah
}on each version of InstanceSpec.
this is probably the lesser of the available evils? in any case this all needs proptests that the conversions are lossless in either direction, don't panic, etc. if I had that here I think I wouldn't care about this unreachable!(), because we'd just find it when adding new versions into proptests. maybe this is fine until then.
There was a problem hiding this comment.
Hum...I don't have much to add here other that saying that this read fine to me. We may need to wait for a v7 to see how it holds-up, but it seems OK for now.
lgfa29
left a comment
lgfa29
left a comment
There was a problem hiding this comment.
Some minor notes and comments that are mostly me trying to understand things, but from what I do understand, this seems like a nice implementation to a hairy problem 😄
| //! some format that a `propolis-server` of a different version can instantiate | ||
| //! an equivalent VM from, for device state everything else to be imported into. |
There was a problem hiding this comment.
for device state everything else to be imported into.
Minor note, but I think there might be word missing here? I didn't quite understand this sentence.
There was a problem hiding this comment.
yeah i've kind of mangled English here. hopefully this rephrasing is more legible?
| api_spec_v2::amend(&mut source_spec, replacements)?; | ||
|
|
||
| let amended_spec: Spec = | ||
| source_spec.try_into().map_err(|e: V1SpecError| { |
There was a problem hiding this comment.
| source_spec.try_into().map_err(|e: V1SpecError| { | |
| source_spec.try_into().map_err(|e: V2SpecError| { |
Minor thing, so feel free to ignore. I think these are the same, but maybe consider explicitly using the same version to keep things consistent? Or would using V2SpecError cause other problems down the line? At first, this kind of looked like a copy-paste error.
There was a problem hiding this comment.
so.. these could be the same, but they're different in ways that I think end up differently confusing. to be able to write this, in api_spec_v2.rs you'd have to pub use crate::api_spec_v1::ApiSpecError; so you can write use api_spec_v2::ApiSpecError as V2SpecError here. that means we've got the same type available through two different crates and I know if I were chasing down the types I'd think they were different until I've followed the re-imports t o discover it's all just V1SpecError.
otoh, if this was a copy-paste error, then to type check we'd have to have a TryFrom<Spec> for v2::instance_spec::InstanceSpec { type = V1SpecError; .. }. that's certainly something you could write, but at the least it would be a few places copy-paste errored together. and the actual impl would have to convert from V2 errors to V1. which since they're all the same that's fine here! but if it were a meaningful difference the type checker would have lots of reason to shoot down simple errors.
the bone I have to pick here, which might be part of why this read like a copy-paste error to you, is .try_into().map_err() doesn't tell you anything about what you're converting. it's all type inferenced glue. the other direction has explicit TryInto::<v1::instance_spec::InstanceSpec>::try_into() which at least helps me a lot..
I've taken a swing at moving this to vN_to_spec_builder() which does a lot of things for us at once: the error type is concrete rather than an associated type, so rustc doesn't need it explicitly written at all. the input spec type is part of the function name, so it's a lot clearer what we're coming from. and since it's Just A Function it's not interesting to annotate the returned amended_spec type, so I just get the builder, convert the error, and finish() into a spec.
| api_spec_v3::amend(&mut source_spec, replacements)?; | ||
|
|
||
| let v6_spec: v6::instance_spec::InstanceSpec = | ||
| source_spec.into(); |
There was a problem hiding this comment.
I haven't read the details on amending specs for each version yet, so maybe the answer is somewhere there, but it could be useful to have a quick blurb on why we convert a v3 to a v6.
| #[error("backend {0} not used by any device")] | ||
| BackendNotUsed(SpecKey), | ||
|
|
||
| #[error("spec contains v1-incompatible component: {0}")] |
There was a problem hiding this comment.
I may be misunderstanding this, but since this enum is shared across versions, would every error message read v1-incomplatible even if attempting to transform it to other versions?
There was a problem hiding this comment.
you understand right! I was wondering about this as I wrote it. I think it's not terrible but not wrong in a very literal sense. api_spec_v1 is the only place we're producing ApiSpecError::IncompatibleComponent, so whenever this happens it's true that the thing is v1-incompatible. the weirdest case is if you have a v6 NvmeDisk without a write cache. then that fails to convert to a v3 Component, which fails to convert to a v1 component.
if that is written out as a string it's something like: spec contains v1-incompatible component: cannot convert component to v3: NvmeDisk with has_write_cache=false cannot be downgraded. so.. we failed to convert the thing to v1, because we couldn't get it down to v3.
the guidance here, I think, is "if you need to produce an ApiSpecError::<anything>, define a new version of the enum". that happens to match the current use, where when the type name is written out it's just to say "I'm returning the error that other function returns" (except the v6->v3 conversion in api_spec_v3.rs)
| | v1::instance_spec::Component::DlpiNetworkBackend(_) => { | ||
| unreachable!("already filtered out backends") | ||
| } | ||
| crate::spec::api_spec_v3::v3_to_spec_builder(v3_spec) |
There was a problem hiding this comment.
This is mostly inferred from reading the code in isolation, so I may be missing hands-on experience like the compiler screaming at me. Feel free to ignore it if this doesn't make sense.
But it's not immediately obvious to me why we only go to v3 here. Looking at v3_to_spec_builder it says:
// Converting v3 to v6 is lossless so just do that and piggyback on the
// v6 `InstanceSpec->SpecBuilder`.So I guess that's the answer? But then I'm not sure if I would need to add a v7 conversion here if I added a new spec version.
I'm also not sure why we even need v3 here? Would this be an equivalent conversion?
pub(crate) fn v1_to_spec_builder(
value: v1::instance_spec::InstanceSpec,
) -> Result<SpecBuilder, ApiSpecError> {
let v2_spec: v2::instance_spec::InstanceSpec = value.into();
crate::spec::api_spec_v2::v2_to_spec_builder(v2_spec)
}And so the logic is effectively that each version just converts to the next, going up the chain until we get the latest version of a SpecBuilder.
There was a problem hiding this comment.
I'm also not sure why we even need v3 here? Would this be an equivalent conversion?
that's right. this goes to v3 because I'd initially not written in api_spec_v2.rs, because v2 was in a weird space of adding a new InstanceSpec type but not a new Component. so I'd thought maybe we didn't need an api_spec_v2.rs at all. I argued myself into adding it, but didn't fix this forward conversion.
so yeah, the logic is each version just goes to the next api_spec_v*.rs until one of the conversions is fallible, which would probably mean we've deprecated a setting/component/etc. we haven't done that yet, so it's all infallible going to newer versions.
There was a problem hiding this comment.
The way I understood this is that we have a chain of calls to convert specs backwards and forwards:
This is pretty neat, and makes sense to me, but there are two aspects that I found a little confusing when going through a mental exercise of "what would I need to change to add a v7".
The first one is ApiSpecError. Each version seems to have its own, which is actually reused from other versions. This is fine, but I don't think it actually represents what they intend to communicate?
Looking at their definitions, the only difference is that v1::ApiSpecError has IncompatibleComponent, which can only happen when going backwards in the conversion, and v6::ApiSpecError can only happen when going forwards.
If I were to add a v7, would I need to move v6::ApiSpecError to v7::ApiSpecError? Or would v6::ApiSpecError always stay there and be reused in v7?
Maybe the error types could be more generic and version independent, or maybe we don't actually need two different error types? In case we do need versioned errors, it may be useful to add some note on how they should evolve when new versions are added.
The second question I had was with regards to v6_to_spec_builder. Would a v7 need to copy this method over and update v6_to_spec_builder to call it?
If so, maybe we can take a note from the HTTP API versioning and have an api_spec_latest where this method is defined. And so adding a v7 means I just update the method in api_spec_latest, call v7_to_spec_builder from v6_to_spec_builder and vlatest_to_spec_builder from v7_to_spec_builder. Looking at the diagram, it would be like adding an anchor node at the end of the linked list with the latest spec conversion logic.
There was a problem hiding this comment.
If I were to add a v7, would I need to move v6::ApiSpecError to v7::ApiSpecError? Or would v6::ApiSpecError always stay there and be reused in v7?
ohhh.. yeah. you would probably move it and then use v1::ApiErrorSpec in v6. that super funky. I didn't want to commit to a generic "ApiSpecError" type because that would imply (from the types at least) that there could be a v6-incompatible component in a Spec. this probably works out much more legibly if there's a latest error type that doesn't know about incompatible components, and the v1 ApiErrorSpec which we use for later versions until we have some kind of novel error case in processing an InstanceSpec.
with regards to v6_to_spec_builder. Would a v7 need to copy this method over and update v6_to_spec_builder to call it?
currently, yes. I bet that moving all this into a "api_spec_latest.rs" makes it make much more sense. I'll give that a try!
| let v3_spec: v3::instance_spec::InstanceSpec = | ||
| val.try_into().unwrap_or_else(|e| { | ||
| unreachable!( | ||
| "Converting to Spec without v6 bits to v3 failed: {e}. \ | ||
| This is currently impossible. When Spec to \ | ||
| v6::instance_spec::InstanceSpec becomes fallible, \ | ||
| this should `?`." | ||
| ); | ||
| }); | ||
|
|
||
| let mut spec: v6::instance_spec::InstanceSpec = v3_spec.into(); | ||
|
|
||
| // Inserts a component entry into the supplied map, asserting first that | ||
| // the supplied key is not present in that map. | ||
| // | ||
| // This assertion is valid because internal instance specs should assign | ||
| // a unique name to each component they describe. The spec builder | ||
| // upholds this invariant at spec creation time. | ||
| #[track_caller] | ||
| fn insert_component( | ||
| spec: &mut v6::instance_spec::InstanceSpec, | ||
| key: SpecKey, | ||
| val: v6::instance_spec::Component, | ||
| ) { | ||
| assert!( | ||
| !spec.components.contains_key(&key), | ||
| "component name {} already exists in output spec", | ||
| &key | ||
| ); | ||
| spec.components.insert(key, val); | ||
| } |
There was a problem hiding this comment.
Hum...I don't have much to add here other that saying that this read fine to me. We may need to wait for a v7 to see how it holds-up, but it seems OK for now.
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
just started reading through this with the doc comment, here are some small notes while i look at the way i tactually works
| //! describe VMs" internal structure. Early in migration we must convert this to | ||
| //! some format that a `propolis-server` of a different version can instantiate | ||
| //! an equivalent VM from, for device state everything else to be imported into. | ||
| //! We *kind of* use API types here, and the rest of this section gets into why |
There was a problem hiding this comment.
i read this sentence and immediately thought to myself, "oh no..."
| //! and what one should consider in adding future versions. | ||
| //! | ||
| //! Even for VMs that have been migrated many times, `propolis-server` must | ||
| //! incarnate a VM that can be described by *some* HTTP API `InstanceSpec` |
There was a problem hiding this comment.
maybe
| //! incarnate a VM that can be described by *some* HTTP API `InstanceSpec` | |
| //! incarnate a VM that has been described by *some* HTTP API `InstanceSpec` |
to make it a little more obvious that what you're saying is that, "if you were able to ever start the VM, this means that you have sent a start request with its spec AT SOME POINT BETWEEN NOW AND THE BEGINNING OF THE UNIVERSE"
There was a problem hiding this comment.
yeah, I was being a little cute with the wording because one possibility here is you start a VM with settings in API version 6 that are also describable in terms of API version 1. there's no implication it was ever actually described in V1 terms until you discover it happens to be possible at migration time. that distinction isn't terribly important and I think has been reads much more naturally here. i might do a footnote or something.
| //! | ||
| //! Even for VMs that have been migrated many times, `propolis-server` must | ||
| //! incarnate a VM that can be described by *some* HTTP API `InstanceSpec` | ||
| //! version at some point in the past. We'll call this "oldest possible VM spec" |
There was a problem hiding this comment.
| //! Since we have to support HTTP API types as far back as `propolis-server`'s | ||
| //! import horizon, it's not much additional work to at least try supporting | ||
| //! migration across downgrades of `propolis-server`. If try converting to all | ||
| //! `v1, v2, v3 ..` forms of `InstanceSpec` in *ascending* order, the only time | ||
| //! conversion will fail to be downgradeable is if a VM has been created using | ||
| //! only-in-newest API language. This means that some VMs created using a | ||
| //! `latest::instance_spec::InstanceSpec` could end up with even `v1` types on | ||
| //! the wire for migration, but as long as `From/TryFrom` use is correct and | ||
| //! *not lossy*, that's fine! | ||
| //! | ||
| //! So, `VersionedInstanceSpec` is a container that is outside the HTTP API but | ||
| //! only contains OpenAPI-described API types. A destination `propolis-server` | ||
| //! is expected to gracefully reject new variants, and a source | ||
| //! `propolis-server` is expected to emit oldest-supported forms of instances. |
3 participants
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!