From dd92511ce63f878098e834564e729260d7eb8fd6 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 20 Jul 2026 12:05:20 -0400 Subject: [PATCH] sim-sled-agent: internally store a unified `OmicronSledConfig` This is several yaks deep from a problem I was poking at: this PR addresses a few TODOs inside sim-sled-agent where it was still using / exposing the legacy triple of separate disks / datasets / zones configs. As of this PR it uses `OmicronSledConfig` internally, although for now it still exposes separate disk/dataset configs (to avoid too much churn in one change). --- .../execution/src/omicron_sled_config.rs | 51 +---- .../tasks/support_bundle_collector.rs | 12 +- sled-agent/src/sim/sled_agent.rs | 100 +++------- sled-agent/src/sim/storage.rs | 185 +++++++++--------- 4 files changed, 127 insertions(+), 221 deletions(-) diff --git a/nexus/reconfigurator/execution/src/omicron_sled_config.rs b/nexus/reconfigurator/execution/src/omicron_sled_config.rs index 8e032904dcb..7dec1d494b5 100644 --- a/nexus/reconfigurator/execution/src/omicron_sled_config.rs +++ b/nexus/reconfigurator/execution/src/omicron_sled_config.rs @@ -102,15 +102,12 @@ mod tests { use omicron_common::api::external::Generation; use omicron_common::api::internal::shared::DatasetKind; use omicron_common::disk::CompressionAlgorithm; - use omicron_common::disk::DatasetsConfig; use omicron_common::disk::DiskIdentity; - use omicron_common::disk::OmicronPhysicalDisksConfig; use omicron_common::zpool_name::ZpoolName; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::ZpoolUuid; - use sled_agent_types::inventory::OmicronZonesConfig; use sled_agent_types::inventory::SledRole; use std::net::Ipv6Addr; use std::net::SocketAddr; @@ -133,8 +130,9 @@ mod tests { _ => panic!("Unexpected address type for sled agent (wanted IPv6)"), }; let sim_sled_agent = &cptestctx.sled_agents[0].sled_agent(); - let sim_sled_agent_config_generation = - sim_sled_agent.omicron_zones_list().generation; + let sim_sled_agent_config_generation = sim_sled_agent + .omicron_sled_config() + .map_or(Generation::new(), |config| config.generation); let sleds_by_id = id_ord_map! { Sled::new( @@ -284,48 +282,9 @@ mod tests { // Observe the latest configuration stored on the simulated sled agent, // and verify that this output matches the input. - // - // TODO-cleanup Simulated sled-agent should report a unified - // `OmicronSledConfig`. - let observed_disks = - sim_sled_agent.omicron_physical_disks_list().unwrap(); - let observed_datasets = sim_sled_agent.datasets_config_list().unwrap(); - let observed_zones = sim_sled_agent.omicron_zones_list(); - + let observed_config = sim_sled_agent.omicron_sled_config().unwrap(); let in_service_config = sled_config.clone().into_in_service_sled_config(); - assert_eq!( - observed_disks, - OmicronPhysicalDisksConfig { - generation: in_service_config.generation, - disks: in_service_config.disks.into_iter().collect(), - } - ); - assert_eq!( - observed_datasets, - DatasetsConfig { - generation: in_service_config.generation, - datasets: in_service_config - .datasets - .into_iter() - .map(|d| (d.id, d)) - .collect(), - } - ); - assert_eq!( - observed_zones, - OmicronZonesConfig { - generation: in_service_config.generation, - zones: in_service_config.zones.into_iter().collect(), - } - ); - - // We expect to see each single in-service item we supplied as input. - assert_eq!(observed_disks.disks.len(), 1); - assert_eq!(observed_disks.disks[0].id, disk_id); - assert_eq!(observed_datasets.datasets.len(), 1); - assert!(observed_datasets.datasets.contains_key(&dataset_id)); - assert_eq!(observed_zones.zones.len(), 1); - assert_eq!(observed_zones.zones[0].id, zone_id); + assert_eq!(observed_config, in_service_config); } } diff --git a/nexus/src/app/background/tasks/support_bundle_collector.rs b/nexus/src/app/background/tasks/support_bundle_collector.rs index 2926bcd40bd..e1c37499b20 100644 --- a/nexus/src/app/background/tasks/support_bundle_collector.rs +++ b/nexus/src/app/background/tasks/support_bundle_collector.rs @@ -726,6 +726,7 @@ 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; @@ -1060,11 +1061,12 @@ mod test { }) .collect(); - // Read current sled config generation from zones (this will change - // slightly once the simulator knows how to keep the unified config - // and be a little less weird) - let current_generation = - cptestctx.first_sled_agent().omicron_zones_list().generation; + // Read current config generation (this will change slightly once + // the simulator stops exposing operations on just datasets) + let current_generation = cptestctx + .first_sled_agent() + .omicron_sled_config() + .map_or(Generation::new(), |config| config.generation); let dataset_config = DatasetsConfig { generation: current_generation.next(), diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index c96cc335b1b..064ba2361fd 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/sled-agent/src/sim/sled_agent.rs @@ -35,7 +35,7 @@ use omicron_common::api::internal::shared::{ }; use omicron_common::disk::{ DatasetsConfig, DatasetsManagementResult, DiskIdentity, DiskVariant, - DisksManagementResult, OmicronPhysicalDisksConfig, + OmicronPhysicalDisksConfig, }; use omicron_uuid_kinds::{ DatasetUuid, GenericUuid, PhysicalDiskUuid, PropolisUuid, SledUuid, @@ -63,10 +63,9 @@ use sled_agent_types::instance::{ }; use sled_agent_types::inventory::{ ConfigReconcilerInventory, ConfigReconcilerInventoryResult, - ConfigReconcilerInventoryStatus, FmdInventory, HostPhase2DesiredSlots, - Inventory, InventoryDataset, InventoryDisk, InventoryZpool, - OmicronFileSourceResolverInventory, OmicronSledConfig, OmicronZonesConfig, - SingleMeasurementInventory, SledRole, ZpoolHealth, + ConfigReconcilerInventoryStatus, FmdInventory, Inventory, InventoryDataset, + InventoryDisk, InventoryZpool, OmicronFileSourceResolverInventory, + OmicronSledConfig, SingleMeasurementInventory, SledRole, ZpoolHealth, }; use sled_agent_types::support_bundle::SupportBundleMetadata; use sled_agent_types::system_networking::SystemNetworkingConfig; @@ -112,7 +111,6 @@ pub struct SledAgent { Mutex>>, pub vpc_routes: Mutex>, config: Config, - fake_zones: Mutex, instance_ensure_state_error: Mutex>, /// Number of remaining local storage operation failures to inject. /// When > 0, local storage ensure/delete operations decrement this @@ -202,10 +200,6 @@ impl SledAgent { vpc_routes: Mutex::new(HashMap::new()), mock_propolis: futures::lock::Mutex::new(None), config: config.clone(), - fake_zones: Mutex::new(OmicronZonesConfig { - generation: Generation::new(), - zones: vec![], - }), instance_ensure_state_error: Mutex::new(None), local_storage_error_count: AtomicU32::new(0), repo_depot, @@ -893,24 +887,10 @@ impl SledAgent { let storage = self.storage.lock(); - let disks_config = - storage.omicron_physical_disks_list().unwrap_or_default(); - let datasets_config = - storage.datasets_config_list().unwrap_or_default(); - let zones_config = self.fake_zones.lock().unwrap().clone(); + let maybe_sled_config = storage.omicron_sled_config(); let smf_services_enabled_not_online = self.health_monitor.to_inventory(); - let sled_config = OmicronSledConfig { - generation: zones_config.generation, - disks: disks_config.disks.into_iter().collect(), - datasets: datasets_config.datasets.into_values().collect(), - zones: zones_config.zones.into_iter().collect(), - remove_mupdate_override: None, - host_phase_2: HostPhase2DesiredSlots::current_contents(), - measurements: Default::default(), - }; - let reference_measurements = vec![ SingleMeasurementInventory { path: "this/is/fake1".into(), @@ -971,14 +951,14 @@ impl SledAgent { // to lie here, but this information should be taken with a // particularly careful grain-of-salt -- it's supposed to // represent the "real" datasets the sled agent can observe. - datasets: storage - .datasets_config_list() - .map(|config| { - config + datasets: maybe_sled_config.as_ref().map_or( + Vec::new(), + |sled_config| { + sled_config .datasets - .into_iter() - .map(|(id, config)| InventoryDataset { - id: Some(id), + .iter() + .map(|config| InventoryDataset { + id: Some(config.id), name: config.name.full_name(), available: ByteCount::from_kibibytes_u32(0), used: ByteCount::from_kibibytes_u32(0), @@ -986,17 +966,16 @@ impl SledAgent { reservation: config.inner.reservation, compression: config.inner.compression.to_string(), }) - .collect::>() - }) - .unwrap_or_else(|_| vec![]), - ledgered_sled_config: Some(sled_config.clone()), + .collect() + }, + ), + ledgered_sled_config: maybe_sled_config.clone(), reconciler_status: ConfigReconcilerInventoryStatus::Idle { completed_at: Utc::now() - Duration::from_secs(10), ran_for: Duration::from_secs(3), }, - last_reconciliation: Some( - ConfigReconcilerInventory::debug_assume_success(sled_config), - ), + last_reconciliation: maybe_sled_config + .map(ConfigReconcilerInventory::debug_assume_success), // TODO: simulate the file source resolver with greater fidelity file_source_resolver: OmicronFileSourceResolverInventory::new_fake( ), @@ -1120,47 +1099,20 @@ impl SledAgent { self.storage.lock().omicron_physical_disks_list() } - pub fn omicron_physical_disks_ensure( - &self, - config: OmicronPhysicalDisksConfig, - ) -> Result { - self.storage.lock().omicron_physical_disks_ensure(config) - } - pub fn set_omicron_config( &self, config: OmicronSledConfig, ) -> Result<(), HttpError> { - // TODO Update the simulator to work on `OmicronSledConfig` instead of - // the three separate legacy configs - let disks_config = OmicronPhysicalDisksConfig { - generation: config.generation, - disks: config.disks.into_iter().collect(), - }; - let datasets_config = DatasetsConfig { - generation: config.generation, - datasets: config.datasets.into_iter().map(|d| (d.id, d)).collect(), - }; - let zones_config = OmicronZonesConfig { - generation: config.generation, - zones: config.zones.into_iter().collect(), - }; - - let mut storage = self.storage.lock(); - let _ = storage.omicron_physical_disks_ensure(disks_config)?; - let _ = storage.datasets_ensure(datasets_config)?; - *self.fake_zones.lock().unwrap() = zones_config; - //*self.sled_config.lock().unwrap() = Some(config); - - Ok(()) - } - - pub fn omicron_zones_list(&self) -> OmicronZonesConfig { - self.fake_zones.lock().unwrap().clone() + // It's a little weird that `self.storage` holds the full + // `OmicronSledConfig` (including zones); this is largely an artifact of + // history (when sim-sled-agent was originally written, we had separate + // zone / storage configs; after unifying them, it was simplest to keep + // the full config inside the sim storage system). + self.storage.lock().set_omicron_config(config) } - pub fn omicron_zones_ensure(&self, requested_zones: OmicronZonesConfig) { - *self.fake_zones.lock().unwrap() = requested_zones; + pub fn omicron_sled_config(&self) -> Option { + self.storage.lock().omicron_sled_config() } pub fn drop_dataset(&self, zpool_id: ZpoolUuid, dataset_id: DatasetUuid) { diff --git a/sled-agent/src/sim/storage.rs b/sled-agent/src/sim/storage.rs index 07c37570b4e..f6f7515f0e2 100644 --- a/sled-agent/src/sim/storage.rs +++ b/sled-agent/src/sim/storage.rs @@ -29,9 +29,7 @@ use omicron_common::disk::DatasetName; use omicron_common::disk::DatasetsConfig; use omicron_common::disk::DatasetsManagementResult; use omicron_common::disk::DiskIdentity; -use omicron_common::disk::DiskManagementStatus; use omicron_common::disk::DiskVariant; -use omicron_common::disk::DisksManagementResult; use omicron_common::disk::OmicronPhysicalDisksConfig; use omicron_common::disk::SharedDatasetConfig; use omicron_uuid_kinds::DatasetUuid; @@ -43,6 +41,7 @@ use omicron_uuid_kinds::ZpoolUuid; use propolis_client::VolumeConstructionRequest; use serde::Serialize; use sled_agent_types::dataset::LocalStorageDatasetEnsureRequest; +use sled_agent_types::inventory::OmicronSledConfig; use sled_agent_types::inventory::ZpoolHealth; use sled_agent_types::support_bundle::NESTED_DATASET_NOT_FOUND; use sled_storage::nested_dataset::NestedDatasetConfig; @@ -1350,8 +1349,7 @@ pub(crate) struct StorageInner { log: Logger, sled_id: Uuid, root: Utf8TempDir, - config: Option, - dataset_config: Option, + sled_config: Option, nested_datasets: HashMap, physical_disks: HashMap, next_disk_slot: i64, @@ -1372,8 +1370,7 @@ impl StorageInner { sled_id, log, root: camino_tempfile::tempdir().unwrap(), - config: None, - dataset_config: None, + sled_config: None, nested_datasets: HashMap::new(), physical_disks: HashMap::new(), next_disk_slot: 0, @@ -1394,34 +1391,98 @@ impl StorageInner { &self.physical_disks } + /// Sets the currently-ledgered sled config. + pub fn set_omicron_config( + &mut self, + config: OmicronSledConfig, + ) -> Result<(), HttpError> { + if let Some(stored_config) = self.sled_config.as_ref() { + if stored_config.generation > config.generation { + return Err(HttpError::for_client_error( + None, + dropshot::ClientErrorStatusCode::BAD_REQUEST, + "Generation number too old".to_string(), + )); + } else if stored_config.generation == config.generation { + if *stored_config != config { + return Err(HttpError::for_client_error( + None, + dropshot::ClientErrorStatusCode::BAD_REQUEST, + "Generation number unchanged but data is different" + .to_string(), + )); + } + // An identical config at the same generation is a no-op. + return Ok(()); + } + } + + // Add a "nested dataset" entry for all datasets that should exist, + // and remove it for all datasets that have been removed. + let dataset_names: HashSet<_> = + config.datasets.iter().map(|config| config.name.clone()).collect(); + for dataset in &dataset_names { + // Datasets delegated to zones manage their own storage. + if dataset.kind().zoned() { + continue; + } + + let root = self.root().to_path_buf(); + self.nested_datasets.entry(dataset.clone()).or_insert_with(|| { + NestedDatasetStorage::new( + &root, + dataset.clone(), + String::new(), + SharedDatasetConfig::default(), + ) + }); + } + self.nested_datasets + .retain(|dataset, _| dataset_names.contains(dataset)); + + self.sled_config = Some(config); + Ok(()) + } + + pub fn omicron_sled_config(&self) -> Option { + self.sled_config.clone() + } + pub fn datasets_config_list(&self) -> Result { - let Some(config) = self.dataset_config.as_ref() else { + let Some(config) = self.sled_config.as_ref() else { return Err(HttpError::for_not_found( None, "No control plane datasets".into(), )); }; - Ok(config.clone()) + Ok(DatasetsConfig { + generation: config.generation, + datasets: config + .datasets + .iter() + .map(|dataset| (dataset.id, dataset.clone())) + .collect(), + }) } pub fn dataset_get( &self, dataset_name: &String, ) -> Result { - let Some(config) = self.dataset_config.as_ref() else { + let Some(config) = self.sled_config.as_ref() else { return Err(HttpError::for_not_found( None, "No control plane datasets".into(), )); }; - for (id, dataset) in config.datasets.iter() { + for dataset in config.datasets.iter() { if dataset.name.full_name().as_str() == dataset_name { return Ok(DatasetProperties { - id: Some(*id), + id: Some(dataset.id), name: dataset_name.to_string(), // We should have an entry in `self.nested_datasets` for - // every entry in `config.datasets` (`datasets_ensure()` + // every entry in `config.datasets` (`set_omicron_config()` // keeps these in sync), but we only keeping track of a // `mounted` property on nested datasets. Look that up here. mounted: self @@ -1461,62 +1522,27 @@ 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 { - if let Some(stored_config) = self.dataset_config.as_ref() { - if stored_config.generation > config.generation { - return Err(HttpError::for_client_error( - None, - dropshot::ClientErrorStatusCode::BAD_REQUEST, - "Generation number too old".to_string(), - )); - } else if stored_config.generation == config.generation - && *stored_config != config - { - return Err(HttpError::for_client_error( - None, - dropshot::ClientErrorStatusCode::BAD_REQUEST, - "Generation number unchanged but data is different" - .to_string(), - )); - } - } - self.dataset_config.replace(config.clone()); - - // Add a "nested dataset" entry for all datasets that should exist, - // and remove it for all datasets that have been removed. - let dataset_names: HashSet<_> = config - .datasets - .values() - .map(|config| config.name.clone()) - .collect(); - for dataset in &dataset_names { - // Datasets delegated to zones manage their own storage. - if dataset.kind().zoned() { - continue; - } - - let root = self.root().to_path_buf(); - self.nested_datasets.entry(dataset.clone()).or_insert_with(|| { - NestedDatasetStorage::new( - &root, - dataset.clone(), - String::new(), - SharedDatasetConfig::default(), - ) - }); - } - self.nested_datasets - .retain(|dataset, _| dataset_names.contains(&dataset)); + 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 - .values() + .into_values() .map(|config| DatasetManagementStatus { - dataset_name: config.name.clone(), + dataset_name: config.name, err: None, }) .collect(), @@ -1728,48 +1754,15 @@ impl StorageInner { pub fn omicron_physical_disks_list( &self, ) -> Result { - let Some(config) = self.config.as_ref() else { + let Some(config) = self.sled_config.as_ref() else { return Err(HttpError::for_not_found( None, "No control plane disks".into(), )); }; - Ok(config.clone()) - } - - pub fn omicron_physical_disks_ensure( - &mut self, - config: OmicronPhysicalDisksConfig, - ) -> Result { - if let Some(stored_config) = self.config.as_ref() { - if stored_config.generation > config.generation { - return Err(HttpError::for_client_error( - None, - dropshot::ClientErrorStatusCode::BAD_REQUEST, - "Generation number too old".to_string(), - )); - } else if stored_config.generation == config.generation - && *stored_config != config - { - return Err(HttpError::for_client_error( - None, - dropshot::ClientErrorStatusCode::BAD_REQUEST, - "Generation number unchanged but data is different" - .to_string(), - )); - } - } - self.config.replace(config.clone()); - - Ok(DisksManagementResult { - status: config - .disks - .into_iter() - .map(|config| DiskManagementStatus { - identity: config.identity, - err: None, - }) - .collect(), + Ok(OmicronPhysicalDisksConfig { + generation: config.generation, + disks: config.disks.iter().cloned().collect(), }) }