[reconfigurator] use tabled to display blueprints and diffs

clients/sled-agent-client/src/lib.rs

@@ -8,6 +8,7 @@ use anyhow::Context;
 use async_trait::async_trait;
 use omicron_common::api::internal::shared::NetworkInterface;
 use std::convert::TryFrom;
+use std::fmt;
 use std::hash::Hash;
 use std::net::IpAddr;
 use std::net::SocketAddr;
@@ -56,25 +57,65 @@ impl Eq for types::OmicronZoneConfig {}
 impl Eq for types::OmicronZoneType {}
 impl Eq for types::OmicronZoneDataset {}
 
+/// Like [`types::OmicronZoneType`], but without any associated data.
+///
+/// We have a few enums of this form floating around. This particular one is
+/// meant to correspond exactly 1:1 with `OmicronZoneType`.
+///
+/// The [`fmt::Display`] impl for this type is a human-readable label, meant
+/// for testing and reporting.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub enum ZoneKind {
+    BoundaryNtp,
+    Clickhouse,
+    ClickhouseKeeper,
+    CockroachDb,
+    Crucible,
+    CruciblePantry,
+    ExternalDns,
+    InternalDns,
+    InternalNtp,
+    Nexus,
+    Oximeter,
+}
+
+impl fmt::Display for ZoneKind {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            ZoneKind::BoundaryNtp => write!(f, "boundary_ntp"),
+            ZoneKind::Clickhouse => write!(f, "clickhouse"),
+            ZoneKind::ClickhouseKeeper => write!(f, "clickhouse_keeper"),
+            ZoneKind::CockroachDb => write!(f, "cockroach_db"),
+            ZoneKind::Crucible => write!(f, "crucible"),
+            ZoneKind::CruciblePantry => write!(f, "crucible_pantry"),
+            ZoneKind::ExternalDns => write!(f, "external_dns"),
+            ZoneKind::InternalDns => write!(f, "internal_dns"),
+            ZoneKind::InternalNtp => write!(f, "internal_ntp"),
+            ZoneKind::Nexus => write!(f, "nexus"),
+            ZoneKind::Oximeter => write!(f, "oximeter"),
+        }
+    }
+}
+
 impl types::OmicronZoneType {
-    /// Human-readable label describing what kind of zone this is
-    ///
-    /// This is just use for testing and reporting.
-    pub fn label(&self) -> impl std::fmt::Display {
+    /// Returns the [`ZoneKind`] corresponding to this variant.
+    pub fn kind(&self) -> ZoneKind {
         match self {
-            types::OmicronZoneType::BoundaryNtp { .. } => "boundary_ntp",
-            types::OmicronZoneType::Clickhouse { .. } => "clickhouse",
+            types::OmicronZoneType::BoundaryNtp { .. } => ZoneKind::BoundaryNtp,
+            types::OmicronZoneType::Clickhouse { .. } => ZoneKind::Clickhouse,
             types::OmicronZoneType::ClickhouseKeeper { .. } => {
-                "clickhouse_keeper"
+                ZoneKind::ClickhouseKeeper
+            }
+            types::OmicronZoneType::CockroachDb { .. } => ZoneKind::CockroachDb,
+            types::OmicronZoneType::Crucible { .. } => ZoneKind::Crucible,
+            types::OmicronZoneType::CruciblePantry { .. } => {
+                ZoneKind::CruciblePantry
             }
-            types::OmicronZoneType::CockroachDb { .. } => "cockroach_db",
-            types::OmicronZoneType::Crucible { .. } => "crucible",
-            types::OmicronZoneType::CruciblePantry { .. } => "crucible_pantry",
-            types::OmicronZoneType::ExternalDns { .. } => "external_dns",
-            types::OmicronZoneType::InternalDns { .. } => "internal_dns",
-            types::OmicronZoneType::InternalNtp { .. } => "internal_ntp",
-            types::OmicronZoneType::Nexus { .. } => "nexus",
-            types::OmicronZoneType::Oximeter { .. } => "oximeter",
+            types::OmicronZoneType::ExternalDns { .. } => ZoneKind::ExternalDns,
+            types::OmicronZoneType::InternalDns { .. } => ZoneKind::InternalDns,
+            types::OmicronZoneType::InternalNtp { .. } => ZoneKind::InternalNtp,
+            types::OmicronZoneType::Nexus { .. } => ZoneKind::Nexus,
+            types::OmicronZoneType::Oximeter { .. } => ZoneKind::Oximeter,
         }
     }
 

dev-tools/omdb/src/bin/omdb/db.rs

@@ -3244,7 +3244,7 @@ fn inv_collection_print_sleds(collection: &Collection) {
 
             println!("    ZONES FOUND");
             for z in &zones.zones.zones {
-                println!("      zone {} (type {})", z.id, z.zone_type.label());
+                println!("      zone {} (type {})", z.id, z.zone_type.kind());
             }
         } else {
             println!("  warning: no zone information found");

dev-tools/omdb/src/bin/omdb/nexus.rs

@@ -972,7 +972,8 @@ async fn cmd_nexus_blueprints_diff(
     let b2 = client.blueprint_view(&args.blueprint2_id).await.with_context(
         || format!("fetching blueprint {}", args.blueprint2_id),
     )?;
-    println!("{}", b1.diff_sleds(&b2).display());
+    let diff = b2.diff_since_blueprint(&b1).context("diffing blueprints")?;
+    println!("{}", diff.display());
     Ok(())
 }
 

dev-tools/reconfigurator-cli/src/main.rs

@@ -669,8 +669,10 @@ fn cmd_blueprint_diff(
         .get(&blueprint2_id)
         .ok_or_else(|| anyhow!("no such blueprint: {}", blueprint2_id))?;
 
-    let sled_diff = blueprint1.diff_sleds(&blueprint2).display().to_string();
-    swriteln!(rv, "{}", sled_diff);
+    let sled_diff = blueprint2
+        .diff_since_blueprint(&blueprint1)
+        .context("failed to diff blueprints")?;
+    swriteln!(rv, "{}", sled_diff.display());
 
     // Diff'ing DNS is a little trickier.  First, compute what DNS should be for
     // each blueprint.  To do that we need to construct a list of sleds suitable
@@ -795,7 +797,9 @@ fn cmd_blueprint_diff_inventory(
         .get(&blueprint_id)
         .ok_or_else(|| anyhow!("no such blueprint: {}", blueprint_id))?;
 
-    let diff = blueprint.diff_sleds_from_collection(&collection);
+    let diff = blueprint
+        .diff_since_collection(&collection)
+        .context("failed to diff blueprint from inventory collection")?;
     Ok(Some(diff.display().to_string()))
 }
 

nexus/db-queries/src/db/datastore/deployment.rs

@@ -486,6 +486,11 @@ impl DataStore {
             }
         }
 
+        // Sort all zones to match what blueprint builders do.
+        for (_, zones_config) in blueprint_zones.iter_mut() {
+            zones_config.sort();
+        }
+
         bail_unless!(
             omicron_zone_nics.is_empty(),
             "found extra Omicron zone NICs: {:?}",
@@ -1185,6 +1190,7 @@ mod tests {
     use omicron_common::address::Ipv6Subnet;
     use omicron_common::api::external::Generation;
     use omicron_test_utils::dev;
+    use pretty_assertions::assert_eq;
     use rand::thread_rng;
     use rand::Rng;
     use std::mem;
@@ -1515,7 +1521,10 @@ mod tests {
             .blueprint_read(&opctx, &authz_blueprint2)
             .await
             .expect("failed to read collection back");
-        println!("diff: {}", blueprint2.diff_sleds(&blueprint_read).display());
+        let diff = blueprint_read
+            .diff_since_blueprint(&blueprint2)
+            .expect("failed to diff blueprints");
+        println!("diff: {}", diff.display());
         assert_eq!(blueprint2, blueprint_read);
         assert_eq!(blueprint2.internal_dns_version, new_internal_dns_version);
         assert_eq!(blueprint2.external_dns_version, new_external_dns_version);

nexus/inventory/src/collector.rs

@@ -490,7 +490,7 @@ mod test {
                         &mut s,
                         "        zone {} type {}\n",
                         zone.id,
-                        zone.zone_type.label(),
+                        zone.zone_type.kind(),
                     )
                     .unwrap();
                 }

nexus/reconfigurator/execution/src/dns.rs

@@ -281,7 +281,7 @@ pub fn blueprint_internal_dns_config(
         let context = || {
             format!(
                 "parsing {} zone with id {}",
-                zone.config.zone_type.label(),
+                zone.config.zone_type.kind(),
                 zone.config.id
             )
         };

nexus/reconfigurator/planning/src/blueprint_builder.rs

@@ -860,6 +860,7 @@ pub mod test {
     use crate::example::example;
     use crate::example::ExampleSystem;
     use crate::system::SledBuilder;
+    use expectorate::assert_contents;
     use omicron_common::address::IpRange;
     use omicron_test_utils::dev::test_setup_log;
     use sled_agent_client::types::{OmicronZoneConfig, OmicronZoneType};
@@ -904,14 +905,23 @@ pub mod test {
             .expect("failed to create initial blueprint");
         verify_blueprint(&blueprint_initial);
 
-        let diff = blueprint_initial.diff_sleds_from_collection(&collection);
+        let diff =
+            blueprint_initial.diff_since_collection(&collection).unwrap();
+        // There are some differences with even a no-op diff between a
+        // collection and a blueprint, such as new data being added to
+        // blueprints like DNS generation numbers.
         println!(
-            "collection -> initial blueprint (expected no changes):\n{}",
+            "collection -> initial blueprint \
+             (expected no non-trivial changes):\n{}",
             diff.display()
         );
-        assert_eq!(diff.sleds_added().count(), 0);
-        assert_eq!(diff.sleds_removed().count(), 0);
-        assert_eq!(diff.sleds_changed().count(), 0);
+        assert_contents(
+            "tests/output/blueprint_builder_initial_diff.txt",
+            &diff.display().to_string(),
+        );
+        assert_eq!(diff.sleds_added().len(), 0);
+        assert_eq!(diff.sleds_removed().len(), 0);
+        assert_eq!(diff.sleds_modified().count(), 0);
 
         // Test a no-op blueprint.
         let builder = BlueprintBuilder::new_based_on(
@@ -925,14 +935,14 @@ pub mod test {
         .expect("failed to create builder");
         let blueprint = builder.build();
         verify_blueprint(&blueprint);
-        let diff = blueprint_initial.diff_sleds(&blueprint);
+        let diff = blueprint.diff_since_blueprint(&blueprint_initial).unwrap();
         println!(
             "initial blueprint -> next blueprint (expected no changes):\n{}",
             diff.display()
         );
-        assert_eq!(diff.sleds_added().count(), 0);
-        assert_eq!(diff.sleds_removed().count(), 0);
-        assert_eq!(diff.sleds_changed().count(), 0);
+        assert_eq!(diff.sleds_added().len(), 0);
+        assert_eq!(diff.sleds_removed().len(), 0);
+        assert_eq!(diff.sleds_modified().count(), 0);
 
         logctx.cleanup_successful();
     }
@@ -970,14 +980,14 @@ pub mod test {
 
         let blueprint2 = builder.build();
         verify_blueprint(&blueprint2);
-        let diff = blueprint1.diff_sleds(&blueprint2);
+        let diff = blueprint2.diff_since_blueprint(&blueprint1).unwrap();
         println!(
             "initial blueprint -> next blueprint (expected no changes):\n{}",
             diff.display()
         );
-        assert_eq!(diff.sleds_added().count(), 0);
-        assert_eq!(diff.sleds_removed().count(), 0);
-        assert_eq!(diff.sleds_changed().count(), 0);
+        assert_eq!(diff.sleds_added().len(), 0);
+        assert_eq!(diff.sleds_removed().len(), 0);
+        assert_eq!(diff.sleds_modified().count(), 0);
 
         // The next step is adding these zones to a new sled.
         let new_sled_id = example.sled_rng.next();
@@ -1003,12 +1013,12 @@ pub mod test {
 
         let blueprint3 = builder.build();
         verify_blueprint(&blueprint3);
-        let diff = blueprint2.diff_sleds(&blueprint3);
+        let diff = blueprint3.diff_since_blueprint(&blueprint2).unwrap();
         println!("expecting new NTP and Crucible zones:\n{}", diff.display());
 
         // No sleds were changed or removed.
-        assert_eq!(diff.sleds_changed().count(), 0);
-        assert_eq!(diff.sleds_removed().count(), 0);
+        assert_eq!(diff.sleds_modified().count(), 0);
+        assert_eq!(diff.sleds_removed().len(), 0);
 
         // One sled was added.
         let sleds: Vec<_> = diff.sleds_added().collect();