Put type specific information into DiskType #9543

jmpesp · 2025-12-18T21:28:24Z

Some existing fields of Disk only make sense for Distributed disks, so move those under that variant of DiskType.

Add sled_id to expose what sled a Local disk is allocated to, so that guests can act on that.

Also removes device_path, as it is 100% wrong.

Some existing fields of `Disk` only make sense for `Distributed` disks, so move those under that variant of `DiskType`. Add `sled_id` to expose what sled a `Local` disk is allocated to, so that guests can act on that. Also removes `device_path`, as it is 100% wrong.

david-crespo · 2025-12-18T21:51:10Z

common/src/api/external/mod.rs

+        /// allocated. Once allocated it cannot be changed or migrated.
+        #[schemars(with = "Option<Uuid>")]
+        sled_id: Option<SledUuid>,
+    },


Need tag = "type" in the serde annotation to make it an internally tagged union instead of the default externally tagged. Instead of getting {"type": "distributed", ... } OR { "type": "local", ... } it's getting { distributed: { ... } OR { local: { ... } }.

https://github.com/jmpesp/omicron/blob/16be32fe12a5c5923f3668ab2244e10ee71b518a/openapi/nexus/nexus-2025121800.0.0-97ef76.json#L18803-L18852

👍 done in 8f96790

Looks good!

The downside here is you're now looking at disk.disk_type.type. I tried getting it to be a top level disk_type with string value and then a details field whose shape depends on the value of disk_type, but you get the dropshot complex schema error. I wonder at what point it becomes worth figuring that out so we can do this. Or maybe that's a horrible schema! I don't know.

diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 287f1c8e53..3879572af5 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1434,7 +1434,7 @@ // DISKS #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] -#[serde(tag = "type", rename_all = "snake_case")] +#[serde(tag = "disk_type", content = "details", rename_all = "snake_case")] pub enum DiskType { Distributed { /// ID of snapshot from which disk was created, if any @@ -1460,6 +1460,7 @@ pub size: ByteCount, pub block_size: ByteCount, pub state: DiskState, + #[serde(flatten)] pub disk_type: DiskType, }

Thinking about it more, I think disk_type and disk_type_details at top level is a pattern I could live with.

However, I can't really picture how the types for that would work in TypeScript (I thought TS wants a discriminator inside the disk_type_details object itself, like we have currently, but the below shows that's not true). It may not be something you can do elegantly in OpenAPI.

The most elegant thing I can come up with in TypeScript is something like

type Disk = { /* shared fields */ } & ( | { disk_type: "distributed", /* dist fields */ } | { disk_type: "local", /* local fields */ } )

Turns out TypeScript does actually handle this pretty well, but I'm not sure whether I could generate this code from the schema.

https://tsplay.dev/WP4qeW

And here it is with disk_type_details, which seems to work equally well:

https://tsplay.dev/NlraOW

@ahl would appreciate your opinion on whether disk.disk_type.type is as good as we're likely to get here for a reasonable amount of effort. I think that's likely. I tried to get the other things to work but they required custom JsonSchema impls and the resulting schema was pretty ugly anyway.

Thanks for catching this! We rely on a discriminator field in the Go SDK and not having it would have been a pain to work around.

ahl

good stuff

ahl · 2025-12-19T16:47:16Z

common/src/api/external/mod.rs

+        /// ID of the sled this local disk is allocated on, if it has been
+        /// allocated. Once allocated it cannot be changed or migrated.
+        #[schemars(with = "Option<Uuid>")]
+        sled_id: Option<SledUuid>,


What's the utility of this? I'm always loth to give users data that they can only misuse.

It's in there to have something to point to when users get errors trying to attach disks to instances, or start instances: if the attached local storage disks are on different sleds then it will never work. It's only on the view of Disk, it will never be something we accept as a parameter.

