sim-sled-agent: remove datasets_ensure() (PR 3/4)

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

@@ -50,7 +50,6 @@ progenitor::generate_api!(
         CommitStatus = trust_quorum_types::status::CommitStatus,
         CoordinatorStatus = trust_quorum_types::status::CoordinatorStatus,
         DatasetsConfig = omicron_common::disk::DatasetsConfig,
-        DatasetManagementStatus = omicron_common::disk::DatasetManagementStatus,
         DatasetKind = omicron_common::api::internal::shared::DatasetKind,
         DiskIdentity = omicron_common::disk::DiskIdentity,
         DiskManagementStatus = omicron_common::disk::DiskManagementStatus,

common/src/disk.rs

@@ -459,34 +459,6 @@ impl Ledgerable for DatasetsConfig {
     fn generation_bump(&mut self) {}
 }
 
-/// Identifies how a single dataset management operation may have succeeded or
-/// failed.
-#[derive(Clone, Debug, JsonSchema, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-pub struct DatasetManagementStatus {
-    pub dataset_name: DatasetName,
-    pub err: Option<String>,
-}
-
-/// The result from attempting to manage datasets.
-#[derive(Default, Debug, JsonSchema, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-#[must_use = "this `DatasetManagementResult` may contain errors, which should be handled"]
-pub struct DatasetsManagementResult {
-    pub status: Vec<DatasetManagementStatus>,
-}
-
-impl DatasetsManagementResult {
-    pub fn has_error(&self) -> bool {
-        for status in &self.status {
-            if status.err.is_some() {
-                return true;
-            }
-        }
-        false
-    }
-}
-
 /// Uniquely identifies a disk.
 #[derive(
     Debug,

nexus/src/app/background/tasks/support_bundle_collector.rs

@@ -726,11 +726,9 @@ mod test {
     use nexus_types::inventory::SpType;
     use nexus_types::support_bundle::BundleDataSelection;
     use omicron_common::api::external::ByteCount;
-    use omicron_common::api::external::Generation;
     use omicron_common::api::internal::shared::DatasetKind;
     use omicron_common::disk::DatasetConfig;
     use omicron_common::disk::DatasetName;
-    use omicron_common::disk::DatasetsConfig;
     use omicron_common::disk::SharedDatasetConfig;
     use omicron_common::zpool_name::ZpoolName;
     use omicron_uuid_kinds::GenericUuid;
@@ -1042,42 +1040,27 @@ mod test {
                 disks.push(Self { zpool_id, dataset_id })
             }
 
-            // Create a configuration for the sled agent consisting of all these
+            // Update the configuration of the sled agent to include all these
             // debug datasets.
-            let datasets = disks
-                .iter()
-                .map(|TestDataset { zpool_id, dataset_id }| {
-                    (
-                        *dataset_id,
-                        DatasetConfig {
-                            id: *dataset_id,
-                            name: DatasetName::new(
-                                ZpoolName::new_external(*zpool_id),
-                                DatasetKind::Debug,
-                            ),
-                            inner: SharedDatasetConfig::default(),
-                        },
-                    )
-                })
-                .collect();
-
-            // Read current config generation (this will change slightly once
-            // the simulator stops exposing operations on just datasets)
-            let current_generation = cptestctx
+            let mut config = cptestctx
                 .first_sled_agent()
                 .omicron_sled_config()
-                .map_or(Generation::new(), |config| config.generation);
-
-            let dataset_config = DatasetsConfig {
-                generation: current_generation.next(),
-                datasets,
-            };
-
-            let res = cptestctx
+                .unwrap_or_default();
+            for TestDataset { zpool_id, dataset_id } in &disks {
+                config.datasets.insert_overwrite(DatasetConfig {
+                    id: *dataset_id,
+                    name: DatasetName::new(
+                        ZpoolName::new_external(*zpool_id),
+                        DatasetKind::Debug,
+                    ),
+                    inner: SharedDatasetConfig::default(),
+                });
+            }
+            config.generation = config.generation.next();
+            cptestctx
                 .first_sled_agent()
-                .datasets_ensure(dataset_config)
-                .unwrap();
-            assert!(!res.has_error());
+                .set_omicron_config(config)
+                .expect("updated sled config");
 
             disks
         }

