diff --git a/dev-tools/reconfigurator-cli/src/main.rs b/dev-tools/reconfigurator-cli/src/main.rs index 1c9d9866a8..b5e65249ce 100644 --- a/dev-tools/reconfigurator-cli/src/main.rs +++ b/dev-tools/reconfigurator-cli/src/main.rs @@ -748,7 +748,10 @@ fn cmd_blueprint_edit( let added = builder .sled_ensure_zone_multiple_nexus(sled_id, current + 1) .context("failed to add Nexus zone")?; - assert_matches::assert_matches!(added, EnsureMultiple::Added(1)); + assert_matches::assert_matches!( + added, + EnsureMultiple::Changed { added: 1, removed: 0 } + ); format!("added Nexus zone to sled {}", sled_id) } }; diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 790dc0d72c..617413f172 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -1343,6 +1343,7 @@ mod tests { use nexus_inventory::now_db_precision; use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder; use nexus_reconfigurator_planning::blueprint_builder::Ensure; + use nexus_reconfigurator_planning::blueprint_builder::EnsureMultiple; use nexus_reconfigurator_planning::example::example; use nexus_test_utils::db::test_setup_database; use nexus_types::deployment::BlueprintZoneDisposition; @@ -1651,7 +1652,7 @@ mod tests { .clone(), ) .unwrap(), - Ensure::Added + EnsureMultiple::Changed { added: 4, removed: 0 } ); // Add zones to our new sled. diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index ec48a35cbe..7272062293 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -1264,7 +1264,7 @@ mod test { let rv = builder .sled_ensure_zone_multiple_nexus(sled_id, nalready + 1) .unwrap(); - assert_eq!(rv, EnsureMultiple::Added(1)); + assert_eq!(rv, EnsureMultiple::Changed { added: 1, removed: 0 }); let blueprint2 = builder.build(); eprintln!("blueprint2: {}", blueprint2.display()); // Figure out the id of the new zone. diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index c822e87a8f..c6b912a683 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -58,6 +58,7 @@ use slog::Logger; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashSet; +use std::fmt; use std::hash::Hash; use std::net::IpAddr; use std::net::Ipv6Addr; @@ -112,11 +113,54 @@ pub enum Ensure { #[derive(Debug, Clone, Copy, Eq, PartialEq)] pub enum EnsureMultiple { /// action was taken, and multiple items were added - Added(usize), + Changed { added: usize, removed: usize }, + /// no action was necessary NotNeeded, } +/// Describes operations which the BlueprintBuilder has performed to arrive +/// at its state. +/// +/// This information is meant to act as a more strongly-typed flavor of +/// "comment", identifying which operations have occurred on the blueprint. +#[derive(Debug, Clone, Eq, PartialEq)] +pub(crate) enum Operation { + AddZone { sled_id: SledUuid, kind: sled_agent_client::ZoneKind }, + UpdateDisks { sled_id: SledUuid, added: usize, removed: usize }, + ZoneExpunged { sled_id: SledUuid, reason: ZoneExpungeReason, count: usize }, +} + +impl fmt::Display for Operation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::AddZone { sled_id, kind } => { + write!(f, "sled {sled_id}: added zone: {kind}") + } + Self::UpdateDisks { sled_id, added, removed } => { + write!(f, "sled {sled_id}: added {added} disks, removed {removed} disks") + } + Self::ZoneExpunged { sled_id, reason, count } => { + let reason = match reason { + ZoneExpungeReason::DiskExpunged => { + "zone using expunged disk" + } + ZoneExpungeReason::SledDecommissioned => { + "sled state is decomissioned" + } + ZoneExpungeReason::SledExpunged => { + "sled policy is expunged" + } + }; + write!( + f, + "sled {sled_id}: expunged {count} zones because: {reason}" + ) + } + } + } +} + /// Helper for assembling a blueprint /// /// There are two basic ways to assemble a new blueprint: @@ -152,6 +196,7 @@ pub struct BlueprintBuilder<'a> { cockroachdb_setting_preserve_downgrade: CockroachDbPreserveDowngrade, creator: String, + operations: Vec, comments: Vec, // Random number generator for new UUIDs @@ -274,6 +319,7 @@ impl<'a> BlueprintBuilder<'a> { cockroachdb_setting_preserve_downgrade: parent_blueprint .cockroachdb_setting_preserve_downgrade, creator: creator.to_owned(), + operations: Vec::new(), comments: Vec::new(), rng: BlueprintBuilderRng::new(), }) @@ -320,7 +366,12 @@ impl<'a> BlueprintBuilder<'a> { .cockroachdb_setting_preserve_downgrade, time_created: now_db_precision(), creator: self.creator, - comment: self.comments.join(", "), + comment: self + .comments + .into_iter() + .chain(self.operations.iter().map(|op| op.to_string())) + .collect::>() + .join(", "), } } @@ -342,10 +393,8 @@ impl<'a> BlueprintBuilder<'a> { self } - /// Sets the blueprints "comment" - /// /// This is a short human-readable string summarizing the changes reflected - /// in the blueprint. This is only intended for debugging. + /// in the blueprint. This is only intended for debugging. pub fn comment(&mut self, comment: S) where String: From, @@ -353,6 +402,14 @@ impl<'a> BlueprintBuilder<'a> { self.comments.push(String::from(comment)); } + /// Records an operation to the blueprint, identifying what changes have + /// occurred. + /// + /// This is currently intended only for debugging. + pub(crate) fn record_operation(&mut self, operation: Operation) { + self.operations.push(operation); + } + /// Expunges all zones requiring expungement from a sled. /// /// Returns a list of zone IDs expunged (excluding zones that were already @@ -456,15 +513,17 @@ impl<'a> BlueprintBuilder<'a> { }; } 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"), + (count_disk_expunged, ZoneExpungeReason::DiskExpunged), + (count_sled_decommissioned, ZoneExpungeReason::SledDecommissioned), + (count_sled_expunged, ZoneExpungeReason::SledExpunged), ]; for (count, reason) in count_and_reason { if count > 0 { - self.comment(format!( - "sled {sled_id} ({reason}): {count} zones expunged", - )); + self.record_operation(Operation::ZoneExpunged { + sled_id, + reason, + count, + }); } } @@ -483,7 +542,7 @@ impl<'a> BlueprintBuilder<'a> { &mut self, sled_id: SledUuid, resources: &SledResources, - ) -> Result { + ) -> Result { let (mut additions, removals) = { // These are the disks known to our (last?) blueprint let blueprint_disks: BTreeMap<_, _> = self @@ -533,8 +592,10 @@ impl<'a> BlueprintBuilder<'a> { }; if additions.is_empty() && removals.is_empty() { - return Ok(Ensure::NotNeeded); + return Ok(EnsureMultiple::NotNeeded); } + let added = additions.len(); + let removed = removals.len(); let disks = &mut self.disks.change_sled_disks(sled_id).disks; @@ -543,7 +604,7 @@ impl<'a> BlueprintBuilder<'a> { !removals.contains(&PhysicalDiskUuid::from_untyped_uuid(config.id)) }); - Ok(Ensure::Added) + Ok(EnsureMultiple::Changed { added, removed }) } pub fn sled_ensure_zone_ntp( @@ -779,7 +840,7 @@ impl<'a> BlueprintBuilder<'a> { self.sled_add_zone(sled_id, zone)?; } - Ok(EnsureMultiple::Added(num_nexus_to_add)) + Ok(EnsureMultiple::Changed { added: num_nexus_to_add, removed: 0 }) } pub fn cockroachdb_preserve_downgrade( @@ -1449,7 +1510,7 @@ pub mod test { builder .sled_ensure_disks(sled_id, &sled_resources) .unwrap(), - Ensure::Added, + EnsureMultiple::Changed { added: 10, removed: 0 }, ); } @@ -1597,7 +1658,7 @@ pub mod test { .sled_ensure_zone_multiple_nexus(sled_id, 1) .expect("failed to ensure nexus zone"); - assert_eq!(added, EnsureMultiple::Added(1)); + assert_eq!(added, EnsureMultiple::Changed { added: 1, removed: 0 }); } { @@ -1615,7 +1676,7 @@ pub mod test { .sled_ensure_zone_multiple_nexus(sled_id, 3) .expect("failed to ensure nexus zone"); - assert_eq!(added, EnsureMultiple::Added(3)); + assert_eq!(added, EnsureMultiple::Changed { added: 3, removed: 0 }); } { diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 7f7b4f61ec..39a57bbbb3 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -10,6 +10,7 @@ use crate::blueprint_builder::BlueprintBuilder; use crate::blueprint_builder::Ensure; use crate::blueprint_builder::EnsureMultiple; use crate::blueprint_builder::Error; +use crate::blueprint_builder::Operation; use crate::planner::omicron_zone_placement::PlacementError; use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintZoneConfig; @@ -25,6 +26,7 @@ use nexus_types::external_api::views::SledPolicy; use nexus_types::external_api::views::SledState; use nexus_types::inventory::Collection; use omicron_uuid_kinds::SledUuid; +use sled_agent_client::ZoneKind; use slog::error; use slog::{info, warn, Logger}; use std::collections::BTreeMap; @@ -228,16 +230,19 @@ impl<'a> Planner<'a> { { // First, we need to ensure that sleds are using their expected // disks. This is necessary before we can allocate any zones. - if self.blueprint.sled_ensure_disks(sled_id, &sled_resources)? - == Ensure::Added + if let EnsureMultiple::Changed { added, removed } = + self.blueprint.sled_ensure_disks(sled_id, &sled_resources)? { info!( &self.log, "altered physical disks"; "sled_id" => %sled_id ); - self.blueprint - .comment(&format!("sled {}: altered disks", sled_id)); + self.blueprint.record_operation(Operation::UpdateDisks { + sled_id, + added, + removed, + }); // Note that this doesn't actually need to short-circuit the // rest of the blueprint planning, as long as during execution @@ -254,8 +259,10 @@ impl<'a> Planner<'a> { "found sled missing NTP zone (will add one)"; "sled_id" => %sled_id ); - self.blueprint - .comment(&format!("sled {}: add NTP zone", sled_id)); + self.blueprint.record_operation(Operation::AddZone { + sled_id, + kind: ZoneKind::BoundaryNtp, + }); // Don't make any other changes to this sled. However, this // change is compatible with any other changes to other sleds, // so we can "continue" here rather than "break". @@ -323,7 +330,10 @@ impl<'a> Planner<'a> { // (Yes, it's currently the last thing in the loop, but being // explicit here means we won't forget to do this when more code // is added below.) - self.blueprint.comment(&format!("sled {}: add zones", sled_id)); + self.blueprint.record_operation(Operation::AddZone { + sled_id, + kind: ZoneKind::Crucible, + }); continue; } } @@ -429,12 +439,12 @@ impl<'a> Planner<'a> { .blueprint .sled_ensure_zone_multiple_nexus(sled_id, new_nexus_count)? { - EnsureMultiple::Added(n) => { + EnsureMultiple::Changed { added, removed: _ } => { info!( - self.log, "will add {n} Nexus zone(s) to sled"; + self.log, "will add {added} Nexus zone(s) to sled"; "sled_id" => %sled_id, ); - total_added += n; + total_added += added; } // This is only possible if we asked the sled to ensure the same // number of zones it already has, but that's impossible based diff --git a/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt b/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt index 3ba829b1d2..1e1f834d6c 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_decommissions_sleds_bp2.txt @@ -104,7 +104,7 @@ WARNING: Zones exist without physical disks! METADATA: created by::::::::::: test_blueprint2 created at::::::::::: 1970-01-01T00:00:00.000Z - comment:::::::::::::: sled a1b477db-b629-48eb-911d-1ccdafca75b9 (sled policy is expunged): 12 zones expunged + comment:::::::::::::: sled a1b477db-b629-48eb-911d-1ccdafca75b9: expunged 12 zones because: sled policy is expunged internal DNS version: 1 external DNS version: 1 diff --git a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt index f7c0886dde..07bf7673c0 100644 --- a/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt +++ b/nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt @@ -168,7 +168,7 @@ WARNING: Zones exist without physical disks! 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 + comment:::::::::::::: sled 48d95fef-bc9f-4f50-9a53-1e075836291d: expunged 12 zones because: sled policy is expunged internal DNS version: 1 external DNS version: 1