From 8b4093df67393dc8a05c8d94f92204115b6c759e Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 10 Dec 2024 17:27:08 -0800 Subject: [PATCH 1/9] Fix get_dataset_properties to avoid propagating inherited UUIDs --- illumos-utils/src/zfs.rs | 343 ++++++++++++++++++++++++++++----------- 1 file changed, 247 insertions(+), 96 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index fa09fb22c5..78b2d209f6 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -5,14 +5,15 @@ //! Utilities for poking at ZFS. use crate::{execute, PFEXEC}; +use anyhow::anyhow; use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; use omicron_common::api::external::ByteCount; use omicron_common::disk::CompressionAlgorithm; use omicron_common::disk::DiskIdentity; use omicron_uuid_kinds::DatasetUuid; +use std::collections::BTreeMap; use std::fmt; -use std::str::FromStr; // These locations in the ramdisk must only be used by the switch zone. // @@ -236,56 +237,123 @@ pub struct DatasetProperties { } impl DatasetProperties { - // care about. - const ZFS_LIST_STR: &'static str = + const ZFS_GET_PROPS: &'static str = "oxide:uuid,name,avail,used,quota,reservation,compression"; } -// An inner parsing function, so that the FromStr implementation can always emit -// the string 's' that failed to parse in the error message. -fn dataset_properties_parse( - s: &str, -) -> Result { - let mut iter = s.split_whitespace(); +impl DatasetProperties { + /// Parses dataset properties, assuming that the caller is providing the + /// output of the following command as stdout: + /// + /// zfs get -rpo name,property,value,source $ZFS_GET_PROPS $DATASETS + fn parse_many( + stdout: &str, + ) -> Result, anyhow::Error> { + let name_prop_val_source_list = stdout.trim().split('\n'); - let id = match iter.next().context("Missing UUID")? { - "-" => None, - anything_else => Some(anything_else.parse::()?), - }; + let mut datasets: BTreeMap<&str, BTreeMap<&str, _>> = BTreeMap::new(); + for name_prop_val_source in name_prop_val_source_list { + let mut iter = name_prop_val_source.split_whitespace(); - let name = iter.next().context("Missing 'name'")?.to_string(); - let avail = - iter.next().context("Missing 'avail'")?.parse::()?.try_into()?; - let used = - iter.next().context("Missing 'used'")?.parse::()?.try_into()?; - let quota = match iter.next().context("Missing 'quota'")?.parse::()? { - 0 => None, - q => Some(q.try_into()?), - }; - let reservation = - match iter.next().context("Missing 'reservation'")?.parse::()? { - 0 => None, - r => Some(r.try_into()?), - }; - let compression = iter.next().context("Missing 'compression'")?.to_string(); - - Ok(DatasetProperties { - id, - name, - avail, - used, - quota, - reservation, - compression, - }) -} + let (name, prop, val, source) = ( + iter.next().context("Missing 'name'")?, + iter.next().context("Missing 'property'")?, + iter.next().context("Missing 'value'")?, + iter.next().context("Missing 'source'")?, + ); -impl FromStr for DatasetProperties { - type Err = anyhow::Error; + let props = datasets.entry(name).or_default(); + props.insert(prop, (val, source)); + } - fn from_str(s: &str) -> Result { - dataset_properties_parse(s) - .with_context(|| format!("Failed to parse: {s}")) + datasets + .into_iter() + .map(|(dataset_name, props)| { + // Dataset UUIDs are properties that are optionally attached to + // datasets. However, some datasets are nested - to avoid them + // from propagating, we explicitly ignore this value if it is + // inherited. + // + // This can be the case for the "zone" filesystem root, which + // can propagate this property to a child zone without it set. + let id = match props.get("oxide:uuid") { + Some((prop, source)) => { + if *source == "inherited" { + None + } else { + Some( + prop.parse::() + .context("Failed to parse UUID")?, + ) + } + } + None => None, + }; + let name = dataset_name.to_string(); + let avail = props + .get("available") + .map(|(prop, _source)| prop) + .ok_or(anyhow!("Missing 'available'"))? + .parse::() + .context("Failed to parse 'available'")? + .try_into()?; + let used = props + .get("used") + .map(|(prop, _source)| prop) + .ok_or(anyhow!("Missing 'used'"))? + .parse::() + .context("Failed to parse 'used'")? + .try_into()?; + let quota = match props.get("quota") { + // If a quota has not been set explicitly, it has a default + // source and a value of "zero". Rather than parsing the value + // as zero, it should be ignored. + Some((prop, source)) => { + if *source == "default" { + None + } else { + Some( + prop.parse::() + .context("Failed to parse 'quota'")? + .try_into()?, + ) + } + } + None => None, + }; + let reservation = match props.get("reservation") { + // If a reservation has not been set explicitly, it has a default + // source and a value of "zero". Rather than parsing the value + // as zero, it should be ignored. + Some((prop, source)) => { + if *source == "default" { + None + } else { + Some( + prop.parse::() + .context("Failed to parse 'reservation'")? + .try_into()?, + ) + } + } + None => None, + }; + let compression = props + .get("compression") + .map(|(prop, _source)| prop.to_string()) + .ok_or_else(|| anyhow!("Missing 'compression'"))?; + + Ok(DatasetProperties { + id, + name, + avail, + used, + quota, + reservation, + compression, + }) + }) + .collect::, _>>() } } @@ -344,26 +412,24 @@ impl Zfs { datasets: &[String], ) -> Result, anyhow::Error> { let mut command = std::process::Command::new(ZFS); - let cmd = command.args(&["list", "-d", "1", "-rHpo"]); + let cmd = command.args(&[ + "get", + "-d", + "1", + "-rHpo", + "name,property,value,source", + ]); // Note: this is tightly coupled with the layout of DatasetProperties - cmd.arg(DatasetProperties::ZFS_LIST_STR); + cmd.arg(DatasetProperties::ZFS_GET_PROPS); cmd.args(datasets); let output = execute(cmd).with_context(|| { format!("Failed to get dataset properties for {datasets:?}") })?; let stdout = String::from_utf8(output.stdout)?; - let mut datasets = stdout - .trim() - .split('\n') - .map(|row| row.parse::()) - .collect::, _>>()?; - - datasets.sort_by(|d1, d2| d1.name.partial_cmp(&d2.name).unwrap()); - datasets.dedup_by(|d1, d2| d1.name.eq(&d2.name)); - Ok(datasets) + DatasetProperties::parse_many(&stdout) } /// Return the name of a dataset for a ZFS object. @@ -859,42 +925,54 @@ mod test { #[test] fn parse_dataset_props() { - let input = - "- dataset_name 1234 5678 0 0 off"; - let props = DatasetProperties::from_str(&input) + let input = "dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name name I_AM_IGNORED -\n\ + dataset_name compression off inherited from parent"; + let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); - - assert_eq!(props.id, None); - assert_eq!(props.name, "dataset_name"); - assert_eq!(props.avail.to_bytes(), 1234); - assert_eq!(props.used.to_bytes(), 5678); - assert_eq!(props.quota, None); - assert_eq!(props.reservation, None); - assert_eq!(props.compression, "off"); + assert_eq!(props.len(), 1); + + assert_eq!(props[0].id, None); + assert_eq!(props[0].name, "dataset_name"); + assert_eq!(props[0].avail.to_bytes(), 1234); + assert_eq!(props[0].used.to_bytes(), 5678); + assert_eq!(props[0].quota, None); + assert_eq!(props[0].reservation, None); + assert_eq!(props[0].compression, "off"); } #[test] fn parse_dataset_props_with_optionals() { - let input = "d4e1e554-7b98-4413-809e-4a42561c3d0c dataset_name 1234 5678 111 222 off"; - let props = DatasetProperties::from_str(&input) + let input = + "dataset_name oxide:uuid d4e1e554-7b98-4413-809e-4a42561c3d0c local\n\ + dataset_name available 1234 -\n\ + dataset_name used 5678 - \n\ + dataset_name quota 111 -\n\ + dataset_name reservation 222 -\n\ + dataset_name compression off inherited from parent"; + let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); - + assert_eq!(props.len(), 1); assert_eq!( - props.id, + props[0].id, Some("d4e1e554-7b98-4413-809e-4a42561c3d0c".parse().unwrap()) ); - assert_eq!(props.name, "dataset_name"); - assert_eq!(props.avail.to_bytes(), 1234); - assert_eq!(props.used.to_bytes(), 5678); - assert_eq!(props.quota.map(|q| q.to_bytes()), Some(111)); - assert_eq!(props.reservation.map(|r| r.to_bytes()), Some(222)); - assert_eq!(props.compression, "off"); + assert_eq!(props[0].name, "dataset_name"); + assert_eq!(props[0].avail.to_bytes(), 1234); + assert_eq!(props[0].used.to_bytes(), 5678); + assert_eq!(props[0].quota.map(|q| q.to_bytes()), Some(111)); + assert_eq!(props[0].reservation.map(|r| r.to_bytes()), Some(222)); + assert_eq!(props[0].compression, "off"); } #[test] fn parse_dataset_bad_uuid() { - let input = "bad dataset_name 1234 5678 111 222 off"; - let err = DatasetProperties::from_str(&input) + let input = "dataset_name oxide:uuid bad -\n\ + dataset_name available 1234 -\n\ + dataset_name used 5678 -"; + + let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( format!("{err:#}").contains("error parsing UUID (dataset)"), @@ -904,8 +982,9 @@ mod test { #[test] fn parse_dataset_bad_avail() { - let input = "- dataset_name BADAVAIL 5678 111 222 off"; - let err = DatasetProperties::from_str(&input) + let input = "dataset_name available BADAVAIL -\n\ + dataset_name used 5678 -"; + let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( format!("{err:#}").contains("invalid digit found in string"), @@ -915,8 +994,9 @@ mod test { #[test] fn parse_dataset_bad_usage() { - let input = "- dataset_name 1234 BADUSAGE 111 222 off"; - let err = DatasetProperties::from_str(&input) + let input = "dataset_name available 1234 -\n\ + dataset_name used BADUSAGE -"; + let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( format!("{err:#}").contains("invalid digit found in string"), @@ -926,8 +1006,10 @@ mod test { #[test] fn parse_dataset_bad_quota() { - let input = "- dataset_name 1234 5678 BADQUOTA 222 off"; - let err = DatasetProperties::from_str(&input) + let input = "dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name quota BADQUOTA -"; + let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( format!("{err:#}").contains("invalid digit found in string"), @@ -937,8 +1019,11 @@ mod test { #[test] fn parse_dataset_bad_reservation() { - let input = "- dataset_name 1234 5678 111 BADRES off"; - let err = DatasetProperties::from_str(&input) + let input = "dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name quota 111 -\n\ + dataset_name reservation BADRES -"; + let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( format!("{err:#}").contains("invalid digit found in string"), @@ -949,24 +1034,90 @@ mod test { #[test] fn parse_dataset_missing_fields() { let expect_missing = |input: &str, what: &str| { - let err = DatasetProperties::from_str(input) + let err = DatasetProperties::parse_many(input) .expect_err("Should have failed to parse"); let err = format!("{err:#}"); assert!(err.contains(&format!("Missing {what}")), "{err}"); }; expect_missing( - "- dataset_name 1234 5678 111 222", - "'compression'", + "dataset_name used 5678 -\n\ + dataset_name quota 111 -\n\ + dataset_name reservation 222 -\n\ + dataset_name compression off inherited", + "'available'", ); expect_missing( - "- dataset_name 1234 5678 111", - "'reservation'", + "dataset_name available 1234 -\n\ + dataset_name quota 111 -\n\ + dataset_name reservation 222 -\n\ + dataset_name compression off inherited", + "'used'", + ); + expect_missing( + "dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name quota 111 -\n\ + dataset_name reservation 222 -", + "'compression'", ); - expect_missing("- dataset_name 1234 5678", "'quota'"); - expect_missing("- dataset_name 1234", "'used'"); - expect_missing("- dataset_name", "'avail'"); - expect_missing("-", "'name'"); - expect_missing("", "UUID"); + } + + #[test] + fn parse_dataset_uuid_ignored_if_inherited() { + let input = + "dataset_name oxide:uuid b8698ede-60c2-4e16-b792-d28c165cfd12 inherited from parent\n\ + dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name compression off -"; + let props = DatasetProperties::parse_many(&input) + .expect("Should have parsed data"); + assert_eq!(props.len(), 1); + assert_eq!(props[0].id, None); + } + + #[test] + fn parse_quota_ignored_if_default() { + let input = "dataset_name quota 0 default\n\ + dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name compression off -"; + let props = DatasetProperties::parse_many(&input) + .expect("Should have parsed data"); + assert_eq!(props.len(), 1); + assert_eq!(props[0].quota, None); + } + + #[test] + fn parse_reservation_ignored_if_default() { + let input = "dataset_name reservation 0 default\n\ + dataset_name available 1234 -\n\ + dataset_name used 5678 -\n\ + dataset_name compression off -"; + let props = DatasetProperties::parse_many(&input) + .expect("Should have parsed data"); + assert_eq!(props.len(), 1); + assert_eq!(props[0].reservation, None); + } + + #[test] + fn parse_sorts_and_dedups() { + let input = "foo available 111 -\n\ + foo used 111 -\n\ + foo compression off -\n\ + foo available 111 -\n\ + foo used 111 -\n\ + foo compression off -\n\ + bar available 222 -\n\ + bar used 222 -\n\ + bar compression off -"; + + let props = DatasetProperties::parse_many(&input) + .expect("Should have parsed data"); + assert_eq!(props.len(), 2); + assert_eq!(props[0].name, "bar"); + assert_eq!(props[0].used, 222.into()); + assert_eq!(props[1].name, "foo"); + assert_eq!(props[1].used, 111.into()); } } From de9b7a553848764e13c480cc0fdaf57f39daf44d Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 11 Dec 2024 11:51:35 -0800 Subject: [PATCH 2/9] Review feedback --- illumos-utils/src/zfs.rs | 249 +++++++++++++++++++++------------------ 1 file changed, 136 insertions(+), 113 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 78b2d209f6..f9edb8de86 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -6,6 +6,7 @@ use crate::{execute, PFEXEC}; use anyhow::anyhow; +use anyhow::bail; use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; use omicron_common::api::external::ByteCount; @@ -253,7 +254,9 @@ impl DatasetProperties { let mut datasets: BTreeMap<&str, BTreeMap<&str, _>> = BTreeMap::new(); for name_prop_val_source in name_prop_val_source_list { - let mut iter = name_prop_val_source.split_whitespace(); + // "-H" indicates that these columns are tab-separated; + // each column may internally have whitespace. + let mut iter = name_prop_val_source.split('\t'); let (name, prop, val, source) = ( iter.next().context("Missing 'name'")?, @@ -261,6 +264,9 @@ impl DatasetProperties { iter.next().context("Missing 'value'")?, iter.next().context("Missing 'source'")?, ); + if let Some(extra) = iter.next() { + bail!("Unexpected column data: '{extra}'"); + } let props = datasets.entry(name).or_default(); props.insert(prop, (val, source)); @@ -269,26 +275,23 @@ impl DatasetProperties { datasets .into_iter() .map(|(dataset_name, props)| { - // Dataset UUIDs are properties that are optionally attached to - // datasets. However, some datasets are nested - to avoid them - // from propagating, we explicitly ignore this value if it is - // inherited. - // - // This can be the case for the "zone" filesystem root, which - // can propagate this property to a child zone without it set. - let id = match props.get("oxide:uuid") { - Some((prop, source)) => { - if *source == "inherited" { - None - } else { - Some( - prop.parse::() - .context("Failed to parse UUID")?, - ) - } - } - None => None, - }; + let id = props + .get("oxide:uuid") + .filter(|(prop, source)| { + // Dataset UUIDs are properties that are optionally attached to + // datasets. However, some datasets are nested - to avoid them + // from propagating, we explicitly ignore this value if it is + // inherited. + // + // This can be the case for the "zone" filesystem root, which + // can propagate this property to a child zone without it set. + !source.starts_with("inherited") && *prop != "-" + }) + .map(|(prop, _source)| { + prop.parse::() + .context("Failed to parse UUID") + }) + .transpose()?; let name = dataset_name.to_string(); let avail = props .get("available") @@ -304,40 +307,33 @@ impl DatasetProperties { .parse::() .context("Failed to parse 'used'")? .try_into()?; - let quota = match props.get("quota") { - // If a quota has not been set explicitly, it has a default - // source and a value of "zero". Rather than parsing the value - // as zero, it should be ignored. - Some((prop, source)) => { - if *source == "default" { - None - } else { - Some( - prop.parse::() - .context("Failed to parse 'quota'")? - .try_into()?, - ) - } - } - None => None, - }; - let reservation = match props.get("reservation") { - // If a reservation has not been set explicitly, it has a default - // source and a value of "zero". Rather than parsing the value - // as zero, it should be ignored. - Some((prop, source)) => { - if *source == "default" { - None - } else { - Some( - prop.parse::() - .context("Failed to parse 'reservation'")? - .try_into()?, - ) - } - } - None => None, - }; + let quota = props + .get("quota") + .filter(|(_prop, source)| { + // If a quota has not been set explicitly, it has a default + // source and a value of "zero". Rather than parsing the value + // as zero, it should be ignored. + *source != "default" + }) + .map(|(prop, _source)| { + prop.parse::().context("Failed to parse 'quota'") + }) + .transpose()? + .and_then(|v| ByteCount::try_from(v).ok()); + let reservation = props + .get("reservation") + .filter(|(_prop, source)| { + // If a reservation has not been set explicitly, it has a default + // source and a value of "zero". Rather than parsing the value + // as zero, it should be ignored. + *source != "default" + }) + .map(|(prop, _source)| { + prop.parse::() + .context("Failed to parse 'reservation'") + }) + .transpose()? + .and_then(|v| ByteCount::try_from(v).ok()); let compression = props .get("compression") .map(|(prop, _source)| prop.to_string()) @@ -403,6 +399,7 @@ impl Zfs { } /// Get information about datasets within a list of zpools / datasets. + /// Returns properties for all input datasets and their direct children. /// /// This function is similar to [Zfs::list_datasets], but provides a more /// substantial results about the datasets found. @@ -416,7 +413,7 @@ impl Zfs { "get", "-d", "1", - "-rHpo", + "-Hpo", "name,property,value,source", ]); @@ -925,10 +922,10 @@ mod test { #[test] fn parse_dataset_props() { - let input = "dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name name I_AM_IGNORED -\n\ - dataset_name compression off inherited from parent"; + let input = "dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tname\tI_AM_IGNORED\t-\n\ + dataset_name\tcompression\toff\tinherited from parent"; let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); assert_eq!(props.len(), 1); @@ -942,15 +939,29 @@ mod test { assert_eq!(props[0].compression, "off"); } + #[test] + fn parse_dataset_too_many_columns() { + let input = "dataset_name\tavailable\t1234\t-\tEXTRA\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tname\tI_AM_IGNORED\t-\n\ + dataset_name\tcompression\toff\tinherited from parent"; + let err = DatasetProperties::parse_many(&input) + .expect_err("Should have parsed data"); + assert!( + err.to_string().contains("Unexpected column data: 'EXTRA'"), + "{err}" + ); + } + #[test] fn parse_dataset_props_with_optionals() { let input = - "dataset_name oxide:uuid d4e1e554-7b98-4413-809e-4a42561c3d0c local\n\ - dataset_name available 1234 -\n\ - dataset_name used 5678 - \n\ - dataset_name quota 111 -\n\ - dataset_name reservation 222 -\n\ - dataset_name compression off inherited from parent"; + "dataset_name\toxide:uuid\td4e1e554-7b98-4413-809e-4a42561c3d0c\tlocal\n\ + dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tquota\t111\t-\n\ + dataset_name\treservation\t222\t-\n\ + dataset_name\tcompression\toff\tinherited from parent"; let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); assert_eq!(props.len(), 1); @@ -968,9 +979,9 @@ mod test { #[test] fn parse_dataset_bad_uuid() { - let input = "dataset_name oxide:uuid bad -\n\ - dataset_name available 1234 -\n\ - dataset_name used 5678 -"; + let input = "dataset_name\toxide:uuid\tbad\t-\n\ + dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-"; let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); @@ -982,8 +993,8 @@ mod test { #[test] fn parse_dataset_bad_avail() { - let input = "dataset_name available BADAVAIL -\n\ - dataset_name used 5678 -"; + let input = "dataset_name\tavailable\tBADAVAIL\t-\n\ + dataset_name\tused\t5678\t-"; let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( @@ -994,8 +1005,8 @@ mod test { #[test] fn parse_dataset_bad_usage() { - let input = "dataset_name available 1234 -\n\ - dataset_name used BADUSAGE -"; + let input = "dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\tBADUSAGE\t-"; let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( @@ -1006,9 +1017,9 @@ mod test { #[test] fn parse_dataset_bad_quota() { - let input = "dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name quota BADQUOTA -"; + let input = "dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tquota\tBADQUOTA\t-"; let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( @@ -1019,10 +1030,10 @@ mod test { #[test] fn parse_dataset_bad_reservation() { - let input = "dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name quota 111 -\n\ - dataset_name reservation BADRES -"; + let input = "dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tquota\t111\t-\n\ + dataset_name\treservation\tBADRES\t-"; let err = DatasetProperties::parse_many(&input) .expect_err("Should have failed to parse"); assert!( @@ -1041,24 +1052,24 @@ mod test { }; expect_missing( - "dataset_name used 5678 -\n\ - dataset_name quota 111 -\n\ - dataset_name reservation 222 -\n\ - dataset_name compression off inherited", + "dataset_name\tused\t5678\t-\n\ + dataset_name\tquota\t111\t-\n\ + dataset_name\treservation\t222\t-\n\ + dataset_name\tcompression\toff\tinherited", "'available'", ); expect_missing( - "dataset_name available 1234 -\n\ - dataset_name quota 111 -\n\ - dataset_name reservation 222 -\n\ - dataset_name compression off inherited", + "dataset_name\tavailable\t1234\t-\n\ + dataset_name\tquota\t111\t-\n\ + dataset_name\treservation\t222\t-\n\ + dataset_name\tcompression\toff\tinherited", "'used'", ); expect_missing( - "dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name quota 111 -\n\ - dataset_name reservation 222 -", + "dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tquota\t111\t-\n\ + dataset_name\treservation\t222\t-", "'compression'", ); } @@ -1066,10 +1077,22 @@ mod test { #[test] fn parse_dataset_uuid_ignored_if_inherited() { let input = - "dataset_name oxide:uuid b8698ede-60c2-4e16-b792-d28c165cfd12 inherited from parent\n\ - dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name compression off -"; + "dataset_name\toxide:uuid\tb8698ede-60c2-4e16-b792-d28c165cfd12\tinherited from parent\n\ + dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tcompression\toff\t-"; + let props = DatasetProperties::parse_many(&input) + .expect("Should have parsed data"); + assert_eq!(props.len(), 1); + assert_eq!(props[0].id, None); + } + + #[test] + fn parse_dataset_uuid_ignored_if_dash() { + let input = "dataset_name\toxide:uuid\t-\t-\n\ + dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tcompression\toff\t-"; let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); assert_eq!(props.len(), 1); @@ -1078,10 +1101,10 @@ mod test { #[test] fn parse_quota_ignored_if_default() { - let input = "dataset_name quota 0 default\n\ - dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name compression off -"; + let input = "dataset_name\tquota\t0\tdefault\n\ + dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tcompression\toff\t-"; let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); assert_eq!(props.len(), 1); @@ -1090,10 +1113,10 @@ mod test { #[test] fn parse_reservation_ignored_if_default() { - let input = "dataset_name reservation 0 default\n\ - dataset_name available 1234 -\n\ - dataset_name used 5678 -\n\ - dataset_name compression off -"; + let input = "dataset_name\treservation\t0\tdefault\n\ + dataset_name\tavailable\t1234\t-\n\ + dataset_name\tused\t5678\t-\n\ + dataset_name\tcompression\toff\t-"; let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); assert_eq!(props.len(), 1); @@ -1102,15 +1125,15 @@ mod test { #[test] fn parse_sorts_and_dedups() { - let input = "foo available 111 -\n\ - foo used 111 -\n\ - foo compression off -\n\ - foo available 111 -\n\ - foo used 111 -\n\ - foo compression off -\n\ - bar available 222 -\n\ - bar used 222 -\n\ - bar compression off -"; + let input = "foo\tavailable\t111\t-\n\ + foo\tused\t111\t-\n\ + foo\tcompression\toff\t-\n\ + foo\tavailable\t111\t-\n\ + foo\tused\t111\t-\n\ + foo\tcompression\toff\t-\n\ + bar\tavailable\t222\t-\n\ + bar\tused\t222\t-\n\ + bar\tcompression\toff\t-"; let props = DatasetProperties::parse_many(&input) .expect("Should have parsed data"); From ab2d682c51c4db7c502bead447f1729b29479936 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 11 Dec 2024 13:25:23 -0800 Subject: [PATCH 3/9] Optimizing storage manager --- sled-storage/src/manager.rs | 147 ++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 8 deletions(-) diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index a760285d3f..1d8eaaf4a0 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -891,6 +891,19 @@ impl StorageManager { generation: config.generation, }); } + + // The ledger already was written, and the datasets were + // ensured for this generation. Exit early. + return Ok(DatasetsManagementResult { + status: config + .datasets + .into_values() + .map(|dataset| DatasetManagementStatus { + dataset_name: dataset.name, + err: None, + }) + .collect(), + }); } else { info!( log, @@ -933,15 +946,17 @@ impl StorageManager { log: &Logger, config: &DatasetsConfig, ) -> DatasetsManagementResult { - let mut status = vec![]; - for dataset in config.datasets.values() { - status.push(self.dataset_ensure_internal(log, dataset).await); - } + // Ensure each dataset concurrently + let futures = config.datasets.values().map(|dataset| async { + self.dataset_ensure_internal(log, dataset).await + }); + + let status = futures::future::join_all(futures).await; DatasetsManagementResult { status } } async fn dataset_ensure_internal( - &mut self, + &self, log: &Logger, config: &DatasetConfig, ) -> DatasetManagementStatus { @@ -1294,7 +1309,7 @@ impl StorageManager { // Invokes [Self::ensure_dataset] and also ensures the dataset has an // expected UUID as a ZFS property. async fn ensure_dataset_with_id( - &mut self, + &self, zpool: &ZpoolName, id: DatasetUuid, config: &SharedDatasetConfig, @@ -1323,7 +1338,7 @@ impl StorageManager { // // Confirms that the zpool exists and is managed by this sled. async fn ensure_dataset( - &mut self, + &self, zpool: &ZpoolName, config: &SharedDatasetConfig, details: &DatasetCreationDetails, @@ -1428,7 +1443,8 @@ impl StorageManager { /// All tests only use synthetic disks, but are expected to be run on illumos /// systems. -#[cfg(all(test, target_os = "illumos"))] +// #[cfg(all(test, target_os = "illumos"))] +#[cfg(test)] mod tests { use crate::disk::RawSyntheticDisk; use crate::manager_test_harness::StorageManagerTestHarness; @@ -1996,6 +2012,121 @@ mod tests { logctx.cleanup_successful(); } + #[tokio::test] + async fn ensure_many_datasets() { + illumos_utils::USE_MOCKS.store(false, Ordering::SeqCst); + let logctx = test_setup_log("ensure_many_datasets"); + let mut harness = StorageManagerTestHarness::new(&logctx.log).await; + + // Test setup: Add U.2s and an M.2, adopt them into the "control plane" + // for usage. + harness.handle().key_manager_ready().await; + let raw_disks = harness + .add_vdevs(&[ + "u2_0.vdev", + "u2_1.vdev", + "u2_2.vdev", + "u2_3.vdev", + "u2_4.vdev", + "u2_5.vdev", + "u2_6.vdev", + "u2_7.vdev", + "u2_8.vdev", + "u2_9.vdev", + "m2_helping.vdev", + ]) + .await; + let config = harness.make_config(1, &raw_disks); + + println!("Ensuring physical disks..."); + let result = harness + .handle() + .omicron_physical_disks_ensure(config.clone()) + .await + .expect("Ensuring disks should work after key manager is ready"); + assert!(!result.has_error(), "{:?}", result); + println!("OK"); + + println!("Ensuring physical disks again..."); + let result = harness + .handle() + .omicron_physical_disks_ensure(config.clone()) + .await + .expect("Ensuring disks should work after key manager is ready"); + assert!(!result.has_error(), "{:?}", result); + println!("OK"); + + // Create datasets on the newly formatted U.2s + let mut datasets = BTreeMap::new(); + for i in 0..10 { + let zpool_name = ZpoolName::new_external(config.disks[i].pool_id); + + let id = DatasetUuid::new_v4(); + let name = + DatasetName::new(zpool_name.clone(), DatasetKind::Crucible); + datasets.insert( + id, + DatasetConfig { + id, + name, + inner: SharedDatasetConfig::default(), + }, + ); + + let id = DatasetUuid::new_v4(); + let name = DatasetName::new(zpool_name.clone(), DatasetKind::Debug); + datasets.insert( + id, + DatasetConfig { + id, + name, + inner: SharedDatasetConfig::default(), + }, + ); + + let id = DatasetUuid::new_v4(); + let name = DatasetName::new( + zpool_name.clone(), + DatasetKind::TransientZoneRoot, + ); + datasets.insert( + id, + DatasetConfig { + id, + name, + inner: SharedDatasetConfig::default(), + }, + ); + } + // "Generation = 1" is reserved as "no requests seen yet", so we jump + // past it. + let generation = Generation::new().next(); + let config = DatasetsConfig { generation, datasets }; + + println!("Ensuring datasets... "); + let status = + harness.handle().datasets_ensure(config.clone()).await.unwrap(); + assert!(!status.has_error()); + println!("OK"); + + // List datasets, expect to see what we just created + println!("List... "); + let observed_config = + harness.handle().datasets_config_list().await.unwrap(); + assert_eq!(config, observed_config); + println!("OK"); + + // Calling "datasets_ensure" with the same input should succeed. + println!("Ensuring datasets again... "); + let status = + harness.handle().datasets_ensure(config.clone()).await.unwrap(); + assert!(!status.has_error()); + println!("OK"); + + harness.cleanup().await; + logctx.cleanup_successful(); + } + #[tokio::test] async fn nested_dataset() { illumos_utils::USE_MOCKS.store(false, Ordering::SeqCst); From e1eca0a38958ba805b56aa55029bcf6bfedad13c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 11 Dec 2024 17:45:23 -0800 Subject: [PATCH 4/9] Optimizations for ensuring datasets --- Cargo.lock | 1 + illumos-utils/Cargo.toml | 1 + illumos-utils/src/zfs.rs | 240 ++++++++++++++++++++---------------- sled-storage/src/dataset.rs | 2 + sled-storage/src/error.rs | 3 + sled-storage/src/manager.rs | 135 ++++++++++++-------- 6 files changed, 228 insertions(+), 154 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8c374f5609..c42be5e5f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4464,6 +4464,7 @@ dependencies = [ "futures", "http", "ipnetwork", + "itertools 0.13.0", "libc", "macaddr", "mockall", diff --git a/illumos-utils/Cargo.toml b/illumos-utils/Cargo.toml index e1421bd3ab..69276713bb 100644 --- a/illumos-utils/Cargo.toml +++ b/illumos-utils/Cargo.toml @@ -21,6 +21,7 @@ dropshot.workspace = true futures.workspace = true http.workspace = true ipnetwork.workspace = true +itertools.workspace = true libc.workspace = true macaddr.workspace = true omicron-common.workspace = true diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index f9edb8de86..13b55c2b48 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -9,9 +9,11 @@ use anyhow::anyhow; use anyhow::bail; use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; +use itertools::Itertools; use omicron_common::api::external::ByteCount; use omicron_common::disk::CompressionAlgorithm; use omicron_common::disk::DiskIdentity; +use omicron_common::disk::SharedDatasetConfig; use omicron_uuid_kinds::DatasetUuid; use std::collections::BTreeMap; use std::fmt; @@ -83,11 +85,10 @@ enum EnsureFilesystemErrorRaw { /// Error returned by [`Zfs::ensure_filesystem`]. #[derive(thiserror::Error, Debug)] #[error( - "Failed to ensure filesystem '{name}' exists at '{mountpoint:?}': {err}" + "Failed to ensure filesystem '{name}': {err}" )] pub struct EnsureFilesystemError { name: String, - mountpoint: Mountpoint, #[source] err: EnsureFilesystemErrorRaw, } @@ -95,12 +96,11 @@ pub struct EnsureFilesystemError { /// Error returned by [`Zfs::set_oxide_value`] #[derive(thiserror::Error, Debug)] #[error( - "Failed to set value '{name}={value}' on filesystem {filesystem}: {err}" + "Failed to set values '{values}' on filesystem {filesystem}: {err}" )] pub struct SetValueError { filesystem: String, - name: String, - value: String, + values: String, err: crate::ExecutionError, } @@ -242,6 +242,18 @@ impl DatasetProperties { "oxide:uuid,name,avail,used,quota,reservation,compression"; } +impl TryFrom<&DatasetProperties> for SharedDatasetConfig { + type Error = anyhow::Error; + + fn try_from(props: &DatasetProperties) -> Result { + Ok(SharedDatasetConfig { + compression: props.compression.parse()?, + quota: props.quota, + reservation: props.reservation, + }) + } +} + impl DatasetProperties { /// Parses dataset properties, assuming that the caller is providing the /// output of the following command as stdout: @@ -307,13 +319,17 @@ impl DatasetProperties { .parse::() .context("Failed to parse 'used'")? .try_into()?; + + // The values of "quota" and "reservation" can be either "-" or + // "0" when they are not actually set. To be cautious, we treat + // both of these values as "the value has not been set + // explicitly". As a result, setting either of these values + // explicitly to zero is indistinguishable from setting them + // with a value of "none". let quota = props .get("quota") - .filter(|(_prop, source)| { - // If a quota has not been set explicitly, it has a default - // source and a value of "zero". Rather than parsing the value - // as zero, it should be ignored. - *source != "default" + .filter(|(prop, _source)| { + *prop != "-" && *prop != "0" }) .map(|(prop, _source)| { prop.parse::().context("Failed to parse 'quota'") @@ -322,11 +338,8 @@ impl DatasetProperties { .and_then(|v| ByteCount::try_from(v).ok()); let reservation = props .get("reservation") - .filter(|(_prop, source)| { - // If a reservation has not been set explicitly, it has a default - // source and a value of "zero". Rather than parsing the value - // as zero, it should be ignored. - *source != "default" + .filter(|(prop, _source)| { + *prop != "-" && *prop != "0" }) .map(|(prop, _source)| { prop.parse::() @@ -375,6 +388,53 @@ impl fmt::Display for PropertySource { } } +#[derive(Copy, Clone, Debug)] +pub enum WhichDatasets { + SelfOnly, + SelfAndChildren, +} + +struct PropertySetter { + props: BTreeMap<&'static str, String>, +} + +impl PropertySetter { + fn new() -> Self { + PropertySetter { + props: BTreeMap::new(), + } + } + + fn add_size_details(&mut self, details: SizeDetails) -> &mut Self { + let SizeDetails { quota, reservation, compression } = details; + let quota = quota + .map(|q| q.to_bytes().to_string()) + .unwrap_or_else(|| String::from("none")); + self.props.insert("quota", quota); + + let reservation = reservation + .map(|r| r.to_bytes().to_string()) + .unwrap_or_else(|| String::from("none")); + self.props.insert("reservation", reservation); + + let compression = compression.to_string(); + self.props.insert("compression", compression); + + self + } + + fn add_id(&mut self, id: DatasetUuid) -> &mut Self { + self.props.insert("oxide:uuid", id.to_string()); + self + } + + fn as_vec(&self) -> Vec<(&str, &str)> { + self.props.iter().map(|(k, v)| { + (*k, v.as_str()) + }).collect() + } +} + #[cfg_attr(any(test, feature = "testing"), mockall::automock, allow(dead_code))] impl Zfs { /// Lists all datasets within a pool or existing dataset. @@ -407,12 +467,17 @@ impl Zfs { /// Sorts results and de-duplicates them by name. pub fn get_dataset_properties( datasets: &[String], + which: WhichDatasets, ) -> Result, anyhow::Error> { let mut command = std::process::Command::new(ZFS); - let cmd = command.args(&[ - "get", - "-d", - "1", + let cmd = command.arg("get"); + match which { + WhichDatasets::SelfOnly => (), + WhichDatasets::SelfAndChildren => { + cmd.args(&["-d", "1"]); + } + } + cmd.args(&[ "-Hpo", "name,property,value,source", ]); @@ -421,8 +486,12 @@ impl Zfs { cmd.arg(DatasetProperties::ZFS_GET_PROPS); cmd.args(datasets); - let output = execute(cmd).with_context(|| { - format!("Failed to get dataset properties for {datasets:?}") + // We are intentionally ignoring the output status of this command. + // + // If one or more dataset doesn't exist, we can still read stdout to + // see about the ones that do exist. + let output = cmd.output().map_err(|err| { + anyhow!("Failed to get dataset properties for {datasets:?}: {err:?}") })?; let stdout = String::from_utf8(output.stdout)?; @@ -490,23 +559,27 @@ impl Zfs { do_format: bool, encryption_details: Option, size_details: Option, + id: Option, additional_options: Option>, ) -> Result<(), EnsureFilesystemError> { let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?; + + let mut props = PropertySetter::new(); + if let Some(size_details) = size_details { + props.add_size_details(size_details); + } + if let Some(id) = id { + props.add_id(id); + } + if exists { - if let Some(SizeDetails { quota, reservation, compression }) = - size_details - { - // apply quota and compression mode (in case they've changed across - // sled-agent versions since creation) - Self::apply_properties( - name, - &mountpoint, - quota, - reservation, - compression, - )?; - } + Self::set_values(name, props.as_vec().as_slice()) + .map_err(|err| { + EnsureFilesystemError { + name: name.to_string(), + err: err.err.into(), + } + })?; if encryption_details.is_none() { // If the dataset exists, we're done. Unencrypted datasets are @@ -518,14 +591,13 @@ impl Zfs { return Ok(()); } // We need to load the encryption key and mount the filesystem - return Self::mount_encrypted_dataset(name, &mountpoint); + return Self::mount_encrypted_dataset(name); } } if !do_format { return Err(EnsureFilesystemError { name: name.to_string(), - mountpoint, err: EnsureFilesystemErrorRaw::NotFoundNotFormatted, }); } @@ -561,7 +633,6 @@ impl Zfs { execute(cmd).map_err(|err| EnsureFilesystemError { name: name.to_string(), - mountpoint: mountpoint.clone(), err: err.into(), })?; @@ -574,82 +645,28 @@ impl Zfs { let cmd = command.args(["chown", "-R", &user, &mount]); execute(cmd).map_err(|err| EnsureFilesystemError { name: name.to_string(), - mountpoint: mountpoint.clone(), err: err.into(), })?; } - if let Some(SizeDetails { quota, reservation, compression }) = - size_details - { - // Apply any quota and compression mode. - Self::apply_properties( - name, - &mountpoint, - quota, - reservation, - compression, - )?; - } + Self::set_values(name, props.as_vec().as_slice()) + .map_err(|err| { + EnsureFilesystemError { + name: name.to_string(), + err: err.err.into(), + } + })?; Ok(()) } - /// Applies the following properties to the filesystem. - /// - /// If any of the options are not supplied, a default "none" or "off" - /// value is supplied. - fn apply_properties( - name: &str, - mountpoint: &Mountpoint, - quota: Option, - reservation: Option, - compression: CompressionAlgorithm, - ) -> Result<(), EnsureFilesystemError> { - let quota = quota - .map(|q| q.to_bytes().to_string()) - .unwrap_or_else(|| String::from("none")); - let reservation = reservation - .map(|r| r.to_bytes().to_string()) - .unwrap_or_else(|| String::from("none")); - let compression = compression.to_string(); - - if let Err(err) = Self::set_value(name, "quota", "a) { - return Err(EnsureFilesystemError { - name: name.to_string(), - mountpoint: mountpoint.clone(), - // Take the execution error from the SetValueError - err: err.err.into(), - }); - } - if let Err(err) = Self::set_value(name, "reservation", &reservation) { - return Err(EnsureFilesystemError { - name: name.to_string(), - mountpoint: mountpoint.clone(), - // Take the execution error from the SetValueError - err: err.err.into(), - }); - } - if let Err(err) = Self::set_value(name, "compression", &compression) { - return Err(EnsureFilesystemError { - name: name.to_string(), - mountpoint: mountpoint.clone(), - // Take the execution error from the SetValueError - err: err.err.into(), - }); - } - Ok(()) - } - fn mount_encrypted_dataset( name: &str, - mountpoint: &Mountpoint, ) -> Result<(), EnsureFilesystemError> { let mut command = std::process::Command::new(PFEXEC); let cmd = command.args(&[ZFS, "mount", "-l", name]); execute(cmd).map_err(|err| EnsureFilesystemError { name: name.to_string(), - mountpoint: mountpoint.clone(), err: EnsureFilesystemErrorRaw::MountEncryptedFsFailed(err), })?; Ok(()) @@ -657,13 +674,11 @@ impl Zfs { pub fn mount_overlay_dataset( name: &str, - mountpoint: &Mountpoint, ) -> Result<(), EnsureFilesystemError> { let mut command = std::process::Command::new(PFEXEC); let cmd = command.args(&[ZFS, "mount", "-O", name]); execute(cmd).map_err(|err| EnsureFilesystemError { name: name.to_string(), - mountpoint: mountpoint.clone(), err: EnsureFilesystemErrorRaw::MountOverlayFsFailed(err), })?; Ok(()) @@ -689,7 +704,6 @@ impl Zfs { if &values[..3] != &[name, "filesystem", &mountpoint.to_string()] { return Err(EnsureFilesystemError { name: name.to_string(), - mountpoint: mountpoint.clone(), err: EnsureFilesystemErrorRaw::Output(stdout.to_string()), }); } @@ -714,13 +728,29 @@ impl Zfs { name: &str, value: &str, ) -> Result<(), SetValueError> { + Self::set_values( + filesystem_name, + &[(name, value)] + ) + } + + fn set_values<'a>( + filesystem_name: &'a str, + name_values: &'a [(&'a str, &'a str)], + ) -> Result<(), SetValueError> { + if name_values.is_empty() { + return Ok(()); + } + let mut command = std::process::Command::new(PFEXEC); - let value_arg = format!("{}={}", name, value); - let cmd = command.args(&[ZFS, "set", &value_arg, filesystem_name]); + let cmd = command.args(&[ZFS, "set"]); + for (name, value) in name_values { + cmd.arg(format!("{name}={value}")); + } + cmd.arg(filesystem_name); execute(cmd).map_err(|err| SetValueError { filesystem: filesystem_name.to_string(), - name: name.to_string(), - value: value.to_string(), + values: name_values.iter().map(|(k,v)| format!("{k}={v}")).join(","), err, })?; Ok(()) diff --git a/sled-storage/src/dataset.rs b/sled-storage/src/dataset.rs index e90a4c0092..717595ad9e 100644 --- a/sled-storage/src/dataset.rs +++ b/sled-storage/src/dataset.rs @@ -269,6 +269,7 @@ pub(crate) async fn ensure_zpool_has_datasets( Some(encryption_details), None, None, + None, ); keyfile.zero_and_unlink().await.map_err(|error| { @@ -331,6 +332,7 @@ pub(crate) async fn ensure_zpool_has_datasets( encryption_details, size_details, None, + None, )?; if dataset.wipe { diff --git a/sled-storage/src/error.rs b/sled-storage/src/error.rs index 351dd7f353..f00e35e654 100644 --- a/sled-storage/src/error.rs +++ b/sled-storage/src/error.rs @@ -77,6 +77,9 @@ pub enum Error { #[error("Not ready to manage U.2s (key manager is not ready)")] KeyManagerNotReady, + #[error("Physical disk configuration changed for the same generation number: {generation}")] + PhysicalDiskConfigurationChanged { generation: Generation }, + #[error("Physical disk configuration out-of-date (asked for {requested}, but latest is {current})")] PhysicalDiskConfigurationOutdated { requested: Generation, diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 1d8eaaf4a0..ff5a8a4dac 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -14,7 +14,7 @@ use camino::Utf8Path; use camino::Utf8PathBuf; use debug_ignore::DebugIgnore; use futures::future::FutureExt; -use illumos_utils::zfs::{DatasetProperties, Mountpoint, Zfs}; +use illumos_utils::zfs::{DatasetProperties, Mountpoint, WhichDatasets, Zfs}; use illumos_utils::zpool::{ZpoolName, ZPOOL_MOUNTPOINT_ROOT}; use key_manager::StorageKeyRequester; use omicron_common::disk::{ @@ -26,6 +26,7 @@ use omicron_common::ledger::Ledger; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::GenericUuid; use slog::{error, info, o, warn, Logger}; +use std::collections::BTreeMap; use std::collections::HashSet; use std::future::Future; use tokio::sync::{mpsc, oneshot, watch}; @@ -891,19 +892,6 @@ impl StorageManager { generation: config.generation, }); } - - // The ledger already was written, and the datasets were - // ensured for this generation. Exit early. - return Ok(DatasetsManagementResult { - status: config - .datasets - .into_values() - .map(|dataset| DatasetManagementStatus { - dataset_name: dataset.name, - err: None, - }) - .collect(), - }); } else { info!( log, @@ -946,9 +934,25 @@ impl StorageManager { log: &Logger, config: &DatasetsConfig, ) -> DatasetsManagementResult { + // Gather properties about these datasets, if they exist. + // + // This pre-fetching lets us avoid individually querying them later. + let old_datasets = Zfs::get_dataset_properties( + config.datasets.values().map(|config| { + config.name.full_name() + }).collect::>().as_slice(), + WhichDatasets::SelfOnly, + ).unwrap_or_default().into_iter().map(|props| { + (props.name.clone(), props) + }).collect::>(); + // Ensure each dataset concurrently let futures = config.datasets.values().map(|dataset| async { - self.dataset_ensure_internal(log, dataset).await + self.dataset_ensure_internal( + log, + dataset, + old_datasets.get(&dataset.name.full_name()), + ).await }); let status = futures::future::join_all(futures).await; @@ -959,6 +963,7 @@ impl StorageManager { &self, log: &Logger, config: &DatasetConfig, + old_dataset: Option<&DatasetProperties>, ) -> DatasetManagementStatus { let log = log.new(o!("name" => config.name.full_name())); info!(log, "Ensuring dataset"); @@ -967,6 +972,45 @@ impl StorageManager { err: None, }; + // If this dataset already exists, we're willing to skip the work of + // "ensure" under specific circumstances + if let Some(old_dataset) = old_dataset { + info!(log, "This dataset already existed"); + // First, we must validate that the old config matches our current + // configuration. + match SharedDatasetConfig::try_from(old_dataset) { + Ok(old_props) => { + info!(log, "Parsed old properties"); + // We also need to ensure that the old dataset exists and + // has our UUID. + if let Some(old_id) = old_dataset.id { + if old_id != config.id { + status.err = Some(Error::UuidMismatch { + name: config.name.full_name(), + old: old_id.into_untyped_uuid(), + new: config.id.into_untyped_uuid(), + }.to_string()); + return status; + } + if old_props == config.inner { + // In this case, there is no work to do. + info!(log, "No changes necessary, returning early"); + return status; + } else { + info!(log, "Old properties changed {:?} vs {:?}", old_props, config.inner); + } + } else { + info!(log, "Old properties missing UUID"); + } + }, + Err(err) => { + warn!(log, "Failed to parse old properties"; "err" => ?err); + } + } + } else { + info!(log, "This dataset did not exist"); + } + let mountpoint_root = &self.resources.disks().mount_config().root; let mountpoint_path = config.name.mountpoint(mountpoint_root); let details = DatasetCreationDetails { @@ -1032,8 +1076,10 @@ impl StorageManager { ]; info!(log, "Listing datasets within zpool"; "zpool" => zpool.to_string()); - Zfs::get_dataset_properties(datasets_of_interest.as_slice()) - .map_err(Error::Other) + Zfs::get_dataset_properties( + datasets_of_interest.as_slice(), + WhichDatasets::SelfAndChildren, + ).map_err(Error::Other) } // Ensures that a dataset exists, nested somewhere arbitrary within @@ -1054,7 +1100,7 @@ impl StorageManager { full_name: config.name.full_name(), }; - self.ensure_dataset(config.name.root.pool(), &config.inner, &details) + self.ensure_dataset(config.name.root.pool(), None, &config.inner, &details) .await?; Ok(()) @@ -1089,7 +1135,7 @@ impl StorageManager { let full_name = name.full_name(); let properties = - Zfs::get_dataset_properties(&[full_name]).map_err(|e| { + Zfs::get_dataset_properties(&[full_name], WhichDatasets::SelfAndChildren).map_err(|e| { warn!( log, "Failed to access nested dataset"; @@ -1174,10 +1220,23 @@ impl StorageManager { requested: config.generation, current: ledger_data.generation, }); - } + } else if config.generation == ledger_data.generation { + info!( + log, + "Requested generation number matches prior request", + ); - // TODO: If the generation is equal, check that the values are - // also equal. + if ledger_data != &config { + error!( + log, + "Requested configuration changed (with the same generation)"; + "generation" => ?config.generation + ); + return Err(Error::PhysicalDiskConfigurationChanged { + generation: config.generation, + }); + } + } info!(log, "Request looks newer than prior requests"); ledger @@ -1315,22 +1374,7 @@ impl StorageManager { config: &SharedDatasetConfig, details: &DatasetCreationDetails, ) -> Result<(), Error> { - self.ensure_dataset(zpool, config, details).await?; - - // Ensure the dataset has a usable UUID. - if let Ok(id_str) = Zfs::get_oxide_value(&details.full_name, "uuid") { - if let Ok(found_id) = id_str.parse::() { - if found_id != id { - return Err(Error::UuidMismatch { - name: details.full_name.clone(), - old: found_id.into_untyped_uuid(), - new: id.into_untyped_uuid(), - }); - } - return Ok(()); - } - } - Zfs::set_oxide_value(&details.full_name, "uuid", &id.to_string())?; + self.ensure_dataset(zpool, Some(id), config, details).await?; Ok(()) } @@ -1340,6 +1384,7 @@ impl StorageManager { async fn ensure_dataset( &self, zpool: &ZpoolName, + dataset_id: Option, config: &SharedDatasetConfig, details: &DatasetCreationDetails, ) -> Result<(), Error> { @@ -1380,6 +1425,7 @@ impl StorageManager { do_format, encryption_details, size_details, + dataset_id, None, )?; @@ -1416,6 +1462,7 @@ impl StorageManager { do_format, encryption_details, size_details, + Some(DatasetUuid::from_untyped_uuid(request.dataset_id)), None, )?; // Ensure the dataset has a usable UUID. @@ -1741,7 +1788,7 @@ mod tests { ); let mut harness = StorageManagerTestHarness::new(&logctx.log).await; - // Queue up a disks, as we haven't told the `StorageManager` that + // Queue up a disk, as we haven't told the `StorageManager` that // the `KeyManager` is ready yet. let raw_disks = harness.add_vdevs(&["u2_under_test.vdev", "m2_helping.vdev"]).await; @@ -2038,23 +2085,19 @@ mod tests { .await; let config = harness.make_config(1, &raw_disks); - println!("Ensuring physical disks..."); let result = harness .handle() .omicron_physical_disks_ensure(config.clone()) .await .expect("Ensuring disks should work after key manager is ready"); assert!(!result.has_error(), "{:?}", result); - println!("OK"); - println!("Ensuring physical disks again..."); let result = harness .handle() .omicron_physical_disks_ensure(config.clone()) .await .expect("Ensuring disks should work after key manager is ready"); assert!(!result.has_error(), "{:?}", result); - println!("OK"); // Create datasets on the newly formatted U.2s let mut datasets = BTreeMap::new(); @@ -2103,25 +2146,19 @@ mod tests { let generation = Generation::new().next(); let config = DatasetsConfig { generation, datasets }; - println!("Ensuring datasets... "); let status = harness.handle().datasets_ensure(config.clone()).await.unwrap(); assert!(!status.has_error()); - println!("OK"); // List datasets, expect to see what we just created - println!("List... "); let observed_config = harness.handle().datasets_config_list().await.unwrap(); assert_eq!(config, observed_config); - println!("OK"); // Calling "datasets_ensure" with the same input should succeed. - println!("Ensuring datasets again... "); let status = harness.handle().datasets_ensure(config.clone()).await.unwrap(); assert!(!status.has_error()); - println!("OK"); harness.cleanup().await; logctx.cleanup_successful(); From 5dcee636bba9f124fa964ceda0df5cfaba6f09af Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 11 Dec 2024 17:58:15 -0800 Subject: [PATCH 5/9] cleanup --- illumos-utils/src/zfs.rs | 76 +++++++++++++---------------- sled-storage/src/manager.rs | 95 ++++++++++++++++++++----------------- 2 files changed, 85 insertions(+), 86 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 13b55c2b48..8c8d3c1db8 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -84,9 +84,7 @@ enum EnsureFilesystemErrorRaw { /// Error returned by [`Zfs::ensure_filesystem`]. #[derive(thiserror::Error, Debug)] -#[error( - "Failed to ensure filesystem '{name}': {err}" -)] +#[error("Failed to ensure filesystem '{name}': {err}")] pub struct EnsureFilesystemError { name: String, #[source] @@ -95,9 +93,7 @@ pub struct EnsureFilesystemError { /// Error returned by [`Zfs::set_oxide_value`] #[derive(thiserror::Error, Debug)] -#[error( - "Failed to set values '{values}' on filesystem {filesystem}: {err}" -)] +#[error("Failed to set values '{values}' on filesystem {filesystem}: {err}")] pub struct SetValueError { filesystem: String, values: String, @@ -245,7 +241,9 @@ impl DatasetProperties { impl TryFrom<&DatasetProperties> for SharedDatasetConfig { type Error = anyhow::Error; - fn try_from(props: &DatasetProperties) -> Result { + fn try_from( + props: &DatasetProperties, + ) -> Result { Ok(SharedDatasetConfig { compression: props.compression.parse()?, quota: props.quota, @@ -328,9 +326,7 @@ impl DatasetProperties { // with a value of "none". let quota = props .get("quota") - .filter(|(prop, _source)| { - *prop != "-" && *prop != "0" - }) + .filter(|(prop, _source)| *prop != "-" && *prop != "0") .map(|(prop, _source)| { prop.parse::().context("Failed to parse 'quota'") }) @@ -338,9 +334,7 @@ impl DatasetProperties { .and_then(|v| ByteCount::try_from(v).ok()); let reservation = props .get("reservation") - .filter(|(prop, _source)| { - *prop != "-" && *prop != "0" - }) + .filter(|(prop, _source)| *prop != "-" && *prop != "0") .map(|(prop, _source)| { prop.parse::() .context("Failed to parse 'reservation'") @@ -394,15 +388,16 @@ pub enum WhichDatasets { SelfAndChildren, } +// A helper structure for gathering all possible key/value pairs to pass to "zfs +// set". This helps callers minimize the number of calls to "zfs set" they need +// to make. struct PropertySetter { props: BTreeMap<&'static str, String>, } impl PropertySetter { fn new() -> Self { - PropertySetter { - props: BTreeMap::new(), - } + PropertySetter { props: BTreeMap::new() } } fn add_size_details(&mut self, details: SizeDetails) -> &mut Self { @@ -429,9 +424,7 @@ impl PropertySetter { } fn as_vec(&self) -> Vec<(&str, &str)> { - self.props.iter().map(|(k, v)| { - (*k, v.as_str()) - }).collect() + self.props.iter().map(|(k, v)| (*k, v.as_str())).collect() } } @@ -477,10 +470,7 @@ impl Zfs { cmd.args(&["-d", "1"]); } } - cmd.args(&[ - "-Hpo", - "name,property,value,source", - ]); + cmd.args(&["-Hpo", "name,property,value,source"]); // Note: this is tightly coupled with the layout of DatasetProperties cmd.arg(DatasetProperties::ZFS_GET_PROPS); @@ -491,7 +481,9 @@ impl Zfs { // If one or more dataset doesn't exist, we can still read stdout to // see about the ones that do exist. let output = cmd.output().map_err(|err| { - anyhow!("Failed to get dataset properties for {datasets:?}: {err:?}") + anyhow!( + "Failed to get dataset properties for {datasets:?}: {err:?}" + ) })?; let stdout = String::from_utf8(output.stdout)?; @@ -573,13 +565,12 @@ impl Zfs { } if exists { - Self::set_values(name, props.as_vec().as_slice()) - .map_err(|err| { - EnsureFilesystemError { - name: name.to_string(), - err: err.err.into(), - } - })?; + Self::set_values(name, props.as_vec().as_slice()).map_err( + |err| EnsureFilesystemError { + name: name.to_string(), + err: err.err.into(), + }, + )?; if encryption_details.is_none() { // If the dataset exists, we're done. Unencrypted datasets are @@ -649,13 +640,12 @@ impl Zfs { })?; } - Self::set_values(name, props.as_vec().as_slice()) - .map_err(|err| { - EnsureFilesystemError { - name: name.to_string(), - err: err.err.into(), - } - })?; + Self::set_values(name, props.as_vec().as_slice()).map_err(|err| { + EnsureFilesystemError { + name: name.to_string(), + err: err.err.into(), + } + })?; Ok(()) } @@ -728,10 +718,7 @@ impl Zfs { name: &str, value: &str, ) -> Result<(), SetValueError> { - Self::set_values( - filesystem_name, - &[(name, value)] - ) + Self::set_values(filesystem_name, &[(name, value)]) } fn set_values<'a>( @@ -750,7 +737,10 @@ impl Zfs { cmd.arg(filesystem_name); execute(cmd).map_err(|err| SetValueError { filesystem: filesystem_name.to_string(), - values: name_values.iter().map(|(k,v)| format!("{k}={v}")).join(","), + values: name_values + .iter() + .map(|(k, v)| format!("{k}={v}")) + .join(","), err, })?; Ok(()) diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index ff5a8a4dac..f0212933af 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -938,13 +938,18 @@ impl StorageManager { // // This pre-fetching lets us avoid individually querying them later. let old_datasets = Zfs::get_dataset_properties( - config.datasets.values().map(|config| { - config.name.full_name() - }).collect::>().as_slice(), + config + .datasets + .values() + .map(|config| config.name.full_name()) + .collect::>() + .as_slice(), WhichDatasets::SelfOnly, - ).unwrap_or_default().into_iter().map(|props| { - (props.name.clone(), props) - }).collect::>(); + ) + .unwrap_or_default() + .into_iter() + .map(|props| (props.name.clone(), props)) + .collect::>(); // Ensure each dataset concurrently let futures = config.datasets.values().map(|dataset| async { @@ -952,7 +957,8 @@ impl StorageManager { log, dataset, old_datasets.get(&dataset.name.full_name()), - ).await + ) + .await }); let status = futures::future::join_all(futures).await; @@ -985,11 +991,14 @@ impl StorageManager { // has our UUID. if let Some(old_id) = old_dataset.id { if old_id != config.id { - status.err = Some(Error::UuidMismatch { - name: config.name.full_name(), - old: old_id.into_untyped_uuid(), - new: config.id.into_untyped_uuid(), - }.to_string()); + status.err = Some( + Error::UuidMismatch { + name: config.name.full_name(), + old: old_id.into_untyped_uuid(), + new: config.id.into_untyped_uuid(), + } + .to_string(), + ); return status; } if old_props == config.inner { @@ -997,12 +1006,17 @@ impl StorageManager { info!(log, "No changes necessary, returning early"); return status; } else { - info!(log, "Old properties changed {:?} vs {:?}", old_props, config.inner); + info!( + log, + "Old properties changed {:?} vs {:?}", + old_props, + config.inner + ); } } else { info!(log, "Old properties missing UUID"); } - }, + } Err(err) => { warn!(log, "Failed to parse old properties"; "err" => ?err); } @@ -1020,9 +1034,9 @@ impl StorageManager { }; if let Err(err) = self - .ensure_dataset_with_id( + .ensure_dataset( config.name.pool(), - config.id, + Some(config.id), &config.inner, &details, ) @@ -1079,7 +1093,8 @@ impl StorageManager { Zfs::get_dataset_properties( datasets_of_interest.as_slice(), WhichDatasets::SelfAndChildren, - ).map_err(Error::Other) + ) + .map_err(Error::Other) } // Ensures that a dataset exists, nested somewhere arbitrary within @@ -1100,8 +1115,13 @@ impl StorageManager { full_name: config.name.full_name(), }; - self.ensure_dataset(config.name.root.pool(), None, &config.inner, &details) - .await?; + self.ensure_dataset( + config.name.root.pool(), + None, + &config.inner, + &details, + ) + .await?; Ok(()) } @@ -1134,15 +1154,18 @@ impl StorageManager { info!(log, "Listing nested datasets"); let full_name = name.full_name(); - let properties = - Zfs::get_dataset_properties(&[full_name], WhichDatasets::SelfAndChildren).map_err(|e| { - warn!( - log, - "Failed to access nested dataset"; - "name" => ?name - ); - crate::dataset::DatasetError::Other(e) - })?; + let properties = Zfs::get_dataset_properties( + &[full_name], + WhichDatasets::SelfAndChildren, + ) + .map_err(|e| { + warn!( + log, + "Failed to access nested dataset"; + "name" => ?name + ); + crate::dataset::DatasetError::Other(e) + })?; let root_path = name.root.full_name(); Ok(properties @@ -1365,19 +1388,6 @@ impl StorageManager { } } - // Invokes [Self::ensure_dataset] and also ensures the dataset has an - // expected UUID as a ZFS property. - async fn ensure_dataset_with_id( - &self, - zpool: &ZpoolName, - id: DatasetUuid, - config: &SharedDatasetConfig, - details: &DatasetCreationDetails, - ) -> Result<(), Error> { - self.ensure_dataset(zpool, Some(id), config, details).await?; - Ok(()) - } - // Ensures a dataset exists within a zpool. // // Confirms that the zpool exists and is managed by this sled. @@ -1490,8 +1500,7 @@ impl StorageManager { /// All tests only use synthetic disks, but are expected to be run on illumos /// systems. -// #[cfg(all(test, target_os = "illumos"))] -#[cfg(test)] +#[cfg(all(test, target_os = "illumos"))] mod tests { use crate::disk::RawSyntheticDisk; use crate::manager_test_harness::StorageManagerTestHarness; From c55eb06c1588eabb2d06a190ec710f4f805fa7be Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 11 Dec 2024 18:03:34 -0800 Subject: [PATCH 6/9] compiling lol --- sled-agent/src/backing_fs.rs | 3 ++- sled-agent/src/bootstrap/pre_server.rs | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sled-agent/src/backing_fs.rs b/sled-agent/src/backing_fs.rs index 265c0152e2..d24f85ad80 100644 --- a/sled-agent/src/backing_fs.rs +++ b/sled-agent/src/backing_fs.rs @@ -150,6 +150,7 @@ pub(crate) fn ensure_backing_fs( true, // do_format None, // encryption_details, size_details, + None, Some(vec!["canmount=noauto".to_string()]), // options )?; @@ -180,7 +181,7 @@ pub(crate) fn ensure_backing_fs( info!(log, "Mounting {} on {}", dataset, mountpoint); - Zfs::mount_overlay_dataset(&dataset, &mountpoint)?; + Zfs::mount_overlay_dataset(&dataset)?; if let Some(subdirs) = bfs.subdirs { for dir in subdirs { diff --git a/sled-agent/src/bootstrap/pre_server.rs b/sled-agent/src/bootstrap/pre_server.rs index 5b89506242..19a63cb71d 100644 --- a/sled-agent/src/bootstrap/pre_server.rs +++ b/sled-agent/src/bootstrap/pre_server.rs @@ -293,6 +293,7 @@ fn ensure_zfs_ramdisk_dataset() -> Result<(), StartError> { encryption_details, quota, None, + None, ) .map_err(StartError::EnsureZfsRamdiskDataset) } From ff29b081ac71c54dc9a665e8cacad483dabe6cca Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 12 Dec 2024 13:39:39 -0800 Subject: [PATCH 7/9] Review feedback --- Cargo.lock | 1 + illumos-utils/src/zfs.rs | 34 +++++++++++++++++----------------- sled-storage/Cargo.toml | 1 + sled-storage/src/manager.rs | 24 +++++++++++++++++++++--- 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c42be5e5f0..f8089c0e17 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10807,6 +10807,7 @@ dependencies = [ "slog", "thiserror 1.0.69", "tokio", + "tokio-stream", "uuid", ] diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 8c8d3c1db8..c704681c77 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -256,7 +256,9 @@ impl DatasetProperties { /// Parses dataset properties, assuming that the caller is providing the /// output of the following command as stdout: /// - /// zfs get -rpo name,property,value,source $ZFS_GET_PROPS $DATASETS + /// zfs get \ + /// [maybe depth arguments] \ + /// -Hpo name,property,value,source $ZFS_GET_PROPS $DATASETS fn parse_many( stdout: &str, ) -> Result, anyhow::Error> { @@ -392,12 +394,12 @@ pub enum WhichDatasets { // set". This helps callers minimize the number of calls to "zfs set" they need // to make. struct PropertySetter { - props: BTreeMap<&'static str, String>, + props: Vec<(&'static str, String)>, } impl PropertySetter { fn new() -> Self { - PropertySetter { props: BTreeMap::new() } + PropertySetter { props: Vec::new() } } fn add_size_details(&mut self, details: SizeDetails) -> &mut Self { @@ -405,30 +407,29 @@ impl PropertySetter { let quota = quota .map(|q| q.to_bytes().to_string()) .unwrap_or_else(|| String::from("none")); - self.props.insert("quota", quota); + self.props.push(("quota", quota)); let reservation = reservation .map(|r| r.to_bytes().to_string()) .unwrap_or_else(|| String::from("none")); - self.props.insert("reservation", reservation); + self.props.push(("reservation", reservation)); let compression = compression.to_string(); - self.props.insert("compression", compression); + self.props.push(("compression", compression)); self } fn add_id(&mut self, id: DatasetUuid) -> &mut Self { - self.props.insert("oxide:uuid", id.to_string()); + self.props.push(("oxide:uuid", id.to_string())); self } - fn as_vec(&self) -> Vec<(&str, &str)> { - self.props.iter().map(|(k, v)| (*k, v.as_str())).collect() + fn as_vec(&self) -> &Vec<(&str, String)> { + &self.props } } -#[cfg_attr(any(test, feature = "testing"), mockall::automock, allow(dead_code))] impl Zfs { /// Lists all datasets within a pool or existing dataset. /// @@ -452,7 +453,9 @@ impl Zfs { } /// Get information about datasets within a list of zpools / datasets. - /// Returns properties for all input datasets and their direct children. + /// Returns properties for all input datasets, and optionally, for + /// their children (depending on the value of [WhichDatasets] is provided + /// as input). /// /// This function is similar to [Zfs::list_datasets], but provides a more /// substantial results about the datasets found. @@ -721,9 +724,9 @@ impl Zfs { Self::set_values(filesystem_name, &[(name, value)]) } - fn set_values<'a>( - filesystem_name: &'a str, - name_values: &'a [(&'a str, &'a str)], + fn set_values( + filesystem_name: &str, + name_values: &[(K, V)], ) -> Result<(), SetValueError> { if name_values.is_empty() { return Ok(()); @@ -828,10 +831,7 @@ impl Zfs { err, }) } -} -// 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 diff --git a/sled-storage/Cargo.toml b/sled-storage/Cargo.toml index 2439c52aa7..d47a89e0ae 100644 --- a/sled-storage/Cargo.toml +++ b/sled-storage/Cargo.toml @@ -28,6 +28,7 @@ sled-hardware.workspace = true slog.workspace = true thiserror.workspace = true tokio.workspace = true +tokio-stream.workspace = true uuid.workspace = true omicron-workspace-hack.workspace = true diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index f0212933af..a51894361b 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -14,6 +14,8 @@ use camino::Utf8Path; use camino::Utf8PathBuf; 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::zpool::{ZpoolName, ZPOOL_MOUNTPOINT_ROOT}; use key_manager::StorageKeyRequester; @@ -930,7 +932,7 @@ impl StorageManager { // includes details about all possible errors that may occur on // a per-dataset granularity. async fn datasets_ensure_internal( - &mut self, + &self, log: &Logger, config: &DatasetsConfig, ) -> DatasetsManagementResult { @@ -951,7 +953,6 @@ impl StorageManager { .map(|props| (props.name.clone(), props)) .collect::>(); - // Ensure each dataset concurrently let futures = config.datasets.values().map(|dataset| async { self.dataset_ensure_internal( log, @@ -961,7 +962,24 @@ impl StorageManager { .await }); - let status = futures::future::join_all(futures).await; + // This "Box::pin" is a workaround for: https://github.com/rust-lang/rust/issues/64552 + // + // Ideally, we would just use: + // + // ``` + // let status: Vec<_> = futures::stream::iter(futures) + // .buffered(...) + // .collect() + // .await; + // ``` + const DATASET_ENSURE_CONCURRENCY_LIMIT: usize = 16; + let results: std::pin::Pin + Send>> = Box::pin( + futures::stream::iter(futures) + .buffered(DATASET_ENSURE_CONCURRENCY_LIMIT), + ); + + let status: Vec = results.collect().await; + DatasetsManagementResult { status } } From 3dbb44f0555370f7b8b1401e59e6bebbf1adfbca Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 12 Dec 2024 13:57:41 -0800 Subject: [PATCH 8/9] Review feedback --- sled-storage/src/manager.rs | 102 ++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 46 deletions(-) diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index a51894361b..46d51ac8f1 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -983,6 +983,50 @@ impl StorageManager { DatasetsManagementResult { status } } + fn should_skip_dataset_ensure( + log: &Logger, + config: &DatasetConfig, + old_dataset: Option<&DatasetProperties>, + ) -> Result { + let Some(old_dataset) = old_dataset else { + info!(log, "This dataset did not exist"); + return Ok(false); + }; + + let Some(old_id) = old_dataset.id else { + info!(log, "Old properties missing UUID"); + return Ok(false); + }; + + let old_props = match SharedDatasetConfig::try_from(old_dataset) { + Ok(old_props) => old_props, + Err(err) => { + warn!(log, "Failed to parse old properties"; "err" => ?err); + return Ok(false); + } + }; + + info!(log, "Parsed old dataset properties"; "props" => ?old_props); + if old_id != config.id { + return Err(Error::UuidMismatch { + name: config.name.full_name(), + old: old_id.into_untyped_uuid(), + new: config.id.into_untyped_uuid(), + }); + } + if old_props != config.inner { + info!( + log, + "Dataset properties changed"; + "old_props" => ?old_props, + "requested_props" => ?config.inner, + ); + return Ok(false); + } + info!(log, "No changes necessary, returning early"); + return Ok(true); + } + async fn dataset_ensure_internal( &self, log: &Logger, @@ -996,51 +1040,13 @@ impl StorageManager { err: None, }; - // If this dataset already exists, we're willing to skip the work of - // "ensure" under specific circumstances - if let Some(old_dataset) = old_dataset { - info!(log, "This dataset already existed"); - // First, we must validate that the old config matches our current - // configuration. - match SharedDatasetConfig::try_from(old_dataset) { - Ok(old_props) => { - info!(log, "Parsed old properties"); - // We also need to ensure that the old dataset exists and - // has our UUID. - if let Some(old_id) = old_dataset.id { - if old_id != config.id { - status.err = Some( - Error::UuidMismatch { - name: config.name.full_name(), - old: old_id.into_untyped_uuid(), - new: config.id.into_untyped_uuid(), - } - .to_string(), - ); - return status; - } - if old_props == config.inner { - // In this case, there is no work to do. - info!(log, "No changes necessary, returning early"); - return status; - } else { - info!( - log, - "Old properties changed {:?} vs {:?}", - old_props, - config.inner - ); - } - } else { - info!(log, "Old properties missing UUID"); - } - } - Err(err) => { - warn!(log, "Failed to parse old properties"; "err" => ?err); - } + match Self::should_skip_dataset_ensure(&log, config, old_dataset) { + Ok(true) => return status, + Ok(false) => (), + Err(err) => { + status.err = Some(err.to_string()); + return status; } - } else { - info!(log, "This dataset did not exist"); } let mountpoint_root = &self.resources.disks().mount_config().root; @@ -1277,9 +1283,13 @@ impl StorageManager { generation: config.generation, }); } + info!( + log, + "Request looks identical to last request, re-sending" + ); + } else { + info!(log, "Request looks newer than prior requests"); } - - info!(log, "Request looks newer than prior requests"); ledger } None => { From b6506393d435740ac3a1fb86ea44770a762df51c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 13 Dec 2024 12:42:09 -0800 Subject: [PATCH 9/9] review feedback --- illumos-utils/src/zfs.rs | 57 +++++++++++++------------------------ sled-storage/src/manager.rs | 17 +++++------ 2 files changed, 28 insertions(+), 46 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index c704681c77..ee1ac58be9 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -390,44 +390,32 @@ pub enum WhichDatasets { SelfAndChildren, } -// A helper structure for gathering all possible key/value pairs to pass to "zfs -// set". This helps callers minimize the number of calls to "zfs set" they need -// to make. -struct PropertySetter { - props: Vec<(&'static str, String)>, -} - -impl PropertySetter { - fn new() -> Self { - PropertySetter { props: Vec::new() } - } - - fn add_size_details(&mut self, details: SizeDetails) -> &mut Self { - let SizeDetails { quota, reservation, compression } = details; +fn build_zfs_set_key_value_pairs( + size_details: Option, + dataset_id: Option, +) -> Vec<(&'static str, String)> { + let mut props = Vec::new(); + if let Some(SizeDetails { quota, reservation, compression }) = size_details + { let quota = quota .map(|q| q.to_bytes().to_string()) .unwrap_or_else(|| String::from("none")); - self.props.push(("quota", quota)); + props.push(("quota", quota)); let reservation = reservation .map(|r| r.to_bytes().to_string()) .unwrap_or_else(|| String::from("none")); - self.props.push(("reservation", reservation)); + props.push(("reservation", reservation)); let compression = compression.to_string(); - self.props.push(("compression", compression)); - - self + props.push(("compression", compression)); } - fn add_id(&mut self, id: DatasetUuid) -> &mut Self { - self.props.push(("oxide:uuid", id.to_string())); - self + if let Some(id) = dataset_id { + props.push(("oxide:uuid", id.to_string())); } - fn as_vec(&self) -> &Vec<(&str, String)> { - &self.props - } + props } impl Zfs { @@ -559,21 +547,14 @@ impl Zfs { ) -> Result<(), EnsureFilesystemError> { let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?; - let mut props = PropertySetter::new(); - if let Some(size_details) = size_details { - props.add_size_details(size_details); - } - if let Some(id) = id { - props.add_id(id); - } - + let props = build_zfs_set_key_value_pairs(size_details, id); if exists { - Self::set_values(name, props.as_vec().as_slice()).map_err( - |err| EnsureFilesystemError { + Self::set_values(name, props.as_slice()).map_err(|err| { + EnsureFilesystemError { name: name.to_string(), err: err.err.into(), - }, - )?; + } + })?; if encryption_details.is_none() { // If the dataset exists, we're done. Unencrypted datasets are @@ -643,7 +624,7 @@ impl Zfs { })?; } - Self::set_values(name, props.as_vec().as_slice()).map_err(|err| { + Self::set_values(name, props.as_slice()).map_err(|err| { EnsureFilesystemError { name: name.to_string(), err: err.err.into(), diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 46d51ac8f1..b1832ca92b 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -998,22 +998,23 @@ impl StorageManager { return Ok(false); }; + if old_id != config.id { + return Err(Error::UuidMismatch { + name: config.name.full_name(), + old: old_id.into_untyped_uuid(), + new: config.id.into_untyped_uuid(), + }); + } + let old_props = match SharedDatasetConfig::try_from(old_dataset) { Ok(old_props) => old_props, Err(err) => { - warn!(log, "Failed to parse old properties"; "err" => ?err); + warn!(log, "Failed to parse old properties"; "err" => #%err); return Ok(false); } }; info!(log, "Parsed old dataset properties"; "props" => ?old_props); - if old_id != config.id { - return Err(Error::UuidMismatch { - name: config.name.full_name(), - old: old_id.into_untyped_uuid(), - new: config.id.into_untyped_uuid(), - }); - } if old_props != config.inner { info!( log,