Skip to content

Put type specific information into DiskType #9543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jmpesp wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Put type specific information into DiskType #9543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16be32f
Put type specific information into DiskType
jmpesp Dec 18, 2025
8f96790
use tag = "type"
jmpesp Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions common/src/api/external/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1433,11 +1433,22 @@ impl SimpleIdentityOrName for AntiAffinityGroupMember {

// DISKS

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
#[serde(rename_all = "snake_case")]
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)]
#[serde(tag = "type", rename_all = "snake_case")]
pub enum DiskType {
Distributed,
Local,
Distributed {
/// ID of snapshot from which disk was created, if any
snapshot_id: Option<Uuid>,
/// ID of image from which disk was created, if any
image_id: Option<Uuid>,
},

Local {
/// 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>,
Comment on lines +1447 to +1450
Copy link
Contributor

@ahl ahl Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jmpesp jmpesp Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

},
Copy link
Contributor

@david-crespo david-crespo Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: { ... } }.

Image

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

Copy link
Contributor Author

@jmpesp jmpesp Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done in 8f96790

Copy link
Contributor

@david-crespo david-crespo Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

image

Copy link
Contributor

@david-crespo david-crespo Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
 }

Copy link
Contributor

@david-crespo david-crespo Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

https://tsplay.dev/NlraOW

image

Copy link
Contributor

@david-crespo david-crespo Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@sudomateo sudomateo Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// View of a Disk
Expand All @@ -1446,14 +1457,9 @@ pub struct Disk {
#[serde(flatten)]
pub identity: IdentityMetadata,
pub project_id: Uuid,
/// ID of snapshot from which disk was created, if any
pub snapshot_id: Option<Uuid>,
/// ID of image from which disk was created, if any
pub image_id: Option<Uuid>,
pub size: ByteCount,
pub block_size: ByteCount,
pub state: DiskState,
pub device_path: String,
pub disk_type: DiskType,
Copy link
Contributor

@ahl ahl Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps disk_details or disk_info

Copy link
Contributor

@david-crespo david-crespo Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ahl ahl Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Contributor

@sudomateo sudomateo Dec 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@david-crespo david-crespo Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sudomateo sudomateo Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DiskDetails it is!

Copy link
Contributor

@david-crespo david-crespo Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sudomateo sudomateo Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

Expand Down
38 changes: 16 additions & 22 deletions nexus/db-queries/src/db/datastore/disk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,40 +263,34 @@ impl Into<api::external::Disk> for Disk {
fn into(self) -> api::external::Disk {
match self {
Disk::Crucible(CrucibleDisk { disk, disk_type_crucible }) => {
// XXX can we remove this?
let device_path = format!("/mnt/{}", disk.name().as_str());
api::external::Disk {
identity: disk.identity(),
project_id: disk.project_id,
snapshot_id: disk_type_crucible.create_snapshot_id,
image_id: disk_type_crucible.create_image_id,
size: disk.size.into(),
block_size: disk.block_size.into(),
state: disk.state().into(),
device_path,
disk_type: api::external::DiskType::Distributed,
disk_type: api::external::DiskType::Distributed {
snapshot_id: disk_type_crucible.create_snapshot_id,
image_id: disk_type_crucible.create_image_id,
},
}
}

Disk::LocalStorage(LocalStorageDisk {
disk,
disk_type_local_storage: _,
local_storage_dataset_allocation: _,
}) => {
// XXX can we remove this?
let device_path = format!("/mnt/{}", disk.name().as_str());
Comment on lines -286 to -287
Copy link
Contributor

@ahl ahl Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAN WE???

api::external::Disk {
identity: disk.identity(),
project_id: disk.project_id,
snapshot_id: None,
image_id: None,
size: disk.size.into(),
block_size: disk.block_size.into(),
state: disk.state().into(),
device_path,
disk_type: api::external::DiskType::Local,
}
}
local_storage_dataset_allocation,
}) => api::external::Disk {
identity: disk.identity(),
project_id: disk.project_id,
size: disk.size.into(),
block_size: disk.block_size.into(),
state: disk.state().into(),
disk_type: api::external::DiskType::Local {
sled_id: local_storage_dataset_allocation
.map(|allocation| allocation.sled_id()),
},
},
}
}
}
Expand Down
169 changes: 163 additions & 6 deletions nexus/external-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use openapiv3::OpenAPI;
/// Copies of data types that changed between versions
mod v2025112000;
mod v2025120300;
mod v2025121200;

