[reconfigurator] Fix assert_planning_makes_no_changes() (#7512)

Despite its name, this test function didn't actually invoke the planner.
Now it does, and this fixes up some tests that now fail (and changes the
behavior of the planner in one case where the test failure arguably
indicated some planner misbehavior).

The specific planner change: Previously, if we added an NTP zone to a
sled, we considered that sled ineligible for any other zones (consistent
with the sled-agent behavior of refusing to start zones if time isn't
sync'd yet). This PR relaxes that slightly: if we're adding an NTP zone
to a sled and that sled already has other zones that needed time to be
sync'd, we do consider it eligible. This avoids having to make multiple
planner iterations to repopulate a sled that had a subset of disks/zones
expunged, while maintaining the existing behavior for new sleds being
added.

I'm not totally convinced that the existing behavior for new sleds is
necessary; since we're only waiting for the presence of an NTP zone, not
an inventory report that time is actually sync'd, I'm not sure there's a
meaningful difference to requiring two planning iterations instead of
one to place extra zones. But I don't have a reason to propose changing
it or testing it other than "seems a little over-cautious", so it seems
fine to leave it.

Author: jgallagher

nexus/reconfigurator/planning/src/blueprint_builder/builder.rs

@@ -2057,8 +2057,8 @@ pub mod test {
     use crate::example::example;
     use crate::example::ExampleSystemBuilder;
     use crate::example::SimRngState;
+    use crate::planner::test::assert_planning_makes_no_changes;
     use crate::system::SledBuilder;
-    use nexus_inventory::CollectionBuilder;
     use nexus_reconfigurator_blippy::Blippy;
     use nexus_reconfigurator_blippy::BlippyReportSortKey;
     use nexus_types::deployment::BlueprintDatasetDisposition;
@@ -2087,34 +2087,6 @@ pub mod test {
         }
     }
 
-    #[track_caller]
-    pub fn assert_planning_makes_no_changes(
-        log: &Logger,
-        blueprint: &Blueprint,
-        input: &PlanningInput,
-        test_name: &'static str,
-    ) {
-        let collection = CollectionBuilder::new("test").build();
-        let builder = BlueprintBuilder::new_based_on(
-            &log,
-            &blueprint,
-            &input,
-            &collection,
-            test_name,
-        )
-        .expect("failed to create builder");
-        let child_blueprint = builder.build();
-        verify_blueprint(&child_blueprint);
-        let diff = child_blueprint.diff_since_blueprint(&blueprint);
-        println!(
-            "diff between blueprints (expected no changes):\n{}",
-            diff.display()
-        );
-        assert_eq!(diff.sleds_added.len(), 0);
-        assert_eq!(diff.sleds_removed.len(), 0);
-        assert_eq!(diff.sleds_modified.len(), 0);
-    }
-
     #[test]
     fn test_basic() {
         static TEST_NAME: &str = "blueprint_builder_test_basic";
@@ -2267,6 +2239,7 @@ pub mod test {
             &logctx.log,
             &blueprint3,
             &input,
+            &example.collection,
             TEST_NAME,
         );
 
@@ -2388,14 +2361,6 @@ pub mod test {
             Some(SledState::Decommissioned),
         );
 
-        // Test a no-op planning iteration.
-        assert_planning_makes_no_changes(
-            &logctx.log,
-            &blueprint4,
-            &input,
-            TEST_NAME,
-        );
-
         logctx.cleanup_successful();
     }
 
@@ -2840,14 +2805,25 @@ pub mod test {
         static TEST_NAME: &str = "blueprint_builder_test_ensure_cockroachdb";
         let logctx = test_setup_log(TEST_NAME);
 
-        // Start with an empty system (sleds with no zones).
+        // Start with an example system (no CRDB zones).
         let (example, parent) =
-            ExampleSystemBuilder::new(&logctx.log, TEST_NAME)
-                .create_zones(false)
-                .build();
+            ExampleSystemBuilder::new(&logctx.log, TEST_NAME).build();
         let collection = example.collection;
         let input = example.input;
 
+        // Ensure no CRDB zones (currently `ExampleSystemBuilder` never
+        // provisions CRDB; this check makes sure we update our use of it if
+        // that changes).
+        for (_, z) in
+            parent.all_omicron_zones(BlueprintZoneFilter::ShouldBeRunning)
+        {
+            assert!(
+                !z.zone_type.is_cockroach(),
+                "unexpected cockroach zone \
+                 (update use of ExampleSystemBuilder?): {z:?}"
+            );
+        }
+
         // Pick an arbitrary sled.
         let (target_sled_id, sled_resources) = input
             .all_sled_resources(SledFilter::InService)
@@ -2895,6 +2871,7 @@ pub mod test {
             &logctx.log,
             &blueprint,
             &input,
+            &collection,
             TEST_NAME,
         );