diff --git a/Cargo.lock b/Cargo.lock index 3f9d01f1566..368ed672f61 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7472,6 +7472,7 @@ dependencies = [ "pq-sys", "predicates", "pretty_assertions", + "proptest", "qorb", "rand 0.9.2", "rcgen", diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index 9f016e857c4..98e235fb770 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -116,6 +116,7 @@ pem.workspace = true petgraph.workspace = true predicates.workspace = true pretty_assertions.workspace = true +proptest.workspace = true rcgen.workspace = true regex.workspace = true rustls.workspace = true diff --git a/nexus/db-queries/src/db/datastore/disk.rs b/nexus/db-queries/src/db/datastore/disk.rs index f155ac830bf..7f6e31b2215 100644 --- a/nexus/db-queries/src/db/datastore/disk.rs +++ b/nexus/db-queries/src/db/datastore/disk.rs @@ -306,17 +306,10 @@ impl LocalStorageDisk { /// Size required for the disk's data plus overhead pub fn required_dataset_size(&self) -> i64 { - // The value in ByteCount is capped at i64::MAX, but a disk backed by - // local storage cannot be created that does not fit in a zpool at the - // time of the disk creation request (note this is different from the - // time of instance reservation where Nexus actually assigns the disk an - // allocation). This means that the size is well below that MAX, meaning - // this won't overflow. - // - // Additionally, there's a checked_add in DiskTypeLocalStorage::New that - // checks this won't happen. Note this won't protect against cases of - // database modification, but not much will! - + // Both values are ByteCounts, so each fits in i64. The overhead is + // roughly 7% of the disk size (see DiskTypeLocalStorage::new), so the + // sum only approaches i64::MAX for disks of several EiB, far beyond + // what any zpool can hold. self.size().to_bytes() as i64 + self.required_dataset_overhead().to_bytes() as i64 } diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 5cd18a14e05..e4e74cd3147 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -52,13 +52,13 @@ use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; -use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_uuid_kinds::DatasetUuid; +use omicron_uuid_kinds::DiskUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::PropolisUuid; @@ -66,12 +66,12 @@ use omicron_uuid_kinds::RackKind; use omicron_uuid_kinds::RackUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; +use rand::SeedableRng; +use rand::rngs::StdRng; +use rand::seq::IndexedRandom; use sled_hardware_types::BaseboardId; use slog::Logger; use slog_error_chain::InlineErrorChain; -use std::cmp::Ordering; -use std::collections::BinaryHeap; -use std::collections::HashMap; use std::collections::HashSet; use std::fmt; use strum::IntoEnumIterator; @@ -354,560 +354,650 @@ fn pick_sled_reservation_target( return Err(SledReservationError::NotFound); } -/// A candidate dataset for a local storage allocation -#[derive(Clone, Debug, Hash, PartialEq, Eq)] -struct CandidateDataset { - rendezvous_local_storage_unencrypted_dataset_id: DatasetUuid, - pool_id: ZpoolUuid, - sled_id: SledUuid, -} +/// A local storage disk that needs a dataset allocation. +#[derive(Debug, Clone)] +struct LocalStorageRequest { + /// The virtual disk requiring the allocation + disk_id: DiskUuid, -/// For a given local storage disk that has not been allocated, store all the -/// candidate datasets that could fulfill that allocation. -#[derive(Clone, Debug)] -struct PossibleAllocationsForRequest<'a> { - request: &'a LocalStorageDisk, - candidate_datasets: HashSet, + /// Disk size plus required dataset overhead, in bytes + required_dataset_size: i64, } -/// Store the intermediate search state during the search for all possible -/// pairings of requests for local storage to datasets. This is ordered by how -/// many pairings have been made so far to prioritize quickly finding a complete -/// allocation. -struct IncompleteAllocationList { - /// Requests for a local storage allocation that have been paired with a - /// dataset - allocations: Vec, - - /// Remaining candidate datasets for non-paired allocation requests - candidates_left: HashSet, - - /// Which allocation request to consider next - request_index: usize, -} +/// A zpool on the target sled that is a valid target for a local storage +/// allocation and is not already used by one of this instance's existing +/// allocations. +#[derive(Debug, Clone)] +struct LocalStorageCandidatePool { + pool_id: ZpoolUuid, + sled_id: SledUuid, + rendezvous_local_storage_unencrypted_dataset_id: DatasetUuid, -impl PartialOrd for IncompleteAllocationList { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } + /// Bytes available for new allocations, see + /// `ZpoolGetForSledReservationResult::headroom` + headroom: i64, } -impl Ord for IncompleteAllocationList { - fn cmp(&self, other: &Self) -> Ordering { - self.allocations.len().cmp(&other.allocations.len()) - } -} +/// Why no complete set of local storage allocations could be chosen for a +/// sled. +#[derive(Debug, thiserror::Error)] +enum LocalStorageUnsatisfiable { + /// No disks required an allocation. Callers are expected to check for + /// this case before choosing allocations. + #[error("no disks require a local storage allocation")] + NoAllocationsRequired, + + /// There are more disks requiring an allocation than usable pools: each + /// disk needs a distinct pool. + #[error( + "{requests} disks require a local storage allocation, \ + but only {pools} pools are usable" + )] + NotEnoughPools { requests: usize, pools: usize }, -impl PartialEq for IncompleteAllocationList { - fn eq(&self, other: &Self) -> bool { - self.allocations.len() == other.allocations.len() - } + /// No unused pool fits this request. Requests are placed largest first, + /// so when even the roomiest remaining pool (reported here) does not fit + /// the request, no assignment of these requests to these pools exists. + #[error( + "disk {disk_id} requires {required_dataset_size} bytes, \ + but pool {pool_id} only has {headroom} bytes of headroom" + )] + RequestDoesNotFit { + disk_id: DiskUuid, + required_dataset_size: i64, + pool_id: ZpoolUuid, + headroom: i64, + }, } -impl Eq for IncompleteAllocationList {} - -/// Store all the state required to iterate through possible mappings of local -/// storage allocation requests to datasets. +/// Pair each local storage request with a distinct pool, or return an error +/// if that isn't possible. /// -/// Yield complete allocation lists as they're found, where complete means all -/// allocations have been performed, because computing all possibilities up -/// front is expensive. -struct CompleteLocalStorageAllocationLists<'a> { - log: Logger, - - sled_target: SledUuid, +/// Place the requests largest-first, each on a pool chosen uniformly at +/// random from the unused pools that fit it. This either produces a valid +/// pairing or proves that none exists: +/// +/// - A pool fits a request iff the request is strictly smaller than the +/// pool's free space, so any pool that fits a request also fits every +/// smaller request. +/// +/// - Placing the largest remaining request on any pool that fits it therefore +/// does not inhibit subsequent requests. If a valid pairing uses that pool for +/// a different (necessarily smaller) subsequent request, that request can +/// instead use whichever pool the larger one would have taken. +/// +/// - Applying that argument at each step down the sorted list means no +/// backtracking is needed. If some request has no fitting pool left, no +/// assignment of these requests to these pools exists at all. +/// +/// A caller seeing an error can conclude that these requests cannot fit on +/// this sled at all, rather than needing to try other arrangements. +/// +/// For a given rng state and inputs the output is deterministic; tests rely +/// on this by passing a seeded rng. +fn pair_local_storage_requests_to_pools( + mut requests: Vec, + mut pools: Vec, + rng: &mut StdRng, +) -> Result, LocalStorageUnsatisfiable> { + if requests.is_empty() { + return Err(LocalStorageUnsatisfiable::NoAllocationsRequired); + } + + // Each request requires a distinct pool. + if requests.len() > pools.len() { + return Err(LocalStorageUnsatisfiable::NotEnoughPools { + requests: requests.len(), + pools: pools.len(), + }); + } - instance_id: InstanceUuid, + requests.sort_by(|a, b| { + b.required_dataset_size + .cmp(&a.required_dataset_size) + .then(a.disk_id.cmp(&b.disk_id)) + }); - /// All allocations that need to be performed - allocations_to_perform: Vec>, + let mut allocations = Vec::with_capacity(requests.len()); - /// A queue of incomplete allocation lists to search through, where - /// incomplete means there are still local storage disk to dataset - /// allocations to be performed. These are sorted by the number of - /// allocations made so far to prioritize yielding a valid allocation. - queue: BinaryHeap, -} + for request in &requests { + let fitting: Vec = (0..pools.len()) + .filter(|&i| request.required_dataset_size < pools[i].headroom) + .collect(); -impl<'a> CompleteLocalStorageAllocationLists<'a> { - fn new( - log: &Logger, - sled_target: SledUuid, - instance_id: InstanceUuid, - mut zpools_for_sled: IdOrdMap, - local_storage_disks: &'a [LocalStorageDisk], - ) -> Option { - let local_storage_allocation_required: Vec<&LocalStorageDisk> = - local_storage_disks + let Some(&choice) = fitting.choose(rng) else { + // Requests are placed largest-first, so if not even the roomiest + // remaining pool fits this request, no assignment of these + // requests to these pools fits either. + let roomiest = pools .iter() - .filter(|disk| disk.local_storage_dataset_allocation.is_none()) - .collect(); - - // First, each request for local storage can possibly be satisfied by a - // number of zpools on the sled. Find this list of candidate zpools for - // each required local storage allocation. + .max_by_key(|pool| pool.headroom) + .expect("more pools than remaining requests"); + + return Err(LocalStorageUnsatisfiable::RequestDoesNotFit { + disk_id: request.disk_id, + required_dataset_size: request.required_dataset_size, + pool_id: roomiest.pool_id, + headroom: roomiest.headroom, + }); + }; - // If there's an existing local storage allocation on a zpool - // (regardless of whether or not it's an encrypted or unencrypted - // allocation), remove that pool from the list of candidates. Local - // storage for the same Instance should not share any zpools. - let local_storage_zpools_used: HashSet = local_storage_disks - .iter() - .filter_map(|disk| { - disk.local_storage_dataset_allocation.as_ref().map( - |allocation| { - ZpoolUuid::from_untyped_uuid( - allocation.pool_id().into_untyped_uuid(), - ) - }, - ) - }) - .collect(); + let pool = pools.swap_remove(choice); - // Do not allow multiple disks backed by local storage to use the same - // pool for the same instance. - zpools_for_sled.retain(|zpool_get_result| { - !local_storage_zpools_used.contains(&zpool_get_result.pool.id()) - }); + allocations.push(LocalStorageAllocation { + disk_id: request.disk_id, - if local_storage_allocation_required.len() > zpools_for_sled.len() { - // Not enough zpools to satisfy the number of allocations required. - // Find another sled! - info!( - &log, - "sled {sled_target} does not have enough zpools to satisfy \ - local storage allocations"; - "zpools" => zpools_for_sled.len(), - "allocations" => local_storage_allocation_required.len(), - ); + local_storage_unencrypted_dataset_allocation_id: + DatasetUuid::new_v4(), - return None; - } + required_dataset_size: request.required_dataset_size, - info!(&log, "filtered zpools for sled: {zpools_for_sled:?}"); + local_storage_unencrypted_dataset_id: pool + .rendezvous_local_storage_unencrypted_dataset_id, - let mut allocations_to_perform = - Vec::with_capacity(local_storage_allocation_required.len()); + pool_id: pool.pool_id, - for request in &local_storage_allocation_required { - // Find all the zpools that could satisfy this local storage - // request. These will be filtered later. - let candidate_datasets: HashSet = zpools_for_sled - .iter() - .filter(|zpool_get_result| { - // Any zpool that has space for this local storage - // dataset allocation is considered a candidate. - zpool_get_result.has_room_for_allocation( - request.required_dataset_size(), - ) - }) - .map(|zpool_get_result| CandidateDataset { - rendezvous_local_storage_unencrypted_dataset_id: - zpool_get_result - .rendezvous_local_storage_unencrypted_dataset_id, - pool_id: zpool_get_result.pool.id(), - sled_id: zpool_get_result.pool.sled_id(), - }) - .collect(); + sled_id: pool.sled_id, + }); + } - if candidate_datasets.is_empty() { - // if there's no local storage datasets on this sled for this - // request's size, then try another sled. - info!( - &log, - "sled {sled_target} does not have any candidate datasets \ - with available space to satisfy local storage allocation"; - "request" => ?request, - ); + // `requests` was checked non-empty above. + NonEmpty::from_vec(allocations) + .ok_or(LocalStorageUnsatisfiable::NoAllocationsRequired) +} - return None; +/// Choose a local storage allocation for every local storage disk of an +/// instance that does not have one, against a snapshot of the target sled's +/// zpools. +/// +/// The returned allocations use distinct pools, and exclude any pool already +/// used by one of the instance's existing local storage allocations: local +/// storage for the same instance must not share zpools. +/// +/// The snapshot is only advisory. `sled_insert_resource_query` re-validates +/// capacity and dataset/pool/sled/disk state atomically at insert time, and +/// inserts nothing if any check fails. +fn choose_local_storage_allocations( + zpools_for_sled: &IdOrdMap, + local_storage_disks: &[LocalStorageDisk], + rng: &mut StdRng, +) -> Result, LocalStorageUnsatisfiable> { + // Each disk either still needs an allocation, or already has one whose + // pool (encrypted or unencrypted) is not eligible for further + // allocations. + let mut requests: Vec = Vec::new(); + let mut used_pools: HashSet = HashSet::new(); + + for disk in local_storage_disks { + match &disk.local_storage_dataset_allocation { + None => requests.push(LocalStorageRequest { + disk_id: DiskUuid::from_untyped_uuid(disk.id()), + required_dataset_size: disk.required_dataset_size(), + }), + + Some(allocation) => { + used_pools.insert(ZpoolUuid::from_untyped_uuid( + allocation.pool_id().into_untyped_uuid(), + )); } - - allocations_to_perform.push(PossibleAllocationsForRequest { - request, - candidate_datasets, - }); } + } - // From the list of allocations to perform, and all the candidate local - // storage datasets that could fit those allocations, find a list of all - // valid request -> zpool mappings. - // - // Start from no allocations made yet, a list of allocations to perform - // (stored in `requests`), and all of the available zpools on the sled. - - let mut queue = BinaryHeap::new(); - - queue.push(IncompleteAllocationList { - allocations: vec![], - candidates_left: zpools_for_sled - .iter() - .map(|zpool_get_result| CandidateDataset { - rendezvous_local_storage_unencrypted_dataset_id: - zpool_get_result - .rendezvous_local_storage_unencrypted_dataset_id, - pool_id: zpool_get_result.pool.id(), - sled_id: zpool_get_result.pool.sled_id(), - }) - .collect(), - request_index: 0, - }); - - Some(Self { - log: log.clone(), - sled_target, - instance_id, - allocations_to_perform, - queue, + let pools: Vec = zpools_for_sled + .iter() + .filter(|zpool_get_result| { + !used_pools.contains(&zpool_get_result.pool.id()) }) - } + .map(|zpool_get_result| LocalStorageCandidatePool { + pool_id: zpool_get_result.pool.id(), + sled_id: zpool_get_result.pool.sled_id(), + rendezvous_local_storage_unencrypted_dataset_id: zpool_get_result + .rendezvous_local_storage_unencrypted_dataset_id, + headroom: zpool_get_result.headroom(), + }) + .collect(); - /// Remove items from the queue if the _current_ size usage for the pools - /// now shows that there is not enough room. Similarly, remove an - /// allocation's candidate datasets if there is now not enough room. - /// - /// Drop the whole queue if any of the disks requiring an allocation were - /// detached or deleted. - pub async fn prune_invalidated_allocation_lists( - &mut self, - conn: &async_bb8_diesel::Connection, - opctx: &OpContext, - ) -> LookupResult<()> { - // Between when the instance allocation was requested and now, any disk - // backed by local storage could have been detached and deleted. Check - // for that here, and prune the entire search space if this happened. + pair_local_storage_requests_to_pools(requests, pools, rng) +} - let disks: HashMap = { - use nexus_db_schema::schema::disk::dsl; +/// Return true if any local storage disk still requiring an allocation has +/// been deleted, or is no longer attached to the given instance. +/// +/// The reservation loop calls this when the insert query inserted zero rows, +/// to tell two situations apart: the zpool snapshot went stale (retry with a +/// fresh one), or a disk in the request went away (no sled can ever satisfy +/// this reservation, so fail now rather than retrying everywhere). +async fn any_request_disk_deleted_or_detached( + conn: &async_bb8_diesel::Connection, + instance_id: InstanceUuid, + local_storage_disks: &[LocalStorageDisk], +) -> LookupResult { + use nexus_db_schema::schema::disk::dsl; + + let disk_ids: Vec = local_storage_disks + .iter() + .filter(|disk| disk.local_storage_dataset_allocation.is_none()) + .map(|disk| disk.id()) + .collect(); + + let expected = disk_ids.len(); + + let attach_instance_ids: Vec> = dsl::disk + .filter(dsl::id.eq_any(disk_ids)) + .filter(dsl::time_deleted.is_null()) + .select(dsl::attach_instance_id) + .load_async(conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + .internal_context("selecting local storage disks failed") + })?; - let disk_ids: Vec = self - .allocations_to_perform - .iter() - .map(|allocation| allocation.request.id()) - .collect(); + // A missing row means the disk was deleted: soft-deleted rows are + // filtered out above, and a hard-deleted row is gone entirely. + if attach_instance_ids.len() != expected { + return Ok(true); + } - dsl::disk - .filter(dsl::id.eq_any(disk_ids)) - .select(db::model::Disk::as_select()) - .load_async(conn) - .await - .map_err(|e| { - public_error_from_diesel(e, ErrorHandler::Server) - .internal_context("selecting multiple disks failed") - })? - .into_iter() - .map(|disk| (disk.id(), disk)) - .collect() - }; + Ok(attach_instance_ids.iter().any(|attach_instance_id| { + *attach_instance_id != Some(instance_id.into_untyped_uuid()) + })) +} - for allocation in &self.allocations_to_perform { - let disk_id = allocation.request.id(); +#[cfg(test)] +mod local_storage_pairing_test { + use super::*; - let Some(disk) = disks.get(&disk_id) else { - // Is it possible the disk has been hard-deleted somehow? - // Otherwise how would we land here, given that we just created - // the map! - return Err(Error::internal_error(&format!( - "disk id {disk_id} not found in map" - ))); - }; + use nexus_types::external_api::disk; + use omicron_uuid_kinds::ExternalZpoolUuid; + use omicron_uuid_kinds::PhysicalDiskUuid; + use proptest::prelude::*; - // Prune the entire space if the disk was deleted, or if the disk - // was detached. - if disk.time_deleted().is_some() - || disk.attach_instance_id - != Some(self.instance_id.into_untyped_uuid()) - { - self.queue.clear(); - return Ok(()); - } + fn request(disk_id: u128, size: i64) -> LocalStorageRequest { + LocalStorageRequest { + disk_id: DiskUuid::from_untyped_uuid(Uuid::from_u128(disk_id)), + required_dataset_size: size, } + } - // Prune incomplete allocations that are no longer valid. - - let zpools_for_sled = - DataStore::zpool_get_for_sled_reservation_on_conn( - conn, - opctx, - self.sled_target, - ) - .await - .internal_context("zpool_get_for_sled_reservation failed")?; + fn pool(pool_id: u128, headroom: i64) -> LocalStorageCandidatePool { + LocalStorageCandidatePool { + pool_id: ZpoolUuid::from_untyped_uuid(Uuid::from_u128(pool_id)), + sled_id: SledUuid::from_untyped_uuid(Uuid::from_u128(0x51ed)), + rendezvous_local_storage_unencrypted_dataset_id: + DatasetUuid::from_untyped_uuid(Uuid::from_u128(pool_id)), + headroom, + } + } - // Right off the top, if there aren't enough zpools for the requested - // allocations, prune the entire search space. + fn test_rng() -> StdRng { + StdRng::seed_from_u64(0) + } - if zpools_for_sled.len() < self.allocations_to_perform.len() { - info!( - self.log, - "only {} zpools for sled, not enough for {} allocations", - zpools_for_sled.len(), - self.allocations_to_perform.len(), - ); + /// An allocation fits only if it is strictly smaller than the pool's + /// headroom, matching the comparison in `sled_insert_resource_query`. + #[test] + fn boundary_is_strict() { + let err = pair_local_storage_requests_to_pools( + vec![request(1, 100)], + vec![pool(1, 100)], + &mut test_rng(), + ) + .unwrap_err(); - self.queue.clear(); - return Ok(()); - } + assert!(matches!( + err, + LocalStorageUnsatisfiable::RequestDoesNotFit { .. } + )); - let mut incomplete_allocations_removed = 0; + pair_local_storage_requests_to_pools( + vec![request(1, 99)], + vec![pool(1, 100)], + &mut test_rng(), + ) + .unwrap(); + } - self.queue.retain(|incomplete_allocation_list| { - // An incomplete allocation list has a set of local storage - // allocations that were matched to zpools with available space: - // - // | allocation -> zpool | allocation -> zpool | ... - // - // Check that each of of these is still valid: this iterator was - // constructed from a `ZpoolGetForSledReservationResult` that was - // taken previously, and other sled reservations (or crucible - // allocations!) may have consumed space that was previously free. - // If the sled reservation query was run for one of these - // allocations it would always fail. - // - // Do not retain an incomplete allocation list if any of the - // allocations are no longer valid. This also prunes the search - // space, as going any further would not make sense. - - for allocation in &incomplete_allocation_list.allocations { - match zpools_for_sled.get(&allocation.pool_id) { - Some(zpool_for_sled) => { - // Zpool still exists and is still a valid target, does - // it still have room? - - if !zpool_for_sled.has_room_for_allocation( - allocation.required_dataset_size, - ) { - // Do not retain this incomplete list of - // allocations: one of them is no longer valid. - incomplete_allocations_removed += 1; - return false; - } - } + /// If our selection of local disk allocation always picked "the pool + /// with the most free space first", this workload of allocations + /// would reliably fail. + /// + /// Instead, our allocation pattern identifies all pools where a the + /// largest disk request COULD fit, and then randomly picks from + /// those options. This does make the selection non-deterministic + /// (hence our supplied RNG). + /// + /// However, for the sake of a test, we're trying to validate + /// that this works "well enough" - we use hard-coded RNGs for determinism, + /// and then validate that "an overwhelming majority" pass. + #[test] + fn descending_workload_across_seeds() { + // 774 GiB of headroom per pool, in bytes. + let pool_headroom: i64 = + external::ByteCount::from_gibibytes_u32(774).to_bytes() as i64; + + // Disk sizes in GiB, chosen so that seven rounds of these disks + // nearly fill ten pools. + const DISK_GIB: [u32; 7] = [388, 195, 98, 50, 26, 14, 8]; + + // Compute each request's size (disk size plus dataset overhead) + // through the same path production uses, so this workload stays in + // sync with the overhead charged by DiskTypeLocalStorage::new. + let request_sizes: Vec = DISK_GIB + .iter() + .enumerate() + .map(|(i, gib)| { + local_storage_disk( + Uuid::from_u128(i as u128 + 1), + external::ByteCount::from_gibibytes_u32(*gib), + ) + .required_dataset_size() + }) + .collect(); - None => { - // Zpool doesn't exist anymore or it's no longer a valid - // target (for example due to being marked - // no_provision). + let mut succeeded = 0; - incomplete_allocations_removed += 1; - return false; - } - } - } + for seed in 0..100u64 { + let mut rng = StdRng::seed_from_u64(seed); + let mut headrooms = [pool_headroom; 10]; - // by default, continue searching further - true - }); + let all_rounds_placed = (0..7).all(|_| { + let requests: Vec = request_sizes + .iter() + .enumerate() + .map(|(i, size)| request(i as u128 + 1, *size)) + .collect(); - if incomplete_allocations_removed > 0 { - info!( - self.log, - "pruned {incomplete_allocations_removed} incomplete \ - allocation lists during instance provisioning", - ); - } + let pools: Vec = headrooms + .iter() + .enumerate() + .map(|(i, headroom)| pool(i as u128 + 1, *headroom)) + .collect(); - // Each allocation to perform was created with a list of candidate - // datasets, themselves located on zpools: - // - // allocation: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9] - // - // If the dataset on zpool6 originally had room for this allocation but - // no longer does, and it is not removed from the candidate list, this - // iterator's search will still add a mapping of that allocation to it, - // only to be pruned by the incomplete_allocation_list pruning step - // above after the insert CTE does not create any rows. Without this - // step, the iterator will continue adding invalid allocations to the - // queue instead of bailing out of this branch of the search tree. - // - // The original list was created based on an old snapshot of zpool - // information. Based on the updated zpool information passed into this - // function, prune each allocation's candidate dataset list. - - let mut candidate_datasets_removed = 0; - - for allocation in &mut self.allocations_to_perform { - // Update the candidate datasets for an allocation based on the - // updated ZpoolGetForSledReservationResult. - allocation.candidate_datasets.retain(|candidate_dataset| { - match zpools_for_sled.get(&candidate_dataset.pool_id) { - Some(zpool_for_sled) => { - // Zpool still exists, does it still have room? - - if !zpool_for_sled.has_room_for_allocation( - allocation.request.required_dataset_size(), - ) { - // Do not retain this as a candidate dataset, it no - // longer has room. - candidate_datasets_removed += 1; - return false; + match pair_local_storage_requests_to_pools( + requests, pools, &mut rng, + ) { + Ok(allocations) => { + for allocation in &allocations { + let i = usize::try_from( + allocation + .pool_id + .into_untyped_uuid() + .as_u128() + - 1, + ) + .unwrap(); + headrooms[i] -= allocation.required_dataset_size; } + true } - - None => { - // Zpool doesn't exist anymore! - candidate_datasets_removed += 1; - return false; - } + Err(_) => false, } - - true }); - // If this causes the list of candidate datasets to become empty, - // then prune the entire search space. - - if allocation.candidate_datasets.is_empty() { - info!( - self.log, - "no more candidate datasets for allocation for disk {}", - allocation.request.id(), - ); - - self.queue.clear(); - return Ok(()); + if all_rounds_placed { + succeeded += 1; } } - if candidate_datasets_removed > 0 { - info!( - self.log, - "pruned {candidate_datasets_removed} candidate datasets \ - during instance provisioning", - ); - } + // The exact count depends on the rng draw pattern, so leave slack; + // anything far from 100 means placement is systematically stranding + // space. (At the time of writing, 99 of the 100 seeds succeed.) + assert!( + succeeded >= 90, + "only {succeeded}/100 seeds placed the workload" + ); + } - // If, after pruning the candidate datasets, there aren't enough - // distinct zpools to provide one per requested allocation, then prune - // the entire search space: each allocation has to be put on a distinct - // zpool, so this iterator will search through the entire search space - // and fail to yield anything valid. Short circuit this so we don't - // waste CPU time! + /// Total free space is not the criterion: each request needs one pool + /// that individually fits it. + #[test] + fn infeasible_despite_total_space() { + let err = pair_local_storage_requests_to_pools( + vec![request(1, 500), request(2, 500)], + vec![pool(1, 1200), pool(2, 300)], + &mut test_rng(), + ) + .unwrap_err(); - let distinct_pools: HashSet = self - .allocations_to_perform - .iter() - .flat_map(|r| r.candidate_datasets.iter().map(|c| c.pool_id)) - .collect(); + assert!(matches!( + err, + LocalStorageUnsatisfiable::RequestDoesNotFit { .. } + )); + } - if distinct_pools.len() < self.allocations_to_perform.len() { - info!( - self.log, - "only {} distinct zpools after pruning, not enough for {} - allocations", - distinct_pools.len(), - self.allocations_to_perform.len(), - ); + #[test] + fn more_requests_than_pools() { + let err = pair_local_storage_requests_to_pools( + vec![request(1, 1), request(2, 1), request(3, 1)], + vec![pool(1, 100), pool(2, 100)], + &mut test_rng(), + ) + .unwrap_err(); - self.queue.clear(); - } + assert!(matches!( + err, + LocalStorageUnsatisfiable::NotEnoughPools { requests: 3, pools: 2 } + )); - Ok(()) + let err = pair_local_storage_requests_to_pools( + vec![request(1, 1)], + vec![], + &mut test_rng(), + ) + .unwrap_err(); + + assert!(matches!( + err, + LocalStorageUnsatisfiable::NotEnoughPools { requests: 1, pools: 0 } + )); } -} -impl<'a> Iterator for CompleteLocalStorageAllocationLists<'a> { - type Item = NonEmpty; + #[test] + fn no_requests() { + let err = pair_local_storage_requests_to_pools( + vec![], + vec![pool(1, 100)], + &mut test_rng(), + ) + .unwrap_err(); - fn next(&mut self) -> Option { - // For each incomplete allocation list: - // - // - find the next request to allocate for - // - // - select a local storage dataset from the list of local storage - // datasets that have not been used yet that could fulfill that - // request - // - // - remove that selected local storage dataset from the list of - // candidates - // - // - add that to the queue - // - // A list of allocations as complete if all requests have been matched - // with a local storage dataset. - - while let Some(incomplete_allocation_list) = self.queue.pop() { - let IncompleteAllocationList { - allocations, - candidates_left, - request_index, - } = incomplete_allocation_list; - - // If we have an allocation for each possible allocation, this one - // is complete. - if request_index == self.allocations_to_perform.len() { - let allocations_len = allocations.len(); - - match NonEmpty::from_vec(allocations) { - Some(allocations) => { - return Some(allocations); - } + assert!(matches!( + err, + LocalStorageUnsatisfiable::NoAllocationsRequired + )); + } - None => { - // There should be `request_index` entries in the - // `allocations` vec, this is weird! - error!( - &self.log, - "expected {request_index} in the \ - allocations vec, saw {allocations_len}", - ); - } - } + fn local_storage_disk( + disk_id: Uuid, + size: external::ByteCount, + ) -> LocalStorageDisk { + let params = disk::DiskCreate { + identity: external::IdentityMetadataCreateParams { + name: format!("disk-{disk_id}").parse().unwrap(), + description: String::from("a local storage disk"), + }, - continue; - } + disk_backend: disk::DiskBackend::Local {}, - // Try to allocate the Nth possible allocation - let request = &self.allocations_to_perform[request_index]; + size, + }; - // Create a possible config based on the what datasets are - // left, and the candidate datasets for this request. - for candidate_dataset in &request.candidate_datasets { - if candidates_left.contains(candidate_dataset) { - // This request could be satisfied by this dataset. - // Select it and search further. + LocalStorageDisk::new( + db::model::Disk::new( + disk_id, + Uuid::new_v4(), + ¶ms, + db::model::BlockSize::AdvancedFormat, + db::model::DiskRuntimeState::new(), + db::model::DiskType::LocalStorage, + ), + db::model::DiskTypeLocalStorage::new(disk_id, size).unwrap(), + ) + } - let mut set_allocations = allocations.clone(); - set_allocations.push(LocalStorageAllocation { - disk_id: request.request.id(), + /// Disks that already have an allocation are not re-allocated, and the + /// pools those allocations use are not eligible for the remaining disks. + #[test] + fn existing_allocations_reserve_their_pool() { + let sled_id = SledUuid::new_v4(); + let pool_a = ZpoolUuid::new_v4(); + let pool_b = ZpoolUuid::new_v4(); + + let mut zpools_for_sled = IdOrdMap::new(); + for pool_id in [pool_a, pool_b] { + zpools_for_sled + .insert_unique(ZpoolGetForSledReservationResult::new_for_test( + db::model::Zpool::new( + pool_id, + sled_id, + PhysicalDiskUuid::new_v4(), + external::ByteCount::from_gibibytes_u32(0).into(), + ), + i64::from(u32::MAX), + DatasetUuid::new_v4(), + 0, + 0, + )) + .unwrap(); + } - local_storage_unencrypted_dataset_allocation_id: - DatasetUuid::new_v4(), + let allocated_disk = { + let mut disk = local_storage_disk( + Uuid::new_v4(), + external::ByteCount::from_gibibytes_u32(1), + ); + disk.local_storage_dataset_allocation = + Some(datastore::LocalStorageAllocation::Unencrypted( + db::model::LocalStorageUnencryptedDatasetAllocation::new_for_tests_only( + DatasetUuid::new_v4(), + Utc::now(), + DatasetUuid::new_v4(), + ExternalZpoolUuid::from_untyped_uuid( + pool_a.into_untyped_uuid(), + ), + sled_id, + external::ByteCount::from_gibibytes_u32(1).into(), + ), + )); + disk + }; - required_dataset_size: request - .request - .required_dataset_size(), + let unallocated_disk = local_storage_disk( + Uuid::new_v4(), + external::ByteCount::from_gibibytes_u32(1), + ); + let unallocated_disk_id = unallocated_disk.id(); - local_storage_unencrypted_dataset_id: candidate_dataset - .rendezvous_local_storage_unencrypted_dataset_id, + let allocations = choose_local_storage_allocations( + &zpools_for_sled, + &[allocated_disk, unallocated_disk], + &mut test_rng(), + ) + .unwrap(); - pool_id: candidate_dataset.pool_id, + // Only the unallocated disk gets an allocation, and it lands on the + // pool the existing allocation does not use. + assert_eq!(allocations.len(), 1); + assert_eq!( + allocations[0].disk_id.into_untyped_uuid(), + unallocated_disk_id, + ); + assert_eq!(allocations[0].pool_id, pool_b); + } + + /// Backtracking search for any assignment of requests to distinct + /// fitting pools, used to check the greedy pairing against ground truth. + fn matching_exists( + requests: &[i64], + pools: &[i64], + used: &mut Vec, + ) -> bool { + let Some((first, rest)) = requests.split_first() else { + return true; + }; - sled_id: candidate_dataset.sled_id, - }); + for (i, headroom) in pools.iter().enumerate() { + if !used[i] && first < headroom { + used[i] = true; + if matching_exists(rest, pools, used) { + return true; + } + used[i] = false; + } + } - // Note by removing a candidate dataset from the list in - // this way, this step mandates that a single local storage - // dataset is not used for multiple local storage - // allocations for a given instance. + false + } - let mut set_candidates_left = candidates_left.clone(); - set_candidates_left.remove(candidate_dataset); + proptest! { + /// The greedy pairing succeeds exactly when some valid assignment + /// exists, and its output is well formed. + #[test] + fn greedy_matches_brute_force( + request_sizes in proptest::collection::vec(0..16i64, 1..=6), + pool_headrooms in proptest::collection::vec(0..16i64, 0..=8), + seed in proptest::prelude::any::(), + ) { + let requests: Vec = request_sizes + .iter() + .enumerate() + .map(|(i, size)| request(i as u128 + 1, *size)) + .collect(); - self.queue.push(IncompleteAllocationList { - allocations: set_allocations, - candidates_left: set_candidates_left, - request_index: request_index + 1, - }); + let pools: Vec = pool_headrooms + .iter() + .enumerate() + .map(|(i, headroom)| pool(i as u128 + 1, *headroom)) + .collect(); + + let mut used = vec![false; pool_headrooms.len()]; + let expect_feasible = pool_headrooms.len() >= request_sizes.len() + && matching_exists(&request_sizes, &pool_headrooms, &mut used); + + match pair_local_storage_requests_to_pools( + requests, + pools, + &mut StdRng::seed_from_u64(seed), + ) { + Ok(allocations) => { + prop_assert!(expect_feasible); + + prop_assert_eq!(allocations.len(), request_sizes.len()); + + let disk_ids: HashSet = + allocations.iter().map(|a| a.disk_id).collect(); + prop_assert_eq!(disk_ids.len(), allocations.len()); + + let pool_ids: HashSet = + allocations.iter().map(|a| a.pool_id).collect(); + prop_assert_eq!(pool_ids.len(), allocations.len()); + + for allocation in &allocations { + let headroom = pool_headrooms[usize::try_from( + allocation + .pool_id + .into_untyped_uuid() + .as_u128() + - 1, + ) + .unwrap()]; + + prop_assert!( + allocation.required_dataset_size < headroom + ); + } } - // Else there are no candidate datasets left for this request, - // and therefore the list of requests cannot be fulfilled by the - // current mapping of requests to datasets + Err(reason) => { + prop_assert!( + !expect_feasible, + "greedy failed ({reason}) but a valid assignment \ + exists", + ); + } } } - - None } } @@ -1129,6 +1219,7 @@ impl DataStore { resources, constraints, reservation_reason, + &mut StdRng::from_os_rng(), ) .await .map_err(|e| match e { @@ -1140,6 +1231,7 @@ impl DataStore { }) } + #[allow(clippy::too_many_arguments)] async fn sled_reservation_create_inner( &self, opctx: &OpContext, @@ -1148,6 +1240,7 @@ impl DataStore { resources: db::model::Resources, constraints: db::model::SledReservationConstraints, reservation_reason: SledReservationReason, + rng: &mut StdRng, ) -> Result { let log = opctx.log.new(o!( @@ -1415,9 +1508,11 @@ impl DataStore { info!(&log, "sled targets: {sled_targets:?}"); - let local_storage_allocation_required = local_storage_disks + let disks_needing_allocation = local_storage_disks .iter() - .any(|disk| disk.local_storage_dataset_allocation.is_none()); + .filter(|disk| disk.local_storage_dataset_allocation.is_none()) + .count(); + let local_storage_allocation_required = disks_needing_allocation > 0; info!( &log, @@ -1432,7 +1527,7 @@ impl DataStore { // In the uncontended case, however, we'll only iterate through this // loop once. - 'sled_reservation: loop { + loop { // Pick a reservation target, given the constraints we previously // saw in the database. let sled_target = pick_sled_reservation_target( @@ -1532,13 +1627,16 @@ impl DataStore { } }; } else { - // If local storage allocation is required, match the requests - // with all the zpools of this sled that have available space. - // The `CompleteLocalStorageAllocationLists` iterator finds all - // possible configurations that would satisfy the requests for - // local storage and tries them all. - - let zpools_for_sled = + // Local storage allocation is required. Choose an assignment + // of unallocated disks to distinct zpools from a fresh + // snapshot of the sled's zpools, then let the insert CTE + // re-validate everything atomically. If the CTE inserts zero + // rows, the snapshot went stale between the fetch and the + // insert (concurrent reservations, crucible allocations, or + // pool and dataset policy changes): take a new snapshot and + // try again, a bounded number of times, before moving to the + // next sled. + let mut zpools_for_sled = DataStore::zpool_get_for_sled_reservation_on_conn( &conn, &opctx, @@ -1546,81 +1644,80 @@ impl DataStore { ) .await?; - if zpools_for_sled.is_empty() { - warn!(&log, "no zpools for {sled_target:?}?"); - - sled_targets.remove(&sled_target); - banned.remove(&sled_target); - unpreferred.remove(&sled_target); - preferred.remove(&sled_target); - - continue 'sled_reservation; - }; + // We only retry here when we fail due to a concurrent + // reservation making our proposed allocation invalid. When this + // happens, at least one of our proposed (disk, zpool) pairings + // must no longer fit. + // + // In scenarios where concurrent actors are performing + // allocations (and not freeing anything!), a pairing that stops + // fitting never fits again. Since we'll only propose pairings + // that fit on a newer snapshot, each failed attempt removes + // one of these pairings. + // There are only disks * zpools pairings, so the sled runs out + // of pairings before this budget runs out. + // + // Admittedly: concurrent frees can revive pairings; under + // sustained free-and-reallocate churn we may give up on the + // sled before it theoretically could satisfy an allocation + // request. + let max_attempts = + disks_needing_allocation * zpools_for_sled.len(); + + 'attempts: for attempt in 0..max_attempts { + if attempt > 0 { + // The previous insert lost its race; take a fresh + // snapshot. + zpools_for_sled = + DataStore::zpool_get_for_sled_reservation_on_conn( + &conn, + &opctx, + sled_target, + ) + .await?; + } - let mut complete_allocation_lists = - match CompleteLocalStorageAllocationLists::new( - &log, - sled_target, - instance_id, - zpools_for_sled, + let allocations = match choose_local_storage_allocations( + &zpools_for_sled, &local_storage_disks, + rng, ) { - Some(complete_allocation_lists) => { - complete_allocation_lists + Ok(allocations) => allocations, + + Err( + LocalStorageUnsatisfiable::NoAllocationsRequired, + ) => { + // This branch is only entered when at least one + // disk needs an allocation. + return Err( + SledReservationTransactionError::Connection( + Error::internal_error( + "local storage allocation required, \ + but no disks need an allocation", + ), + ), + ); } - None => { - // Cannot use this sled, as no complete allocation - // lists exist: `new` will have logged why, so try + Err(reason) => { + // The greedy pairing failing means no assignment + // exists on this sled with the current data. Try // another sled. + info!( + &log, + "sled {sled_target} cannot satisfy local \ + storage allocations: {reason}", + ); - sled_targets.remove(&sled_target); - banned.remove(&sled_target); - unpreferred.remove(&sled_target); - preferred.remove(&sled_target); - - continue 'sled_reservation; + break 'attempts; } }; - // Search for each complete set of local storage allocations - // required, and attempt the sled insert resource query with - // that particular set. - // - // If the `complate_allocation_lists` iterator returns None, - // control will exit the `local_storage_allocation_search` loop, - // which will then try the next possible sled target. In the - // case where there were pre-existing local storage allocations - // there will _not_ be any more sleds to try and the user will - // see a capacity error. - - 'local_storage_allocation_search: loop { - // If other concurrent sled reservations are not taken into - // account, another sled reservation could allocate local - // storage onto the same pools that this one considers - // candidates, and then this iterator will return - // allocations that will never work. Because the iterator - // searches for _every_ possible combination, this will end - // up searching for a long time. It's important to prune the - // list that we're searching from! - - complete_allocation_lists - .prune_invalidated_allocation_lists(&conn, &opctx) - .await?; - - let Some(allocations) = complete_allocation_lists.next() - else { - // All done searching, nothing worked. Try another - // sled! - break 'local_storage_allocation_search; - }; - info!( &log, "attempting to insert sled resource record"; "local_storage_required" => true, "allocations" => ?allocations, - "queue size" => complete_allocation_lists.queue.len(), ); // Try to INSERT the record plus the new local storage @@ -1641,11 +1738,36 @@ impl DataStore { return Ok(resource); } - // Ending up here without inserting any rows is - // expected during concurrent reservations requiring - // local storage allocations, but repeatedly ending - // up here without pruning any allocations is likely - // a bug. + // Zero rows means one of the CTE's local storage + // validity checks failed even though our snapshot + // said everything fit (the sled-level checks all + // raise sentinel errors instead). Either the + // snapshot went stale, or a disk was deleted or + // detached concurrently. In the latter case no + // sled can ever satisfy this reservation, so fail + // now instead of retrying here and on every other + // sled. + if any_request_disk_deleted_or_detached( + &conn, + instance_id, + &local_storage_disks, + ) + .await? + { + info!( + &log, + "local storage disk deleted or detached \ + during reservation", + ); + + return Err( + SledReservationTransactionError::Reservation( + SledReservationError::NotFound, + ), + ); + } + + // Stale snapshot: take a fresh one and recompute. } Err(diesel::result::Error::DatabaseError( @@ -1670,32 +1792,15 @@ impl DataStore { &e, &SLED_INSERT_QUERY_SENTINELS, ) { - // Concurrent sled reservations could have - // allocated enough hardware threads, RSS RAM, - // and/or reservoir RAM that means this - // sled_target is no longer valid. - // - // Alternatively, a concurrent reservation could - // have allocated another instance to this sled - // target which makes it invalid for this - // instance we are trying to allocate due to - // affinity / anti-affinity constraints. - // - // Both of these cases are not a problem when - // _not_ performing local storage allocations: - // in that branch, when the insert query returns - // that it inserted 0 rows or returned an error - // sentinel, another sled_target will be chosen - // right away. - // - // If a sentinel is returned, the insert query - // isn't succeeding for reasons other than the - // available space for local storage. If we - // don't bail out here, the loop will keep - // searching through permutations of local - // storage allocations to try. Bail out of the - // search right away and pick another - // sled_target. + // Sentinels indicate a sled-level problem: + // concurrent reservations could have consumed + // enough hardware threads, RSS RAM, and/or + // reservoir RAM that this sled_target is no + // longer valid, or another instance landed + // here and now violates affinity or + // anti-affinity constraints. Recomputing the + // local storage assignment cannot help; pick + // another sled_target. let reason = sentinel_to_reason(sentinel); info!( @@ -1704,7 +1809,7 @@ impl DataStore { "sentinel" => sentinel, ); - break 'local_storage_allocation_search; + break 'attempts; } else { // The query failed, return this as an error error!( @@ -1720,6 +1825,10 @@ impl DataStore { } } } + + // Attempts exhausted or this sled cannot satisfy the + // allocations; fall through to remove this sled from the + // candidate set and try another. } sled_targets.remove(&sled_target); @@ -2323,6 +2432,7 @@ pub(in crate::db::datastore) mod test { use predicates::{BoxPredicate, prelude::*}; use sled_agent_types::inventory::ZpoolHealth; use std::collections::BTreeMap; + use std::collections::HashMap; use std::net::SocketAddrV6; #[tokio::test] @@ -2897,6 +3007,7 @@ pub(in crate::db::datastore) mod test { instance.resources.clone(), constraints.build(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await?; @@ -6413,6 +6524,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await .unwrap() @@ -6554,6 +6666,7 @@ pub(in crate::db::datastore) mod test { db::model::SledReservationConstraints::none( ), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await } @@ -6738,6 +6851,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await .unwrap() @@ -7741,6 +7855,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await; @@ -7852,6 +7967,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await; @@ -7959,6 +8075,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await; @@ -8071,6 +8188,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await; @@ -8186,6 +8304,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await .unwrap(); @@ -8293,6 +8412,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await .unwrap(); @@ -8401,6 +8521,7 @@ pub(in crate::db::datastore) mod test { instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::from_os_rng(), ) .await .unwrap(); @@ -8512,6 +8633,27 @@ pub(in crate::db::datastore) mod test { }) .collect(); + // Choose allocations based on the stale disk list, as the reservation + // loop would have. + + let allocations = { + let zpools_for_sled = + DataStore::zpool_get_for_sled_reservation_on_conn( + &conn, + &opctx, + config.sleds[0].sled_id, + ) + .await + .unwrap(); + + choose_local_storage_allocations( + &zpools_for_sled, + &local_storage_disks, + &mut StdRng::from_os_rng(), + ) + .unwrap() + }; + // Set `time_deleted` on the first disk { @@ -8527,36 +8669,38 @@ pub(in crate::db::datastore) mod test { .unwrap(); }; - // Duplicate the loop logic that performs the allocation search. + // The insertion CTE must not create any records for allocations that + // reference a deleted disk. - let zpools_for_sled = - DataStore::zpool_get_for_sled_reservation_on_conn( - &conn, - &opctx, - config.sleds[0].sled_id, - ) - .await - .unwrap(); + let resource = SledResourceVmm::new( + PropolisUuid::new_v4(), + instance.id, + config.sleds[0].sled_id, + instance.resources(), + SledReservationReason::Start.into(), + ); - let mut complete_allocation_lists = - CompleteLocalStorageAllocationLists::new( - &logctx.log, - config.sleds[0].sled_id, + let result = sled_insert_resource_query( + &resource, + &LocalStorageAllocationRequired::Yes { allocations }, + ) + .execute_async(&*conn) + .await; + + assert_eq!(result, Ok(0)); + + // The reservation loop treats zero inserted rows plus a deleted disk + // as a terminal failure; check the signal it uses. + + assert!( + any_request_disk_deleted_or_detached( + &conn, instance.id, - zpools_for_sled, &local_storage_disks, ) - .unwrap(); - - // After pruning, there should be no allocations left.to try: a disk was - // deleted, so the search should stop. - - complete_allocation_lists - .prune_invalidated_allocation_lists(&conn, &opctx) .await - .unwrap(); - - assert!(complete_allocation_lists.next().is_none()); + .unwrap() + ); let allocation_records: Vec<_> = { let conn = datastore.pool_connection_for_tests().await.unwrap(); @@ -8698,33 +8842,26 @@ pub(in crate::db::datastore) mod test { SledReservationReason::Start.into(), ); - // Duplicate the loop logic that performs the allocation search + // Choose allocations based on the disk list captured before the + // detach - what we're testing here is that the insertion CTE does not + // perform a reservation if the disk is detached. - let zpools_for_sled = - DataStore::zpool_get_for_sled_reservation_on_conn( - &conn, - &opctx, - config.sleds[0].sled_id, - ) - .await - .unwrap(); + let allocations = { + let zpools_for_sled = + DataStore::zpool_get_for_sled_reservation_on_conn( + &conn, + &opctx, + config.sleds[0].sled_id, + ) + .await + .unwrap(); - let mut complete_allocation_lists = - CompleteLocalStorageAllocationLists::new( - &logctx.log, - config.sleds[0].sled_id, - instance.id, - zpools_for_sled, + choose_local_storage_allocations( + &zpools_for_sled, &local_storage_disks, + &mut StdRng::from_os_rng(), ) - .unwrap(); - - // Skip the pruning step, as that would prune the detached disk - what - // we're testing here is that the insertion CTE does not perform a - // reservation if the disk is detached. - - let Some(allocations) = complete_allocation_lists.next() else { - panic!("no allocations returned!"); + .unwrap() }; let result = sled_insert_resource_query( @@ -8738,6 +8875,19 @@ pub(in crate::db::datastore) mod test { assert_eq!(result, Ok(0)); + // The reservation loop treats zero inserted rows plus a detached disk + // as a terminal failure; check the signal it uses. + + assert!( + any_request_disk_deleted_or_detached( + &conn, + instance.id, + &local_storage_disks, + ) + .await + .unwrap() + ); + let allocation_records: Vec<_> = { let conn = datastore.pool_connection_for_tests().await.unwrap(); @@ -8759,26 +8909,29 @@ pub(in crate::db::datastore) mod test { logctx.cleanup_successful(); } - /// Ensure the iterator that searches through local storage allocations - /// bails out when a concurrent allocation occurs - /// - /// Uses two instances each with 10 disks. + // A local storage assignment computed from a zpool snapshot can go stale + // if a concurrent reservation consumes the chosen pool. The insert CTE + // must reject the stale assignment, and a reservation computed from a + // fresh snapshot must succeed on the remaining pool. #[tokio::test] - async fn local_storage_allocation_iterator_pruning_1() { - let logctx = - dev::test_setup_log("local_storage_allocation_iterator_pruning_1"); + async fn local_storage_allocation_stale_snapshot_retry() { + let logctx = dev::test_setup_log( + "local_storage_allocation_stale_snapshot_retry", + ); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let config = LocalStorageTest { - // One sled, with ten U2 + // One sled with two U2s, where each U2 has room for exactly one + // 512 GiB disk: usable space is 1024 GiB - 250 GiB buffer, and + // each disk requires 512 GiB plus overhead. sleds: vec![LocalStorageTestSled { sled_id: SledUuid::new_v4(), sled_serial: String::from("sled_0"), - u2s: (0..10) + u2s: (0..2) .map(|i| LocalStorageTestSledU2 { physical_disk_id: PhysicalDiskUuid::new_v4(), - physical_disk_serial: format!("phys{i}"), + physical_disk_serial: format!("phys-{i}"), zpool_id: ZpoolUuid::new_v4(), control_plane_storage_buffer: @@ -8801,144 +8954,175 @@ pub(in crate::db::datastore) mod test { }], affinity_groups: vec![], anti_affinity_groups: vec![], - // Two instances, with ten local storage disks each. - instances: vec![ - LocalStorageTestInstance { + // Three instances with one 512 GiB local storage disk each: only + // two can fit on this sled. + instances: ["one", "two", "three"] + .iter() + .map(|name| LocalStorageTestInstance { id: InstanceUuid::new_v4(), - name: "local1".to_string(), + name: name.to_string(), affinity: None, ncpus: 2, memory: external::ByteCount::from_gibibytes_u32(16), - disks: (0..10) - .map(|i| LocalStorageTestInstanceDisk { - id: Uuid::new_v4(), - name: external::Name::try_from(format!( - "local1-{i}" - )) - .unwrap(), - size: external::ByteCount::from_gibibytes_u32(512), - }) - .collect(), - }, - LocalStorageTestInstance { - id: InstanceUuid::new_v4(), - name: "local2".to_string(), - ncpus: 2, - memory: external::ByteCount::from_gibibytes_u32(16), - affinity: None, - disks: (0..10) - .map(|i| LocalStorageTestInstanceDisk { - id: Uuid::new_v4(), - name: external::Name::try_from(format!( - "local2-{i}" - )) + disks: vec![LocalStorageTestInstanceDisk { + id: Uuid::new_v4(), + name: external::Name::try_from(format!("disk-{name}")) .unwrap(), - size: external::ByteCount::from_gibibytes_u32(512), - }) - .collect(), - }, - ], + size: external::ByteCount::from_gibibytes_u32(512), + }], + }) + .collect(), }; setup_local_storage_allocation_test(&opctx, datastore, &config).await; - // Create the iterator for the second instance's search - let conn = datastore.pool_connection_for_tests().await.unwrap(); - let instance_2_local_storage_disks: Vec = { - datastore - .instance_list_disks_on_conn( - &conn, - config.instances[1].id.into_untyped_uuid(), - &PaginatedBy::Name(DataPageParams { - marker: None, - direction: dropshot::PaginationOrder::Ascending, - limit: std::num::NonZeroU32::new( - MAX_DISKS_PER_INSTANCE, - ) + // Compute an assignment for the second instance's disk. The chooser + // is deterministic for a given rng seed, and the first instance's + // reservation below runs against an identical snapshot with the same + // seed, so both pick the same pool. + + let instance_two = + Instance::from_local_storage_test_instance(&config.instances[1]); + + let instance_two_disks: Vec = datastore + .instance_list_disks_on_conn( + &conn, + instance_two.id.into_untyped_uuid(), + &PaginatedBy::Name(DataPageParams { + marker: None, + direction: dropshot::PaginationOrder::Ascending, + limit: std::num::NonZeroU32::new(MAX_DISKS_PER_INSTANCE) .unwrap(), - }), - ) - .await - .unwrap() - .into_iter() - .filter_map(|disk| match disk { - db::datastore::Disk::LocalStorage(disk) => Some(disk), - db::datastore::Disk::Crucible(_) => None, - }) - .collect() - }; + }), + ) + .await + .unwrap() + .into_iter() + .filter_map(|disk| match disk { + db::datastore::Disk::LocalStorage(disk) => Some(disk), + db::datastore::Disk::Crucible(_) => None, + }) + .collect(); - let mut iterator = { - let zpools_for_sled = datastore - .zpool_get_for_sled_reservation(&opctx, config.sleds[0].sled_id) + let stale_allocations = { + let zpools_for_sled = + DataStore::zpool_get_for_sled_reservation_on_conn( + &conn, + &opctx, + config.sleds[0].sled_id, + ) .await .unwrap(); - CompleteLocalStorageAllocationLists::new( - &logctx.log, - config.sleds[0].sled_id, - config.instances[1].id, - zpools_for_sled, - &instance_2_local_storage_disks, + choose_local_storage_allocations( + &zpools_for_sled, + &instance_two_disks, + &mut StdRng::seed_from_u64(0), ) .unwrap() }; - // Perform the concurrent reservation for first instance + // Reserve the first instance with the same seed; this consumes the + // pool the stale assignment chose. { let instance = Instance::from_local_storage_test_instance( &config.instances[0], ); - let instance_id = instance.id; - let resources = instance.resources(); datastore - .sled_reservation_create( + .sled_reservation_create_inner( &opctx, - instance_id, + instance.id, PropolisUuid::new_v4(), - resources, + instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut StdRng::seed_from_u64(0), ) .await .unwrap(); } - // After pruning, the iterator should not yield any more: the whole - // search space should have been pruned. + // The insert CTE must reject the stale assignment: its pool no + // longer has room. This is the signal the reservation loop consumes + // before recomputing from a fresh snapshot. + + let resource = SledResourceVmm::new( + PropolisUuid::new_v4(), + instance_two.id, + config.sleds[0].sled_id, + instance_two.resources(), + SledReservationReason::Start.into(), + ); - iterator - .prune_invalidated_allocation_lists(&conn, &opctx) + let result = sled_insert_resource_query( + &resource, + &LocalStorageAllocationRequired::Yes { + allocations: stale_allocations, + }, + ) + .execute_async(&*conn) + .await; + + assert_eq!(result, Ok(0)); + + // A full reservation for the second instance succeeds: recomputing + // from a fresh snapshot lands on the remaining pool. + + datastore + .sled_reservation_create( + &opctx, + instance_two.id, + PropolisUuid::new_v4(), + instance_two.resources(), + db::model::SledReservationConstraints::none(), + SledReservationReason::Start, + ) .await .unwrap(); - assert!(iterator.next().is_none()); + // Both pools are now full, so a third reservation fails cleanly. + + { + let instance = Instance::from_local_storage_test_instance( + &config.instances[2], + ); + + datastore + .sled_reservation_create( + &opctx, + instance.id, + PropolisUuid::new_v4(), + instance.resources(), + db::model::SledReservationConstraints::none(), + SledReservationReason::Start, + ) + .await + .unwrap_err(); + } + + validate_local_storage_allocations(&datastore).await; db.terminate().await; logctx.cleanup_successful(); } - /// Ensure the iterator that searches through local storage allocations - /// bails out when a concurrent allocation occurs - /// - /// Uses one instance with 1 disk, and one instance with 10 disks. + /// What happens when you ask for more disks than there are zpools? #[tokio::test] - async fn local_storage_allocation_iterator_pruning_2() { - let logctx = - dev::test_setup_log("local_storage_allocation_iterator_pruning_2"); + async fn local_storage_allocation_too_many() { + let logctx = dev::test_setup_log("local_storage_allocation_too_many"); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let config = LocalStorageTest { - // One sled, with ten U2 + // One sled, with five U2 sleds: vec![LocalStorageTestSled { sled_id: SledUuid::new_v4(), sled_serial: String::from("sled_0"), - u2s: (0..10) + u2s: (0..5) .map(|i| LocalStorageTestSledU2 { physical_disk_id: PhysicalDiskUuid::new_v4(), physical_disk_serial: format!("phys{i}"), @@ -8964,93 +9148,27 @@ pub(in crate::db::datastore) mod test { }], affinity_groups: vec![], anti_affinity_groups: vec![], - // Two instances, one with one disk, one with ten local storage - // disks. - instances: vec![ - LocalStorageTestInstance { - id: InstanceUuid::new_v4(), - name: "local1".to_string(), - affinity: None, - ncpus: 2, - memory: external::ByteCount::from_gibibytes_u32(16), - disks: (0..1) - .map(|i| LocalStorageTestInstanceDisk { - id: Uuid::new_v4(), - name: external::Name::try_from(format!( - "local1-{i}" - )) - .unwrap(), - size: external::ByteCount::from_gibibytes_u32(512), - }) - .collect(), - }, - LocalStorageTestInstance { - id: InstanceUuid::new_v4(), - name: "local2".to_string(), - ncpus: 2, - memory: external::ByteCount::from_gibibytes_u32(16), - affinity: None, - disks: (0..10) - .map(|i| LocalStorageTestInstanceDisk { - id: Uuid::new_v4(), - name: external::Name::try_from(format!( - "local2-{i}" - )) + // One instance with ten local storage disks. + instances: vec![LocalStorageTestInstance { + id: InstanceUuid::new_v4(), + name: "local1".to_string(), + affinity: None, + ncpus: 2, + memory: external::ByteCount::from_gibibytes_u32(16), + disks: (0..10) + .map(|i| LocalStorageTestInstanceDisk { + id: Uuid::new_v4(), + name: external::Name::try_from(format!("local1-{i}")) .unwrap(), - size: external::ByteCount::from_gibibytes_u32(512), - }) - .collect(), - }, - ], + size: external::ByteCount::from_gibibytes_u32(512), + }) + .collect(), + }], }; setup_local_storage_allocation_test(&opctx, datastore, &config).await; - // Create the iterator for the second instance's search - - let conn = datastore.pool_connection_for_tests().await.unwrap(); - - let instance_2_local_storage_disks: Vec = { - datastore - .instance_list_disks_on_conn( - &conn, - config.instances[1].id.into_untyped_uuid(), - &PaginatedBy::Name(DataPageParams { - marker: None, - direction: dropshot::PaginationOrder::Ascending, - limit: std::num::NonZeroU32::new( - MAX_DISKS_PER_INSTANCE, - ) - .unwrap(), - }), - ) - .await - .unwrap() - .into_iter() - .filter_map(|disk| match disk { - db::datastore::Disk::LocalStorage(disk) => Some(disk), - db::datastore::Disk::Crucible(_) => None, - }) - .collect() - }; - - let mut iterator = { - let zpools_for_sled = datastore - .zpool_get_for_sled_reservation(&opctx, config.sleds[0].sled_id) - .await - .unwrap(); - - CompleteLocalStorageAllocationLists::new( - &logctx.log, - config.sleds[0].sled_id, - config.instances[1].id, - zpools_for_sled, - &instance_2_local_storage_disks, - ) - .unwrap() - }; - - // Perform the concurrent reservation for first instance + // Sled reservation should _not_ succeed { let instance = Instance::from_local_storage_test_instance( @@ -9069,42 +9187,67 @@ pub(in crate::db::datastore) mod test { SledReservationReason::Start, ) .await - .unwrap(); + .unwrap_err(); } - // After pruning, the iterator should not yield any more: the whole - // search space should have been pruned. + let allocation_records: Vec<_> = { + let conn = datastore.pool_connection_for_tests().await.unwrap(); - iterator - .prune_invalidated_allocation_lists(&conn, &opctx) - .await - .unwrap(); + use nexus_db_schema::schema::local_storage_unencrypted_dataset_allocation::dsl; + + dsl::local_storage_unencrypted_dataset_allocation + .filter(dsl::time_deleted.is_null()) + .select(db::model::LocalStorageUnencryptedDatasetAllocation::as_select()) + .load_async(&*conn) + .await + .unwrap() + }; + + assert_eq!(allocation_records.len(), 0); - assert!(iterator.next().is_none()); + validate_local_storage_allocations(&datastore).await; db.terminate().await; logctx.cleanup_successful(); } - /// What happens when you ask for more disks than there are zpools? + /// Reserve seven instances one at a time, each with seven local storage + /// disks sized in descending halves of a pool's usable space. The disks + /// fit in aggregate, but whether each reservation succeeds depends on the + /// placement choices made for the reservations before it: a pool must + /// survive with room for the largest disk. + /// + /// A placement rule that always picks the roomiest fitting pool levels + /// every pool down together and fails this workload at the seventh + /// instance, for every rng seed. Random placement succeeds for nearly + /// every seed; this test pins one such seed, along with fixed zpool ids + /// so the seed selects the same pools on every run. #[tokio::test] - async fn local_storage_allocation_iterator_too_many() { + async fn local_storage_allocation_descending_serial() { let logctx = - dev::test_setup_log("local_storage_allocation_iterator_too_many"); + dev::test_setup_log("local_storage_allocation_descending_serial"); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); + let mut rng = StdRng::seed_from_u64(0); + + // Each disk is sized just over half of what remains of a pool's 774 + // GiB of usable space after the disks larger than it. + let disk_gb: Vec = vec![388, 195, 98, 50, 26, 14, 8]; + let config = LocalStorageTest { - // One sled, with five U2 + // One sled, with ten U2 sleds: vec![LocalStorageTestSled { sled_id: SledUuid::new_v4(), sled_serial: String::from("sled_0"), - u2s: (0..5) + u2s: (0..10) .map(|i| LocalStorageTestSledU2 { physical_disk_id: PhysicalDiskUuid::new_v4(), physical_disk_serial: format!("phys{i}"), - zpool_id: ZpoolUuid::new_v4(), + zpool_id: ZpoolUuid::from_untyped_uuid( + Uuid::from_u128(0x51ed_2001 + i), + ), control_plane_storage_buffer: external::ByteCount::from_gibibytes_u32(250), @@ -9125,63 +9268,51 @@ pub(in crate::db::datastore) mod test { }], affinity_groups: vec![], anti_affinity_groups: vec![], - // One instance with ten local storage disks. - instances: vec![LocalStorageTestInstance { - id: InstanceUuid::new_v4(), - name: "local1".to_string(), - affinity: None, - ncpus: 2, - memory: external::ByteCount::from_gibibytes_u32(16), - disks: (0..10) - .map(|i| LocalStorageTestInstanceDisk { - id: Uuid::new_v4(), - name: external::Name::try_from(format!("local1-{i}")) + // 7 instances, each with 7 disks + instances: (0..7) + .map(|i| LocalStorageTestInstance { + id: InstanceUuid::new_v4(), + name: format!("local{i}"), + affinity: None, + ncpus: 2, + memory: external::ByteCount::from_gibibytes_u32(2), + disks: (0..7) + .map(|j| LocalStorageTestInstanceDisk { + id: Uuid::new_v4(), + name: external::Name::try_from(format!( + "local{i}-{j}" + )) .unwrap(), - size: external::ByteCount::from_gibibytes_u32(512), - }) - .collect(), - }], + size: external::ByteCount::from_gibibytes_u32( + disk_gb[j], + ), + }) + .collect(), + }) + .collect(), }; setup_local_storage_allocation_test(&opctx, datastore, &config).await; - // Sled reservation should _not_ succeed - - { - let instance = Instance::from_local_storage_test_instance( - &config.instances[0], - ); - let instance_id = instance.id; - let resources = instance.resources(); + // All reservations should succeed, issued one at a time. + for config_instance in &config.instances { + let instance = + Instance::from_local_storage_test_instance(config_instance); datastore - .sled_reservation_create( + .sled_reservation_create_inner( &opctx, - instance_id, + instance.id, PropolisUuid::new_v4(), - resources, + instance.resources(), db::model::SledReservationConstraints::none(), SledReservationReason::Start, + &mut rng, ) .await - .unwrap_err(); + .unwrap(); } - let allocation_records: Vec<_> = { - let conn = datastore.pool_connection_for_tests().await.unwrap(); - - use nexus_db_schema::schema::local_storage_unencrypted_dataset_allocation::dsl; - - dsl::local_storage_unencrypted_dataset_allocation - .filter(dsl::time_deleted.is_null()) - .select(db::model::LocalStorageUnencryptedDatasetAllocation::as_select()) - .load_async(&*conn) - .await - .unwrap() - }; - - assert_eq!(allocation_records.len(), 0); - validate_local_storage_allocations(&datastore).await; db.terminate().await; diff --git a/nexus/db-queries/src/db/datastore/zpool.rs b/nexus/db-queries/src/db/datastore/zpool.rs index 1dd045e9972..139d167f5d5 100644 --- a/nexus/db-queries/src/db/datastore/zpool.rs +++ b/nexus/db-queries/src/db/datastore/zpool.rs @@ -64,19 +64,39 @@ pub struct ZpoolGetForSledReservationResult { } impl ZpoolGetForSledReservationResult { - /// Does this Zpool have room for additional bytes to be allocated to it? - pub fn has_room_for_allocation(&self, additional_size: i64) -> bool { - let new_size_used: i64 = self.crucible_dataset_usage - + self.local_storage_usage - + additional_size; - + /// Bytes available for new allocations on this zpool: the last reported + /// inventory size, minus the control plane storage buffer, minus the + /// upper bound on existing usage. An allocation of `additional_size` + /// bytes fits if and only if `additional_size < self.headroom()`, + /// matching the strict comparison performed by + /// `sled_insert_resource_query`. + pub fn headroom(&self) -> i64 { let control_plane_storage_buffer: i64 = self.pool.control_plane_storage_buffer().into(); - let adjusted_total_available: i64 = - self.last_inv_total_size - control_plane_storage_buffer; + self.last_inv_total_size + - control_plane_storage_buffer + - self.crucible_dataset_usage + - self.local_storage_usage + } - new_size_used < adjusted_total_available + /// Construct a result directly for tests; production code builds these + /// only via `zpool_get_for_sled_reservation`. + #[cfg(test)] + pub(crate) fn new_for_test( + pool: Zpool, + last_inv_total_size: i64, + rendezvous_local_storage_unencrypted_dataset_id: DatasetUuid, + crucible_dataset_usage: i64, + local_storage_usage: i64, + ) -> Self { + Self { + pool, + last_inv_total_size, + rendezvous_local_storage_unencrypted_dataset_id, + crucible_dataset_usage, + local_storage_usage, + } } } diff --git a/nexus/db-queries/src/db/queries/sled_reservation.rs b/nexus/db-queries/src/db/queries/sled_reservation.rs index 137727ca9d2..6a504c51d3c 100644 --- a/nexus/db-queries/src/db/queries/sled_reservation.rs +++ b/nexus/db-queries/src/db/queries/sled_reservation.rs @@ -16,15 +16,16 @@ use nexus_db_schema::enums::SledCpuFamilyEnum; use nexus_db_schema::enums::SledResourceVmmStateEnum; use nonempty::NonEmpty; use omicron_uuid_kinds::DatasetUuid; +use omicron_uuid_kinds::DiskUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; -use uuid::Uuid; #[derive(Debug, Clone)] pub struct LocalStorageAllocation { - pub disk_id: Uuid, + /// The virtual disk requiring this allocation + pub disk_id: DiskUuid, pub local_storage_unencrypted_dataset_allocation_id: DatasetUuid, pub required_dataset_size: i64, pub local_storage_unencrypted_dataset_id: DatasetUuid, @@ -614,7 +615,9 @@ pub fn sled_insert_resource_query( .bind::(instance_id) .sql(" FROM DISK WHERE ID = ") .param() - .bind::(allocation.disk_id); + .bind::( + allocation.disk_id.into_untyped_uuid(), + ); query.sql(") "); @@ -676,7 +679,7 @@ pub fn sled_insert_resource_query( query .sql("WHEN ") .param() - .bind::(*disk_id) + .bind::(disk_id.into_untyped_uuid()) .sql(" THEN ") .param() .bind::( @@ -691,7 +694,9 @@ pub fn sled_insert_resource_query( for (index, allocation) in allocations.iter().enumerate() { let LocalStorageAllocation { disk_id, .. } = allocation; - query.param().bind::(*disk_id); + query + .param() + .bind::(disk_id.into_untyped_uuid()); if index != (allocations.len() - 1) { query.sql(","); @@ -1010,7 +1015,7 @@ mod test { &LocalStorageAllocationRequired::Yes { allocations: nonempty![ LocalStorageAllocation { - disk_id: Uuid::nil(), + disk_id: DiskUuid::nil(), local_storage_unencrypted_dataset_allocation_id: DatasetUuid::nil(), required_dataset_size: 64 * 1024 * 1024 * 1024, @@ -1020,7 +1025,7 @@ mod test { sled_id: SledUuid::nil(), }, LocalStorageAllocation { - disk_id: Uuid::nil(), + disk_id: DiskUuid::nil(), local_storage_unencrypted_dataset_allocation_id: DatasetUuid::nil(), required_dataset_size: 128 * 1024 * 1024 * 1024, @@ -1080,7 +1085,7 @@ mod test { &resource, &LocalStorageAllocationRequired::Yes { allocations: nonempty![LocalStorageAllocation { - disk_id: Uuid::nil(), + disk_id: DiskUuid::nil(), local_storage_unencrypted_dataset_allocation_id: DatasetUuid::nil(), required_dataset_size: 128 * 1024 * 1024 * 1024, @@ -1101,7 +1106,7 @@ mod test { &LocalStorageAllocationRequired::Yes { allocations: nonempty![ LocalStorageAllocation { - disk_id: Uuid::nil(), + disk_id: DiskUuid::nil(), local_storage_unencrypted_dataset_allocation_id: DatasetUuid::nil(), required_dataset_size: 128 * 1024 * 1024 * 1024, @@ -1111,7 +1116,7 @@ mod test { sled_id: SledUuid::nil(), }, LocalStorageAllocation { - disk_id: Uuid::nil(), + disk_id: DiskUuid::nil(), local_storage_unencrypted_dataset_allocation_id: DatasetUuid::nil(), required_dataset_size: 256 * 1024 * 1024 * 1024, diff --git a/uuid-kinds/src/lib.rs b/uuid-kinds/src/lib.rs index 945afdedead..db975898a1d 100644 --- a/uuid-kinds/src/lib.rs +++ b/uuid-kinds/src/lib.rs @@ -56,6 +56,9 @@ impl_typed_uuid_kinds! { ConsoleSession = {}, Dataset = {}, DemoSaga = {}, + // A virtual disk (the customer-facing `disk` resource), as opposed to + // PhysicalDisk below. + Disk = {}, Downstairs = {}, DownstairsRegion = {}, EreporterRestart = {},