diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 527327450aa..82ea196a507 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -755,7 +755,9 @@ pub struct Disk { pub identity: IdentityMetadata, 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 b67212eb2d8..474e87be932 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 ( + '512', + '2048', + '4096' +); + CREATE TABLE omicron.public.disk ( /* Identity metadata (resource) */ id UUID PRIMARY KEY, @@ -400,7 +406,9 @@ CREATE TABLE omicron.public.disk ( /* Disk configuration */ size_bytes INT NOT NULL, - origin_snapshot UUID + block_size omicron.public.block_size 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 f1893f4c25f..e1e56f37093 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3052,7 +3052,9 @@ mod test { description: name.to_string(), }, snapshot_id: None, + image_id: None, size, + block_size: params::BlockSize::try_from(4096).unwrap(), } } diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index 4d65040ad05..b1ab41bdf89 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,49 @@ 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"512" + Iso => b"2048" + AdvancedFormat => b"4096" +); + +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 TryFrom for BlockSize { + type Error = anyhow::Error; + 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.0), + } + } +} + /// A Disk (network block device). #[derive( Queryable, @@ -1313,10 +1356,19 @@ pub struct Disk { /// size of the Disk #[column_name = "size_bytes"] pub 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) #[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 { @@ -1326,17 +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.try_into()?, create_snapshot_id: params.snapshot_id, - } + create_image_id: params.image_id, + }) } pub fn state(&self) -> DiskState { @@ -1360,7 +1414,9 @@ impl Into for Disk { identity: self.identity(), 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 a102de7c298..e6d75af967b 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 -> 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 ee44b406e63..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 { @@ -236,21 +297,24 @@ 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? - /// size of the Disk + pub snapshot_id: Option, + /// id for image from which the Disk should be created, if any + pub image_id: Option, + /// 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: BlockSize, } -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) + ByteCount::from(self.block_size.0) } pub fn blocks_per_extent(&self) -> i64 { - EXTENT_SIZE as i64 / BLOCK_SIZE as i64 + EXTENT_SIZE as i64 / i64::from(self.block_size.0) } pub fn extent_count(&self) -> i64 { @@ -335,7 +399,9 @@ mod test { description: "desc".to_string(), }, snapshot_id: None, + image_id: None, size, + block_size: BlockSize::try_from(4096).unwrap(), } } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index b96c0072d63..23d9cbf4d54 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -773,8 +773,19 @@ impl Nexus { .lookup_for(authz::Action::CreateChild) .await?; - // Until we implement snapshots, do not allow disks to be created with a - // snapshot id. + // 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() { return Err(Error::InvalidValue { label: String::from("snapshot_id"), @@ -782,6 +793,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 176d5a6367b..ba3c0f8c7a4 100644 --- a/nexus/src/sagas.rs +++ b/nexus/src/sagas.rs @@ -1019,18 +1019,24 @@ 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, 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() .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..a6c9fc655d9 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: params::BlockSize::try_from(512).unwrap(), }, ) .await diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index d32ccc52cac..7c5bdf87be6 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: 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); @@ -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: params::BlockSize::try_from(512).unwrap(), }; // Unfortunately, the error message is only posted internally to the @@ -685,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: params::BlockSize(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: params::BlockSize::try_from(512).unwrap(), + }; + + 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) @@ -724,7 +820,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 28314c98d0b..81c3fb3b817 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -143,7 +143,9 @@ lazy_static! { description: "".parse().unwrap(), }, snapshot_id: None, + image_id: None, size: ByteCount::from_gibibytes_u32(16), + block_size: params::BlockSize::try_from(4096).unwrap(), }; // Instance used for testing diff --git a/openapi/nexus.json b/openapi/nexus.json index 876d96bd117..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", @@ -4774,6 +4783,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" @@ -4786,6 +4798,11 @@ "type": "string", "format": "uuid" }, + "image_id": { + "nullable": true, + "type": "string", + "format": "uuid" + }, "name": { "description": "unique, mutable, user-controlled identifier for each resource", "allOf": [ @@ -4821,6 +4838,7 @@ } }, "required": [ + "block_size", "description", "device_path", "id", @@ -4836,14 +4854,28 @@ "description": "Create-time parameters for a [`Disk`](omicron_common::api::external::Disk)", "type": "object", "properties": { + "block_size": { + "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096", + "allOf": [ + { + "$ref": "#/components/schemas/BlockSize" + } + ] + }, "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" }, "size": { - "description": "size of the Disk", + "description": "total size of the Disk in bytes", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -4858,6 +4890,7 @@ } }, "required": [ + "block_size", "description", "name", "size" @@ -5342,14 +5375,28 @@ "description": "During instance creation, create and attach disks", "type": "object", "properties": { + "block_size": { + "description": "size of blocks for this Disk. valid values are: 512, 2048, or 4096", + "allOf": [ + { + "$ref": "#/components/schemas/BlockSize" + } + ] + }, "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" }, "size": { - "description": "size of the Disk", + "description": "total size of the Disk in bytes", "allOf": [ { "$ref": "#/components/schemas/ByteCount" @@ -5370,6 +5417,7 @@ } }, "required": [ + "block_size", "description", "name", "size",