[nexus] Expand BlueprintDiff to identify changed datasets

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

@@ -869,17 +869,22 @@ impl<'a> BlueprintBuilder<'a> {
         let (mut additions, mut updates, mut expunges, removals) = {
             let mut datasets_builder = BlueprintSledDatasetsBuilder::new(
                 self.log.clone(),
+                &mut self.rng,
                 sled_id,
                 &self.datasets,
                 resources,
             );
 
             // Ensure each zpool has a "Debug" and "Zone Root" dataset.
-            let bp_zpools = self
+            let mut bp_zpools = self
                 .disks
                 .current_sled_disks(sled_id)
                 .map(|disk_config| disk_config.pool_id)
                 .collect::<Vec<ZpoolUuid>>();
+            // We iterate over the zpools in a deterministic order to ensure
+            // that "new Dataset UUIDs" are distributed in a reliable order.
+            bp_zpools.sort();
+
             for zpool_id in bp_zpools {
                 let zpool = ZpoolName::new_external(zpool_id);
                 let address = None;
@@ -2357,6 +2362,7 @@ impl<'a> BlueprintDatasetsBuilder<'a> {
 #[derive(Debug)]
 struct BlueprintSledDatasetsBuilder<'a> {
     log: Logger,
+    rng: &'a mut PlannerRng,
     blueprint_datasets:
         BTreeMap<ZpoolUuid, BTreeMap<DatasetKind, &'a BlueprintDatasetConfig>>,
     database_datasets:
@@ -2377,6 +2383,7 @@ struct BlueprintSledDatasetsBuilder<'a> {
 impl<'a> BlueprintSledDatasetsBuilder<'a> {
     pub fn new(
         log: Logger,
+        rng: &'a mut PlannerRng,
         sled_id: SledUuid,
         datasets: &'a BlueprintDatasetsBuilder<'_>,
         resources: &'a SledResources,
@@ -2407,6 +2414,7 @@ impl<'a> BlueprintSledDatasetsBuilder<'a> {
 
         Self {
             log,
+            rng,
             blueprint_datasets,
             database_datasets,
             unchanged_datasets: BTreeMap::new(),
@@ -2470,7 +2478,7 @@ impl<'a> BlueprintSledDatasetsBuilder<'a> {
         let id = if let Some(old_config) = self.get_from_db(zpool_id, kind) {
             old_config.id
         } else {
-            DatasetUuid::new_v4()
+            self.rng.next_dataset()
         };
 
         let new_config = make_config(id);

nexus/reconfigurator/planning/src/example.rs

@@ -410,6 +410,20 @@ impl ExampleSystemBuilder {
             }
         }
 
+        // We just ensured that a handful of datasets should exist in
+        // the blueprint, but they don't yet exist in the SystemDescription.
+        //
+        // Go back and add them so that the blueprint is consistent with
+        // inventory.
+        for (sled_id, datasets) in &blueprint.blueprint_datasets {
+            let sled = system.get_sled_mut(*sled_id).unwrap();
+
+            for dataset_config in datasets.datasets.values() {
+                let config = dataset_config.clone().try_into().unwrap();
+                sled.add_synthetic_dataset(config);
+            }
+        }
+
         let mut builder =
             system.to_collection_builder().expect("failed to build collection");
         builder.set_rng_seed((&self.test_name, "ExampleSystem collection"));

nexus/reconfigurator/planning/src/planner.rs

@@ -950,6 +950,10 @@ mod test {
         assert_eq!(diff.zones.errors.len(), 0);
         assert_eq!(diff.physical_disks.added.len(), 0);
         assert_eq!(diff.physical_disks.removed.len(), 0);
+        assert_eq!(diff.datasets.added.len(), 0);
+        assert_eq!(diff.datasets.removed.len(), 0);
+        assert_eq!(diff.datasets.modified.len(), 0);
+        assert_eq!(diff.datasets.unchanged.len(), 3);
         verify_blueprint(&blueprint2);
 
         // Now add a new sled.
@@ -981,6 +985,8 @@ mod test {
             &diff.display().to_string(),
         );
         assert_eq!(diff.sleds_added.len(), 1);
+        assert_eq!(diff.physical_disks.added.len(), 1);
+        assert_eq!(diff.datasets.added.len(), 1);
         let sled_id = *diff.sleds_added.first().unwrap();
         let sled_zones = diff.zones.added.get(&sled_id).unwrap();
         // We have defined elsewhere that the first generation contains no
@@ -1153,6 +1159,12 @@ mod test {
         assert_eq!(*changed_sled_id, sled_id);
         assert_eq!(diff.zones.removed.len(), 0);
         assert_eq!(diff.zones.modified.len(), 0);
+        assert_eq!(diff.physical_disks.added.len(), 0);
+        assert_eq!(diff.physical_disks.removed.len(), 0);
+        assert_eq!(diff.datasets.added.len(), 1);
+        assert_eq!(diff.datasets.modified.len(), 0);
+        assert_eq!(diff.datasets.removed.len(), 0);
+
         let zones_added = diff.zones.added.get(changed_sled_id).unwrap();
         assert_eq!(
             zones_added.zones.len(),
@@ -1743,6 +1755,11 @@ mod test {
             NEW_IN_SERVICE_DISKS
         );
         assert!(!diff.zones.removed.contains_key(sled_id));
+        assert_eq!(diff.physical_disks.added.len(), 1);
+        assert_eq!(diff.physical_disks.removed.len(), 0);
+        assert_eq!(diff.datasets.added.len(), 1);
+        assert_eq!(diff.datasets.modified.len(), 0);
+        assert_eq!(diff.datasets.removed.len(), 0);
 
         // Test a no-op planning iteration.
         assert_planning_makes_no_changes(
@@ -1755,6 +1772,79 @@ mod test {
         logctx.cleanup_successful();
     }
 
+    #[test]
+    fn test_dataset_settings_modified_in_place() {
+        static TEST_NAME: &str = "planner_dataset_settings_modified_in_place";
+        let logctx = test_setup_log(TEST_NAME);
+
+        // Create an example system with a single sled
+        let (example, mut blueprint1) =
+            ExampleSystemBuilder::new(&logctx.log, TEST_NAME).nsleds(1).build();
+        let collection = example.collection;
+        let input = example.input;
+
+        let mut builder = input.into_builder();
+
+        // Avoid churning on the quantity of Nexus and internal DNS zones -
+        // we're okay staying at one each.
+        builder.policy_mut().target_nexus_zone_count = 1;
+        builder.policy_mut().target_internal_dns_zone_count = 1;
+
+        // Manually update the blueprint to report an abnormal "Debug dataset"
+        let (_sled_id, datasets_config) =
+            blueprint1.blueprint_datasets.iter_mut().next().unwrap();
+        let (_dataset_id, dataset_config) = datasets_config
+            .datasets
+            .iter_mut()
+            .find(|(_, config)| {
+                matches!(config.kind, omicron_common::disk::DatasetKind::Debug)
+            })
+            .expect("No debug dataset found");
+
+        // These values are out-of-sync with what the blueprint will typically
+        // enforce.
+        dataset_config.quota = None;
+        dataset_config.reservation = Some(
+            omicron_common::api::external::ByteCount::from_gibibytes_u32(1),
+        );
+
+        let input = builder.build();
+
+        let blueprint2 = Planner::new_based_on(
+            logctx.log.clone(),
+            &blueprint1,
+            &input,
+            "test: fix a dataset",
+            &collection,
+        )
+        .expect("failed to create planner")
+        .with_rng_seed((TEST_NAME, "bp2"))
+        .plan()
+        .expect("failed to plan");
+
+        let diff = blueprint2.diff_since_blueprint(&blueprint1);
+        println!("1 -> 2 (modify a dataset):\n{}", diff.display());
+        assert_contents(
+            "tests/output/planner_dataset_settings_modified_in_place_1_2.txt",
+            &diff.display().to_string(),
+        );
+
+        assert_eq!(diff.sleds_added.len(), 0);
+        assert_eq!(diff.sleds_removed.len(), 0);
+        assert_eq!(diff.sleds_modified.len(), 1);
+
+        assert_eq!(diff.zones.added.len(), 0);
+        assert_eq!(diff.zones.removed.len(), 0);
+        assert_eq!(diff.zones.modified.len(), 0);
+        assert_eq!(diff.physical_disks.added.len(), 0);
+        assert_eq!(diff.physical_disks.removed.len(), 0);
+        assert_eq!(diff.datasets.added.len(), 0);
+        assert_eq!(diff.datasets.removed.len(), 0);
+        assert_eq!(diff.datasets.modified.len(), 1);
+
+        logctx.cleanup_successful();
+    }
+
     #[test]
     fn test_disk_expungement_removes_zones_durable_zpool() {
         static TEST_NAME: &str =
@@ -1828,6 +1918,13 @@ mod test {
         assert_eq!(diff.zones.added.len(), 0);
         assert_eq!(diff.zones.removed.len(), 0);
         assert_eq!(diff.zones.modified.len(), 1);
+        assert_eq!(diff.physical_disks.added.len(), 0);
+        assert_eq!(diff.physical_disks.removed.len(), 1);
+        assert_eq!(diff.datasets.added.len(), 0);
+        // NOTE: Expunging a disk doesn't immediately delete datasets; see the
+        // "decommissioned_disk_cleaner" background task for more context.
+        assert_eq!(diff.datasets.removed.len(), 0);
+        assert_eq!(diff.datasets.modified.len(), 0);
 
         let (_zone_id, modified_zones) =
             diff.zones.modified.iter().next().unwrap();

nexus/reconfigurator/planning/src/planner/rng.rs

@@ -5,6 +5,8 @@
 //! RNG for blueprint planning to allow reproducibility (particularly for
 //! tests).
 
+use omicron_uuid_kinds::DatasetKind;
+use omicron_uuid_kinds::DatasetUuid;
 use omicron_uuid_kinds::ExternalIpKind;
 use omicron_uuid_kinds::ExternalIpUuid;
 use omicron_uuid_kinds::OmicronZoneKind;
@@ -26,6 +28,7 @@ pub(crate) struct PlannerRng {
     // associated with a specific `TypedUuidKind`.
     blueprint_rng: UuidRng,
     zone_rng: TypedUuidRng<OmicronZoneKind>,
+    dataset_rng: TypedUuidRng<DatasetKind>,
     network_interface_rng: UuidRng,
     external_ip_rng: TypedUuidRng<ExternalIpKind>,
 }
@@ -38,12 +41,19 @@ impl PlannerRng {
     pub fn new_from_parent(mut parent: StdRng) -> Self {
         let blueprint_rng = UuidRng::from_parent_rng(&mut parent, "blueprint");
         let zone_rng = TypedUuidRng::from_parent_rng(&mut parent, "zone");
+        let dataset_rng = TypedUuidRng::from_parent_rng(&mut parent, "dataset");
         let network_interface_rng =
             UuidRng::from_parent_rng(&mut parent, "network_interface");
         let external_ip_rng =
             TypedUuidRng::from_parent_rng(&mut parent, "external_ip");
 
-        Self { blueprint_rng, zone_rng, network_interface_rng, external_ip_rng }
+        Self {
+            blueprint_rng,
+            zone_rng,
+            dataset_rng,
+            network_interface_rng,
+            external_ip_rng,
+        }
     }
 
     pub fn set_seed<H: Hash>(&mut self, seed: H) {
@@ -61,6 +71,10 @@ impl PlannerRng {
         self.zone_rng.next()
     }
 
+    pub fn next_dataset(&mut self) -> DatasetUuid {
+        self.dataset_rng.next()
+    }
+
     pub fn next_network_interface(&mut self) -> Uuid {
         self.network_interface_rng.next()
     }

nexus/reconfigurator/planning/src/system.rs

@@ -14,7 +14,9 @@ use ipnet::Ipv6Subnets;
 use nexus_inventory::CollectionBuilder;
 use nexus_sled_agent_shared::inventory::Baseboard;
 use nexus_sled_agent_shared::inventory::Inventory;
+use nexus_sled_agent_shared::inventory::InventoryDataset;
 use nexus_sled_agent_shared::inventory::InventoryDisk;
+use nexus_sled_agent_shared::inventory::InventoryZpool;
 use nexus_sled_agent_shared::inventory::OmicronZonesConfig;
 use nexus_sled_agent_shared::inventory::SledRole;
 use nexus_types::deployment::ClickhousePolicy;
@@ -247,6 +249,16 @@ impl SystemDescription {
         self
     }
 
+    pub fn get_sled_mut(
+        &mut self,
+        sled_id: SledUuid,
+    ) -> anyhow::Result<&mut Sled> {
+        let Some(sled) = self.sleds.get_mut(&sled_id) else {
+            bail!("Sled not found with id {sled_id}");
+        };
+        Ok(Arc::make_mut(sled))
+    }
+
     /// Add a sled to the system, as described by a SledBuilder
     pub fn sled(&mut self, sled: SledBuilder) -> anyhow::Result<&mut Self> {
         let sled_id = sled.id.unwrap_or_else(SledUuid::new_v4);
@@ -537,7 +549,7 @@ pub struct SledHwInventory<'a> {
 /// This needs to be rich enough to generate a PlanningInput and inventory
 /// Collection.
 #[derive(Clone, Debug)]
-struct Sled {
+pub struct Sled {
     sled_id: SledUuid,
     inventory_sp: Option<(u16, SpState)>,
     inventory_sled_agent: Inventory,
@@ -662,9 +674,13 @@ impl Sled {
                         )],
                     })
                     .collect(),
-                // Zpools & Datasets won't necessarily show up until our first
-                // request to provision storage, so we omit them.
-                zpools: vec![],
+                zpools: zpools
+                    .keys()
+                    .map(|id| InventoryZpool {
+                        id: *id,
+                        total_size: ByteCount::from_gibibytes_u32(100),
+                    })
+                    .collect(),
                 datasets: vec![],
             }
         };
@@ -816,6 +832,25 @@ impl Sled {
         }
     }
 
+    /// Adds a dataset to the system description.
+    ///
+    /// The inventory values for "available space" and "used space" are
+    /// made up, since this is a synthetic dataset.
+    pub fn add_synthetic_dataset(
+        &mut self,
+        config: omicron_common::disk::DatasetConfig,
+    ) {
+        self.inventory_sled_agent.datasets.push(InventoryDataset {
+            id: Some(config.id),
+            name: config.name.full_name(),
+            available: ByteCount::from_gibibytes_u32(1),
+            used: ByteCount::from_gibibytes_u32(0),
+            quota: config.quota,
+            reservation: config.reservation,
+            compression: config.compression.to_string(),
+        });
+    }
+
     fn sp_state(&self) -> Option<&(u16, SpState)> {
         self.inventory_sp.as_ref()
     }