some review fixes

oxidecomputer · May 15, 2024 · 8104331 · 8104331
1 parent e7cc233
commit 8104331
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 68 deletions.
diff --git 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
@@ -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<ModifiedZone, BpDiffZoneError> {
+        // 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 {