From 7ff5a2855a1bf0154e5f63538346ccce5d9f98b9 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 16 Dec 2024 12:33:47 -0800 Subject: [PATCH] Clean up "Zfs::ensure_filesystem" (#7246) - Moves arguments into a struct - Removes unused "do_format" argument - Renamed to "ensure_dataset" Fixes https://github.com/oxidecomputer/omicron/issues/7237 --- illumos-utils/src/zfs.rs | 138 +++++++++++++------------ sled-agent/src/backing_fs.rs | 22 ++-- sled-agent/src/bootstrap/pre_server.rs | 23 ++--- sled-agent/src/bootstrap/server.rs | 2 +- sled-agent/src/zone_bundle.rs | 6 +- sled-storage/src/dataset.rs | 33 +++--- sled-storage/src/error.rs | 2 +- sled-storage/src/manager.rs | 34 +++--- 8 files changed, 129 insertions(+), 131 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index ee1ac58be9..e99ebc5b59 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -65,13 +65,10 @@ pub struct DestroyDatasetError { } #[derive(thiserror::Error, Debug)] -enum EnsureFilesystemErrorRaw { +enum EnsureDatasetErrorRaw { #[error("ZFS execution error: {0}")] Execution(#[from] crate::ExecutionError), - #[error("Filesystem does not exist, and formatting was not requested")] - NotFoundNotFormatted, - #[error("Unexpected output from ZFS commands: {0}")] Output(String), @@ -82,13 +79,13 @@ enum EnsureFilesystemErrorRaw { MountOverlayFsFailed(crate::ExecutionError), } -/// Error returned by [`Zfs::ensure_filesystem`]. +/// Error returned by [`Zfs::ensure_dataset`]. #[derive(thiserror::Error, Debug)] #[error("Failed to ensure filesystem '{name}': {err}")] -pub struct EnsureFilesystemError { +pub struct EnsureDatasetError { name: String, #[source] - err: EnsureFilesystemErrorRaw, + err: EnsureDatasetErrorRaw, } /// Error returned by [`Zfs::set_oxide_value`] @@ -418,6 +415,49 @@ fn build_zfs_set_key_value_pairs( props } +/// Arguments to [Zfs::ensure_dataset]. +pub struct DatasetEnsureArgs<'a> { + /// The full path of the ZFS dataset. + pub name: &'a str, + + /// The expected mountpoint of this filesystem. + /// If the filesystem already exists, and is not mounted here, an error is + /// returned. + pub mountpoint: Mountpoint, + + /// Identifies whether or not this filesystem should be + /// used in a zone. Only used when creating a new filesystem - ignored + /// if the filesystem already exists. + pub zoned: bool, + + /// Ensures a filesystem as an encryption root. + /// + /// For new filesystems, this supplies the key, and all datasets within this + /// root are implicitly encrypted. For existing filesystems, ensures that + /// they are mounted (and that keys are loaded), but does not verify the + /// input details. + pub encryption_details: Option, + + /// Optional properties that can be set for the dataset regarding + /// space usage. + /// + /// Can be used to change settings on new or existing datasets. + pub size_details: Option, + + /// An optional UUID of the dataset. + /// + /// If provided, this is set as the value "oxide:uuid" through "zfs set". + /// + /// Can be used to change settings on new or existing datasets. + pub id: Option, + + /// ZFS options passed to "zfs create" with the "-o" flag. + /// + /// Only used when the filesystem is being created. + /// Each string in this optional Vec should have the format "key=value". + pub additional_options: Option>, +} + impl Zfs { /// Lists all datasets within a pool or existing dataset. /// @@ -513,44 +553,26 @@ impl Zfs { Ok(()) } - /// Creates a new ZFS filesystem unless one already exists. + /// Creates a new ZFS dataset unless one already exists. /// - /// - `name`: the full path to the zfs dataset - /// - `mountpoint`: The expected mountpoint of this filesystem. - /// If the filesystem already exists, and is not mounted here, and error is - /// returned. - /// - `zoned`: identifies whether or not this filesystem should be - /// used in a zone. Only used when creating a new filesystem - ignored - /// if the filesystem already exists. - /// - `do_format`: if "false", prevents a new filesystem from being created, - /// and returns an error if it is not found. - /// - `encryption_details`: Ensures a filesystem as an encryption root. - /// For new filesystems, this supplies the key, and all datasets within this - /// root are implicitly encrypted. For existing filesystems, ensures that - /// they are mounted (and that keys are loaded), but does not verify the - /// input details. - /// - `size_details`: If supplied, sets size-related information. These - /// values are set on both new filesystem creation as well as when loading - /// existing filesystems. - /// - `additional_options`: Additional ZFS options, which are only set when - /// creating new filesystems. - #[allow(clippy::too_many_arguments)] - pub fn ensure_filesystem( - name: &str, - mountpoint: Mountpoint, - zoned: bool, - do_format: bool, - encryption_details: Option, - size_details: Option, - id: Option, - additional_options: Option>, - ) -> Result<(), EnsureFilesystemError> { + /// Refer to [DatasetEnsureArgs] for details on the supplied arguments. + pub fn ensure_dataset( + DatasetEnsureArgs { + name, + mountpoint, + zoned, + encryption_details, + size_details, + id, + additional_options, + }: DatasetEnsureArgs, + ) -> Result<(), EnsureDatasetError> { let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?; let props = build_zfs_set_key_value_pairs(size_details, id); if exists { Self::set_values(name, props.as_slice()).map_err(|err| { - EnsureFilesystemError { + EnsureDatasetError { name: name.to_string(), err: err.err.into(), } @@ -570,13 +592,6 @@ impl Zfs { } } - if !do_format { - return Err(EnsureFilesystemError { - name: name.to_string(), - err: EnsureFilesystemErrorRaw::NotFoundNotFormatted, - }); - } - // If it doesn't exist, make it. let mut command = std::process::Command::new(PFEXEC); let cmd = command.args(&[ZFS, "create"]); @@ -606,7 +621,7 @@ impl Zfs { cmd.args(&["-o", &format!("mountpoint={}", mountpoint), name]); - execute(cmd).map_err(|err| EnsureFilesystemError { + execute(cmd).map_err(|err| EnsureDatasetError { name: name.to_string(), err: err.into(), })?; @@ -618,42 +633,35 @@ impl Zfs { let user = whoami::username(); let mount = format!("{mountpoint}"); let cmd = command.args(["chown", "-R", &user, &mount]); - execute(cmd).map_err(|err| EnsureFilesystemError { + execute(cmd).map_err(|err| EnsureDatasetError { name: name.to_string(), err: err.into(), })?; } Self::set_values(name, props.as_slice()).map_err(|err| { - EnsureFilesystemError { - name: name.to_string(), - err: err.err.into(), - } + EnsureDatasetError { name: name.to_string(), err: err.err.into() } })?; Ok(()) } - fn mount_encrypted_dataset( - name: &str, - ) -> Result<(), EnsureFilesystemError> { + fn mount_encrypted_dataset(name: &str) -> Result<(), EnsureDatasetError> { let mut command = std::process::Command::new(PFEXEC); let cmd = command.args(&[ZFS, "mount", "-l", name]); - execute(cmd).map_err(|err| EnsureFilesystemError { + execute(cmd).map_err(|err| EnsureDatasetError { name: name.to_string(), - err: EnsureFilesystemErrorRaw::MountEncryptedFsFailed(err), + err: EnsureDatasetErrorRaw::MountEncryptedFsFailed(err), })?; Ok(()) } - pub fn mount_overlay_dataset( - name: &str, - ) -> Result<(), EnsureFilesystemError> { + pub fn mount_overlay_dataset(name: &str) -> Result<(), EnsureDatasetError> { let mut command = std::process::Command::new(PFEXEC); let cmd = command.args(&[ZFS, "mount", "-O", name]); - execute(cmd).map_err(|err| EnsureFilesystemError { + execute(cmd).map_err(|err| EnsureDatasetError { name: name.to_string(), - err: EnsureFilesystemErrorRaw::MountOverlayFsFailed(err), + err: EnsureDatasetErrorRaw::MountOverlayFsFailed(err), })?; Ok(()) } @@ -663,7 +671,7 @@ impl Zfs { fn dataset_exists( name: &str, mountpoint: &Mountpoint, - ) -> Result<(bool, bool), EnsureFilesystemError> { + ) -> Result<(bool, bool), EnsureDatasetError> { let mut command = std::process::Command::new(ZFS); let cmd = command.args(&[ "list", @@ -676,9 +684,9 @@ impl Zfs { let stdout = String::from_utf8_lossy(&output.stdout); let values: Vec<&str> = stdout.trim().split('\t').collect(); if &values[..3] != &[name, "filesystem", &mountpoint.to_string()] { - return Err(EnsureFilesystemError { + return Err(EnsureDatasetError { name: name.to_string(), - err: EnsureFilesystemErrorRaw::Output(stdout.to_string()), + err: EnsureDatasetErrorRaw::Output(stdout.to_string()), }); } let mounted = values[3] == "yes"; diff --git a/sled-agent/src/backing_fs.rs b/sled-agent/src/backing_fs.rs index d24f85ad80..1b04c7597b 100644 --- a/sled-agent/src/backing_fs.rs +++ b/sled-agent/src/backing_fs.rs @@ -23,7 +23,8 @@ use camino::Utf8PathBuf; use illumos_utils::zfs::{ - EnsureFilesystemError, GetValueError, Mountpoint, SizeDetails, Zfs, + DatasetEnsureArgs, EnsureDatasetError, GetValueError, Mountpoint, + SizeDetails, Zfs, }; use omicron_common::api::external::ByteCount; use omicron_common::disk::CompressionAlgorithm; @@ -38,7 +39,7 @@ pub enum BackingFsError { DatasetProperty(#[from] GetValueError), #[error("Error initializing dataset: {0}")] - Mount(#[from] EnsureFilesystemError), + Mount(#[from] EnsureDatasetError), #[error("Failed to ensure subdirectory {0}")] EnsureSubdir(#[from] io::Error), @@ -143,16 +144,15 @@ pub(crate) fn ensure_backing_fs( compression: bfs.compression, }); - Zfs::ensure_filesystem( - &dataset, - mountpoint.clone(), - false, // zoned - true, // do_format - None, // encryption_details, + Zfs::ensure_dataset(DatasetEnsureArgs { + name: &dataset, + mountpoint: mountpoint.clone(), + zoned: false, + encryption_details: None, size_details, - None, - Some(vec!["canmount=noauto".to_string()]), // options - )?; + id: None, + additional_options: Some(vec!["canmount=noauto".to_string()]), + })?; // Check if a ZFS filesystem is already mounted on bfs.mountpoint by // retrieving the ZFS `mountpoint` property and comparing it. This diff --git a/sled-agent/src/bootstrap/pre_server.rs b/sled-agent/src/bootstrap/pre_server.rs index 19a63cb71d..ee72101da2 100644 --- a/sled-agent/src/bootstrap/pre_server.rs +++ b/sled-agent/src/bootstrap/pre_server.rs @@ -279,22 +279,17 @@ fn ensure_zfs_key_directory_exists(log: &Logger) -> Result<(), StartError> { } fn ensure_zfs_ramdisk_dataset() -> Result<(), StartError> { - let zoned = false; - let do_format = true; - let encryption_details = None; - let quota = None; - Zfs::ensure_filesystem( - zfs::ZONE_ZFS_RAMDISK_DATASET, - zfs::Mountpoint::Path(Utf8PathBuf::from( + Zfs::ensure_dataset(zfs::DatasetEnsureArgs { + name: zfs::ZONE_ZFS_RAMDISK_DATASET, + mountpoint: zfs::Mountpoint::Path(Utf8PathBuf::from( zfs::ZONE_ZFS_RAMDISK_DATASET_MOUNTPOINT, )), - zoned, - do_format, - encryption_details, - quota, - None, - None, - ) + zoned: false, + encryption_details: None, + size_details: None, + id: None, + additional_options: None, + }) .map_err(StartError::EnsureZfsRamdiskDataset) } diff --git a/sled-agent/src/bootstrap/server.rs b/sled-agent/src/bootstrap/server.rs index cdd09fbbe8..f5d8bce3d6 100644 --- a/sled-agent/src/bootstrap/server.rs +++ b/sled-agent/src/bootstrap/server.rs @@ -115,7 +115,7 @@ pub enum StartError { CreateDdmAdminLocalhostClient(#[source] DdmError), #[error("Failed to create ZFS ramdisk dataset")] - EnsureZfsRamdiskDataset(#[source] zfs::EnsureFilesystemError), + EnsureZfsRamdiskDataset(#[source] zfs::EnsureDatasetError), #[error("Failed to list zones")] ListZones(#[source] zone::AdmError), diff --git a/sled-agent/src/zone_bundle.rs b/sled-agent/src/zone_bundle.rs index 7a66ca088f..853f341c40 100644 --- a/sled-agent/src/zone_bundle.rs +++ b/sled-agent/src/zone_bundle.rs @@ -18,7 +18,7 @@ use illumos_utils::running_zone::ServiceProcess; use illumos_utils::zfs::CreateSnapshotError; use illumos_utils::zfs::DestroyDatasetError; use illumos_utils::zfs::DestroySnapshotError; -use illumos_utils::zfs::EnsureFilesystemError; +use illumos_utils::zfs::EnsureDatasetError; use illumos_utils::zfs::GetValueError; use illumos_utils::zfs::ListDatasetsError; use illumos_utils::zfs::ListSnapshotsError; @@ -565,8 +565,8 @@ pub enum BundleError { #[error("Failed to list ZFS snapshots")] ListSnapshot(#[from] ListSnapshotsError), - #[error("Failed to ensure ZFS filesystem")] - EnsureFilesystem(#[from] EnsureFilesystemError), + #[error("Failed to ensure ZFS dataset")] + EnsureDataset(#[from] EnsureDatasetError), #[error("Failed to destroy ZFS dataset")] DestroyDataset(#[from] DestroyDatasetError), diff --git a/sled-storage/src/dataset.rs b/sled-storage/src/dataset.rs index 717595ad9e..42c1d324ca 100644 --- a/sled-storage/src/dataset.rs +++ b/sled-storage/src/dataset.rs @@ -158,7 +158,7 @@ pub enum DatasetError { #[error(transparent)] DestroyFilesystem(#[from] illumos_utils::zfs::DestroyDatasetError), #[error(transparent)] - EnsureFilesystem(#[from] illumos_utils::zfs::EnsureFilesystemError), + EnsureDataset(#[from] illumos_utils::zfs::EnsureDatasetError), #[error("KeyManager error: {0}")] KeyManager(#[from] key_manager::Error), #[error("Missing StorageKeyRequester when creating U.2 disk")] @@ -200,7 +200,6 @@ pub(crate) async fn ensure_zpool_has_datasets( }; let zoned = false; - let do_format = true; // Ensure the root encrypted filesystem exists // Datasets below this in the hierarchy will inherit encryption @@ -261,16 +260,15 @@ pub(crate) async fn ensure_zpool_has_datasets( log, "Ensuring encrypted filesystem: {} for epoch {}", dataset, epoch ); - let result = Zfs::ensure_filesystem( - &format!("{}/{}", zpool_name, dataset), - Mountpoint::Path(mountpoint), + let result = Zfs::ensure_dataset(zfs::DatasetEnsureArgs { + name: &format!("{}/{}", zpool_name, dataset), + mountpoint: Mountpoint::Path(mountpoint), zoned, - do_format, - Some(encryption_details), - None, - None, - None, - ); + encryption_details: Some(encryption_details), + size_details: None, + id: None, + additional_options: None, + }); keyfile.zero_and_unlink().await.map_err(|error| { DatasetError::IoError { path: keyfile.path().0.clone(), error } @@ -324,16 +322,15 @@ pub(crate) async fn ensure_zpool_has_datasets( reservation: None, compression: dataset.compression, }); - Zfs::ensure_filesystem( + Zfs::ensure_dataset(zfs::DatasetEnsureArgs { name, - Mountpoint::Path(mountpoint), + mountpoint: Mountpoint::Path(mountpoint), zoned, - do_format, encryption_details, size_details, - None, - None, - )?; + id: None, + additional_options: None, + })?; if dataset.wipe { Zfs::set_oxide_value(name, "agent", agent_local_value).map_err( @@ -356,7 +353,7 @@ pub enum DatasetEncryptionMigrationError { FailedCommand { command: String, stderr: Option }, #[error("Cannot create new encrypted dataset")] - DatasetCreation(#[from] illumos_utils::zfs::EnsureFilesystemError), + DatasetCreation(#[from] illumos_utils::zfs::EnsureDatasetError), #[error("Missing stdout stream during 'zfs send' command")] MissingStdoutForZfsSend, diff --git a/sled-storage/src/error.rs b/sled-storage/src/error.rs index f00e35e654..1e49616162 100644 --- a/sled-storage/src/error.rs +++ b/sled-storage/src/error.rs @@ -24,7 +24,7 @@ pub enum Error { ZfsListDataset(#[from] illumos_utils::zfs::ListDatasetsError), #[error(transparent)] - ZfsEnsureFilesystem(#[from] illumos_utils::zfs::EnsureFilesystemError), + ZfsEnsureDataset(#[from] illumos_utils::zfs::EnsureDatasetError), #[error(transparent)] ZfsSetValue(#[from] illumos_utils::zfs::SetValueError), diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index b1832ca92b..f0653c7905 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -16,7 +16,9 @@ use debug_ignore::DebugIgnore; use futures::future::FutureExt; use futures::Stream; use futures::StreamExt; -use illumos_utils::zfs::{DatasetProperties, Mountpoint, WhichDatasets, Zfs}; +use illumos_utils::zfs::{ + DatasetEnsureArgs, DatasetProperties, Mountpoint, WhichDatasets, Zfs, +}; use illumos_utils::zpool::{ZpoolName, ZPOOL_MOUNTPOINT_ROOT}; use key_manager::StorageKeyRequester; use omicron_common::disk::{ @@ -1448,7 +1450,6 @@ impl StorageManager { } let DatasetCreationDetails { zoned, mountpoint, full_name } = details; - let do_format = true; // The "crypt" dataset needs these details, but should already exist // by the time we're creating datasets inside. let encryption_details = None; @@ -1457,16 +1458,15 @@ impl StorageManager { reservation: config.reservation, compression: config.compression, }); - Zfs::ensure_filesystem( - &full_name, - mountpoint.clone(), - *zoned, - do_format, + Zfs::ensure_dataset(DatasetEnsureArgs { + name: &full_name, + mountpoint: mountpoint.clone(), + zoned: *zoned, encryption_details, size_details, - dataset_id, - None, - )?; + id: dataset_id, + additional_options: None, + })?; Ok(()) } @@ -1491,19 +1491,17 @@ impl StorageManager { let zoned = true; let fs_name = &request.dataset_name.full_name(); - let do_format = true; let encryption_details = None; let size_details = None; - Zfs::ensure_filesystem( - fs_name, - Mountpoint::Path(Utf8PathBuf::from("/data")), + Zfs::ensure_dataset(DatasetEnsureArgs { + name: fs_name, + mountpoint: Mountpoint::Path(Utf8PathBuf::from("/data")), zoned, - do_format, encryption_details, size_details, - Some(DatasetUuid::from_untyped_uuid(request.dataset_id)), - None, - )?; + id: Some(DatasetUuid::from_untyped_uuid(request.dataset_id)), + additional_options: None, + })?; // Ensure the dataset has a usable UUID. if let Ok(id_str) = Zfs::get_oxide_value(&fs_name, "uuid") { if let Ok(id) = id_str.parse::() {