[api] reorganize per RFD 619 by sunshowers · Pull Request #996 · oxidecomputer/propolis

sunshowers · 2025-12-22T22:08:07Z

In RFD 619, we've decided on a new way to organize published types; this PR makes the Propolis API conform to the RFD.

Created using spr 1.3.6-beta.1

sunshowers · 2025-12-23T23:33:33Z

bin/propolis-server/src/lib/server.rs

-    async fn v0_instance_ensure(
-        rqctx: RequestContext<Self::Context>,
-        request: TypedBody<api::v0::InstanceEnsureRequestV0>,
-    ) -> Result<HttpResponseCreated<api::InstanceEnsureResponse>, HttpError>
-    {


This code was replaced with a simpler From conversion.

sunshowers · 2025-12-23T23:34:03Z

bin/propolis-server/src/lib/vm/mod.rs

-    ///
-    /// - `Some` if the VM has been created.
-    /// - `None` if no VM has ever been created.
-    pub(super) async fn v0_get(&self) -> Option<InstanceSpecGetResponseV0> {


This code becomes unnecessary now that the response type has a From implementation.

Created using spr 1.3.6-beta.1

hawkw · 2026-03-02T21:33:11Z

bin/propolis-server/src/lib/spec/mod.rs

    fn try_from(value: InstanceSpec) -> Result<Self, Self::Error> {
        let InstanceSpec { board, components, smbios } = value;
-        let v0 = InstanceSpecV0 { board, components };
+        let v0 = v1::instance_spec::InstanceSpec { board, components };


the string let v0 = v1::instance_spec::InstanceSpec sure feels goofy, i wonder if we want to rename the variable instead...

Yeah probably!

Created using spr 1.3.6-beta.1

iximeow

thank you for dragging propolis-server's HTTP API into the future along with everyone else :) again, sorry for how long this has been sitting open.

I do want to say I found this surprisingly difficult to review. I know it would have been much more straightforward for me to see something like a commit that was just moving hunks of code around, and (at least one) separate commit that got into the renaming. as-is, my review approach ended up being to grab the full diff, match up blocks of removed code with where they moved to (and so you can see why the internal reordering was surprising), noticing several places that were not just motion, removing the compared parts from the diff, and repeating until fixed-point.

this is a skill issue on my part but helper functions going away because of new From etc: great! but also which from didn't really stand out to me until I'd cleared out the other motion. in retrospect I should have looked for new From on the relevant types, but I don't think that occurred to me because I didn't know which impls were new vs moved.

particularly renaming ComponentV0 surprised me, but I trust you know what's OK to do with the API better than me. basically the rest of my comments are really questions about process and what's consistent

crates/propolis-api-types-versions/Cargo.toml

iximeow · 2026-03-04T16:14:34Z

crates/propolis-api-types-versions/src/initial/instance_spec.rs

+    content = "component",
+    rename_all = "snake_case"
+)]
+#[schemars(rename = "ComponentV0")]


I'm kinda surprised by the rename here - I think this is OK, and clearly this makes for an identical openapi spec, but it seems really sketchy to introduce a rename like this on an existing API type. am I thinking about this wrong?

I think I would have expected this rename as a use initial::instance_spec::ComponentV0 as Component; to keep it as part of this crate and its dependents rather than introducing a difference between serde's identifier and schemars' identifier for this type.

other cases I saw in Omicron seem to be more for papering over awkward generics as part of public API type names, nothing that struck me as comparable, so I couldn't really find a prior art to help think through this (unless I didn't look hard enough!).

To be honest I didn't even realize serde had names for types like this. Per serde-rs/serde#2004 type names aren't serialized to JSON -- but they are serialized to the JSON schema.

I can switch this to #[serde(rename = "ComponentV0")] if you prefer; that shouldn't result in any functional changes either way.

I would like to drop V0 from the Rust identifier name, though, because something like v1::ComponentV0 is pretty confusing overall.

yeah totally, agreed re. v1::ComponentV0 being confusing. type names not being serialized to json is only true if all the serializer/deserializers are derived (or manual implementations that uphold the fact) - I'm only familiar with this because I have written ser/des that violate it 😁

having said that, and not knowing of any manual impls of either trait around here, I'm also convinced this should be OK. not worried about #[schemars(..)] vs #[serde(..)] here (though we're on an old enough schemars at this point that it was kind of a journey to realize that the serde attributes are valid and carry the same meaning here)

Yeah -- note that schemars boostrapping itself off of serde is still true as of schemars v1.

crates/propolis-api-types-versions/src/initial/mod.rs