sled-agent/src/sim/sled_agent.rs

@@ -34,8 +34,7 @@ use omicron_common::api::internal::shared::{
     RouterKind, RouterVersion, VirtualNetworkInterfaceHost,
 };
 use omicron_common::disk::{
-    DatasetsConfig, DatasetsManagementResult, DiskIdentity, DiskVariant,
-    OmicronPhysicalDisksConfig,
+    DatasetsConfig, DiskIdentity, DiskVariant, OmicronPhysicalDisksConfig,
 };
 use omicron_uuid_kinds::{
     DatasetUuid, GenericUuid, PhysicalDiskUuid, PropolisUuid, SledUuid,
@@ -1082,13 +1081,6 @@ impl SledAgent {
             .map_err(|err| err.into())
     }
 
-    pub fn datasets_ensure(
-        &self,
-        config: DatasetsConfig,
-    ) -> Result<DatasetsManagementResult, HttpError> {
-        self.storage.lock().datasets_ensure(config)
-    }
-
     pub fn datasets_config_list(&self) -> Result<DatasetsConfig, HttpError> {
         self.storage.lock().datasets_config_list()
     }

sled-agent/src/sim/storage.rs

@@ -24,10 +24,8 @@ use dropshot::HandlerTaskMode;
 use dropshot::HttpError;
 use illumos_utils::zfs::DatasetProperties;
 use omicron_common::api::external::ByteCount;
-use omicron_common::disk::DatasetManagementStatus;
 use omicron_common::disk::DatasetName;
 use omicron_common::disk::DatasetsConfig;
-use omicron_common::disk::DatasetsManagementResult;
 use omicron_common::disk::DiskIdentity;
 use omicron_common::disk::DiskVariant;
 use omicron_common::disk::OmicronPhysicalDisksConfig;
@@ -472,13 +470,22 @@ impl CrucibleDataInner {
 #[cfg(test)]
 mod test {
     use super::*;
-    use omicron_common::api::external::Generation;
     use omicron_common::disk::DatasetConfig;
     use omicron_common::disk::DatasetKind;
     use omicron_common::disk::DatasetName;
     use omicron_common::zpool_name::ZpoolName;
     use omicron_test_utils::dev::test_setup_log;
 
+    fn append_dataset_to_config(
+        storage: &mut StorageInner,
+        dataset: DatasetConfig,
+    ) {
+        let mut config = storage.omicron_sled_config().unwrap_or_default();
+        config.datasets.insert_overwrite(dataset);
+        config.generation = config.generation.next();
+        storage.set_omicron_config(config).expect("set new config");
+    }
+
     /// Validate that the simulated Crucible agent reuses ports when regions are
     /// deleted.
     #[test]
@@ -786,21 +793,15 @@ mod test {
         let dataset_id = DatasetUuid::new_v4();
         let dataset_name = DatasetName::new(zpool_name, DatasetKind::Debug);
 
-        let config = DatasetsConfig {
-            generation: Generation::new(),
-            datasets: BTreeMap::from([(
-                dataset_id,
-                DatasetConfig {
-                    id: dataset_id,
-                    name: dataset_name.clone(),
-                    inner: SharedDatasetConfig::default(),
-                },
-            )]),
-        };
-
         // Create the debug dataset on which we'll store everything else.
-        let result = storage.datasets_ensure(config).unwrap();
-        assert!(!result.has_error());
+        append_dataset_to_config(
+            &mut storage,
+            DatasetConfig {
+                id: dataset_id,
+                name: dataset_name.clone(),
+                inner: SharedDatasetConfig::default(),
+            },
+        );
 
         // The list of nested datasets should only contain the root dataset.
         let nested_datasets = storage
@@ -895,21 +896,16 @@ mod test {
         let dataset_id = DatasetUuid::new_v4();
         let dataset_name = DatasetName::new(zpool_name, DatasetKind::Debug);
 
-        let config = DatasetsConfig {
-            generation: Generation::new(),
-            datasets: BTreeMap::from([(
-                dataset_id,
-                DatasetConfig {
-                    id: dataset_id,
-                    name: dataset_name.clone(),
-                    inner: SharedDatasetConfig::default(),
-                },
-            )]),
-        };
-
         // Create the debug dataset on which we'll store everything else.
-        let result = storage.datasets_ensure(config).unwrap();
-        assert!(!result.has_error());
+        append_dataset_to_config(
+            &mut storage,
+            DatasetConfig {
+                id: dataset_id,
+                name: dataset_name.clone(),
+                inner: SharedDatasetConfig::default(),
+            },
+        );
+
         let nested_dataset_root = NestedDatasetLocation {
             path: String::new(),
             root: dataset_name.clone(),
@@ -1522,33 +1518,6 @@ impl StorageInner {
         return Err(HttpError::for_not_found(None, "Dataset not found".into()));
     }
 
-    /// Splats a legacy `DatasetsConfig` onto the `OmicronSledConfig` held by
-    /// this sim sled-agent, preserving any ledgered disks and zones.
-    ///
-    /// This is a stepping stone for tests that still manage datasets
-    /// separately from the rest of the sled config; new callers should build
-    /// a full `OmicronSledConfig` and use `set_omicron_config()` instead.
-    pub fn datasets_ensure(
-        &mut self,
-        config: DatasetsConfig,
-    ) -> Result<DatasetsManagementResult, HttpError> {
-        let mut sled_config = self.sled_config.clone().unwrap_or_default();
-        sled_config.generation = config.generation;
-        sled_config.datasets = config.datasets.values().cloned().collect();
-        self.set_omicron_config(sled_config)?;
-
-        Ok(DatasetsManagementResult {
-            status: config
-                .datasets
-                .into_values()
-                .map(|config| DatasetManagementStatus {
-                    dataset_name: config.name,
-                    err: None,
-                })
-                .collect(),
-        })
-    }
-
     pub fn nested_dataset_list(
         &self,
         name: NestedDatasetLocation,

sled-agent/src/support_bundle/storage.rs

@@ -1110,16 +1110,15 @@ mod tests {
     use hyper::header::{
         ACCEPT_RANGES, CONTENT_LENGTH, CONTENT_RANGE, CONTENT_TYPE,
     };
+    use iddqd::id_ord_map;
     use omicron_common::disk::DatasetConfig;
     use omicron_common::disk::DatasetKind;
     use omicron_common::disk::DatasetName;
-    use omicron_common::disk::DatasetsConfig;
     use omicron_common::zpool_name::ZpoolName;
     use omicron_test_utils::dev::test_setup_log;
     use omicron_uuid_kinds::PhysicalDiskUuid;
     use sha2::Sha256;
     use sled_agent_types::inventory::ZpoolHealth;
-    use std::collections::BTreeMap;
     use uuid::Uuid;
     use zip::ZipWriter;
     use zip::write::SimpleFileOptions;
@@ -1155,25 +1154,20 @@ mod tests {
             dataset_id: DatasetUuid,
             kind: DatasetKind,
         ) {
-            let result = self
-                .storage_test_harness
-                .lock()
-                .datasets_ensure(DatasetsConfig {
-                    datasets: BTreeMap::from([(
-                        dataset_id,
-                        DatasetConfig {
-                            id: dataset_id,
-                            name: DatasetName::new(
-                                ZpoolName::new_external(self.zpool_id),
-                                kind,
-                            ),
-                            inner: Default::default(),
-                        },
-                    )]),
-                    ..Default::default()
-                })
-                .expect("Failed to ensure datasets");
-            assert!(!result.has_error(), "{result:?}");
+            let mut storage = self.storage_test_harness.lock();
+            let mut config = storage.omicron_sled_config().unwrap_or_default();
+            config.generation = config.generation.next();
+            config.datasets = id_ord_map! {
+                DatasetConfig {
+                    id: dataset_id,
+                    name: DatasetName::new(
+                        ZpoolName::new_external(self.zpool_id),
+                        kind,
+                    ),
+                    inner: Default::default(),
+                },
+            };
+            storage.set_omicron_config(config).expect("set config");
         }
 
         fn is_nested_dataset_mounted(