From a86740bb15e0515bd37bac92d75818ec48ffe798 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 20 Jul 2026 15:58:40 -0400 Subject: [PATCH] sim-sled-agent: remove datasets_ensure() --- clients/sled-agent-client/src/lib.rs | 1 - common/src/disk.rs | 28 ------ .../tasks/support_bundle_collector.rs | 51 ++++------- sled-agent/src/sim/sled_agent.rs | 10 +-- sled-agent/src/sim/storage.rs | 85 ++++++------------- sled-agent/src/support_bundle/storage.rs | 36 ++++---- 6 files changed, 60 insertions(+), 151 deletions(-) diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index 2ef713cb890..6db8650ea30 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/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, diff --git a/common/src/disk.rs b/common/src/disk.rs index b57f4c1d466..ebb072fa2fd 100644 --- a/common/src/disk.rs +++ b/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, -} - -/// 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, -} - -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, diff --git a/nexus/src/app/background/tasks/support_bundle_collector.rs b/nexus/src/app/background/tasks/support_bundle_collector.rs index e1c37499b20..31d176fbc2d 100644 --- a/nexus/src/app/background/tasks/support_bundle_collector.rs +++ b/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 } diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index 064ba2361fd..363be251708 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/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 { - self.storage.lock().datasets_ensure(config) - } - pub fn datasets_config_list(&self) -> Result { self.storage.lock().datasets_config_list() } diff --git a/sled-agent/src/sim/storage.rs b/sled-agent/src/sim/storage.rs index f6f7515f0e2..92af5dd8608 100644 --- a/sled-agent/src/sim/storage.rs +++ b/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 { - 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, diff --git a/sled-agent/src/support_bundle/storage.rs b/sled-agent/src/support_bundle/storage.rs index cbe90273efb..a06406166ed 100644 --- a/sled-agent/src/support_bundle/storage.rs +++ b/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(