From 810433142a062d5423bf2e82aba41ab791ee3d88 Mon Sep 17 00:00:00 2001 From: "Andrew J. Stone" Date: Wed, 15 May 2024 21:43:31 +0000 Subject: [PATCH] some review fixes --- nexus/types/src/deployment.rs | 58 +++------------- nexus/types/src/deployment/blueprint_diff.rs | 69 ++++++++++++++------ 2 files changed, 59 insertions(+), 68 deletions(-) diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index d1587ae967..39118a2b27 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -407,24 +407,22 @@ impl<'a> fmt::Display for BlueprintDisplay<'a> { ); // Construct the zones subtable - let zones_table = match self.blueprint.blueprint_zones.get(sled_id) - { + match self.blueprint.blueprint_zones.get(sled_id) { Some(zones) => { let zones = BlueprintOrCollectionZonesConfig::from(zones.clone()); - BpSledSubtable::new( + let zones_tab = BpSledSubtable::new( BpOmicronZonesSubtableSchema {}, zones.bp_generation(), zones.rows(BpDiffState::Unchanged).collect(), - ) - .to_string() + ); + writeln!( + f, + "\n sled: {sled_id}\n\n{disks_table}\n\n{zones_tab}\n" + )?; } - None => "".to_string(), - }; - writeln!( - f, - "\n sled: {sled_id}\n\n{disks_table}\n\n{zones_table}\n" - )?; + None => writeln!(f, "\n sled: {sled_id}\n\n{disks_table}\n")?, + } seen_sleds.insert(sled_id); } @@ -433,7 +431,7 @@ impl<'a> fmt::Display for BlueprintDisplay<'a> { // // This should basically be impossible, so we warn if it occurs. for (sled_id, zones) in &self.blueprint.blueprint_zones { - if !seen_sleds.contains(sled_id) { + if !seen_sleds.contains(sled_id) && !zones.zones.is_empty() { let zones = BlueprintOrCollectionZonesConfig::from(zones.clone()); writeln!( @@ -1201,42 +1199,6 @@ impl BlueprintOrCollectionZoneConfig { } } } - - /// We only allow `disposition` to change, otherwise the modification - /// is invalid and we return an error message. - pub fn is_valid_modification( - &self, - after: &BlueprintZoneConfig, - ) -> Result<(), String> { - let mut reason = String::new(); - if self.kind() != after.kind() { - let msg = format!( - "mismatched zone kind: before: {}, after: {}\n", - self.kind(), - after.kind() - ); - reason.push_str(&msg); - } - if self.underlay_address() != after.underlay_address { - let msg = format!( - "mismatched underlay address: before: {}, after: {}\n", - self.underlay_address(), - after.underlay_address - ); - reason.push_str(&msg); - } - if !self.is_zone_type_equal(&after.zone_type) { - let msg = - format!("mismatched zone type: after: {:?}\n", after.zone_type); - reason.push_str(&msg); - } - - if reason.is_empty() { - Ok(()) - } else { - Err(reason) - } - } } /// Single sled's disks config for "before" version within a [`BlueprintDiff`]. diff --git a/nexus/types/src/deployment/blueprint_diff.rs b/nexus/types/src/deployment/blueprint_diff.rs index bb09348c21..037ffdcd02 100644 --- a/nexus/types/src/deployment/blueprint_diff.rs +++ b/nexus/types/src/deployment/blueprint_diff.rs @@ -21,6 +21,7 @@ use crate::deployment::{ BlueprintOrCollectionZoneConfig, BlueprintOrCollectionZonesConfig, BlueprintPhysicalDisksConfig, BlueprintZoneConfig, BlueprintZoneDisposition, BlueprintZonesConfig, DiffBeforeMetadata, + ZoneSortKey, }; /// Diffs for omicron zones on a given sled with a given `BpDiffState` @@ -68,6 +69,49 @@ pub struct ModifiedZone { pub zone: BlueprintOrCollectionZoneConfig, } +impl ModifiedZone { + pub fn new( + before: BlueprintOrCollectionZoneConfig, + after: BlueprintZoneConfig, + ) -> Result { + // Do we have any errors? If so, create a "reason" string. + let mut reason = String::new(); + if before.kind() != after.kind() { + let msg = format!( + "mismatched zone kind: before: {}, after: {}\n", + before.kind(), + after.kind() + ); + reason.push_str(&msg); + } + if before.underlay_address() != after.underlay_address { + let msg = format!( + "mismatched underlay address: before: {}, after: {}\n", + before.underlay_address(), + after.underlay_address + ); + reason.push_str(&msg); + } + if !before.is_zone_type_equal(&after.zone_type) { + let msg = + format!("mismatched zone type: after: {:?}\n", after.zone_type); + reason.push_str(&msg); + } + if reason.is_empty() { + Ok(ModifiedZone { + prior_disposition: before.disposition(), + zone: after.into(), + }) + } else { + Err(BpDiffZoneError { + zone_before: before, + zone_after: after.into(), + reason, + }) + } + } +} + /// Details of modified zones on a given sled #[derive(Debug)] pub struct BpDiffZonesModified { @@ -163,24 +207,11 @@ impl BpDiffZones { } else { // The zones are different. They are only allowed to differ in terms // of `disposition`, otherwise we have an error. - match zone_before.is_valid_modification(&zone_after) - { - Ok(()) => { - let modified_zone = ModifiedZone { - prior_disposition: zone_before - .disposition(), - zone: zone_after.into(), - }; - modified.push(modified_zone); - } - Err(reason) => { - let error = BpDiffZoneError { - zone_before, - zone_after: zone_after.into(), - reason, - }; - errors.push(error); + match ModifiedZone::new(zone_before, zone_after) { + Ok(modified_zone) => { + modified.push(modified_zone) } + Err(error) => errors.push(error), } } } else { @@ -303,8 +334,7 @@ impl BpDiffZones { /// back less. /// /// Errors are printed in a more freeform manner after the table is - // displayed. - + /// displayed. pub fn to_bp_sled_subtable( &self, sled_id: &SledUuid, @@ -499,7 +529,6 @@ impl BpDiffPhysicalDisks { } } -// TODO: Make this pub and remove the other from deployment.rs /// Summarizes the differences between two blueprints #[derive(Debug)] pub struct BlueprintDiff {