From e7b9805c3094c53c14e9282890f2c49e9893bb82 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 7 Jul 2023 12:19:21 -0700 Subject: [PATCH 1/8] Add support for 'disabling' physical disks to prevent future allocations --- common/src/sql/dbinit.sql | 17 + nexus/db-model/src/lib.rs | 2 + nexus/db-model/src/physical_disk.rs | 5 +- nexus/db-model/src/physical_disk_state.rs | 34 ++ .../db-model/src/queries/region_allocation.rs | 4 +- nexus/db-model/src/schema.rs | 1 + nexus/db-queries/src/db/datastore/mod.rs | 407 ++++++++++++------ .../src/db/datastore/physical_disk.rs | 30 ++ .../src/db/queries/region_allocation.rs | 17 +- nexus/src/app/sled.rs | 32 ++ nexus/src/external_api/http_entrypoints.rs | 28 ++ nexus/tests/integration_tests/sleds.rs | 35 +- nexus/tests/output/nexus_tags.txt | 1 + nexus/types/src/external_api/params.rs | 18 + nexus/types/src/external_api/views.rs | 13 + openapi/nexus.json | 104 +++++ 16 files changed, 619 insertions(+), 129 deletions(-) create mode 100644 nexus/db-model/src/physical_disk_state.rs diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 0ee95b4310..b4b06d5dfa 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -239,6 +239,20 @@ CREATE TYPE omicron.public.physical_disk_kind AS ENUM ( 'u2' ); +CREATE TYPE omicron.public.physical_disk_state AS ENUM ( + -- The disk is actively being used, and should be a target + -- for future allocations. + 'active', + -- The disk may still be in usage, but should not be used + -- for subsequent allocations. + -- + -- This state could be set when we have, for example, datasets + -- actively being used by the disk which we haven't fully retired. + 'draining', + -- The disk is not currently being used. + 'inactive' +); + -- A physical disk which exists inside the rack. CREATE TABLE omicron.public.physical_disk ( id UUID PRIMARY KEY, @@ -256,6 +270,9 @@ CREATE TABLE omicron.public.physical_disk ( -- FK into the Sled table sled_id UUID NOT NULL, + -- Describes how the control plane manages this disk + state omicron.public.physical_disk_state NOT NULL, + -- This constraint should be upheld, even for deleted disks -- in the fleet. CONSTRAINT vendor_serial_model_unique UNIQUE ( diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index 8854133359..8780ee3c9c 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -41,6 +41,7 @@ mod network_interface; mod oximeter_info; mod physical_disk; mod physical_disk_kind; +mod physical_disk_state; mod producer_endpoint; mod project; mod semver_version; @@ -126,6 +127,7 @@ pub use network_interface::*; pub use oximeter_info::*; pub use physical_disk::*; pub use physical_disk_kind::*; +pub use physical_disk_state::*; pub use producer_endpoint::*; pub use project::*; pub use rack::*; diff --git a/nexus/db-model/src/physical_disk.rs b/nexus/db-model/src/physical_disk.rs index 93afdba89c..dbf752c4db 100644 --- a/nexus/db-model/src/physical_disk.rs +++ b/nexus/db-model/src/physical_disk.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use super::{Generation, PhysicalDiskKind}; +use super::{Generation, PhysicalDiskKind, PhysicalDiskState}; use crate::collection::DatastoreCollectionConfig; use crate::schema::{physical_disk, zpool}; use chrono::{DateTime, Utc}; @@ -25,6 +25,7 @@ pub struct PhysicalDisk { pub variant: PhysicalDiskKind, pub sled_id: Uuid, + pub state: PhysicalDiskState, } impl PhysicalDisk { @@ -44,6 +45,7 @@ impl PhysicalDisk { model, variant, sled_id, + state: PhysicalDiskState::Active, } } @@ -75,6 +77,7 @@ impl From for views::PhysicalDisk { serial: disk.serial, model: disk.model, disk_type: disk.variant.into(), + state: disk.state.into(), } } } diff --git a/nexus/db-model/src/physical_disk_state.rs b/nexus/db-model/src/physical_disk_state.rs new file mode 100644 index 0000000000..2129a54a8f --- /dev/null +++ b/nexus/db-model/src/physical_disk_state.rs @@ -0,0 +1,34 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use super::impl_enum_type; +use nexus_types::external_api::views; +use serde::{Deserialize, Serialize}; + +impl_enum_type!( + #[derive(Clone, SqlType, Debug, QueryId)] + #[diesel(postgres_type(name = "physical_disk_state"))] + pub struct PhysicalDiskStateEnum; + + #[derive(Clone, Copy, Debug, AsExpression, FromSqlRow, Serialize, Deserialize, PartialEq)] + #[diesel(sql_type = PhysicalDiskStateEnum)] + pub enum PhysicalDiskState; + + // Enum values + Active => b"active" + Draining => b"draining" + Inactive => b"inactive" +); + +impl From for views::PhysicalDiskState { + fn from(state: PhysicalDiskState) -> Self { + use views::PhysicalDiskState as api; + use PhysicalDiskState as db; + match state { + db::Active => api::Active, + db::Draining => api::Draining, + db::Inactive => api::Inactive, + } + } +} diff --git a/nexus/db-model/src/queries/region_allocation.rs b/nexus/db-model/src/queries/region_allocation.rs index b150b05377..35ee537ea5 100644 --- a/nexus/db-model/src/queries/region_allocation.rs +++ b/nexus/db-model/src/queries/region_allocation.rs @@ -23,6 +23,7 @@ // a CTE (where we want the alias name to come first). use crate::schema::dataset; +use crate::schema::physical_disk; use crate::schema::zpool; table! { @@ -147,10 +148,11 @@ diesel::allow_tables_to_appear_in_same_query!( do_insert, candidate_regions, dataset, + physical_disk, zpool, ); -diesel::allow_tables_to_appear_in_same_query!(candidate_zpools, dataset,); +diesel::allow_tables_to_appear_in_same_query!(candidate_zpools, dataset); diesel::allow_tables_to_appear_in_same_query!( old_zpool_usage, diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index dcbd9f4c8a..1a53d3c811 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -746,6 +746,7 @@ table! { variant -> crate::PhysicalDiskKindEnum, sled_id -> Uuid, + state -> crate::PhysicalDiskStateEnum, } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 882e79f7cb..a0b70c3a6f 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -532,10 +532,6 @@ mod test { sled_id } - fn test_zpool_size() -> ByteCount { - ByteCount::from_gibibytes_u32(100) - } - const TEST_VENDOR: &str = "test-vendor"; const TEST_SERIAL: &str = "test-serial"; const TEST_MODEL: &str = "test-model"; @@ -548,7 +544,7 @@ mod test { ) -> Uuid { let physical_disk = PhysicalDisk::new( TEST_VENDOR.into(), - TEST_SERIAL.into(), + format!("{TEST_SERIAL}-{}", Uuid::new_v4()), TEST_MODEL.into(), kind, sled_id, @@ -565,14 +561,11 @@ mod test { datastore: &DataStore, sled_id: Uuid, physical_disk_id: Uuid, + size: ByteCount, ) -> Uuid { let zpool_id = Uuid::new_v4(); - let zpool = Zpool::new( - zpool_id, - sled_id, - physical_disk_id, - test_zpool_size().into(), - ); + let zpool = + Zpool::new(zpool_id, sled_id, physical_disk_id, size.into()); datastore.zpool_upsert(zpool).await.unwrap(); zpool_id } @@ -593,38 +586,140 @@ mod test { } } + struct RegionAllocationTestCtxBuilder { + sleds: usize, + // It's assumed that we have one zpool per disk + disks_per_sled: usize, + zpool_size: ByteCount, + datasets_per_zpool: usize, + } + + impl RegionAllocationTestCtxBuilder { + pub fn sleds(mut self, s: usize) -> Self { + self.sleds = s; + self + } + + pub fn disks_per_sled(mut self, d: usize) -> Self { + self.disks_per_sled = d; + self + } + + pub fn zpool_size(mut self, b: ByteCount) -> Self { + self.zpool_size = b; + self + } + + pub fn datasets_per_zpool(mut self, d: usize) -> Self { + self.datasets_per_zpool = d; + self + } + } + + impl Default for RegionAllocationTestCtxBuilder { + fn default() -> Self { + Self { + sleds: 1, + disks_per_sled: 1, + zpool_size: ByteCount::from_gibibytes_u32(100), + datasets_per_zpool: 1, + } + } + } + + impl RegionAllocationTestCtxBuilder { + async fn build( + &self, + opctx: &OpContext, + datastore: &Arc, + ) -> RegionAllocationTestCtx { + let mut sleds = vec![]; + + for _ in 0..self.sleds { + // Create a sled... + let mut sled = SledInfo { + id: create_test_sled(&datastore).await, + disks: vec![], + }; + + for _ in 0..self.disks_per_sled { + // ... and a disk on that sled... + let physical_disk_id = create_test_physical_disk( + &datastore, + opctx, + sled.id, + PhysicalDiskKind::U2, + ) + .await; + + // ... and a zpool within that disk... + let zpool_id = create_test_zpool( + &datastore, + sled.id, + physical_disk_id, + self.zpool_size, + ) + .await; + + let mut disk = DiskInfo { + id: physical_disk_id, + zpool_id, + datasets: vec![], + }; + + for _ in 0..self.datasets_per_zpool { + // ... and datasets within that zpool. + let bogus_addr = + SocketAddrV6::new(Ipv6Addr::LOCALHOST, 8080, 0, 0); + let dataset_id = Uuid::new_v4(); + let dataset = Dataset::new( + dataset_id, + zpool_id, + bogus_addr, + DatasetKind::Crucible, + ); + datastore.dataset_upsert(dataset).await.unwrap(); + disk.datasets.push(dataset_id); + } + sled.disks.push(disk); + } + sleds.push(sled); + } + + RegionAllocationTestCtx { sleds } + } + } + + struct DiskInfo { + id: Uuid, + #[allow(unused)] + zpool_id: Uuid, + #[allow(unused)] + datasets: Vec, + } + + struct SledInfo { + id: Uuid, + disks: Vec, + } + + struct RegionAllocationTestCtx { + sleds: Vec, + } + #[tokio::test] async fn test_region_allocation() { let logctx = dev::test_setup_log("test_region_allocation"); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; - // Create a sled... - let sled_id = create_test_sled(&datastore).await; - - // ... and a disk on that sled... - let physical_disk_id = create_test_physical_disk( - &datastore, - &opctx, - sled_id, - PhysicalDiskKind::U2, - ) - .await; - - // ... and a zpool within that disk... - let zpool_id = - create_test_zpool(&datastore, sled_id, physical_disk_id).await; - - // ... and datasets within that zpool. - let dataset_count = REGION_REDUNDANCY_THRESHOLD * 2; - let bogus_addr = SocketAddrV6::new(Ipv6Addr::LOCALHOST, 8080, 0, 0); - let dataset_ids: Vec = - (0..dataset_count).map(|_| Uuid::new_v4()).collect(); - for id in &dataset_ids { - let dataset = - Dataset::new(*id, zpool_id, bogus_addr, DatasetKind::Crucible); - datastore.dataset_upsert(dataset).await.unwrap(); - } + let _testctx = RegionAllocationTestCtxBuilder::default() + .sleds(1) + .disks_per_sled(REGION_REDUNDANCY_THRESHOLD * 2) + .zpool_size(ByteCount::from_gibibytes_u32(100)) + .datasets_per_zpool(1) + .build(&opctx, &datastore) + .await; // Allocate regions from the datasets for this disk. let params = create_test_disk_create_params( @@ -704,32 +799,14 @@ mod test { let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; - // Create a sled... - let sled_id = create_test_sled(&datastore).await; - - // ... and a disk on that sled... - let physical_disk_id = create_test_physical_disk( - &datastore, - &opctx, - sled_id, - PhysicalDiskKind::U2, - ) - .await; - - // ... and a zpool within that disk... - let zpool_id = - create_test_zpool(&datastore, sled_id, physical_disk_id).await; - - // ... and datasets within that zpool. - let dataset_count = REGION_REDUNDANCY_THRESHOLD; - let bogus_addr = SocketAddrV6::new(Ipv6Addr::LOCALHOST, 8080, 0, 0); - let dataset_ids: Vec = - (0..dataset_count).map(|_| Uuid::new_v4()).collect(); - for id in &dataset_ids { - let dataset = - Dataset::new(*id, zpool_id, bogus_addr, DatasetKind::Crucible); - datastore.dataset_upsert(dataset).await.unwrap(); - } + let disk_count = REGION_REDUNDANCY_THRESHOLD; + let _testctx = RegionAllocationTestCtxBuilder::default() + .sleds(1) + .disks_per_sled(disk_count) + .zpool_size(ByteCount::from_gibibytes_u32(100)) + .datasets_per_zpool(1) + .build(&opctx, &datastore) + .await; // Allocate regions from the datasets for this volume. let params = create_test_disk_create_params( @@ -786,32 +863,13 @@ mod test { let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; - // Create a sled... - let sled_id = create_test_sled(&datastore).await; - - // ... and a disk on that sled... - let physical_disk_id = create_test_physical_disk( - &datastore, - &opctx, - sled_id, - PhysicalDiskKind::U2, - ) - .await; - - // ... and a zpool within that disk... - let zpool_id = - create_test_zpool(&datastore, sled_id, physical_disk_id).await; - - // ... and datasets within that zpool. - let dataset_count = REGION_REDUNDANCY_THRESHOLD - 1; - let bogus_addr = SocketAddrV6::new(Ipv6Addr::LOCALHOST, 8080, 0, 0); - let dataset_ids: Vec = - (0..dataset_count).map(|_| Uuid::new_v4()).collect(); - for id in &dataset_ids { - let dataset = - Dataset::new(*id, zpool_id, bogus_addr, DatasetKind::Crucible); - datastore.dataset_upsert(dataset).await.unwrap(); - } + let _testctx = RegionAllocationTestCtxBuilder::default() + .sleds(1) + .disks_per_sled(REGION_REDUNDANCY_THRESHOLD - 1) + .zpool_size(ByteCount::from_gibibytes_u32(100)) + .datasets_per_zpool(1) + .build(&opctx, &datastore) + .await; // Allocate regions from the datasets for this volume. let params = create_test_disk_create_params( @@ -842,56 +900,159 @@ mod test { } #[tokio::test] - async fn test_region_allocation_out_of_space_fails() { - let logctx = - dev::test_setup_log("test_region_allocation_out_of_space_fails"); + async fn test_region_allocation_out_of_space_fails_one_disk() { + let logctx = dev::test_setup_log( + "test_region_allocation_out_of_space_fails_one_disk", + ); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; - // Create a sled... - let sled_id = create_test_sled(&datastore).await; + let zpool_size = ByteCount::from_gibibytes_u32(100); + let _testctx = RegionAllocationTestCtxBuilder::default() + .sleds(1) + .disks_per_sled(1) + .zpool_size(zpool_size) + // The Zpool's space is shared by all these datasets. + // + // So even though we have enough datasets, we don't have enough + // space. + .datasets_per_zpool(REGION_REDUNDANCY_THRESHOLD) + .build(&opctx, &datastore) + .await; - // ... and a disk on that sled... - let physical_disk_id = create_test_physical_disk( - &datastore, - &opctx, - sled_id, - PhysicalDiskKind::U2, - ) - .await; - - // ... and a zpool within that disk... - let zpool_id = - create_test_zpool(&datastore, sled_id, physical_disk_id).await; - - // ... and datasets within that zpool. - let dataset_count = REGION_REDUNDANCY_THRESHOLD; - let bogus_addr = SocketAddrV6::new(Ipv6Addr::LOCALHOST, 8080, 0, 0); - let dataset_ids: Vec = - (0..dataset_count).map(|_| Uuid::new_v4()).collect(); - for id in &dataset_ids { - let dataset = - Dataset::new(*id, zpool_id, bogus_addr, DatasetKind::Crucible); - datastore.dataset_upsert(dataset).await.unwrap(); - } + // Allocate regions from the datasets for this disk. + // + // Note that we ask for a disk which is as large as the zpool, + // so we shouldn't have space for redundancy. + let params = create_test_disk_create_params("disk1", zpool_size); + let volume1_id = Uuid::new_v4(); + + datastore + .region_allocate( + &opctx, + volume1_id, + ¶ms.disk_source, + params.size, + ) + .await + .unwrap_err(); + + let _ = db.cleanup().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_region_allocation_out_of_space_fails_many_sleds() { + let logctx = dev::test_setup_log( + "test_region_allocation_out_of_space_fails_many_sleds", + ); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let zpool_size = ByteCount::from_gibibytes_u32(100); + let disk_size = ByteCount::from_gibibytes_u32(101); + let _testctx = RegionAllocationTestCtxBuilder::default() + .sleds(REGION_REDUNDANCY_THRESHOLD) + .disks_per_sled(1) + .zpool_size(zpool_size) + .datasets_per_zpool(1) + .build(&opctx, &datastore) + .await; // Allocate regions from the datasets for this disk. // // Note that we ask for a disk which is as large as the zpool, // so we shouldn't have space for redundancy. - let disk_size = test_zpool_size(); let params = create_test_disk_create_params("disk1", disk_size); let volume1_id = Uuid::new_v4(); - assert!(datastore + datastore .region_allocate( &opctx, volume1_id, ¶ms.disk_source, - params.size + params.size, + ) + .await + .unwrap_err(); + + let _ = db.cleanup().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_region_allocation_ignores_inactive_disks() { + let logctx = dev::test_setup_log( + "test_region_allocation_ignores_inactive_disks", + ); + let mut db = test_setup_database(&logctx.log).await; + let (opctx, datastore) = datastore_test(&logctx, &db).await; + + let zpool_size = ByteCount::from_gibibytes_u32(100); + let disk_size = ByteCount::from_mebibytes_u32(100); + let testctx = RegionAllocationTestCtxBuilder::default() + .sleds(1) + .disks_per_sled(REGION_REDUNDANCY_THRESHOLD) + .zpool_size(zpool_size) + .datasets_per_zpool(1) + .build(&opctx, &datastore) + .await; + + // Allocate one disk, observe that it works. + let params = create_test_disk_create_params("disk1", disk_size); + let volume_id = Uuid::new_v4(); + datastore + .region_allocate( + &opctx, + volume_id, + ¶ms.disk_source, + params.size, + ) + .await + .unwrap(); + + // First, find the disk which we plan on deactivating. + let disk_to_disable = testctx.sleds[0].disks[0].id; + let disk = datastore + .physical_disk_list( + &opctx, + &DataPageParams:: { + marker: None, + direction: dropshot::PaginationOrder::Ascending, + limit: std::num::NonZeroU32::new(1024).unwrap(), + }, ) .await - .is_err()); + .unwrap() + .into_iter() + .find(|disk| disk.uuid() == disk_to_disable) + .unwrap(); + + let (.., physical_disk_authz) = LookupPath::new(&opctx, &datastore) + .physical_disk(&disk.vendor, &disk.serial, &disk.model) + .lookup_for(authz::Action::Modify) + .await + .unwrap(); + + // Deactivate the disk. + datastore + .physical_disk_deactivate(&opctx, &physical_disk_authz) + .await + .unwrap(); + + // After marking a disk as non-active, the provision fails, since + // one of the necessary disks cannot be used. + let params = create_test_disk_create_params("disk2", disk_size); + let volume_id = Uuid::new_v4(); + datastore + .region_allocate( + &opctx, + volume_id, + ¶ms.disk_source, + params.size, + ) + .await + .unwrap_err(); let _ = db.cleanup().await; logctx.cleanup_successful(); @@ -1415,10 +1576,10 @@ mod test { ); // Deleting a non-existing record fails - assert!(datastore + datastore .deallocate_external_ip(&opctx, Uuid::nil()) .await - .is_err()); + .unwrap_err(); db.cleanup().await.unwrap(); logctx.cleanup_successful(); diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index ec9f29d27d..1dcad34aa6 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -13,6 +13,7 @@ use crate::db::collection_insert::DatastoreCollection; use crate::db::error::public_error_from_diesel_pool; use crate::db::error::ErrorHandler; use crate::db::model::PhysicalDisk; +use crate::db::model::PhysicalDiskState; use crate::db::model::Sled; use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; @@ -26,6 +27,7 @@ use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupType; use omicron_common::api::external::ResourceType; +use omicron_common::api::external::UpdateResult; use uuid::Uuid; impl DataStore { @@ -107,6 +109,34 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } + pub async fn physical_disk_deactivate( + &self, + opctx: &OpContext, + authz_physical_disk: &authz::PhysicalDisk, + ) -> UpdateResult { + opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; + use db::schema::physical_disk::dsl; + + let (vendor, serial, model) = authz_physical_disk.id(); + + diesel::update(dsl::physical_disk) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::vendor.eq(vendor)) + .filter(dsl::serial.eq(serial)) + .filter(dsl::model.eq(model)) + .filter(dsl::state.eq(PhysicalDiskState::Active)) + .set(dsl::state.eq(PhysicalDiskState::Draining)) + .returning(PhysicalDisk::as_returning()) + .get_result_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_physical_disk), + ) + }) + } + /// Deletes a disk from the database. pub async fn physical_disk_delete( &self, diff --git a/nexus/db-queries/src/db/queries/region_allocation.rs b/nexus/db-queries/src/db/queries/region_allocation.rs index 4c76689cff..b78baf94f3 100644 --- a/nexus/db-queries/src/db/queries/region_allocation.rs +++ b/nexus/db-queries/src/db/queries/region_allocation.rs @@ -6,7 +6,7 @@ use crate::db::alias::ExpressionAlias; use crate::db::datastore::REGION_REDUNDANCY_THRESHOLD; -use crate::db::model::{Dataset, DatasetKind, Region}; +use crate::db::model::{Dataset, DatasetKind, PhysicalDiskState, Region}; use crate::db::pool::DbConnection; use crate::db::subquery::{AsQuerySource, Cte, CteBuilder, CteQuery}; use crate::db::true_or_cast_error::{matches_sentinel, TrueOrCastError}; @@ -88,13 +88,28 @@ struct CandidateDatasets { impl CandidateDatasets { fn new() -> Self { use crate::db::schema::dataset::dsl as dataset_dsl; + use crate::db::schema::physical_disk::dsl as physical_disk_dsl; + use crate::db::schema::zpool::dsl as zpool_dsl; Self { query: Box::new( dataset_dsl::dataset + // Only consider non-deleted datasets for Crucible .filter(dataset_dsl::time_deleted.is_null()) .filter(dataset_dsl::size_used.is_not_null()) .filter(dataset_dsl::kind.eq(DatasetKind::Crucible)) + // Only consider datasets from disks that are "Active" + .inner_join( + zpool_dsl::zpool + .on(zpool_dsl::id.eq(dataset_dsl::pool_id)), + ) + .inner_join(physical_disk_dsl::physical_disk.on( + physical_disk_dsl::id.eq(zpool_dsl::physical_disk_id), + )) + .filter( + physical_disk_dsl::state.eq(PhysicalDiskState::Active), + ) + // Sort by "least-space-used" first .order(dataset_dsl::size_used.asc()) .limit(REGION_REDUNDANCY_THRESHOLD.try_into().unwrap()) .select((dataset_dsl::id, dataset_dsl::pool_id)), diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index 2124760f6a..a01dd2d5c8 100644 --- a/nexus/src/app/sled.rs +++ b/nexus/src/app/sled.rs @@ -11,6 +11,7 @@ use crate::db::identity::Asset; use crate::db::lookup::LookupPath; use crate::db::model::DatasetKind; use crate::db::model::ServiceKind; +use crate::external_api::params; use crate::internal_api::params::{ PhysicalDiskDeleteRequest, PhysicalDiskPutRequest, SledAgentStartupInfo, SledRole, ZpoolPutRequest, @@ -21,6 +22,7 @@ use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; +use omicron_common::api::external::UpdateResult; use sled_agent_client::types::SetVirtualNetworkInterfaceHost; use sled_agent_client::Client as SledAgentClient; use std::net::SocketAddrV6; @@ -137,6 +139,18 @@ impl super::Nexus { // Physical disks + pub fn physical_disk_lookup<'a>( + &'a self, + opctx: &'a OpContext, + disk_selector: ¶ms::PhysicalDiskPath, + ) -> lookup::PhysicalDisk<'a> { + LookupPath::new(&opctx, &self.db_datastore).physical_disk( + &disk_selector.vendor, + &disk_selector.serial, + &disk_selector.model, + ) + } + pub async fn sled_list_physical_disks( &self, opctx: &OpContext, @@ -156,6 +170,24 @@ impl super::Nexus { self.db_datastore.physical_disk_list(&opctx, pagparams).await } + pub async fn physical_disk_update( + &self, + opctx: &OpContext, + physical_disk_lookup: &lookup::PhysicalDisk<'_>, + update_command: params::PhysicalDiskUpdate, + ) -> UpdateResult { + let (.., authz_physical_disk) = + physical_disk_lookup.lookup_for(authz::Action::Modify).await?; + + match update_command { + params::PhysicalDiskUpdate::Disable => { + self.db_datastore + .physical_disk_deactivate(&opctx, &authz_physical_disk) + .await + } + } + } + /// Upserts a physical disk into the database, updating it if it already exists. pub async fn upsert_physical_disk( &self, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index ae2240affd..01b3cf184f 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -211,6 +211,7 @@ pub fn external_api() -> NexusApiDescription { api.register(sled_instance_list)?; api.register(sled_physical_disk_list)?; api.register(physical_disk_list)?; + api.register(physical_disk_update)?; api.register(switch_list)?; api.register(switch_view)?; @@ -4275,6 +4276,33 @@ async fn physical_disk_list( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Update a physical disk's state +#[endpoint { + method = PUT, + path = "/v1/system/hardware/disks/{vendor}/{serial}/{model}", + tags = ["system/hardware"], +}] +async fn physical_disk_update( + rqctx: RequestContext>, + path_params: Path, + update_command: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + let update_command = update_command.into_inner(); + let path = path_params.into_inner(); + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + + let physical_disk_lookup = nexus.physical_disk_lookup(&opctx, &path); + let physical_disk = nexus + .physical_disk_update(&opctx, &physical_disk_lookup, update_command) + .await?; + Ok(HttpResponseOk(physical_disk.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + // Switches /// List switches diff --git a/nexus/tests/integration_tests/sleds.rs b/nexus/tests/integration_tests/sleds.rs index de585b7b2d..11703971d9 100644 --- a/nexus/tests/integration_tests/sleds.rs +++ b/nexus/tests/integration_tests/sleds.rs @@ -11,14 +11,15 @@ use nexus_test_utils::resource_helpers::create_instance; use nexus_test_utils::resource_helpers::create_physical_disk; use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils::resource_helpers::delete_physical_disk; +use nexus_test_utils::resource_helpers::object_put; use nexus_test_utils::resource_helpers::objects_list_page_authz; use nexus_test_utils::resource_helpers::populate_ip_pool; use nexus_test_utils::start_sled_agent; use nexus_test_utils::SLED_AGENT_UUID; use nexus_test_utils_macros::nexus_test; -use omicron_nexus::external_api::views::SledInstance; +use omicron_nexus::external_api::params::PhysicalDiskUpdate; use omicron_nexus::external_api::views::{ - PhysicalDisk, PhysicalDiskType, Sled, + PhysicalDisk, PhysicalDiskState, PhysicalDiskType, Sled, SledInstance, }; use omicron_nexus::internal_api::params as internal_params; use omicron_sled_agent::sim; @@ -39,6 +40,14 @@ async fn physical_disks_list( objects_list_page_authz::(client, url).await.items } +async fn physical_disks_update( + client: &ClientTestContext, + url: &str, + state: &PhysicalDiskUpdate, +) -> PhysicalDisk { + object_put::(client, url, state).await +} + async fn sled_instance_list( client: &ClientTestContext, url: &str, @@ -93,7 +102,7 @@ async fn test_sleds_list(cptestctx: &ControlPlaneTestContext) { } #[nexus_test] -async fn test_physical_disk_create_list_delete( +async fn test_physical_disk_create_list_disable_delete( cptestctx: &ControlPlaneTestContext, ) { let external_client = &cptestctx.external_client; @@ -125,6 +134,26 @@ async fn test_physical_disk_create_list_delete( assert_eq!(disks[0].serial, "s"); assert_eq!(disks[0].model, "m"); assert_eq!(disks[0].disk_type, PhysicalDiskType::External); + assert_eq!(disks[0].state, PhysicalDiskState::Active); + + // Disable the disk, marking it as "not-for-use". + let disk_url = format!("/v1/system/hardware/disks/v/s/m"); + let disk = physical_disks_update( + &external_client, + &disk_url, + &PhysicalDiskUpdate::Disable, + ) + .await; + assert_eq!(disk.vendor, "v"); + assert_eq!(disk.serial, "s"); + assert_eq!(disk.model, "m"); + assert_eq!(disk.disk_type, PhysicalDiskType::External); + assert_eq!(disk.state, PhysicalDiskState::Draining); + + // Confirm that listing the disks again shows this new state too + let disks = physical_disks_list(&external_client, &disks_url).await; + assert_eq!(disks.len(), 1); + assert_eq!(disks[0].state, PhysicalDiskState::Draining); // Delete that disk using the internal API, observe it in the external API delete_physical_disk(&internal_client, "v", "s", "m", sled_id).await; diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index ca2f737cb0..e372a11cb4 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -114,6 +114,7 @@ networking_switch_port_apply_settings POST /v1/system/hardware/switch-por networking_switch_port_clear_settings DELETE /v1/system/hardware/switch-port/{port}/settings networking_switch_port_list GET /v1/system/hardware/switch-port physical_disk_list GET /v1/system/hardware/disks +physical_disk_update PUT /v1/system/hardware/disks/{vendor}/{serial}/{model} rack_list GET /v1/system/hardware/racks rack_view GET /v1/system/hardware/racks/{rack_id} sled_instance_list GET /v1/system/hardware/sleds/{sled_id}/instances diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 976ad0cc69..d3c69ac2b6 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -75,6 +75,24 @@ pub struct SledSelector { pub sled: Uuid, } +#[derive(Serialize, Deserialize, JsonSchema)] +pub struct PhysicalDiskPath { + pub vendor: String, + pub serial: String, + pub model: String, +} + +/// Updateable properties of a `PhysicalDisk`. +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum PhysicalDiskUpdate { + /// Prevents the disk from being used for future provisioning. + /// + /// This does not immediately cause services to be evacuated from using + /// the underlying disk, that process may happen asynchronously. + Disable, +} + pub struct SwitchSelector { /// ID of the switch pub switch: Uuid, diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 2fe9450a7f..88fdecc64b 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -332,6 +332,18 @@ pub enum PhysicalDiskType { External, } +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum PhysicalDiskState { + /// The disk is actively in-use. + Active, + /// The disk has been marked for removal, and is transitioning + /// to the Inactive state. + Draining, + /// The disk is not in-use by the system. + Inactive, +} + /// View of a Physical Disk /// /// Physical disks reside in a particular sled and are used to store both @@ -349,6 +361,7 @@ pub struct PhysicalDisk { pub model: String, pub disk_type: PhysicalDiskType, + pub state: PhysicalDiskState, } // SILO USERS diff --git a/openapi/nexus.json b/openapi/nexus.json index e3c5558764..d390de9c59 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -3436,6 +3436,69 @@ } } }, + "/v1/system/hardware/disks/{vendor}/{serial}/{model}": { + "put": { + "tags": [ + "system/hardware" + ], + "summary": "Update a physical disk's state", + "operationId": "physical_disk_update", + "parameters": [ + { + "in": "path", + "name": "model", + "required": true, + "schema": { + "type": "string" + } + }, + { + "in": "path", + "name": "serial", + "required": true, + "schema": { + "type": "string" + } + }, + { + "in": "path", + "name": "vendor", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PhysicalDiskUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PhysicalDisk" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/system/hardware/racks": { "get": { "tags": [ @@ -10608,6 +10671,9 @@ "type": "string", "format": "uuid" }, + "state": { + "$ref": "#/components/schemas/PhysicalDiskState" + }, "time_created": { "description": "timestamp when this resource was created", "type": "string", @@ -10627,6 +10693,7 @@ "id", "model", "serial", + "state", "time_created", "time_modified", "vendor" @@ -10653,6 +10720,31 @@ "items" ] }, + "PhysicalDiskState": { + "oneOf": [ + { + "description": "The disk is actively in-use.", + "type": "string", + "enum": [ + "active" + ] + }, + { + "description": "The disk has been marked for removal, and is transitioning to the Inactive state.", + "type": "string", + "enum": [ + "draining" + ] + }, + { + "description": "The disk is not in-use by the system.", + "type": "string", + "enum": [ + "inactive" + ] + } + ] + }, "PhysicalDiskType": { "type": "string", "enum": [ @@ -10660,6 +10752,18 @@ "external" ] }, + "PhysicalDiskUpdate": { + "description": "Updateable properties of a `PhysicalDisk`.", + "oneOf": [ + { + "description": "Prevents the disk from being used for future provisioning.\n\nThis does not immediately cause services to be evacuated from using the underlying disk, that process may happen asynchronously.", + "type": "string", + "enum": [ + "disable" + ] + } + ] + }, "Project": { "description": "View of a Project", "type": "object", From 5abfb26c6b2e232c91eee7132c1cb10139a7a64f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 7 Jul 2023 12:29:19 -0700 Subject: [PATCH 2/8] clippy --- nexus/tests/integration_tests/sleds.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/tests/integration_tests/sleds.rs b/nexus/tests/integration_tests/sleds.rs index 11703971d9..2762c034f2 100644 --- a/nexus/tests/integration_tests/sleds.rs +++ b/nexus/tests/integration_tests/sleds.rs @@ -137,7 +137,7 @@ async fn test_physical_disk_create_list_disable_delete( assert_eq!(disks[0].state, PhysicalDiskState::Active); // Disable the disk, marking it as "not-for-use". - let disk_url = format!("/v1/system/hardware/disks/v/s/m"); + let disk_url = "/v1/system/hardware/disks/v/s/m".to_string(); let disk = physical_disks_update( &external_client, &disk_url, From 69f1bf75c045aa1070a5f972045a112abd511dbc Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 7 Jul 2023 15:21:42 -0700 Subject: [PATCH 3/8] Review feedback --- .../src/db/datastore/physical_disk.rs | 35 ++++++++++- nexus/src/app/sled.rs | 16 ++--- nexus/src/external_api/http_entrypoints.rs | 5 +- nexus/tests/integration_tests/sleds.rs | 3 +- nexus/tests/output/nexus_tags.txt | 2 +- nexus/types/src/external_api/params.rs | 4 +- openapi/nexus.json | 61 ++++--------------- 7 files changed, 63 insertions(+), 63 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index fcc80c0cbc..6af41f2f41 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -13,6 +13,7 @@ use crate::db::collection_insert::DatastoreCollection; use crate::db::error::public_error_from_diesel_pool; use crate::db::error::ErrorHandler; use crate::db::model::PhysicalDisk; +use crate::db::model::PhysicalDiskKind; use crate::db::model::PhysicalDiskState; use crate::db::model::Sled; use crate::db::pagination::paginated; @@ -31,6 +32,38 @@ use omicron_common::api::external::UpdateResult; use uuid::Uuid; impl DataStore { + /// - Sled Agents like to look up physical disks by "Vendor, Serial, Model" + /// - The external API likes to look up physical disks by UUID + /// - LookuPath objects are opinionated about how they perform lookups. They + /// support "primary keys" or "names", but they're opinionated about the + /// name objects being a single string. + /// + /// This function bridges that gap, by allowing the external API to + /// translate "UUID" type into a "Vendor, Serial, Model" type which can + /// be used internally. + pub async fn physical_disk_id_to_name_no_auth( + &self, + id: Uuid, + ) -> Result<(String, String, String), Error> { + use db::schema::physical_disk::dsl; + + dsl::physical_disk + .filter(dsl::time_deleted.is_null()) + .filter(dsl::id.eq(id)) + .select((dsl::vendor, dsl::serial, dsl::model)) + .get_result_async(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::PhysicalDisk, + LookupType::ById(id), + ), + ) + }) + } + /// Stores a new physical disk in the database. /// /// - If the Vendor, Serial, and Model fields are the same as an existing @@ -124,7 +157,7 @@ impl DataStore { .filter(dsl::vendor.eq(vendor)) .filter(dsl::serial.eq(serial)) .filter(dsl::model.eq(model)) - .filter(dsl::disk_type.eq(PhysicalDiskKind::U2)) + .filter(dsl::variant.eq(PhysicalDiskKind::U2)) .filter(dsl::state.eq(PhysicalDiskState::Active)) .set(dsl::state.eq(PhysicalDiskState::Draining)) .returning(PhysicalDisk::as_returning()) diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index a01dd2d5c8..080b78c623 100644 --- a/nexus/src/app/sled.rs +++ b/nexus/src/app/sled.rs @@ -139,16 +139,18 @@ impl super::Nexus { // Physical disks - pub fn physical_disk_lookup<'a>( + pub async fn physical_disk_lookup<'a>( &'a self, opctx: &'a OpContext, disk_selector: ¶ms::PhysicalDiskPath, - ) -> lookup::PhysicalDisk<'a> { - LookupPath::new(&opctx, &self.db_datastore).physical_disk( - &disk_selector.vendor, - &disk_selector.serial, - &disk_selector.model, - ) + ) -> Result, Error> { + let (v, s, m) = self + .db_datastore + .physical_disk_id_to_name_no_auth(disk_selector.id) + .await?; + + Ok(LookupPath::new(&opctx, &self.db_datastore) + .physical_disk(&v, &s, &m)) } pub async fn sled_list_physical_disks( diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index dbfe80466c..6a881d49fc 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -4274,7 +4274,7 @@ async fn physical_disk_list( /// Update a physical disk's state #[endpoint { method = PUT, - path = "/v1/system/hardware/disks/{vendor}/{serial}/{model}", + path = "/v1/system/hardware/disks/{id}", tags = ["system/hardware"], }] async fn physical_disk_update( @@ -4289,7 +4289,8 @@ async fn physical_disk_update( let path = path_params.into_inner(); let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let physical_disk_lookup = nexus.physical_disk_lookup(&opctx, &path); + let physical_disk_lookup = + nexus.physical_disk_lookup(&opctx, &path).await?; let physical_disk = nexus .physical_disk_update(&opctx, &physical_disk_lookup, update_command) .await?; diff --git a/nexus/tests/integration_tests/sleds.rs b/nexus/tests/integration_tests/sleds.rs index 1bc5ee7101..ace69da257 100644 --- a/nexus/tests/integration_tests/sleds.rs +++ b/nexus/tests/integration_tests/sleds.rs @@ -136,7 +136,8 @@ async fn test_physical_disk_create_list_disable_delete( assert_eq!(disks[0].state, PhysicalDiskState::Active); // Disable the disk, marking it as "not-for-use". - let disk_url = "/v1/system/hardware/disks/v/s/m".to_string(); + let disk_url = + format!("/v1/system/hardware/disks/{}", disks[0].identity.id); let disk = physical_disks_update( &external_client, &disk_url, diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index e372a11cb4..8ebcdc690a 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -114,7 +114,7 @@ networking_switch_port_apply_settings POST /v1/system/hardware/switch-por networking_switch_port_clear_settings DELETE /v1/system/hardware/switch-port/{port}/settings networking_switch_port_list GET /v1/system/hardware/switch-port physical_disk_list GET /v1/system/hardware/disks -physical_disk_update PUT /v1/system/hardware/disks/{vendor}/{serial}/{model} +physical_disk_update PUT /v1/system/hardware/disks/{id} rack_list GET /v1/system/hardware/racks rack_view GET /v1/system/hardware/racks/{rack_id} sled_instance_list GET /v1/system/hardware/sleds/{sled_id}/instances diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index c9400388b0..914347752f 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -77,9 +77,7 @@ pub struct SledSelector { #[derive(Serialize, Deserialize, JsonSchema)] pub struct PhysicalDiskPath { - pub vendor: String, - pub serial: String, - pub model: String, + pub id: Uuid, } /// Updateable properties of a `PhysicalDisk`. diff --git a/openapi/nexus.json b/openapi/nexus.json index d390de9c59..8c42facbc8 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -960,15 +960,6 @@ "description": "List images which are global or scoped to the specified project. The images are returned sorted by creation date, with the most recent images appearing first.", "operationId": "image_list", "parameters": [ - { - "in": "query", - "name": "include_silo_images", - "description": "Flag used to indicate if silo scoped images should be included when listing project images. Only valid when `project` is provided.", - "schema": { - "nullable": true, - "type": "boolean" - } - }, { "in": "query", "name": "limit", @@ -3436,7 +3427,7 @@ } } }, - "/v1/system/hardware/disks/{vendor}/{serial}/{model}": { + "/v1/system/hardware/disks/{id}": { "put": { "tags": [ "system/hardware" @@ -3446,26 +3437,11 @@ "parameters": [ { "in": "path", - "name": "model", - "required": true, - "schema": { - "type": "string" - } - }, - { - "in": "path", - "name": "serial", + "name": "id", "required": true, "schema": { - "type": "string" - } - }, - { - "in": "path", - "name": "vendor", - "required": true, - "schema": { - "type": "string" + "type": "string", + "format": "uuid" } } ], @@ -9433,14 +9409,6 @@ "description": "Create-time parameters for an `Image`", "type": "object", "properties": { - "block_size": { - "description": "block size in bytes", - "allOf": [ - { - "$ref": "#/components/schemas/BlockSize" - } - ] - }, "description": { "type": "string" }, @@ -9465,7 +9433,6 @@ } }, "required": [ - "block_size", "description", "name", "os", @@ -9500,6 +9467,14 @@ { "type": "object", "properties": { + "block_size": { + "description": "The block size in bytes", + "allOf": [ + { + "$ref": "#/components/schemas/BlockSize" + } + ] + }, "type": { "type": "string", "enum": [ @@ -9511,6 +9486,7 @@ } }, "required": [ + "block_size", "type", "url" ] @@ -10651,9 +10627,6 @@ "description": "View of a Physical Disk\n\nPhysical disks reside in a particular sled and are used to store both Instance Disk data as well as internal metadata.", "type": "object", "properties": { - "disk_type": { - "$ref": "#/components/schemas/PhysicalDiskType" - }, "id": { "description": "unique, immutable, system-controlled identifier for each resource", "type": "string", @@ -10689,7 +10662,6 @@ } }, "required": [ - "disk_type", "id", "model", "serial", @@ -10745,13 +10717,6 @@ } ] }, - "PhysicalDiskType": { - "type": "string", - "enum": [ - "internal", - "external" - ] - }, "PhysicalDiskUpdate": { "description": "Updateable properties of a `PhysicalDisk`.", "oneOf": [ From dddcdd89f800058135acb6723695686699f1e751 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 10 Jul 2023 12:13:14 -0600 Subject: [PATCH 4/8] Disk view, add auth test --- nexus/src/external_api/http_entrypoints.rs | 24 +++++++++++++++ nexus/test-utils/src/lib.rs | 1 + nexus/tests/integration_tests/endpoints.rs | 21 +++++++++++-- nexus/tests/output/nexus_tags.txt | 1 + openapi/nexus.json | 36 ++++++++++++++++++++++ 5 files changed, 81 insertions(+), 2 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 6a881d49fc..580c71f189 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -211,6 +211,7 @@ pub fn external_api() -> NexusApiDescription { api.register(sled_instance_list)?; api.register(sled_physical_disk_list)?; api.register(physical_disk_list)?; + api.register(physical_disk_view)?; api.register(physical_disk_update)?; api.register(switch_list)?; api.register(switch_view)?; @@ -4271,6 +4272,29 @@ async fn physical_disk_list( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Get a physical disk +#[endpoint { + method = GET, + path = "/v1/system/hardware/disks/{id}", + tags = ["system/hardware"], +}] +async fn physical_disk_view( + rqctx: RequestContext>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + + let (.., physical_disk) = + nexus.physical_disk_lookup(&opctx, &path).await?.fetch().await?; + Ok(HttpResponseOk(physical_disk.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// Update a physical disk's state #[endpoint { method = PUT, diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index 14203d90c3..956d5ff70d 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -55,6 +55,7 @@ pub const RACK_UUID: &str = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc"; pub const SWITCH_UUID: &str = "dae4e1f1-410e-4314-bff1-fec0504be07e"; pub const OXIMETER_UUID: &str = "39e6175b-4df2-4730-b11d-cbc1e60a2e78"; pub const PRODUCER_UUID: &str = "a6458b7d-87c3-4483-be96-854d814c20de"; +pub const PHYSICAL_DISK_UUID: &str = "2092c014-cb33-4654-b43e-d0958f855642"; /// The reported amount of hardware threads for an emulated sled agent. pub const TEST_HARDWARE_THREADS: u32 = 16; diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index a5ae612351..1475564662 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -13,6 +13,7 @@ use chrono::Utc; use http::method::Method; use lazy_static::lazy_static; use nexus_test_utils::resource_helpers::DiskTest; +use nexus_test_utils::PHYSICAL_DISK_UUID; use nexus_test_utils::RACK_UUID; use nexus_test_utils::SLED_AGENT_UUID; use nexus_test_utils::SWITCH_UUID; @@ -46,8 +47,10 @@ lazy_static! { format!("/v1/system/hardware/sleds/{}", SLED_AGENT_UUID); pub static ref HARDWARE_SWITCH_URL: String = format!("/v1/system/hardware/switches/{}", SWITCH_UUID); - pub static ref HARDWARE_DISK_URL: String = + pub static ref HARDWARE_DISKS_URL: String = format!("/v1/system/hardware/disks"); + pub static ref HARDWARE_DISK_URL: String = + format!("/v1/system/hardware/disks/{}", PHYSICAL_DISK_UUID); pub static ref HARDWARE_SLED_DISK_URL: String = format!("/v1/system/hardware/sleds/{}/disks", SLED_AGENT_UUID); @@ -1562,12 +1565,26 @@ lazy_static! { }, VerifyEndpoint { - url: &HARDWARE_DISK_URL, + url: &HARDWARE_DISKS_URL, visibility: Visibility::Public, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![AllowedMethod::Get], }, + VerifyEndpoint { + url: &HARDWARE_DISK_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Put( + serde_json::to_value( + params::PhysicalDiskUpdate::Disable, + ).unwrap() + ) + ], + }, + VerifyEndpoint { url: &HARDWARE_SLED_DISK_URL, visibility: Visibility::Public, diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 8ebcdc690a..5ac5cf8d45 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -115,6 +115,7 @@ networking_switch_port_clear_settings DELETE /v1/system/hardware/switch-por networking_switch_port_list GET /v1/system/hardware/switch-port physical_disk_list GET /v1/system/hardware/disks physical_disk_update PUT /v1/system/hardware/disks/{id} +physical_disk_view GET /v1/system/hardware/disks/{id} rack_list GET /v1/system/hardware/racks rack_view GET /v1/system/hardware/racks/{rack_id} sled_instance_list GET /v1/system/hardware/sleds/{sled_id}/instances diff --git a/openapi/nexus.json b/openapi/nexus.json index 9d63879317..444a257518 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -3428,6 +3428,42 @@ } }, "/v1/system/hardware/disks/{id}": { + "get": { + "tags": [ + "system/hardware" + ], + "summary": "Get a physical disk", + "operationId": "physical_disk_view", + "parameters": [ + { + "in": "path", + "name": "id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PhysicalDisk" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, "put": { "tags": [ "system/hardware" From d4796332dcbb98be90d0b3afef76d437a4cd6561 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 26 Dec 2023 16:50:41 -0800 Subject: [PATCH 5/8] Fix tests, update schema version, openapi spec --- nexus/db-model/src/schema.rs | 2 +- nexus/db-queries/src/db/datastore/mod.rs | 23 ++-- .../src/db/datastore/physical_disk.rs | 8 +- nexus/src/app/sled.rs | 2 +- nexus/tests/integration_tests/sleds.rs | 4 +- openapi/nexus.json | 125 ++++++++++++++++++ schema/crdb/1.0.0/up.sql | 17 --- schema/crdb/22.0.0/up01.sql | 13 ++ schema/crdb/22.0.0/up02.sql | 1 + schema/crdb/22.0.0/up03.sql | 1 + schema/crdb/dbinit.sql | 19 ++- 11 files changed, 177 insertions(+), 38 deletions(-) create mode 100644 schema/crdb/22.0.0/up01.sql create mode 100644 schema/crdb/22.0.0/up02.sql create mode 100644 schema/crdb/22.0.0/up03.sql diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 9852676022..58de0279c9 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -13,7 +13,7 @@ use omicron_common::api::external::SemverVersion; /// /// This should be updated whenever the schema is changed. For more details, /// refer to: schema/crdb/README.adoc -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(21, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(22, 0, 0); table! { disk (id) { diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index deffa633aa..468c68659a 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -854,7 +854,7 @@ mod test { datastore: Arc, number_of_sleds: usize, ) -> Vec { - RegionAllocationTestCtxBuilder::default() + let sleds = RegionAllocationTestCtxBuilder::default() .sleds(number_of_sleds) // create 9 disks per sled .disks_per_sled(9) @@ -862,15 +862,20 @@ mod test { .datasets_per_zpool(3) .build(&opctx, &datastore) .await - .sleds - .into_iter() - .map(|sled_info| { - TestDataset { - sled_id: sled_info.id, - dataset_id: sled_info.disks[0].datasets[0], + .sleds; + + let mut result = vec![]; + for sled_info in &sleds { + for disk in &sled_info.disks { + for dataset_id in &disk.datasets { + result.push(TestDataset { + sled_id: sled_info.id, + dataset_id: *dataset_id, + }); } - }) - .collect() + } + } + result } #[tokio::test] diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index 76cbebd1d6..f83dbd7ae4 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -47,9 +47,7 @@ impl DataStore { ) -> Result<(String, String, String), Error> { use db::schema::physical_disk::dsl; - let conn = self - .pool_connection_unauthorized() - .await?; + let conn = self.pool_connection_unauthorized().await?; dsl::physical_disk .filter(dsl::time_deleted.is_null()) @@ -158,9 +156,7 @@ impl DataStore { let (vendor, serial, model) = authz_physical_disk.id(); - let conn = self - .pool_connection_authorized(opctx) - .await?; + let conn = self.pool_connection_authorized(opctx).await?; diesel::update(dsl::physical_disk) .filter(dsl::time_deleted.is_null()) diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index 391c3bf03d..6984e9f194 100644 --- a/nexus/src/app/sled.rs +++ b/nexus/src/app/sled.rs @@ -4,11 +4,11 @@ //! Sleds, and the hardware and services within them. +use crate::external_api::params; use crate::internal_api::params::{ PhysicalDiskDeleteRequest, PhysicalDiskPutRequest, SledAgentStartupInfo, SledRole, ZpoolPutRequest, }; -use crate::external_api::params; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; diff --git a/nexus/tests/integration_tests/sleds.rs b/nexus/tests/integration_tests/sleds.rs index 8b04f6f5c7..a2cb54b558 100644 --- a/nexus/tests/integration_tests/sleds.rs +++ b/nexus/tests/integration_tests/sleds.rs @@ -17,9 +17,7 @@ use nexus_test_utils::resource_helpers::populate_ip_pool; use nexus_test_utils::start_sled_agent; use nexus_test_utils::SLED_AGENT_UUID; use nexus_test_utils_macros::nexus_test; -use nexus_types::external_api::params::{ - PhysicalDiskKind, PhysicalDiskUpdate, -}; +use nexus_types::external_api::params::{PhysicalDiskKind, PhysicalDiskUpdate}; use nexus_types::external_api::views::{ PhysicalDisk, PhysicalDiskState, Sled, SledInstance, }; diff --git a/openapi/nexus.json b/openapi/nexus.json index 4131460149..72c83d7c14 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -3604,6 +3604,90 @@ } } }, + "/v1/system/hardware/disks/{id}": { + "get": { + "tags": [ + "system/hardware" + ], + "summary": "Get a physical disk", + "operationId": "physical_disk_view", + "parameters": [ + { + "in": "path", + "name": "id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PhysicalDisk" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "tags": [ + "system/hardware" + ], + "summary": "Update a physical disk's state", + "operationId": "physical_disk_update", + "parameters": [ + { + "in": "path", + "name": "id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PhysicalDiskUpdate" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PhysicalDisk" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/v1/system/hardware/racks": { "get": { "tags": [ @@ -12892,6 +12976,9 @@ "type": "string", "format": "uuid" }, + "state": { + "$ref": "#/components/schemas/PhysicalDiskState" + }, "time_created": { "description": "timestamp when this resource was created", "type": "string", @@ -12911,6 +12998,7 @@ "id", "model", "serial", + "state", "time_created", "time_modified", "vendor" @@ -12945,6 +13033,43 @@ "items" ] }, + "PhysicalDiskState": { + "oneOf": [ + { + "description": "The disk is actively in-use.", + "type": "string", + "enum": [ + "active" + ] + }, + { + "description": "The disk has been marked for removal, and is transitioning to the Inactive state.", + "type": "string", + "enum": [ + "draining" + ] + }, + { + "description": "The disk is not in-use by the system.", + "type": "string", + "enum": [ + "inactive" + ] + } + ] + }, + "PhysicalDiskUpdate": { + "description": "Updateable properties of a `PhysicalDisk`.", + "oneOf": [ + { + "description": "Prevents the disk from being used for future provisioning.\n\nThis does not immediately cause services to be evacuated from using the underlying disk, that process may happen asynchronously.", + "type": "string", + "enum": [ + "disable" + ] + } + ] + }, "Ping": { "type": "object", "properties": { diff --git a/schema/crdb/1.0.0/up.sql b/schema/crdb/1.0.0/up.sql index 71f7bdaa67..d3c17009f6 100644 --- a/schema/crdb/1.0.0/up.sql +++ b/schema/crdb/1.0.0/up.sql @@ -212,20 +212,6 @@ CREATE TYPE IF NOT EXISTS omicron.public.physical_disk_kind AS ENUM ( 'u2' ); -CREATE TYPE omicron.public.physical_disk_state AS ENUM ( - -- The disk is actively being used, and should be a target - -- for future allocations. - 'active', - -- The disk may still be in usage, but should not be used - -- for subsequent allocations. - -- - -- This state could be set when we have, for example, datasets - -- actively being used by the disk which we haven't fully retired. - 'draining', - -- The disk is not currently being used. - 'inactive' -); - -- A physical disk which exists inside the rack. CREATE TABLE IF NOT EXISTS omicron.public.physical_disk ( id UUID PRIMARY KEY, @@ -243,9 +229,6 @@ CREATE TABLE IF NOT EXISTS omicron.public.physical_disk ( -- FK into the Sled table sled_id UUID NOT NULL, - -- Describes how the control plane manages this disk - state omicron.public.physical_disk_state NOT NULL, - -- This constraint should be upheld, even for deleted disks -- in the fleet. CONSTRAINT vendor_serial_model_unique UNIQUE ( diff --git a/schema/crdb/22.0.0/up01.sql b/schema/crdb/22.0.0/up01.sql new file mode 100644 index 0000000000..78edca8882 --- /dev/null +++ b/schema/crdb/22.0.0/up01.sql @@ -0,0 +1,13 @@ +CREATE TYPE IF NOT EXISTS omicron.public.physical_disk_state AS ENUM ( + -- The disk is actively being used, and should be a target + -- for future allocations. + 'active', + -- The disk may still be in usage, but should not be used + -- for subsequent allocations. + -- + -- This state could be set when we have, for example, datasets + -- actively being used by the disk which we haven't fully retired. + 'draining', + -- The disk is not currently being used. + 'inactive' +); diff --git a/schema/crdb/22.0.0/up02.sql b/schema/crdb/22.0.0/up02.sql new file mode 100644 index 0000000000..8e72f0d3d4 --- /dev/null +++ b/schema/crdb/22.0.0/up02.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.physical_disk ADD COLUMN IF NOT EXISTS state omicron.public.physical_disk_state NOT NULL DEFAULT 'active'; diff --git a/schema/crdb/22.0.0/up03.sql b/schema/crdb/22.0.0/up03.sql new file mode 100644 index 0000000000..4aa981f058 --- /dev/null +++ b/schema/crdb/22.0.0/up03.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.physical_disk ALTER COLUMN state DROP DEFAULT; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index cc61148048..4bcbef47cd 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -295,6 +295,20 @@ CREATE TYPE IF NOT EXISTS omicron.public.physical_disk_kind AS ENUM ( 'u2' ); +CREATE TYPE IF NOT EXISTS omicron.public.physical_disk_state AS ENUM ( + -- The disk is actively being used, and should be a target + -- for future allocations. + 'active', + -- The disk may still be in usage, but should not be used + -- for subsequent allocations. + -- + -- This state could be set when we have, for example, datasets + -- actively being used by the disk which we haven't fully retired. + 'draining', + -- The disk is not currently being used. + 'inactive' +); + -- A physical disk which exists inside the rack. CREATE TABLE IF NOT EXISTS omicron.public.physical_disk ( id UUID PRIMARY KEY, @@ -312,6 +326,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.physical_disk ( -- FK into the Sled table sled_id UUID NOT NULL, + -- Describes how the control plane manages this disk + state omicron.public.physical_disk_state NOT NULL, + -- This constraint should be upheld, even for deleted disks -- in the fleet. CONSTRAINT vendor_serial_model_unique UNIQUE ( @@ -3096,7 +3113,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '21.0.0', NULL) + ( TRUE, NOW(), NOW(), '22.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 10127d7c7bf5ae568ad8e5e0f1d28400400e8c3b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 27 Dec 2023 12:32:07 -0800 Subject: [PATCH 6/8] Deterministic UUIDs path auth tests --- Cargo.lock | 7 ++++ Cargo.toml | 2 +- nexus/db-model/src/lib.rs | 11 +++++ nexus/db-model/src/physical_disk.rs | 18 +++++++- nexus/db-queries/src/db/datastore/mod.rs | 2 +- .../src/db/datastore/physical_disk.rs | 2 +- nexus/db-queries/src/db/lookup.rs | 12 +++--- nexus/preprocessed_configs/config.xml | 41 +++++++++++++++++++ nexus/src/app/sled.rs | 11 +++-- nexus/src/external_api/http_entrypoints.rs | 4 +- nexus/test-utils/src/lib.rs | 8 +++- nexus/tests/integration_tests/endpoints.rs | 2 +- nexus/tests/output/nexus_tags.txt | 4 +- nexus/types/src/external_api/params.rs | 11 +---- openapi/nexus.json | 8 ++-- 15 files changed, 108 insertions(+), 35 deletions(-) create mode 100644 nexus/preprocessed_configs/config.xml diff --git a/Cargo.lock b/Cargo.lock index 3cdf3dd678..40fe529100 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7572,6 +7572,12 @@ dependencies = [ "digest", ] +[[package]] +name = "sha1_smol" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae1a47186c03a32177042e55dbc5fd5aee900b8e0069a8d70fba96a9375cd012" + [[package]] name = "sha2" version = "0.10.8" @@ -9468,6 +9474,7 @@ checksum = "5e395fcf16a7a3d8127ec99782007af141946b4795001f876d54fb0d55978560" dependencies = [ "getrandom 0.2.10", "serde", + "sha1_smol", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index d4f81b0310..8371ead1d7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -388,7 +388,7 @@ tufaceous-lib = { path = "tufaceous-lib" } unicode-width = "0.1.11" update-engine = { path = "update-engine" } usdt = "0.3" -uuid = { version = "1.6.1", features = ["serde", "v4"] } +uuid = { version = "1.6.1", features = ["serde", "v4", "v5"] } walkdir = "2.4" wicket = { path = "wicket" } wicket-common = { path = "wicket-common" } diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index d07e564ae1..e1c505a934 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -9,6 +9,8 @@ extern crate diesel; #[macro_use] extern crate newtype_derive; +use uuid::Uuid; + mod address_lot; mod bgp; mod block_size; @@ -307,6 +309,15 @@ macro_rules! impl_enum_type { pub(crate) use impl_enum_type; +/// This is an arbitrary UUID, but it's stable because it's embedded as a +/// constant. This defines a namespace, according to +/// , which allows generation of v5 +/// UUIDs which are deterministic. +/// +/// This UUID is used to identify hardware. +pub(crate) const HARDWARE_UUID_NAMESPACE: Uuid = + Uuid::from_u128(206230429496795504636731999500138461979); + /// Describes a type that's represented in the database using a String /// /// If you're reaching for this type, consider whether it'd be better to use an diff --git a/nexus/db-model/src/physical_disk.rs b/nexus/db-model/src/physical_disk.rs index 44316111d6..1174800e42 100644 --- a/nexus/db-model/src/physical_disk.rs +++ b/nexus/db-model/src/physical_disk.rs @@ -36,8 +36,24 @@ impl PhysicalDisk { variant: PhysicalDiskKind, sled_id: Uuid, ) -> Self { + // NOTE: We may want to be more restrictive when parsing the vendor, + // serial, and model values, so that we can supply a separator + // distinguishing them. + // + // Theoretically, we could have the following problem: + // + // - A Disk vendor "Foo" makes a disk with serial "Bar", and model "Rev1". + // - This becomes: "FooBarRev1". + // - A Disk vendor "FooBar" makes a disk with serial "Rev", and model "1". + // - This becomes: "FooBarRev1", and conflicts. + let interpolated_name = format!("{vendor}{serial}{model}"); + let disk_id = Uuid::new_v5( + &crate::HARDWARE_UUID_NAMESPACE, + interpolated_name.as_bytes(), + ); + println!("Physical Disk ID: {disk_id}, from {interpolated_name}"); Self { - identity: PhysicalDiskIdentity::new(Uuid::new_v4()), + identity: PhysicalDiskIdentity::new(disk_id), time_deleted: None, rcgen: Generation::new(), vendor, diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 468c68659a..59bdaa275b 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -1348,7 +1348,7 @@ mod test { .unwrap(); let (.., physical_disk_authz) = LookupPath::new(&opctx, &datastore) - .physical_disk(&disk.vendor, &disk.serial, &disk.model) + .physical_disk(disk.vendor, disk.serial, disk.model) .lookup_for(authz::Action::Modify) .await .unwrap(); diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index f83dbd7ae4..0e1fd56881 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -34,7 +34,7 @@ use uuid::Uuid; impl DataStore { /// - Sled Agents like to look up physical disks by "Vendor, Serial, Model" /// - The external API likes to look up physical disks by UUID - /// - LookuPath objects are opinionated about how they perform lookups. They + /// - LookupPath objects are opinionated about how they perform lookups. They /// support "primary keys" or "names", but they're opinionated about the /// name objects being a single string. /// diff --git a/nexus/db-queries/src/db/lookup.rs b/nexus/db-queries/src/db/lookup.rs index 028694dc4b..9b165af328 100644 --- a/nexus/db-queries/src/db/lookup.rs +++ b/nexus/db-queries/src/db/lookup.rs @@ -370,15 +370,15 @@ impl<'a> LookupPath<'a> { /// Select a resource of type PhysicalDisk, identified by its id pub fn physical_disk( self, - vendor: &str, - serial: &str, - model: &str, + vendor: String, + serial: String, + model: String, ) -> PhysicalDisk<'a> { PhysicalDisk::PrimaryKey( Root { lookup_root: self }, - vendor.to_string(), - serial.to_string(), - model.to_string(), + vendor, + serial, + model, ) } diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml new file mode 100644 index 0000000000..9b13f12aea --- /dev/null +++ b/nexus/preprocessed_configs/config.xml @@ -0,0 +1,41 @@ + + + + + trace + true + + + 8123 + 9000 + 9004 + + ./ + + true + + + + + + + ::/0 + + + default + default + 1 + + + + + + + + + + + \ No newline at end of file diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index 6984e9f194..1ab35d2533 100644 --- a/nexus/src/app/sled.rs +++ b/nexus/src/app/sled.rs @@ -168,11 +168,10 @@ impl super::Nexus { ) -> Result, Error> { let (v, s, m) = self .db_datastore - .physical_disk_id_to_name_no_auth(disk_selector.id) + .physical_disk_id_to_name_no_auth(disk_selector.disk_id) .await?; - Ok(LookupPath::new(&opctx, &self.db_datastore) - .physical_disk(&v, &s, &m)) + Ok(LookupPath::new(&opctx, &self.db_datastore).physical_disk(v, s, m)) } pub(crate) async fn sled_list_physical_disks( @@ -278,9 +277,9 @@ impl super::Nexus { let (_authz_disk, db_disk) = LookupPath::new(&opctx, &self.db_datastore) .physical_disk( - &info.disk_vendor, - &info.disk_serial, - &info.disk_model, + info.disk_vendor, + info.disk_serial, + info.disk_model, ) .fetch() .await?; diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 88b91d1f37..834d448c89 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -4869,7 +4869,7 @@ async fn physical_disk_list( /// Get a physical disk #[endpoint { method = GET, - path = "/v1/system/hardware/disks/{id}", + path = "/v1/system/hardware/disks/{disk_id}", tags = ["system/hardware"], }] async fn physical_disk_view( @@ -4892,7 +4892,7 @@ async fn physical_disk_view( /// Update a physical disk's state #[endpoint { method = PUT, - path = "/v1/system/hardware/disks/{id}", + path = "/v1/system/hardware/disks/{disk_id}", tags = ["system/hardware"], }] async fn physical_disk_update( diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs index 95e3d10c17..9eb5bcd258 100644 --- a/nexus/test-utils/src/lib.rs +++ b/nexus/test-utils/src/lib.rs @@ -60,7 +60,13 @@ pub const RACK_UUID: &str = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc"; pub const SWITCH_UUID: &str = "dae4e1f1-410e-4314-bff1-fec0504be07e"; pub const OXIMETER_UUID: &str = "39e6175b-4df2-4730-b11d-cbc1e60a2e78"; pub const PRODUCER_UUID: &str = "a6458b7d-87c3-4483-be96-854d814c20de"; -pub const PHYSICAL_DISK_UUID: &str = "2092c014-cb33-4654-b43e-d0958f855642"; +/// This is not random: It's a v5 UUID derived from: +/// +/// Uuid::new_v5( +/// HARDWARE_UUID_NAMESPACE, +/// ("test-vendor", "test-serial", "test-model"), +/// ) +pub const PHYSICAL_DISK_UUID: &str = "25849923-2232-5d20-b939-ffee5bc3dd89"; pub const RACK_SUBNET: &str = "fd00:1122:3344:01::/56"; /// The reported amount of hardware threads for an emulated sled agent. diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index a823d2c75e..411ef78bf3 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -1844,7 +1844,7 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { VerifyEndpoint { url: &HARDWARE_DISK_URL, - visibility: Visibility::Public, + visibility: Visibility::Protected, unprivileged_access: UnprivilegedAccess::None, allowed_methods: vec![ AllowedMethod::Get, diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 4ec1e7a66c..25c230223b 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -122,8 +122,8 @@ networking_switch_port_apply_settings POST /v1/system/hardware/switch-por networking_switch_port_clear_settings DELETE /v1/system/hardware/switch-port/{port}/settings networking_switch_port_list GET /v1/system/hardware/switch-port physical_disk_list GET /v1/system/hardware/disks -physical_disk_update PUT /v1/system/hardware/disks/{id} -physical_disk_view GET /v1/system/hardware/disks/{id} +physical_disk_update PUT /v1/system/hardware/disks/{disk_id} +physical_disk_view GET /v1/system/hardware/disks/{disk_id} rack_list GET /v1/system/hardware/racks rack_view GET /v1/system/hardware/racks/{rack_id} sled_instance_list GET /v1/system/hardware/sleds/{sled_id}/instances diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 7132ee7175..8fb27585eb 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -70,16 +70,7 @@ id_path_param!(GroupPath, group_id, "group"); // ID that can be used to deterministically generate the UUID. id_path_param!(SledPath, sled_id, "sled"); id_path_param!(SwitchPath, switch_id, "switch"); - -pub struct SledSelector { - /// ID of the sled - pub sled: Uuid, -} - -#[derive(Serialize, Deserialize, JsonSchema)] -pub struct PhysicalDiskPath { - pub id: Uuid, -} +id_path_param!(PhysicalDiskPath, disk_id, "physical_disk"); /// Updateable properties of a `PhysicalDisk`. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] diff --git a/openapi/nexus.json b/openapi/nexus.json index 72c83d7c14..ca8d319e33 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -3604,7 +3604,7 @@ } } }, - "/v1/system/hardware/disks/{id}": { + "/v1/system/hardware/disks/{disk_id}": { "get": { "tags": [ "system/hardware" @@ -3614,7 +3614,8 @@ "parameters": [ { "in": "path", - "name": "id", + "name": "disk_id", + "description": "ID of the physical_disk", "required": true, "schema": { "type": "string", @@ -3650,7 +3651,8 @@ "parameters": [ { "in": "path", - "name": "id", + "name": "disk_id", + "description": "ID of the physical_disk", "required": true, "schema": { "type": "string", From 4e7338b6246ddf6030954600d149b2086945eeca Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 27 Dec 2023 12:34:43 -0800 Subject: [PATCH 7/8] hakari, cleanup preprocessed configs --- nexus/preprocessed_configs/config.xml | 41 --------------------------- workspace-hack/Cargo.toml | 4 +-- 2 files changed, 2 insertions(+), 43 deletions(-) delete mode 100644 nexus/preprocessed_configs/config.xml diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml deleted file mode 100644 index 9b13f12aea..0000000000 --- a/nexus/preprocessed_configs/config.xml +++ /dev/null @@ -1,41 +0,0 @@ - - - - - trace - true - - - 8123 - 9000 - 9004 - - ./ - - true - - - - - - - ::/0 - - - default - default - 1 - - - - - - - - - - - \ No newline at end of file diff --git a/workspace-hack/Cargo.toml b/workspace-hack/Cargo.toml index 8998f7594b..e5a873534d 100644 --- a/workspace-hack/Cargo.toml +++ b/workspace-hack/Cargo.toml @@ -110,7 +110,7 @@ trust-dns-proto = { version = "0.22.0" } unicode-bidi = { version = "0.3.13" } unicode-normalization = { version = "0.1.22" } usdt = { version = "0.3.5" } -uuid = { version = "1.6.1", features = ["serde", "v4"] } +uuid = { version = "1.6.1", features = ["serde", "v4", "v5"] } yasna = { version = "0.5.2", features = ["bit-vec", "num-bigint", "std", "time"] } zerocopy = { version = "0.7.31", features = ["derive", "simd"] } zeroize = { version = "1.7.0", features = ["std", "zeroize_derive"] } @@ -214,7 +214,7 @@ trust-dns-proto = { version = "0.22.0" } unicode-bidi = { version = "0.3.13" } unicode-normalization = { version = "0.1.22" } usdt = { version = "0.3.5" } -uuid = { version = "1.6.1", features = ["serde", "v4"] } +uuid = { version = "1.6.1", features = ["serde", "v4", "v5"] } yasna = { version = "0.5.2", features = ["bit-vec", "num-bigint", "std", "time"] } zerocopy = { version = "0.7.31", features = ["derive", "simd"] } zeroize = { version = "1.7.0", features = ["std", "zeroize_derive"] } From 79e6446fb090cfaf9cf45bf423904cdb010558d8 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 27 Dec 2023 21:26:26 -0800 Subject: [PATCH 8/8] Update physical disk test to identify that UUID generation is deterministic now --- .../src/db/datastore/physical_disk.rs | 38 ++++++++--------- nexus/preprocessed_configs/config.xml | 41 +++++++++++++++++++ 2 files changed, 58 insertions(+), 21 deletions(-) create mode 100644 nexus/preprocessed_configs/config.xml diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index 0e1fd56881..f8d0282117 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -242,12 +242,10 @@ mod test { } // Only checking some fields: - // - The UUID of the disk may actually not be the same as the upserted one; - // the "vendor/serial/model" value is the more critical unique identifier. - // NOTE: Could we derive a UUID from the VSM values? // - The 'time' field precision can be modified slightly when inserted into // the DB. - fn assert_disks_equal_ignore_uuid(lhs: &PhysicalDisk, rhs: &PhysicalDisk) { + fn assert_disks_equal_ignore_time(lhs: &PhysicalDisk, rhs: &PhysicalDisk) { + assert_eq!(lhs.uuid(), rhs.uuid()); assert_eq!(lhs.time_deleted().is_some(), rhs.time_deleted().is_some()); assert_eq!(lhs.vendor, rhs.vendor); assert_eq!(lhs.serial, rhs.serial); @@ -257,9 +255,9 @@ mod test { } #[tokio::test] - async fn physical_disk_upsert_different_uuid_idempotent() { + async fn physical_disk_upsert_uuid_generation_deterministic() { let logctx = dev::test_setup_log( - "physical_disk_upsert_different_uuid_idempotent", + "physical_disk_upsert_uuid_generation_deterministic", ); let mut db = test_setup_database(&logctx.log).await; let (opctx, datastore) = datastore_test(&logctx, &db).await; @@ -280,7 +278,7 @@ mod test { .await .expect("Failed first attempt at upserting disk"); assert_eq!(disk.uuid(), first_observed_disk.uuid()); - assert_disks_equal_ignore_uuid(&disk, &first_observed_disk); + assert_disks_equal_ignore_time(&disk, &first_observed_disk); // Observe the inserted disk let pagparams = list_disk_params(); @@ -290,9 +288,12 @@ mod test { .expect("Failed to list physical disks"); assert_eq!(disks.len(), 1); assert_eq!(disk.uuid(), disks[0].uuid()); - assert_disks_equal_ignore_uuid(&disk, &disks[0]); + assert_disks_equal_ignore_time(&disk, &disks[0]); - // Insert the same disk, with a different UUID primary key + // Insert the same disk, but don't re-state the UUID. + // + // The rest of this test relies on the UUID derivation being + // deterministic. let disk_again = PhysicalDisk::new( String::from("Oxide"), String::from("123"), @@ -300,15 +301,14 @@ mod test { PhysicalDiskKind::U2, sled_id, ); + // With the same input parameters, the UUID should be deterministic. + assert_eq!(disk.uuid(), disk_again.uuid()); + let second_observed_disk = datastore .physical_disk_upsert(&opctx, disk_again.clone()) .await .expect("Failed second upsert of physical disk"); - // This check is pretty important - note that we return the original - // UUID, not the new one. - assert_ne!(disk_again.uuid(), second_observed_disk.uuid()); - assert_eq!(disk_again.id(), second_observed_disk.id()); - assert_disks_equal_ignore_uuid(&disk_again, &second_observed_disk); + assert_disks_equal_ignore_time(&disk_again, &second_observed_disk); assert!( first_observed_disk.time_modified() <= second_observed_disk.time_modified() @@ -318,13 +318,9 @@ mod test { .sled_list_physical_disks(&opctx, sled_id, &pagparams) .await .expect("Failed to re-list physical disks"); - - // We'll use the old primary key assert_eq!(disks.len(), 1); - assert_eq!(disk.uuid(), disks[0].uuid()); - assert_ne!(disk_again.uuid(), disks[0].uuid()); - assert_disks_equal_ignore_uuid(&disk, &disks[0]); - assert_disks_equal_ignore_uuid(&disk_again, &disks[0]); + assert_disks_equal_ignore_time(&disk, &disks[0]); + assert_disks_equal_ignore_time(&disk_again, &disks[0]); db.cleanup().await.unwrap(); logctx.cleanup_successful(); @@ -364,7 +360,7 @@ mod test { first_observed_disk.time_modified() <= second_observed_disk.time_modified() ); - assert_disks_equal_ignore_uuid( + assert_disks_equal_ignore_time( &first_observed_disk, &second_observed_disk, ); diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml new file mode 100644 index 0000000000..9b13f12aea --- /dev/null +++ b/nexus/preprocessed_configs/config.xml @@ -0,0 +1,41 @@ + + + + + trace + true + + + 8123 + 9000 + 9004 + + ./ + + true + + + + + + + ::/0 + + + default + default + 1 + + + + + + + + + + + \ No newline at end of file