Skip to content

Commit e051ce7

Browse files
authored
[nexus] Accept DiskSource::{Image, Snapshot}s without read_only (#9766)
@ahl rightly [points out][1] that the common case for the `disk_create` APIs is to create read-write disks, and it seems like [kind of a drag][2] to require all calls to create a read-write disk from an image or snapshot to include a `"read_only": false` field in the JSON body. Thus, this commit adds `#[serde(default)]` so that disk source params _without_ a `"read_only"` are the same as `"read_only": false`. Unfortunately, we didn't catch this until _after_ #9731 merged, so fixing it the proper way requires a whole new API version. Since the conversion between the Rust params types in `nexus-types` is trivial, it might have been possible to bend the rules a bit here and just tweak the previously-generated API version to make the field nullable --- clients operating with the slightly-older version of that version would still always be accepted since they would _always_ send the field. But, bending the rules makes me scared I'm gonna get in trouble, so I did it the proper way, at the expense of a whole lot of code that does basically nothing. [1]: #9731 (review) [2]: https://github.com/oxidecomputer/oxide.rs/pull/1334/files#r2749779106
1 parent 30bae5d commit e051ce7

File tree

6 files changed

+31784
-5
lines changed

6 files changed

+31784
-5
lines changed

nexus/external-api/src/lib.rs

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ mod v2026011600;
4646
mod v2026012200;
4747
mod v2026012300;
4848
mod v2026013000;
49+
mod v2026013001;
4950

5051
#[cfg(test)]
5152
mod test_utils;
@@ -78,6 +79,7 @@ api_versions!([
7879
// | date-based version should be at the top of the list.
7980
// v
8081
// (next_yyyymmddnn, IDENT),
82+
(2026013100, READ_ONLY_DISKS_NULLABLE),
8183
(2026013001, READ_ONLY_DISKS),
8284
(2026013000, INSTANCES_EXTERNAL_SUBNETS),
8385
(2026012800, REMOVE_SUBNET_POOL_POOL_TYPE),
@@ -2309,7 +2311,23 @@ pub trait NexusExternalApi {
23092311
method = POST,
23102312
path = "/v1/disks",
23112313
tags = ["disks"],
2312-
versions = VERSION_READ_ONLY_DISKS..,
2314+
versions = VERSION_READ_ONLY_DISKS..VERSION_READ_ONLY_DISKS_NULLABLE,
2315+
}]
2316+
async fn v2026013001_disk_create(
2317+
rqctx: RequestContext<Self::Context>,
2318+
query_params: Query<params::ProjectSelector>,
2319+
new_disk: TypedBody<v2026013001::DiskCreate>,
2320+
) -> Result<HttpResponseCreated<Disk>, HttpError> {
2321+
Self::disk_create(rqctx, query_params, new_disk.map(Into::into)).await
2322+
}
2323+
2324+
// TODO-correctness See note about instance create. This should be async.
2325+
/// Create disk
2326+
#[endpoint {
2327+
method = POST,
2328+
path = "/v1/disks",
2329+
tags = ["disks"],
2330+
versions = VERSION_READ_ONLY_DISKS_NULLABLE..,
23132331
}]
23142332
async fn disk_create(
23152333
rqctx: RequestContext<Self::Context>,
@@ -2555,12 +2573,27 @@ pub trait NexusExternalApi {
25552573
.await
25562574
}
25572575

2576+
#[endpoint {
2577+
method = POST,
2578+
path = "/v1/instances",
2579+
tags = ["instances"],
2580+
versions = VERSION_READ_ONLY_DISKS..VERSION_READ_ONLY_DISKS_NULLABLE,
2581+
}]
2582+
async fn v2026013001_instance_create(
2583+
rqctx: RequestContext<Self::Context>,
2584+
query_params: Query<params::ProjectSelector>,
2585+
new_instance: TypedBody<v2026013001::InstanceCreate>,
2586+
) -> Result<HttpResponseCreated<Instance>, HttpError> {
2587+
Self::instance_create(rqctx, query_params, new_instance.map(Into::into))
2588+
.await
2589+
}
2590+
25582591
/// Create instance
25592592
#[endpoint {
25602593
method = POST,
25612594
path = "/v1/instances",
25622595
tags = ["instances"],
2563-
versions = VERSION_READ_ONLY_DISKS..,
2596+
versions = VERSION_READ_ONLY_DISKS_NULLABLE..,
25642597
}]
25652598
async fn instance_create(
25662599
rqctx: RequestContext<Self::Context>,

nexus/external-api/src/v2026013000.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
// License, v. 2.0. If a copy of the MPL was not distributed with this
33
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
44

5-
//! Types from API version 2026012800 (`REMOVE_SUBNET_POOL_POOL_TYPE`) that changed
6-
//! in version 2026013000 (`READ_ONLY_DISKS`).
5+
//! Types from API version 2026013000 (`REMOVE_SUBNET_POOL_POOL_TYPE`) that changed
6+
//! in version 2026013001 (`READ_ONLY_DISKS`).
77
88
use crate::v2025112000;
99
use api_identity::ObjectIdentity;
Lines changed: 279 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,279 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
//! Types from API version 2026013001 (`READ_ONLY_DISKS`) that changed
6+
//! in version 2026013100 (`READ_ONLY_DISKS_NULLABLE`).
7+
//!
8+
//! This is one of the silliest API versions so far (if not *the* silliest),
9+
//! since all the Rust structs are completely identical. However, making the API
10+
//! accept [`DiskSource::Snapshot`] and [`DiskSource::Image`] messages _without_
11+
//! a `read_only` field requires adding a `#[serde(default)]` attribute, which
12+
//! changes the generated OpenAPI document, and the easiest way to generate a
13+
//! new OpenAPI document is just to...copy and paste all the types again. Yay.
14+
15+
use nexus_types::external_api::params;
16+
use omicron_common::api::external;
17+
use omicron_common::api::external::ByteCount;
18+
use omicron_common::api::external::IdentityMetadataCreateParams;
19+
use schemars::JsonSchema;
20+
use serde::Deserialize;
21+
use serde::Serialize;
22+
use uuid::Uuid;
23+
24+
/// Create-time parameters for a `Disk`
25+
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
26+
pub struct DiskCreate {
27+
/// The common identifying metadata for the disk
28+
#[serde(flatten)]
29+
pub identity: IdentityMetadataCreateParams,
30+
31+
/// The source for this `Disk`'s blocks
32+
pub disk_backend: DiskBackend,
33+
34+
/// The total size of the Disk (in bytes)
35+
pub size: ByteCount,
36+
}
37+
38+
impl From<DiskCreate> for params::DiskCreate {
39+
fn from(old: DiskCreate) -> Self {
40+
let DiskCreate { identity, disk_backend, size } = old;
41+
params::DiskCreate { identity, disk_backend: disk_backend.into(), size }
42+
}
43+
}
44+
45+
/// The source of a `Disk`'s blocks
46+
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
47+
#[serde(tag = "type", rename_all = "snake_case")]
48+
pub enum DiskBackend {
49+
Local {},
50+
51+
Distributed {
52+
/// The initial source for this disk
53+
disk_source: DiskSource,
54+
},
55+
}
56+
57+
impl From<DiskBackend> for params::DiskBackend {
58+
fn from(old: DiskBackend) -> Self {
59+
match old {
60+
DiskBackend::Local {} => params::DiskBackend::Local {},
61+
DiskBackend::Distributed { disk_source } => {
62+
params::DiskBackend::Distributed {
63+
disk_source: disk_source.into(),
64+
}
65+
}
66+
}
67+
}
68+
}
69+
70+
/// Different sources for a Distributed Disk
71+
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
72+
#[serde(tag = "type", rename_all = "snake_case")]
73+
pub enum DiskSource {
74+
/// Create a blank disk
75+
Blank {
76+
/// size of blocks for this Disk. valid values are: 512, 2048, or 4096
77+
block_size: params::BlockSize,
78+
},
79+
80+
/// Create a disk from a disk snapshot
81+
Snapshot {
82+
snapshot_id: Uuid,
83+
/// If `true`, the disk created from this snapshot will be read-only.
84+
read_only: bool,
85+
},
86+
87+
/// Create a disk from an image
88+
Image {
89+
image_id: Uuid,
90+
/// If `true`, the disk created from this image will be read-only.
91+
read_only: bool,
92+
},
93+
94+
/// Create a blank disk that will accept bulk writes or pull blocks from an
95+
/// external source.
96+
ImportingBlocks { block_size: params::BlockSize },
97+
}
98+
99+
impl From<DiskSource> for params::DiskSource {
100+
fn from(old: DiskSource) -> Self {
101+
// This is the funny part: you'll note that all these types are
102+
// ~*EXACTLY THE SAME*~. I love API versioning!
103+
match old {
104+
DiskSource::Blank { block_size } => Self::Blank { block_size },
105+
DiskSource::Snapshot { snapshot_id, read_only } => {
106+
Self::Snapshot { snapshot_id, read_only }
107+
}
108+
DiskSource::Image { image_id, read_only } => {
109+
Self::Image { image_id, read_only }
110+
}
111+
DiskSource::ImportingBlocks { block_size } => {
112+
Self::ImportingBlocks { block_size }
113+
}
114+
}
115+
}
116+
}
117+
118+
/// Describe the instance's disks at creation time
119+
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
120+
#[serde(tag = "type", rename_all = "snake_case")]
121+
pub enum InstanceDiskAttachment {
122+
/// During instance creation, create and attach disks
123+
Create(DiskCreate),
124+
125+
/// During instance creation, attach this disk
126+
Attach(params::InstanceDiskAttach),
127+
}
128+
129+
impl From<InstanceDiskAttachment> for params::InstanceDiskAttachment {
130+
fn from(old: InstanceDiskAttachment) -> params::InstanceDiskAttachment {
131+
match old {
132+
InstanceDiskAttachment::Create(create) => {
133+
params::InstanceDiskAttachment::Create(create.into())
134+
}
135+
136+
InstanceDiskAttachment::Attach(attach) => {
137+
params::InstanceDiskAttachment::Attach(attach)
138+
}
139+
}
140+
}
141+
}
142+
143+
/// Create-time parameters for an `Instance`
144+
#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
145+
pub struct InstanceCreate {
146+
#[serde(flatten)]
147+
pub identity: external::IdentityMetadataCreateParams,
148+
/// The number of vCPUs to be allocated to the instance
149+
pub ncpus: external::InstanceCpuCount,
150+
/// The amount of RAM (in bytes) to be allocated to the instance
151+
pub memory: external::ByteCount,
152+
/// The hostname to be assigned to the instance
153+
pub hostname: external::Hostname,
154+
155+
/// User data for instance initialization systems (such as cloud-init).
156+
/// Must be a Base64-encoded string, as specified in RFC 4648 § 4 (+ and /
157+
/// characters with padding). Maximum 32 KiB unencoded data.
158+
// While serde happily accepts #[serde(with = "<mod>")] as a shorthand for
159+
// specifying `serialize_with` and `deserialize_with`, schemars requires the
160+
// argument to `with` to be a type rather than merely a path prefix (i.e. a
161+
// mod or type). It's admittedly a bit tricky for schemars to address;
162+
// unlike `serialize` or `deserialize`, `JsonSchema` requires several
163+
// functions working together. It's unfortunate that schemars has this
164+
// built-in incompatibility, exacerbated by its glacial rate of progress
165+
// and immunity to offers of help.
166+
#[serde(default, with = "params::UserData")]
167+
pub user_data: Vec<u8>,
168+
169+
/// The network interfaces to be created for this instance.
170+
#[serde(default)]
171+
pub network_interfaces: params::InstanceNetworkInterfaceAttachment,
172+
173+
/// The external IP addresses provided to this instance.
174+
///
175+
/// By default, all instances have outbound connectivity, but no inbound
176+
/// connectivity. These external addresses can be used to provide a fixed,
177+
/// known IP address for making inbound connections to the instance.
178+
// Delegates through v2025121200 → params::ExternalIpCreate
179+
#[serde(default)]
180+
pub external_ips: Vec<params::ExternalIpCreate>,
181+
182+
/// Multicast groups this instance should join at creation.
183+
///
184+
/// Groups can be specified by name, UUID, or IP address. Non-existent
185+
/// groups are created automatically.
186+
#[serde(default)]
187+
pub multicast_groups: Vec<params::MulticastGroupJoinSpec>,
188+
189+
/// A list of disks to be attached to the instance.
190+
///
191+
/// Disk attachments of type "create" will be created, while those of type
192+
/// "attach" must already exist.
193+
///
194+
/// The order of this list does not guarantee a boot order for the instance.
195+
/// Use the boot_disk attribute to specify a boot disk. When boot_disk is
196+
/// specified it will count against the disk attachment limit.
197+
#[serde(default)]
198+
pub disks: Vec<InstanceDiskAttachment>,
199+
200+
/// The disk the instance is configured to boot from.
201+
///
202+
/// This disk can either be attached if it already exists or created along
203+
/// with the instance.
204+
///
205+
/// Specifying a boot disk is optional but recommended to ensure predictable
206+
/// boot behavior. The boot disk can be set during instance creation or
207+
/// later if the instance is stopped. The boot disk counts against the disk
208+
/// attachment limit.
209+
///
210+
/// An instance that does not have a boot disk set will use the boot
211+
/// options specified in its UEFI settings, which are controlled by both the
212+
/// instance's UEFI firmware and the guest operating system. Boot options
213+
/// can change as disks are attached and detached, which may result in an
214+
/// instance that only boots to the EFI shell until a boot disk is set.
215+
#[serde(default)]
216+
pub boot_disk: Option<InstanceDiskAttachment>,
217+
218+
/// An allowlist of SSH public keys to be transferred to the instance via
219+
/// cloud-init during instance creation.
220+
///
221+
/// If not provided, all SSH public keys from the user's profile will be sent.
222+
/// If an empty list is provided, no public keys will be transmitted to the
223+
/// instance.
224+
pub ssh_public_keys: Option<Vec<external::NameOrId>>,
225+
226+
/// Should this instance be started upon creation; true by default.
227+
#[serde(default = "params::bool_true")]
228+
pub start: bool,
229+
230+
/// The auto-restart policy for this instance.
231+
///
232+
/// This policy determines whether the instance should be automatically
233+
/// restarted by the control plane on failure. If this is `null`, no
234+
/// auto-restart policy will be explicitly configured for this instance, and
235+
/// the control plane will select the default policy when determining
236+
/// whether the instance can be automatically restarted.
237+
///
238+
/// Currently, the global default auto-restart policy is "best-effort", so
239+
/// instances with `null` auto-restart policies will be automatically
240+
/// restarted. However, in the future, the default policy may be
241+
/// configurable through other mechanisms, such as on a per-project basis.
242+
/// In that case, any configured default policy will be used if this is
243+
/// `null`.
244+
#[serde(default)]
245+
pub auto_restart_policy: Option<external::InstanceAutoRestartPolicy>,
246+
247+
/// Anti-Affinity groups which this instance should be added.
248+
#[serde(default)]
249+
pub anti_affinity_groups: Vec<external::NameOrId>,
250+
251+
/// The CPU platform to be used for this instance. If this is `null`, the
252+
/// instance requires no particular CPU platform; when it is started the
253+
/// instance will have the most general CPU platform supported by the sled
254+
/// it is initially placed on.
255+
#[serde(default)]
256+
pub cpu_platform: Option<external::InstanceCpuPlatform>,
257+
}
258+
259+
impl From<InstanceCreate> for params::InstanceCreate {
260+
fn from(old: InstanceCreate) -> params::InstanceCreate {
261+
params::InstanceCreate {
262+
identity: old.identity,
263+
ncpus: old.ncpus,
264+
memory: old.memory,
265+
hostname: old.hostname,
266+
user_data: old.user_data,
267+
network_interfaces: old.network_interfaces,
268+
external_ips: old.external_ips,
269+
multicast_groups: old.multicast_groups,
270+
disks: old.disks.into_iter().map(Into::into).collect(),
271+
boot_disk: old.boot_disk.map(Into::into),
272+
ssh_public_keys: old.ssh_public_keys,
273+
start: old.start,
274+
auto_restart_policy: old.auto_restart_policy,
275+
anti_affinity_groups: old.anti_affinity_groups,
276+
cpu_platform: old.cpu_platform,
277+
}
278+
}
279+
}

nexus/types/src/external_api/params.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2364,13 +2364,15 @@ pub enum DiskSource {
23642364
Snapshot {
23652365
snapshot_id: Uuid,
23662366
/// If `true`, the disk created from this snapshot will be read-only.
2367+
#[serde(default)]
23672368
read_only: bool,
23682369
},
23692370

23702371
/// Create a disk from an image
23712372
Image {
23722373
image_id: Uuid,
23732374
/// If `true`, the disk created from this image will be read-only.
2375+
#[serde(default)]
23742376
read_only: bool,
23752377
},
23762378

0 commit comments

Comments
 (0)