diff --git a/sled-agent/src/storage_manager.rs b/sled-agent/src/storage_manager.rs index 3d3e544573..68fb7df7df 100644 --- a/sled-agent/src/storage_manager.rs +++ b/sled-agent/src/storage_manager.rs @@ -47,97 +47,6 @@ use illumos_utils::{zfs::Zfs, zpool::Zpool}; // boot when the bootstore has detected it has a key share. static KEY_MANAGER_READY: OnceLock<()> = OnceLock::new(); -#[derive(thiserror::Error, Debug)] -pub enum Error { - #[error(transparent)] - DiskError(#[from] sled_hardware::PooledDiskError), - - // TODO: We could add the context of "why are we doint this op", maybe? - #[error(transparent)] - ZfsListDataset(#[from] illumos_utils::zfs::ListDatasetsError), - - #[error(transparent)] - ZfsEnsureFilesystem(#[from] illumos_utils::zfs::EnsureFilesystemError), - - #[error(transparent)] - ZfsSetValue(#[from] illumos_utils::zfs::SetValueError), - - #[error(transparent)] - ZfsGetValue(#[from] illumos_utils::zfs::GetValueError), - - #[error(transparent)] - GetZpoolInfo(#[from] illumos_utils::zpool::GetInfoError), - - #[error(transparent)] - Fstyp(#[from] illumos_utils::fstyp::Error), - - #[error(transparent)] - ZoneCommand(#[from] illumos_utils::running_zone::RunCommandError), - - #[error(transparent)] - ZoneBoot(#[from] illumos_utils::running_zone::BootError), - - #[error(transparent)] - ZoneEnsureAddress(#[from] illumos_utils::running_zone::EnsureAddressError), - - #[error(transparent)] - ZoneInstall(#[from] illumos_utils::running_zone::InstallZoneError), - - #[error("No U.2 Zpools found")] - NoU2Zpool, - - #[error("Failed to parse UUID from {path}: {err}")] - ParseUuid { - path: Utf8PathBuf, - #[source] - err: uuid::Error, - }, - - #[error("Dataset {name:?} exists with a different uuid (has {old}, requested {new})")] - UuidMismatch { name: Box, old: Uuid, new: Uuid }, - - #[error("Error parsing pool {name}'s size: {err}")] - BadPoolSize { - name: String, - #[source] - err: ByteCountRangeError, - }, - - #[error("Failed to parse the dataset {name}'s UUID: {err}")] - ParseDatasetUuid { - name: String, - #[source] - err: uuid::Error, - }, - - #[error("Zpool Not Found: {0}")] - ZpoolNotFound(String), - - #[error("Underlay not yet initialized")] - UnderlayNotInitialized, -} - -/// A ZFS storage pool. -struct Pool { - name: ZpoolName, - info: ZpoolInfo, - parent: DiskIdentity, -} - -impl Pool { - /// Queries for an existing Zpool by name. - /// - /// Returns Ok if the pool exists. - fn new(name: ZpoolName, parent: DiskIdentity) -> Result { - let info = Zpool::get_info(&name.to_string())?; - Ok(Pool { name, info, parent }) - } - - fn parent(&self) -> &DiskIdentity { - &self.parent - } -} - // The type of a future which is used to send a notification to Nexus. type NotifyFut = Pin> + Send>>; @@ -154,179 +63,12 @@ struct UnderlayRequest { responder: oneshot::Sender>, } -#[derive(PartialEq, Eq, Clone)] -pub(crate) enum DiskWrapper { - Real { disk: Disk, devfs_path: Utf8PathBuf }, - Synthetic { zpool_name: ZpoolName }, -} - -impl From for DiskWrapper { - fn from(disk: Disk) -> Self { - let devfs_path = disk.devfs_path().clone(); - Self::Real { disk, devfs_path } - } -} - -impl DiskWrapper { - fn identity(&self) -> DiskIdentity { - match self { - DiskWrapper::Real { disk, .. } => disk.identity().clone(), - DiskWrapper::Synthetic { zpool_name } => { - let id = zpool_name.id(); - DiskIdentity { - vendor: "synthetic-vendor".to_string(), - serial: format!("synthetic-serial-{id}"), - model: "synthetic-model".to_string(), - } - } - } - } - - fn variant(&self) -> DiskVariant { - match self { - DiskWrapper::Real { disk, .. } => disk.variant(), - DiskWrapper::Synthetic { zpool_name } => match zpool_name.kind() { - ZpoolKind::External => DiskVariant::U2, - ZpoolKind::Internal => DiskVariant::M2, - }, - } - } - - fn zpool_name(&self) -> &ZpoolName { - match self { - DiskWrapper::Real { disk, .. } => disk.zpool_name(), - DiskWrapper::Synthetic { zpool_name } => zpool_name, - } - } -} - -#[derive(Clone)] -pub struct StorageResources { - // All disks, real and synthetic, being managed by this sled - disks: Arc>>, - - // A map of "Uuid" to "pool". - pools: Arc>>, -} - // The directory within the debug dataset in which bundles are created. const BUNDLE_DIRECTORY: &str = "bundle"; // The directory for zone bundles. const ZONE_BUNDLE_DIRECTORY: &str = "zone"; -impl StorageResources { - /// Creates a fabricated view of storage resources. - /// - /// Use this only when you want to reference the disks, but not actually - /// access them. Creates one internal and one external disk. - #[cfg(test)] - pub fn new_for_test() -> Self { - let new_disk_identity = || DiskIdentity { - vendor: "vendor".to_string(), - serial: Uuid::new_v4().to_string(), - model: "model".to_string(), - }; - - Self { - disks: Arc::new(Mutex::new(HashMap::from([ - ( - new_disk_identity(), - DiskWrapper::Synthetic { - zpool_name: ZpoolName::new_internal(Uuid::new_v4()), - }, - ), - ( - new_disk_identity(), - DiskWrapper::Synthetic { - zpool_name: ZpoolName::new_external(Uuid::new_v4()), - }, - ), - ]))), - pools: Arc::new(Mutex::new(HashMap::new())), - } - } - - /// Returns the identity of the boot disk. - /// - /// If this returns `None`, we have not processed the boot disk yet. - pub async fn boot_disk(&self) -> Option<(DiskIdentity, ZpoolName)> { - let disks = self.disks.lock().await; - disks.iter().find_map(|(id, disk)| { - match disk { - // This is the "real" use-case: if we have real disks, query - // their properties to identify if they truly are the boot disk. - DiskWrapper::Real { disk, .. } => { - if disk.is_boot_disk() { - return Some((id.clone(), disk.zpool_name().clone())); - } - } - // This is the "less real" use-case: if we have synthetic disks, - // just label the first M.2-looking one as a "boot disk". - DiskWrapper::Synthetic { .. } => { - if matches!(disk.variant(), DiskVariant::M2) { - return Some((id.clone(), disk.zpool_name().clone())); - } - } - }; - None - }) - } - - // TODO: Could be generic over DiskVariant - - /// Returns all M.2 zpools - pub async fn all_m2_zpools(&self) -> Vec { - self.all_zpools(DiskVariant::M2).await - } - - /// Returns all U.2 zpools - pub async fn all_u2_zpools(&self) -> Vec { - self.all_zpools(DiskVariant::U2).await - } - - /// Returns all mountpoints within all M.2s for a particular dataset. - pub async fn all_m2_mountpoints(&self, dataset: &str) -> Vec { - let m2_zpools = self.all_m2_zpools().await; - m2_zpools - .iter() - .map(|zpool| zpool.dataset_mountpoint(dataset)) - .collect() - } - - /// Returns all mountpoints within all U.2s for a particular dataset. - pub async fn all_u2_mountpoints(&self, dataset: &str) -> Vec { - let u2_zpools = self.all_u2_zpools().await; - u2_zpools - .iter() - .map(|zpool| zpool.dataset_mountpoint(dataset)) - .collect() - } - - /// Returns all zpools of a particular variant - pub async fn all_zpools(&self, variant: DiskVariant) -> Vec { - let disks = self.disks.lock().await; - disks - .values() - .filter_map(|disk| { - if disk.variant() == variant { - return Some(disk.zpool_name().clone()); - } - None - }) - .collect() - } - - /// Return the directories for storing zone service bundles. - pub async fn all_zone_bundle_directories(&self) -> Vec { - self.all_m2_mountpoints(sled_hardware::disk::M2_DEBUG_DATASET) - .await - .into_iter() - .map(|p| p.join(BUNDLE_DIRECTORY).join(ZONE_BUNDLE_DIRECTORY)) - .collect() - } -} - /// Describes the access to the underlay used by the StorageManager. pub struct UnderlayAccess { pub nexus_client: NexusClientWithResolver, @@ -1392,15 +1134,3 @@ impl StorageManager { &self.inner.resources } } - -impl Drop for StorageManagerInner { - fn drop(&mut self) { - // NOTE: Ideally, with async drop, we'd await completion of the worker - // somehow. - // - // Without that option, we instead opt to simply cancel the worker - // task to ensure it does not remain alive beyond the StorageManager - // itself. - self.task.abort(); - } -} diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 2a7dcbda9b..f5304262e4 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -34,6 +34,7 @@ enum StorageRequest { AddDisk(UnparsedDisk), AddSyntheticDisk(ZpoolName), RemoveDisk(UnparsedDisk), + RemoveSyntheticDisk(ZpoolName), DisksChanged(HashSet), // NewFilesystem(NewFilesystemRequest), KeyManagerReady, @@ -66,6 +67,12 @@ impl StorageHandle { self.tx.send(StorageRequest::RemoveDisk(disk)).await.unwrap(); } + /// Removes a synthetic disk, if it's tracked by the storage manager, as + /// well as any associated zpools. + pub async fn delete_synthetic_disk(&self, pool: ZpoolName) { + self.tx.send(StorageRequest::RemoveSyntheticDisk(pool)).await.unwrap(); + } + /// Ensures that the storage manager tracks exactly the provided disks. /// /// This acts similar to a batch [Self::upsert_disk] for all new disks, and @@ -209,7 +216,12 @@ impl StorageManager { } } } - StorageRequest::RemoveDisk(_unparsed_disk) => todo!(), + StorageRequest::RemoveDisk(unparsed_disk) => { + self.remove_disk(unparsed_disk).await; + } + StorageRequest::RemoveSyntheticDisk(pool) => { + self.remove_synthetic_disk(pool).await; + } StorageRequest::DisksChanged(_unparsed_disks) => todo!(), StorageRequest::KeyManagerReady => { self.state = StorageManagerState::Normal; @@ -437,6 +449,24 @@ impl StorageManager { } } } + + // Delete a real disk + async fn remove_disk(&mut self, unparsed_disk: UnparsedDisk) { + // If the disk is a U.2, we want to first delete it from any queued disks + let _ = self.queued_u2_drives.remove(&unparsed_disk); + if self.resources.remove_real_disk(unparsed_disk) { + let _ = self.resource_updates.send_replace(self.resources.clone()); + } + } + + // Delete a synthetic disk + async fn remove_synthetic_disk(&mut self, pool: ZpoolName) { + // If the disk is a U.2, we want to first delete it from any queued disks + let _ = self.queued_synthetic_u2_drives.remove(&pool); + if self.resources.remove_synthetic_disk(pool) { + let _ = self.resource_updates.send_replace(self.resources.clone()); + } + } } /// All tests only use synthetic disks, but are expected to be run on illumos @@ -634,7 +664,9 @@ mod tests { /// This allows us to control timing precisely. #[tokio::test] async fn queued_disks_get_requeued_on_secret_retriever_error() { - let logctx = test_setup_log("queued_disks_get_added_as_resources"); + let logctx = test_setup_log( + "queued_disks_get_requeued_on_secret_retriever_error", + ); let inject_error = Arc::new(AtomicBool::new(false)); let (mut key_manager, key_requester) = KeyManager::new( &logctx.log, @@ -681,4 +713,43 @@ mod tests { Zpool::destroy(&zpool_name).unwrap(); logctx.cleanup_successful(); } + + #[tokio::test] + async fn delete_disk_triggers_notification() { + let logctx = test_setup_log("delete_disk_triggers_notification"); + let (mut key_manager, key_requester) = + KeyManager::new(&logctx.log, HardcodedSecretRetriever::default()); + let (mut manager, mut handle) = + StorageManager::new(&logctx.log, key_requester); + + // Spawn the key_manager so that it will respond to requests for encryption keys + tokio::spawn(async move { key_manager.run().await }); + + // Spawn the storage manager as done by sled-agent + tokio::spawn(async move { + manager.run().await; + }); + + // Inform the storage manager that the key manager is ready, so disks + // don't get queued + handle.key_manager_ready().await; + + // Create and add a disk + let zpool_name = ZpoolName::new_external(Uuid::new_v4()); + let dir = tempdir().unwrap(); + let _ = new_disk(dir.path(), &zpool_name); + handle.upsert_synthetic_disk(zpool_name.clone()).await; + + // Wait for the add disk notification + let resources = handle.wait_for_changes().await; + assert_eq!(resources.all_u2_zpools().len(), 1); + + // Delete the disk and wait for a notification + handle.delete_synthetic_disk(zpool_name.clone()).await; + let resources = handle.wait_for_changes().await; + assert!(resources.all_u2_zpools().is_empty()); + + Zpool::destroy(&zpool_name).unwrap(); + logctx.cleanup_successful(); + } } diff --git a/sled-storage/src/resources.rs b/sled-storage/src/resources.rs index fb57d742e3..82c588bd27 100644 --- a/sled-storage/src/resources.rs +++ b/sled-storage/src/resources.rs @@ -12,7 +12,7 @@ use camino::Utf8PathBuf; use illumos_utils::zpool::ZpoolName; use omicron_common::api::external::{ByteCount, ByteCountRangeError}; use omicron_common::disk::DiskIdentity; -use sled_hardware::DiskVariant; +use sled_hardware::{DiskVariant, UnparsedDisk}; use std::collections::BTreeMap; use std::sync::Arc; use uuid::Uuid; @@ -92,6 +92,38 @@ impl StorageResources { Ok(true) } + /// Delete a real disk and its zpool + /// + /// Return true, if data was changed, false otherwise + pub(crate) fn remove_real_disk(&mut self, disk: UnparsedDisk) -> bool { + if !self.disks.contains_key(disk.identity()) { + return false; + } + // Safe to unwrap as we just checked the key existed above + let parsed_disk = + Arc::make_mut(&mut self.disks).remove(disk.identity()).unwrap(); + Arc::make_mut(&mut self.pools).remove(&parsed_disk.zpool_name().id()); + true + } + + /// Delete a synthetic disk and its zpool + /// + /// Return true, if data was changed, false otherwise + pub(crate) fn remove_synthetic_disk( + &mut self, + zpool_name: ZpoolName, + ) -> bool { + let disk = DiskWrapper::Synthetic { zpool_name: zpool_name.clone() }; + if !self.disks.contains_key(&disk.identity()) { + return false; + } + // Safe to unwrap as we just checked the key existed above + let parsed_disk = + Arc::make_mut(&mut self.disks).remove(&disk.identity()).unwrap(); + Arc::make_mut(&mut self.pools).remove(&parsed_disk.zpool_name().id()); + true + } + /// Returns the identity of the boot disk. /// /// If this returns `None`, we have not processed the boot disk yet.