api_versions!([
// API versions are in the format YYYYMMDDNN.0.0, defined below as
Expand Down Expand Up @@ -65,6 +66,7 @@ api_versions!([
// | date-based version should be at the top of the list.
// v
// (next_yyyymmddnn, IDENT),
(2025121800, DISK_TYPE_DETAILS),
(2025121200, BGP_PEER_COLLISION_STATE),
(2025120300, LOCAL_STORAGE),
(2025112000, INITIAL),
Expand Down Expand Up @@ -1439,10 +1441,35 @@ pub trait NexusExternalApi {

/// List disks
#[endpoint {
operation_id = "disk_list",
method = GET,
path = "/v1/disks",
tags = ["disks"],
versions = VERSION_LOCAL_STORAGE..,
versions = VERSION_LOCAL_STORAGE..VERSION_DISK_TYPE_DETAILS,
}]
async fn v2025121200_disk_list(
rqctx: RequestContext<Self::Context>,
query_params: Query<PaginatedByNameOrId<params::ProjectSelector>>,
) -> Result<HttpResponseOk<ResultsPage<v2025121200::Disk>>, HttpError> {
match Self::disk_list(rqctx, query_params).await {
Ok(page) => {
let new_page = ResultsPage {
next_page: page.0.next_page,
items: page.0.items.into_iter().map(Into::into).collect(),
};

Ok(HttpResponseOk(new_page))
}
Err(e) => Err(e),
}
}

/// List disks
#[endpoint {
method = GET,
path = "/v1/disks",
tags = ["disks"],
versions = VERSION_DISK_TYPE_DETAILS..,
}]
async fn disk_list(
rqctx: RequestContext<Self::Context>,
Expand Down Expand Up @@ -1476,10 +1503,32 @@ pub trait NexusExternalApi {
// TODO-correctness See note about instance create. This should be async.
/// Create a disk
#[endpoint {
operation_id = "disk_create",
method = POST,
path = "/v1/disks",
tags = ["disks"],
versions = VERSION_LOCAL_STORAGE..,
versions = VERSION_LOCAL_STORAGE..VERSION_DISK_TYPE_DETAILS,
}]
async fn v2025121200_disk_create(
rqctx: RequestContext<Self::Context>,
query_params: Query<params::ProjectSelector>,
new_disk: TypedBody<params::DiskCreate>,
) -> Result<HttpResponseCreated<v2025121200::Disk>, HttpError> {
match Self::disk_create(rqctx, query_params, new_disk).await {
Ok(HttpResponseCreated(disk)) => {
Ok(HttpResponseCreated(disk.into()))
}
Err(e) => Err(e),
}
}

// TODO-correctness See note about instance create. This should be async.
/// Create a disk
#[endpoint {
method = POST,
path = "/v1/disks",
tags = ["disks"],
versions = VERSION_DISK_TYPE_DETAILS..,
}]
async fn disk_create(
rqctx: RequestContext<Self::Context>,
Expand Down Expand Up @@ -1508,10 +1557,29 @@ pub trait NexusExternalApi {

/// Fetch disk
#[endpoint {
operation_id = "disk_view",
method = GET,
path = "/v1/disks/{disk}",
tags = ["disks"],
versions = VERSION_LOCAL_STORAGE..,
versions = VERSION_LOCAL_STORAGE..VERSION_DISK_TYPE_DETAILS,
}]
async fn v2025121200_disk_view(
rqctx: RequestContext<Self::Context>,
path_params: Path<params::DiskPath>,
query_params: Query<params::OptionalProjectSelector>,
) -> Result<HttpResponseOk<v2025121200::Disk>, HttpError> {
match Self::disk_view(rqctx, path_params, query_params).await {
Ok(HttpResponseOk(disk)) => Ok(HttpResponseOk(disk.into())),
Err(e) => Err(e),
}
}