Nexus doesn't currently reject disk attachment requests that would create this situation, that's still work to do (#9520)

ahl · 2025-12-19T16:49:59Z

nexus/external-api/src/v2025112000.rs

+        // This path was fake before, so retain that behaviour here
+        let device_path = format!("/mnt/{}", new.identity.name.as_str());


why do we return fake data? if we're doing this change, can we remove this field if it's useless?

Unfortunately we have to maintain compatibility with the previous API versions, in case anyone has cached this result and will compare against it.

I'm not sure I follow. What would the consequence be if we were to remove this? I think we need to normalize breaking changes.

My understanding was that this is the conversion to the old shape of response, so this is precisely what normalizing breaking changes looks like in a versioned world.

ahl · 2025-12-19T16:59:34Z

common/src/api/external/mod.rs

    pub block_size: ByteCount,
    pub state: DiskState,
-    pub device_path: String,
    pub disk_type: DiskType,


perhaps disk_details or disk_info

Kind of generic but it's probably better than disk_type since it's not just a type. I like disk_details better than disk_info. What about disk_type_details to convey that they are details specific to the type?

I think "details" implies "specific to whatever the fuck this thing is", but I'm happy to let you have this one since I had the last one ;-)

Ideally this would be something like backend so you'd have disk.backend.type but we already have a conflicting DiskBackend type for the create path in the API.

I'm game for details or spec though to produce disk.details.type and disk.spec.type respectively.

There's more fields than just type on the variants and when you consider those something like disk.disk_type.image_id reads silly.

I don’t love backend anyway because while you can only have an image ID if you have the distributed backend, the image ID isn’t a property of the backend, it’s just metadata on the disk you can have when you use the distributed backend.

I think you’re right that disk_ is redundant, so details is probably best.

DiskDetails it is!

Hm, you know, I kind of like backend and backend.type. It could be made to work — could do DiskCreateBackend vs DiskBackend.

I'm game for that. It gives us some symmetry across the create and read paths.

ahl · 2025-12-19T16:59:51Z

nexus/db-queries/src/db/datastore/disk.rs

-                // XXX can we remove this?
-                let device_path = format!("/mnt/{}", disk.name().as_str());


david-crespo · 2025-12-19T19:11:25Z

For posterity: at 2025 Dec 19 Product Roundtable we were persuaded to punt on including the sled ID until we get user reactions to local disks and their attendant edge case errors.

jmpesp requested a review from david-crespo December 18, 2025 21:28

sudomateo mentioned this pull request Dec 18, 2025

Support local disk changes to DiskCreate oxidecomputer/oxide.go#352

Open

david-crespo reviewed Dec 18, 2025

View reviewed changes

use tag = "type"

8f96790

ahl reviewed Dec 19, 2025

View reviewed changes

AlejandroME added the local storage relating to the local storage feature label Dec 19, 2025

AlejandroME added this to the 18 milestone Dec 19, 2025

AlejandroME assigned jmpesp Dec 19, 2025

		// This path was fake before, so retain that behaviour here
		let device_path = format!("/mnt/{}", new.identity.name.as_str());

		// XXX can we remove this?
		let device_path = format!("/mnt/{}", disk.name().as_str());

Put type specific information into DiskType #9543

Are you sure you want to change the base?

Put type specific information into DiskType #9543

Conversation

jmpesp commented Dec 18, 2025

Uh oh!

david-crespo Dec 18, 2025 • 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!

david-crespo Dec 19, 2025 • 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!

david-crespo Dec 19, 2025 • 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!

ahl left a comment

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!

Choose a reason for hiding this comment

Uh oh!

david-crespo Dec 19, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

sudomateo Dec 20, 2025 • 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!

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!

david-crespo commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

david-crespo Dec 18, 2025 •

edited

Loading

david-crespo Dec 19, 2025 •

edited

Loading

david-crespo Dec 19, 2025 •

edited

Loading

david-crespo Dec 19, 2025 •

edited

Loading

sudomateo Dec 20, 2025 •

edited

Loading