Skip to content

Commit

Permalink
Use local properties when querying oxide values (#6988)
Browse files Browse the repository at this point in the history
We use Oxide-specific ZFS properties to attach additional metadata to
datasets, such as UUIDs.

Unfortunately, by default, when we query these properties, the
information is inherited.

This means that, for example, if we try to set the "oxide:uuid" property
on a transient zone filesystem root, say,

```
oxp_7b24095a-72df-45e3-984f-2b795e052ac7/crypt/zone
```

And we have a dataset within that existing dataset which **does not**
have the "oxide:uuid" property set, at, say:

```
oxp_7b24095a-72df-45e3-984f-2b795e052ac7/crypt/zone/oxz_crucible_01f93020-7e7d-4185-93fb-6ca234056c82
```

Then when we query `zfs get oxide:uuid` on each of these datasets, we'll
see the UUID of the transient root for **both** datasets.

By using the `-s local` argument to ZFS get, we prevent this from
happening - now, these properties will only be accessible for the
dataset where they are directly set.
  • Loading branch information
smklein authored Nov 5, 2024
1 parent 122eccf commit 7ca80f1
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 3 deletions.
44 changes: 41 additions & 3 deletions illumos-utils/src/zfs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,28 @@ impl FromStr for DatasetProperties {
}
}

#[derive(Debug, Copy, Clone)]
pub enum PropertySource {
Local,
Default,
Inherited,
Temporary,
None,
}

impl fmt::Display for PropertySource {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let ps = match self {
PropertySource::Local => "local",
PropertySource::Default => "default",
PropertySource::Inherited => "inherited",
PropertySource::Temporary => "temporary",
PropertySource::None => "none",
};
write!(f, "{ps}")
}
}

#[cfg_attr(any(test, feature = "testing"), mockall::automock, allow(dead_code))]
impl Zfs {
/// Lists all datasets within a pool or existing dataset.
Expand Down Expand Up @@ -646,15 +668,21 @@ impl Zfs {
filesystem_name: &str,
name: &str,
) -> Result<String, GetValueError> {
Zfs::get_value(filesystem_name, &format!("oxide:{}", name))
let property = format!("oxide:{name}");
let [value] = Self::get_values(
filesystem_name,
&[&property],
Some(PropertySource::Local),
)?;
Ok(value)
}

/// Calls "zfs get" with a single value
pub fn get_value(
filesystem_name: &str,
name: &str,
) -> Result<String, GetValueError> {
let [value] = Self::get_values(filesystem_name, &[name])?;
let [value] = Self::get_values(filesystem_name, &[name], None)?;
Ok(value)
}

Expand Down Expand Up @@ -722,14 +750,24 @@ impl Zfs {
// These methods don't work with mockall, so they exist in a separate impl block
impl Zfs {
/// Calls "zfs get" to acquire multiple values
///
/// - `names`: The properties being acquired
/// - `source`: The optioanl property source (origin of the property)
/// Defaults to "all sources" when unspecified.
pub fn get_values<const N: usize>(
filesystem_name: &str,
names: &[&str; N],
source: Option<PropertySource>,
) -> Result<[String; N], GetValueError> {
let mut cmd = std::process::Command::new(PFEXEC);
let all_names =
names.into_iter().map(|n| *n).collect::<Vec<&str>>().join(",");
cmd.args(&[ZFS, "get", "-Ho", "value", &all_names, filesystem_name]);

cmd.args(&[ZFS, "get", "-Ho", "value", &all_names]);
if let Some(source) = source {
cmd.args(&["-s", &source.to_string()]);
}
cmd.arg(filesystem_name);
let output = execute(&mut cmd).map_err(|err| GetValueError {
filesystem: filesystem_name.to_string(),
name: format!("{:?}", names),
Expand Down
1 change: 1 addition & 0 deletions sled-agent/src/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3619,6 +3619,7 @@ impl ServiceManager {
illumos_utils::zfs::Zfs::get_values(
&dataset.full_name(),
&["zoned", "canmount", "encryption"],
None,
)
.map_err(|err| Error::GetZfsValue {
zone: zone.zone_name(),
Expand Down

0 comments on commit 7ca80f1

Please sign in to comment.