/// Fetch disk
#[endpoint {
method = GET,
path = "/v1/disks/{disk}",
tags = ["disks"],
versions = VERSION_DISK_TYPE_DETAILS..,
}]
async fn disk_view(
rqctx: RequestContext<Self::Context>,
Expand Down Expand Up @@ -1782,10 +1850,39 @@ pub trait NexusExternalApi {

/// List disks for instance
#[endpoint {
operation_id = "instance_disk_list",
method = GET,
path = "/v1/instances/{instance}/disks",
tags = ["instances"],
versions = VERSION_LOCAL_STORAGE..,
versions = VERSION_LOCAL_STORAGE..VERSION_DISK_TYPE_DETAILS,
}]
async fn v2025121200_instance_disk_list(
rqctx: RequestContext<Self::Context>,
query_params: Query<
PaginatedByNameOrId<params::OptionalProjectSelector>,
>,
path_params: Path<params::InstancePath>,
) -> Result<HttpResponseOk<ResultsPage<v2025121200::Disk>>, HttpError> {
match Self::instance_disk_list(rqctx, query_params, path_params).await {
Ok(page) => {
let page = ResultsPage {
next_page: page.0.next_page,
items: page.0.items.into_iter().map(Into::into).collect(),
};

Ok(HttpResponseOk(page))
}

Err(e) => Err(e),
}
}

/// List disks for instance
#[endpoint {
method = GET,
path = "/v1/instances/{instance}/disks",
tags = ["instances"],
versions = VERSION_DISK_TYPE_DETAILS..,
}]
async fn instance_disk_list(
rqctx: RequestContext<Self::Context>,
Expand Down Expand Up @@ -1827,10 +1924,40 @@ pub trait NexusExternalApi {

/// Attach disk to instance
#[endpoint {
operation_id = "instance_disk_attach",
method = POST,
path = "/v1/instances/{instance}/disks/attach",
tags = ["instances"],
versions = VERSION_LOCAL_STORAGE..,
versions = VERSION_LOCAL_STORAGE..VERSION_DISK_TYPE_DETAILS,
}]
async fn v2025121200_instance_disk_attach(
rqctx: RequestContext<Self::Context>,
path_params: Path<params::InstancePath>,
query_params: Query<params::OptionalProjectSelector>,
disk_to_attach: TypedBody<params::DiskPath>,
) -> Result<HttpResponseAccepted<v2025121200::Disk>, HttpError> {
match Self::instance_disk_attach(
rqctx,
path_params,
query_params,
disk_to_attach,
)
.await
{
Ok(HttpResponseAccepted(disk)) => {
Ok(HttpResponseAccepted(disk.into()))
}

Err(e) => Err(e),
}
}

/// Attach disk to instance
#[endpoint {
method = POST,
path = "/v1/instances/{instance}/disks/attach",
tags = ["instances"],
versions = VERSION_DISK_TYPE_DETAILS..,
}]
async fn instance_disk_attach(
rqctx: RequestContext<Self::Context>,
Expand Down Expand Up @@ -1871,10 +1998,40 @@ pub trait NexusExternalApi {

/// Detach disk from instance
#[endpoint {
operation_id = "instance_disk_detach",
method = POST,
path = "/v1/instances/{instance}/disks/detach",
tags = ["instances"],
versions = VERSION_LOCAL_STORAGE..,
versions = VERSION_LOCAL_STORAGE..VERSION_DISK_TYPE_DETAILS,
}]
async fn v2025121200_instance_disk_detach(
rqctx: RequestContext<Self::Context>,
path_params: Path<params::InstancePath>,
query_params: Query<params::OptionalProjectSelector>,
disk_to_detach: TypedBody<params::DiskPath>,
) -> Result<HttpResponseAccepted<v2025121200::Disk>, HttpError> {
match Self::instance_disk_detach(
rqctx,
path_params,
query_params,
disk_to_detach,
)
.await
{
Ok(HttpResponseAccepted(disk)) => {
Ok(HttpResponseAccepted(disk.into()))
}

Err(e) => Err(e),
}
}

/// Detach disk from instance
#[endpoint {
method = POST,
path = "/v1/instances/{instance}/disks/detach",
tags = ["instances"],
versions = VERSION_DISK_TYPE_DETAILS..,
}]
async fn instance_disk_detach(
rqctx: RequestContext<Self::Context>,
Expand Down
Loading
Loading