From 6925aedbe7de7db37ac66e03e89bd4430b7fb528 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 8 Apr 2025 12:11:17 -0700 Subject: [PATCH 1/6] [sled-agent] Ensure Support Bundle datasets get mounted --- illumos-utils/src/zfs.rs | 116 +++++++++++++++++----- sled-agent/src/sim/storage.rs | 50 ++++++++++ sled-agent/src/support_bundle/storage.rs | 117 ++++++++++++++++++----- sled-storage/src/manager.rs | 34 ++++++- 4 files changed, 265 insertions(+), 52 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 92da7a208d2..1a8da8d3cdf 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -507,12 +507,22 @@ fn build_zfs_set_key_value_pairs( } /// Describes the ZFS "canmount" options. +#[derive(Copy, Clone, Debug)] pub enum CanMount { On, Off, NoAuto, } +impl CanMount { + fn wants_mounting(&self) -> bool { + match self { + CanMount::On => true, + CanMount::Off | CanMount::NoAuto => false, + } + } +} + /// Arguments to [Zfs::ensure_dataset]. pub struct DatasetEnsureArgs<'a> { /// The full path of the ZFS dataset. @@ -796,6 +806,21 @@ fn is_directory_immutable( return Ok(result); } +struct DatasetMountInfo { + exists: bool, + mounted: bool, +} + +impl DatasetMountInfo { + fn exists(mounted: bool) -> Self { + Self { exists: true, mounted } + } + + fn does_not_exist() -> Self { + Self { exists: false, mounted: false } + } +} + impl Zfs { /// Lists all datasets within a pool or existing dataset. /// @@ -900,6 +925,57 @@ impl Zfs { Ok(()) } + /// Ensures that a ZFS dataset is mounted, if it can be. + /// + /// Pre-requisites to be mounted: + /// - The dataset exists + /// - The mountpoint is valid + /// - `can_mount` is CanMount::On + /// - `zoned` is false + /// + /// Returns "true" if the dataset exists and is mounted + /// Returns "false" if the dataset does not exist + /// + /// Returns an error if the dataset exists, but cannot be mounted. + pub fn ensure_dataset_mounted_if_exists( + name: &str, + mountpoint: &Mountpoint, + can_mount: CanMount, + zoned: bool, + ) -> Result { + let res = Self::ensure_dataset_mounted_if_exists_inner( + name, mountpoint, can_mount, zoned, + ) + .map_err(|err| EnsureDatasetError { name: name.to_string(), err })?; + Ok(res.exists) + } + + fn ensure_dataset_mounted_if_exists_inner( + name: &str, + mountpoint: &Mountpoint, + can_mount: CanMount, + zoned: bool, + ) -> Result { + let DatasetMountInfo { exists, mut mounted } = + Self::dataset_exists(name, mountpoint)?; + if !exists { + return Ok(DatasetMountInfo::does_not_exist()); + } + if !zoned && !mounted && can_mount.wants_mounting() { + if let Mountpoint::Path(path) = &mountpoint { + ensure_empty_immutable_mountpoint(&path).map_err(|err| { + EnsureDatasetErrorRaw::MountpointCreation { + mountpoint: path.to_path_buf(), + err, + } + })?; + Self::mount_dataset(name)?; + mounted = true; + } + } + return Ok(DatasetMountInfo::exists(mounted)); + } + /// Creates a new ZFS dataset unless one already exists. /// /// Refer to [DatasetEnsureArgs] for details on the supplied arguments. @@ -923,29 +999,21 @@ impl Zfs { additional_options, }: DatasetEnsureArgs, ) -> Result<(), EnsureDatasetErrorRaw> { - let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?; + // Determine if the dataset exists, and mount it if necessary. + let exists = Self::ensure_dataset_mounted_if_exists_inner( + name, + &mountpoint, + can_mount, + zoned, + )? + .exists; let props = build_zfs_set_key_value_pairs(size_details, id); if exists { - // If the dataset already exists: Update properties which might - // have changed, and ensure it has been mounted if it needs - // to be mounted. + // If the dataset already exists: Update properties which might have + // changed. Self::set_values(name, props.as_slice()) .map_err(|err| EnsureDatasetErrorRaw::from(err.err))?; - - if !zoned && !mounted { - if let (CanMount::On, Mountpoint::Path(path)) = - (&can_mount, &mountpoint) - { - ensure_empty_immutable_mountpoint(&path).map_err( - |err| EnsureDatasetErrorRaw::MountpointCreation { - mountpoint: path.to_path_buf(), - err, - }, - )?; - Self::mount_dataset(name)?; - } - } return Ok(()); } @@ -959,10 +1027,8 @@ impl Zfs { // // Zoned datasets are mounted when their zones are booted, so // we don't do this mountpoint manipulation for them. - if !zoned { - if let (CanMount::On, Mountpoint::Path(path)) = - (&can_mount, &mountpoint) - { + if !zoned && can_mount.wants_mounting() { + if let Mountpoint::Path(path) = &mountpoint { ensure_empty_immutable_mountpoint(&path).map_err(|err| { EnsureDatasetErrorRaw::MountpointCreation { mountpoint: path.to_path_buf(), @@ -1048,7 +1114,7 @@ impl Zfs { fn dataset_exists( name: &str, mountpoint: &Mountpoint, - ) -> Result<(bool, bool), EnsureDatasetErrorRaw> { + ) -> Result { let mut command = std::process::Command::new(ZFS); let cmd = command.args(&[ "list", @@ -1064,9 +1130,9 @@ impl Zfs { return Err(EnsureDatasetErrorRaw::Output(stdout.to_string())); } let mounted = values[3] == "yes"; - Ok((true, mounted)) + Ok(DatasetMountInfo::exists(mounted)) } else { - Ok((false, false)) + Ok(DatasetMountInfo::does_not_exist()) } } diff --git a/sled-agent/src/sim/storage.rs b/sled-agent/src/sim/storage.rs index c669684814d..a8b1f26384b 100644 --- a/sled-agent/src/sim/storage.rs +++ b/sled-agent/src/sim/storage.rs @@ -22,6 +22,8 @@ use crucible_agent_client::types::{ }; 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; @@ -1362,6 +1364,54 @@ impl StorageInner { Ok(config.clone()) } + pub fn dataset_get( + &self, + dataset_name: &String, + ) -> Result { + let Some(config) = self.dataset_config.as_ref() else { + return Err(HttpError::for_not_found( + None, + "No control plane datasets".into(), + )); + }; + + for (id, dataset) in config.datasets.iter() { + if dataset.name.full_name().as_str() == dataset_name { + return Ok(DatasetProperties { + id: Some(*id), + name: dataset_name.to_string(), + mounted: true, + avail: ByteCount::from_kibibytes_u32(1024), + used: ByteCount::from_kibibytes_u32(1024), + quota: dataset.inner.quota, + reservation: dataset.inner.reservation, + compression: dataset.inner.compression.to_string(), + }); + } + } + + for (nested_dataset_name, nested_dataset_storage) in + self.nested_datasets.iter() + { + if nested_dataset_name.full_name().as_str() == dataset_name { + let config = &nested_dataset_storage.config.inner; + + return Ok(DatasetProperties { + id: None, + name: dataset_name.to_string(), + mounted: true, + avail: ByteCount::from_kibibytes_u32(1024), + used: ByteCount::from_kibibytes_u32(1024), + quota: config.quota, + reservation: config.reservation, + compression: config.compression.to_string(), + }); + } + } + + return Err(HttpError::for_not_found(None, "Dataset not found".into())); + } + pub fn datasets_ensure( &mut self, config: DatasetsConfig, diff --git a/sled-agent/src/support_bundle/storage.rs b/sled-agent/src/support_bundle/storage.rs index 4060bbcaeaf..bcdeb05cdd8 100644 --- a/sled-agent/src/support_bundle/storage.rs +++ b/sled-agent/src/support_bundle/storage.rs @@ -11,9 +11,11 @@ use dropshot::Body; use dropshot::HttpError; use futures::Stream; use futures::StreamExt; +use illumos_utils::zfs::DatasetProperties; use omicron_common::api::external::Error as ExternalError; use omicron_common::disk::CompressionAlgorithm; use omicron_common::disk::DatasetConfig; +use omicron_common::disk::DatasetName; use omicron_common::disk::DatasetsConfig; use omicron_common::disk::SharedDatasetConfig; use omicron_uuid_kinds::DatasetUuid; @@ -64,6 +66,12 @@ pub enum Error { #[error("Dataset not found")] DatasetNotFound, + #[error("Could not look up dataset")] + DatasetLookup(#[source] anyhow::Error), + + #[error("Cannot access dataset {dataset:?} which is not mounted")] + DatasetNotMounted { dataset: DatasetName }, + #[error( "Dataset exists, but has an invalid configuration: (wanted {wanted}, saw {actual})" )] @@ -133,6 +141,12 @@ pub trait LocalStorage: Sync { /// Returns all configured datasets async fn dyn_datasets_config_list(&self) -> Result; + /// Returns properties about a dataset + fn dyn_dataset_get( + &self, + dataset_name: &String, + ) -> Result; + /// Returns all nested datasets within an existing dataset async fn dyn_nested_dataset_list( &self, @@ -166,6 +180,19 @@ impl LocalStorage for StorageHandle { self.datasets_config_list().await.map_err(|err| err.into()) } + // TODO: Should this be a part of "StorageHandle"? + fn dyn_dataset_get( + &self, + dataset_name: &String, + ) -> Result { + Ok(illumos_utils::zfs::Zfs::get_dataset_properties( + &[dataset_name.clone()], + illumos_utils::zfs::WhichDatasets::SelfOnly, + ) + .map_err(|err| Error::DatasetLookup(err))? + .remove(0)) + } + async fn dyn_nested_dataset_list( &self, name: NestedDatasetLocation, @@ -200,6 +227,13 @@ impl LocalStorage for crate::sim::Storage { self.lock().datasets_config_list().map_err(|err| err.into()) } + fn dyn_dataset_get( + &self, + dataset_name: &String, + ) -> Result { + self.lock().dataset_get(dataset_name).map_err(|err| err.into()) + } + async fn dyn_nested_dataset_list( &self, name: NestedDatasetLocation, @@ -378,7 +412,9 @@ impl<'a> SupportBundleManager<'a> { } // Returns a dataset that the sled has been explicitly configured to use. - async fn get_configured_dataset( + // + // Returns an error if this dataset is not mounted. + async fn get_configured_dataset_if_mounted( &self, zpool_id: ZpoolUuid, dataset_id: DatasetUuid, @@ -389,6 +425,14 @@ impl<'a> SupportBundleManager<'a> { .get(&dataset_id) .ok_or_else(|| Error::DatasetNotFound)?; + let dataset_props = + self.storage.dyn_dataset_get(&dataset.name.full_name())?; + if !dataset_props.mounted { + return Err(Error::DatasetNotMounted { + dataset: dataset.name.clone(), + }); + } + if dataset.id != dataset_id { return Err(Error::DatasetExistsBadConfig { wanted: dataset_id, @@ -411,8 +455,10 @@ impl<'a> SupportBundleManager<'a> { zpool_id: ZpoolUuid, dataset_id: DatasetUuid, ) -> Result, Error> { - let root = - self.get_configured_dataset(zpool_id, dataset_id).await?.name; + let root = self + .get_configured_dataset_if_mounted(zpool_id, dataset_id) + .await? + .name; let dataset_location = NestedDatasetLocation { path: String::from(""), root }; let datasets = self @@ -436,7 +482,10 @@ impl<'a> SupportBundleManager<'a> { // The dataset for a support bundle exists. let support_bundle_path = dataset .name - .mountpoint(&self.storage.zpool_mountpoint_root()) + .ensure_mounted_and_get_mountpoint( + &self.storage.zpool_mountpoint_root(), + ) + .await? .join(BUNDLE_FILE_NAME); // Identify whether or not the final "bundle" file exists. @@ -522,28 +571,21 @@ impl<'a> SupportBundleManager<'a> { "bundle_id" => support_bundle_id.to_string(), )); info!(log, "creating support bundle"); - let root = - self.get_configured_dataset(zpool_id, dataset_id).await?.name; + + // Access the parent dataset (presumably "crypt/debug") + // where the support bundled will be mounted. + let root = self + .get_configured_dataset_if_mounted(zpool_id, dataset_id) + .await? + .name; let dataset = NestedDatasetLocation { path: support_bundle_id.to_string(), root }; - // The mounted root of the support bundle dataset - let support_bundle_dir = - dataset.mountpoint(&self.storage.zpool_mountpoint_root()); - let support_bundle_path = support_bundle_dir.join(BUNDLE_FILE_NAME); - let support_bundle_path_tmp = support_bundle_dir.join(format!( - "bundle-{}.tmp", - thread_rng() - .sample_iter(Alphanumeric) - .take(6) - .map(char::from) - .collect::() - )); // Ensure that the dataset exists. info!(log, "Ensuring dataset exists for bundle"); self.storage .dyn_nested_dataset_ensure(NestedDatasetConfig { - name: dataset, + name: dataset.clone(), inner: SharedDatasetConfig { compression: CompressionAlgorithm::On, quota: None, @@ -553,6 +595,22 @@ impl<'a> SupportBundleManager<'a> { .await?; info!(log, "Dataset does exist for bundle"); + // The mounted root of the support bundle dataset + let support_bundle_dir = dataset + .ensure_mounted_and_get_mountpoint( + &self.storage.zpool_mountpoint_root(), + ) + .await?; + let support_bundle_path = support_bundle_dir.join(BUNDLE_FILE_NAME); + let support_bundle_path_tmp = support_bundle_dir.join(format!( + "bundle-{}.tmp", + thread_rng() + .sample_iter(Alphanumeric) + .take(6) + .map(char::from) + .collect::() + )); + // Exit early if the support bundle already exists if tokio::fs::try_exists(&support_bundle_path).await? { if !Self::sha2_checksum_matches( @@ -623,8 +681,10 @@ impl<'a> SupportBundleManager<'a> { "bundle_id" => support_bundle_id.to_string(), )); info!(log, "Destroying support bundle"); - let root = - self.get_configured_dataset(zpool_id, dataset_id).await?.name; + let root = self + .get_configured_dataset_if_mounted(zpool_id, dataset_id) + .await? + .name; self.storage .dyn_nested_dataset_destroy(NestedDatasetLocation { path: support_bundle_id.to_string(), @@ -641,13 +701,20 @@ impl<'a> SupportBundleManager<'a> { dataset_id: DatasetUuid, support_bundle_id: SupportBundleUuid, ) -> Result { - let root = - self.get_configured_dataset(zpool_id, dataset_id).await?.name; + // Access the parent dataset where the support bundle is stored. + let root = self + .get_configured_dataset_if_mounted(zpool_id, dataset_id) + .await? + .name; let dataset = NestedDatasetLocation { path: support_bundle_id.to_string(), root }; + // The mounted root of the support bundle dataset - let support_bundle_dir = - dataset.mountpoint(&self.storage.zpool_mountpoint_root()); + let support_bundle_dir = dataset + .ensure_mounted_and_get_mountpoint( + &self.storage.zpool_mountpoint_root(), + ) + .await?; let path = support_bundle_dir.join(BUNDLE_FILE_NAME); let f = tokio::fs::File::open(&path).await?; diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 37e4747622b..ab331b2edb9 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -136,7 +136,10 @@ pub struct NestedDatasetLocation { } impl NestedDatasetLocation { - pub fn mountpoint(&self, root: &Utf8Path) -> Utf8PathBuf { + /// Returns the desired mountpoint of this dataset. + /// + /// Does not ensure that the dataset is mounted. + pub fn mountpoint(&self, mount_root: &Utf8Path) -> Utf8PathBuf { let mut path = Utf8Path::new(&self.path); // This path must be nested, so we need it to be relative to @@ -152,9 +155,34 @@ impl NestedDatasetLocation { .expect("Path is absolute, but we cannot strip '/' character"); } - self.root.mountpoint(root).join(path) + // mount_root: Usually "/", but can be a tmp dir for tests + // self.root: Parent dataset mountpoint + // path: Path to nested dataset within parent dataset + self.root.mountpoint(mount_root).join(path) + } + + /// Access the mountpoint of this nested dataset, and ensure it's mounted. + /// + /// If it is not mounted, or cannot be mounted, return an error. + pub async fn ensure_mounted_and_get_mountpoint( + &self, + mount_root: &Utf8Path, + ) -> Result { + let mountpoint = self.mountpoint(mount_root); + let zoned = false; + Zfs::ensure_dataset_mounted_if_exists( + &self.full_name(), + &Mountpoint::Path(mountpoint.clone()), + CanMount::On, + zoned, + )?; + + return Ok(mountpoint); } + /// Returns the full name of the nested dataset. + /// + /// This is a combination of the parent and child dataset names. pub fn full_name(&self) -> String { if self.path.is_empty() { self.root.full_name().to_string() @@ -1209,6 +1237,8 @@ impl StorageManager { let log = self.log.new(o!("request" => "nested_dataset_list")); info!(log, "Listing nested datasets"); + // Observe all propreties for this nested datasets, including + // children. We'll apply user-specified filters later. let full_name = name.full_name(); let properties = Zfs::get_dataset_properties( &[full_name], From 4a7341ce6df61f1e88ba44a9f392a0f2edf4dacd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 8 Apr 2025 13:00:07 -0700 Subject: [PATCH 2/6] Removing legacy path, trying to simplify code --- illumos-utils/src/zfs.rs | 111 ++++++++++++------------- sled-agent/src/backing_fs.rs | 2 +- sled-agent/src/bootstrap/pre_server.rs | 2 +- sled-storage/src/dataset.rs | 4 +- sled-storage/src/manager.rs | 11 +-- 5 files changed, 59 insertions(+), 71 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 1a8da8d3cdf..1325164fed3 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -220,18 +220,11 @@ pub struct Zfs {} /// Describes a mountpoint for a ZFS filesystem. #[derive(Debug, Clone)] -pub enum Mountpoint { - #[allow(dead_code)] - Legacy, - Path(Utf8PathBuf), -} +pub struct Mountpoint(pub Utf8PathBuf); impl fmt::Display for Mountpoint { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Mountpoint::Legacy => write!(f, "legacy"), - Mountpoint::Path(p) => write!(f, "{p}"), - } + write!(f, "{}", self.0) } } @@ -927,12 +920,6 @@ impl Zfs { /// Ensures that a ZFS dataset is mounted, if it can be. /// - /// Pre-requisites to be mounted: - /// - The dataset exists - /// - The mountpoint is valid - /// - `can_mount` is CanMount::On - /// - `zoned` is false - /// /// Returns "true" if the dataset exists and is mounted /// Returns "false" if the dataset does not exist /// @@ -940,40 +927,44 @@ impl Zfs { pub fn ensure_dataset_mounted_if_exists( name: &str, mountpoint: &Mountpoint, - can_mount: CanMount, - zoned: bool, ) -> Result { - let res = Self::ensure_dataset_mounted_if_exists_inner( - name, mountpoint, can_mount, zoned, - ) - .map_err(|err| EnsureDatasetError { name: name.to_string(), err })?; + let res = + Self::ensure_dataset_mounted_if_exists_inner(name, mountpoint) + .map_err(|err| EnsureDatasetError { + name: name.to_string(), + err, + })?; Ok(res.exists) } fn ensure_dataset_mounted_if_exists_inner( name: &str, mountpoint: &Mountpoint, - can_mount: CanMount, - zoned: bool, ) -> Result { - let DatasetMountInfo { exists, mut mounted } = - Self::dataset_exists(name, mountpoint)?; - if !exists { - return Ok(DatasetMountInfo::does_not_exist()); + let mut mount_info = Self::dataset_exists(name, mountpoint)?; + if !mount_info.exists { + return Ok(mount_info); } - if !zoned && !mounted && can_mount.wants_mounting() { - if let Mountpoint::Path(path) = &mountpoint { - ensure_empty_immutable_mountpoint(&path).map_err(|err| { - EnsureDatasetErrorRaw::MountpointCreation { - mountpoint: path.to_path_buf(), - err, - } - })?; - Self::mount_dataset(name)?; - mounted = true; - } + + if !mount_info.mounted { + Self::ensure_dataset_mounted(name, mountpoint)?; + mount_info.mounted = true; } - return Ok(DatasetMountInfo::exists(mounted)); + return Ok(mount_info); + } + + fn ensure_dataset_mounted( + name: &str, + mountpoint: &Mountpoint, + ) -> Result<(), EnsureDatasetErrorRaw> { + ensure_empty_immutable_mountpoint(&mountpoint.0).map_err(|err| { + EnsureDatasetErrorRaw::MountpointCreation { + mountpoint: mountpoint.0.to_path_buf(), + err, + } + })?; + Self::mount_dataset(name)?; + Ok(()) } /// Creates a new ZFS dataset unless one already exists. @@ -999,21 +990,22 @@ impl Zfs { additional_options, }: DatasetEnsureArgs, ) -> Result<(), EnsureDatasetErrorRaw> { - // Determine if the dataset exists, and mount it if necessary. - let exists = Self::ensure_dataset_mounted_if_exists_inner( - name, - &mountpoint, - can_mount, - zoned, - )? - .exists; - + let dataset_info = Self::dataset_exists(name, &mountpoint)?; + let wants_mounting = + !zoned && !dataset_info.mounted && can_mount.wants_mounting(); let props = build_zfs_set_key_value_pairs(size_details, id); - if exists { - // If the dataset already exists: Update properties which might have - // changed. + + if dataset_info.exists { + // If the dataset already exists: Update properties which might + // have changed, and ensure it has been mounted if it needs + // to be mounted. Self::set_values(name, props.as_slice()) .map_err(|err| EnsureDatasetErrorRaw::from(err.err))?; + + if wants_mounting { + Self::ensure_dataset_mounted(name, &mountpoint)?; + } + return Ok(()); } @@ -1027,15 +1019,14 @@ impl Zfs { // // Zoned datasets are mounted when their zones are booted, so // we don't do this mountpoint manipulation for them. - if !zoned && can_mount.wants_mounting() { - if let Mountpoint::Path(path) = &mountpoint { - ensure_empty_immutable_mountpoint(&path).map_err(|err| { - EnsureDatasetErrorRaw::MountpointCreation { - mountpoint: path.to_path_buf(), - err, - } - })?; - } + if wants_mounting { + let path = &mountpoint.0; + ensure_empty_immutable_mountpoint(&path).map_err(|err| { + EnsureDatasetErrorRaw::MountpointCreation { + mountpoint: path.to_path_buf(), + err, + } + })?; } let mut command = std::process::Command::new(PFEXEC); diff --git a/sled-agent/src/backing_fs.rs b/sled-agent/src/backing_fs.rs index 7c227e4c5ab..b6c5e5ce6bd 100644 --- a/sled-agent/src/backing_fs.rs +++ b/sled-agent/src/backing_fs.rs @@ -134,7 +134,7 @@ pub(crate) fn ensure_backing_fs( sled_storage::dataset::M2_BACKING_DATASET, bfs.name ); - let mountpoint = Mountpoint::Path(Utf8PathBuf::from(bfs.mountpoint)); + let mountpoint = Mountpoint(Utf8PathBuf::from(bfs.mountpoint)); info!(log, "Ensuring dataset {}", dataset); diff --git a/sled-agent/src/bootstrap/pre_server.rs b/sled-agent/src/bootstrap/pre_server.rs index 41f5993298a..35a8c1f3b86 100644 --- a/sled-agent/src/bootstrap/pre_server.rs +++ b/sled-agent/src/bootstrap/pre_server.rs @@ -275,7 +275,7 @@ fn ensure_zfs_key_directory_exists(log: &Logger) -> Result<(), StartError> { fn ensure_zfs_ramdisk_dataset() -> Result<(), StartError> { Zfs::ensure_dataset(zfs::DatasetEnsureArgs { name: zfs::ZONE_ZFS_RAMDISK_DATASET, - mountpoint: zfs::Mountpoint::Path(Utf8PathBuf::from( + mountpoint: zfs::Mountpoint(Utf8PathBuf::from( zfs::ZONE_ZFS_RAMDISK_DATASET_MOUNTPOINT, )), can_mount: zfs::CanMount::On, diff --git a/sled-storage/src/dataset.rs b/sled-storage/src/dataset.rs index 0d28acad01e..4a5fd6feae5 100644 --- a/sled-storage/src/dataset.rs +++ b/sled-storage/src/dataset.rs @@ -266,7 +266,7 @@ pub(crate) async fn ensure_zpool_has_datasets( let name = format!("{}/{}", zpool_name, dataset); let result = Zfs::ensure_dataset(zfs::DatasetEnsureArgs { name: &name, - mountpoint: Mountpoint::Path(mountpoint), + mountpoint: Mountpoint(mountpoint), can_mount: zfs::CanMount::On, zoned, encryption_details: Some(encryption_details), @@ -337,7 +337,7 @@ pub(crate) async fn ensure_zpool_has_datasets( }); Zfs::ensure_dataset(zfs::DatasetEnsureArgs { name, - mountpoint: Mountpoint::Path(mountpoint), + mountpoint: Mountpoint(mountpoint), can_mount: zfs::CanMount::On, zoned, encryption_details, diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index ab331b2edb9..98e5b33a9c5 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -169,12 +169,9 @@ impl NestedDatasetLocation { mount_root: &Utf8Path, ) -> Result { let mountpoint = self.mountpoint(mount_root); - let zoned = false; Zfs::ensure_dataset_mounted_if_exists( &self.full_name(), - &Mountpoint::Path(mountpoint.clone()), - CanMount::On, - zoned, + &Mountpoint(mountpoint.clone()), )?; return Ok(mountpoint); @@ -1106,7 +1103,7 @@ impl StorageManager { let mountpoint_path = config.name.mountpoint(mountpoint_root); let details = DatasetCreationDetails { zoned: config.name.kind().zoned(), - mountpoint: Mountpoint::Path(mountpoint_path), + mountpoint: Mountpoint(mountpoint_path), full_name: config.name.full_name(), }; @@ -1195,7 +1192,7 @@ impl StorageManager { let details = DatasetCreationDetails { zoned: false, - mountpoint: Mountpoint::Path(mountpoint_path), + mountpoint: Mountpoint(mountpoint_path), full_name: config.name.full_name(), }; @@ -1554,7 +1551,7 @@ impl StorageManager { let size_details = None; Zfs::ensure_dataset(DatasetEnsureArgs { name: fs_name, - mountpoint: Mountpoint::Path(Utf8PathBuf::from("/data")), + mountpoint: Mountpoint(Utf8PathBuf::from("/data")), can_mount: CanMount::On, zoned, encryption_details, From ff7a399c7578c22e452290d9c325cef1751359bc Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 8 Apr 2025 13:38:44 -0700 Subject: [PATCH 3/6] Add tests --- sled-agent/src/support_bundle/storage.rs | 229 +++++++++++++++++++++++ 1 file changed, 229 insertions(+) diff --git a/sled-agent/src/support_bundle/storage.rs b/sled-agent/src/support_bundle/storage.rs index bcdeb05cdd8..8f39ddf5bdf 100644 --- a/sled-agent/src/support_bundle/storage.rs +++ b/sled-agent/src/support_bundle/storage.rs @@ -1495,6 +1495,235 @@ mod tests { logctx.cleanup_successful(); } + async fn is_mounted(dataset: &str) -> bool { + let mut command = tokio::process::Command::new(illumos_utils::zfs::ZFS); + let cmd = command.args(&["list", "-Hpo", "mounted", dataset]); + let output = cmd.output().await.unwrap(); + assert!(output.status.success(), "Failed to list dataset: {output:?}"); + String::from_utf8_lossy(&output.stdout).trim() == "yes" + } + + async fn unmount(dataset: &str) { + let mut command = tokio::process::Command::new(illumos_utils::PFEXEC); + let cmd = + command.args(&[illumos_utils::zfs::ZFS, "unmount", "-f", dataset]); + let output = cmd.output().await.unwrap(); + assert!( + output.status.success(), + "Failed to unmount dataset: {output:?}" + ); + } + + #[tokio::test] + async fn cannot_create_bundle_on_unmounted_parent() { + let logctx = test_setup_log("cannot_create_bundle_on_unmounted_parent"); + let log = &logctx.log; + + // Set up storage + let harness = SingleU2StorageHarness::new(log).await; + + // For this test, we'll add a dataset that can contain our bundles. + let dataset_id = DatasetUuid::new_v4(); + harness.configure_dataset(dataset_id, DatasetKind::Debug).await; + + // Access the Support Bundle API + let mgr = SupportBundleManager::new( + log, + harness.storage_test_harness.handle(), + ); + let support_bundle_id = SupportBundleUuid::new_v4(); + let zipfile_data = example_zipfile(); + let hash = ArtifactHash( + Sha256::digest(zipfile_data.as_slice()) + .as_slice() + .try_into() + .unwrap(), + ); + + // Before we actually create the bundle: + // + // Unmount the "parent dataset". This is equivalent to trying to create + // a support bundle when the debug dataset exists, but has not been + // mounted yet. + let parent_dataset = mgr + .get_configured_dataset_if_mounted(harness.zpool_id, dataset_id) + .await + .expect("Could not get parent dataset from test harness") + .name; + let parent_dataset_name = parent_dataset.full_name(); + assert!(is_mounted(&parent_dataset_name).await); + unmount(&parent_dataset_name).await; + assert!(!is_mounted(&parent_dataset_name).await); + + // Create a new bundle + let err = mgr + .create( + harness.zpool_id, + dataset_id, + support_bundle_id, + hash, + stream::once(async { + Ok(Bytes::copy_from_slice(zipfile_data.as_slice())) + }), + ) + .await + .expect_err("Should not have been able to create support bundle"); + let Error::DatasetNotMounted { dataset } = err else { + panic!("Unexpected error: {err:?}"); + }; + assert_eq!( + dataset, parent_dataset, + "Unexpected 'parent dataset' in error message" + ); + + harness.cleanup().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn listing_bundles_mounts_them() { + let logctx = test_setup_log("listing_bundles_mounts_them"); + let log = &logctx.log; + + // Set up storage + let harness = SingleU2StorageHarness::new(log).await; + + // For this test, we'll add a dataset that can contain our bundles. + let dataset_id = DatasetUuid::new_v4(); + harness.configure_dataset(dataset_id, DatasetKind::Debug).await; + + // Access the Support Bundle API + let mgr = SupportBundleManager::new( + log, + harness.storage_test_harness.handle(), + ); + let support_bundle_id = SupportBundleUuid::new_v4(); + let zipfile_data = example_zipfile(); + let hash = ArtifactHash( + Sha256::digest(zipfile_data.as_slice()) + .as_slice() + .try_into() + .unwrap(), + ); + + // Create a new bundle + let _ = mgr + .create( + harness.zpool_id, + dataset_id, + support_bundle_id, + hash, + stream::once(async { + Ok(Bytes::copy_from_slice(zipfile_data.as_slice())) + }), + ) + .await + .expect("Should have created support bundle"); + + // Peek under the hood: We should be able to observe the support + // bundle as a nested dataset. + let root = mgr + .get_configured_dataset_if_mounted(harness.zpool_id, dataset_id) + .await + .expect("Could not get parent dataset from test harness") + .name; + let nested_dataset = + NestedDatasetLocation { path: support_bundle_id.to_string(), root }; + let nested_dataset_name = nested_dataset.full_name(); + + // The dataset was mounted after creation. + assert!(is_mounted(&nested_dataset_name).await); + + // We can manually unmount this dataset. + unmount(&nested_dataset_name).await; + assert!(!is_mounted(&nested_dataset_name).await); + + // When we "list" this nested dataset, it'll be mounted once more. + let _ = mgr + .list(harness.zpool_id, dataset_id) + .await + .expect("Should have been able to list bundle"); + assert!(is_mounted(&nested_dataset_name).await); + + harness.cleanup().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn getting_bundles_mounts_them() { + let logctx = test_setup_log("getting_bundles_mounts_them"); + let log = &logctx.log; + + // Set up storage + let harness = SingleU2StorageHarness::new(log).await; + + // For this test, we'll add a dataset that can contain our bundles. + let dataset_id = DatasetUuid::new_v4(); + harness.configure_dataset(dataset_id, DatasetKind::Debug).await; + + // Access the Support Bundle API + let mgr = SupportBundleManager::new( + log, + harness.storage_test_harness.handle(), + ); + let support_bundle_id = SupportBundleUuid::new_v4(); + let zipfile_data = example_zipfile(); + let hash = ArtifactHash( + Sha256::digest(zipfile_data.as_slice()) + .as_slice() + .try_into() + .unwrap(), + ); + + // Create a new bundle + let _ = mgr + .create( + harness.zpool_id, + dataset_id, + support_bundle_id, + hash, + stream::once(async { + Ok(Bytes::copy_from_slice(zipfile_data.as_slice())) + }), + ) + .await + .expect("Should have created support bundle"); + + // Peek under the hood: We should be able to observe the support + // bundle as a nested dataset. + let root = mgr + .get_configured_dataset_if_mounted(harness.zpool_id, dataset_id) + .await + .expect("Could not get parent dataset from test harness") + .name; + let nested_dataset = + NestedDatasetLocation { path: support_bundle_id.to_string(), root }; + let nested_dataset_name = nested_dataset.full_name(); + + // The dataset was mounted after creation. + assert!(is_mounted(&nested_dataset_name).await); + + // We can manually unmount this dataset. + unmount(&nested_dataset_name).await; + assert!(!is_mounted(&nested_dataset_name).await); + + // When we "get" this nested dataset, it'll be mounted once more. + let _ = mgr + .get( + harness.zpool_id, + dataset_id, + support_bundle_id, + None, + SupportBundleQueryType::Whole, + ) + .await + .expect("Should have been able to GET bundle"); + assert!(is_mounted(&nested_dataset_name).await); + + harness.cleanup().await; + logctx.cleanup_successful(); + } + #[tokio::test] async fn creation_idempotency() { let logctx = test_setup_log("creation_idempotency"); From c94d8de2dd42861270246d2681100a478a32d8e0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 8 Apr 2025 14:24:38 -0700 Subject: [PATCH 4/6] Less confusing internal functions, more error handling --- illumos-utils/src/zfs.rs | 36 ++++++++--------- sled-agent/src/support_bundle/storage.rs | 49 +++++++++++++----------- sled-storage/src/manager.rs | 11 +++++- 3 files changed, 53 insertions(+), 43 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 1325164fed3..f3fe8bb4a39 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -139,6 +139,9 @@ enum EnsureDatasetErrorRaw { #[error("Unexpected output from ZFS commands: {0}")] Output(String), + #[error("Dataset does not exist")] + DoesNotExist, + #[error("Failed to mount filesystem")] MountFsFailed(#[source] crate::ExecutionError), @@ -918,39 +921,34 @@ impl Zfs { Ok(()) } - /// Ensures that a ZFS dataset is mounted, if it can be. - /// - /// Returns "true" if the dataset exists and is mounted - /// Returns "false" if the dataset does not exist + /// Ensures that a ZFS dataset is mounted /// /// Returns an error if the dataset exists, but cannot be mounted. - pub fn ensure_dataset_mounted_if_exists( + pub fn ensure_dataset_mounted_and_exists( name: &str, mountpoint: &Mountpoint, - ) -> Result { - let res = - Self::ensure_dataset_mounted_if_exists_inner(name, mountpoint) - .map_err(|err| EnsureDatasetError { - name: name.to_string(), - err, - })?; - Ok(res.exists) + ) -> Result<(), EnsureDatasetError> { + Self::ensure_dataset_mounted_and_exists_inner(name, mountpoint) + .map_err(|err| EnsureDatasetError { + name: name.to_string(), + err, + })?; + Ok(()) } - fn ensure_dataset_mounted_if_exists_inner( + fn ensure_dataset_mounted_and_exists_inner( name: &str, mountpoint: &Mountpoint, - ) -> Result { - let mut mount_info = Self::dataset_exists(name, mountpoint)?; + ) -> Result<(), EnsureDatasetErrorRaw> { + let mount_info = Self::dataset_exists(name, mountpoint)?; if !mount_info.exists { - return Ok(mount_info); + return Err(EnsureDatasetErrorRaw::DoesNotExist); } if !mount_info.mounted { Self::ensure_dataset_mounted(name, mountpoint)?; - mount_info.mounted = true; } - return Ok(mount_info); + return Ok(()); } fn ensure_dataset_mounted( diff --git a/sled-agent/src/support_bundle/storage.rs b/sled-agent/src/support_bundle/storage.rs index 8f39ddf5bdf..57f1a1e185c 100644 --- a/sled-agent/src/support_bundle/storage.rs +++ b/sled-agent/src/support_bundle/storage.rs @@ -180,17 +180,25 @@ impl LocalStorage for StorageHandle { self.datasets_config_list().await.map_err(|err| err.into()) } - // TODO: Should this be a part of "StorageHandle"? fn dyn_dataset_get( &self, dataset_name: &String, ) -> Result { - Ok(illumos_utils::zfs::Zfs::get_dataset_properties( + let Some(dataset) = illumos_utils::zfs::Zfs::get_dataset_properties( &[dataset_name.clone()], illumos_utils::zfs::WhichDatasets::SelfOnly, ) .map_err(|err| Error::DatasetLookup(err))? - .remove(0)) + .pop() else { + // This should not be possible, unless the "zfs get" command is + // behaving unpredictably. We're only asking for a single dataset, + // so on success, we should see the result of that dataset. + return Err(Error::DatasetLookup(anyhow::anyhow!( + "Zfs::get_dataset_properties returned an empty vec?" + ))); + }; + + Ok(dataset) } async fn dyn_nested_dataset_list( @@ -413,8 +421,11 @@ impl<'a> SupportBundleManager<'a> { // Returns a dataset that the sled has been explicitly configured to use. // + // In the context of Support Bundles, this is a "parent dataset", within + // which the "nested support bundle" dataset will be stored. + // // Returns an error if this dataset is not mounted. - async fn get_configured_dataset_if_mounted( + async fn get_mounted_dataset_config( &self, zpool_id: ZpoolUuid, dataset_id: DatasetUuid, @@ -455,10 +466,8 @@ impl<'a> SupportBundleManager<'a> { zpool_id: ZpoolUuid, dataset_id: DatasetUuid, ) -> Result, Error> { - let root = self - .get_configured_dataset_if_mounted(zpool_id, dataset_id) - .await? - .name; + let root = + self.get_mounted_dataset_config(zpool_id, dataset_id).await?.name; let dataset_location = NestedDatasetLocation { path: String::from(""), root }; let datasets = self @@ -574,10 +583,8 @@ impl<'a> SupportBundleManager<'a> { // Access the parent dataset (presumably "crypt/debug") // where the support bundled will be mounted. - let root = self - .get_configured_dataset_if_mounted(zpool_id, dataset_id) - .await? - .name; + let root = + self.get_mounted_dataset_config(zpool_id, dataset_id).await?.name; let dataset = NestedDatasetLocation { path: support_bundle_id.to_string(), root }; @@ -681,10 +688,8 @@ impl<'a> SupportBundleManager<'a> { "bundle_id" => support_bundle_id.to_string(), )); info!(log, "Destroying support bundle"); - let root = self - .get_configured_dataset_if_mounted(zpool_id, dataset_id) - .await? - .name; + let root = + self.get_mounted_dataset_config(zpool_id, dataset_id).await?.name; self.storage .dyn_nested_dataset_destroy(NestedDatasetLocation { path: support_bundle_id.to_string(), @@ -702,10 +707,8 @@ impl<'a> SupportBundleManager<'a> { support_bundle_id: SupportBundleUuid, ) -> Result { // Access the parent dataset where the support bundle is stored. - let root = self - .get_configured_dataset_if_mounted(zpool_id, dataset_id) - .await? - .name; + let root = + self.get_mounted_dataset_config(zpool_id, dataset_id).await?.name; let dataset = NestedDatasetLocation { path: support_bundle_id.to_string(), root }; @@ -1546,7 +1549,7 @@ mod tests { // a support bundle when the debug dataset exists, but has not been // mounted yet. let parent_dataset = mgr - .get_configured_dataset_if_mounted(harness.zpool_id, dataset_id) + .get_mounted_dataset_config(harness.zpool_id, dataset_id) .await .expect("Could not get parent dataset from test harness") .name; @@ -1623,7 +1626,7 @@ mod tests { // Peek under the hood: We should be able to observe the support // bundle as a nested dataset. let root = mgr - .get_configured_dataset_if_mounted(harness.zpool_id, dataset_id) + .get_mounted_dataset_config(harness.zpool_id, dataset_id) .await .expect("Could not get parent dataset from test harness") .name; @@ -1692,7 +1695,7 @@ mod tests { // Peek under the hood: We should be able to observe the support // bundle as a nested dataset. let root = mgr - .get_configured_dataset_if_mounted(harness.zpool_id, dataset_id) + .get_mounted_dataset_config(harness.zpool_id, dataset_id) .await .expect("Could not get parent dataset from test harness") .name; diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 98e5b33a9c5..ba835176f3a 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -169,7 +169,7 @@ impl NestedDatasetLocation { mount_root: &Utf8Path, ) -> Result { let mountpoint = self.mountpoint(mount_root); - Zfs::ensure_dataset_mounted_if_exists( + Zfs::ensure_dataset_mounted_and_exists( &self.full_name(), &Mountpoint(mountpoint.clone()), )?; @@ -425,6 +425,15 @@ impl StorageHandle { rx.await.unwrap() } + /// Ensures that a dataset exists, nested somewhere arbitrary within + /// a Nexus-controlled dataset. + /// + /// This function does mount the dataset according to `config`. + /// However, this dataset is not automatically mounted on reboot. + /// + /// If you're trying to access a nested dataset later, consider + /// using the [NestedDatasetLocation::ensure_mounted_and_get_mountpoint] + /// function. pub async fn nested_dataset_ensure( &self, config: NestedDatasetConfig, From be88715bafcc678b1b447bf63077301a79dbc10e Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 8 Apr 2025 16:07:35 -0700 Subject: [PATCH 5/6] Abstract mounting in sim-sled-agent for Nexus tests --- sled-agent/src/support_bundle/storage.rs | 47 ++++++++++++++++++++---- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/sled-agent/src/support_bundle/storage.rs b/sled-agent/src/support_bundle/storage.rs index 57f1a1e185c..4040823fda6 100644 --- a/sled-agent/src/support_bundle/storage.rs +++ b/sled-agent/src/support_bundle/storage.rs @@ -7,6 +7,7 @@ use async_trait::async_trait; use bytes::Bytes; use camino::Utf8Path; +use camino::Utf8PathBuf; use dropshot::Body; use dropshot::HttpError; use futures::Stream; @@ -147,6 +148,13 @@ pub trait LocalStorage: Sync { dataset_name: &String, ) -> Result; + /// Ensure a dataset is mounted + async fn dyn_ensure_mounted_and_get_mountpoint( + &self, + dataset: NestedDatasetLocation, + mount_root: &Utf8Path, + ) -> Result; + /// Returns all nested datasets within an existing dataset async fn dyn_nested_dataset_list( &self, @@ -201,6 +209,17 @@ impl LocalStorage for StorageHandle { Ok(dataset) } + async fn dyn_ensure_mounted_and_get_mountpoint( + &self, + dataset: NestedDatasetLocation, + mount_root: &Utf8Path, + ) -> Result { + dataset + .ensure_mounted_and_get_mountpoint(mount_root) + .await + .map_err(Error::from) + } + async fn dyn_nested_dataset_list( &self, name: NestedDatasetLocation, @@ -242,6 +261,15 @@ impl LocalStorage for crate::sim::Storage { self.lock().dataset_get(dataset_name).map_err(|err| err.into()) } + async fn dyn_ensure_mounted_and_get_mountpoint( + &self, + dataset: NestedDatasetLocation, + mount_root: &Utf8Path, + ) -> Result { + // Simulated storage treats all datasets as mounted. + Ok(dataset.mountpoint(mount_root)) + } + async fn dyn_nested_dataset_list( &self, name: NestedDatasetLocation, @@ -489,9 +517,10 @@ impl<'a> SupportBundleManager<'a> { }; // The dataset for a support bundle exists. - let support_bundle_path = dataset - .name - .ensure_mounted_and_get_mountpoint( + let support_bundle_path = self + .storage + .dyn_ensure_mounted_and_get_mountpoint( + dataset.name, &self.storage.zpool_mountpoint_root(), ) .await? @@ -603,8 +632,10 @@ impl<'a> SupportBundleManager<'a> { info!(log, "Dataset does exist for bundle"); // The mounted root of the support bundle dataset - let support_bundle_dir = dataset - .ensure_mounted_and_get_mountpoint( + let support_bundle_dir = self + .storage + .dyn_ensure_mounted_and_get_mountpoint( + dataset, &self.storage.zpool_mountpoint_root(), ) .await?; @@ -713,8 +744,10 @@ impl<'a> SupportBundleManager<'a> { NestedDatasetLocation { path: support_bundle_id.to_string(), root }; // The mounted root of the support bundle dataset - let support_bundle_dir = dataset - .ensure_mounted_and_get_mountpoint( + let support_bundle_dir = self + .storage + .dyn_ensure_mounted_and_get_mountpoint( + dataset, &self.storage.zpool_mountpoint_root(), ) .await?; From 6213a86080f11030361958b4c76b2f8a8ec756e3 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Apr 2025 10:11:48 -0700 Subject: [PATCH 6/6] comment --- illumos-utils/src/zfs.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index f3fe8bb4a39..f2ae4cd3bc6 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -989,6 +989,12 @@ impl Zfs { }: DatasetEnsureArgs, ) -> Result<(), EnsureDatasetErrorRaw> { let dataset_info = Self::dataset_exists(name, &mountpoint)?; + + // Non-zoned datasets with an explicit mountpoint and the + // "canmount=on" property should be mounted within the global zone. + // + // Zoned datasets are mounted when their zones are booted, so + // we don't do this mountpoint manipulation for them. let wants_mounting = !zoned && !dataset_info.mounted && can_mount.wants_mounting(); let props = build_zfs_set_key_value_pairs(size_details, id); @@ -1009,14 +1015,8 @@ impl Zfs { // If the dataset doesn't exist, create it. - // Non-zoned datasets with an explicit mountpoint and the - // "canmount=on" property should be mounted within the global zone. - // // We'll ensure they have an empty immutable mountpoint before // creating the dataset itself, which will also mount it. - // - // Zoned datasets are mounted when their zones are booted, so - // we don't do this mountpoint manipulation for them. if wants_mounting { let path = &mountpoint.0; ensure_empty_immutable_mountpoint(&path).map_err(|err| {