From a195e57e0d019010da51d9213c86355420d55732 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 30 Mar 2022 10:46:50 -0400 Subject: [PATCH 1/8] Block size must be given during disk create Block size of disks must be variable and declared during disk creation. In other instances, it can be inherited (for example, creating a disk from a snapshot will set the block size to whatever the snapshot's was). Store this in the disks table. Also, prepare for creating disks from images and snapshots by adding the appropriate columns to the disks table. Externally, the API should be returning names, not IDs, but this commit doesn't tackle that. At minimum this will require Nexus to convert a db::model::Disk into an external::Disk by looking up ids and filling in the appropriate names (instead of a simple From impl). I'll create an issue to track this work. --- common/src/api/external/mod.rs | 3 ++ common/src/sql/dbinit.sql | 4 ++- nexus/src/db/datastore.rs | 2 ++ nexus/src/db/model.rs | 14 ++++++++ nexus/src/db/schema.rs | 2 ++ nexus/src/external_api/params.rs | 11 ++++-- nexus/src/nexus.rs | 29 ++++++++++++++-- nexus/src/sagas.rs | 3 ++ nexus/test-utils/src/resource_helpers.rs | 2 ++ nexus/tests/integration_tests/disks.rs | 10 ++++++ nexus/tests/integration_tests/endpoints.rs | 2 ++ openapi/nexus.json | 39 ++++++++++++++++++++++ 12 files changed, 115 insertions(+), 6 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index a7cb8cb3344..4965d85caaa 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -753,9 +753,12 @@ pub struct Instance { pub struct Disk { #[serde(flatten)] pub identity: IdentityMetadata, + // TODO these should be names! pub project_id: Uuid, pub snapshot_id: Option, + pub image_id: Option, pub size: ByteCount, + pub block_size: ByteCount, pub state: DiskState, pub device_path: String, } diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 5a72569ee99..fdcae4f0e5c 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -400,7 +400,9 @@ CREATE TABLE omicron.public.disk ( /* Disk configuration */ size_bytes INT NOT NULL, - origin_snapshot UUID + block_size INT NOT NULL, + origin_snapshot UUID, + origin_image UUID ); CREATE UNIQUE INDEX ON omicron.public.disk ( diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index e26d30efcca..8154aef35c2 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3930,7 +3930,9 @@ mod test { description: name.to_string(), }, snapshot_id: None, + image_id: None, size, + block_size: ByteCount::from(4096), } } diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 5f51ec218b1..4c641a316d4 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1313,10 +1313,19 @@ pub struct Disk { /// size of the Disk #[column_name = "size_bytes"] pub size: ByteCount, + + /// size of blocks + pub block_size: ByteCount, + /// id for the snapshot from which this Disk was created (None means a blank /// disk) #[column_name = "origin_snapshot"] pub create_snapshot_id: Option, + + /// id for the image from which this Disk was created (None means a blank + /// disk) + #[column_name = "origin_image"] + pub create_image_id: Option, } impl Disk { @@ -1335,7 +1344,9 @@ impl Disk { volume_id, runtime_state: runtime_initial, size: params.size.into(), + block_size: params.block_size.into(), create_snapshot_id: params.snapshot_id, + create_image_id: params.image_id, } } @@ -1358,9 +1369,12 @@ impl Into for Disk { let device_path = format!("/mnt/{}", self.name().as_str()); external::Disk { identity: self.identity(), + // TODO these should be names! project_id: self.project_id, snapshot_id: self.create_snapshot_id, + image_id: self.create_image_id, size: self.size.into(), + block_size: self.block_size.into(), state: self.state().into(), device_path, } diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 4353f5e375f..a9325193073 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -22,7 +22,9 @@ table! { state_generation -> Int8, time_state_updated -> Timestamptz, size_bytes -> Int8, + block_size -> Int8, origin_snapshot -> Nullable, + origin_image -> Nullable, } } diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 97cb8a03688..9d29bbaea77 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -242,20 +242,23 @@ pub struct DiskCreate { pub identity: IdentityMetadataCreateParams, /// id for snapshot from which the Disk should be created, if any pub snapshot_id: Option, // TODO should be a name? + /// id for image from which the Disk should be created, if any + pub image_id: Option, // TODO should be a name? /// size of the Disk pub size: ByteCount, + /// block size for this disk + pub block_size: ByteCount, } -const BLOCK_SIZE: u32 = 1_u32 << 12; const EXTENT_SIZE: u32 = 1_u32 << 20; impl DiskCreate { pub fn block_size(&self) -> ByteCount { - ByteCount::from(BLOCK_SIZE) + self.block_size } pub fn blocks_per_extent(&self) -> i64 { - EXTENT_SIZE as i64 / BLOCK_SIZE as i64 + EXTENT_SIZE as i64 / i64::from(self.block_size) } pub fn extent_count(&self) -> i64 { @@ -319,7 +322,9 @@ mod test { description: "desc".to_string(), }, snapshot_id: None, + image_id: None, size, + block_size: ByteCount::from(512), } } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index c5b27852646..16f9a797f57 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -751,8 +751,24 @@ impl Nexus { // (if possibly redundant) to check this here. opctx.authorize(authz::Action::CreateChild, &authz_project).await?; - // Until we implement snapshots, do not allow disks to be created with a - // snapshot id. + // Reject invalid block sizes + match params.block_size.to_bytes() { + 512 | 4096 => { + // ok + } + + _ => { + return Err(Error::InvalidValue { + label: String::from("block_size"), + message: String::from( + "supported block sizes are 512 and 4096", + ), + }); + } + } + + // Until we implement snapshots, do not allow disks to be created from a + // snapshot. if params.snapshot_id.is_some() { return Err(Error::InvalidValue { label: String::from("snapshot_id"), @@ -760,6 +776,15 @@ impl Nexus { }); } + // Until we implement images, do not allow disks to be created from an + // image. + if params.image_id.is_some() { + return Err(Error::InvalidValue { + label: String::from("image_id"), + message: String::from("images are not yet supported"), + }); + } + let saga_params = Arc::new(sagas::ParamsDiskCreate { serialized_authn: authn::saga::Serialized::for_opctx(opctx), project_id: authz_project.id(), diff --git a/nexus/src/sagas.rs b/nexus/src/sagas.rs index 6cb886f9b4c..9953feed33b 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -1033,6 +1033,7 @@ async fn sdc_create_disk_record( // but this should be acceptable because the disk remains in a "Creating" // state until the saga has completed. let volume_id = sagactx.lookup::("volume_id")?; + let disk = db::model::Disk::new( disk_id, params.project_id, @@ -1040,11 +1041,13 @@ async fn sdc_create_disk_record( params.create_params.clone(), db::model::DiskRuntimeState::new(), ); + let disk_created = osagactx .datastore() .project_create_disk(disk) .await .map_err(ActionError::action_failed)?; + Ok(disk_created) } diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 29417c0f580..a4d62e4f34a 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -133,7 +133,9 @@ pub async fn create_disk( description: String::from("sells rainsticks"), }, snapshot_id: None, + image_id: None, size: ByteCount::from_gibibytes_u32(1), + block_size: ByteCount::from(512), }, ) .await diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index d32ccc52cac..2e24ec7aac2 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -130,7 +130,9 @@ async fn test_disk_create_attach_detach_delete( assert_eq!(disk.identity.description, "sells rainsticks"); assert_eq!(disk.project_id, project_id); assert_eq!(disk.snapshot_id, None); + assert_eq!(disk.image_id, None); assert_eq!(disk.size.to_whole_mebibytes(), 1024); + assert_eq!(disk.block_size.to_bytes(), 512); assert_eq!(disk.state, DiskState::Creating); // Fetch the disk and expect it to match what we just created except that @@ -141,7 +143,9 @@ async fn test_disk_create_attach_detach_delete( assert_eq!(disk.identity.description, "sells rainsticks"); assert_eq!(disk.project_id, project_id); assert_eq!(disk.snapshot_id, None); + assert_eq!(disk.image_id, None); assert_eq!(disk.size.to_whole_mebibytes(), 1024); + assert_eq!(disk.block_size.to_bytes(), 512); assert_eq!(disk.state, DiskState::Detached); // List disks again and expect to find the one we just created. @@ -264,7 +268,9 @@ async fn test_disk_create_disk_that_already_exists_fails( description: String::from("sells rainsticks"), }, snapshot_id: None, + image_id: None, size: ByteCount::from_gibibytes_u32(1), + block_size: ByteCount::from(512), }; let _ = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; let disk_url = format!("{}/{}", disks_url, DISK_NAME); @@ -643,7 +649,9 @@ async fn test_disk_region_creation_failure( description: String::from("sells rainsticks"), }, snapshot_id: None, + image_id: None, size: disk_size, + block_size: ByteCount::from(512), }; // Unfortunately, the error message is only posted internally to the @@ -724,7 +732,9 @@ fn disks_eq(disk1: &Disk, disk2: &Disk) { identity_eq(&disk1.identity, &disk2.identity); assert_eq!(disk1.project_id, disk2.project_id); assert_eq!(disk1.snapshot_id, disk2.snapshot_id); + assert_eq!(disk1.image_id, disk2.image_id); assert_eq!(disk1.size.to_bytes(), disk2.size.to_bytes()); + assert_eq!(disk1.block_size.to_bytes(), disk2.block_size.to_bytes()); assert_eq!(disk1.state, disk2.state); assert_eq!(disk1.device_path, disk2.device_path); } diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index f6649b9b16e..29059c6beee 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -141,7 +141,9 @@ lazy_static! { description: "".parse().unwrap(), }, snapshot_id: None, + image_id: None, size: ByteCount::from_gibibytes_u32(16), + block_size: ByteCount::from(512), }; // Instance used for testing diff --git a/openapi/nexus.json b/openapi/nexus.json index 6d337a41cee..63893ba9fbc 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4364,6 +4364,9 @@ "description": "Client view of an [`Disk`]", "type": "object", "properties": { + "block_size": { + "$ref": "#/components/schemas/ByteCount" + }, "description": { "description": "human-readable free-form text about a resource", "type": "string" @@ -4376,6 +4379,11 @@ "type": "string", "format": "uuid" }, + "image_id": { + "nullable": true, + "type": "string", + "format": "uuid" + }, "name": { "description": "unique, mutable, user-controlled identifier for each resource", "allOf": [ @@ -4411,6 +4419,7 @@ } }, "required": [ + "block_size", "description", "device_path", "id", @@ -4426,9 +4435,23 @@ "description": "Create-time parameters for a [`Disk`](omicron_common::api::external::Disk)", "type": "object", "properties": { + "block_size": { + "description": "block size for this disk", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] + }, "description": { "type": "string" }, + "image_id": { + "nullable": true, + "description": "id for image from which the Disk should be created, if any", + "type": "string", + "format": "uuid" + }, "name": { "$ref": "#/components/schemas/Name" }, @@ -4448,6 +4471,7 @@ } }, "required": [ + "block_size", "description", "name", "size" @@ -4803,9 +4827,23 @@ "description": "During instance creation, create and attach disks", "type": "object", "properties": { + "block_size": { + "description": "block size for this disk", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] + }, "description": { "type": "string" }, + "image_id": { + "nullable": true, + "description": "id for image from which the Disk should be created, if any", + "type": "string", + "format": "uuid" + }, "name": { "$ref": "#/components/schemas/Name" }, @@ -4831,6 +4869,7 @@ } }, "required": [ + "block_size", "description", "name", "size", From df508d22b2646f6ab165c4faa56ac8578087e761 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 31 Mar 2022 10:16:18 -0400 Subject: [PATCH 2/8] better API documentation --- nexus/src/external_api/params.rs | 4 ++-- openapi/nexus.json | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 9d29bbaea77..508ae327e69 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -244,9 +244,9 @@ pub struct DiskCreate { pub snapshot_id: Option, // TODO should be a name? /// id for image from which the Disk should be created, if any pub image_id: Option, // TODO should be a name? - /// size of the Disk + /// total size of the Disk in bytes pub size: ByteCount, - /// block size for this disk + /// size of blocks for this Disk. valid values are currently 512 or 4096 pub block_size: ByteCount, } diff --git a/openapi/nexus.json b/openapi/nexus.json index 63893ba9fbc..1c6b082ddfc 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4436,7 +4436,7 @@ "type": "object", "properties": { "block_size": { - "description": "block size for this disk", + "description": "size of blocks for this Disk. valid values are currently 512 or 4096", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -4456,7 +4456,7 @@ "$ref": "#/components/schemas/Name" }, "size": { - "description": "size of the Disk", + "description": "total size of the Disk in bytes", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -4828,7 +4828,7 @@ "type": "object", "properties": { "block_size": { - "description": "block size for this disk", + "description": "size of blocks for this Disk. valid values are currently 512 or 4096", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -4848,7 +4848,7 @@ "$ref": "#/components/schemas/Name" }, "size": { - "description": "size of the Disk", + "description": "total size of the Disk in bytes", "allOf": [ { "$ref": "#/components/schemas/ByteCount" From 2c8b41c7e09997f7557de7cc698c07c930838682 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 31 Mar 2022 10:27:45 -0400 Subject: [PATCH 3/8] Reject disks where the block size doesnt evenly divide the total actually commit tests --- nexus/src/nexus.rs | 11 ++++ nexus/tests/integration_tests/disks.rs | 88 ++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 16f9a797f57..19ece915a20 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -767,6 +767,17 @@ impl Nexus { } } + // Reject disks where the block size doesn't evenly divide the total + // size + if (params.size.to_bytes() % params.block_size.to_bytes()) != 0 { + return Err(Error::InvalidValue { + label: String::from("size and block_size"), + message: String::from( + "total size must be a multiple of block size", + ), + }); + } + // Until we implement snapshots, do not allow disks to be created from a // snapshot. if params.snapshot_id.is_some() { diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 2e24ec7aac2..4c7915e0316 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -693,6 +693,94 @@ async fn test_disk_region_creation_failure( let _ = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } +// Tests that invalid block sizes are rejected +#[nexus_test] +async fn test_disk_invalid_block_size_rejected( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let test = DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; + + let disk_size = ByteCount::from_gibibytes_u32(3); + let dataset_count = test.dataset_ids.len() as u64; + assert!( + disk_size.to_bytes() * dataset_count < test.zpool_size.to_bytes(), + "Disk size too big for Zpool size" + ); + assert!( + 2 * disk_size.to_bytes() * dataset_count > test.zpool_size.to_bytes(), + "(test constraint) Zpool needs to be smaller (to store only one disk)", + ); + + // Attempt to allocate the disk, observe a server error. + let disks_url = get_disks_url(); + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DISK_NAME.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + snapshot_id: None, + image_id: None, + size: disk_size, + block_size: ByteCount::from(1024), + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + +// Tests that a disk is rejected if the total size isn't divided by the block size +#[nexus_test] +async fn test_disk_reject_total_size_not_divisible_by_block_size( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let test = DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; + + let disk_size = ByteCount::from(3 * 1024 * 1024 * 1024 + 256); + let dataset_count = test.dataset_ids.len() as u64; + assert!( + disk_size.to_bytes() * dataset_count < test.zpool_size.to_bytes(), + "Disk size too big for Zpool size" + ); + assert!( + 2 * disk_size.to_bytes() * dataset_count > test.zpool_size.to_bytes(), + "(test constraint) Zpool needs to be smaller (to store only one disk)", + ); + + // Attempt to allocate the disk, observe a server error. + let disks_url = get_disks_url(); + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DISK_NAME.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + snapshot_id: None, + image_id: None, + size: disk_size, + block_size: ByteCount::from(512), + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { NexusRequest::object_get(client, disk_url) .authn_as(AuthnMode::PrivilegedUser) From 88f23c5ec8a65800c7bf788af0e123e513ccab60 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 31 Mar 2022 11:04:54 -0400 Subject: [PATCH 4/8] remove "should be names" comments --- common/src/api/external/mod.rs | 1 - nexus/src/db/model.rs | 1 - nexus/src/external_api/params.rs | 4 ++-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 4965d85caaa..99b9f6531df 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -753,7 +753,6 @@ pub struct Instance { pub struct Disk { #[serde(flatten)] pub identity: IdentityMetadata, - // TODO these should be names! pub project_id: Uuid, pub snapshot_id: Option, pub image_id: Option, diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 4c641a316d4..1952e62c098 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1369,7 +1369,6 @@ impl Into for Disk { let device_path = format!("/mnt/{}", self.name().as_str()); external::Disk { identity: self.identity(), - // TODO these should be names! project_id: self.project_id, snapshot_id: self.create_snapshot_id, image_id: self.create_image_id, diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 508ae327e69..150268ba04f 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -241,9 +241,9 @@ pub struct DiskCreate { #[serde(flatten)] pub identity: IdentityMetadataCreateParams, /// id for snapshot from which the Disk should be created, if any - pub snapshot_id: Option, // TODO should be a name? + pub snapshot_id: Option, /// id for image from which the Disk should be created, if any - pub image_id: Option, // TODO should be a name? + pub image_id: Option, /// total size of the Disk in bytes pub size: ByteCount, /// size of blocks for this Disk. valid values are currently 512 or 4096 From a20f0eb2b5d577c14cb599c6f48c329e8e350bc9 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 7 Apr 2022 11:31:55 -0400 Subject: [PATCH 5/8] use an enum for block size --- common/src/sql/dbinit.sql | 8 +++- nexus/src/db/datastore.rs | 2 +- nexus/src/db/model.rs | 47 ++++++++++++++++++-- nexus/src/db/schema.rs | 2 +- nexus/src/external_api/params.rs | 22 +++++++--- nexus/src/nexus.rs | 18 +------- nexus/test-utils/src/resource_helpers.rs | 2 +- nexus/tests/integration_tests/disks.rs | 50 ++-------------------- nexus/tests/integration_tests/endpoints.rs | 2 +- openapi/nexus.json | 16 +++++-- 10 files changed, 88 insertions(+), 81 deletions(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index fdcae4f0e5c..468032bce7e 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -365,6 +365,12 @@ CREATE UNIQUE INDEX ON omicron.public.instance ( -- 'faulted' -- ); +CREATE TYPE omicron.public.block_size AS ENUM ( + 'traditional', + 'iso', + 'advancedformat' +); + CREATE TABLE omicron.public.disk ( /* Identity metadata (resource) */ id UUID PRIMARY KEY, @@ -400,7 +406,7 @@ CREATE TABLE omicron.public.disk ( /* Disk configuration */ size_bytes INT NOT NULL, - block_size INT NOT NULL, + block_size omicron.public.block_size NOT NULL, origin_snapshot UUID, origin_image UUID ); diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 8154aef35c2..c949dc9b5de 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3932,7 +3932,7 @@ mod test { snapshot_id: None, image_id: None, size, - block_size: ByteCount::from(4096), + block_size: params::BlockSize::AdvancedFormat, } } diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 1952e62c098..e9d6e0f1b8c 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -120,9 +120,9 @@ macro_rules! impl_enum_type { $(#[$model_meta:meta])* pub enum $model_type:ident; + $($enum_item:ident => $sql_value:literal)+ ) => { - $(#[$enum_meta])* pub struct $diesel_type; @@ -1281,6 +1281,47 @@ impl From for sled_agent_client::types::InstanceState { } } +impl_enum_type!( + #[derive(SqlType, Debug, QueryId)] + #[postgres(type_name = "block_size", type_schema = "public")] + pub struct BlockSizeEnum; + + #[derive(Copy, Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize, PartialEq)] + #[sql_type = "BlockSizeEnum"] + pub enum BlockSize; + + // Enum values + Traditional => b"traditional" + Iso => b"iso" + AdvancedFormat => b"advancedformat" +); + +impl BlockSize { + pub fn to_bytes(&self) -> u32 { + match self { + BlockSize::Traditional => 512, + BlockSize::Iso => 2048, + BlockSize::AdvancedFormat => 4096, + } + } +} + +impl Into for BlockSize { + fn into(self) -> external::ByteCount { + external::ByteCount::from(self.to_bytes()) + } +} + +impl From for BlockSize { + fn from(block_size: params::BlockSize) -> BlockSize { + match block_size { + params::BlockSize::Traditional => BlockSize::Traditional, + params::BlockSize::Iso => BlockSize::Iso, + params::BlockSize::AdvancedFormat => BlockSize::AdvancedFormat, + } + } +} + /// A Disk (network block device). #[derive( Queryable, @@ -1314,8 +1355,8 @@ pub struct Disk { #[column_name = "size_bytes"] pub size: ByteCount, - /// size of blocks - pub block_size: ByteCount, + /// size of blocks (512, 2048, or 4096) + pub block_size: BlockSize, /// id for the snapshot from which this Disk was created (None means a blank /// disk) diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index a9325193073..387d12e20df 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -22,7 +22,7 @@ table! { state_generation -> Int8, time_state_updated -> Timestamptz, size_bytes -> Int8, - block_size -> Int8, + block_size -> crate::db::model::BlockSizeEnum, origin_snapshot -> Nullable, origin_image -> Nullable, } diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 150268ba04f..3cb4487f961 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -234,6 +234,18 @@ pub struct VpcRouterUpdate { // DISKS +#[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub enum BlockSize { + /// 512 byte blocks + Traditional = 512, + + /// 2048 byte blocks + Iso = 2048, + + /// 4096 byte blocks + AdvancedFormat = 4096, +} + /// Create-time parameters for a [`Disk`](omicron_common::api::external::Disk) #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct DiskCreate { @@ -246,19 +258,19 @@ pub struct DiskCreate { pub image_id: Option, /// total size of the Disk in bytes pub size: ByteCount, - /// size of blocks for this Disk. valid values are currently 512 or 4096 - pub block_size: ByteCount, + /// size of blocks for this Disk. + pub block_size: BlockSize, } const EXTENT_SIZE: u32 = 1_u32 << 20; impl DiskCreate { pub fn block_size(&self) -> ByteCount { - self.block_size + ByteCount::from(self.block_size as u32) } pub fn blocks_per_extent(&self) -> i64 { - EXTENT_SIZE as i64 / i64::from(self.block_size) + EXTENT_SIZE as i64 / i64::from(self.block_size as u32) } pub fn extent_count(&self) -> i64 { @@ -324,7 +336,7 @@ mod test { snapshot_id: None, image_id: None, size, - block_size: ByteCount::from(512), + block_size: BlockSize::AdvancedFormat, } } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 19ece915a20..5c8005fd2d7 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -751,25 +751,9 @@ impl Nexus { // (if possibly redundant) to check this here. opctx.authorize(authz::Action::CreateChild, &authz_project).await?; - // Reject invalid block sizes - match params.block_size.to_bytes() { - 512 | 4096 => { - // ok - } - - _ => { - return Err(Error::InvalidValue { - label: String::from("block_size"), - message: String::from( - "supported block sizes are 512 and 4096", - ), - }); - } - } - // Reject disks where the block size doesn't evenly divide the total // size - if (params.size.to_bytes() % params.block_size.to_bytes()) != 0 { + if (params.size.to_bytes() % (params.block_size as u64)) != 0 { return Err(Error::InvalidValue { label: String::from("size and block_size"), message: String::from( diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index a4d62e4f34a..2e3eac2b9ac 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -135,7 +135,7 @@ pub async fn create_disk( snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(1), - block_size: ByteCount::from(512), + block_size: params::BlockSize::Traditional, }, ) .await diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 4c7915e0316..6832628f3e1 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -270,7 +270,7 @@ async fn test_disk_create_disk_that_already_exists_fails( snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(1), - block_size: ByteCount::from(512), + block_size: params::BlockSize::Traditional, }; let _ = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; let disk_url = format!("{}/{}", disks_url, DISK_NAME); @@ -651,7 +651,7 @@ async fn test_disk_region_creation_failure( snapshot_id: None, image_id: None, size: disk_size, - block_size: ByteCount::from(512), + block_size: params::BlockSize::Traditional, }; // Unfortunately, the error message is only posted internally to the @@ -693,50 +693,6 @@ async fn test_disk_region_creation_failure( let _ = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } -// Tests that invalid block sizes are rejected -#[nexus_test] -async fn test_disk_invalid_block_size_rejected( - cptestctx: &ControlPlaneTestContext, -) { - let client = &cptestctx.external_client; - let test = DiskTest::new(&cptestctx).await; - create_org_and_project(client).await; - - let disk_size = ByteCount::from_gibibytes_u32(3); - let dataset_count = test.dataset_ids.len() as u64; - assert!( - disk_size.to_bytes() * dataset_count < test.zpool_size.to_bytes(), - "Disk size too big for Zpool size" - ); - assert!( - 2 * disk_size.to_bytes() * dataset_count > test.zpool_size.to_bytes(), - "(test constraint) Zpool needs to be smaller (to store only one disk)", - ); - - // Attempt to allocate the disk, observe a server error. - let disks_url = get_disks_url(); - let new_disk = params::DiskCreate { - identity: IdentityMetadataCreateParams { - name: DISK_NAME.parse().unwrap(), - description: String::from("sells rainsticks"), - }, - snapshot_id: None, - image_id: None, - size: disk_size, - block_size: ByteCount::from(1024), - }; - - NexusRequest::new( - RequestBuilder::new(client, Method::POST, &disks_url) - .body(Some(&new_disk)) - .expect_status(Some(StatusCode::BAD_REQUEST)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap(); -} - // Tests that a disk is rejected if the total size isn't divided by the block size #[nexus_test] async fn test_disk_reject_total_size_not_divisible_by_block_size( @@ -767,7 +723,7 @@ async fn test_disk_reject_total_size_not_divisible_by_block_size( snapshot_id: None, image_id: None, size: disk_size, - block_size: ByteCount::from(512), + block_size: params::BlockSize::Traditional, }; NexusRequest::new( diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 29059c6beee..c140d9ed121 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -143,7 +143,7 @@ lazy_static! { snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(16), - block_size: ByteCount::from(512), + block_size: params::BlockSize::AdvancedFormat, }; // Instance used for testing diff --git a/openapi/nexus.json b/openapi/nexus.json index 1c6b082ddfc..e6b7fafdc21 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4339,6 +4339,14 @@ } }, "schemas": { + "BlockSize": { + "type": "string", + "enum": [ + "Traditional", + "Iso", + "AdvancedFormat" + ] + }, "ByteCount": { "description": "A count of bytes, typically used either for memory or storage capacity\n\nThe maximum supported byte count is [`i64::MAX`]. This makes it somewhat inconvenient to define constructors: a u32 constructor can be infallible, but an i64 constructor can fail (if the value is negative) and a u64 constructor can fail (if the value is larger than i64::MAX). We provide all of these for consumers' convenience.", "type": "integer", @@ -4436,10 +4444,10 @@ "type": "object", "properties": { "block_size": { - "description": "size of blocks for this Disk. valid values are currently 512 or 4096", + "description": "size of blocks for this Disk.", "allOf": [ { - "$ref": "#/components/schemas/ByteCount" + "$ref": "#/components/schemas/BlockSize" } ] }, @@ -4828,10 +4836,10 @@ "type": "object", "properties": { "block_size": { - "description": "size of blocks for this Disk. valid values are currently 512 or 4096", + "description": "size of blocks for this Disk.", "allOf": [ { - "$ref": "#/components/schemas/ByteCount" + "$ref": "#/components/schemas/BlockSize" } ] }, From c5d762042c0c6dabc176d00a15f389998a7c8dbf Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 7 Apr 2022 13:44:46 -0400 Subject: [PATCH 6/8] use numeric block size for api --- nexus/src/db/datastore.rs | 2 +- nexus/src/db/model.rs | 22 +++++----- nexus/src/external_api/params.rs | 22 +++------- nexus/src/nexus.rs | 2 +- nexus/src/sagas.rs | 5 ++- nexus/test-utils/src/resource_helpers.rs | 2 +- nexus/tests/integration_tests/disks.rs | 50 ++++++++++++++++++++-- nexus/tests/integration_tests/endpoints.rs | 2 +- openapi/nexus.json | 16 ++----- 9 files changed, 76 insertions(+), 47 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index a19c8fd9b47..3a1b1ecc415 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3054,7 +3054,7 @@ mod test { snapshot_id: None, image_id: None, size, - block_size: params::BlockSize::AdvancedFormat, + block_size: ByteCount::from(4096), } } diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index cda0b795684..84444670296 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1312,12 +1312,14 @@ impl Into for BlockSize { } } -impl From for BlockSize { - fn from(block_size: params::BlockSize) -> BlockSize { - match block_size { - params::BlockSize::Traditional => BlockSize::Traditional, - params::BlockSize::Iso => BlockSize::Iso, - params::BlockSize::AdvancedFormat => BlockSize::AdvancedFormat, +impl TryFrom for BlockSize { + type Error = anyhow::Error; + fn try_from(block_size: external::ByteCount) -> Result { + match block_size.to_bytes() { + 512 => Ok(BlockSize::Traditional), + 2048 => Ok(BlockSize::Iso), + 4096 => Ok(BlockSize::AdvancedFormat), + _ => anyhow::bail!("invalid block size {}", block_size.to_bytes()), } } } @@ -1376,19 +1378,19 @@ impl Disk { volume_id: Uuid, params: params::DiskCreate, runtime_initial: DiskRuntimeState, - ) -> Self { + ) -> Result { let identity = DiskIdentity::new(disk_id, params.identity); - Self { + Ok(Self { identity, rcgen: external::Generation::new().into(), project_id, volume_id, runtime_state: runtime_initial, size: params.size.into(), - block_size: params.block_size.into(), + block_size: params.block_size.try_into()?, create_snapshot_id: params.snapshot_id, create_image_id: params.image_id, - } + }) } pub fn state(&self) -> DiskState { diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index ffeeb6b5879..825d5a7499e 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -229,18 +229,6 @@ pub struct VpcRouterUpdate { // DISKS -#[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub enum BlockSize { - /// 512 byte blocks - Traditional = 512, - - /// 2048 byte blocks - Iso = 2048, - - /// 4096 byte blocks - AdvancedFormat = 4096, -} - /// Create-time parameters for a [`Disk`](omicron_common::api::external::Disk) #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct DiskCreate { @@ -253,19 +241,19 @@ pub struct DiskCreate { pub image_id: Option, /// total size of the Disk in bytes pub size: ByteCount, - /// size of blocks for this Disk. - pub block_size: BlockSize, + /// size of blocks for this Disk. valid values are: 512, 2048, or 4096 + pub block_size: ByteCount, } const EXTENT_SIZE: u32 = 1_u32 << 20; impl DiskCreate { pub fn block_size(&self) -> ByteCount { - ByteCount::from(self.block_size as u32) + self.block_size } pub fn blocks_per_extent(&self) -> i64 { - EXTENT_SIZE as i64 / i64::from(self.block_size as u32) + EXTENT_SIZE as i64 / i64::from(self.block_size) } pub fn extent_count(&self) -> i64 { @@ -352,7 +340,7 @@ mod test { snapshot_id: None, image_id: None, size, - block_size: BlockSize::AdvancedFormat, + block_size: ByteCount::from(4096), } } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 1c722b65c08..d3bff96adf8 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -775,7 +775,7 @@ impl Nexus { // Reject disks where the block size doesn't evenly divide the total // size - if (params.size.to_bytes() % (params.block_size as u64)) != 0 { + if (params.size.to_bytes() % params.block_size.to_bytes()) != 0 { return Err(Error::InvalidValue { label: String::from("size and block_size"), message: String::from( diff --git a/nexus/src/sagas.rs b/nexus/src/sagas.rs index b64785dd3a6..ba3c0f8c7a4 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -1026,7 +1026,10 @@ async fn sdc_create_disk_record( volume_id, params.create_params.clone(), db::model::DiskRuntimeState::new(), - ); + ) + .map_err(|e| { + ActionError::action_failed(Error::invalid_request(&e.to_string())) + })?; let disk_created = osagactx .datastore() diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 2e3eac2b9ac..a4d62e4f34a 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -135,7 +135,7 @@ pub async fn create_disk( snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(1), - block_size: params::BlockSize::Traditional, + block_size: ByteCount::from(512), }, ) .await diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 6832628f3e1..4c7915e0316 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -270,7 +270,7 @@ async fn test_disk_create_disk_that_already_exists_fails( snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(1), - block_size: params::BlockSize::Traditional, + block_size: ByteCount::from(512), }; let _ = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; let disk_url = format!("{}/{}", disks_url, DISK_NAME); @@ -651,7 +651,7 @@ async fn test_disk_region_creation_failure( snapshot_id: None, image_id: None, size: disk_size, - block_size: params::BlockSize::Traditional, + block_size: ByteCount::from(512), }; // Unfortunately, the error message is only posted internally to the @@ -693,6 +693,50 @@ async fn test_disk_region_creation_failure( let _ = create_disk(client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; } +// Tests that invalid block sizes are rejected +#[nexus_test] +async fn test_disk_invalid_block_size_rejected( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + let test = DiskTest::new(&cptestctx).await; + create_org_and_project(client).await; + + let disk_size = ByteCount::from_gibibytes_u32(3); + let dataset_count = test.dataset_ids.len() as u64; + assert!( + disk_size.to_bytes() * dataset_count < test.zpool_size.to_bytes(), + "Disk size too big for Zpool size" + ); + assert!( + 2 * disk_size.to_bytes() * dataset_count > test.zpool_size.to_bytes(), + "(test constraint) Zpool needs to be smaller (to store only one disk)", + ); + + // Attempt to allocate the disk, observe a server error. + let disks_url = get_disks_url(); + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: DISK_NAME.parse().unwrap(), + description: String::from("sells rainsticks"), + }, + snapshot_id: None, + image_id: None, + size: disk_size, + block_size: ByteCount::from(1024), + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::BAD_REQUEST)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + // Tests that a disk is rejected if the total size isn't divided by the block size #[nexus_test] async fn test_disk_reject_total_size_not_divisible_by_block_size( @@ -723,7 +767,7 @@ async fn test_disk_reject_total_size_not_divisible_by_block_size( snapshot_id: None, image_id: None, size: disk_size, - block_size: params::BlockSize::Traditional, + block_size: ByteCount::from(512), }; NexusRequest::new( diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index d97be0fb4ae..87c4825f64e 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -145,7 +145,7 @@ lazy_static! { snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(16), - block_size: params::BlockSize::AdvancedFormat, + block_size: ByteCount::from(4096), }; // Instance used for testing diff --git a/openapi/nexus.json b/openapi/nexus.json index 297ed6cf817..b2f92c61035 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4749,14 +4749,6 @@ } }, "schemas": { - "BlockSize": { - "type": "string", - "enum": [ - "Traditional", - "Iso", - "AdvancedFormat" - ] - }, "ByteCount": { "description": "A count of bytes, typically used either for memory or storage capacity\n\nThe maximum supported byte count is [`i64::MAX`]. This makes it somewhat inconvenient to define constructors: a u32 constructor can be infallible, but an i64 constructor can fail (if the value is negative) and a u64 constructor can fail (if the value is larger than i64::MAX). We provide all of these for consumers' convenience.", "type": "integer", @@ -4854,10 +4846,10 @@ "type": "object", "properties": { "block_size": { - "description": "size of blocks for this Disk.", + "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096", "allOf": [ { - "$ref": "#/components/schemas/BlockSize" + "$ref": "#/components/schemas/ByteCount" } ] }, @@ -5375,10 +5367,10 @@ "type": "object", "properties": { "block_size": { - "description": "size of blocks for this Disk.", + "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096", "allOf": [ { - "$ref": "#/components/schemas/BlockSize" + "$ref": "#/components/schemas/ByteCount" } ] }, From d5db0b5e7de5b7223dcafeb66d944f05d30056ec Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 7 Apr 2022 15:24:26 -0400 Subject: [PATCH 7/8] block_size strings should just be numbers --- common/src/sql/dbinit.sql | 6 +++--- nexus/src/db/model.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 47200095321..474e87be932 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -366,9 +366,9 @@ CREATE UNIQUE INDEX ON omicron.public.instance ( -- ); CREATE TYPE omicron.public.block_size AS ENUM ( - 'traditional', - 'iso', - 'advancedformat' + '512', + '2048', + '4096' ); CREATE TABLE omicron.public.disk ( diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 84444670296..b2344217f8f 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1291,9 +1291,9 @@ impl_enum_type!( pub enum BlockSize; // Enum values - Traditional => b"traditional" - Iso => b"iso" - AdvancedFormat => b"advancedformat" + Traditional => b"512" + Iso => b"2048" + AdvancedFormat => b"4096" ); impl BlockSize { From ce7e8fd69dacf8455b70bd77cde397f063210444 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Thu, 7 Apr 2022 16:27:44 -0400 Subject: [PATCH 8/8] block size param should be an integer enum --- nexus/src/db/datastore.rs | 2 +- nexus/src/db/model.rs | 8 +-- nexus/src/external_api/params.rs | 69 ++++++++++++++++++++-- nexus/src/nexus.rs | 2 +- nexus/test-utils/src/resource_helpers.rs | 2 +- nexus/tests/integration_tests/disks.rs | 8 +-- nexus/tests/integration_tests/endpoints.rs | 2 +- openapi/nexus.json | 13 +++- 8 files changed, 88 insertions(+), 18 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 3a1b1ecc415..e1e56f37093 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3054,7 +3054,7 @@ mod test { snapshot_id: None, image_id: None, size, - block_size: ByteCount::from(4096), + block_size: params::BlockSize::try_from(4096).unwrap(), } } diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index b2344217f8f..b1ab41bdf89 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -1312,14 +1312,14 @@ impl Into for BlockSize { } } -impl TryFrom for BlockSize { +impl TryFrom for BlockSize { type Error = anyhow::Error; - fn try_from(block_size: external::ByteCount) -> Result { - match block_size.to_bytes() { + fn try_from(block_size: params::BlockSize) -> Result { + match block_size.0 { 512 => Ok(BlockSize::Traditional), 2048 => Ok(BlockSize::Iso), 4096 => Ok(BlockSize::AdvancedFormat), - _ => anyhow::bail!("invalid block size {}", block_size.to_bytes()), + _ => anyhow::bail!("invalid block size {}", block_size.0), } } } diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index 825d5a7499e..14d85d89e30 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -10,6 +10,7 @@ use omicron_common::api::external::{ }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; use std::net::IpAddr; use uuid::Uuid; @@ -229,6 +230,66 @@ pub struct VpcRouterUpdate { // DISKS +#[derive(Copy, Clone, Debug, Deserialize, Serialize)] +pub struct BlockSize(pub u32); + +impl TryFrom for BlockSize { + type Error = anyhow::Error; + fn try_from(x: u32) -> Result { + if ![512, 2048, 4096].contains(&x) { + anyhow::bail!("invalid block size {}", x); + } + + Ok(BlockSize(x)) + } +} + +impl Into for BlockSize { + fn into(self) -> ByteCount { + ByteCount::from(self.0) + } +} + +impl JsonSchema for BlockSize { + fn schema_name() -> String { + "BlockSize".to_string() + } + + fn json_schema( + _gen: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::Schema::Object(schemars::schema::SchemaObject { + metadata: Some(Box::new(schemars::schema::Metadata { + id: None, + title: Some("disk block size in bytes".to_string()), + description: None, + default: None, + deprecated: false, + read_only: false, + write_only: false, + examples: vec![], + })), + instance_type: Some(schemars::schema::SingleOrVec::Single( + Box::new(schemars::schema::InstanceType::Integer), + )), + format: None, + enum_values: Some(vec![ + serde_json::json!(512), + serde_json::json!(2048), + serde_json::json!(4096), + ]), + const_value: None, + subschemas: None, + number: None, + string: None, + array: None, + object: None, + reference: None, + extensions: BTreeMap::new(), + }) + } +} + /// Create-time parameters for a [`Disk`](omicron_common::api::external::Disk) #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct DiskCreate { @@ -242,18 +303,18 @@ pub struct DiskCreate { /// total size of the Disk in bytes pub size: ByteCount, /// size of blocks for this Disk. valid values are: 512, 2048, or 4096 - pub block_size: ByteCount, + pub block_size: BlockSize, } const EXTENT_SIZE: u32 = 1_u32 << 20; impl DiskCreate { pub fn block_size(&self) -> ByteCount { - self.block_size + ByteCount::from(self.block_size.0) } pub fn blocks_per_extent(&self) -> i64 { - EXTENT_SIZE as i64 / i64::from(self.block_size) + EXTENT_SIZE as i64 / i64::from(self.block_size.0) } pub fn extent_count(&self) -> i64 { @@ -340,7 +401,7 @@ mod test { snapshot_id: None, image_id: None, size, - block_size: ByteCount::from(4096), + block_size: BlockSize::try_from(4096).unwrap(), } } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index d3bff96adf8..23d9cbf4d54 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -775,7 +775,7 @@ impl Nexus { // Reject disks where the block size doesn't evenly divide the total // size - if (params.size.to_bytes() % params.block_size.to_bytes()) != 0 { + if (params.size.to_bytes() % params.block_size().to_bytes()) != 0 { return Err(Error::InvalidValue { label: String::from("size and block_size"), message: String::from( diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index a4d62e4f34a..a6c9fc655d9 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -135,7 +135,7 @@ pub async fn create_disk( snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(1), - block_size: ByteCount::from(512), + block_size: params::BlockSize::try_from(512).unwrap(), }, ) .await diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 4c7915e0316..7c5bdf87be6 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -270,7 +270,7 @@ async fn test_disk_create_disk_that_already_exists_fails( snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(1), - block_size: ByteCount::from(512), + block_size: params::BlockSize::try_from(512).unwrap(), }; let _ = create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await; let disk_url = format!("{}/{}", disks_url, DISK_NAME); @@ -651,7 +651,7 @@ async fn test_disk_region_creation_failure( snapshot_id: None, image_id: None, size: disk_size, - block_size: ByteCount::from(512), + block_size: params::BlockSize::try_from(512).unwrap(), }; // Unfortunately, the error message is only posted internally to the @@ -723,7 +723,7 @@ async fn test_disk_invalid_block_size_rejected( snapshot_id: None, image_id: None, size: disk_size, - block_size: ByteCount::from(1024), + block_size: params::BlockSize(1024), }; NexusRequest::new( @@ -767,7 +767,7 @@ async fn test_disk_reject_total_size_not_divisible_by_block_size( snapshot_id: None, image_id: None, size: disk_size, - block_size: ByteCount::from(512), + block_size: params::BlockSize::try_from(512).unwrap(), }; NexusRequest::new( diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 87c4825f64e..81c3fb3b817 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -145,7 +145,7 @@ lazy_static! { snapshot_id: None, image_id: None, size: ByteCount::from_gibibytes_u32(16), - block_size: ByteCount::from(4096), + block_size: params::BlockSize::try_from(4096).unwrap(), }; // Instance used for testing diff --git a/openapi/nexus.json b/openapi/nexus.json index b2f92c61035..9beb4e4bac6 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4749,6 +4749,15 @@ } }, "schemas": { + "BlockSize": { + "title": "disk block size in bytes", + "type": "integer", + "enum": [ + 512, + 2048, + 4096 + ] + }, "ByteCount": { "description": "A count of bytes, typically used either for memory or storage capacity\n\nThe maximum supported byte count is [`i64::MAX`]. This makes it somewhat inconvenient to define constructors: a u32 constructor can be infallible, but an i64 constructor can fail (if the value is negative) and a u64 constructor can fail (if the value is larger than i64::MAX). We provide all of these for consumers' convenience.", "type": "integer", @@ -4849,7 +4858,7 @@ "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096", "allOf": [ { - "$ref": "#/components/schemas/ByteCount" + "$ref": "#/components/schemas/BlockSize" } ] }, @@ -5370,7 +5379,7 @@ "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096", "allOf": [ { - "$ref": "#/components/schemas/ByteCount" + "$ref": "#/components/schemas/BlockSize" } ] },