From 46f2be699280a76795fb52ef8883f114f4a1bfb4 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Apr 2024 17:44:34 -0700 Subject: [PATCH 01/12] [common] Share a common implementation of ZpoolName --- Cargo.lock | 4 +- clients/sled-agent-client/src/lib.rs | 2 + common/Cargo.toml | 1 + common/src/lib.rs | 1 + common/src/zpool_name.rs | 279 ++++++++++++++++++ illumos-utils/src/zfs.rs | 3 +- illumos-utils/src/zpool.rs | 267 +---------------- nexus/db-model/src/omicron_zone_config.rs | 3 +- nexus/reconfigurator/execution/Cargo.toml | 1 - .../reconfigurator/execution/src/datasets.rs | 25 +- nexus/reconfigurator/planning/Cargo.toml | 1 - .../planning/src/blueprint_builder/builder.rs | 14 +- nexus/reconfigurator/preparation/Cargo.toml | 1 - nexus/types/src/inventory.rs | 2 +- openapi/nexus-internal.json | 6 +- sled-agent/src/params.rs | 2 +- sled-hardware/src/disk.rs | 3 +- sled-storage/src/disk.rs | 2 +- sled-storage/src/manager_test_harness.rs | 4 +- 19 files changed, 308 insertions(+), 313 deletions(-) create mode 100644 common/src/zpool_name.rs diff --git a/Cargo.lock b/Cargo.lock index 0f5477f077..aeebb30e6d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4725,7 +4725,6 @@ dependencies = [ "dns-service-client", "futures", "httptest", - "illumos-utils", "internal-dns", "ipnet", "nexus-config", @@ -4762,7 +4761,6 @@ dependencies = [ "debug-ignore", "expectorate", "gateway-client", - "illumos-utils", "indexmap 2.2.6", "internal-dns", "ipnet", @@ -4791,7 +4789,6 @@ version = "0.1.0" dependencies = [ "anyhow", "futures", - "illumos-utils", "nexus-db-model", "nexus-db-queries", "nexus-types", @@ -5228,6 +5225,7 @@ dependencies = [ "test-strategy", "thiserror", "tokio", + "toml 0.8.12", "uuid 1.8.0", ] diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index 1986280125..59f03bd861 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/clients/sled-agent-client/src/lib.rs @@ -48,6 +48,8 @@ progenitor::generate_api!( Vni = omicron_common::api::external::Vni, NetworkInterface = omicron_common::api::internal::shared::NetworkInterface, TypedUuidForZpoolKind = omicron_uuid_kinds::ZpoolUuid, + ZpoolKind = omicron_common::zpool_name::ZpoolKind, + ZpoolName = omicron_common::zpool_name::ZpoolName, } ); diff --git a/common/Cargo.toml b/common/Cargo.toml index 0485d3973b..8b54733405 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -55,6 +55,7 @@ libc.workspace = true regress.workspace = true serde_urlencoded.workspace = true tokio = { workspace = true, features = ["test-util"] } +toml.workspace = true [features] testing = ["proptest", "test-strategy"] diff --git a/common/src/lib.rs b/common/src/lib.rs index 24fa4dfba0..a92237adfa 100644 --- a/common/src/lib.rs +++ b/common/src/lib.rs @@ -28,6 +28,7 @@ pub mod disk; pub mod ledger; pub mod update; pub mod vlan; +pub mod zpool_name; pub use update::hex_schema; diff --git a/common/src/zpool_name.rs b/common/src/zpool_name.rs new file mode 100644 index 0000000000..df5ca8ea31 --- /dev/null +++ b/common/src/zpool_name.rs @@ -0,0 +1,279 @@ +// 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/. + +//! Zpool labels and kinds shared between Nexus and Sled Agents + +use camino::{Utf8Path, Utf8PathBuf}; +use omicron_uuid_kinds::ZpoolUuid; +use schemars::JsonSchema; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use std::fmt; +use std::str::FromStr; +pub const ZPOOL_EXTERNAL_PREFIX: &str = "oxp_"; +pub const ZPOOL_INTERNAL_PREFIX: &str = "oxi_"; + +/// Describes the different classes of Zpools. +#[derive( + Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, JsonSchema, +)] +#[serde(rename_all = "snake_case")] +pub enum ZpoolKind { + // This zpool is used for external storage (u.2) + External, + // This zpool is used for internal storage (m.2) + Internal, +} + +/// A wrapper around a zpool name. +/// +/// This expects that the format will be: `ox{i,p}_` - we parse the prefix +/// when reading the structure, and validate that the UUID can be utilized. +#[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] +pub struct ZpoolName { + id: ZpoolUuid, + kind: ZpoolKind, +} + +const ZPOOL_NAME_REGEX: &str = r"^ox[ip]_[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"; + +/// Custom JsonSchema implementation to encode the constraints on Name. +impl JsonSchema for ZpoolName { + fn schema_name() -> String { + "ZpoolName".to_string() + } + fn json_schema( + _: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + schemars::schema::SchemaObject { + metadata: Some(Box::new(schemars::schema::Metadata { + title: Some( + "The name of a Zpool".to_string(), + ), + description: Some( + "Zpool names are of the format ox{i,p}_. They are either \ + Internal or External, and should be unique" + .to_string(), + ), + ..Default::default() + })), + instance_type: Some(schemars::schema::InstanceType::String.into()), + string: Some(Box::new(schemars::schema::StringValidation { + pattern: Some(ZPOOL_NAME_REGEX.to_owned()), + ..Default::default() + })), + ..Default::default() + } + .into() + } +} + +impl ZpoolName { + pub fn new_internal(id: ZpoolUuid) -> Self { + Self { id, kind: ZpoolKind::Internal } + } + + pub fn new_external(id: ZpoolUuid) -> Self { + Self { id, kind: ZpoolKind::External } + } + + pub fn id(&self) -> ZpoolUuid { + self.id + } + + pub fn kind(&self) -> ZpoolKind { + self.kind + } + + /// Returns a path to a dataset's mountpoint within the zpool. + /// + /// For example: oxp_(UUID) -> /pool/ext/(UUID)/(dataset) + pub fn dataset_mountpoint( + &self, + root: &Utf8Path, + dataset: &str, + ) -> Utf8PathBuf { + let mut path = Utf8PathBuf::new(); + path.push(root); + path.push("pool"); + match self.kind { + ZpoolKind::External => path.push("ext"), + ZpoolKind::Internal => path.push("int"), + }; + path.push(self.id().to_string()); + path.push(dataset); + path + } +} + +impl<'de> Deserialize<'de> for ZpoolName { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + ZpoolName::from_str(&s).map_err(serde::de::Error::custom) + } +} + +impl Serialize for ZpoolName { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.to_string()) + } +} + +impl FromStr for ZpoolName { + type Err = String; + + fn from_str(s: &str) -> Result { + if let Some(s) = s.strip_prefix(ZPOOL_EXTERNAL_PREFIX) { + let id = ZpoolUuid::from_str(s).map_err(|e| e.to_string())?; + Ok(ZpoolName::new_external(id)) + } else if let Some(s) = s.strip_prefix(ZPOOL_INTERNAL_PREFIX) { + let id = ZpoolUuid::from_str(s).map_err(|e| e.to_string())?; + Ok(ZpoolName::new_internal(id)) + } else { + Err(format!( + "Bad zpool name {s}; must start with '{ZPOOL_EXTERNAL_PREFIX}' or '{ZPOOL_INTERNAL_PREFIX}'", + )) + } + } +} + +impl fmt::Display for ZpoolName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let prefix = match self.kind { + ZpoolKind::External => ZPOOL_EXTERNAL_PREFIX, + ZpoolKind::Internal => ZPOOL_INTERNAL_PREFIX, + }; + write!(f, "{prefix}{}", self.id) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_zpool_name_regex() { + let valid = [ + "oxi_d462a7f7-b628-40fe-80ff-4e4189e2d62b", + "oxp_d462a7f7-b628-40fe-80ff-4e4189e2d62b", + ]; + + let invalid = [ + "", + // Whitespace + " oxp_d462a7f7-b628-40fe-80ff-4e4189e2d62b", + "oxp_d462a7f7-b628-40fe-80ff-4e4189e2d62b ", + // Case sensitivity + "oxp_D462A7F7-b628-40fe-80ff-4e4189e2d62b", + // Bad prefix + "ox_d462a7f7-b628-40fe-80ff-4e4189e2d62b", + "oxa_d462a7f7-b628-40fe-80ff-4e4189e2d62b", + "oxi-d462a7f7-b628-40fe-80ff-4e4189e2d62b", + "oxp-d462a7f7-b628-40fe-80ff-4e4189e2d62b", + // Missing Prefix + "d462a7f7-b628-40fe-80ff-4e4189e2d62b", + // Bad UUIDs (Not following UUIDv4 format) + "oxi_d462a7f7-b628-30fe-80ff-4e4189e2d62b", + "oxi_d462a7f7-b628-40fe-c0ff-4e4189e2d62b", + ]; + + let r = regress::Regex::new(ZPOOL_NAME_REGEX) + .expect("validation regex is valid"); + for input in valid { + let m = r + .find(input) + .unwrap_or_else(|| panic!("input {input} did not match regex")); + assert_eq!(m.start(), 0, "input {input} did not match start"); + assert_eq!(m.end(), input.len(), "input {input} did not match end"); + } + + for input in invalid { + assert!( + r.find(input).is_none(), + "invalid input {input} should not match validation regex" + ); + } + } + + #[test] + fn test_parse_zpool_name_json() { + #[derive(Serialize, Deserialize, JsonSchema)] + struct TestDataset { + pool_name: ZpoolName, + } + + // Confirm that we can convert from a JSON string to a a ZpoolName + let json_string = + r#"{"pool_name":"oxi_d462a7f7-b628-40fe-80ff-4e4189e2d62b"}"#; + let dataset: TestDataset = serde_json::from_str(json_string) + .expect("Could not parse ZpoolName from Json Object"); + assert!(matches!(dataset.pool_name.kind, ZpoolKind::Internal)); + + // Confirm we can go the other way (ZpoolName to JSON string) too. + let j = serde_json::to_string(&dataset) + .expect("Cannot convert back to JSON string"); + assert_eq!(j, json_string); + } + + fn toml_string(s: &str) -> String { + format!("zpool_name = \"{}\"", s) + } + + fn parse_name(s: &str) -> Result { + toml_string(s) + .parse::() + .expect("Cannot parse as TOML value") + .get("zpool_name") + .expect("Missing key") + .clone() + .try_into::() + } + + #[test] + fn test_parse_external_zpool_name() { + let uuid: ZpoolUuid = + "d462a7f7-b628-40fe-80ff-4e4189e2d62b".parse().unwrap(); + let good_name = format!("{}{}", ZPOOL_EXTERNAL_PREFIX, uuid); + + let name = parse_name(&good_name).expect("Cannot parse as ZpoolName"); + assert_eq!(uuid, name.id()); + assert_eq!(ZpoolKind::External, name.kind()); + } + + #[test] + fn test_parse_internal_zpool_name() { + let uuid: ZpoolUuid = + "d462a7f7-b628-40fe-80ff-4e4189e2d62b".parse().unwrap(); + let good_name = format!("{}{}", ZPOOL_INTERNAL_PREFIX, uuid); + + let name = parse_name(&good_name).expect("Cannot parse as ZpoolName"); + assert_eq!(uuid, name.id()); + assert_eq!(ZpoolKind::Internal, name.kind()); + } + + #[test] + fn test_parse_bad_zpool_names() { + let bad_names = vec![ + // Nonsense string + "this string is GARBAGE", + // Missing prefix + "d462a7f7-b628-40fe-80ff-4e4189e2d62b", + // Underscores + "oxp_d462a7f7_b628_40fe_80ff_4e4189e2d62b", + ]; + + for bad_name in &bad_names { + assert!( + parse_name(&bad_name).is_err(), + "Parsing {} should fail", + bad_name + ); + } + } +} diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 3dbf018ecc..139e6fe607 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -622,7 +622,8 @@ pub fn get_all_omicron_datasets_for_delete() -> anyhow::Result> { // This includes cockroachdb, clickhouse, and crucible datasets. let zpools = crate::zpool::Zpool::list()?; for pool in &zpools { - let internal = pool.kind() == crate::zpool::ZpoolKind::Internal; + let internal = + pool.kind() == omicron_common::zpool_name::ZpoolKind::Internal; let pool = pool.to_string(); for dataset in &Zfs::list_datasets(&pool)? { // Avoid erasing crashdump, backing data and swap datasets on diff --git a/illumos-utils/src/zpool.rs b/illumos-utils/src/zpool.rs index f2b4df6996..fa93760f99 100644 --- a/illumos-utils/src/zpool.rs +++ b/illumos-utils/src/zpool.rs @@ -5,15 +5,11 @@ //! Utilities for managing Zpools. use crate::{execute, ExecutionError, PFEXEC}; -use camino::{Utf8Path, Utf8PathBuf}; -use omicron_uuid_kinds::ZpoolUuid; -use schemars::JsonSchema; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use std::fmt; +use camino::Utf8Path; use std::str::FromStr; -pub const ZPOOL_EXTERNAL_PREFIX: &str = "oxp_"; -pub const ZPOOL_INTERNAL_PREFIX: &str = "oxi_"; +pub use omicron_common::zpool_name::ZpoolName; + const ZPOOL: &str = "/usr/sbin/zpool"; pub const ZPOOL_MOUNTPOINT_ROOT: &str = "/"; @@ -304,267 +300,10 @@ impl Zpool { } } -#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, JsonSchema)] -#[serde(rename_all = "snake_case")] -pub enum ZpoolKind { - // This zpool is used for external storage (u.2) - External, - // This zpool is used for internal storage (m.2) - Internal, -} - -/// A wrapper around a zpool name. -/// -/// This expects that the format will be: `ox{i,p}_` - we parse the prefix -/// when reading the structure, and validate that the UUID can be utilized. -#[derive(Clone, Debug, Hash, PartialEq, Eq)] -pub struct ZpoolName { - id: ZpoolUuid, - kind: ZpoolKind, -} - -const ZPOOL_NAME_REGEX: &str = r"^ox[ip]_[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$"; - -/// Custom JsonSchema implementation to encode the constraints on Name. -impl JsonSchema for ZpoolName { - fn schema_name() -> String { - "ZpoolName".to_string() - } - fn json_schema( - _: &mut schemars::gen::SchemaGenerator, - ) -> schemars::schema::Schema { - schemars::schema::SchemaObject { - metadata: Some(Box::new(schemars::schema::Metadata { - title: Some( - "The name of a Zpool".to_string(), - ), - description: Some( - "Zpool names are of the format ox{i,p}_. They are either \ - Internal or External, and should be unique" - .to_string(), - ), - ..Default::default() - })), - instance_type: Some(schemars::schema::InstanceType::String.into()), - string: Some(Box::new(schemars::schema::StringValidation { - pattern: Some(ZPOOL_NAME_REGEX.to_owned()), - ..Default::default() - })), - ..Default::default() - } - .into() - } -} - -impl ZpoolName { - pub fn new_internal(id: ZpoolUuid) -> Self { - Self { id, kind: ZpoolKind::Internal } - } - - pub fn new_external(id: ZpoolUuid) -> Self { - Self { id, kind: ZpoolKind::External } - } - - pub fn id(&self) -> ZpoolUuid { - self.id - } - - pub fn kind(&self) -> ZpoolKind { - self.kind - } - - /// Returns a path to a dataset's mountpoint within the zpool. - /// - /// For example: oxp_(UUID) -> /pool/ext/(UUID)/(dataset) - pub fn dataset_mountpoint( - &self, - root: &Utf8Path, - dataset: &str, - ) -> Utf8PathBuf { - let mut path = Utf8PathBuf::new(); - path.push(root); - path.push("pool"); - match self.kind { - ZpoolKind::External => path.push("ext"), - ZpoolKind::Internal => path.push("int"), - }; - path.push(self.id().to_string()); - path.push(dataset); - path - } -} - -impl<'de> Deserialize<'de> for ZpoolName { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let s = String::deserialize(deserializer)?; - ZpoolName::from_str(&s).map_err(serde::de::Error::custom) - } -} - -impl Serialize for ZpoolName { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - serializer.serialize_str(&self.to_string()) - } -} - -impl FromStr for ZpoolName { - type Err = String; - - fn from_str(s: &str) -> Result { - if let Some(s) = s.strip_prefix(ZPOOL_EXTERNAL_PREFIX) { - let id = ZpoolUuid::from_str(s).map_err(|e| e.to_string())?; - Ok(ZpoolName::new_external(id)) - } else if let Some(s) = s.strip_prefix(ZPOOL_INTERNAL_PREFIX) { - let id = ZpoolUuid::from_str(s).map_err(|e| e.to_string())?; - Ok(ZpoolName::new_internal(id)) - } else { - Err(format!( - "Bad zpool name {s}; must start with '{ZPOOL_EXTERNAL_PREFIX}' or '{ZPOOL_INTERNAL_PREFIX}'", - )) - } - } -} - -impl fmt::Display for ZpoolName { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let prefix = match self.kind { - ZpoolKind::External => ZPOOL_EXTERNAL_PREFIX, - ZpoolKind::Internal => ZPOOL_INTERNAL_PREFIX, - }; - write!(f, "{prefix}{}", self.id) - } -} - #[cfg(test)] mod test { use super::*; - #[test] - fn test_zpool_name_regex() { - let valid = [ - "oxi_d462a7f7-b628-40fe-80ff-4e4189e2d62b", - "oxp_d462a7f7-b628-40fe-80ff-4e4189e2d62b", - ]; - - let invalid = [ - "", - // Whitespace - " oxp_d462a7f7-b628-40fe-80ff-4e4189e2d62b", - "oxp_d462a7f7-b628-40fe-80ff-4e4189e2d62b ", - // Case sensitivity - "oxp_D462A7F7-b628-40fe-80ff-4e4189e2d62b", - // Bad prefix - "ox_d462a7f7-b628-40fe-80ff-4e4189e2d62b", - "oxa_d462a7f7-b628-40fe-80ff-4e4189e2d62b", - "oxi-d462a7f7-b628-40fe-80ff-4e4189e2d62b", - "oxp-d462a7f7-b628-40fe-80ff-4e4189e2d62b", - // Missing Prefix - "d462a7f7-b628-40fe-80ff-4e4189e2d62b", - // Bad UUIDs (Not following UUIDv4 format) - "oxi_d462a7f7-b628-30fe-80ff-4e4189e2d62b", - "oxi_d462a7f7-b628-40fe-c0ff-4e4189e2d62b", - ]; - - let r = regress::Regex::new(ZPOOL_NAME_REGEX) - .expect("validation regex is valid"); - for input in valid { - let m = r - .find(input) - .unwrap_or_else(|| panic!("input {input} did not match regex")); - assert_eq!(m.start(), 0, "input {input} did not match start"); - assert_eq!(m.end(), input.len(), "input {input} did not match end"); - } - - for input in invalid { - assert!( - r.find(input).is_none(), - "invalid input {input} should not match validation regex" - ); - } - } - - #[test] - fn test_parse_zpool_name_json() { - #[derive(Serialize, Deserialize, JsonSchema)] - struct TestDataset { - pool_name: ZpoolName, - } - - // Confirm that we can convert from a JSON string to a a ZpoolName - let json_string = - r#"{"pool_name":"oxi_d462a7f7-b628-40fe-80ff-4e4189e2d62b"}"#; - let dataset: TestDataset = serde_json::from_str(json_string) - .expect("Could not parse ZpoolName from Json Object"); - assert!(matches!(dataset.pool_name.kind, ZpoolKind::Internal)); - - // Confirm we can go the other way (ZpoolName to JSON string) too. - let j = serde_json::to_string(&dataset) - .expect("Cannot convert back to JSON string"); - assert_eq!(j, json_string); - } - - fn toml_string(s: &str) -> String { - format!("zpool_name = \"{}\"", s) - } - - fn parse_name(s: &str) -> Result { - toml_string(s) - .parse::() - .expect("Cannot parse as TOML value") - .get("zpool_name") - .expect("Missing key") - .clone() - .try_into::() - } - - #[test] - fn test_parse_external_zpool_name() { - let uuid: ZpoolUuid = - "d462a7f7-b628-40fe-80ff-4e4189e2d62b".parse().unwrap(); - let good_name = format!("{}{}", ZPOOL_EXTERNAL_PREFIX, uuid); - - let name = parse_name(&good_name).expect("Cannot parse as ZpoolName"); - assert_eq!(uuid, name.id()); - assert_eq!(ZpoolKind::External, name.kind()); - } - - #[test] - fn test_parse_internal_zpool_name() { - let uuid: ZpoolUuid = - "d462a7f7-b628-40fe-80ff-4e4189e2d62b".parse().unwrap(); - let good_name = format!("{}{}", ZPOOL_INTERNAL_PREFIX, uuid); - - let name = parse_name(&good_name).expect("Cannot parse as ZpoolName"); - assert_eq!(uuid, name.id()); - assert_eq!(ZpoolKind::Internal, name.kind()); - } - - #[test] - fn test_parse_bad_zpool_names() { - let bad_names = vec![ - // Nonsense string - "this string is GARBAGE", - // Missing prefix - "d462a7f7-b628-40fe-80ff-4e4189e2d62b", - // Underscores - "oxp_d462a7f7_b628_40fe_80ff_4e4189e2d62b", - ]; - - for bad_name in &bad_names { - assert!( - parse_name(&bad_name).is_err(), - "Parsing {} should fail", - bad_name - ); - } - } - #[test] fn test_parse_zpool() { let name = "rpool"; diff --git a/nexus/db-model/src/omicron_zone_config.rs b/nexus/db-model/src/omicron_zone_config.rs index f6d272a1cd..9838deed63 100644 --- a/nexus/db-model/src/omicron_zone_config.rs +++ b/nexus/db-model/src/omicron_zone_config.rs @@ -175,8 +175,7 @@ impl OmicronZone { } }; - let dataset_zpool_name = - dataset.map(|d| d.pool_name.as_str().to_string()); + let dataset_zpool_name = dataset.map(|d| d.pool_name.to_string()); let primary_service_sockaddr = primary_service_sockaddr_str .parse::() .with_context(|| { diff --git a/nexus/reconfigurator/execution/Cargo.toml b/nexus/reconfigurator/execution/Cargo.toml index 7e57107c43..3ab2d91586 100644 --- a/nexus/reconfigurator/execution/Cargo.toml +++ b/nexus/reconfigurator/execution/Cargo.toml @@ -11,7 +11,6 @@ anyhow.workspace = true dns-service-client.workspace = true chrono.workspace = true futures.workspace = true -illumos-utils.workspace = true internal-dns.workspace = true nexus-config.workspace = true nexus-db-model.workspace = true diff --git a/nexus/reconfigurator/execution/src/datasets.rs b/nexus/reconfigurator/execution/src/datasets.rs index 5f1f738f3b..f888593ac4 100644 --- a/nexus/reconfigurator/execution/src/datasets.rs +++ b/nexus/reconfigurator/execution/src/datasets.rs @@ -5,7 +5,6 @@ //! Ensures dataset records required by a given blueprint use anyhow::Context; -use illumos_utils::zpool::ZpoolName; use nexus_db_model::Dataset; use nexus_db_model::DatasetKind; use nexus_db_queries::context::OpContext; @@ -70,19 +69,7 @@ pub(crate) async fn ensure_crucible_dataset_records_exist( // Map progenitor client strings into the types we need. We never // expect these to fail. - let zpool_name: ZpoolName = match dataset.pool_name.parse() { - Ok(name) => name, - Err(err) => { - warn!( - opctx.log, "failed to parse crucible zone pool name"; - "pool_name" => &*dataset.pool_name, - "err" => err, - ); - continue; - } - }; - - let pool_id = zpool_name.id(); + let pool_id = dataset.pool_name.id(); let dataset = Dataset::new( id.into_untyped_uuid(), pool_id.into_untyped_uuid(), @@ -145,6 +132,7 @@ mod tests { use nexus_test_utils_macros::nexus_test; use nexus_types::deployment::BlueprintZoneDisposition; use nexus_types::deployment::BlueprintZoneFilter; + use omicron_common::zpool_name::ZpoolName; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::ZpoolUuid; use sled_agent_client::types::OmicronZoneDataset; @@ -199,10 +187,8 @@ mod tests { else { continue; }; - let zpool_name: ZpoolName = - dataset.pool_name.parse().expect("invalid zpool name"); let zpool = Zpool::new( - zpool_name.id().into_untyped_uuid(), + dataset.pool_name.id().into_untyped_uuid(), sled_id.into_untyped_uuid(), Uuid::new_v4(), // physical_disk_id ); @@ -298,10 +284,7 @@ mod tests { blueprint_zone_type::Crucible { address: "[::1]:0".parse().unwrap(), dataset: OmicronZoneDataset { - pool_name: ZpoolName::new_external(new_zpool_id) - .to_string() - .parse() - .unwrap(), + pool_name: ZpoolName::new_external(new_zpool_id), }, }, ), diff --git a/nexus/reconfigurator/planning/Cargo.toml b/nexus/reconfigurator/planning/Cargo.toml index 9c1d462a3b..d31832482d 100644 --- a/nexus/reconfigurator/planning/Cargo.toml +++ b/nexus/reconfigurator/planning/Cargo.toml @@ -8,7 +8,6 @@ anyhow.workspace = true chrono.workspace = true debug-ignore.workspace = true gateway-client.workspace = true -illumos-utils.workspace = true indexmap.workspace = true internal-dns.workspace = true ipnet.workspace = true diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index a58b96162b..8335815b4a 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -65,7 +65,6 @@ use std::net::IpAddr; use std::net::Ipv4Addr; use std::net::Ipv6Addr; use std::net::SocketAddrV6; -use std::str::FromStr; use thiserror::Error; use typed_rng::TypedUuidRng; use typed_rng::UuidRng; @@ -112,13 +111,8 @@ pub enum EnsureMultiple { NotNeeded, } -fn zpool_id_to_external_name(zpool_id: ZpoolUuid) -> anyhow::Result { - let pool_name_generated = - illumos_utils::zpool::ZpoolName::new_external(zpool_id).to_string(); - let pool_name = ZpoolName::from_str(&pool_name_generated).map_err(|e| { - anyhow!("Failed to create zpool name from {zpool_id}: {e}") - })?; - Ok(pool_name) +fn zpool_id_to_external_name(zpool_id: ZpoolUuid) -> ZpoolName { + ZpoolName::new_external(zpool_id) } /// Helper for assembling a blueprint @@ -632,7 +626,7 @@ impl<'a> BlueprintBuilder<'a> { sled_id: SledUuid, zpool_id: ZpoolUuid, ) -> Result { - let pool_name = zpool_id_to_external_name(zpool_id)?; + let pool_name = zpool_id_to_external_name(zpool_id); // If this sled already has a Crucible zone on this pool, do nothing. let has_crucible_on_this_pool = @@ -1354,7 +1348,7 @@ pub mod test { new_sled_resources .zpools .keys() - .map(|id| { zpool_id_to_external_name(*id).unwrap() }) + .map(|id| { zpool_id_to_external_name(*id) }) .collect() ); diff --git a/nexus/reconfigurator/preparation/Cargo.toml b/nexus/reconfigurator/preparation/Cargo.toml index ab4dbb396e..0b90b5d702 100644 --- a/nexus/reconfigurator/preparation/Cargo.toml +++ b/nexus/reconfigurator/preparation/Cargo.toml @@ -6,7 +6,6 @@ edition = "2021" [dependencies] anyhow.workspace = true futures.workspace = true -illumos-utils.workspace = true nexus-db-model.workspace = true nexus-db-queries.workspace = true nexus-types.workspace = true diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index 1b1b42a8f5..6acbcaca6a 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -21,6 +21,7 @@ use omicron_common::api::external::ByteCount; pub use omicron_common::api::internal::shared::NetworkInterface; pub use omicron_common::api::internal::shared::NetworkInterfaceKind; pub use omicron_common::api::internal::shared::SourceNatConfig; +pub use omicron_common::zpool_name::ZpoolName; use omicron_uuid_kinds::CollectionUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; @@ -31,7 +32,6 @@ pub use sled_agent_client::types::OmicronZoneDataset; pub use sled_agent_client::types::OmicronZoneType; pub use sled_agent_client::types::OmicronZonesConfig; pub use sled_agent_client::types::SledRole; -pub use sled_agent_client::types::ZpoolName; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::net::SocketAddrV6; diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 593a841bfc..35d89c1fd4 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -7267,8 +7267,10 @@ "minimum": 0 }, "ZpoolName": { - "description": "Zpool names are of the format ox{i,p}_. They are either Internal or External, and should be unique\n\n
JSON schema\n\n```json { \"title\": \"The name of a Zpool\", \"description\": \"Zpool names are of the format ox{i,p}_. They are either Internal or External, and should be unique\", \"type\": \"string\", \"pattern\": \"^ox[ip]_[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$\" } ```
", - "type": "string" + "title": "The name of a Zpool", + "description": "Zpool names are of the format ox{i,p}_. They are either Internal or External, and should be unique", + "type": "string", + "pattern": "^ox[ip]_[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" }, "ZpoolPutRequest": { "description": "Identifies information about a Zpool that should be part of the control plane.", diff --git a/sled-agent/src/params.rs b/sled-agent/src/params.rs index 1393934031..a51443af70 100644 --- a/sled-agent/src/params.rs +++ b/sled-agent/src/params.rs @@ -389,7 +389,7 @@ pub struct OmicronZoneDataset { impl From for sled_agent_client::types::OmicronZoneDataset { fn from(local: OmicronZoneDataset) -> Self { Self { - pool_name: sled_agent_client::types::ZpoolName::from_str( + pool_name: omicron_common::zpool_name::ZpoolName::from_str( &local.pool_name.to_string(), ) .unwrap(), diff --git a/sled-hardware/src/disk.rs b/sled-hardware/src/disk.rs index 471f9925ca..d48dd88c3d 100644 --- a/sled-hardware/src/disk.rs +++ b/sled-hardware/src/disk.rs @@ -5,9 +5,8 @@ use camino::{Utf8Path, Utf8PathBuf}; use illumos_utils::fstyp::Fstyp; use illumos_utils::zpool::Zpool; -use illumos_utils::zpool::ZpoolKind; -use illumos_utils::zpool::ZpoolName; use omicron_common::disk::DiskIdentity; +use omicron_common::zpool_name::{ZpoolKind, ZpoolName}; use omicron_uuid_kinds::ZpoolUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index cf34c689bf..c9e848559e 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -7,11 +7,11 @@ use anyhow::bail; use camino::{Utf8Path, Utf8PathBuf}; use derive_more::From; -use illumos_utils::zpool::{ZpoolKind, ZpoolName}; use key_manager::StorageKeyRequester; use omicron_common::api::external::Generation; use omicron_common::disk::DiskIdentity; use omicron_common::ledger::Ledgerable; +use omicron_common::zpool_name::{ZpoolKind, ZpoolName}; use omicron_uuid_kinds::ZpoolUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; diff --git a/sled-storage/src/manager_test_harness.rs b/sled-storage/src/manager_test_harness.rs index 19501dd4e4..a2180a95b5 100644 --- a/sled-storage/src/manager_test_harness.rs +++ b/sled-storage/src/manager_test_harness.rs @@ -88,11 +88,11 @@ impl Drop for StorageManagerTestHarness { let pools = [ ( - illumos_utils::zpool::ZPOOL_INTERNAL_PREFIX, + omicron_common::zpool_name::ZPOOL_INTERNAL_PREFIX, vdev_dir.path().join("pool/int"), ), ( - illumos_utils::zpool::ZPOOL_EXTERNAL_PREFIX, + omicron_common::zpool_name::ZPOOL_EXTERNAL_PREFIX, vdev_dir.path().join("pool/ext"), ), ]; From 138a116874d58cfa6a966e3ed7ad93b1978511ac Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Apr 2024 17:49:32 -0700 Subject: [PATCH 02/12] [reconfigurator] Only place Crucible zones on provisionable zpools --- nexus/reconfigurator/planning/src/planner.rs | 4 +-- nexus/types/src/deployment/planning_input.rs | 28 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index a252f9b821..7fb7ee784a 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -196,9 +196,9 @@ impl<'a> Planner<'a> { continue; } - // Every zpool on the sled should have a Crucible zone on it. + // Every provisionable zpool on the sled should have a Crucible zone on it. let mut ncrucibles_added = 0; - for zpool_id in sled_resources.zpools.keys() { + for zpool_id in sled_resources.provisionable_zpools() { if self .blueprint .sled_ensure_zone_crucible(sled_id, *zpool_id)? diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 2503ff81f3..c66e1777ad 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -42,6 +42,15 @@ pub struct SledDisk { pub state: PhysicalDiskState, } +impl SledDisk { + fn provisionable(&self) -> bool { + match (self.policy, self.state) { + (PhysicalDiskPolicy::InService, PhysicalDiskState::Active) => true, + _ => false, + } + } +} + /// Filters that apply to disks. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum DiskFilter { @@ -87,6 +96,25 @@ pub struct SledResources { } impl SledResources { + /// Returns if the zpool is provisionable (known, in-service, and active). + pub fn zpool_is_provisionable(&self, zpool: &ZpoolUuid) -> bool { + let Some(disk) = self.zpools.get(zpool) else { return false }; + disk.provisionable() + } + + /// Returns all in-service, active zpools + pub fn provisionable_zpools( + &self, + ) -> impl Iterator + '_ { + self.zpools.iter().filter_map(|(zpool, disk)| { + if disk.provisionable() { + Some(zpool) + } else { + None + } + }) + } + pub fn all_disks( &self, filter: DiskFilter, From 5229effef30ba61a15f84cb637069242ce470538 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Apr 2024 16:47:40 -0700 Subject: [PATCH 03/12] [nexus] Expunge zones on expunged disks, only provision crucible to in-service disks --- .../planning/src/blueprint_builder/builder.rs | 25 ++++++++++++ nexus/reconfigurator/planning/src/planner.rs | 38 +++++++++++++++---- nexus/types/src/deployment/zone_type.rs | 30 +++++++++++++++ 3 files changed, 85 insertions(+), 8 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 8335815b4a..fa3d3d656b 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -404,6 +404,19 @@ impl<'a> BlueprintBuilder<'a> { &mut self, sled_id: SledUuid, reason: ZoneExpungeReason, + ) -> Result, Error> { + self.expunge_all_zones_for_sled_where(sled_id, reason, |_| true) + } + + /// Expunges all zones from a sled where `filter_fn` returns `true`. + /// + /// Returns a list of zone IDs expunged (excluding zones that were already + /// expunged). If the list is empty, then the operation was a no-op. + pub(crate) fn expunge_all_zones_for_sled_where( + &mut self, + sled_id: SledUuid, + reason: ZoneExpungeReason, + filter_fn: impl Fn(&BlueprintZoneConfig) -> bool, ) -> Result, Error> { let log = self.log.new(o!( "sled_id" => sled_id.to_string(), @@ -414,6 +427,11 @@ impl<'a> BlueprintBuilder<'a> { let sled_zones = self.zones.current_sled_zones(sled_id); for (z, state) in sled_zones { + // Only consider zones for which the filter returns true. + if !filter_fn(z) { + continue; + } + let is_expunged = is_already_expunged(z, state).map_err(|error| { Error::Planner(anyhow!(error).context(format!( @@ -435,6 +453,12 @@ impl<'a> BlueprintBuilder<'a> { } match reason { + ZoneExpungeReason::DiskExpunged => { + info!( + &log, + "expunged disk with non-expunged zone(s) was found" + ); + } ZoneExpungeReason::SledDecommissioned { policy } => { // A sled marked as decommissioned should have no resources // allocated to it. If it does, it's an illegal state, possibly @@ -468,6 +492,7 @@ impl<'a> BlueprintBuilder<'a> { // Finally, add a comment describing what happened. let reason = match reason { + ZoneExpungeReason::DiskExpunged => "zone was using expunged disk", ZoneExpungeReason::SledDecommissioned { .. } => { "sled state is decommissioned" } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 7fb7ee784a..0352ad33bf 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -85,18 +85,39 @@ impl<'a> Planner<'a> { fn do_plan_expunge(&mut self) -> Result<(), Error> { // Remove services from sleds marked expunged. We use `SledFilter::All` - // and have a custom `needs_zone_expungement` function that allows us + // and have a custom `needs_all_zones_expungement` function that allows us // to produce better errors. for (sled_id, sled_details) in self.input.all_sleds(SledFilter::All) { // Does this sled need zone expungement based on the details? - let Some(reason) = - needs_zone_expungement(sled_details.state, sled_details.policy) - else { + if let Some(reason) = needs_all_zones_expungement( + sled_details.state, + sled_details.policy, + ) { + // Perform the expungement. + self.blueprint.expunge_all_zones_for_sled(sled_id, reason)?; + + // In this case, all zones have been expunged. We can jump to + // the next sled. continue; - }; + } - // Perform the expungement. - self.blueprint.expunge_all_zones_for_sled(sled_id, reason)?; + // The sled isn't being decommissioned, but there still might be + // some zones that need expungement. + // + // Remove any zones which rely on expunged disks. + self.blueprint.expunge_all_zones_for_sled_where( + sled_id, + ZoneExpungeReason::DiskExpunged, + |zone_config| { + let durable_storage_zpool = + match zone_config.zone_type.zpool() { + Some(pool) => pool, + None => return false, + }; + let zpool_id = durable_storage_zpool.id(); + !sled_details.resources.zpool_is_provisionable(&zpool_id) + }, + )?; } Ok(()) @@ -360,7 +381,7 @@ impl<'a> Planner<'a> { /// Returns `Some(reason)` if the sled needs its zones to be expunged, /// based on the policy and state. -fn needs_zone_expungement( +fn needs_all_zones_expungement( state: SledState, policy: SledPolicy, ) -> Option { @@ -387,6 +408,7 @@ fn needs_zone_expungement( /// logical flow. #[derive(Copy, Clone, Debug)] pub(crate) enum ZoneExpungeReason { + DiskExpunged, SledDecommissioned { policy: SledPolicy }, SledExpunged, } diff --git a/nexus/types/src/deployment/zone_type.rs b/nexus/types/src/deployment/zone_type.rs index a258ab53a1..76989b7738 100644 --- a/nexus/types/src/deployment/zone_type.rs +++ b/nexus/types/src/deployment/zone_type.rs @@ -33,6 +33,36 @@ 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 external_ip(&self) -> Option { match self { BlueprintZoneType::Nexus(nexus) => Some(nexus.external_ip), From cf201dc79ef2c7a0d0309d92ff9957a00f8e28d1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Apr 2024 19:03:34 -0700 Subject: [PATCH 04/12] Add a test for crucible provisioning --- nexus/reconfigurator/planning/src/planner.rs | 80 ++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 7fb7ee784a..a83c2974bc 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -414,8 +414,11 @@ mod test { use nexus_types::external_api::views::SledState; use nexus_types::inventory::OmicronZonesFound; use omicron_common::api::external::Generation; + use omicron_common::disk::DiskIdentity; use omicron_test_utils::dev::test_setup_log; use omicron_uuid_kinds::GenericUuid; + use omicron_uuid_kinds::PhysicalDiskUuid; + use omicron_uuid_kinds::ZpoolUuid; use std::collections::HashMap; /// Runs through a basic sequence of blueprints for adding a sled @@ -760,6 +763,83 @@ mod test { logctx.cleanup_successful(); } + #[test] + fn test_crucible_allocation_skips_nonprovisionable_disks() { + static TEST_NAME: &str = + "planner_crucible_allocation_skips_nonprovisionable_disks"; + let logctx = test_setup_log(TEST_NAME); + + // Create an example system with a single sled + let (collection, input, blueprint1) = + example(&logctx.log, TEST_NAME, 1); + + let mut builder = input.into_builder(); + + // Avoid churning on the quantity of Nexus zones - we're okay staying at + // one. + builder.policy_mut().target_nexus_zone_count = 1; + + let new_sled_disk = |policy| nexus_types::deployment::SledDisk { + disk_identity: DiskIdentity { + vendor: "test-vendor".to_string(), + serial: "test-serial".to_string(), + model: "test-model".to_string(), + }, + disk_id: PhysicalDiskUuid::new_v4(), + policy, + state: nexus_types::external_api::views::PhysicalDiskState::Active, + }; + + let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); + + // Inject some new disks into the input. + // + // These counts are arbitrary, as long as they're non-zero + // for the sake of the test. + + const NEW_IN_SERVICE_DISKS: usize = 2; + const NEW_EXPUNGED_DISKS: usize = 1; + + for _ in 0..NEW_IN_SERVICE_DISKS { + sled_details.resources.zpools.insert( + ZpoolUuid::new_v4(), + new_sled_disk(nexus_types::external_api::views::PhysicalDiskPolicy::InService), + ); + } + for _ in 0..NEW_EXPUNGED_DISKS { + sled_details.resources.zpools.insert( + ZpoolUuid::new_v4(), + new_sled_disk(nexus_types::external_api::views::PhysicalDiskPolicy::Expunged), + ); + } + + let input = builder.build(); + + let blueprint2 = Planner::new_based_on( + logctx.log.clone(), + &blueprint1, + &input, + "test: some new disks", + &collection, + ) + .expect("failed to create planner") + .with_rng_seed((TEST_NAME, "bp2")) + .plan() + .expect("failed to plan"); + + let diff = blueprint2.diff_since_blueprint(&blueprint1).unwrap(); + println!("1 -> 2 (some new disks, one expunged):\n{}", diff.display()); + let mut modified_sleds = diff.sleds_modified(); + assert_eq!(modified_sleds.len(), 1); + let (_, diff_modified) = modified_sleds.next().unwrap(); + + // We should be adding a Crucible zone for each new in-service disk. + assert_eq!(diff_modified.zones_added().count(), NEW_IN_SERVICE_DISKS); + assert_eq!(diff_modified.zones_removed().len(), 0); + + logctx.cleanup_successful(); + } + /// Check that the planner will skip non-provisionable sleds when allocating /// extra Nexus zones #[test] From 5ba3a5ca0b58a9f354ca5757e248b56c15adbdcc Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Apr 2024 19:11:45 -0700 Subject: [PATCH 05/12] Add test --- nexus/reconfigurator/planning/src/planner.rs | 49 ++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index d9ad429604..9baa964b24 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -862,6 +862,55 @@ mod test { logctx.cleanup_successful(); } + #[test] + fn test_disk_expungement_removes_zones() { + static TEST_NAME: &str = "planner_disk_expungement_removes_zones"; + let logctx = test_setup_log(TEST_NAME); + + // Create an example system with a single sled + let (collection, input, blueprint1) = + example(&logctx.log, TEST_NAME, 1); + + let mut builder = input.into_builder(); + + // Avoid churning on the quantity of Nexus zones - we're okay staying at + // one. + builder.policy_mut().target_nexus_zone_count = 1; + + let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); + let (_, disk) = sled_details.resources.zpools.iter_mut().next().unwrap(); + disk.policy = nexus_types::external_api::views::PhysicalDiskPolicy::Expunged; + + let input = builder.build(); + + let blueprint2 = Planner::new_based_on( + logctx.log.clone(), + &blueprint1, + &input, + "test: expunge a disk", + &collection, + ) + .expect("failed to create planner") + .with_rng_seed((TEST_NAME, "bp2")) + .plan() + .expect("failed to plan"); + + let diff = blueprint2.diff_since_blueprint(&blueprint1).unwrap(); + println!("1 -> 2 (expunge a disk):\n{}", diff.display()); + let mut modified_sleds = diff.sleds_modified(); + assert_eq!(modified_sleds.len(), 1); + let (_, diff_modified) = modified_sleds.next().unwrap(); + + // We should be removing a single zone, associated with the Crucible + // using that device. + assert_eq!(diff_modified.zones_added().len(), 0); + assert_eq!(diff_modified.zones_removed().len(), 0); + assert_eq!(diff_modified.zones_modified().count(), 1); + assert!(diff_modified.zones_modified().next().unwrap().disposition_changed()); + + logctx.cleanup_successful(); + } + /// Check that the planner will skip non-provisionable sleds when allocating /// extra Nexus zones #[test] From 309ab31e2417f0db5aecf598a3ce2781c1de2bad Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Apr 2024 19:12:31 -0700 Subject: [PATCH 06/12] fmt --- nexus/reconfigurator/planning/src/planner.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 9baa964b24..a6b1a2fe5b 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -878,8 +878,10 @@ mod test { builder.policy_mut().target_nexus_zone_count = 1; let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); - let (_, disk) = sled_details.resources.zpools.iter_mut().next().unwrap(); - disk.policy = nexus_types::external_api::views::PhysicalDiskPolicy::Expunged; + let (_, disk) = + sled_details.resources.zpools.iter_mut().next().unwrap(); + disk.policy = + nexus_types::external_api::views::PhysicalDiskPolicy::Expunged; let input = builder.build(); @@ -906,7 +908,11 @@ mod test { assert_eq!(diff_modified.zones_added().len(), 0); assert_eq!(diff_modified.zones_removed().len(), 0); assert_eq!(diff_modified.zones_modified().count(), 1); - assert!(diff_modified.zones_modified().next().unwrap().disposition_changed()); + assert!(diff_modified + .zones_modified() + .next() + .unwrap() + .disposition_changed()); logctx.cleanup_successful(); } From 75c689bc053da7a56a8588927b5fce11d6f14c73 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Apr 2024 09:24:20 -0700 Subject: [PATCH 07/12] review feedback --- nexus/reconfigurator/execution/src/datasets.rs | 2 -- .../planning/src/blueprint_builder/builder.rs | 8 ++------ 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/nexus/reconfigurator/execution/src/datasets.rs b/nexus/reconfigurator/execution/src/datasets.rs index f888593ac4..6e4286f9db 100644 --- a/nexus/reconfigurator/execution/src/datasets.rs +++ b/nexus/reconfigurator/execution/src/datasets.rs @@ -67,8 +67,6 @@ pub(crate) async fn ensure_crucible_dataset_records_exist( continue; } - // Map progenitor client strings into the types we need. We never - // expect these to fail. let pool_id = dataset.pool_name.id(); let dataset = Dataset::new( id.into_untyped_uuid(), diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 8335815b4a..908a22f43b 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -111,10 +111,6 @@ pub enum EnsureMultiple { NotNeeded, } -fn zpool_id_to_external_name(zpool_id: ZpoolUuid) -> ZpoolName { - ZpoolName::new_external(zpool_id) -} - /// Helper for assembling a blueprint /// /// There are two basic ways to assemble a new blueprint: @@ -626,7 +622,7 @@ impl<'a> BlueprintBuilder<'a> { sled_id: SledUuid, zpool_id: ZpoolUuid, ) -> Result { - let pool_name = zpool_id_to_external_name(zpool_id); + let pool_name = ZpoolName::new_external(zpool_id); // If this sled already has a Crucible zone on this pool, do nothing. let has_crucible_on_this_pool = @@ -1348,7 +1344,7 @@ pub mod test { new_sled_resources .zpools .keys() - .map(|id| { zpool_id_to_external_name(*id) }) + .map(|id| { ZpoolName::new_external(*id) }) .collect() ); From 2e887c544da19e4a6b77aecbd7f3a16312eab13d Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Apr 2024 10:46:35 -0700 Subject: [PATCH 08/12] zpool filter --- nexus/reconfigurator/planning/src/planner.rs | 3 +- nexus/types/src/deployment.rs | 1 + nexus/types/src/deployment/planning_input.rs | 48 +++++++++++++++----- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index a83c2974bc..1238692b67 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -13,6 +13,7 @@ use crate::blueprint_builder::Error; use nexus_types::deployment::Blueprint; use nexus_types::deployment::PlanningInput; use nexus_types::deployment::SledFilter; +use nexus_types::deployment::ZpoolFilter; use nexus_types::external_api::views::SledPolicy; use nexus_types::external_api::views::SledState; use nexus_types::inventory::Collection; @@ -198,7 +199,7 @@ impl<'a> Planner<'a> { // Every provisionable zpool on the sled should have a Crucible zone on it. let mut ncrucibles_added = 0; - for zpool_id in sled_resources.provisionable_zpools() { + for zpool_id in sled_resources.all_zpools(ZpoolFilter::InService) { if self .blueprint .sled_ensure_zone_crucible(sled_id, *zpool_id)? diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 36f87a2eb3..5ded144c94 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -55,6 +55,7 @@ pub use planning_input::SledDetails; pub use planning_input::SledDisk; pub use planning_input::SledFilter; pub use planning_input::SledResources; +pub use planning_input::ZpoolFilter; pub use zone_type::blueprint_zone_type; pub use zone_type::BlueprintZoneType; diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index c66e1777ad..0a4c12b11c 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -44,10 +44,7 @@ pub struct SledDisk { impl SledDisk { fn provisionable(&self) -> bool { - match (self.policy, self.state) { - (PhysicalDiskPolicy::InService, PhysicalDiskState::Active) => true, - _ => false, - } + DiskFilter::InService.matches_policy_and_state(self.policy, self.state) } } @@ -79,6 +76,34 @@ impl DiskFilter { } } +/// Filters that apply to zpools. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub enum ZpoolFilter { + /// All zpools + All, + + /// All zpools which are in-service. + InService, +} + +impl ZpoolFilter { + fn matches_policy_and_state( + self, + policy: PhysicalDiskPolicy, + state: PhysicalDiskState, + ) -> bool { + match self { + ZpoolFilter::All => true, + ZpoolFilter::InService => match (policy, state) { + (PhysicalDiskPolicy::InService, PhysicalDiskState::Active) => { + true + } + _ => false, + }, + } + } +} + /// Describes the resources available on each sled for the planner #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SledResources { @@ -102,16 +127,15 @@ impl SledResources { disk.provisionable() } - /// Returns all in-service, active zpools - pub fn provisionable_zpools( + /// Returns all zpools matching the given filter. + pub fn all_zpools( &self, + filter: ZpoolFilter, ) -> impl Iterator + '_ { - self.zpools.iter().filter_map(|(zpool, disk)| { - if disk.provisionable() { - Some(zpool) - } else { - None - } + self.zpools.iter().filter_map(move |(zpool, disk)| { + filter + .matches_policy_and_state(disk.policy, disk.state) + .then_some(zpool) }) } From 09f416202b93c263aee224fe3a0bee813fff8490 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Apr 2024 11:47:48 -0700 Subject: [PATCH 09/12] use imports --- nexus/reconfigurator/planning/src/planner.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 3a1d804bb6..6bb113f9a2 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -432,6 +432,9 @@ mod test { use nexus_types::deployment::BlueprintZoneFilter; use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::DiffSledModified; + use nexus_types::deployment::SledDisk; + use nexus_types::external_api::views::PhysicalDiskPolicy; + use nexus_types::external_api::views::PhysicalDiskState; use nexus_types::external_api::views::SledPolicy; use nexus_types::external_api::views::SledProvisionPolicy; use nexus_types::external_api::views::SledState; @@ -802,7 +805,7 @@ mod test { // one. builder.policy_mut().target_nexus_zone_count = 1; - let new_sled_disk = |policy| nexus_types::deployment::SledDisk { + let new_sled_disk = |policy| SledDisk { disk_identity: DiskIdentity { vendor: "test-vendor".to_string(), serial: "test-serial".to_string(), @@ -810,7 +813,7 @@ mod test { }, disk_id: PhysicalDiskUuid::new_v4(), policy, - state: nexus_types::external_api::views::PhysicalDiskState::Active, + state: PhysicalDiskState::Active, }; let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); @@ -826,13 +829,13 @@ mod test { for _ in 0..NEW_IN_SERVICE_DISKS { sled_details.resources.zpools.insert( ZpoolUuid::new_v4(), - new_sled_disk(nexus_types::external_api::views::PhysicalDiskPolicy::InService), + new_sled_disk(PhysicalDiskPolicy::InService), ); } for _ in 0..NEW_EXPUNGED_DISKS { sled_details.resources.zpools.insert( ZpoolUuid::new_v4(), - new_sled_disk(nexus_types::external_api::views::PhysicalDiskPolicy::Expunged), + new_sled_disk(PhysicalDiskPolicy::Expunged), ); } @@ -881,8 +884,7 @@ mod test { let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); let (_, disk) = sled_details.resources.zpools.iter_mut().next().unwrap(); - disk.policy = - nexus_types::external_api::views::PhysicalDiskPolicy::Expunged; + disk.policy = PhysicalDiskPolicy::Expunged; let input = builder.build(); From 9e1e18a3b11b2cd27f5ee5fd1e21a5f13c54792c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Apr 2024 13:28:57 -0700 Subject: [PATCH 10/12] Refactor to minimize arbitrary filter functions --- .../planning/src/blueprint_builder/builder.rs | 151 +++++++++--------- nexus/reconfigurator/planning/src/planner.rs | 65 ++++---- .../output/planner_nonprovisionable_bp2.txt | 10 +- 3 files changed, 112 insertions(+), 114 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index c4275b3255..f89d2a8696 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -5,6 +5,7 @@ //! Low-level facility for generating Blueprints use crate::ip_allocator::IpAllocator; +use crate::planner::zone_needs_expungement; use crate::planner::ZoneExpungeReason; use anyhow::anyhow; use anyhow::bail; @@ -26,6 +27,7 @@ use nexus_types::deployment::BlueprintZonesConfig; use nexus_types::deployment::DiskFilter; use nexus_types::deployment::OmicronZoneDataset; use nexus_types::deployment::PlanningInput; +use nexus_types::deployment::SledDetails; use nexus_types::deployment::SledFilter; use nexus_types::deployment::SledResources; use nexus_types::deployment::ZpoolName; @@ -58,7 +60,6 @@ use slog::o; use slog::Logger; use std::borrow::Cow; use std::collections::BTreeMap; -use std::collections::BTreeSet; use std::collections::HashSet; use std::hash::Hash; use std::net::IpAddr; @@ -392,51 +393,68 @@ impl<'a> BlueprintBuilder<'a> { self.comments.push(String::from(comment)); } - /// Expunges all zones from a sled. + /// Expunges all zones requiring expungement from a sled. /// /// Returns a list of zone IDs expunged (excluding zones that were already /// expunged). If the list is empty, then the operation was a no-op. - pub(crate) fn expunge_all_zones_for_sled( + pub(crate) fn expunge_zones_for_sled( &mut self, sled_id: SledUuid, - reason: ZoneExpungeReason, - ) -> Result, Error> { - self.expunge_all_zones_for_sled_where(sled_id, reason, |_| true) - } - - /// Expunges all zones from a sled where `filter_fn` returns `true`. - /// - /// Returns a list of zone IDs expunged (excluding zones that were already - /// expunged). If the list is empty, then the operation was a no-op. - pub(crate) fn expunge_all_zones_for_sled_where( - &mut self, - sled_id: SledUuid, - reason: ZoneExpungeReason, - filter_fn: impl Fn(&BlueprintZoneConfig) -> bool, - ) -> Result, Error> { + sled_details: &SledDetails, + ) -> Result, Error> { let log = self.log.new(o!( "sled_id" => sled_id.to_string(), )); // Do any zones need to be marked expunged? - let mut zones_to_expunge = BTreeSet::new(); + let mut zones_to_expunge = BTreeMap::new(); let sled_zones = self.zones.current_sled_zones(sled_id); - for (z, state) in sled_zones { - // Only consider zones for which the filter returns true. - if !filter_fn(z) { + for (zone_config, state) in sled_zones { + let zone_id = zone_config.id; + let Some(reason) = + zone_needs_expungement(sled_details, zone_config) + else { continue; - } + }; let is_expunged = - is_already_expunged(z, state).map_err(|error| { + is_already_expunged(zone_config, state).map_err(|error| { Error::Planner(anyhow!(error).context(format!( "for sled {sled_id}, error computing zones to expunge" ))) })?; if !is_expunged { - zones_to_expunge.insert(z.id); + match reason { + ZoneExpungeReason::DiskExpunged => { + info!( + &log, + "expunged disk with non-expunged zone(s) was found" + ); + } + ZoneExpungeReason::SledDecommissioned => { + // A sled marked as decommissioned should have no resources + // allocated to it. If it does, it's an illegal state, possibly + // introduced by a bug elsewhere in the system -- we need to + // produce a loud warning (i.e. an ERROR-level log message) on + // this, while still removing the zones. + error!( + &log, + "sled has state Decommissioned, yet has zone {zone_id} \ + allocated to it; will expunge it" + ); + } + ZoneExpungeReason::SledExpunged => { + // This is the expected situation. + info!( + &log, + "expunged sled with non-expunged zone {zone_id} found" + ); + } + } + + zones_to_expunge.insert(zone_id, reason); } } @@ -448,58 +466,43 @@ impl<'a> BlueprintBuilder<'a> { return Ok(zones_to_expunge); } - match reason { - ZoneExpungeReason::DiskExpunged => { - info!( - &log, - "expunged disk with non-expunged zone(s) was found" - ); - } - ZoneExpungeReason::SledDecommissioned { policy } => { - // A sled marked as decommissioned should have no resources - // allocated to it. If it does, it's an illegal state, possibly - // introduced by a bug elsewhere in the system -- we need to - // produce a loud warning (i.e. an ERROR-level log message) on - // this, while still removing the zones. - error!( - &log, - "sled has state Decommissioned, yet has zones \ - allocated to it; will expunge them \ - (sled policy is \"{policy}\")" - ); - } - ZoneExpungeReason::SledExpunged => { - // This is the expected situation. - info!( - &log, - "expunged sled with {} non-expunged zones found \ - (will expunge all zones)", - zones_to_expunge.len() - ); - } - } - // Now expunge all the zones that need it. let change = self.zones.change_sled_zones(sled_id); - change.expunge_zones(zones_to_expunge.clone()).map_err(|error| { - anyhow!(error) - .context(format!("for sled {sled_id}, error expunging zones")) - })?; - - // Finally, add a comment describing what happened. - let reason = match reason { - ZoneExpungeReason::DiskExpunged => "zone was using expunged disk", - ZoneExpungeReason::SledDecommissioned { .. } => { - "sled state is decommissioned" + change + .expunge_zones(zones_to_expunge.keys().cloned().collect()) + .map_err(|error| { + anyhow!(error).context(format!( + "for sled {sled_id}, error expunging zones" + )) + })?; + + // Finally, add comments describing what happened. + // + // Group the zones by their reason for expungement. + let mut count_disk_expunged = 0; + let mut count_sled_decommissioned = 0; + let mut count_sled_expunged = 0; + for reason in zones_to_expunge.values() { + match reason { + ZoneExpungeReason::DiskExpunged => count_disk_expunged += 1, + ZoneExpungeReason::SledDecommissioned => { + count_sled_decommissioned += 1; + } + ZoneExpungeReason::SledExpunged => count_sled_expunged += 1, + }; + } + let count_and_reason = [ + (count_disk_expunged, "zone was using expunged disk"), + (count_sled_decommissioned, "sled state is decommissioned"), + (count_sled_expunged, "sled policy is expunged"), + ]; + for (count, reason) in count_and_reason { + if count > 0 { + self.comment(format!( + "sled {sled_id} expunging {count} zones: {reason}", + )); } - ZoneExpungeReason::SledExpunged => "sled policy is expunged", - }; - - self.comment(format!( - "sled {} ({reason}): {} zones expunged", - sled_id, - zones_to_expunge.len(), - )); + } Ok(zones_to_expunge) } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 6bb113f9a2..d21511652b 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -11,7 +11,9 @@ use crate::blueprint_builder::Ensure; use crate::blueprint_builder::EnsureMultiple; use crate::blueprint_builder::Error; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintZoneConfig; use nexus_types::deployment::PlanningInput; +use nexus_types::deployment::SledDetails; use nexus_types::deployment::SledFilter; use nexus_types::deployment::ZpoolFilter; use nexus_types::external_api::views::SledPolicy; @@ -86,39 +88,10 @@ impl<'a> Planner<'a> { fn do_plan_expunge(&mut self) -> Result<(), Error> { // Remove services from sleds marked expunged. We use `SledFilter::All` - // and have a custom `needs_all_zones_expungement` function that allows us + // and have a custom `expunge_zones_for_sled` function that allows us // to produce better errors. for (sled_id, sled_details) in self.input.all_sleds(SledFilter::All) { - // Does this sled need zone expungement based on the details? - if let Some(reason) = needs_all_zones_expungement( - sled_details.state, - sled_details.policy, - ) { - // Perform the expungement. - self.blueprint.expunge_all_zones_for_sled(sled_id, reason)?; - - // In this case, all zones have been expunged. We can jump to - // the next sled. - continue; - } - - // The sled isn't being decommissioned, but there still might be - // some zones that need expungement. - // - // Remove any zones which rely on expunged disks. - self.blueprint.expunge_all_zones_for_sled_where( - sled_id, - ZoneExpungeReason::DiskExpunged, - |zone_config| { - let durable_storage_zpool = - match zone_config.zone_type.zpool() { - Some(pool) => pool, - None => return false, - }; - let zpool_id = durable_storage_zpool.id(); - !sled_details.resources.zpool_is_provisionable(&zpool_id) - }, - )?; + self.blueprint.expunge_zones_for_sled(sled_id, sled_details)?; } Ok(()) @@ -382,7 +355,7 @@ impl<'a> Planner<'a> { /// Returns `Some(reason)` if the sled needs its zones to be expunged, /// based on the policy and state. -fn needs_all_zones_expungement( +fn sled_needs_all_zones_expunged( state: SledState, policy: SledPolicy, ) -> Option { @@ -393,7 +366,7 @@ fn needs_all_zones_expungement( // an illegal state, but representable. If we see a sled in this // state, we should still expunge all zones in it, but parent code // should warn on it. - return Some(ZoneExpungeReason::SledDecommissioned { policy }); + return Some(ZoneExpungeReason::SledDecommissioned); } } @@ -403,14 +376,36 @@ fn needs_all_zones_expungement( } } +pub(crate) fn zone_needs_expungement( + sled_details: &SledDetails, + zone_config: &BlueprintZoneConfig, +) -> Option { + // Should we expunge the zone because the sled is gone? + if let Some(reason) = + sled_needs_all_zones_expunged(sled_details.state, sled_details.policy) + { + return Some(reason); + } + + // Should we expunge the zone because durable storage is gone? + if let Some(durable_storage_zpool) = zone_config.zone_type.zpool() { + let zpool_id = durable_storage_zpool.id(); + if !sled_details.resources.zpool_is_provisionable(&zpool_id) { + return Some(ZoneExpungeReason::DiskExpunged); + } + }; + + None +} + /// The reason a sled's zones need to be expunged. /// /// This is used only for introspection and logging -- it's not part of the /// logical flow. -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] pub(crate) enum ZoneExpungeReason { DiskExpunged, - SledDecommissioned { policy: SledPolicy }, + SledDecommissioned, SledExpunged, } diff --git a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt index 623bf0a756..233c3cec08 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt @@ -82,8 +82,8 @@ parent: 4d4e6c38-cd95-4c4e-8f45-6af4d686964b nexus c26b3bda-5561-44a1-a69f-22103fe209a1 in service fd00:1122:3344:101::2f METADATA: - created by: test_blueprint2 - created at: 1970-01-01T00:00:00.000Z - comment: sled 48d95fef-bc9f-4f50-9a53-1e075836291d (sled policy is expunged): 12 zones expunged, sled 68d24ac5-f341-49ea-a92a-0381b52ab387 (sled state is decommissioned): 12 zones expunged, sled 2d1cb4f2-cf44-40fc-b118-85036eb732a9: altered disks, sled 75bc286f-2b4b-482c-9431-59272af529da: altered disks, sled affab35f-600a-4109-8ea0-34a067a4e0bc: altered disks - internal DNS version: 1 - external DNS version: 1 + created by: test_blueprint2 + created at: 1970-01-01T00:00:00.000Z + comment: sled 48d95fef-bc9f-4f50-9a53-1e075836291d expunging 12 zones: sled policy is expunged, sled 68d24ac5-f341-49ea-a92a-0381b52ab387 expunging 12 zones: sled state is decommissioned, sled 2d1cb4f2-cf44-40fc-b118-85036eb732a9: altered disks, sled 75bc286f-2b4b-482c-9431-59272af529da: altered disks, sled affab35f-600a-4109-8ea0-34a067a4e0bc: altered disks + internal DNS version: 1 + external DNS version: 1 From ffbdb3b7d792428ba7665b0a65bf02f3e3a004f5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Apr 2024 13:31:53 -0700 Subject: [PATCH 11/12] Minimize output diff --- .../planning/src/blueprint_builder/builder.rs | 2 +- .../tests/output/planner_nonprovisionable_bp2.txt | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index f89d2a8696..d56b2731eb 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -499,7 +499,7 @@ impl<'a> BlueprintBuilder<'a> { for (count, reason) in count_and_reason { if count > 0 { self.comment(format!( - "sled {sled_id} expunging {count} zones: {reason}", + "sled {sled_id} ({reason}): {count} zones expunged", )); } } diff --git a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt index 233c3cec08..623bf0a756 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt @@ -82,8 +82,8 @@ parent: 4d4e6c38-cd95-4c4e-8f45-6af4d686964b nexus c26b3bda-5561-44a1-a69f-22103fe209a1 in service fd00:1122:3344:101::2f METADATA: - created by: test_blueprint2 - created at: 1970-01-01T00:00:00.000Z - comment: sled 48d95fef-bc9f-4f50-9a53-1e075836291d expunging 12 zones: sled policy is expunged, sled 68d24ac5-f341-49ea-a92a-0381b52ab387 expunging 12 zones: sled state is decommissioned, sled 2d1cb4f2-cf44-40fc-b118-85036eb732a9: altered disks, sled 75bc286f-2b4b-482c-9431-59272af529da: altered disks, sled affab35f-600a-4109-8ea0-34a067a4e0bc: altered disks - internal DNS version: 1 - external DNS version: 1 + created by: test_blueprint2 + created at: 1970-01-01T00:00:00.000Z + comment: sled 48d95fef-bc9f-4f50-9a53-1e075836291d (sled policy is expunged): 12 zones expunged, sled 68d24ac5-f341-49ea-a92a-0381b52ab387 (sled state is decommissioned): 12 zones expunged, sled 2d1cb4f2-cf44-40fc-b118-85036eb732a9: altered disks, sled 75bc286f-2b4b-482c-9431-59272af529da: altered disks, sled affab35f-600a-4109-8ea0-34a067a4e0bc: altered disks + internal DNS version: 1 + external DNS version: 1 From e80756b00b689e939a2b265472eeffe32d3a5a68 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 23 Apr 2024 17:33:36 -0700 Subject: [PATCH 12/12] More explicit logs, tests --- .../planning/src/blueprint_builder/builder.rs | 10 +++++++--- nexus/reconfigurator/planning/src/planner.rs | 18 +++++++++++++----- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index d56b2731eb..7703e71544 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -412,6 +412,10 @@ impl<'a> BlueprintBuilder<'a> { let sled_zones = self.zones.current_sled_zones(sled_id); for (zone_config, state) in sled_zones { let zone_id = zone_config.id; + let log = log.new(o!( + "zone_id" => zone_id.to_string() + )); + let Some(reason) = zone_needs_expungement(sled_details, zone_config) else { @@ -430,7 +434,7 @@ impl<'a> BlueprintBuilder<'a> { ZoneExpungeReason::DiskExpunged => { info!( &log, - "expunged disk with non-expunged zone(s) was found" + "expunged disk with non-expunged zone was found" ); } ZoneExpungeReason::SledDecommissioned => { @@ -441,7 +445,7 @@ impl<'a> BlueprintBuilder<'a> { // this, while still removing the zones. error!( &log, - "sled has state Decommissioned, yet has zone {zone_id} \ + "sled has state Decommissioned, yet has zone \ allocated to it; will expunge it" ); } @@ -449,7 +453,7 @@ impl<'a> BlueprintBuilder<'a> { // This is the expected situation. info!( &log, - "expunged sled with non-expunged zone {zone_id} found" + "expunged sled with non-expunged zone found" ); } } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index d21511652b..6f7f78fd1e 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -906,11 +906,19 @@ mod test { assert_eq!(diff_modified.zones_added().len(), 0); assert_eq!(diff_modified.zones_removed().len(), 0); assert_eq!(diff_modified.zones_modified().count(), 1); - assert!(diff_modified - .zones_modified() - .next() - .unwrap() - .disposition_changed()); + + let modified_zone = + &diff_modified.zones_modified().next().unwrap().zone_after; + assert!( + modified_zone.zone_type.is_crucible(), + "Expected the modified zone to be a Crucible zone, but it was: {:?}", + modified_zone.zone_type + ); + assert_eq!( + modified_zone.disposition, + BlueprintZoneDisposition::Expunged, + "Should have expunged this zone" + ); logctx.cleanup_successful(); }