diff --git a/Cargo.lock b/Cargo.lock index f742c5c3da6..f24bf42faee 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12038,11 +12038,13 @@ name = "sled-agent-types" version = "0.1.0" dependencies = [ "anyhow", + "assert_matches", "async-trait", "bootstore", "camino", "camino-tempfile", "chrono", + "futures", "iddqd", "nexus-sled-agent-shared", "omicron-common", @@ -12051,17 +12053,22 @@ dependencies = [ "omicron-workspace-hack", "oxnet", "propolis-client 0.1.0 (git+https://github.com/oxidecomputer/propolis?rev=e3988414bd68ecf806078fb898120e1194451ee9)", + "proptest", "rcgen", "schemars", "serde", "serde_human_bytes", "serde_json", "sha3", + "sled-hardware", "sled-hardware-types", + "sled-storage", "slog", "slog-error-chain", "strum", + "test-strategy", "thiserror 2.0.12", + "tokio", "toml 0.8.23", "tufaceous-artifact", "uuid", @@ -12086,7 +12093,6 @@ dependencies = [ "serde", "serde_json", "sha2", - "sled-agent-config-reconciler", "sled-agent-types", "sled-agent-zone-images-examples", "sled-storage", @@ -12197,6 +12203,7 @@ dependencies = [ "expectorate", "futures", "glob", + "iddqd", "illumos-utils", "key-manager", "omicron-common", diff --git a/sled-agent/config-reconciler/Cargo.toml b/sled-agent/config-reconciler/Cargo.toml index 6e9dfa1dc43..1715ef65cb3 100644 --- a/sled-agent/config-reconciler/Cargo.toml +++ b/sled-agent/config-reconciler/Cargo.toml @@ -50,6 +50,7 @@ proptest.workspace = true schemars.workspace = true scopeguard.workspace = true serde_json.workspace = true +sled-agent-types = { workspace = true, features = ["testing"] } sled-storage = { workspace = true, features = ["testing"] } test-strategy.workspace = true xshell.workspace = true diff --git a/sled-agent/config-reconciler/src/handle.rs b/sled-agent/config-reconciler/src/handle.rs index f412cd2fc62..4b0565bcedd 100644 --- a/sled-agent/config-reconciler/src/handle.rs +++ b/sled-agent/config-reconciler/src/handle.rs @@ -13,6 +13,10 @@ use nexus_sled_agent_shared::inventory::InventoryZpool; use nexus_sled_agent_shared::inventory::OmicronSledConfig; use omicron_common::disk::DatasetName; use sled_agent_api::ArtifactConfig; +use sled_agent_types::internal_disks::InternalDisksReceiver; +use sled_agent_types::raw_disks; +use sled_agent_types::raw_disks::RawDisksReceiver; +use sled_agent_types::raw_disks::RawDisksSender; use sled_storage::config::MountConfig; use sled_storage::disk::Disk; use sled_storage::manager::NestedDatasetConfig; @@ -51,12 +55,8 @@ use crate::TimeSyncStatus; use crate::dataset_serialization_task::DatasetTaskHandle; use crate::dataset_serialization_task::NestedDatasetMountError; use crate::dump_setup_task; -use crate::internal_disks::InternalDisksReceiver; use crate::ledger::CurrentSledConfig; use crate::ledger::LedgerTaskHandle; -use crate::raw_disks; -use crate::raw_disks::RawDisksReceiver; -use crate::raw_disks::RawDisksSender; use crate::reconciler_task; use crate::reconciler_task::CurrentlyManagedZpools; use crate::reconciler_task::CurrentlyManagedZpoolsReceiver; diff --git a/sled-agent/config-reconciler/src/lib.rs b/sled-agent/config-reconciler/src/lib.rs index 24a3e7b4489..b21ca7a3431 100644 --- a/sled-agent/config-reconciler/src/lib.rs +++ b/sled-agent/config-reconciler/src/lib.rs @@ -15,16 +15,17 @@ //! will spawn several tokio tasks that run for the duration of the sled-agent //! process: //! -//! * A task for managing internal disks (in the `internal_disks` module of this -//! crate). This task takes raw disks as input over a watch channel, and emits -//! managed internal disks as output on another watch channel. Sled-agent -//! components that care about internal disks can get access to this output -//! channel via [`ConfigReconcilerHandle::internal_disks_rx()`]. On Gimlet and -//! Cosmo, "internal" disks have the M.2 form factor; much existing code will -//! refer to them as `m2` disks, but the form factor may change in future -//! sleds. The term "internal disks" is for "the disks that are not easily -//! swappable and can be managed before trust quorum has unlocked"; we use -//! them to store ledgers, etc. +//! * A task for managing internal disks (implemented in the `sled-agent-types` +//! crate to avoid cyclic crate dependencies). This task takes raw disks as +//! input over a watch channel, and emits managed internal disks as output on +//! another watch channel. Sled-agent components that care about internal +//! disks can get access to this output channel via +//! [`ConfigReconcilerHandle::internal_disks_rx()`]. On Gimlet and Cosmo, +//! "internal" disks have the M.2 form factor; much existing code will refer +//! to them as `m2` disks, but the form factor may change in future sleds. The +//! term "internal disks" is for "the disks that are not easily swappable and +//! can be managed before trust quorum has unlocked"; we use them to store +//! ledgers, etc. //! * A task for serializing ZFS operations on datasets (in the //! `dataset_serialization_task` module of this crate). This task takes //! requests over an `mpsc` channel. This channel is not exposed directly; @@ -53,9 +54,7 @@ mod dataset_serialization_task; mod disks_common; mod dump_setup_task; mod handle; -mod internal_disks; mod ledger; -mod raw_disks; mod reconciler_task; mod sled_agent_facilities; @@ -74,16 +73,19 @@ pub use handle::ConfigReconcilerSpawnToken; pub use handle::InventoryError; pub use handle::ReconcilerInventory; pub use handle::TimeSyncConfig; -pub use internal_disks::InternalDisks; -pub use internal_disks::InternalDisksReceiver; -pub use internal_disks::InternalDisksWithBootDisk; pub use ledger::LedgerArtifactConfigError; pub use ledger::LedgerNewConfigError; pub use ledger::LedgerTaskError; -pub use raw_disks::RawDisksSender; pub use reconciler_task::CurrentlyManagedZpools; pub use reconciler_task::CurrentlyManagedZpoolsReceiver; pub use reconciler_task::TimeSyncError; pub use reconciler_task::TimeSyncStatus; pub use sled_agent_facilities::SledAgentArtifactStore; pub use sled_agent_facilities::SledAgentFacilities; + +// Re-export types that have moved to sled-agent-types purely for crate +// dependency graph reasons. +pub use sled_agent_types::internal_disks::InternalDisks; +pub use sled_agent_types::internal_disks::InternalDisksReceiver; +pub use sled_agent_types::internal_disks::InternalDisksWithBootDisk; +pub use sled_agent_types::raw_disks::RawDisksSender; diff --git a/sled-agent/config-reconciler/src/reconciler_task.rs b/sled-agent/config-reconciler/src/reconciler_task.rs index e7900dbb167..07fe8e5b884 100644 --- a/sled-agent/config-reconciler/src/reconciler_task.rs +++ b/sled-agent/config-reconciler/src/reconciler_task.rs @@ -21,6 +21,7 @@ use omicron_common::disk::DatasetKind; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::PhysicalDiskUuid; +use sled_agent_types::raw_disks::RawDisksReceiver; use sled_storage::config::MountConfig; use sled_storage::dataset::U2_DEBUG_DATASET; use sled_storage::dataset::ZONE_DATASET; @@ -39,7 +40,6 @@ use tokio::sync::watch; use crate::TimeSyncConfig; use crate::dataset_serialization_task::DatasetTaskHandle; use crate::ledger::CurrentSledConfig; -use crate::raw_disks::RawDisksReceiver; use crate::sled_agent_facilities::SledAgentFacilities; mod datasets; diff --git a/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs b/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs index a8400a9550e..c17ec2c512a 100644 --- a/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs +++ b/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs @@ -10,6 +10,7 @@ use futures::future; use id_map::IdMap; use id_map::IdMappable; +use iddqd::IdOrdMap; use illumos_utils::zpool::Zpool; use illumos_utils::zpool::ZpoolName; use key_manager::StorageKeyRequester; @@ -38,7 +39,6 @@ use tokio::sync::watch; use crate::disks_common::MaybeUpdatedDisk; use crate::disks_common::update_properties_from_raw_disk; -use crate::raw_disks::RawDiskWithId; /// Set of currently managed zpools. /// @@ -316,7 +316,7 @@ impl ExternalDisks { /// set. pub(super) fn stop_managing_if_needed( &mut self, - raw_disks: &IdMap, + raw_disks: &IdOrdMap, config: &IdMap, log: &Logger, ) { @@ -373,7 +373,7 @@ impl ExternalDisks { /// already managing. pub(super) async fn start_managing_if_needed( &mut self, - raw_disks: &IdMap, + raw_disks: &IdOrdMap, config: &IdMap, key_requester: &StorageKeyRequester, log: &Logger, @@ -389,7 +389,7 @@ impl ExternalDisks { async fn start_managing_if_needed_with_disk_adopter( &mut self, - raw_disks: &IdMap, + raw_disks: &IdOrdMap, config: &IdMap, log: &Logger, disk_adopter: &T, @@ -705,7 +705,7 @@ mod tests { }) } - fn make_raw_test_disk(variant: DiskVariant, serial: &str) -> RawDiskWithId { + fn make_raw_test_disk(variant: DiskVariant, serial: &str) -> RawDisk { RawDisk::Real(UnparsedDisk::new( "/test-devfs".into(), None, @@ -719,7 +719,6 @@ mod tests { false, DiskFirmware::new(0, None, false, 1, vec![]), )) - .into() } fn with_test_runtime(fut: Fut) -> T @@ -770,7 +769,7 @@ mod tests { }) } - async fn internal_disks_are_rejected_impl(raw_disks: IdMap) { + async fn internal_disks_are_rejected_impl(raw_disks: IdOrdMap) { let logctx = dev::test_setup_log("internal_disks_are_rejected"); let (currently_managed_zpools_tx, _rx) = watch::channel(Arc::default()); @@ -853,7 +852,7 @@ mod tests { // Report errors for any requested disks that don't exist. #[proptest] fn fail_if_disk_not_present(disks: BTreeMap) { - let mut raw_disks = IdMap::default(); + let mut raw_disks = IdOrdMap::default(); let mut config_disks = IdMap::default(); let mut not_present = BTreeSet::new(); @@ -865,7 +864,7 @@ mod tests { pool_id: ZpoolUuid::new_v4(), }; if is_present { - raw_disks.insert(raw_disk); + raw_disks.insert_overwrite(raw_disk); } else { not_present.insert(config_disk.id); } @@ -879,7 +878,7 @@ mod tests { } async fn fail_if_disk_not_present_impl( - raw_disks: IdMap, + raw_disks: IdOrdMap, config_disks: IdMap, not_present: BTreeSet, ) { @@ -937,7 +936,7 @@ mod tests { // Stop managing disks if so requested. #[proptest] fn firmware_updates_are_propagated(disks: BTreeMap) { - let mut raw_disks = IdMap::default(); + let mut raw_disks = IdOrdMap::default(); let mut config_disks = IdMap::default(); let mut should_mutate_firmware = BTreeSet::new(); @@ -951,7 +950,7 @@ mod tests { if should_mutate { should_mutate_firmware.insert(raw_disk.identity().clone()); } - raw_disks.insert(raw_disk); + raw_disks.insert_overwrite(raw_disk); config_disks.insert(config_disk); } @@ -966,7 +965,7 @@ mod tests { } async fn firmware_updates_are_propagated_impl( - mut raw_disks: IdMap, + mut raw_disks: IdOrdMap, config_disks: IdMap, should_mutate_firmware: BTreeSet, ) { @@ -1011,16 +1010,13 @@ mod tests { // Change the firmware on some subset of disks. for id in should_mutate_firmware { let mut entry = raw_disks.get_mut(&id).unwrap(); - let mut raw_disk = RawDisk::from(entry.clone()); - let new_firmware = DiskFirmware::new( - raw_disk.firmware().active_slot().wrapping_add(1), + *entry.firmware_mut() = DiskFirmware::new( + entry.firmware().active_slot().wrapping_add(1), None, false, 1, Vec::new(), ); - *raw_disk.firmware_mut() = new_firmware; - *entry = raw_disk.into(); } // Attempt to adopt all the config disks again; we should pick up the @@ -1059,7 +1055,7 @@ mod tests { // `ExternalDiskState`. #[proptest] fn remove_disks_not_in_config(disks: BTreeMap) { - let mut raw_disks = IdMap::default(); + let mut raw_disks = IdOrdMap::default(); let mut config_disks = IdMap::default(); let mut should_remove_after_adding = BTreeSet::new(); @@ -1073,7 +1069,7 @@ mod tests { if should_remove { should_remove_after_adding.insert(config_disk.id); } - raw_disks.insert(raw_disk); + raw_disks.insert_overwrite(raw_disk); config_disks.insert(config_disk); } @@ -1088,7 +1084,7 @@ mod tests { } async fn remove_disks_not_in_config_impl( - raw_disks: IdMap, + raw_disks: IdOrdMap, mut config_disks: IdMap, should_remove_after_adding: BTreeSet, ) { diff --git a/sled-agent/types/Cargo.toml b/sled-agent/types/Cargo.toml index a61a14508f8..b5a4899a3f3 100644 --- a/sled-agent/types/Cargo.toml +++ b/sled-agent/types/Cargo.toml @@ -13,6 +13,7 @@ async-trait.workspace = true bootstore.workspace = true camino.workspace = true chrono.workspace = true +futures.workspace = true iddqd.workspace = true nexus-sled-agent-shared.workspace = true # Note: we're trying to avoid a dependency from sled-agent-types to nexus-types @@ -28,16 +29,26 @@ serde.workspace = true serde_human_bytes.workspace = true serde_json.workspace = true sha3.workspace = true +sled-hardware.workspace = true sled-hardware-types.workspace = true +sled-storage.workspace = true slog.workspace = true slog-error-chain.workspace = true strum.workspace = true thiserror.workspace = true +tokio.workspace = true toml.workspace = true tufaceous-artifact.workspace = true uuid.workspace = true [dev-dependencies] +assert_matches.workspace = true camino-tempfile.workspace = true omicron-test-utils.workspace = true +proptest.workspace = true rcgen.workspace = true +sled-storage = { workspace = true, features = ["testing"] } +test-strategy.workspace = true + +[features] +testing = [] diff --git a/sled-agent/config-reconciler/src/internal_disks.rs b/sled-agent/types/src/internal_disks.rs similarity index 89% rename from sled-agent/config-reconciler/src/internal_disks.rs rename to sled-agent/types/src/internal_disks.rs index 14866d86844..4132280fcf8 100644 --- a/sled-agent/config-reconciler/src/internal_disks.rs +++ b/sled-agent/types/src/internal_disks.rs @@ -13,8 +13,9 @@ use camino::Utf8PathBuf; use core::cmp; use futures::future; use futures::future::Either; -use id_map::IdMap; -use id_map::IdMappable; +use iddqd::IdOrdItem; +use iddqd::IdOrdMap; +use iddqd::id_upcast; use omicron_common::backoff::Backoff as _; use omicron_common::backoff::ExponentialBackoffBuilder; use omicron_common::disk::DiskIdentity; @@ -46,10 +47,9 @@ use std::time::Duration; use tokio::sync::watch; use tokio::sync::watch::error::RecvError; -use crate::disks_common::MaybeUpdatedDisk; -use crate::disks_common::update_properties_from_raw_disk; -use crate::raw_disks::RawDiskWithId; +use crate::raw_disks::MaybeUpdatedDisk; use crate::raw_disks::RawDisksReceiver; +use crate::raw_disks::update_properties_from_raw_disk; /// A thin wrapper around a [`watch::Receiver`] that presents a similar API. #[derive(Debug, Clone)] @@ -61,11 +61,11 @@ pub struct InternalDisksReceiver { #[derive(Debug, Clone)] enum InternalDisksReceiverInner { - Real(watch::Receiver>), + Real(watch::Receiver>), #[cfg(any(test, feature = "testing"))] - FakeStatic(Arc>), + FakeStatic(Arc>), #[cfg(any(test, feature = "testing"))] - FakeDynamic(watch::Receiver>>), + FakeDynamic(watch::Receiver>>), } impl InternalDisksReceiver { @@ -156,7 +156,7 @@ impl InternalDisksReceiver { Self { mount_config, inner, errors_rx } } - pub(crate) fn spawn_internal_disks_task( + pub fn spawn_internal_disks_task( mount_config: Arc, raw_disks_rx: RawDisksReceiver, base_log: &Logger, @@ -169,7 +169,7 @@ impl InternalDisksReceiver { ) } - pub(crate) fn mount_config(&self) -> &Arc { + pub fn mount_config(&self) -> &Arc { &self.mount_config } @@ -179,7 +179,7 @@ impl InternalDisksReceiver { base_log: &Logger, disk_adopter: T, ) -> Self { - let (disks_tx, disks_rx) = watch::channel(IdMap::default()); + let (disks_tx, disks_rx) = watch::channel(IdOrdMap::default()); let (errors_tx, errors_rx) = watch::channel(Arc::default()); tokio::spawn( @@ -257,14 +257,18 @@ impl InternalDisksReceiver { /// Get the raw set of `Disk`s. /// /// This operation will panic if this receiver is backed by a fake set of - /// disk properties (e.g., as created by `Self::fake_static()`). It is also - /// only exposed to this crate; external callers should only operate on the - /// view provided by `InternalDisks`. Internal-to-this-crate callers should - /// take care not to hold the returned reference too long (as this keeps the - /// watch channel locked). - pub(crate) fn borrow_and_update_raw_disks( + /// disk properties (e.g., as created by `Self::fake_static()`). + /// + /// This method is very low-level and should only be used by + /// sled-agent-config-reconciler's inner workings. Other sled-agent + /// consumers should not use this method and should prefer operating on the + /// `InternalDisks` returned by `current()`. + /// + /// Callers should take care not to hold the returned reference too long (as + /// this keeps the watch channel locked). + pub fn borrow_and_update_raw_disks( &mut self, - ) -> watch::Ref<'_, IdMap> { + ) -> watch::Ref<'_, IdOrdMap> { match &mut self.inner { InternalDisksReceiverInner::Real(rx) => rx.borrow_and_update(), #[cfg(any(test, feature = "testing"))] @@ -292,9 +296,7 @@ impl InternalDisksReceiver { /// Wait until the boot disk is managed, returning its identity. /// /// Internally updates the most-recently-seen value. - pub(crate) async fn wait_for_boot_disk( - &mut self, - ) -> InternalDisksWithBootDisk { + pub async fn wait_for_boot_disk(&mut self) -> InternalDisksWithBootDisk { loop { let current = self.current_and_update(); if let Some(with_boot_disk) = @@ -311,13 +313,13 @@ impl InternalDisksReceiver { /// Note that this error set is not atomically collected with the /// `current()` set of disks. It is only useful for inventory reporting /// purposes. - pub(crate) fn errors(&self) -> Arc> { + pub fn errors(&self) -> Arc> { Arc::clone(&*self.errors_rx.borrow()) } } pub struct InternalDisks { - disks: Arc>, + disks: Arc>, mount_config: Arc, } @@ -411,27 +413,30 @@ impl InternalDisks { /// An [`InternalDisks`] with a guaranteed-present boot disk. pub struct InternalDisksWithBootDisk { inner: InternalDisks, - boot_disk: InternalDiskDetailsId, + boot_disk: DiskIdentity, } impl InternalDisksWithBootDisk { fn new(inner: InternalDisks) -> Option { - let boot_disk = inner - .disks - .iter() - .find_map(|d| if d.is_boot_disk() { Some(d.id()) } else { None })?; + let boot_disk = inner.disks.iter().find_map(|d| { + if d.is_boot_disk() { Some(d.identity.clone()) } else { None } + })?; Some(Self { inner, boot_disk }) } fn boot_disk(&self) -> &InternalDiskDetails { - match self.inner.disks.get(&self.boot_disk) { + let key = InternalDiskDetailsId { + is_boot_disk: true, + identity: &self.boot_disk, + }; + match self.inner.disks.get(&key) { Some(details) => details, None => unreachable!("boot disk present by construction"), } } - pub fn boot_disk_id(&self) -> &Arc { - &self.boot_disk().id.identity + pub fn boot_disk_id(&self) -> &DiskIdentity { + &self.boot_disk().identity } pub fn boot_disk_zpool_name(&self) -> ZpoolName { @@ -452,16 +457,18 @@ impl InternalDisksWithBootDisk { pub fn non_boot_disk_install_datasets( &self, ) -> impl Iterator + '_ { - self.inner.disks.iter().filter(|disk| disk.id != self.boot_disk).map( - |disk| { + self.inner + .disks + .iter() + .filter(|disk| disk.identity != self.boot_disk) + .map(|disk| { let dataset = ZpoolName::Internal(disk.zpool_id) .dataset_mountpoint( &self.inner.mount_config.root, INSTALL_DATASET, ); (disk.zpool_id, dataset) - }, - ) + }) } } @@ -470,7 +477,8 @@ impl InternalDisksWithBootDisk { // easier faking for tests. #[derive(Debug, Clone)] struct InternalDiskDetails { - id: InternalDiskDetailsId, + identity: DiskIdentity, + is_boot_disk: bool, zpool_id: InternalZpoolUuid, // These two fields are optional because they don't exist for synthetic @@ -479,22 +487,25 @@ struct InternalDiskDetails { raw_devfs_path: Option>>, } -impl IdMappable for InternalDiskDetails { - type Id = InternalDiskDetailsId; +impl IdOrdItem for InternalDiskDetails { + type Key<'a> = InternalDiskDetailsId<'a>; - fn id(&self) -> Self::Id { - self.id.clone() + fn key(&self) -> Self::Key<'_> { + InternalDiskDetailsId { + is_boot_disk: self.is_boot_disk, + identity: &self.identity, + } } + + id_upcast!(); } -impl From<&'_ Disk> for InternalDiskDetails { - fn from(disk: &'_ Disk) -> Self { +impl From<&'_ InternalDisk> for InternalDiskDetails { + fn from(disk: &'_ InternalDisk) -> Self { let zpool_id = match disk.zpool_name() { ZpoolName::Internal(id) => *id, - ZpoolName::External(id) => { - panic!( - "InternalDiskDetails::from called for external disk ({id:?})" - ); + ZpoolName::External(_) => { + panic!("InternalDisk has external ZpoolName: {disk:?}"); } }; // Synthetic disks panic if asked for their `slot()`, so filter @@ -513,10 +524,8 @@ impl From<&'_ Disk> for InternalDiskDetails { }; Self { - id: InternalDiskDetailsId { - identity: Arc::new(disk.identity().clone()), - is_boot_disk: disk.is_boot_disk(), - }, + identity: disk.identity().clone(), + is_boot_disk: disk.is_boot_disk(), zpool_id, slot, raw_devfs_path, @@ -524,12 +533,6 @@ impl From<&'_ Disk> for InternalDiskDetails { } } -impl From<&'_ InternalDisk> for InternalDiskDetails { - fn from(disk: &'_ InternalDisk) -> Self { - Self::from(&disk.disk) - } -} - impl InternalDiskDetails { #[cfg(any(test, feature = "testing"))] fn fake_details( @@ -538,10 +541,8 @@ impl InternalDiskDetails { is_boot_disk: bool, ) -> Self { Self { - id: InternalDiskDetailsId { - identity: Arc::new(identity), - is_boot_disk, - }, + identity, + is_boot_disk, zpool_id, // We can expand the interface for fake disks if we need to be able // to specify more of these properties in future tests. @@ -555,7 +556,7 @@ impl InternalDiskDetails { } fn is_boot_disk(&self) -> bool { - self.id.is_boot_disk + self.is_boot_disk } } @@ -566,12 +567,12 @@ impl InternalDiskDetails { // boot disk if they're performing a "check the first disk that succeeds" kind // of operation. #[derive(Debug, Clone, PartialEq, Eq)] -struct InternalDiskDetailsId { +struct InternalDiskDetailsId<'a> { is_boot_disk: bool, - identity: Arc, + identity: &'a DiskIdentity, } -impl cmp::Ord for InternalDiskDetailsId { +impl cmp::Ord for InternalDiskDetailsId<'_> { fn cmp(&self, other: &Self) -> cmp::Ordering { match self.is_boot_disk.cmp(&other.is_boot_disk) { cmp::Ordering::Equal => {} @@ -583,45 +584,32 @@ impl cmp::Ord for InternalDiskDetailsId { } } -impl cmp::PartialOrd for InternalDiskDetailsId { +impl cmp::PartialOrd for InternalDiskDetailsId<'_> { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } -// Wrapper around a `Disk` with a cheaply-cloneable identity. +// Wrapper around a `Disk` that ensures statically it is an internal disk. #[derive(Debug, Clone)] -pub(crate) struct InternalDisk { - identity: Arc, - disk: Disk, -} - -impl From for InternalDisk { - fn from(disk: Disk) -> Self { - // Invariant: We should only be constructed for internal disks; our - // caller should have already filtered out all external disks. - match disk.variant() { - DiskVariant::M2 => (), - DiskVariant::U2 => panic!("InternalDisk called with external Disk"), - } - Self { identity: Arc::new(disk.identity().clone()), disk } - } -} +pub struct InternalDisk(Disk); impl Deref for InternalDisk { type Target = Disk; fn deref(&self) -> &Self::Target { - &self.disk + &self.0 } } -impl IdMappable for InternalDisk { - type Id = Arc; +impl IdOrdItem for InternalDisk { + type Key<'a> = &'a DiskIdentity; - fn id(&self) -> Self::Id { - Arc::clone(&self.identity) + fn key(&self) -> Self::Key<'_> { + self.0.identity() } + + id_upcast!(); } struct InternalDisksTask { @@ -629,7 +617,7 @@ struct InternalDisksTask { raw_disks_rx: RawDisksReceiver, // The set of disks we've successfully started managing. - disks_tx: watch::Sender>, + disks_tx: watch::Sender>, // Output channel summarizing any adoption errors. // @@ -681,7 +669,7 @@ impl InternalDisksTask { DiskVariant::M2 => true, }) .cloned() - .collect::>(); + .collect::>(); // Perform actual reconciliation, updating our output watch // channels. @@ -736,7 +724,7 @@ impl InternalDisksTask { async fn reconcile_internal_disks( &mut self, - internal_raw_disks: IdMap, + internal_raw_disks: IdOrdMap, disk_adopter: &T, ) { info!( @@ -747,8 +735,13 @@ impl InternalDisksTask { // We don't want to hold the `disks_tx` lock while we adopt disks, so // first capture a snapshot of which disks we have. - let current_disks = - self.disks_tx.borrow().keys().cloned().collect::>(); + let current_disks = self + .disks_tx + .borrow() + .iter() + .map(|disk| disk.identity()) + .cloned() + .collect::>(); // Built the list of disks that are gone. let mut disks_to_remove = Vec::new(); @@ -779,7 +772,7 @@ impl InternalDisksTask { // This is a new disk: attempt to adopt it. let identity = raw_disk.identity().clone(); let adopt_result = disk_adopter - .adopt_disk(raw_disk.into(), &self.mount_config, &self.log) + .adopt_disk(raw_disk, &self.mount_config, &self.log) .await; match adopt_result { @@ -811,7 +804,7 @@ impl InternalDisksTask { changed = true; } for new_disk in disks_to_insert { - disks.insert(new_disk.into()); + disks.insert_overwrite(InternalDisk(new_disk)); changed = true; } @@ -824,12 +817,12 @@ impl InternalDisksTask { .get(raw_disk.identity()) .expect("disk should still be present"); match update_properties_from_raw_disk( - &existing_disk.disk, + &existing_disk.0, &raw_disk, &self.log, ) { MaybeUpdatedDisk::Updated(new_disk) => { - disks.insert(InternalDisk::from(new_disk)); + disks.insert_overwrite(InternalDisk(new_disk)); changed = true; } MaybeUpdatedDisk::Unchanged => (), @@ -906,29 +899,21 @@ mod tests { serial: String, } - impl From for InternalDiskDetailsId { - fn from(id: ArbitraryInternalDiskDetailsId) -> Self { - InternalDiskDetailsId { - identity: Arc::new(DiskIdentity { - vendor: id.vendor, - model: id.model, - serial: id.serial, - }), - is_boot_disk: id.is_boot_disk, - } - } - } - #[proptest] fn boot_disks_sort_ahead_of_non_boot_disks_in_id_map( #[any(size_range(2..4).lift())] values: Vec< ArbitraryInternalDiskDetailsId, >, ) { - let disk_map: IdMap<_> = values + let disk_map: IdOrdMap<_> = values .into_iter() .map(|id| InternalDiskDetails { - id: id.into(), + identity: DiskIdentity { + vendor: id.vendor, + model: id.model, + serial: id.serial, + }, + is_boot_disk: id.is_boot_disk, zpool_id: InternalZpoolUuid::new_v4(), slot: None, raw_devfs_path: None, @@ -1044,7 +1029,7 @@ mod tests { new_raw_test_disk(DiskVariant::M2, "m2-1"), new_raw_test_disk(DiskVariant::U2, "u2-1"), ] { - disks.insert(disk.into()); + disks.insert_overwrite(disk); } }); @@ -1060,7 +1045,7 @@ mod tests { let adopted_disks = Arc::clone(&disks_rx.current_and_update().disks); let serials = adopted_disks .iter() - .map(|d| d.id.identity.serial.as_str()) + .map(|d| d.identity.serial.as_str()) .collect::>(); let expected_serials = ["m2-0", "m2-1"].into_iter().collect::>(); @@ -1084,9 +1069,8 @@ mod tests { // Setup: one disk. let mut raw_disk = new_raw_test_disk(DiskVariant::M2, "test-m2"); - let (raw_disks_tx, raw_disks_rx) = watch::channel(Arc::new( - [raw_disk.clone().into()].into_iter().collect(), - )); + let (raw_disks_tx, raw_disks_rx) = + watch::channel(Arc::new([raw_disk.clone()].into_iter().collect())); let disk_adopter = Arc::new(TestDiskAdopter::default()); let mut disks_rx = InternalDisksReceiver::spawn_with_disk_adopter( Arc::new(any_mount_config()), @@ -1112,7 +1096,7 @@ mod tests { ); *raw_disk.firmware_mut() = new_firmware; raw_disks_tx.send_modify(|disks| { - Arc::make_mut(disks).insert(raw_disk.clone().into()); + Arc::make_mut(disks).insert_overwrite(raw_disk.clone()); }); // Wait for the change to be noticed. @@ -1143,11 +1127,7 @@ mod tests { let raw_disk1 = new_raw_test_disk(DiskVariant::M2, "m2-1"); let raw_disk2 = new_raw_test_disk(DiskVariant::M2, "m2-2"); let (raw_disks_tx, raw_disks_rx) = watch::channel(Arc::new( - [&raw_disk1, &raw_disk2] - .into_iter() - .cloned() - .map(From::from) - .collect(), + [&raw_disk1, &raw_disk2].into_iter().cloned().collect(), )); let disk_adopter = Arc::new(TestDiskAdopter::default()); let mut disks_rx = InternalDisksReceiver::spawn_with_disk_adopter( @@ -1177,7 +1157,7 @@ mod tests { let adopted_disks = Arc::clone(&disks_rx.current_and_update().disks); let serials = adopted_disks .iter() - .map(|d| d.id.identity.serial.as_str()) + .map(|d| d.identity.serial.as_str()) .collect::>(); let expected_serials = ["m2-2"].into_iter().collect::>(); assert_eq!(serials, expected_serials); @@ -1192,7 +1172,7 @@ mod tests { // Setup: one disk, and configure the disk adopter to fail. let raw_disk = new_raw_test_disk(DiskVariant::M2, "test-m2"); let (_raw_disks_tx, raw_disks_rx) = watch::channel(Arc::new( - [&raw_disk].into_iter().cloned().map(From::from).collect(), + [&raw_disk].into_iter().cloned().collect(), )); let disk_adopter = Arc::new(TestDiskAdopter::default()); disk_adopter.inner.lock().unwrap().should_fail_requests.insert( diff --git a/sled-agent/types/src/lib.rs b/sled-agent/types/src/lib.rs index 172bbed8a39..b910ee81df1 100644 --- a/sled-agent/types/src/lib.rs +++ b/sled-agent/types/src/lib.rs @@ -10,8 +10,10 @@ pub mod disk; pub mod early_networking; pub mod firewall_rules; pub mod instance; +pub mod internal_disks; pub mod rack_init; pub mod rack_ops; +pub mod raw_disks; pub mod sled; pub mod support_bundle; pub mod time_sync; diff --git a/sled-agent/config-reconciler/src/raw_disks.rs b/sled-agent/types/src/raw_disks.rs similarity index 85% rename from sled-agent/config-reconciler/src/raw_disks.rs rename to sled-agent/types/src/raw_disks.rs index 17479da0f08..e637d885ea0 100644 --- a/sled-agent/config-reconciler/src/raw_disks.rs +++ b/sled-agent/types/src/raw_disks.rs @@ -5,30 +5,71 @@ //! Provides thin wrappers around a watch channel managing the set of //! [`RawDisk`]s sled-agent is aware of. -use id_map::IdMap; -use id_map::IdMappable; +use iddqd::IdOrdMap; use nexus_sled_agent_shared::inventory::InventoryDisk; use omicron_common::disk::DiskIdentity; +use sled_storage::disk::Disk; use sled_storage::disk::RawDisk; use slog::Logger; use slog::info; +use slog::warn; use std::ops::Deref; use std::ops::DerefMut; use std::sync::Arc; use tokio::sync::watch; -pub(crate) fn new() -> (RawDisksSender, RawDisksReceiver) { +#[derive(Debug)] +// `Disk` is ~200 bytes, but the callers wants ownership of them, so it'd be +// annoying to box them. Suppress the clippy lint about the variants having +// large size differences. +#[allow(clippy::large_enum_variant)] +pub enum MaybeUpdatedDisk { + Unchanged, + Updated(Disk), +} + +pub fn update_properties_from_raw_disk( + disk: &Disk, + raw_disk: &RawDisk, + log: &Logger, +) -> MaybeUpdatedDisk { + if *raw_disk == RawDisk::from(disk.clone()) { + return MaybeUpdatedDisk::Unchanged; + } + + // The only property we expect to change is the firmware metadata. Update + // that and check again; if they're still not equal, something weird is + // going on. At least log a warning. + let mut disk = disk.clone(); + disk.update_firmware_metadata(raw_disk); + if *raw_disk == RawDisk::from(disk.clone()) { + info!( + log, "Updated disk firmware metadata"; + "firmware" => ?disk.firmware(), + "identity" => ?disk.identity(), + ); + } else { + warn!( + log, + "Updated disk firmware metadata from raw disk properties, \ + but other properties are different!"; + "disk" => ?disk, + "raw_disk" => ?*raw_disk, + ); + } + MaybeUpdatedDisk::Updated(disk) +} + +pub fn new() -> (RawDisksSender, RawDisksReceiver) { let (tx, rx) = watch::channel(Arc::default()); (RawDisksSender(tx), RawDisksReceiver(rx)) } #[derive(Debug, Clone)] -pub(crate) struct RawDisksReceiver( - pub(crate) watch::Receiver>>, -); +pub struct RawDisksReceiver(pub(crate) watch::Receiver>>); impl Deref for RawDisksReceiver { - type Target = watch::Receiver>>; + type Target = watch::Receiver>>; fn deref(&self) -> &Self::Target { &self.0 @@ -42,7 +83,7 @@ impl DerefMut for RawDisksReceiver { } #[derive(Debug, Clone)] -pub struct RawDisksSender(watch::Sender>>); +pub struct RawDisksSender(watch::Sender>>); impl RawDisksSender { /// Set the complete set of raw disks visible to sled-agent. @@ -50,17 +91,16 @@ impl RawDisksSender { where I: Iterator, { - let mut new_disks = - raw_disks.map(From::from).collect::>(); + let mut new_disks = raw_disks.collect::>(); self.0.send_if_modified(|disks| { // We can't just set `*disks = new_disks` here because we may have // disks that shouldn't be removed even if they're not present in // `new_disks`; check for that first. for old_disk in disks.iter() { - if !new_disks.contains_key(&old_disk.identity) + if !new_disks.contains_key(old_disk.identity()) && !can_remove_disk(old_disk, log) { - new_disks.insert(old_disk.clone()); + new_disks.insert_overwrite(old_disk.clone()); } } @@ -75,28 +115,27 @@ impl RawDisksSender { /// Add or update the properties of a raw disk visible to sled-agent. pub fn add_or_update_raw_disk(&self, disk: RawDisk, log: &Logger) -> bool { - let disk = RawDiskWithId::from(disk); self.0.send_if_modified(|disks| { - match disks.get(&disk.identity) { + match disks.get(disk.identity()) { Some(existing) if *existing == disk => { return false; } Some(existing) => { info!( log, "Updating raw disk"; - "old" => ?existing.disk, - "new" => ?disk.disk, + "old" => ?existing, + "new" => ?disk, ); } None => { info!( log, "Adding new raw disk"; - "disk" => ?disk.disk, + "disk" => ?disk, ); } } - Arc::make_mut(disks).insert(disk); + Arc::make_mut(disks).insert_overwrite(disk); true }) } @@ -126,7 +165,7 @@ impl RawDisksSender { }) } - pub(crate) fn to_inventory(&self) -> Vec { + pub fn to_inventory(&self) -> Vec { self.0 .borrow() .iter() @@ -149,12 +188,12 @@ impl RawDisksSender { // Synthetic disks added by sled-agent on startup in test/dev environments are // never added again; refuse to remove them. -fn can_remove_disk(disk: &RawDiskWithId, log: &Logger) -> bool { +fn can_remove_disk(disk: &RawDisk, log: &Logger) -> bool { if disk.is_synthetic() { // Synthetic disks are only added once; don't remove them. info!( log, "Not removing synthetic disk"; - "identity" => ?disk.identity, + "identity" => ?disk.identity(), ); false } else { @@ -162,41 +201,6 @@ fn can_remove_disk(disk: &RawDiskWithId, log: &Logger) -> bool { } } -// Adapter to store `RawDisk` in an `IdMap` with cheap key cloning. -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct RawDiskWithId { - identity: Arc, - disk: RawDisk, -} - -impl IdMappable for RawDiskWithId { - type Id = Arc; - - fn id(&self) -> Self::Id { - Arc::clone(&self.identity) - } -} - -impl From for RawDiskWithId { - fn from(disk: RawDisk) -> Self { - Self { identity: Arc::new(disk.identity().clone()), disk } - } -} - -impl From for RawDisk { - fn from(disk: RawDiskWithId) -> Self { - disk.disk - } -} - -impl Deref for RawDiskWithId { - type Target = RawDisk; - - fn deref(&self) -> &Self::Target { - &self.disk - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/sled-agent/zone-images/Cargo.toml b/sled-agent/zone-images/Cargo.toml index a9a09a89d05..19cbd63c2a7 100644 --- a/sled-agent/zone-images/Cargo.toml +++ b/sled-agent/zone-images/Cargo.toml @@ -19,7 +19,6 @@ rayon.workspace = true serde.workspace = true serde_json.workspace = true sha2.workspace = true -sled-agent-config-reconciler.workspace = true sled-agent-types.workspace = true sled-storage.workspace = true slog.workspace = true @@ -32,5 +31,5 @@ dropshot.workspace = true expectorate.workspace = true omicron-uuid-kinds.workspace = true pretty_assertions.workspace = true -sled-agent-config-reconciler = { workspace = true, features = ["testing"] } +sled-agent-types = { workspace = true, features = ["testing"] } sled-agent-zone-images-examples.workspace = true diff --git a/sled-agent/zone-images/src/install_dataset_metadata.rs b/sled-agent/zone-images/src/install_dataset_metadata.rs index 97fedbfbcfc..7f6c7c54062 100644 --- a/sled-agent/zone-images/src/install_dataset_metadata.rs +++ b/sled-agent/zone-images/src/install_dataset_metadata.rs @@ -8,7 +8,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; use omicron_uuid_kinds::InternalZpoolUuid; use serde::de::DeserializeOwned; -use sled_agent_config_reconciler::InternalDisksWithBootDisk; +use sled_agent_types::internal_disks::InternalDisksWithBootDisk; use sled_agent_types::zone_images::{ ArcIoError, ArcSerdeJsonError, InstallMetadataReadError, }; diff --git a/sled-agent/zone-images/src/mupdate_override.rs b/sled-agent/zone-images/src/mupdate_override.rs index cfc565c545a..1af9b04411d 100644 --- a/sled-agent/zone-images/src/mupdate_override.rs +++ b/sled-agent/zone-images/src/mupdate_override.rs @@ -14,7 +14,7 @@ use camino::Utf8PathBuf; use iddqd::IdOrdMap; use omicron_common::update::MupdateOverrideInfo; use omicron_uuid_kinds::InternalZpoolUuid; -use sled_agent_config_reconciler::InternalDisksWithBootDisk; +use sled_agent_types::internal_disks::InternalDisksWithBootDisk; use sled_agent_types::zone_images::MupdateOverrideNonBootInfo; use sled_agent_types::zone_images::MupdateOverrideNonBootMismatch; use sled_agent_types::zone_images::MupdateOverrideNonBootResult; diff --git a/sled-agent/zone-images/src/source_resolver.rs b/sled-agent/zone-images/src/source_resolver.rs index 328973cd0dd..b2894f02867 100644 --- a/sled-agent/zone-images/src/source_resolver.rs +++ b/sled-agent/zone-images/src/source_resolver.rs @@ -12,8 +12,8 @@ use crate::zone_manifest::AllZoneManifests; use camino::Utf8PathBuf; use illumos_utils::running_zone::ZoneImageFileSource; use nexus_sled_agent_shared::inventory::OmicronZoneImageSource; -use sled_agent_config_reconciler::InternalDisks; -use sled_agent_config_reconciler::InternalDisksWithBootDisk; +use sled_agent_types::internal_disks::InternalDisks; +use sled_agent_types::internal_disks::InternalDisksWithBootDisk; use sled_agent_types::zone_images::MupdateOverrideReadError; use sled_agent_types::zone_images::ResolverStatus; use slog::error; diff --git a/sled-agent/zone-images/src/test_utils.rs b/sled-agent/zone-images/src/test_utils.rs index bdfaef11114..cca1cf12327 100644 --- a/sled-agent/zone-images/src/test_utils.rs +++ b/sled-agent/zone-images/src/test_utils.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use camino::Utf8Path; use omicron_common::disk::DiskIdentity; use omicron_uuid_kinds::InternalZpoolUuid; -use sled_agent_config_reconciler::InternalDisksReceiver; +use sled_agent_types::internal_disks::InternalDisksReceiver; use sled_storage::config::MountConfig; pub(crate) fn make_internal_disks_rx( diff --git a/sled-agent/zone-images/src/zone_manifest.rs b/sled-agent/zone-images/src/zone_manifest.rs index 3d0223c0e43..a46881ce7e0 100644 --- a/sled-agent/zone-images/src/zone_manifest.rs +++ b/sled-agent/zone-images/src/zone_manifest.rs @@ -10,7 +10,7 @@ use omicron_common::update::{ use omicron_uuid_kinds::InternalZpoolUuid; use rayon::iter::{ParallelBridge, ParallelIterator}; use sha2::{Digest, Sha256}; -use sled_agent_config_reconciler::InternalDisksWithBootDisk; +use sled_agent_types::internal_disks::InternalDisksWithBootDisk; use sled_agent_types::zone_images::{ ArcIoError, ArtifactReadResult, InstallMetadataReadError, ZoneManifestArtifactResult, ZoneManifestArtifactsResult, diff --git a/sled-storage/Cargo.toml b/sled-storage/Cargo.toml index aa68be45350..1961157f714 100644 --- a/sled-storage/Cargo.toml +++ b/sled-storage/Cargo.toml @@ -16,6 +16,7 @@ debug-ignore.workspace = true derive_more.workspace = true glob.workspace = true futures.workspace = true +iddqd.workspace = true illumos-utils.workspace = true key-manager.workspace = true omicron-common.workspace = true diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index 66569f339b7..75e15a542a8 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -7,6 +7,7 @@ use anyhow::bail; use camino::{Utf8Path, Utf8PathBuf}; use derive_more::From; +use iddqd::{IdOrdItem, id_upcast}; use key_manager::StorageKeyRequester; use omicron_common::disk::{DiskIdentity, DiskVariant}; use omicron_common::zpool_name::{ZpoolKind, ZpoolName}; @@ -253,6 +254,16 @@ impl RawDisk { } } +impl IdOrdItem for RawDisk { + type Key<'a> = &'a DiskIdentity; + + fn key(&self) -> Self::Key<'_> { + self.identity() + } + + id_upcast!(); +} + /// A physical [`PooledDisk`] or a [`SyntheticDisk`] that contains or is backed /// by a single zpool and that has provisioned datasets. This disk is ready for /// usage by higher level software.