From ae4139961252c065e1b6849af8cc79530a3fd3bf Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 25 Jun 2024 13:41:29 -0700 Subject: [PATCH] Merge zpool selection fns --- Cargo.lock | 1 - nexus/inventory/Cargo.toml | 1 - .../planning/src/blueprint_builder/builder.rs | 98 ++++++------------- nexus/reconfigurator/planning/src/planner.rs | 2 +- nexus/types/src/deployment/zone_type.rs | 31 +----- 5 files changed, 35 insertions(+), 98 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3f6773ea9c..f9ae4c29c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4720,7 +4720,6 @@ dependencies = [ "omicron-sled-agent", "omicron-uuid-kinds", "omicron-workspace-hack", - "rand 0.8.5", "regex", "reqwest", "serde_json", diff --git a/nexus/inventory/Cargo.toml b/nexus/inventory/Cargo.toml index 2fbfbace21..e185808caa 100644 --- a/nexus/inventory/Cargo.toml +++ b/nexus/inventory/Cargo.toml @@ -31,6 +31,5 @@ omicron-workspace-hack.workspace = true expectorate.workspace = true gateway-test-utils.workspace = true omicron-sled-agent.workspace = true -rand.workspace = true regex.workspace = true tokio.workspace = true diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 2931801dcd..94276fd17b 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -6,7 +6,6 @@ use crate::ip_allocator::IpAllocator; use crate::planner::zone_needs_expungement; -use crate::planner::DiscretionaryOmicronZone; use crate::planner::ZoneExpungeReason; use anyhow::anyhow; use internal_dns::config::Host; @@ -50,7 +49,6 @@ use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; -use rand::prelude::IteratorRandom; use rand::rngs::StdRng; use rand::SeedableRng; use sled_agent_client::ZoneKind; @@ -667,7 +665,7 @@ impl<'a> BlueprintBuilder<'a> { domain: None, }); let filesystem_pool = - self.sled_select_filesystem_pool(sled_id, &zone_type)?; + self.sled_select_zpool(sled_id, zone_type.kind())?; let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -720,10 +718,9 @@ impl<'a> BlueprintBuilder<'a> { let zone_type = BlueprintZoneType::Crucible(blueprint_zone_type::Crucible { address, - dataset: OmicronZoneDataset { pool_name }, + dataset: OmicronZoneDataset { pool_name: pool_name.clone() }, }); - let filesystem_pool = - self.sled_select_filesystem_pool(sled_id, &zone_type)?; + let filesystem_pool = pool_name; let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -851,7 +848,7 @@ impl<'a> BlueprintBuilder<'a> { external_dns_servers: external_dns_servers.clone(), }); let filesystem_pool = - self.sled_select_filesystem_pool(sled_id, &zone_type)?; + self.sled_select_zpool(sled_id, zone_type.kind())?; let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -895,20 +892,19 @@ impl<'a> BlueprintBuilder<'a> { for _ in 0..num_crdb_to_add { let zone_id = self.rng.zone_rng.next(); let underlay_ip = self.sled_alloc_ip(sled_id)?; - let pool_name = self.sled_alloc_zpool( - sled_id, - DiscretionaryOmicronZone::CockroachDb, - )?; + let pool_name = + self.sled_select_zpool(sled_id, ZoneKind::CockroachDb)?; let port = omicron_common::address::COCKROACH_PORT; let address = SocketAddrV6::new(underlay_ip, port, 0, 0); let zone_type = BlueprintZoneType::CockroachDb( blueprint_zone_type::CockroachDb { address, - dataset: OmicronZoneDataset { pool_name }, + dataset: OmicronZoneDataset { + pool_name: pool_name.clone(), + }, }, ); - let filesystem_pool = - self.sled_select_filesystem_pool(sled_id, &zone_type)?; + let filesystem_pool = pool_name; let zone = BlueprintZoneConfig { disposition: BlueprintZoneDisposition::InService, @@ -923,36 +919,6 @@ impl<'a> BlueprintBuilder<'a> { Ok(EnsureMultiple::Changed { added: num_crdb_to_add, removed: 0 }) } - // Selects a filesystem zpool for this zone type. - // - // For zones containing durable datasets, the filesystem pools are - // co-located, and determistic. - // - // For zones without durable datasets, the filesystem pools are randomly - // selected. This random choice makes this function not idempotent. - fn sled_select_filesystem_pool( - &self, - sled_id: SledUuid, - zone_type: &BlueprintZoneType, - ) -> Result { - let resources = self.sled_resources(sled_id)?; - - // Transient filesystem is co-located with durable dataset. - if let Some(dataset) = zone_type.durable_dataset() { - return Ok(dataset.pool_name.clone()); - } - - // No durable dataset exists; pick one randomly. - resources - .all_zpools(ZpoolFilter::InService) - .map(|id| ZpoolName::new_external(*id)) - .choose(&mut rand::thread_rng()) - .ok_or_else(|| Error::NoAvailableZpool { - sled_id, - kind: zone_type.kind(), - }) - } - fn sled_add_zone( &mut self, sled_id: SledUuid, @@ -1009,44 +975,40 @@ impl<'a> BlueprintBuilder<'a> { allocator.alloc().ok_or(Error::OutOfAddresses { sled_id }) } - fn sled_alloc_zpool( - &mut self, + // Selects a zpools for this zone type. + // + // This zpool may be used for either durable storage or transient + // storage - the usage is up to the caller. + // + // If `zone_kind` already exists on `sled_id`, it is prevented + // from using the same zpool as exisitng zones with the same kind. + fn sled_select_zpool( + &self, sled_id: SledUuid, - kind: DiscretionaryOmicronZone, + zone_kind: ZoneKind, ) -> Result { - let resources = self.sled_resources(sled_id)?; + let all_in_service_zpools = + self.sled_resources(sled_id)?.all_zpools(ZpoolFilter::InService); - // We refuse to choose a zpool for a zone of a given `kind` if this - // sled already has a zone of that kind on the same zpool. Build up a - // set of invalid zpools for this sled/kind pair. + // We refuse to choose a zpool for a zone of a given `zone_kind` if this + // sled already has a durable zone of that kind on the same zpool. Build + // up a set of invalid zpools for this sled/kind pair. let mut skip_zpools = BTreeSet::new(); for zone_config in self.current_sled_zones(sled_id) { - match (kind, &zone_config.zone_type) { - ( - DiscretionaryOmicronZone::Nexus, - BlueprintZoneType::Nexus(_), - ) => { - // TODO handle this case once we track transient datasets + if let Some(zpool) = zone_config.zone_type.durable_zpool() { + if zone_kind == zone_config.zone_type.kind() { + skip_zpools.insert(zpool); } - ( - DiscretionaryOmicronZone::CockroachDb, - BlueprintZoneType::CockroachDb(crdb), - ) => { - skip_zpools.insert(&crdb.dataset.pool_name); - } - (DiscretionaryOmicronZone::Nexus, _) - | (DiscretionaryOmicronZone::CockroachDb, _) => (), } } - for &zpool_id in resources.all_zpools(ZpoolFilter::InService) { + for &zpool_id in all_in_service_zpools { let zpool_name = ZpoolName::new_external(zpool_id); if !skip_zpools.contains(&zpool_name) { return Ok(zpool_name); } } - - Err(Error::NoAvailableZpool { sled_id, kind: kind.into() }) + Err(Error::NoAvailableZpool { sled_id, kind: zone_kind }) } fn sled_resources( diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 4e288e8d8a..a4756f8405 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -672,7 +672,7 @@ pub(crate) fn zone_needs_expungement( } // Should we expunge the zone because durable storage is gone? - if let Some(durable_storage_zpool) = zone_config.zone_type.zpool() { + if let Some(durable_storage_zpool) = zone_config.zone_type.durable_zpool() { let zpool_id = durable_storage_zpool.id(); if !sled_details.resources.zpool_is_provisionable(&zpool_id) { return Some(ZoneExpungeReason::DiskExpunged); diff --git a/nexus/types/src/deployment/zone_type.rs b/nexus/types/src/deployment/zone_type.rs index f2ac9d5a65..4c40bfc1de 100644 --- a/nexus/types/src/deployment/zone_type.rs +++ b/nexus/types/src/deployment/zone_type.rs @@ -35,33 +35,10 @@ pub enum BlueprintZoneType { impl BlueprintZoneType { /// Returns the zpool being used by this zone, if any. - pub fn zpool(&self) -> Option<&omicron_common::zpool_name::ZpoolName> { - match self { - BlueprintZoneType::ExternalDns( - blueprint_zone_type::ExternalDns { dataset, .. }, - ) - | BlueprintZoneType::Clickhouse( - blueprint_zone_type::Clickhouse { dataset, .. }, - ) - | BlueprintZoneType::ClickhouseKeeper( - blueprint_zone_type::ClickhouseKeeper { dataset, .. }, - ) - | BlueprintZoneType::CockroachDb( - blueprint_zone_type::CockroachDb { dataset, .. }, - ) - | BlueprintZoneType::Crucible(blueprint_zone_type::Crucible { - dataset, - .. - }) - | BlueprintZoneType::InternalDns( - blueprint_zone_type::InternalDns { dataset, .. }, - ) => Some(&dataset.pool_name), - BlueprintZoneType::BoundaryNtp(_) - | BlueprintZoneType::InternalNtp(_) - | BlueprintZoneType::Nexus(_) - | BlueprintZoneType::Oximeter(_) - | BlueprintZoneType::CruciblePantry(_) => None, - } + pub fn durable_zpool( + &self, + ) -> Option<&omicron_common::zpool_name::ZpoolName> { + self.durable_dataset().map(|dataset| &dataset.pool_name) } pub fn external_networking(