-
Notifications
You must be signed in to change notification settings - Fork 63
[sled-agent] Ensure Support Bundles datasets are mounted #7938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6925aed
4a7341c
ff7a399
c94d8de
be88715
026700a
6213a86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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), | ||
|
|
||
|
|
@@ -220,18 +223,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) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -507,12 +503,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 { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just to make the code more readable below. Should be the same functionality as before. |
||
| 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 +802,21 @@ fn is_directory_immutable( | |
| return Ok(result); | ||
| } | ||
|
|
||
| struct DatasetMountInfo { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We used to return a tuple of bools, I'm just making this explicit for readability. |
||
| 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 +921,50 @@ impl Zfs { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Ensures that a ZFS dataset is mounted | ||
| /// | ||
| /// Returns an error if the dataset exists, but cannot be mounted. | ||
| pub fn ensure_dataset_mounted_and_exists( | ||
| name: &str, | ||
| mountpoint: &Mountpoint, | ||
| ) -> 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_and_exists_inner( | ||
| name: &str, | ||
| mountpoint: &Mountpoint, | ||
| ) -> Result<(), EnsureDatasetErrorRaw> { | ||
| let mount_info = Self::dataset_exists(name, mountpoint)?; | ||
| if !mount_info.exists { | ||
| return Err(EnsureDatasetErrorRaw::DoesNotExist); | ||
| } | ||
|
|
||
| if !mount_info.mounted { | ||
| Self::ensure_dataset_mounted(name, mountpoint)?; | ||
| } | ||
| return Ok(()); | ||
| } | ||
|
|
||
| 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. | ||
| /// | ||
| /// Refer to [DatasetEnsureArgs] for details on the supplied arguments. | ||
|
|
@@ -923,53 +988,43 @@ impl Zfs { | |
| additional_options, | ||
| }: DatasetEnsureArgs, | ||
| ) -> Result<(), EnsureDatasetErrorRaw> { | ||
| let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?; | ||
| 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); | ||
| if exists { | ||
|
|
||
| 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 !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)?; | ||
| } | ||
| if wants_mounting { | ||
| Self::ensure_dataset_mounted(name, &mountpoint)?; | ||
| } | ||
|
|
||
| return Ok(()); | ||
| } | ||
|
|
||
| // 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 !zoned { | ||
| 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, | ||
| } | ||
| })?; | ||
| } | ||
| if wants_mounting { | ||
| let path = &mountpoint.0; | ||
| ensure_empty_immutable_mountpoint(&path).map_err(|err| { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is on the creation path. The dataset does not exist yet.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah okay, the "wants_mounting" is a little confusing but I understand now. Maybe add a comment that mentions creating the immutable mountpoint for future mounting.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I have a comment on lines 1012 - 1019 which tries to cover this: Do you have recommendations for how to make this more clear? My read of this is that it already does mention "creating the immutable mountpoint for future mounting", in particular:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I can re-shuffle the comment to go alongside the |
||
| EnsureDatasetErrorRaw::MountpointCreation { | ||
| mountpoint: path.to_path_buf(), | ||
| err, | ||
| } | ||
| })?; | ||
| } | ||
|
|
||
| let mut command = std::process::Command::new(PFEXEC); | ||
|
|
@@ -1048,7 +1103,7 @@ impl Zfs { | |
| fn dataset_exists( | ||
| name: &str, | ||
| mountpoint: &Mountpoint, | ||
| ) -> Result<(bool, bool), EnsureDatasetErrorRaw> { | ||
| ) -> Result<DatasetMountInfo, EnsureDatasetErrorRaw> { | ||
| let mut command = std::process::Command::new(ZFS); | ||
| let cmd = command.args(&[ | ||
| "list", | ||
|
|
@@ -1064,9 +1119,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()) | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It simplifies logic elsewhere if this is "always a path". I went ahead and updated this, since there appear to be no users of "legacy".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dataset mounted at "/" will actually show up as a legacy mountpoint, however I did a quick spot check and it doesn't look like we use this anywhere when reading from zfs to get a mountpoint.