[reconfigurator] use tabled to display blueprints and diffs (#5270)

While developing #5238, I noticed that the output was getting
significantly busier and less aligned. I decided to prototype out using
`tabled` to display outputs, and I really liked the results.

Examples that cover all of the cases are included in the PR. In the
future I'd also like to add color support on the CLI, and expand it to
inventory and `omdb` (it's similar except it doesn't have the zone
policy table).

Some other changes that are bundled into this PR:

* Sort by (zone type, zone ID) rather than zone ID, to keep zones of the
same type grouped together.
* Moved unchanged data to the top to allow users to see less scrollback.
* Moved metadata to the bottom for the same reason.
* Add information about the zone config being changed.
* Change `Blueprint::diff_sleds` and
`Blueprint::diff_sleds_from_collection` to
`Blueprint::diff_since_blueprint` and `diff_since_collection` recently.
* Reordered `diff_since_blueprint`'s arguments so that `self` is after
and the argument is before, to align with `diff_since_collection`. (I
found that surprising!)
* Renamed the diff type from `OmicronZonesDiff` to `BlueprintDiff`,
since it's going to contain a lot more than zones.
* Return an error from the diff methods, specifically if the before and
after have the same zone ID but different types.

Depends on #5238 and #5341.

Author: sunshowers

Cargo.lock

@@ -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,
         }
     }
 

clients/sled-agent-client/src/lib.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/db.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/omdb/src/bin/omdb/nexus.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()))
 }
 

dev-tools/reconfigurator-cli/src/main.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/db-queries/src/db/datastore/deployment.rs

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

nexus/inventory/src/collector.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/execution/src/dns.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();