oxidecomputer / oxidecomputer/propolis

propolis-server: migration needs to learn about spec versions

Open
#1,136 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

migration
Dominant language
Rust
Stars
270
Forks
42
Avg merge
4d 5h
Merged PRs (30d)
6

Description

I thought I had gotten these notes down, but I can't find them and it's not on the migration board! there are some relevant bits of lib/migrate to look at in the context of the HTTP API versioning work that has since landed:
https://github.com/oxidecomputer/propolis/blob/9343361f922d10c5825906a69c7bb05d139a3662/bin/propolis-server/src/lib/migrate/preamble.rs#L16-L53

and

https://github.com/oxidecomputer/propolis/blob/9343361f922d10c5825906a69c7bb05d139a3662/bin/propolis-server/src/lib/migrate/source.rs#L468-L473

these together mean that on the recipient side, propolis-server only knows how to accept a v1::VersionedInstanceSpec::V0 and that spec must be whatever you can get from converting the active VM's Spec down to an v1::InstanceSpec. in turn, this means that instance specs must be convertible to a v1::InstanceSpec. this all should be alarming, and maybe make your skin crawl.

ideally we can handle migration with an overall strategy like:

  • try converting the current VM's Spec to each API type, with conversions as TryFrom, starting from the latest version.
  • one of these should work; the spec came from the API at some point[1], so we should be able to convert back to one of those types.
  • send a migration Prelude with that converted spec and a version indicator, something like VersionedInstanceSpec was intended to handle.
  • ... and then handle device payloads and versioning thereof.
converting from a current Spec down to v1::InstanceSpec is lossy! we lose information in migrating out!

it is, and this is where the lossy conversion happens.

  • we should eliminate impl From<Spec> for v1::instance_spec::InstanceSpec.

in general we can't impl From<Spec> for [HTTP API Spec], because the API type is not guaranteed to be backwards compatible - it's possible we decide to remove a field from a device's configuration, for example.

migration does need an alternative to propolis_api_types::*::instance_spec::VersionedInstanceSpec

the big wrinkle with this being in propolis_api_types is that we want this type to be a sum of all versions' InstanceSpec. but that's problematic when a new API version would change the enum, and make different API versions' VersionedInstanceSpec incompatible with each other. as the comment there mentions, there is an endpoint to get the current instance's spec, which speaks in terms of this somewhat-doomed type:

https://github.com/oxidecomputer/propolis/blob/master/crates/propolis-api-types-versions/src/initial/instance_spec.rs#L151-L177

I believe this is only used for debugging at this point, but being able to get the current instance's spec is definitely valuable. we may want to keep something like VersionedInstanceSpec around for this API, and if so we need to:

  • note more explicitly that we'd expect a new VersionedInstanceSpec that is the sum of previous InstanceSpecs, any time we add a new latest InstanceSpec.

in either case we need a non-HTTP-API-shaped means to negotiate instance specs:

  • propolis-server migration needs an InstanceSpec container with version information that is forwards-compatible; the destination accepting new variants must not preclude accepting old shared variants.

I don't have great ideas for how we test this, other than that we should round-trip API specs through Spec and back and verify they are lossless. we should also be able to convert up and down (where lossless) and not lose instance spec information. we might want a selection of "old" instance specs that describe instance we expect to migrate in, and verify those do migrate in; I'm thinking a selection of externally-serialized data because this is all getting outside the OpenAPI versioning schema. so producing specs from the latest propolis-server opens the risk of a backwards-incompatible change happening which causes us to diverge from a real older instance spec.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with bin/propolis-server/src/lib/migrate/preamble.rs and source.rs, then inspect bin/propolis-server/src/lib/spec/api_spec_v0.rs and crates/propolis-api-types-versions/src/initial/instance_spec.rs. Trace the existing VersionedInstanceSpec and Spec conversions before deciding on the migration representation. Done means migration can negotiate versioned instance specs without lossy conversion, with round-trip and compatibility tests for older serialized specs.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, backend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.