iximeow · 2026-03-04T16:40:17Z

crates/propolis-api-types/src/instance_spec/v0.rs

-pub enum InstanceSpecStatusV0 {
-    WaitingForMigrationSource,
-    Present(VersionedInstanceSpec),
-}


this obviously doesn't matter but I'm curious mostly as a matter of process or if you'd noticed: InstanceSpecGetResponse and InstanceSpecStatus swapped order in the move (https://github.com/oxidecomputer/propolis/pull/996/changes#diff-b1f50f7461d361705de65356da1eb3bda031195a3dfa017def012f9796dc1420R180-R192). this happened several times in the diff and, again, I'm not worried about the order of structures or anything ridiculous like that. but it did make it confusing as I was trying to follow which blocks of code moved where, so I'm curious about where the reordering came from.

I ended up ordering it from lower-level type to higher-level type, which resulted in reordering in this and some other cases. Not something I'm wedded to at all -- just picked something reasonable here.

crates/propolis-api-types-versions/src/initial/instance.rs

crates/propolis-api-types-versions/src/programmable_smbios/mod.rs

iximeow · 2026-03-04T17:03:35Z

crates/propolis-api-types/src/lib.rs

-impl From<ReplacementComponent> for instance_spec::v0::ComponentV0 {
-    fn from(value: ReplacementComponent) -> Self {
-        match value {
-            ReplacementComponent::MigrationFailureInjector(c) => {
-                ComponentV0::MigrationFailureInjector(c)
-            }
-            ReplacementComponent::CrucibleStorageBackend(c) => {
-                ComponentV0::CrucibleStorageBackend(c)
-            }
-            ReplacementComponent::VirtioNetworkBackend(c) => {
-                ComponentV0::VirtioNetworkBackend(c)
-            }
-        }
-    }
-}


this seems to have just gone away in the move. it looks like it was dead code, so.. ok! but I was kinda surprised to see it and took a second to see if this is used in Omicron for example (I don't think so?)

Yeah, this appeared to be unused.

iximeow · 2026-03-04T17:07:16Z

crates/propolis-server-api/src/lib.rs

        rqctx: RequestContext<Self::Context>,
-    ) -> Result<HttpResponseOk<InstanceSpecGetResponse>, HttpError>;
+    ) -> Result<
+        HttpResponseOk<latest::instance_spec::InstanceSpecGetResponse>,


this kinda applies to all of the top-level names now: is it normal in our other APIs to write latest::foo::FooThingy? I know I personally kind of like latest::foo::Thingy, admitting the faults with that pattern, but especially with instance_spec::InstanceSpec* this seems to get a bit awkward to read.

if that's just the typical pattern across our HTTP APIs, I'm not going to sweat it - this is definitely where I defer strongly to consistency.

Yeah. This is definitely a little wordy but we've found the clarity to overall be helpful.

bin/propolis-server/src/lib/serial/history_buffer.rs

Created using spr 1.3.6-beta.1

sunshowers added 2 commits December 22, 2025 22:08

[spr] initial version

c41b396

Created using spr 1.3.6-beta.1

update clients

800b05e

Created using spr 1.3.6-beta.1

sunshowers mentioned this pull request Dec 23, 2025

Migrate versioned types crates to RFD 619 scheme oxidecomputer/omicron#9526

Open

25 tasks

more fixups

936bd60

Created using spr 1.3.6-beta.1

sunshowers requested a review from iximeow December 23, 2025 23:33

sunshowers commented Dec 23, 2025

View reviewed changes

sunshowers added 2 commits January 21, 2026 02:12

rebase on main

9631e41

Created using spr 1.3.6-beta.1

rebase on main, restore comment

8d2e676

Created using spr 1.3.6-beta.1

hawkw reviewed Mar 2, 2026

View reviewed changes

rebase on main, review feedback

6f08518

Created using spr 1.3.6-beta.1

iximeow approved these changes Mar 4, 2026

View reviewed changes

sunshowers added 2 commits March 4, 2026 13:00

rebase on main, address most review comments

ee036b5

Created using spr 1.3.6-beta.1

rustfmt

726399b

Created using spr 1.3.6-beta.1

sunshowers merged commit 8967fe2 into master Mar 4, 2026
5 checks passed

sunshowers deleted the sunshowers/spr/api-reorganize-per-rfd-619 branch March 4, 2026 21:15

Conversation

sunshowers commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iximeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunshowers Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sunshowers commented Dec 22, 2025 •

edited

Loading

sunshowers Mar 4, 2026 •

edited

Loading