Clean up "Zfs::ensure_filesystem"

- Moves arguments into a struct
- Removes unused "do_format" argument
- Renamed to "ensure_dataset"

Fixes #7237

illumos-utils/src/zfs.rs

@@ -69,9 +69,6 @@ enum EnsureFilesystemErrorRaw {
     #[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,7 +79,7 @@ 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 {
@@ -430,6 +427,49 @@ impl PropertySetter {
     }
 }
 
+/// 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<EncryptionDetails>,
+
+    /// 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<SizeDetails>,
+
+    /// 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<DatasetUuid>,
+
+    /// 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<Vec<String>>,
+}
+
 impl Zfs {
     /// Lists all datasets within a pool or existing dataset.
     ///
@@ -525,37 +565,19 @@ 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<EncryptionDetails>,
-        size_details: Option<SizeDetails>,
-        id: Option<DatasetUuid>,
-        additional_options: Option<Vec<String>>,
+    /// 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<(), EnsureFilesystemError> {
         let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?;
 
@@ -589,13 +611,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"]);

sled-agent/src/backing_fs.rs

@@ -23,7 +23,8 @@
 
 use camino::Utf8PathBuf;
 use illumos_utils::zfs::{
-    EnsureFilesystemError, GetValueError, Mountpoint, SizeDetails, Zfs,
+    DatasetEnsureArgs, EnsureFilesystemError, GetValueError, Mountpoint,
+    SizeDetails, Zfs,
 };
 use omicron_common::api::external::ByteCount;
 use omicron_common::disk::CompressionAlgorithm;
@@ -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

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)
 }
 

sled-storage/src/dataset.rs

@@ -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(

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::{
@@ -1447,7 +1449,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;
@@ -1456,16 +1457,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(())
     }
@@ -1490,19 +1490,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::<Uuid>() {