diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 7e98b3906d..c822e87a8f 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 internal_dns::config::Host; @@ -25,6 +26,7 @@ use nexus_types::deployment::DiskFilter; use nexus_types::deployment::OmicronZoneDataset; use nexus_types::deployment::OmicronZoneExternalFloatingIp; 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; @@ -351,33 +353,72 @@ 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> { + 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 { + 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 { + 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 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 \ + allocated to it; will expunge it" + ); + } + ZoneExpungeReason::SledExpunged => { + // This is the expected situation. + info!( + &log, + "expunged sled with non-expunged zone found" + ); + } + } + + zones_to_expunge.insert(zone_id, reason); } } @@ -389,51 +430,43 @@ impl<'a> BlueprintBuilder<'a> { return Ok(zones_to_expunge); } - match reason { - 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::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} ({reason}): {count} zones expunged", + )); } - 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 6ed81cbb63..7f7b4f61ec 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -12,11 +12,13 @@ use crate::blueprint_builder::EnsureMultiple; use crate::blueprint_builder::Error; use crate::planner::omicron_zone_placement::PlacementError; use nexus_types::deployment::Blueprint; +use nexus_types::deployment::BlueprintZoneConfig; use nexus_types::deployment::BlueprintZoneDisposition; use nexus_types::deployment::CockroachDbClusterVersion; use nexus_types::deployment::CockroachDbPreserveDowngrade; use nexus_types::deployment::CockroachDbSettings; 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; @@ -170,15 +172,8 @@ impl<'a> Planner<'a> { { commissioned_sled_ids.insert(sled_id); - // Does this sled need zone expungement based on the details? - let Some(reason) = - needs_zone_expungement(sled_details.state, sled_details.policy) - else { - continue; - }; - - // Perform the expungement. - self.blueprint.expunge_all_zones_for_sled(sled_id, reason)?; + // Perform the expungement, for any zones that might need it. + self.blueprint.expunge_zones_for_sled(sled_id, sled_details)?; } // Check for any decommissioned sleds (i.e., sleds for which our @@ -558,7 +553,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 sled_needs_all_zones_expunged( state: SledState, policy: SledPolicy, ) -> Option { @@ -569,7 +564,7 @@ fn needs_zone_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); } } @@ -579,13 +574,36 @@ fn needs_zone_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 { - SledDecommissioned { policy: SledPolicy }, + DiskExpunged, + SledDecommissioned, SledExpunged, } @@ -611,6 +629,9 @@ mod test { use nexus_types::deployment::CockroachDbPreserveDowngrade; use nexus_types::deployment::CockroachDbSettings; use nexus_types::deployment::OmicronZoneNetworkResources; + 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; @@ -1032,7 +1053,7 @@ mod test { // Make generated disk ids deterministic let mut disk_rng = TypedUuidRng::from_seed(TEST_NAME, "NewPhysicalDisks"); - let mut new_sled_disk = |policy| nexus_types::deployment::SledDisk { + let mut new_sled_disk = |policy| SledDisk { disk_identity: DiskIdentity { vendor: "test-vendor".to_string(), serial: "test-serial".to_string(), @@ -1040,7 +1061,7 @@ mod test { }, disk_id: PhysicalDiskUuid::from(disk_rng.next()), policy, - state: nexus_types::external_api::views::PhysicalDiskState::Active, + state: PhysicalDiskState::Active, }; let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); @@ -1057,13 +1078,13 @@ mod test { for _ in 0..NEW_IN_SERVICE_DISKS { sled_details.resources.zpools.insert( ZpoolUuid::from(zpool_rng.next()), - 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::from(zpool_rng.next()), - new_sled_disk(nexus_types::external_api::views::PhysicalDiskPolicy::Expunged), + new_sled_disk(PhysicalDiskPolicy::Expunged), ); } @@ -1096,6 +1117,73 @@ 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(); + + // Aside: Avoid churning on the quantity of Nexus zones - we're okay + // staying at one. + builder.policy_mut().target_nexus_zone_count = 1; + + // The example system should be assigning crucible zones to each + // in-service disk. When we expunge one of these disks, the planner + // should remove the associated zone. + let (_, sled_details) = builder.sleds_mut().iter_mut().next().unwrap(); + let (_, disk) = + sled_details.resources.zpools.iter_mut().next().unwrap(); + disk.policy = 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); + println!("1 -> 2 (expunge a disk):\n{}", diff.display()); + assert_eq!(diff.sleds_added.len(), 0); + assert_eq!(diff.sleds_removed.len(), 0); + assert_eq!(diff.sleds_modified.len(), 1); + + // We should be removing a single zone, associated with the Crucible + // using that device. + assert_eq!(diff.zones.added.len(), 0); + assert_eq!(diff.zones.removed.len(), 0); + assert_eq!(diff.zones.modified.len(), 1); + + let (_zone_id, modified_zones) = + diff.zones.modified.iter().next().unwrap(); + assert_eq!(modified_zones.zones.len(), 1); + let modified_zone = &modified_zones.zones.first().unwrap().zone; + assert!( + matches!(modified_zone.kind(), ZoneKind::Crucible), + "Expected the modified zone to be a Crucible zone, but it was: {:?}", + modified_zone.kind() + ); + assert_eq!( + modified_zone.disposition(), + BlueprintZoneDisposition::Expunged, + "Should have expunged this zone" + ); + + logctx.cleanup_successful(); + } + /// Check that the planner will skip non-provisionable sleds when allocating /// extra Nexus zones #[test] diff --git a/nexus/types/src/deployment/zone_type.rs b/nexus/types/src/deployment/zone_type.rs index 9f663015cd..5b14f1ee3c 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_networking( &self, ) -> Option<(OmicronZoneExternalIp, &NetworkInterface)> {