From 5c2e5b0ec82b25f14d2d76451c05413c4f798961 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 18 Jul 2024 10:47:02 -0700 Subject: [PATCH 01/36] [wip] Starting sled agent API to manage datasets explicitly --- illumos-utils/src/zfs.rs | 44 +++++- sled-agent/src/backing_fs.rs | 3 +- sled-agent/src/http_entrypoints.rs | 39 +++++- sled-agent/src/params.rs | 2 + sled-agent/src/sled_agent.rs | 33 ++++- sled-storage/src/dataset.rs | 25 +++- sled-storage/src/disk.rs | 63 +++++++++ sled-storage/src/manager.rs | 216 ++++++++++++++++++++++++++++- sled-storage/src/resources.rs | 17 +++ uuid-kinds/src/lib.rs | 1 + 10 files changed, 419 insertions(+), 24 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 139e6fe607..21de2a50da 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -203,7 +203,8 @@ pub struct EncryptionDetails { #[derive(Debug, Default)] pub struct SizeDetails { pub quota: Option, - pub compression: Option<&'static str>, + pub reservation: Option, + pub compression: Option, } #[cfg_attr(any(test, feature = "testing"), mockall::automock, allow(dead_code))] @@ -274,10 +275,18 @@ impl Zfs { ) -> Result<(), EnsureFilesystemError> { let (exists, mounted) = Self::dataset_exists(name, &mountpoint)?; if exists { - if let Some(SizeDetails { quota, compression }) = size_details { + 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, compression)?; + Self::apply_properties( + name, + &mountpoint, + quota, + reservation, + compression, + )?; } if encryption_details.is_none() { @@ -351,9 +360,17 @@ impl Zfs { })?; } - if let Some(SizeDetails { quota, compression }) = size_details { + if let Some(SizeDetails { quota, reservation, compression }) = + size_details + { // Apply any quota and compression mode. - Self::apply_properties(name, &mountpoint, quota, compression)?; + Self::apply_properties( + name, + &mountpoint, + quota, + reservation, + compression, + )?; } Ok(()) @@ -363,7 +380,8 @@ impl Zfs { name: &str, mountpoint: &Mountpoint, quota: Option, - compression: Option<&'static str>, + reservation: Option, + compression: Option, ) -> Result<(), EnsureFilesystemError> { if let Some(quota) = quota { if let Err(err) = @@ -377,8 +395,20 @@ impl Zfs { }); } } + if let Some(reservation) = reservation { + if let Err(err) = + Self::set_value(name, "reservation", &format!("{reservation}")) + { + return Err(EnsureFilesystemError { + name: name.to_string(), + mountpoint: mountpoint.clone(), + // Take the execution error from the SetValueError + err: err.err.into(), + }); + } + } if let Some(compression) = compression { - if let Err(err) = Self::set_value(name, "compression", compression) + if let Err(err) = Self::set_value(name, "compression", &compression) { return Err(EnsureFilesystemError { name: name.to_string(), diff --git a/sled-agent/src/backing_fs.rs b/sled-agent/src/backing_fs.rs index 2e9ea4c8d9..48002a8841 100644 --- a/sled-agent/src/backing_fs.rs +++ b/sled-agent/src/backing_fs.rs @@ -137,7 +137,8 @@ pub(crate) fn ensure_backing_fs( let size_details = Some(SizeDetails { quota: bfs.quota, - compression: bfs.compression, + reservation: None, + compression: bfs.compression.map(|s| s.to_string()), }); Zfs::ensure_filesystem( diff --git a/sled-agent/src/http_entrypoints.rs b/sled-agent/src/http_entrypoints.rs index 2612e504f5..9c1d5a4e11 100644 --- a/sled-agent/src/http_entrypoints.rs +++ b/sled-agent/src/http_entrypoints.rs @@ -8,11 +8,12 @@ use super::sled_agent::SledAgent; use crate::bootstrap::early_networking::EarlyNetworkConfig; use crate::bootstrap::params::AddSledRequest; use crate::params::{ - BootstoreStatus, CleanupContextUpdate, DiskEnsureBody, InstanceEnsureBody, - InstanceExternalIpBody, InstancePutMigrationIdsBody, InstancePutStateBody, - InstancePutStateResponse, InstanceUnregisterResponse, Inventory, - OmicronPhysicalDisksConfig, OmicronZonesConfig, SledRole, TimeSync, - VpcFirewallRulesEnsureBody, ZoneBundleId, ZoneBundleMetadata, Zpool, + BootstoreStatus, CleanupContextUpdate, DatasetsConfig, DiskEnsureBody, + InstanceEnsureBody, InstanceExternalIpBody, InstancePutMigrationIdsBody, + InstancePutStateBody, InstancePutStateResponse, InstanceUnregisterResponse, + Inventory, OmicronPhysicalDisksConfig, OmicronZonesConfig, SledRole, + TimeSync, VpcFirewallRulesEnsureBody, ZoneBundleId, ZoneBundleMetadata, + Zpool, }; use crate::sled_agent::Error as SledAgentError; use crate::zone_bundle; @@ -38,6 +39,7 @@ use omicron_uuid_kinds::{GenericUuid, InstanceUuid}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use sled_hardware::DiskVariant; +use sled_storage::resources::DatasetsManagementResult; use sled_storage::resources::DisksManagementResult; use std::collections::BTreeMap; use uuid::Uuid; @@ -62,6 +64,8 @@ pub fn api() -> SledApiDescription { api.register(omicron_zones_get)?; api.register(omicron_zones_put)?; api.register(zones_list)?; + api.register(datasets_get)?; + api.register(datasets_put)?; api.register(omicron_physical_disks_get)?; api.register(omicron_physical_disks_put)?; api.register(zone_bundle_list)?; @@ -345,6 +349,31 @@ async fn omicron_zones_get( Ok(HttpResponseOk(sa.omicron_zones_list().await?)) } +#[endpoint { + method = PUT, + path = "/datasets", +}] +async fn datasets_put( + rqctx: RequestContext, + body: TypedBody, +) -> Result, HttpError> { + let sa = rqctx.context(); + let body_args = body.into_inner(); + let result = sa.datasets_ensure(body_args).await?; + Ok(HttpResponseOk(result)) +} + +#[endpoint { + method = GET, + path = "/datasets", +}] +async fn datasets_get( + rqctx: RequestContext, +) -> Result, HttpError> { + let sa = rqctx.context(); + Ok(HttpResponseOk(sa.datasets_list().await?)) +} + #[endpoint { method = PUT, path = "/omicron-physical-disks", diff --git a/sled-agent/src/params.rs b/sled-agent/src/params.rs index 465a4abb56..836b030a87 100644 --- a/sled-agent/src/params.rs +++ b/sled-agent/src/params.rs @@ -300,6 +300,8 @@ pub type OmicronPhysicalDiskConfig = sled_storage::disk::OmicronPhysicalDiskConfig; pub type OmicronPhysicalDisksConfig = sled_storage::disk::OmicronPhysicalDisksConfig; +pub type DatasetConfig = sled_storage::disk::DatasetConfig; +pub type DatasetsConfig = sled_storage::disk::DatasetsConfig; /// Describes the set of Omicron-managed zones running on a sled #[derive( diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index dc946c1bfa..66e457b181 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -19,11 +19,12 @@ use crate::nexus::{ NexusNotifierTask, }; use crate::params::{ - DiskStateRequested, InstanceExternalIpBody, InstanceHardware, - InstanceMetadata, InstanceMigrationSourceParams, InstancePutStateResponse, - InstanceStateRequested, InstanceUnregisterResponse, Inventory, - OmicronPhysicalDisksConfig, OmicronZonesConfig, SledRole, TimeSync, - VpcFirewallRule, ZoneBundleMetadata, Zpool, + DatasetsConfig, DiskStateRequested, InstanceExternalIpBody, + InstanceHardware, InstanceMetadata, InstanceMigrationSourceParams, + InstancePutStateResponse, InstanceStateRequested, + InstanceUnregisterResponse, Inventory, OmicronPhysicalDisksConfig, + OmicronZonesConfig, SledRole, TimeSync, VpcFirewallRule, + ZoneBundleMetadata, Zpool, }; use crate::probe_manager::ProbeManager; use crate::services::{self, ServiceManager}; @@ -67,6 +68,7 @@ use sled_hardware::{underlay, HardwareManager}; use sled_hardware_types::underlay::BootstrapInterface; use sled_hardware_types::Baseboard; use sled_storage::manager::StorageHandle; +use sled_storage::resources::DatasetsManagementResult; use sled_storage::resources::DisksManagementResult; use slog::Logger; use std::collections::BTreeMap; @@ -803,6 +805,25 @@ impl SledAgent { self.inner.zone_bundler.cleanup().await.map_err(Error::from) } + pub async fn datasets_list(&self) -> Result { + Ok(self.storage().datasets_list().await?) + } + + pub async fn datasets_ensure( + &self, + config: DatasetsConfig, + ) -> Result { + info!(self.log, "datasets ensure"); + let datasets_result = self.storage().datasets_ensure(config).await?; + info!(self.log, "datasets ensure: Updated storage"); + + // TODO: See omicron_physical_disks_ensure, below - do we similarly + // need to ensure that old datasets are no longer in-use before we + // return here? + + Ok(datasets_result) + } + /// Requests the set of physical disks currently managed by the Sled Agent. /// /// This should be contrasted by the set of disks in the inventory, which @@ -891,7 +912,7 @@ impl SledAgent { &self, requested_zones: OmicronZonesConfig, ) -> Result<(), Error> { - // TODO: + // TODO(https://github.com/oxidecomputer/omicron/issues/6043): // - If these are the set of filesystems, we should also consider // removing the ones which are not listed here. // - It's probably worth sending a bulk request to the storage system, diff --git a/sled-storage/src/dataset.rs b/sled-storage/src/dataset.rs index 7846826ee8..c1267f81b6 100644 --- a/sled-storage/src/dataset.rs +++ b/sled-storage/src/dataset.rs @@ -129,7 +129,16 @@ impl ExpectedDataset { /// The type of a dataset, and an auxiliary information necessary /// to successfully launch a zone managing the associated data. #[derive( - Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash, + Clone, + Debug, + Deserialize, + Serialize, + JsonSchema, + PartialEq, + Eq, + Hash, + Ord, + PartialOrd, )] #[serde(tag = "type", rename_all = "snake_case")] pub enum DatasetKind { @@ -198,7 +207,16 @@ impl std::fmt::Display for DatasetKind { } #[derive( - Debug, PartialEq, Eq, Hash, Serialize, Deserialize, Clone, JsonSchema, + Debug, + PartialEq, + Eq, + Hash, + Serialize, + Deserialize, + Clone, + JsonSchema, + PartialOrd, + Ord, )] pub struct DatasetName { // A unique identifier for the Zpool on which the dataset is stored. @@ -412,7 +430,8 @@ pub(crate) async fn ensure_zpool_has_datasets( let encryption_details = None; let size_details = Some(SizeDetails { quota: dataset.quota, - compression: dataset.compression, + reservation: None, + compression: dataset.compression.map(|s| s.to_string()), }); Zfs::ensure_filesystem( name, diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index c67cce0dfc..982e2bee26 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -12,6 +12,7 @@ use omicron_common::api::external::Generation; use omicron_common::disk::DiskIdentity; use omicron_common::ledger::Ledgerable; use omicron_common::zpool_name::{ZpoolKind, ZpoolName}; +use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::ZpoolUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -43,6 +44,68 @@ pub struct OmicronPhysicalDiskConfig { pub pool_id: ZpoolUuid, } +/// Configuration information necessary to request a single dataset +#[derive( + Clone, + Debug, + Deserialize, + Serialize, + JsonSchema, + PartialEq, + Eq, + Hash, + PartialOrd, + Ord, +)] +pub struct DatasetConfig { + /// The UUID of the dataset being requested + pub id: DatasetUuid, + + /// The dataset's name + pub name: dataset::DatasetName, + + /// The compression mode to be supplied, if any + pub compression: Option, + + /// The upper bound on the amount of storage used by this dataset + pub quota: Option, + + /// The lower bound on the amount of storage usable by this dataset + pub reservation: Option, +} + +#[derive( + Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash, +)] +pub struct DatasetsConfig { + /// generation number of this configuration + /// + /// This generation number is owned by the control plane (i.e., RSS or + /// Nexus, depending on whether RSS-to-Nexus handoff has happened). It + /// should not be bumped within Sled Agent. + /// + /// Sled Agent rejects attempts to set the configuration to a generation + /// older than the one it's currently running. + pub generation: Generation, + + pub datasets: Vec, +} + +impl Default for DatasetsConfig { + fn default() -> Self { + Self { generation: Generation::new(), datasets: vec![] } + } +} + +impl Ledgerable for DatasetsConfig { + fn is_newer_than(&self, other: &Self) -> bool { + self.generation > other.generation + } + + // No need to do this, the generation number is provided externally. + fn generation_bump(&mut self) {} +} + #[derive( Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq, Hash, )] diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index e081bc5034..21da17a8ad 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -8,9 +8,14 @@ use std::collections::HashSet; use crate::config::MountConfig; use crate::dataset::{DatasetName, CONFIG_DATASET}; -use crate::disk::{OmicronPhysicalDisksConfig, RawDisk}; +use crate::disk::{ + DatasetConfig, DatasetsConfig, OmicronPhysicalDisksConfig, RawDisk, +}; use crate::error::Error; -use crate::resources::{AllDisks, DisksManagementResult, StorageResources}; +use crate::resources::{ + AllDisks, DatasetManagementStatus, DatasetsManagementResult, + DisksManagementResult, StorageResources, +}; use camino::Utf8PathBuf; use debug_ignore::DebugIgnore; use futures::future::FutureExt; @@ -19,6 +24,8 @@ use illumos_utils::zpool::ZpoolName; use key_manager::StorageKeyRequester; use omicron_common::disk::DiskIdentity; use omicron_common::ledger::Ledger; +use omicron_uuid_kinds::DatasetUuid; +use omicron_uuid_kinds::GenericUuid; use sled_hardware::DiskVariant; use slog::{info, o, warn, Logger}; use std::future::Future; @@ -112,6 +119,16 @@ pub(crate) enum StorageRequest { tx: DebugIgnore>>, }, + DatasetsEnsure { + config: DatasetsConfig, + tx: DebugIgnore< + oneshot::Sender>, + >, + }, + DatasetsList { + tx: DebugIgnore>>, + }, + // Requests to explicitly manage or stop managing a set of devices OmicronPhysicalDisksEnsure { config: OmicronPhysicalDisksConfig, @@ -238,6 +255,31 @@ impl StorageHandle { rx.map(|result| result.unwrap()) } + pub async fn datasets_ensure( + &self, + config: DatasetsConfig, + ) -> Result { + let (tx, rx) = oneshot::channel(); + self.tx + .send(StorageRequest::DatasetsEnsure { config, tx: tx.into() }) + .await + .unwrap(); + + rx.await.unwrap() + } + + /// Reads the last value written to storage by + /// [Self::datasets_ensure]. + pub async fn datasets_list(&self) -> Result { + let (tx, rx) = oneshot::channel(); + self.tx + .send(StorageRequest::DatasetsList { tx: tx.into() }) + .await + .unwrap(); + + rx.await.unwrap() + } + pub async fn omicron_physical_disks_ensure( &self, config: OmicronPhysicalDisksConfig, @@ -320,6 +362,10 @@ impl StorageHandle { rx.await.unwrap() } + // TODO(https://github.com/oxidecomputer/omicron/issues/6043): + // + // Deprecate usage of this function, prefer to call "datasets_ensure" + // and ask for the set of all datasets from Nexus. pub async fn upsert_filesystem( &self, dataset_id: Uuid, @@ -426,6 +472,12 @@ impl StorageManager { self.ensure_using_exactly_these_disks(raw_disks).await; let _ = tx.0.send(Ok(())); } + StorageRequest::DatasetsEnsure { config, tx } => { + let _ = tx.0.send(self.datasets_ensure(config).await); + } + StorageRequest::DatasetsList { tx } => { + let _ = tx.0.send(self.datasets_list().await); + } StorageRequest::OmicronPhysicalDisksEnsure { config, tx } => { let _ = tx.0.send(self.omicron_physical_disks_ensure(config).await); @@ -592,6 +644,103 @@ impl StorageManager { Ok(()) } + async fn datasets_ensure( + &mut self, + mut config: DatasetsConfig, + ) -> Result { + let log = self.log.new(o!("request" => "datasets_ensure")); + + // Ensure that the datasets arrive in a consistent order + config.datasets.sort_by(|a, b| a.id.partial_cmp(&b.id).unwrap()); + + // We rely on the schema being stable across reboots -- observe + // "test_datasets_schema" below for that property guarantee. + let ledger_paths = self.all_omicron_disk_ledgers().await; + let maybe_ledger = + Ledger::::new(&log, ledger_paths.clone()).await; + + let mut ledger = match maybe_ledger { + Some(ledger) => { + info!( + log, + "Comparing 'requested datasets' to ledger on internal storage" + ); + let ledger_data = ledger.data(); + if config.generation < ledger_data.generation { + warn!( + log, + "Request looks out-of-date compared to prior request" + ); + return Err(Error::PhysicalDiskConfigurationOutdated { + requested: config.generation, + current: ledger_data.generation, + }); + } + + // TODO: If the generation is equal, check that the values are + // also equal. + + info!(log, "Request looks newer than prior requests"); + ledger + } + None => { + info!(log, "No previously-stored 'requested datasets', creating new ledger"); + Ledger::::new_with( + &log, + ledger_paths.clone(), + DatasetsConfig::default(), + ) + } + }; + + let result = self.datasets_ensure_internal(&log, &config).await; + + let ledger_data = ledger.data_mut(); + if *ledger_data == config { + return Ok(result); + } + *ledger_data = config; + ledger.commit().await?; + + Ok(result) + } + + async fn datasets_ensure_internal( + &mut self, + log: &Logger, + config: &DatasetsConfig, + ) -> DatasetsManagementResult { + let mut status = vec![]; + for dataset in &config.datasets { + status.push(self.dataset_ensure_internal(log, dataset).await); + } + DatasetsManagementResult { status } + } + + async fn dataset_ensure_internal( + &mut self, + log: &Logger, + config: &DatasetConfig, + ) -> DatasetManagementStatus { + info!(log, "Ensuring dataset"; "name" => config.name.full_name()); + let mut status = DatasetManagementStatus { + dataset_name: config.name.clone(), + err: None, + }; + + if let Err(err) = self.ensure_dataset(config).await { + status.err = Some(err.to_string()); + }; + + status + } + + async fn datasets_list(&mut self) -> Result { + let log = self.log.new(o!("request" => "datasets_list")); + + todo!(); + } + // Makes an U.2 disk managed by the control plane within [`StorageResources`]. async fn omicron_physical_disks_ensure( &mut self, @@ -763,6 +912,60 @@ impl StorageManager { } } + // Ensures a dataset exists within a zpool, according to `config`. + async fn ensure_dataset( + &mut self, + config: &DatasetConfig, + ) -> Result<(), Error> { + info!(self.log, "ensure_dataset"; "config" => ?config); + if !self + .resources + .disks() + .iter_managed() + .any(|(_, disk)| disk.zpool_name() == config.name.pool()) + { + return Err(Error::ZpoolNotFound(format!( + "{}", + config.name.pool(), + ))); + } + + let zoned = true; + let fs_name = &config.name.full_name(); + let do_format = true; + let encryption_details = None; + let size_details = Some(illumos_utils::zfs::SizeDetails { + quota: config.quota, + reservation: config.reservation, + compression: config.compression.clone(), + }); + Zfs::ensure_filesystem( + fs_name, + Mountpoint::Path(Utf8PathBuf::from("/data")), + zoned, + do_format, + encryption_details, + size_details, + 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::() { + if id != config.id { + return Err(Error::UuidMismatch { + name: Box::new(config.name.clone()), + old: id.into_untyped_uuid(), + new: config.id.into_untyped_uuid(), + }); + } + return Ok(()); + } + } + Zfs::set_oxide_value(&fs_name, "uuid", &config.id.to_string())?; + + Ok(()) + } + // Attempts to add a dataset within a zpool, according to `request`. async fn add_dataset( &mut self, @@ -1320,4 +1523,13 @@ mod test { &serde_json::to_string_pretty(&schema).unwrap(), ); } + + #[test] + fn test_datasets_schema() { + let schema = schemars::schema_for!(DatasetsConfig); + expectorate::assert_contents( + "../schema/datasets.json", + &serde_json::to_string_pretty(&schema).unwrap(), + ); + } } diff --git a/sled-storage/src/resources.rs b/sled-storage/src/resources.rs index f02f62e0a6..a13e816d11 100644 --- a/sled-storage/src/resources.rs +++ b/sled-storage/src/resources.rs @@ -57,6 +57,23 @@ impl DiskManagementError { } } +/// Identifies how a single dataset management operation may have succeeded or +/// failed. +#[derive(Debug, JsonSchema, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub struct DatasetManagementStatus { + pub dataset_name: crate::dataset::DatasetName, + pub err: Option, +} + +/// The result from attempting to manage datasets. +#[derive(Default, Debug, JsonSchema, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +#[must_use = "this `DatasetManagementResult` may contain errors, which should be handled"] +pub struct DatasetsManagementResult { + pub status: Vec, +} + /// Identifies how a single disk management operation may have succeeded or /// failed. #[derive(Debug, JsonSchema, Serialize, Deserialize)] diff --git a/uuid-kinds/src/lib.rs b/uuid-kinds/src/lib.rs index 53acc9c1ed..ebc505d7e3 100644 --- a/uuid-kinds/src/lib.rs +++ b/uuid-kinds/src/lib.rs @@ -50,6 +50,7 @@ macro_rules! impl_typed_uuid_kind { impl_typed_uuid_kind! { Collection => "collection", + Dataset => "dataset", Downstairs => "downstairs", DownstairsRegion => "downstairs_region", ExternalIp => "external_ip", From a80313dc82fe3f7fbf7d0b6a86c93ce79ee9bc96 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Jul 2024 11:55:44 -0700 Subject: [PATCH 02/36] list implementation --- sled-storage/src/error.rs | 6 ++++ sled-storage/src/manager.rs | 66 ++++++++++++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/sled-storage/src/error.rs b/sled-storage/src/error.rs index 4c5582fd79..c10095ad6d 100644 --- a/sled-storage/src/error.rs +++ b/sled-storage/src/error.rs @@ -83,6 +83,12 @@ pub enum Error { current: Generation, }, + #[error("Dataset configuration out-of-date (asked for {requested}, but latest is {current})")] + DatasetConfigurationOutdated { + requested: Generation, + current: Generation, + }, + #[error("Failed to update ledger in internal storage")] Ledger(#[from] omicron_common::ledger::Error), diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 21da17a8ad..f77e703829 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -67,6 +67,9 @@ const SYNCHRONIZE_INTERVAL: Duration = Duration::from_secs(10); // The filename of the ledger storing physical disk info const DISKS_LEDGER_FILENAME: &str = "omicron-physical-disks.json"; +// The filename of the ledger storing dataset info +const DATASETS_LEDGER_FILENAME: &str = "omicron-datasets.json"; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum StorageManagerState { // We know that any attempts to manage disks will fail, as the key manager @@ -535,6 +538,10 @@ impl StorageManager { ); } + // Sled Agents can remember which disks they need to manage by reading + // a configuration file from the M.2s. + // + // This function returns the paths to those configuration files. async fn all_omicron_disk_ledgers(&self) -> Vec { self.resources .disks() @@ -544,6 +551,19 @@ impl StorageManager { .collect() } + // Sled Agents can remember which datasets they need to manage by reading + // a configuration file from the M.2s. + // + // This function returns the paths to those configuration files. + async fn all_omicron_dataset_ledgers(&self) -> Vec { + self.resources + .disks() + .all_m2_mountpoints(CONFIG_DATASET) + .into_iter() + .map(|p| p.join(DATASETS_LEDGER_FILENAME)) + .collect() + } + // Manages a newly detected disk that has been attached to this sled. // // For U.2s: we update our inventory. @@ -595,9 +615,9 @@ impl StorageManager { self.resources.insert_or_update_disk(raw_disk).await } - async fn load_ledger(&self) -> Option> { + async fn load_disks_ledger(&self) -> Option> { let ledger_paths = self.all_omicron_disk_ledgers().await; - let log = self.log.new(o!("request" => "load_ledger")); + let log = self.log.new(o!("request" => "load_disks_ledger")); let maybe_ledger = Ledger::::new( &log, ledger_paths.clone(), @@ -629,7 +649,7 @@ impl StorageManager { // Now that we're actually able to unpack U.2s, attempt to load the // set of disks which we previously stored in the ledger, if one // existed. - let ledger = self.load_ledger().await; + let ledger = self.load_disks_ledger().await; if let Some(ledger) = ledger { info!(self.log, "Setting StorageResources state to match ledger"); @@ -641,6 +661,12 @@ impl StorageManager { info!(self.log, "KeyManager ready, but no ledger detected"); } + // We don't load any configuration for datasets, since we aren't + // currently storing any dataset information in-memory. + // + // If we ever wanted to do so, however, we could load that information + // here. + Ok(()) } @@ -655,7 +681,7 @@ impl StorageManager { // We rely on the schema being stable across reboots -- observe // "test_datasets_schema" below for that property guarantee. - let ledger_paths = self.all_omicron_disk_ledgers().await; + let ledger_paths = self.all_omicron_dataset_ledgers().await; let maybe_ledger = Ledger::::new(&log, ledger_paths.clone()).await; @@ -671,7 +697,7 @@ impl StorageManager { log, "Request looks out-of-date compared to prior request" ); - return Err(Error::PhysicalDiskConfigurationOutdated { + return Err(Error::DatasetConfigurationOutdated { requested: config.generation, current: ledger_data.generation, }); @@ -722,13 +748,15 @@ impl StorageManager { log: &Logger, config: &DatasetConfig, ) -> DatasetManagementStatus { - info!(log, "Ensuring dataset"; "name" => config.name.full_name()); + let log = log.new(o!("name" => config.name.full_name())); + info!(log, "Ensuring dataset"); let mut status = DatasetManagementStatus { dataset_name: config.name.clone(), err: None, }; if let Err(err) = self.ensure_dataset(config).await { + warn!(log, "Failed to ensure dataset"; "err" => ?err); status.err = Some(err.to_string()); }; @@ -738,7 +766,23 @@ impl StorageManager { async fn datasets_list(&mut self) -> Result { let log = self.log.new(o!("request" => "datasets_list")); - todo!(); + let ledger_paths = self.all_omicron_dataset_ledgers().await; + let maybe_ledger = Ledger::::new( + &log, + ledger_paths.clone(), + ) + .await; + + match maybe_ledger { + Some(ledger) => { + info!(log, "Found ledger on internal storage"); + return Ok(ledger.data().clone()); + } + None => { + info!(log, "No ledger detected on internal storage"); + return Err(Error::LedgerNotFound); + } + } } // Makes an U.2 disk managed by the control plane within [`StorageResources`]. @@ -918,6 +962,10 @@ impl StorageManager { config: &DatasetConfig, ) -> Result<(), Error> { info!(self.log, "ensure_dataset"; "config" => ?config); + + // We can only place datasets within managed disks. + // If a disk is attached to this sled, but not a part of the Control + // Plane, it is treated as "not found" for dataset placement. if !self .resources .disks() @@ -930,6 +978,8 @@ impl StorageManager { ))); } + // TODO: Revisit these args, they might need more configuration + // tweaking. let zoned = true; let fs_name = &config.name.full_name(); let do_format = true; @@ -1528,7 +1578,7 @@ mod test { fn test_datasets_schema() { let schema = schemars::schema_for!(DatasetsConfig); expectorate::assert_contents( - "../schema/datasets.json", + "../schema/omicron-datasets.json", &serde_json::to_string_pretty(&schema).unwrap(), ); } From 7193d1b65667dfd11fe60e6cf920bd1e3ea21c28 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Jul 2024 13:16:59 -0700 Subject: [PATCH 03/36] Tests --- schema/omicron-datasets.json | 198 ++++++++++++++++++++++++++++++++++ sled-storage/src/disk.rs | 4 + sled-storage/src/error.rs | 8 +- sled-storage/src/manager.rs | 110 +++++++++++++++++-- sled-storage/src/resources.rs | 11 ++ 5 files changed, 316 insertions(+), 15 deletions(-) create mode 100644 schema/omicron-datasets.json diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json new file mode 100644 index 0000000000..6d0617b5b3 --- /dev/null +++ b/schema/omicron-datasets.json @@ -0,0 +1,198 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "DatasetsConfig", + "type": "object", + "required": [ + "datasets", + "generation" + ], + "properties": { + "datasets": { + "type": "array", + "items": { + "$ref": "#/definitions/DatasetConfig" + } + }, + "generation": { + "description": "generation number of this configuration\n\nThis generation number is owned by the control plane (i.e., RSS or Nexus, depending on whether RSS-to-Nexus handoff has happened). It should not be bumped within Sled Agent.\n\nSled Agent rejects attempts to set the configuration to a generation older than the one it's currently running.", + "allOf": [ + { + "$ref": "#/definitions/Generation" + } + ] + } + }, + "definitions": { + "DatasetConfig": { + "description": "Configuration information necessary to request a single dataset", + "type": "object", + "required": [ + "id", + "name" + ], + "properties": { + "compression": { + "description": "The compression mode to be supplied, if any", + "type": [ + "string", + "null" + ] + }, + "id": { + "description": "The UUID of the dataset being requested", + "allOf": [ + { + "$ref": "#/definitions/TypedUuidForDatasetKind" + } + ] + }, + "name": { + "description": "The dataset's name", + "allOf": [ + { + "$ref": "#/definitions/DatasetName" + } + ] + }, + "quota": { + "description": "The upper bound on the amount of storage used by this dataset", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + }, + "reservation": { + "description": "The lower bound on the amount of storage usable by this dataset", + "type": [ + "integer", + "null" + ], + "format": "uint", + "minimum": 0.0 + } + } + }, + "DatasetKind": { + "description": "The type of a dataset, and an auxiliary information necessary to successfully launch a zone managing the associated data.", + "oneOf": [ + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "cockroach_db" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "crucible" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse_keeper" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "external_dns" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "internal_dns" + ] + } + } + } + ] + }, + "DatasetName": { + "type": "object", + "required": [ + "kind", + "pool_name" + ], + "properties": { + "kind": { + "$ref": "#/definitions/DatasetKind" + }, + "pool_name": { + "$ref": "#/definitions/ZpoolName" + } + } + }, + "Generation": { + "description": "Generation numbers stored in the database, used for optimistic concurrency control", + "type": "integer", + "format": "uint64", + "minimum": 0.0 + }, + "TypedUuidForDatasetKind": { + "type": "string", + "format": "uuid" + }, + "ZpoolName": { + "title": "The name of a Zpool", + "description": "Zpool names are of the format ox{i,p}_. They are either Internal or External, and should be unique", + "type": "string", + "pattern": "^ox[ip]_[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$" + } + } +} \ No newline at end of file diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index 982e2bee26..7736bfe7ca 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -86,6 +86,10 @@ pub struct DatasetsConfig { /// /// Sled Agent rejects attempts to set the configuration to a generation /// older than the one it's currently running. + /// + /// Note that "Generation::new()", AKA, the first generation number, + /// is reserved for "no datasets". This is the default configuration + /// for a sled before any requests have been made. pub generation: Generation, pub datasets: Vec, diff --git a/sled-storage/src/error.rs b/sled-storage/src/error.rs index c10095ad6d..3b30df9e63 100644 --- a/sled-storage/src/error.rs +++ b/sled-storage/src/error.rs @@ -84,10 +84,10 @@ pub enum Error { }, #[error("Dataset configuration out-of-date (asked for {requested}, but latest is {current})")] - DatasetConfigurationOutdated { - requested: Generation, - current: Generation, - }, + DatasetConfigurationOutdated { requested: Generation, current: Generation }, + + #[error("Dataset configuration changed for the same generation number: {generation}")] + DatasetConfigurationChanged { generation: Generation }, #[error("Failed to update ledger in internal storage")] Ledger(#[from] omicron_common::ledger::Error), diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index f77e703829..06a6c5706f 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -27,7 +27,7 @@ use omicron_common::ledger::Ledger; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::GenericUuid; use sled_hardware::DiskVariant; -use slog::{info, o, warn, Logger}; +use slog::{error, info, o, warn, Logger}; use std::future::Future; use tokio::sync::{mpsc, oneshot, watch}; use tokio::time::{interval, Duration, MissedTickBehavior}; @@ -615,7 +615,9 @@ impl StorageManager { self.resources.insert_or_update_disk(raw_disk).await } - async fn load_disks_ledger(&self) -> Option> { + async fn load_disks_ledger( + &self, + ) -> Option> { let ledger_paths = self.all_omicron_disk_ledgers().await; let log = self.log.new(o!("request" => "load_disks_ledger")); let maybe_ledger = Ledger::::new( @@ -701,12 +703,24 @@ impl StorageManager { requested: config.generation, current: ledger_data.generation, }); - } + } else if config.generation == ledger_data.generation { + info!( + log, + "Requested geenration 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)"); + return Err(Error::DatasetConfigurationChanged { + generation: config.generation, + }); + } + } - info!(log, "Request looks newer than prior requests"); + info!( + log, + "Request looks newer than (or identical to) prior requests" + ); ledger } None => { @@ -767,11 +781,8 @@ impl StorageManager { let log = self.log.new(o!("request" => "datasets_list")); let ledger_paths = self.all_omicron_dataset_ledgers().await; - let maybe_ledger = Ledger::::new( - &log, - ledger_paths.clone(), - ) - .await; + let maybe_ledger = + Ledger::::new(&log, ledger_paths.clone()).await; match maybe_ledger { Some(ledger) => { @@ -1082,6 +1093,7 @@ mod tests { use super::*; use camino_tempfile::tempdir_in; + use omicron_common::api::external::Generation; use omicron_common::ledger; use omicron_test_utils::dev::test_setup_log; use sled_hardware::DiskFirmware; @@ -1560,6 +1572,82 @@ mod tests { harness.cleanup().await; logctx.cleanup_successful(); } + + #[tokio::test] + async fn ensure_datasets() { + illumos_utils::USE_MOCKS.store(false, Ordering::SeqCst); + let logctx = test_setup_log("ensure_datasets"); + let mut harness = StorageManagerTestHarness::new(&logctx.log).await; + + // Test setup: Add a U.2 and M.2, adopt them into the "control plane" + // for usage. + harness.handle().key_manager_ready().await; + let raw_disks = + harness.add_vdevs(&["u2_under_test.vdev", "m2_helping.vdev"]).await; + let config = harness.make_config(1, &raw_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); + + // Create a dataset on the newly formatted U.2 + let id = DatasetUuid::new_v4(); + let zpool_name = ZpoolName::new_external(config.disks[0].pool_id); + let name = DatasetName::new(zpool_name.clone(), DatasetKind::Crucible); + let datasets = vec![DatasetConfig { + id, + name, + compression: None, + quota: None, + reservation: None, + }]; + // "Generation = 1" is reserved as "no requests seen yet", so we jump + // past it. + let generation = Generation::new().next(); + let mut config = DatasetsConfig { generation, datasets }; + + let status = + harness.handle().datasets_ensure(config.clone()).await.unwrap(); + assert!(!status.has_error()); + + // List datasets, expect to see what we just created + let observed_config = harness.handle().datasets_list().await.unwrap(); + assert_eq!(config, observed_config); + + // Calling "datasets_ensure" with the same input should succeed. + let status = + harness.handle().datasets_ensure(config.clone()).await.unwrap(); + assert!(!status.has_error()); + + let current_config_generation = config.generation; + let next_config_generation = config.generation.next(); + + // Calling "datasets_ensure" with an old generation should fail + config.generation = Generation::new(); + let err = + harness.handle().datasets_ensure(config.clone()).await.unwrap_err(); + assert!(matches!(err, Error::DatasetConfigurationOutdated { .. })); + + // However, calling it with a different input and the same generation + // number should fail. + config.generation = current_config_generation; + config.datasets[0].reservation = Some(1024); + let err = + harness.handle().datasets_ensure(config.clone()).await.unwrap_err(); + assert!(matches!(err, Error::DatasetConfigurationChanged { .. })); + + // If we bump the generation number while making a change, updated + // configs will work. + config.generation = next_config_generation; + let status = + harness.handle().datasets_ensure(config.clone()).await.unwrap(); + assert!(!status.has_error()); + + harness.cleanup().await; + logctx.cleanup_successful(); + } } #[cfg(test)] diff --git a/sled-storage/src/resources.rs b/sled-storage/src/resources.rs index a13e816d11..19313738af 100644 --- a/sled-storage/src/resources.rs +++ b/sled-storage/src/resources.rs @@ -74,6 +74,17 @@ pub struct DatasetsManagementResult { pub status: Vec, } +impl DatasetsManagementResult { + pub fn has_error(&self) -> bool { + for status in &self.status { + if status.err.is_some() { + return true; + } + } + false + } +} + /// Identifies how a single disk management operation may have succeeded or /// failed. #[derive(Debug, JsonSchema, Serialize, Deserialize)] From 9464cc1cdab1330d22666dde0cf89ceb60b18f50 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 22 Jul 2024 14:26:54 -0700 Subject: [PATCH 04/36] schemas n stuff --- openapi/sled-agent.json | 261 +++++++++++++++++++++++++++++++++++ schema/omicron-datasets.json | 2 +- 2 files changed, 262 insertions(+), 1 deletion(-) diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index 1323769da2..ac49552c76 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -177,6 +177,60 @@ } } }, + "/datasets": { + "get": { + "operationId": "datasets_get", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DatasetsConfig" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "operationId": "datasets_put", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DatasetsConfig" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/DatasetsManagementResult" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/disks/{disk_id}": { "put": { "operationId": "disk_put", @@ -2103,6 +2157,209 @@ "target" ] }, + "DatasetConfig": { + "description": "Configuration information necessary to request a single dataset", + "type": "object", + "properties": { + "compression": { + "nullable": true, + "description": "The compression mode to be supplied, if any", + "type": "string" + }, + "id": { + "description": "The UUID of the dataset being requested", + "allOf": [ + { + "$ref": "#/components/schemas/TypedUuidForDatasetKind" + } + ] + }, + "name": { + "description": "The dataset's name", + "allOf": [ + { + "$ref": "#/components/schemas/DatasetName" + } + ] + }, + "quota": { + "nullable": true, + "description": "The upper bound on the amount of storage used by this dataset", + "type": "integer", + "format": "uint", + "minimum": 0 + }, + "reservation": { + "nullable": true, + "description": "The lower bound on the amount of storage usable by this dataset", + "type": "integer", + "format": "uint", + "minimum": 0 + } + }, + "required": [ + "id", + "name" + ] + }, + "DatasetKind": { + "description": "The type of a dataset, and an auxiliary information necessary to successfully launch a zone managing the associated data.", + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "cockroach_db" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "crucible" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse_keeper" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "external_dns" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "internal_dns" + ] + } + }, + "required": [ + "type" + ] + } + ] + }, + "DatasetManagementStatus": { + "description": "Identifies how a single dataset management operation may have succeeded or failed.", + "type": "object", + "properties": { + "dataset_name": { + "$ref": "#/components/schemas/DatasetName" + }, + "err": { + "nullable": true, + "type": "string" + } + }, + "required": [ + "dataset_name" + ] + }, + "DatasetName": { + "type": "object", + "properties": { + "kind": { + "$ref": "#/components/schemas/DatasetKind" + }, + "pool_name": { + "$ref": "#/components/schemas/ZpoolName" + } + }, + "required": [ + "kind", + "pool_name" + ] + }, + "DatasetsConfig": { + "type": "object", + "properties": { + "datasets": { + "type": "array", + "items": { + "$ref": "#/components/schemas/DatasetConfig" + } + }, + "generation": { + "description": "generation number of this configuration\n\nThis generation number is owned by the control plane (i.e., RSS or Nexus, depending on whether RSS-to-Nexus handoff has happened). It should not be bumped within Sled Agent.\n\nSled Agent rejects attempts to set the configuration to a generation older than the one it's currently running.\n\nNote that \"Generation::new()\", AKA, the first generation number, is reserved for \"no datasets\". This is the default configuration for a sled before any requests have been made.", + "allOf": [ + { + "$ref": "#/components/schemas/Generation" + } + ] + } + }, + "required": [ + "datasets", + "generation" + ] + }, + "DatasetsManagementResult": { + "description": "The result from attempting to manage datasets.", + "type": "object", + "properties": { + "status": { + "type": "array", + "items": { + "$ref": "#/components/schemas/DatasetManagementStatus" + } + } + }, + "required": [ + "status" + ] + }, "DhcpConfig": { "description": "DHCP configuration for a port\n\nNot present here: Hostname (DHCPv4 option 12; used in DHCPv6 option 39); we use `InstanceRuntimeState::hostname` for this value.", "type": "object", @@ -4856,6 +5113,10 @@ "sync" ] }, + "TypedUuidForDatasetKind": { + "type": "string", + "format": "uuid" + }, "TypedUuidForPropolisKind": { "type": "string", "format": "uuid" diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index 6d0617b5b3..35da5de627 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -14,7 +14,7 @@ } }, "generation": { - "description": "generation number of this configuration\n\nThis generation number is owned by the control plane (i.e., RSS or Nexus, depending on whether RSS-to-Nexus handoff has happened). It should not be bumped within Sled Agent.\n\nSled Agent rejects attempts to set the configuration to a generation older than the one it's currently running.", + "description": "generation number of this configuration\n\nThis generation number is owned by the control plane (i.e., RSS or Nexus, depending on whether RSS-to-Nexus handoff has happened). It should not be bumped within Sled Agent.\n\nSled Agent rejects attempts to set the configuration to a generation older than the one it's currently running.\n\nNote that \"Generation::new()\", AKA, the first generation number, is reserved for \"no datasets\". This is the default configuration for a sled before any requests have been made.", "allOf": [ { "$ref": "#/definitions/Generation" From 817a39729c2cce1b4eaba7c1a652580b2115afb5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 24 Jul 2024 10:53:52 -0700 Subject: [PATCH 05/36] The sled agent side of datasets in inventory --- illumos-utils/src/zfs.rs | 214 +++++++++++++++++++++++++++++++ sled-agent/src/params.rs | 45 +++++++ sled-agent/src/sim/sled_agent.rs | 2 + sled-agent/src/sled_agent.rs | 44 +++++++ 4 files changed, 305 insertions(+) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 21de2a50da..7dfd574c97 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -5,9 +5,12 @@ //! Utilities for poking at ZFS. use crate::{execute, PFEXEC}; +use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; use omicron_common::disk::DiskIdentity; +use omicron_uuid_kinds::DatasetUuid; use std::fmt; +use std::str::FromStr; // These locations in the ramdisk must only be used by the switch zone. // @@ -207,9 +210,71 @@ pub struct SizeDetails { pub compression: Option, } +#[derive(Debug)] +pub struct DatasetProperties { + /// The Uuid of the dataset + pub id: Option, + /// The full name of the dataset. + pub name: String, + /// Remaining space in the dataset and descendents. + pub avail: u64, + /// Space used by dataset and descendents. + pub used: u64, + /// Maximum space usable by dataset and descendents. + pub quota: Option, + /// Minimum space guaranteed to dataset and descendents. + pub reservation: Option, + /// The compression algorithm used for this dataset. + pub compression: String, +} + +impl FromStr for DatasetProperties { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + let mut iter = s.split_whitespace(); + + let id = match iter.next().context("Missing UUID")? { + "-" => None, + anything_else => Some(anything_else.parse::()?), + }; + + let name = iter.next().context("Missing 'name'")?.to_string(); + let avail = iter.next().context("Missing 'avail'")?.parse::()?; + let used = iter.next().context("Missing 'used'")?.parse::()?; + let quota = + match iter.next().context("Missing 'quota'")?.parse::()? { + 0 => None, + q => Some(q), + }; + let reservation = match iter + .next() + .context("Missing 'reservation'")? + .parse::()? + { + 0 => None, + r => Some(r), + }; + let compression = + iter.next().context("Missing 'compression'")?.to_string(); + + Ok(DatasetProperties { + id, + name, + avail, + used, + quota, + reservation, + compression, + }) + } +} + #[cfg_attr(any(test, feature = "testing"), mockall::automock, allow(dead_code))] impl Zfs { /// Lists all datasets within a pool or existing dataset. + /// + /// Strips the input `name` from the output dataset names. pub fn list_datasets(name: &str) -> Result, ListDatasetsError> { let mut command = std::process::Command::new(ZFS); let cmd = command.args(&["list", "-d", "1", "-rHpo", "name", name]); @@ -228,6 +293,38 @@ impl Zfs { Ok(filesystems) } + /// Get information about datasets within a list of zpools / datasets. + /// + /// This function is similar to [Zfs::list_datasets], but provides a more + /// substantial results about the datasets found. + /// + /// Sorts results and de-duplicates them by name. + pub fn get_dataset_properties( + datasets: &[String], + ) -> Result, anyhow::Error> { + let mut command = std::process::Command::new(ZFS); + let cmd = command.args(&["list", "-d", "1", "-rHpo"]); + + // Note: this is tightly coupled with the layout of DatasetProperties + cmd.arg("oxide:uuid,name,avail,used,quota,reservation,compression"); + cmd.args(datasets); + + let output = execute(cmd).with_context(|| { + format!("Failed to get dataset properties for {datasets:?}") + })?; + let stdout = String::from_utf8_lossy(&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) + } + /// Return the name of a dataset for a ZFS object. /// /// The object can either be a dataset name, or a path, in which case it @@ -679,3 +776,120 @@ pub fn get_all_omicron_datasets_for_delete() -> anyhow::Result> { Ok(datasets) } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn parse_dataset_props() { + let input = + "- dataset_name 1234 5678 0 0 off"; + let props = DatasetProperties::from_str(&input) + .expect("Should have parsed data"); + + assert_eq!(props.id, None); + assert_eq!(props.name, "dataset_name"); + assert_eq!(props.avail, 1234); + assert_eq!(props.used, 5678); + assert_eq!(props.quota, None); + assert_eq!(props.reservation, None); + assert_eq!(props.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) + .expect("Should have parsed data"); + + assert_eq!( + props.id, + Some("d4e1e554-7b98-4413-809e-4a42561c3d0c".parse().unwrap()) + ); + assert_eq!(props.name, "dataset_name"); + assert_eq!(props.avail, 1234); + assert_eq!(props.used, 5678); + assert_eq!(props.quota, Some(111)); + assert_eq!(props.reservation, Some(222)); + assert_eq!(props.compression, "off"); + } + + #[test] + fn parse_dataset_bad_uuid() { + let input = "bad dataset_name 1234 5678 111 222 off"; + let err = DatasetProperties::from_str(&input) + .expect_err("Should have failed to parse"); + assert!( + err.to_string().contains("error parsing UUID (dataset)"), + "{err}" + ); + } + + #[test] + fn parse_dataset_bad_avail() { + let input = "- dataset_name BADAVAIL 5678 111 222 off"; + let err = DatasetProperties::from_str(&input) + .expect_err("Should have failed to parse"); + assert!( + err.to_string().contains("invalid digit found in string"), + "{err}" + ); + } + + #[test] + fn parse_dataset_bad_usage() { + let input = "- dataset_name 1234 BADUSAGE 111 222 off"; + let err = DatasetProperties::from_str(&input) + .expect_err("Should have failed to parse"); + assert!( + err.to_string().contains("invalid digit found in string"), + "{err}" + ); + } + + #[test] + fn parse_dataset_bad_quota() { + let input = "- dataset_name 1234 5678 BADQUOTA 222 off"; + let err = DatasetProperties::from_str(&input) + .expect_err("Should have failed to parse"); + assert!( + err.to_string().contains("invalid digit found in string"), + "{err}" + ); + } + + #[test] + fn parse_dataset_bad_reservation() { + let input = "- dataset_name 1234 5678 111 BADRES off"; + let err = DatasetProperties::from_str(&input) + .expect_err("Should have failed to parse"); + assert!( + err.to_string().contains("invalid digit found in string"), + "{err}" + ); + } + + #[test] + fn parse_dataset_missing_fields() { + let expect_missing = |input: &str, what: &str| { + let err = DatasetProperties::from_str(input) + .expect_err("Should have failed to parse"); + assert!(err.to_string().contains(&format!("Missing {what}"))); + }; + + expect_missing( + "- dataset_name 1234 5678 111 222", + "'compression'", + ); + expect_missing( + "- dataset_name 1234 5678 111", + "'reservation'", + ); + expect_missing("- dataset_name 1234 5678", "'quota'"); + expect_missing("- dataset_name 1234", "'used'"); + expect_missing("- dataset_name", "'avail'"); + expect_missing("-", "'name'"); + expect_missing("", "UUID"); + } +} diff --git a/sled-agent/src/params.rs b/sled-agent/src/params.rs index a421bda3a6..e0b74e11b0 100644 --- a/sled-agent/src/params.rs +++ b/sled-agent/src/params.rs @@ -19,6 +19,7 @@ use omicron_common::api::internal::nexus::{ use omicron_common::api::internal::shared::{ NetworkInterface, SourceNatConfig, }; +use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::PropolisUuid; use omicron_uuid_kinds::ZpoolUuid; use schemars::JsonSchema; @@ -763,6 +764,49 @@ pub struct InventoryZpool { pub total_size: ByteCount, } +/// Identifies information about datasets within Oxide-managed zpools +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] +pub struct InventoryDataset { + /// Although datasets mandated by the control plane will have UUIDs, + /// datasets can be created (and have been created) without UUIDs. + pub id: Option, + + /// This name is the full path of the dataset. + // This is akin to [sled_storage::dataset::DatasetName::full_name], + // and it's also what you'd see when running "zfs list". + pub name: String, + + /// The amount of remaining space usable by the dataset (and children) + /// assuming there is no other activity within the pool. + pub available: u64, + + /// The amount of space consumed by this dataset and descendents. + pub used: u64, + + /// The maximum amount of space usable by a dataset and all descendents. + pub quota: Option, + + /// The minimum amount of space guaranteed to a dataset and descendents. + pub reservation: Option, + + /// The compression algorithm used for this dataset, if any. + pub compression: String, +} + +impl From for InventoryDataset { + fn from(props: illumos_utils::zfs::DatasetProperties) -> Self { + Self { + id: props.id, + name: props.name, + available: props.avail, + used: props.used, + quota: props.quota, + reservation: props.reservation, + compression: props.compression, + } + } +} + /// Identity and basic status information about this sled agent #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] pub struct Inventory { @@ -775,6 +819,7 @@ pub struct Inventory { pub reservoir_size: ByteCount, pub disks: Vec, pub zpools: Vec, + pub datasets: Vec, } #[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index f23b14c377..43d4fd310f 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/sled-agent/src/sim/sled_agent.rs @@ -866,6 +866,8 @@ impl SledAgent { }) }) .collect::, anyhow::Error>>()?, + // TODO: Make this more real? + datasets: vec![], }) } diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 6b212c96ce..6669e8e4ca 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -66,6 +66,7 @@ use sled_agent_types::early_networking::EarlyNetworkConfig; use sled_hardware::{underlay, HardwareManager}; use sled_hardware_types::underlay::BootstrapInterface; use sled_hardware_types::Baseboard; +use sled_storage::dataset::{CRYPT_DATASET, ZONE_DATASET}; use sled_storage::manager::StorageHandle; use sled_storage::resources::DatasetsManagementResult; use sled_storage::resources::DisksManagementResult; @@ -1250,6 +1251,7 @@ impl SledAgent { let mut disks = vec![]; let mut zpools = vec![]; + let mut datasets = vec![]; let all_disks = self.storage().get_latest_disks().await; for (identity, variant, slot, _firmware) in all_disks.iter_all() { disks.push(crate::params::InventoryDisk { @@ -1278,6 +1280,47 @@ impl SledAgent { id: zpool.id(), total_size: ByteCount::try_from(info.size())?, }); + + // We do care about the total space usage within zpools, but mapping + // the layering back to "datasets we care about" is a little + // awkward. + // + // We could query for all datasets within a pool, but the sled agent + // doesn't really care about the children of datasets that it + // allocates. As an example: Sled Agent might provision a "crucible" + // dataset, but how region allocation occurs within that dataset + // is a detail for Crucible to care about, not the Sled Agent. + // + // To balance this effort, we ask for information about datasets + // that the Sled Agent is directly resopnsible for managing. + let datasets_of_interest = [ + // We care about the zpool itself, and all direct children. + zpool.to_string(), + // Likewise, we care about the encrypted dataset, and all + // direct children. + format!("{zpool}/{CRYPT_DATASET}"), + // The zone dataset gives us additional context on "what zones + // have datasets provisioned". + format!("{zpool}/{ZONE_DATASET}"), + ]; + let inv_props = + match illumos_utils::zfs::Zfs::get_dataset_properties( + datasets_of_interest.as_slice(), + ) { + Ok(props) => props.into_iter().map(|prop| { + crate::params::InventoryDataset::from(prop) + }), + Err(err) => { + warn!( + self.log, + "Failed to access dataset info within zpool"; + "zpool" => %zpool, + "err" => %err + ); + continue; + } + }; + datasets.extend(inv_props); } Ok(Inventory { @@ -1290,6 +1333,7 @@ impl SledAgent { reservoir_size, disks, zpools, + datasets, }) } } From 54b465832ab38648433c258bd56352b35de17a99 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 24 Jul 2024 17:54:26 -0700 Subject: [PATCH 06/36] Plumb inventory into nexus, omdb --- clients/sled-agent-client/src/lib.rs | 1 + dev-tools/omdb/src/bin/omdb/db.rs | 38 ++++++++++ dev-tools/omdb/src/bin/omdb/sled_agent.rs | 33 +++++++++ dev-tools/omdb/tests/usage_errors.out | 1 + illumos-utils/src/zfs.rs | 37 ++++++---- nexus/db-model/src/inventory.rs | 53 +++++++++++++- nexus/db-model/src/schema.rs | 15 ++++ .../db-queries/src/db/datastore/inventory.rs | 68 ++++++++++++++++++ .../src/db/datastore/physical_disk.rs | 1 + nexus/inventory/src/builder.rs | 5 ++ nexus/inventory/src/examples.rs | 7 ++ nexus/reconfigurator/planning/src/system.rs | 6 +- nexus/types/src/inventory.rs | 43 ++++++++++++ openapi/sled-agent.json | 70 +++++++++++++++++++ schema/crdb/dbinit.sql | 26 +++++++ sled-agent/src/params.rs | 8 +-- sled-agent/src/rack_setup/service.rs | 1 + 17 files changed, 392 insertions(+), 21 deletions(-) diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index 8a63cecd4f..7cb1121988 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/clients/sled-agent-client/src/lib.rs @@ -58,6 +58,7 @@ progenitor::generate_api!( RouterVersion = omicron_common::api::internal::shared::RouterVersion, SourceNatConfig = omicron_common::api::internal::shared::SourceNatConfig, SwitchLocation = omicron_common::api::external::SwitchLocation, + TypedUuidForDatasetKind = omicron_uuid_kinds::DatasetUuid, TypedUuidForInstanceKind = omicron_uuid_kinds::InstanceUuid, TypedUuidForPropolisKind = omicron_uuid_kinds::PropolisUuid, TypedUuidForZpoolKind = omicron_uuid_kinds::ZpoolUuid, diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index 98669ddc06..516a0f5028 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -3720,6 +3720,44 @@ fn inv_collection_print_sleds(collection: &Collection) { sled.reservoir_size.to_whole_gibibytes() ); + if !sled.zpools.is_empty() { + println!(" physical disks:"); + } + for disk in &sled.disks { + let nexus_types::inventory::PhysicalDisk { + identity, + variant, + slot, + } = disk; + println!(" {variant:?}: {identity:?} in {slot}"); + } + + if !sled.zpools.is_empty() { + println!(" zpools"); + } + for zpool in &sled.zpools { + let nexus_types::inventory::Zpool { id, total_size, .. } = zpool; + println!(" {id}: total size: {total_size}"); + } + + if !sled.datasets.is_empty() { + println!(" datasets:"); + } + for dataset in &sled.datasets { + let nexus_types::inventory::Dataset { + id, + name, + available, + used, + quota, + reservation, + compression, + } = dataset; + println!(" {name} - id: {id:?}, compression: {compression}"); + println!(" available: {available}, used: {used}"); + println!(" reservation: {reservation:?}, quota: {quota:?}"); + } + if let Some(zones) = collection.omicron_zones.get(&sled.sled_id) { println!( " zones collected from {} at {}", diff --git a/dev-tools/omdb/src/bin/omdb/sled_agent.rs b/dev-tools/omdb/src/bin/omdb/sled_agent.rs index 9a9a17eff4..b97fb35e8c 100644 --- a/dev-tools/omdb/src/bin/omdb/sled_agent.rs +++ b/dev-tools/omdb/src/bin/omdb/sled_agent.rs @@ -38,6 +38,10 @@ enum SledAgentCommands { #[clap(subcommand)] Zpools(ZpoolCommands), + /// print information about datasets + #[clap(subcommand)] + Datasets(DatasetCommands), + /// print information about the local bootstore node #[clap(subcommand)] Bootstore(BootstoreCommands), @@ -55,6 +59,12 @@ enum ZpoolCommands { List, } +#[derive(Debug, Subcommand)] +enum DatasetCommands { + /// Print list of all datasets managed by the sled agent + List, +} + #[derive(Debug, Subcommand)] enum BootstoreCommands { /// Show the internal state of the local bootstore node @@ -86,6 +96,9 @@ impl SledAgentArgs { SledAgentCommands::Zpools(ZpoolCommands::List) => { cmd_zpools_list(&client).await } + SledAgentCommands::Datasets(DatasetCommands::List) => { + cmd_datasets_list(&client).await + } SledAgentCommands::Bootstore(BootstoreCommands::Status) => { cmd_bootstore_status(&client).await } @@ -130,6 +143,26 @@ async fn cmd_zpools_list( Ok(()) } +/// Runs `omdb sled-agent datasets list` +async fn cmd_datasets_list( + client: &sled_agent_client::Client, +) -> Result<(), anyhow::Error> { + let response = client.datasets_get().await.context("listing datasets")?; + let response = response.into_inner(); + + println!("dataset configuration @ generation {}:", response.generation); + let datasets = response.datasets; + + if datasets.is_empty() { + println!(" "); + } + for dataset in &datasets { + println!(" {:?}", dataset); + } + + Ok(()) +} + /// Runs `omdb sled-agent bootstore status` async fn cmd_bootstore_status( client: &sled_agent_client::Client, diff --git a/dev-tools/omdb/tests/usage_errors.out b/dev-tools/omdb/tests/usage_errors.out index 3d6f2af112..8b240958ba 100644 --- a/dev-tools/omdb/tests/usage_errors.out +++ b/dev-tools/omdb/tests/usage_errors.out @@ -556,6 +556,7 @@ Usage: omdb sled-agent [OPTIONS] Commands: zones print information about zones zpools print information about zpools + datasets print information about datasets bootstore print information about the local bootstore node help Print this message or the help of the given subcommand(s) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 7dfd574c97..1eaf946911 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -7,6 +7,7 @@ use crate::{execute, PFEXEC}; use anyhow::Context; use camino::{Utf8Path, Utf8PathBuf}; +use omicron_common::api::external::ByteCount; use omicron_common::disk::DiskIdentity; use omicron_uuid_kinds::DatasetUuid; use std::fmt; @@ -217,13 +218,13 @@ pub struct DatasetProperties { /// The full name of the dataset. pub name: String, /// Remaining space in the dataset and descendents. - pub avail: u64, + pub avail: ByteCount, /// Space used by dataset and descendents. - pub used: u64, + pub used: ByteCount, /// Maximum space usable by dataset and descendents. - pub quota: Option, + pub quota: Option, /// Minimum space guaranteed to dataset and descendents. - pub reservation: Option, + pub reservation: Option, /// The compression algorithm used for this dataset. pub compression: String, } @@ -240,12 +241,20 @@ impl FromStr for DatasetProperties { }; let name = iter.next().context("Missing 'name'")?.to_string(); - let avail = iter.next().context("Missing 'avail'")?.parse::()?; - let used = iter.next().context("Missing 'used'")?.parse::()?; + 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), + q => Some(q.try_into()?), }; let reservation = match iter .next() @@ -253,7 +262,7 @@ impl FromStr for DatasetProperties { .parse::()? { 0 => None, - r => Some(r), + r => Some(r.try_into()?), }; let compression = iter.next().context("Missing 'compression'")?.to_string(); @@ -790,8 +799,8 @@ mod test { assert_eq!(props.id, None); assert_eq!(props.name, "dataset_name"); - assert_eq!(props.avail, 1234); - assert_eq!(props.used, 5678); + 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"); @@ -808,10 +817,10 @@ mod test { Some("d4e1e554-7b98-4413-809e-4a42561c3d0c".parse().unwrap()) ); assert_eq!(props.name, "dataset_name"); - assert_eq!(props.avail, 1234); - assert_eq!(props.used, 5678); - assert_eq!(props.quota, Some(111)); - assert_eq!(props.reservation, Some(222)); + 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"); } diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 14c4684e1e..cd259701e2 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -7,7 +7,7 @@ use crate::omicron_zone_config::{OmicronZone, OmicronZoneNic}; use crate::schema::{ hw_baseboard_id, inv_caboose, inv_collection, inv_collection_error, - inv_omicron_zone, inv_omicron_zone_nic, inv_physical_disk, + inv_dataset, inv_omicron_zone, inv_omicron_zone_nic, inv_physical_disk, inv_root_of_trust, inv_root_of_trust_page, inv_service_processor, inv_sled_agent, inv_sled_omicron_zones, inv_zpool, sw_caboose, sw_root_of_trust_page, @@ -34,6 +34,7 @@ use nexus_types::inventory::{ use omicron_common::api::internal::shared::NetworkInterface; use omicron_uuid_kinds::CollectionKind; use omicron_uuid_kinds::CollectionUuid; +use omicron_uuid_kinds::DatasetKind; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::SledKind; use omicron_uuid_kinds::SledUuid; @@ -921,6 +922,56 @@ impl From for nexus_types::inventory::Zpool { } } +/// See [`nexus_types::inventory::Dataset`]. +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = inv_dataset)] +pub struct InvDataset { + pub inv_collection_id: DbTypedUuid, + pub sled_id: DbTypedUuid, + pub id: Option>, + pub name: String, + pub available: ByteCount, + pub used: ByteCount, + pub quota: Option, + pub reservation: Option, + pub compression: String, +} + +impl InvDataset { + pub fn new( + inv_collection_id: CollectionUuid, + sled_id: SledUuid, + dataset: &nexus_types::inventory::Dataset, + ) -> Self { + Self { + inv_collection_id: inv_collection_id.into(), + sled_id: sled_id.into(), + + id: dataset.id.map(|id| id.into()), + name: dataset.name.clone(), + available: dataset.available.into(), + used: dataset.used.into(), + quota: dataset.quota.map(|q| q.into()), + reservation: dataset.reservation.map(|r| r.into()), + compression: dataset.compression.clone(), + } + } +} + +impl From for nexus_types::inventory::Dataset { + fn from(dataset: InvDataset) -> Self { + Self { + id: dataset.id.map(|id| id.0), + name: dataset.name, + available: *dataset.available, + used: *dataset.used, + quota: dataset.quota.map(|q| *q), + reservation: dataset.reservation.map(|r| *r), + compression: dataset.compression, + } + } +} + /// See [`nexus_types::inventory::OmicronZonesFound`]. #[derive(Queryable, Clone, Debug, Selectable, Insertable)] #[diesel(table_name = inv_sled_omicron_zones)] diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index dc57de9263..4412f642b5 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1442,6 +1442,21 @@ table! { } } +table! { + inv_dataset (inv_collection_id, sled_id, name) { + inv_collection_id -> Uuid, + sled_id -> Uuid, + + id -> Nullable, + name -> Text, + available -> Int8, + used -> Int8, + quota -> Nullable, + reservation -> Nullable, + compression -> Text, + } +} + table! { inv_sled_omicron_zones (inv_collection_id, sled_id) { inv_collection_id -> Uuid, diff --git a/nexus/db-queries/src/db/datastore/inventory.rs b/nexus/db-queries/src/db/datastore/inventory.rs index 289e443213..4840af9076 100644 --- a/nexus/db-queries/src/db/datastore/inventory.rs +++ b/nexus/db-queries/src/db/datastore/inventory.rs @@ -38,6 +38,7 @@ use nexus_db_model::HwRotSlotEnum; use nexus_db_model::InvCaboose; use nexus_db_model::InvCollection; use nexus_db_model::InvCollectionError; +use nexus_db_model::InvDataset; use nexus_db_model::InvOmicronZone; use nexus_db_model::InvOmicronZoneNic; use nexus_db_model::InvPhysicalDisk; @@ -157,6 +158,17 @@ impl DataStore { }) .collect(); + // Pull datasets out of all sled agents + let datasets: Vec<_> = collection + .sled_agents + .iter() + .flat_map(|(sled_id, sled_agent)| { + sled_agent.datasets.iter().map(|dataset| { + InvDataset::new(collection_id, *sled_id, dataset) + }) + }) + .collect(); + // Partition the sled agents into those with an associated baseboard id // and those without one. We handle these pretty differently. let (sled_agents_baseboards, sled_agents_no_baseboards): ( @@ -745,6 +757,25 @@ impl DataStore { } } + // Insert rows for all the datasets we found. + { + use db::schema::inv_dataset::dsl; + + let batch_size = SQL_BATCH_SIZE.get().try_into().unwrap(); + let mut datasets = datasets.into_iter(); + loop { + let some_datasets = + datasets.by_ref().take(batch_size).collect::>(); + if some_datasets.is_empty() { + break; + } + let _ = diesel::insert_into(dsl::inv_dataset) + .values(some_datasets) + .execute_async(&conn) + .await?; + } + } + // Insert rows for the sled agents that we found. In practice, we'd // expect these to all have baseboards (if using Oxide hardware) or // none have baseboards (if not). @@ -1601,6 +1632,39 @@ impl DataStore { zpools }; + // Mapping of "Sled ID" -> "All datasets reported by that sled" + let datasets: BTreeMap> = { + use db::schema::inv_dataset::dsl; + + let mut datasets = + BTreeMap::>::new(); + let mut paginator = Paginator::new(batch_size); + while let Some(p) = paginator.next() { + let batch = paginated_multicolumn( + dsl::inv_dataset, + (dsl::sled_id, dsl::name), + &p.current_pagparams(), + ) + .filter(dsl::inv_collection_id.eq(db_id)) + .select(InvDataset::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + paginator = p.found_batch(&batch, &|row| { + (row.sled_id, row.name.clone()) + }); + for dataset in batch { + datasets + .entry(dataset.sled_id.into_untyped_uuid()) + .or_default() + .push(dataset.into()); + } + } + datasets + }; + // Collect the unique baseboard ids referenced by SPs, RoTs, and Sled // Agents. let baseboard_id_ids: BTreeSet<_> = sps @@ -1709,6 +1773,10 @@ impl DataStore { .get(sled_id.as_untyped_uuid()) .map(|zpools| zpools.to_vec()) .unwrap_or_default(), + datasets: datasets + .get(sled_id.as_untyped_uuid()) + .map(|datasets| datasets.to_vec()) + .unwrap_or_default(), }; Ok((sled_id, sled_agent)) }) diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index 5e3b51f228..e63f476f9d 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -696,6 +696,7 @@ mod test { usable_physical_ram: ByteCount::from(1024 * 1024), disks, zpools: vec![], + datasets: vec![], }, ) .unwrap(); diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 65bdae63ce..0f747c93d9 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -526,6 +526,11 @@ impl CollectionBuilder { .into_iter() .map(|z| Zpool::new(time_collected, z)) .collect(), + datasets: inventory + .datasets + .into_iter() + .map(|d| d.into()) + .collect(), }; if let Some(previous) = self.sleds.get(&sled_id) { diff --git a/nexus/inventory/src/examples.rs b/nexus/inventory/src/examples.rs index c2e283a640..f22b5fd8db 100644 --- a/nexus/inventory/src/examples.rs +++ b/nexus/inventory/src/examples.rs @@ -315,6 +315,7 @@ pub fn representative() -> Representative { }, ]; let zpools = vec![]; + let datasets = vec![]; builder .found_sled_inventory( @@ -329,6 +330,7 @@ pub fn representative() -> Representative { sled_agent_client::types::SledRole::Gimlet, disks, zpools, + datasets, ), ) .unwrap(); @@ -355,6 +357,7 @@ pub fn representative() -> Representative { sled_agent_client::types::SledRole::Scrimlet, vec![], vec![], + vec![], ), ) .unwrap(); @@ -376,6 +379,7 @@ pub fn representative() -> Representative { sled_agent_client::types::SledRole::Gimlet, vec![], vec![], + vec![], ), ) .unwrap(); @@ -395,6 +399,7 @@ pub fn representative() -> Representative { sled_agent_client::types::SledRole::Gimlet, vec![], vec![], + vec![], ), ) .unwrap(); @@ -505,6 +510,7 @@ pub fn sled_agent( sled_role: sled_agent_client::types::SledRole, disks: Vec, zpools: Vec, + datasets: Vec, ) -> sled_agent_client::types::Inventory { sled_agent_client::types::Inventory { baseboard, @@ -516,5 +522,6 @@ pub fn sled_agent( usable_physical_ram: ByteCount::from(1024 * 1024), disks, zpools, + datasets, } } diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index 5f00ea8172..ed961f330a 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -551,9 +551,10 @@ impl Sled { slot: i64::try_from(i).unwrap(), }) .collect(), - // Zpools won't necessarily show up until our first request - // to provision storage, so we omit them. + // Zpools & Datasets won't necessarily show up until our first + // request to provision storage, so we omit them. zpools: vec![], + datasets: vec![], } }; @@ -689,6 +690,7 @@ impl Sled { usable_physical_ram: inv_sled_agent.usable_physical_ram, disks: vec![], zpools: vec![], + datasets: vec![], }; Sled { diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index 661c4c088d..7f197057bb 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -24,6 +24,7 @@ pub use omicron_common::api::internal::shared::NetworkInterfaceKind; pub use omicron_common::api::internal::shared::SourceNatConfig; pub use omicron_common::zpool_name::ZpoolName; use omicron_uuid_kinds::CollectionUuid; +use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; use serde::{Deserialize, Serialize}; @@ -396,6 +397,47 @@ impl Zpool { } } +/// A dataset reported by a sled agent. +#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] +pub struct Dataset { + /// Although datasets mandated by the control plane will have UUIDs, + /// datasets can be created (and have been created) without UUIDs. + pub id: Option, + + /// This name is the full path of the dataset. + pub name: String, + + /// The amount of remaining space usable by the dataset (and children) + /// assuming there is no other activity within the pool. + pub available: ByteCount, + + /// The amount of space consumed by this dataset and descendents. + pub used: ByteCount, + + /// The maximum amount of space usable by a dataset and all descendents. + pub quota: Option, + + /// The minimum amount of space guaranteed to a dataset and descendents. + pub reservation: Option, + + /// The compression algorithm used for this dataset, if any. + pub compression: String, +} + +impl From for Dataset { + fn from(disk: sled_agent_client::types::InventoryDataset) -> Self { + Self { + id: disk.id, + name: disk.name, + available: disk.available, + used: disk.used, + quota: disk.quota, + reservation: disk.reservation, + compression: disk.compression, + } + } +} + /// Inventory reported by sled agent /// /// This is a software notion of a sled, distinct from an underlying baseboard. @@ -415,6 +457,7 @@ pub struct SledAgent { pub reservoir_size: ByteCount, pub disks: Vec, pub zpools: Vec, + pub datasets: Vec, } #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index ac49552c76..97d0a60f9e 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -3577,6 +3577,12 @@ "baseboard": { "$ref": "#/components/schemas/Baseboard" }, + "datasets": { + "type": "array", + "items": { + "$ref": "#/components/schemas/InventoryDataset" + } + }, "disks": { "type": "array", "items": { @@ -3613,6 +3619,7 @@ }, "required": [ "baseboard", + "datasets", "disks", "reservoir_size", "sled_agent_address", @@ -3623,6 +3630,69 @@ "zpools" ] }, + "InventoryDataset": { + "description": "Identifies information about datasets within Oxide-managed zpools", + "type": "object", + "properties": { + "available": { + "description": "The amount of remaining space usable by the dataset (and children) assuming there is no other activity within the pool.", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] + }, + "compression": { + "description": "The compression algorithm used for this dataset, if any.", + "type": "string" + }, + "id": { + "nullable": true, + "description": "Although datasets mandated by the control plane will have UUIDs, datasets can be created (and have been created) without UUIDs.", + "allOf": [ + { + "$ref": "#/components/schemas/TypedUuidForDatasetKind" + } + ] + }, + "name": { + "description": "This name is the full path of the dataset.", + "type": "string" + }, + "quota": { + "nullable": true, + "description": "The maximum amount of space usable by a dataset and all descendents.", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] + }, + "reservation": { + "nullable": true, + "description": "The minimum amount of space guaranteed to a dataset and descendents.", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] + }, + "used": { + "description": "The amount of space consumed by this dataset and descendents.", + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] + } + }, + "required": [ + "available", + "compression", + "name", + "used" + ] + }, "InventoryDisk": { "description": "Identifies information about disks which may be attached to Sleds.", "type": "object", diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 7fc83ad5d0..ecef64d197 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3184,6 +3184,32 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_zpool ( -- Allow looking up the most recent Zpool by ID CREATE INDEX IF NOT EXISTS inv_zpool_by_id_and_time ON omicron.public.inv_zpool (id, time_collected DESC); +CREATE TABLE IF NOT EXISTS omicron.public.inv_dataset ( + -- where this observation came from + -- (foreign key into `inv_collection` table) + inv_collection_id UUID NOT NULL, + sled_id UUID NOT NULL, + + -- The control plane ID of the zpool. + -- This is nullable because datasets have been historically + -- self-managed by the Sled Agent, and some don't have explicit UUIDs. + id UUID, + + name TEXT NOT NULL, + available INT8 NOT NULL, + used INT8 NOT NULL, + quota INT8, + reservation INT8, + compression TEXT NOT NULL, + + -- PK consisting of: + -- - Which collection this was + -- - The sled reporting the disk + -- - The name of this dataset + -- - The slot in which this disk was found + PRIMARY KEY (inv_collection_id, sled_id, name) +); + CREATE TABLE IF NOT EXISTS omicron.public.inv_sled_omicron_zones ( -- where this observation came from -- (foreign key into `inv_collection` table) diff --git a/sled-agent/src/params.rs b/sled-agent/src/params.rs index e0b74e11b0..7de7e996f4 100644 --- a/sled-agent/src/params.rs +++ b/sled-agent/src/params.rs @@ -778,16 +778,16 @@ pub struct InventoryDataset { /// The amount of remaining space usable by the dataset (and children) /// assuming there is no other activity within the pool. - pub available: u64, + pub available: ByteCount, /// The amount of space consumed by this dataset and descendents. - pub used: u64, + pub used: ByteCount, /// The maximum amount of space usable by a dataset and all descendents. - pub quota: Option, + pub quota: Option, /// The minimum amount of space guaranteed to a dataset and descendents. - pub reservation: Option, + pub reservation: Option, /// The compression algorithm used for this dataset, if any. pub compression: String, diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs index c8e56ae9f4..b23e31e1b4 100644 --- a/sled-agent/src/rack_setup/service.rs +++ b/sled-agent/src/rack_setup/service.rs @@ -1591,6 +1591,7 @@ mod test { }) .collect(), zpools: vec![], + datasets: vec![], }, true, ) From 24d93a8b058ff1eec93b6ac1da4e4daf9304907d Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 26 Jul 2024 12:52:59 -0700 Subject: [PATCH 07/36] Fix mismerge --- common/src/api/internal/shared.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 149d6020f5..99a8037393 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -718,7 +718,8 @@ pub struct ResolvedVpcRouteSet { )] #[serde(tag = "type", rename_all = "snake_case")] pub enum DatasetKind { - CockroachDb, + #[serde(rename = "cockroach_db")] + Cockroach, Crucible, Clickhouse, ClickhouseKeeper, @@ -744,7 +745,7 @@ impl fmt::Display for DatasetKind { use DatasetKind::*; let s = match self { Crucible => "crucible", - CockroachDb => "cockroach_db", + Cockroach => "cockroach_db", Clickhouse => "clickhouse", ClickhouseKeeper => "clickhouse_keeper", ExternalDns => "external_dns", @@ -767,7 +768,7 @@ impl FromStr for DatasetKind { use DatasetKind::*; let kind = match s { "crucible" => Crucible, - "cockroach" | "cockroachdb" | "cockroach_db" => CockroachDb, + "cockroach" | "cockroachdb" | "cockroach_db" => Cockroach, "clickhouse" => Clickhouse, "clickhouse_keeper" => ClickhouseKeeper, "external_dns" => ExternalDns, From 178e20e4a5fe4727752e63edb3f026476032826e Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 26 Jul 2024 13:35:36 -0700 Subject: [PATCH 08/36] Clippy and helios --- sled-agent/src/rack_setup/plan/service.rs | 2 +- sled-storage/src/manager.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sled-agent/src/rack_setup/plan/service.rs b/sled-agent/src/rack_setup/plan/service.rs index ec19863bef..6f763dddbf 100644 --- a/sled-agent/src/rack_setup/plan/service.rs +++ b/sled-agent/src/rack_setup/plan/service.rs @@ -925,7 +925,7 @@ impl SledInfo { // enumerates the valid zpool indexes. let allocator = self .u2_zpool_allocators - .entry(kind.clone()) + .entry(kind) .or_insert_with(|| Box::new(0..self.u2_zpools.len())); match allocator.next() { None => Err(PlanError::NotEnoughSleds), diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index fd28b8a67d..59c1dd8bf5 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -1086,7 +1086,6 @@ impl StorageManager { /// systems. #[cfg(all(test, target_os = "illumos"))] mod tests { - use crate::dataset::DatasetType; use crate::disk::RawSyntheticDisk; use crate::manager_test_harness::StorageManagerTestHarness; use crate::resources::DiskManagementError; @@ -1094,6 +1093,7 @@ mod tests { use super::*; use camino_tempfile::tempdir_in; use omicron_common::api::external::Generation; + use omicron_common::disk::DatasetKind; use omicron_common::ledger; use omicron_test_utils::dev::test_setup_log; use sled_hardware::DiskFirmware; @@ -1562,7 +1562,7 @@ mod tests { let dataset_id = Uuid::new_v4(); let zpool_name = ZpoolName::new_external(config.disks[0].pool_id); let dataset_name = - DatasetName::new(zpool_name.clone(), DatasetType::Crucible); + DatasetName::new(zpool_name.clone(), DatasetKind::Crucible); harness .handle() .upsert_filesystem(dataset_id, dataset_name) From cf3f35c95d215bf7396b46f9c5842f64e8f9cb07 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 26 Jul 2024 14:26:03 -0700 Subject: [PATCH 09/36] openapi --- openapi/nexus-internal.json | 93 +++++++++++++++++++++++++++++++++---- openapi/sled-agent.json | 93 +++++++++++++++++++++++++++++++++---- 2 files changed, 170 insertions(+), 16 deletions(-) diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 4e1a11aff1..a50af1990d 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2592,14 +2592,91 @@ }, "DatasetKind": { "description": "Describes the purpose of the dataset.", - "type": "string", - "enum": [ - "crucible", - "cockroach", - "clickhouse", - "clickhouse_keeper", - "external_dns", - "internal_dns" + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "cockroach_db" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "crucible" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse_keeper" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "external_dns" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "internal_dns" + ] + } + }, + "required": [ + "type" + ] + } ] }, "DatasetPutRequest": { diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index d0a1ddd8ad..bba3812b94 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2204,14 +2204,91 @@ }, "DatasetKind": { "description": "Describes the purpose of the dataset.", - "type": "string", - "enum": [ - "crucible", - "cockroach", - "clickhouse", - "clickhouse_keeper", - "external_dns", - "internal_dns" + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "cockroach_db" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "crucible" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "clickhouse_keeper" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "external_dns" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "internal_dns" + ] + } + }, + "required": [ + "type" + ] + } ] }, "DatasetManagementStatus": { From 67bf6faa844ad02c507b8dc4fc0fa0edea527db3 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 26 Jul 2024 15:02:08 -0700 Subject: [PATCH 10/36] schema upgrade --- nexus/db-model/src/schema_versions.rs | 3 ++- schema/crdb/dbinit.sql | 3 +-- schema/crdb/inv-dataset/up01.sql | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 schema/crdb/inv-dataset/up01.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index cc34a3581c..2e7226305a 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(83, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(84, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(84, "inv-dataset"), KnownVersion::new(83, "dataset-address-optional"), KnownVersion::new(82, "region-port"), KnownVersion::new(81, "add-nullable-filesystem-pool"), diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index ecef64d197..957de216f7 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -3206,7 +3206,6 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_dataset ( -- - Which collection this was -- - The sled reporting the disk -- - The name of this dataset - -- - The slot in which this disk was found PRIMARY KEY (inv_collection_id, sled_id, name) ); @@ -4171,7 +4170,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '83.0.0', NULL) + (TRUE, NOW(), NOW(), '84.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/inv-dataset/up01.sql b/schema/crdb/inv-dataset/up01.sql new file mode 100644 index 0000000000..4504768c40 --- /dev/null +++ b/schema/crdb/inv-dataset/up01.sql @@ -0,0 +1,16 @@ +CREATE TABLE IF NOT EXISTS omicron.public.inv_dataset ( + inv_collection_id UUID NOT NULL, + sled_id UUID NOT NULL, + + id UUID, + + name TEXT NOT NULL, + available INT8 NOT NULL, + used INT8 NOT NULL, + quota INT8, + reservation INT8, + compression TEXT NOT NULL, + + PRIMARY KEY (inv_collection_id, sled_id, name) +); + From 11f49fbc49d208e95583b41ab3dfb7e0700400a9 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 29 Jul 2024 18:37:16 -0700 Subject: [PATCH 11/36] More broad support for datasets --- common/src/api/internal/shared.rs | 51 +++++++++++++++++-- nexus/db-model/src/dataset.rs | 3 ++ nexus/db-model/src/dataset_kind.rs | 6 +++ nexus/db-model/src/schema.rs | 1 + nexus/db-queries/src/db/datastore/dataset.rs | 4 ++ nexus/db-queries/src/db/datastore/mod.rs | 4 ++ .../reconfigurator/execution/src/datasets.rs | 3 +- .../execution/src/omicron_physical_disks.rs | 1 + .../tasks/decommissioned_disk_cleaner.rs | 1 + nexus/src/app/rack.rs | 3 +- .../src/app/sagas/region_replacement_start.rs | 4 ++ nexus/src/app/sled.rs | 8 +-- nexus/src/lib.rs | 7 +-- openapi/nexus-internal.json | 46 +++++++++++++++++ openapi/sled-agent.json | 46 +++++++++++++++++ schema/crdb/dbinit.sql | 13 ++++- schema/omicron-datasets.json | 46 +++++++++++++++++ sled-agent/src/rack_setup/plan/service.rs | 2 +- sled-agent/src/rack_setup/service.rs | 2 +- sled-agent/src/sled_agent.rs | 10 ++-- sled-storage/src/manager.rs | 41 ++++++++++----- 21 files changed, 269 insertions(+), 33 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 99a8037393..673378c1d3 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -709,7 +709,6 @@ pub struct ResolvedVpcRouteSet { Deserialize, JsonSchema, Clone, - Copy, PartialEq, Eq, Ord, @@ -718,6 +717,11 @@ pub struct ResolvedVpcRouteSet { )] #[serde(tag = "type", rename_all = "snake_case")] pub enum DatasetKind { + // Durable datasets for zones + + // This renaming exists for backwards compatibility -- this enum variant + // was serialized to "all-zones-request" as "cockroach_db" and should + // stay that way, unless we perform an explicit schema change. #[serde(rename = "cockroach_db")] Cockroach, Crucible, @@ -725,6 +729,15 @@ pub enum DatasetKind { ClickhouseKeeper, ExternalDns, InternalDns, + + // Zone filesystems + ZoneRoot, + Zone { + name: String, + }, + + // Other datasets + Debug, } impl DatasetKind { @@ -738,18 +751,50 @@ impl DatasetKind { _ => true, } } + + /// Returns true if this dataset is delegated to a non-global zone. + pub fn zoned(&self) -> bool { + use DatasetKind::*; + match self { + Cockroach | Crucible | Clickhouse | ClickhouseKeeper + | ExternalDns | InternalDns => true, + ZoneRoot | Zone { .. } | Debug => false, + } + } + + /// Returns the zone name, if this is dataset for a zone filesystem. + /// + /// Otherwise, returns "None". + pub fn zone_name(&self) -> Option { + if let DatasetKind::Zone { name } = self { + Some(name.clone()) + } else { + None + } + } } +// Be cautious updating this implementation: +// +// - It should align with [DatasetKind::FromStr], below +// - The strings here are used here comprise the dataset name, stored durably +// on-disk impl fmt::Display for DatasetKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use DatasetKind::*; let s = match self { Crucible => "crucible", - Cockroach => "cockroach_db", + Cockroach => "cockroachdb", Clickhouse => "clickhouse", ClickhouseKeeper => "clickhouse_keeper", ExternalDns => "external_dns", InternalDns => "internal_dns", + ZoneRoot => "zone", + Zone { name } => { + write!(f, "zone/{}", name)?; + return Ok(()); + } + Debug => "debug", }; write!(f, "{}", s) } @@ -768,7 +813,7 @@ impl FromStr for DatasetKind { use DatasetKind::*; let kind = match s { "crucible" => Crucible, - "cockroach" | "cockroachdb" | "cockroach_db" => Cockroach, + "cockroachdb" | "cockroach_db" => Cockroach, "clickhouse" => Clickhouse, "clickhouse_keeper" => ClickhouseKeeper, "external_dns" => ExternalDns, diff --git a/nexus/db-model/src/dataset.rs b/nexus/db-model/src/dataset.rs index a9dee990b9..d525b80241 100644 --- a/nexus/db-model/src/dataset.rs +++ b/nexus/db-model/src/dataset.rs @@ -41,6 +41,7 @@ pub struct Dataset { pub kind: DatasetKind, pub size_used: Option, + zone_name: Option, } impl Dataset { @@ -49,6 +50,7 @@ impl Dataset { pool_id: Uuid, addr: Option, kind: DatasetKind, + zone_name: Option, ) -> Self { let size_used = match kind { DatasetKind::Crucible => Some(0), @@ -63,6 +65,7 @@ impl Dataset { port: addr.map(|addr| addr.port().into()), kind, size_used, + zone_name, } } diff --git a/nexus/db-model/src/dataset_kind.rs b/nexus/db-model/src/dataset_kind.rs index 395d01353e..2e71b96a41 100644 --- a/nexus/db-model/src/dataset_kind.rs +++ b/nexus/db-model/src/dataset_kind.rs @@ -22,6 +22,9 @@ impl_enum_type!( ClickhouseKeeper => b"clickhouse_keeper" ExternalDns => b"external_dns" InternalDns => b"internal_dns" + ZoneRoot => b"zone_root" + Zone => b"zone" + Debug => b"debug" ); impl From for DatasetKind { @@ -41,6 +44,9 @@ impl From for DatasetKind { internal::shared::DatasetKind::InternalDns => { DatasetKind::InternalDns } + internal::shared::DatasetKind::ZoneRoot => DatasetKind::ZoneRoot, + internal::shared::DatasetKind::Zone { .. } => DatasetKind::Zone, + internal::shared::DatasetKind::Debug => DatasetKind::Debug, } } } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index dc57de9263..6e94f3458a 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1027,6 +1027,7 @@ table! { kind -> crate::DatasetKindEnum, size_used -> Nullable, + zone_name -> Nullable, } } diff --git a/nexus/db-queries/src/db/datastore/dataset.rs b/nexus/db-queries/src/db/datastore/dataset.rs index a08e346fe8..8a814aea80 100644 --- a/nexus/db-queries/src/db/datastore/dataset.rs +++ b/nexus/db-queries/src/db/datastore/dataset.rs @@ -292,6 +292,7 @@ mod test { zpool_id, Some("[::1]:0".parse().unwrap()), DatasetKind::Crucible, + None, )) .await .expect("failed to insert dataset") @@ -325,6 +326,7 @@ mod test { zpool_id, Some("[::1]:12345".parse().unwrap()), DatasetKind::Cockroach, + None, )) .await .expect("failed to do-nothing insert dataset"); @@ -341,6 +343,7 @@ mod test { zpool_id, Some("[::1]:0".parse().unwrap()), DatasetKind::Cockroach, + None, )) .await .expect("failed to upsert dataset"); @@ -373,6 +376,7 @@ mod test { zpool_id, Some("[::1]:12345".parse().unwrap()), DatasetKind::Cockroach, + None, )) .await .expect("failed to do-nothing insert dataset"); diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 2540790477..364ae3e970 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -907,6 +907,7 @@ mod test { zpool.pool_id, bogus_addr, DatasetKind::Crucible, + None, ); let datastore = datastore.clone(); @@ -1279,6 +1280,7 @@ mod test { zpool_id, bogus_addr, DatasetKind::Crucible, + None, ); let datastore = datastore.clone(); async move { @@ -1379,6 +1381,7 @@ mod test { zpool_id, bogus_addr, DatasetKind::Crucible, + None, ); let datastore = datastore.clone(); async move { @@ -1454,6 +1457,7 @@ mod test { zpool_id, bogus_addr, DatasetKind::Crucible, + None, ); datastore.dataset_upsert(dataset).await.unwrap(); physical_disk_ids.push(physical_disk_id); diff --git a/nexus/reconfigurator/execution/src/datasets.rs b/nexus/reconfigurator/execution/src/datasets.rs index 6444934ba6..003861519e 100644 --- a/nexus/reconfigurator/execution/src/datasets.rs +++ b/nexus/reconfigurator/execution/src/datasets.rs @@ -67,7 +67,8 @@ pub(crate) async fn ensure_dataset_records_exist( id.into_untyped_uuid(), pool_id.into_untyped_uuid(), Some(address), - kind.into(), + kind.clone().into(), + kind.zone_name(), ); let maybe_inserted = datastore .dataset_insert_if_not_exists(dataset) diff --git a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs index 9ae72d2049..4d1fe67bc5 100644 --- a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs +++ b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs @@ -435,6 +435,7 @@ mod test { 0, )), DatasetKind::Crucible, + None, )) .await .unwrap(); diff --git a/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs b/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs index cb3ef9a569..982909b644 100644 --- a/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs +++ b/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs @@ -246,6 +246,7 @@ mod tests { 0, )), DatasetKind::Crucible, + None, )) .await .unwrap(); diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 13b30fd47a..f4162f55ab 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -146,7 +146,8 @@ impl super::Nexus { dataset.dataset_id, dataset.zpool_id, Some(dataset.request.address), - dataset.request.kind.into(), + dataset.request.kind.clone().into(), + dataset.request.kind.zone_name(), ) }) .collect(); diff --git a/nexus/src/app/sagas/region_replacement_start.rs b/nexus/src/app/sagas/region_replacement_start.rs index 1297158b24..73842d03f1 100644 --- a/nexus/src/app/sagas/region_replacement_start.rs +++ b/nexus/src/app/sagas/region_replacement_start.rs @@ -896,24 +896,28 @@ pub(crate) mod test { Uuid::new_v4(), Some("[fd00:1122:3344:101::1]:12345".parse().unwrap()), DatasetKind::Crucible, + None, ), Dataset::new( Uuid::new_v4(), Uuid::new_v4(), Some("[fd00:1122:3344:102::1]:12345".parse().unwrap()), DatasetKind::Crucible, + None, ), Dataset::new( Uuid::new_v4(), Uuid::new_v4(), Some("[fd00:1122:3344:103::1]:12345".parse().unwrap()), DatasetKind::Crucible, + None, ), Dataset::new( Uuid::new_v4(), Uuid::new_v4(), Some("[fd00:1122:3344:104::1]:12345".parse().unwrap()), DatasetKind::Crucible, + None, ), ]; diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index 261045670e..a0e1cc3526 100644 --- a/nexus/src/app/sled.rs +++ b/nexus/src/app/sled.rs @@ -292,13 +292,12 @@ impl super::Nexus { // Datasets (contained within zpools) - /// Upserts a dataset into the database, updating it if it already exists. - pub(crate) async fn upsert_dataset( + /// Upserts a crucible dataset into the database, updating it if it already exists. + pub(crate) async fn upsert_crucible_dataset( &self, id: Uuid, zpool_id: Uuid, address: SocketAddrV6, - kind: DatasetKind, ) -> Result<(), Error> { info!( self.log, @@ -307,8 +306,9 @@ impl super::Nexus { "dataset_id" => id.to_string(), "address" => address.to_string() ); + let kind = DatasetKind::Crucible; let dataset = - db::model::Dataset::new(id, zpool_id, Some(address), kind); + db::model::Dataset::new(id, zpool_id, Some(address), kind, None); self.db_datastore.dataset_upsert(dataset).await?; Ok(()) } diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index d5c853b15b..284e8de2ea 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -384,12 +384,7 @@ impl nexus_test_interface::NexusServer for Server { self.apictx .context .nexus - .upsert_dataset( - dataset_id, - zpool_id, - address, - nexus_db_queries::db::model::DatasetKind::Crucible, - ) + .upsert_crucible_dataset(dataset_id, zpool_id, address) .await .unwrap(); } diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index a50af1990d..49a96934e4 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2676,6 +2676,52 @@ "required": [ "type" ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "zone_root" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "type": { + "type": "string", + "enum": [ + "zone" + ] + } + }, + "required": [ + "name", + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "debug" + ] + } + }, + "required": [ + "type" + ] } ] }, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index bba3812b94..d42ffd8fae 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2288,6 +2288,52 @@ "required": [ "type" ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "zone_root" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "name": { + "type": "string" + }, + "type": { + "type": "string", + "enum": [ + "zone" + ] + } + }, + "required": [ + "name", + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "debug" + ] + } + }, + "required": [ + "type" + ] } ] }, diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 7fc83ad5d0..e6eec9b7b6 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -507,7 +507,10 @@ CREATE TYPE IF NOT EXISTS omicron.public.dataset_kind AS ENUM ( 'clickhouse', 'clickhouse_keeper', 'external_dns', - 'internal_dns' + 'internal_dns', + 'zone_root', + 'zone', + 'debug' ); /* @@ -533,6 +536,9 @@ CREATE TABLE IF NOT EXISTS omicron.public.dataset ( /* An upper bound on the amount of space that might be in-use */ size_used INT, + /* Only valid if kind = zone -- the name of this zone */ + zone_name TEXT, + /* Crucible must make use of 'size_used'; other datasets manage their own storage */ CONSTRAINT size_used_column_set_for_crucible CHECK ( (kind != 'crucible') OR @@ -542,6 +548,11 @@ CREATE TABLE IF NOT EXISTS omicron.public.dataset ( CONSTRAINT ip_and_port_set_for_crucible CHECK ( (kind != 'crucible') OR (kind = 'crucible' AND ip IS NOT NULL and port IS NOT NULL) + ), + + CONSTRAINT zone_name_for_zone_kind CHECK ( + (kind != 'zone') OR + (kind = 'zone' AND zone_name IS NOT NULL) ) ); diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index 5a5cc2ea18..1821eddac7 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -160,6 +160,52 @@ ] } } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "zone_root" + ] + } + } + }, + { + "type": "object", + "required": [ + "name", + "type" + ], + "properties": { + "name": { + "type": "string" + }, + "type": { + "type": "string", + "enum": [ + "zone" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "debug" + ] + } + } } ] }, diff --git a/sled-agent/src/rack_setup/plan/service.rs b/sled-agent/src/rack_setup/plan/service.rs index 6f763dddbf..ec19863bef 100644 --- a/sled-agent/src/rack_setup/plan/service.rs +++ b/sled-agent/src/rack_setup/plan/service.rs @@ -925,7 +925,7 @@ impl SledInfo { // enumerates the valid zpool indexes. let allocator = self .u2_zpool_allocators - .entry(kind) + .entry(kind.clone()) .or_insert_with(|| Box::new(0..self.u2_zpools.len())); match allocator.next() { None => Err(PlanError::NotEnoughSleds), diff --git a/sled-agent/src/rack_setup/service.rs b/sled-agent/src/rack_setup/service.rs index 0f6a77fd80..07c8794a73 100644 --- a/sled-agent/src/rack_setup/service.rs +++ b/sled-agent/src/rack_setup/service.rs @@ -725,7 +725,7 @@ impl ServiceInner { dataset_id: zone.id, request: NexusTypes::DatasetPutRequest { address: dataset_address.to_string(), - kind: *dataset_name.dataset(), + kind: dataset_name.dataset().clone(), }, }) } diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 9ea3a0fb3c..44fc46987b 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -818,9 +818,13 @@ impl SledAgent { let datasets_result = self.storage().datasets_ensure(config).await?; info!(self.log, "datasets ensure: Updated storage"); - // TODO: See omicron_physical_disks_ensure, below - do we similarly - // need to ensure that old datasets are no longer in-use before we - // return here? + // TODO(https://github.com/oxidecomputer/omicron/issues/6177): + // At the moment, we don't actually remove any datasets -- this function + // just adds new datasets. + // + // Once we start removing old datasets, we should probably ensure that + // they are not longer in-use before returning (similar to + // omicron_physical_disks_ensure). Ok(datasets_result) } diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 59c1dd8bf5..7f61097b6d 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -706,21 +706,22 @@ impl StorageManager { } else if config.generation == ledger_data.generation { info!( log, - "Requested geenration number matches prior request", + "Requested generation number matches prior request", ); if ledger_data != &config { - error!(log, "Requested configuration changed (with the same generation)"); + error!( + log, + "Requested configuration changed (with the same generation)"; + "generation" => ?config.generation + ); return Err(Error::DatasetConfigurationChanged { generation: config.generation, }); } + } else { + info!(log, "Request looks newer than prior requests"); } - - info!( - log, - "Request looks newer than (or identical to) prior requests" - ); ledger } None => { @@ -745,6 +746,11 @@ impl StorageManager { Ok(result) } + // Attempts to ensure that each dataset exist. + // + // Does not return an error, because the [DatasetsManagementResult] type + // includes details about all possible errors that may occur on + // a per-dataset granularity. async fn datasets_ensure_internal( &mut self, log: &Logger, @@ -770,7 +776,7 @@ impl StorageManager { }; if let Err(err) = self.ensure_dataset(config).await { - warn!(log, "Failed to ensure dataset"; "err" => ?err); + warn!(log, "Failed to ensure dataset"; "dataset" => ?status.dataset_name, "err" => ?err); status.err = Some(err.to_string()); }; @@ -989,11 +995,22 @@ impl StorageManager { ))); } - // TODO: Revisit these args, they might need more configuration - // tweaking. - let zoned = true; + let zoned = config.name.dataset().zoned(); + let mountpoint_path = if zoned { + Utf8PathBuf::from("/data") + } else { + config.name.pool().dataset_mountpoint( + &Utf8PathBuf::from("/"), + &config.name.dataset().to_string(), + ) + }; + let mountpoint = Mountpoint::Path(mountpoint_path); + let fs_name = &config.name.full_name(); 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; let size_details = Some(illumos_utils::zfs::SizeDetails { quota: config.quota, @@ -1002,7 +1019,7 @@ impl StorageManager { }); Zfs::ensure_filesystem( fs_name, - Mountpoint::Path(Utf8PathBuf::from("/data")), + mountpoint, zoned, do_format, encryption_details, From b999bf709050e76c3983ddbff33075a02d9c5689 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 29 Jul 2024 18:43:52 -0700 Subject: [PATCH 12/36] make it a schema change --- nexus/db-model/src/schema_versions.rs | 3 ++- schema/crdb/dataset-kinds-zone-and-debug/up01.sql | 1 + schema/crdb/dataset-kinds-zone-and-debug/up02.sql | 1 + schema/crdb/dataset-kinds-zone-and-debug/up03.sql | 1 + schema/crdb/dataset-kinds-zone-and-debug/up04.sql | 1 + schema/crdb/dataset-kinds-zone-and-debug/up05.sql | 4 ++++ schema/crdb/dbinit.sql | 2 +- 7 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 schema/crdb/dataset-kinds-zone-and-debug/up01.sql create mode 100644 schema/crdb/dataset-kinds-zone-and-debug/up02.sql create mode 100644 schema/crdb/dataset-kinds-zone-and-debug/up03.sql create mode 100644 schema/crdb/dataset-kinds-zone-and-debug/up04.sql create mode 100644 schema/crdb/dataset-kinds-zone-and-debug/up05.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index cc34a3581c..8b4ddce45a 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(83, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(84, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(84, "dataset-kinds-zone-and-debug"), KnownVersion::new(83, "dataset-address-optional"), KnownVersion::new(82, "region-port"), KnownVersion::new(81, "add-nullable-filesystem-pool"), diff --git a/schema/crdb/dataset-kinds-zone-and-debug/up01.sql b/schema/crdb/dataset-kinds-zone-and-debug/up01.sql new file mode 100644 index 0000000000..1cfe718d00 --- /dev/null +++ b/schema/crdb/dataset-kinds-zone-and-debug/up01.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.dataset_kind ADD VALUE IF NOT EXISTS 'zone_root' AFTER 'internal_dns'; diff --git a/schema/crdb/dataset-kinds-zone-and-debug/up02.sql b/schema/crdb/dataset-kinds-zone-and-debug/up02.sql new file mode 100644 index 0000000000..93178e3685 --- /dev/null +++ b/schema/crdb/dataset-kinds-zone-and-debug/up02.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.dataset_kind ADD VALUE IF NOT EXISTS 'zone' AFTER 'zone_root'; diff --git a/schema/crdb/dataset-kinds-zone-and-debug/up03.sql b/schema/crdb/dataset-kinds-zone-and-debug/up03.sql new file mode 100644 index 0000000000..58d215d177 --- /dev/null +++ b/schema/crdb/dataset-kinds-zone-and-debug/up03.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.dataset_kind ADD VALUE IF NOT EXISTS 'debug' AFTER 'zone'; diff --git a/schema/crdb/dataset-kinds-zone-and-debug/up04.sql b/schema/crdb/dataset-kinds-zone-and-debug/up04.sql new file mode 100644 index 0000000000..b92bce1b6c --- /dev/null +++ b/schema/crdb/dataset-kinds-zone-and-debug/up04.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.dataset ADD COLUMN IF NOT EXISTS zone_name TEXT; diff --git a/schema/crdb/dataset-kinds-zone-and-debug/up05.sql b/schema/crdb/dataset-kinds-zone-and-debug/up05.sql new file mode 100644 index 0000000000..3f33b79c72 --- /dev/null +++ b/schema/crdb/dataset-kinds-zone-and-debug/up05.sql @@ -0,0 +1,4 @@ +ALTER TABLE omicron.public.dataset ADD CONSTRAINT IF NOT EXISTS zone_name_for_zone_kind CHECK ( + (kind != 'zone') OR + (kind = 'zone' AND zone_name IS NOT NULL) +) diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index e6eec9b7b6..49d338851a 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4156,7 +4156,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '83.0.0', NULL) + (TRUE, NOW(), NOW(), '84.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 1baf7dd6262ce5766e141aee12647bd52efe27fe Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 29 Jul 2024 22:18:33 -0700 Subject: [PATCH 13/36] welp I guess I changed some db queries --- .../tests/output/region_allocate_distinct_sleds.sql | 5 ++++- .../db-queries/tests/output/region_allocate_random_sleds.sql | 5 ++++- .../output/region_allocate_with_snapshot_distinct_sleds.sql | 5 ++++- .../output/region_allocate_with_snapshot_random_sleds.sql | 5 ++++- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql b/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql index 6331770ef5..4e7dde244b 100644 --- a/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql @@ -270,7 +270,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -284,6 +285,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -310,6 +312,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, diff --git a/nexus/db-queries/tests/output/region_allocate_random_sleds.sql b/nexus/db-queries/tests/output/region_allocate_random_sleds.sql index e713121d34..b2c164a6d9 100644 --- a/nexus/db-queries/tests/output/region_allocate_random_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_random_sleds.sql @@ -268,7 +268,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -282,6 +283,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -308,6 +310,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, diff --git a/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql b/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql index 0b8dc4fca6..97ee23f82e 100644 --- a/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql @@ -281,7 +281,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -295,6 +296,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -321,6 +323,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, diff --git a/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql b/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql index 9ac945f71d..a1cc103594 100644 --- a/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql @@ -279,7 +279,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -293,6 +294,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -319,6 +321,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, From fdf0644aa0f825d0fb9fb88799087365bbf24b43 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 29 Jul 2024 22:23:04 -0700 Subject: [PATCH 14/36] queries --- .../tests/output/region_allocate_distinct_sleds.sql | 5 ++++- .../db-queries/tests/output/region_allocate_random_sleds.sql | 5 ++++- .../output/region_allocate_with_snapshot_distinct_sleds.sql | 5 ++++- .../output/region_allocate_with_snapshot_random_sleds.sql | 5 ++++- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql b/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql index 6331770ef5..4e7dde244b 100644 --- a/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_distinct_sleds.sql @@ -270,7 +270,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -284,6 +285,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -310,6 +312,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, diff --git a/nexus/db-queries/tests/output/region_allocate_random_sleds.sql b/nexus/db-queries/tests/output/region_allocate_random_sleds.sql index e713121d34..b2c164a6d9 100644 --- a/nexus/db-queries/tests/output/region_allocate_random_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_random_sleds.sql @@ -268,7 +268,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -282,6 +283,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -308,6 +310,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, diff --git a/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql b/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql index 0b8dc4fca6..97ee23f82e 100644 --- a/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_with_snapshot_distinct_sleds.sql @@ -281,7 +281,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -295,6 +296,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -321,6 +323,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, diff --git a/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql b/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql index 9ac945f71d..a1cc103594 100644 --- a/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql +++ b/nexus/db-queries/tests/output/region_allocate_with_snapshot_random_sleds.sql @@ -279,7 +279,8 @@ WITH dataset.ip, dataset.port, dataset.kind, - dataset.size_used + dataset.size_used, + dataset.zone_name ) ( SELECT @@ -293,6 +294,7 @@ WITH dataset.port, dataset.kind, dataset.size_used, + dataset.zone_name, old_regions.id, old_regions.time_created, old_regions.time_modified, @@ -319,6 +321,7 @@ UNION updated_datasets.port, updated_datasets.kind, updated_datasets.size_used, + updated_datasets.zone_name, inserted_regions.id, inserted_regions.time_created, inserted_regions.time_modified, From c5621256ccc7a8995842d057e608c8b3b66effe5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 13:06:11 -0700 Subject: [PATCH 15/36] Optional properties still get set --- illumos-utils/src/zfs.rs | 67 +++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 21de2a50da..0178a43e84 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -376,6 +376,10 @@ impl Zfs { 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, @@ -383,40 +387,39 @@ impl Zfs { reservation: Option, compression: Option, ) -> Result<(), EnsureFilesystemError> { - if let Some(quota) = quota { - if let Err(err) = - Self::set_value(name, "quota", &format!("{quota}")) - { - return Err(EnsureFilesystemError { - name: name.to_string(), - mountpoint: mountpoint.clone(), - // Take the execution error from the SetValueError - err: err.err.into(), - }); - } + let quota = quota + .map(|q| q.to_string()) + .unwrap_or_else(|| String::from("none")); + let reservation = reservation + .map(|r| r.to_string()) + .unwrap_or_else(|| String::from("none")); + let compression = compression.unwrap_or_else(|| String::from("off")); + + if let Err(err) = Self::set_value(name, "quota", &format!("{quota}")) { + return Err(EnsureFilesystemError { + name: name.to_string(), + mountpoint: mountpoint.clone(), + // Take the execution error from the SetValueError + err: err.err.into(), + }); } - if let Some(reservation) = reservation { - if let Err(err) = - Self::set_value(name, "reservation", &format!("{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, "reservation", &format!("{reservation}")) + { + return Err(EnsureFilesystemError { + name: name.to_string(), + mountpoint: mountpoint.clone(), + // Take the execution error from the SetValueError + err: err.err.into(), + }); } - if let Some(compression) = compression { - 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(), - }); - } + 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(()) } From c7a5adfd7484b4578c8695fbc58d20b0d0dba4f1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 13:09:45 -0700 Subject: [PATCH 16/36] clippy --- illumos-utils/src/zfs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 0178a43e84..9b3c288d66 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -395,7 +395,7 @@ impl Zfs { .unwrap_or_else(|| String::from("none")); let compression = compression.unwrap_or_else(|| String::from("off")); - if let Err(err) = Self::set_value(name, "quota", &format!("{quota}")) { + if let Err(err) = Self::set_value(name, "quota", "a) { return Err(EnsureFilesystemError { name: name.to_string(), mountpoint: mountpoint.clone(), @@ -404,7 +404,7 @@ impl Zfs { }); } if let Err(err) = - Self::set_value(name, "reservation", &format!("{reservation}")) + Self::set_value(name, "reservation", &reservation) { return Err(EnsureFilesystemError { name: name.to_string(), From 181507cdb8fd6ad4dc70bef26283071622dda9a4 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 13:11:16 -0700 Subject: [PATCH 17/36] fmt --- illumos-utils/src/zfs.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 9b3c288d66..c9fd7fa315 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -403,9 +403,7 @@ impl Zfs { err: err.err.into(), }); } - if let Err(err) = - Self::set_value(name, "reservation", &reservation) - { + if let Err(err) = Self::set_value(name, "reservation", &reservation) { return Err(EnsureFilesystemError { name: name.to_string(), mountpoint: mountpoint.clone(), From 8e1df07079619c10445395582a3e7946698c7a44 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 13:35:52 -0700 Subject: [PATCH 18/36] str not string --- common/src/api/internal/shared.rs | 4 ++-- nexus/reconfigurator/execution/src/datasets.rs | 2 +- nexus/src/app/rack.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 673378c1d3..d30789ddf9 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -765,9 +765,9 @@ impl DatasetKind { /// Returns the zone name, if this is dataset for a zone filesystem. /// /// Otherwise, returns "None". - pub fn zone_name(&self) -> Option { + pub fn zone_name(&self) -> Option<&str> { if let DatasetKind::Zone { name } = self { - Some(name.clone()) + Some(name) } else { None } diff --git a/nexus/reconfigurator/execution/src/datasets.rs b/nexus/reconfigurator/execution/src/datasets.rs index 003861519e..d20de82cef 100644 --- a/nexus/reconfigurator/execution/src/datasets.rs +++ b/nexus/reconfigurator/execution/src/datasets.rs @@ -68,7 +68,7 @@ pub(crate) async fn ensure_dataset_records_exist( pool_id.into_untyped_uuid(), Some(address), kind.clone().into(), - kind.zone_name(), + kind.zone_name().map(String::from), ); let maybe_inserted = datastore .dataset_insert_if_not_exists(dataset) diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index f4162f55ab..432c44bea4 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -147,7 +147,7 @@ impl super::Nexus { dataset.zpool_id, Some(dataset.request.address), dataset.request.kind.clone().into(), - dataset.request.kind.zone_name(), + dataset.request.kind.zone_name().map(String::from), ) }) .collect(); From 93134c25232d244cd3c54dcbd6d847640f9525ab Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 14:05:56 -0700 Subject: [PATCH 19/36] cockroachdb, not cockroach_db --- common/src/api/internal/shared.rs | 5 +---- openapi/sled-agent.json | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index d30789ddf9..79f5eb8c3b 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -719,10 +719,7 @@ pub struct ResolvedVpcRouteSet { pub enum DatasetKind { // Durable datasets for zones - // This renaming exists for backwards compatibility -- this enum variant - // was serialized to "all-zones-request" as "cockroach_db" and should - // stay that way, unless we perform an explicit schema change. - #[serde(rename = "cockroach_db")] + #[serde(rename = "cockroachdb")] Cockroach, Crucible, Clickhouse, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index d42ffd8fae..173a1c8058 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2211,7 +2211,7 @@ "type": { "type": "string", "enum": [ - "cockroach_db" + "cockroachdb" ] } }, From d88d5413c74b1e5b7c8199cbb907e29df4f30d49 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 14:34:17 -0700 Subject: [PATCH 20/36] generation numbers --- common/src/api/internal/shared.rs | 1 - sled-storage/src/manager.rs | 11 +++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 79f5eb8c3b..874d1a5866 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -718,7 +718,6 @@ pub struct ResolvedVpcRouteSet { #[serde(tag = "type", rename_all = "snake_case")] pub enum DatasetKind { // Durable datasets for zones - #[serde(rename = "cockroachdb")] Cockroach, Crucible, diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 7f61097b6d..3b32b36c09 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -697,7 +697,9 @@ impl StorageManager { if config.generation < ledger_data.generation { warn!( log, - "Request looks out-of-date compared to prior request" + "Request looks out-of-date compared to prior request"; + "requested_generation" => ?config.generation, + "ledger_generation" => ?ledger_data.generation, ); return Err(Error::DatasetConfigurationOutdated { requested: config.generation, @@ -720,7 +722,12 @@ impl StorageManager { }); } } else { - info!(log, "Request looks newer than prior requests"); + info!( + log, + "Request looks newer than prior requests"; + "requested_generation" => ?config.generation, + "ledger_generation" => ?ledger_data.generation, + ); } ledger } From 95ebbb87174c84c57eb38bdd8bfc698a7c23475b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 15:35:52 -0700 Subject: [PATCH 21/36] review feedback --- illumos-utils/src/zfs.rs | 22 ++++++++++++++++++++-- openapi/sled-agent.json | 2 ++ sled-agent/src/http_entrypoints.rs | 4 +++- sled-agent/src/sled_agent.rs | 4 ++-- sled-storage/src/manager.rs | 12 +++++++----- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index c9fd7fa315..5df1b73c07 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -260,9 +260,27 @@ impl Zfs { Ok(()) } - /// Creates a new ZFS filesystem named `name`, unless one already exists. + /// Creates a new ZFS filesystem unless one already exists. /// - /// Applies an optional quota, provided _in bytes_. + /// - `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, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index 173a1c8058..c9e5c709e3 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -179,6 +179,7 @@ }, "/datasets": { "get": { + "summary": "Lists the datasets that this sled is configured to use", "operationId": "datasets_get", "responses": { "200": { @@ -200,6 +201,7 @@ } }, "put": { + "summary": "Configures datasets to be used on this sled", "operationId": "datasets_put", "requestBody": { "content": { diff --git a/sled-agent/src/http_entrypoints.rs b/sled-agent/src/http_entrypoints.rs index c36de9a787..97671b42e6 100644 --- a/sled-agent/src/http_entrypoints.rs +++ b/sled-agent/src/http_entrypoints.rs @@ -352,6 +352,7 @@ async fn omicron_zones_get( Ok(HttpResponseOk(sa.omicron_zones_list().await?)) } +/// Configures datasets to be used on this sled #[endpoint { method = PUT, path = "/datasets", @@ -366,6 +367,7 @@ async fn datasets_put( Ok(HttpResponseOk(result)) } +/// Lists the datasets that this sled is configured to use #[endpoint { method = GET, path = "/datasets", @@ -374,7 +376,7 @@ async fn datasets_get( rqctx: RequestContext, ) -> Result, HttpError> { let sa = rqctx.context(); - Ok(HttpResponseOk(sa.datasets_list().await?)) + Ok(HttpResponseOk(sa.datasets_config_list().await?)) } #[endpoint { diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 5368539445..29cf4c6de2 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -810,8 +810,8 @@ impl SledAgent { self.inner.zone_bundler.cleanup().await.map_err(Error::from) } - pub async fn datasets_list(&self) -> Result { - Ok(self.storage().datasets_list().await?) + pub async fn datasets_config_list(&self) -> Result { + Ok(self.storage().datasets_config_list().await?) } pub async fn datasets_ensure( diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 3b32b36c09..c086d656d1 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -273,7 +273,7 @@ impl StorageHandle { /// Reads the last value written to storage by /// [Self::datasets_ensure]. - pub async fn datasets_list(&self) -> Result { + pub async fn datasets_config_list(&self) -> Result { let (tx, rx) = oneshot::channel(); self.tx .send(StorageRequest::DatasetsList { tx: tx.into() }) @@ -479,7 +479,7 @@ impl StorageManager { let _ = tx.0.send(self.datasets_ensure(config).await); } StorageRequest::DatasetsList { tx } => { - let _ = tx.0.send(self.datasets_list().await); + let _ = tx.0.send(self.datasets_config_list().await); } StorageRequest::OmicronPhysicalDisksEnsure { config, tx } => { let _ = @@ -790,8 +790,9 @@ impl StorageManager { status } - async fn datasets_list(&mut self) -> Result { - let log = self.log.new(o!("request" => "datasets_list")); + // Lists datasets that this sled is configured to use. + async fn datasets_config_list(&mut self) -> Result { + let log = self.log.new(o!("request" => "datasets_config_list")); let ledger_paths = self.all_omicron_dataset_ledgers().await; let maybe_ledger = @@ -1637,7 +1638,8 @@ mod tests { assert!(!status.has_error()); // List datasets, expect to see what we just created - let observed_config = harness.handle().datasets_list().await.unwrap(); + let observed_config = + harness.handle().datasets_config_list().await.unwrap(); assert_eq!(config, observed_config); // Calling "datasets_ensure" with the same input should succeed. From 994757e017e145f483da68f2702d2e4b4359301f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 16:50:34 -0700 Subject: [PATCH 22/36] Serialize via string, not tag --- common/src/api/internal/shared.rs | 96 +++++++++++++++++++++++++------ common/src/disk.rs | 5 +- openapi/nexus-internal.json | 2 +- openapi/sled-agent.json | 4 +- schema/omicron-datasets.json | 6 +- sled-storage/src/error.rs | 3 + sled-storage/src/manager.rs | 33 +++++++---- 7 files changed, 113 insertions(+), 36 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 874d1a5866..403e0855a3 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -10,13 +10,14 @@ use crate::{ }; use oxnet::{IpNet, Ipv4Net, Ipv6Net}; use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use std::{ collections::{HashMap, HashSet}, fmt, net::{IpAddr, Ipv4Addr, Ipv6Addr}, str::FromStr, }; +use strum::EnumCount; use uuid::Uuid; /// The type of network interface @@ -704,16 +705,7 @@ pub struct ResolvedVpcRouteSet { /// Describes the purpose of the dataset. #[derive( - Debug, - Serialize, - Deserialize, - JsonSchema, - Clone, - PartialEq, - Eq, - Ord, - PartialOrd, - Hash, + Debug, JsonSchema, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, EnumCount, )] #[serde(tag = "type", rename_all = "snake_case")] pub enum DatasetKind { @@ -736,6 +728,25 @@ pub enum DatasetKind { Debug, } +impl Serialize for DatasetKind { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.to_string()) + } +} + +impl<'de> Deserialize<'de> for DatasetKind { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + s.parse().map_err(de::Error::custom) + } +} + impl DatasetKind { pub fn dataset_should_be_encrypted(&self) -> bool { match self { @@ -758,7 +769,7 @@ impl DatasetKind { } } - /// Returns the zone name, if this is dataset for a zone filesystem. + /// Returns the zone name, if this is a dataset for a zone filesystem. /// /// Otherwise, returns "None". pub fn zone_name(&self) -> Option<&str> { @@ -808,16 +819,22 @@ impl FromStr for DatasetKind { fn from_str(s: &str) -> Result { use DatasetKind::*; let kind = match s { + "cockroachdb" => Cockroach, "crucible" => Crucible, - "cockroachdb" | "cockroach_db" => Cockroach, "clickhouse" => Clickhouse, "clickhouse_keeper" => ClickhouseKeeper, "external_dns" => ExternalDns, "internal_dns" => InternalDns, - _ => { - return Err(DatasetKindParseError::UnknownDataset( - s.to_string(), - )) + "zone" => ZoneRoot, + "debug" => Debug, + other => { + if let Some(name) = other.strip_prefix("zone/") { + Zone { name: name.to_string() } + } else { + return Err(DatasetKindParseError::UnknownDataset( + s.to_string(), + )); + } } }; Ok(kind) @@ -846,6 +863,7 @@ pub struct SledIdentifiers { #[cfg(test)] mod tests { + use super::*; use crate::api::internal::shared::AllowedSourceIps; use oxnet::{IpNet, Ipv4Net, Ipv6Net}; use std::net::{Ipv4Addr, Ipv6Addr}; @@ -890,4 +908,48 @@ mod tests { serde_json::from_str(r#"{"allow":"any"}"#).unwrap(), ); } + + #[test] + fn test_dataset_kind_serialization() { + let kinds = [ + DatasetKind::Crucible, + DatasetKind::Cockroach, + DatasetKind::Clickhouse, + DatasetKind::ClickhouseKeeper, + DatasetKind::ExternalDns, + DatasetKind::InternalDns, + DatasetKind::ZoneRoot, + DatasetKind::Zone { name: String::from("myzone") }, + DatasetKind::Debug, + ]; + + assert_eq!(kinds.len(), DatasetKind::COUNT); + + for kind in &kinds { + // To string, from string + let as_str = kind.to_string(); + let from_str = + DatasetKind::from_str(&as_str).unwrap_or_else(|_| { + panic!("Failed to convert {kind} to and from string") + }); + assert_eq!( + *kind, from_str, + "{kind} failed to convert to/from a string" + ); + + // Serialize, deserialize + let ser = serde_json::to_string(&kind) + .unwrap_or_else(|_| panic!("Failed to serialize {kind}")); + let de: DatasetKind = serde_json::from_str(&ser) + .unwrap_or_else(|_| panic!("Failed to deserialize {kind}")); + assert_eq!(*kind, de, "{kind} failed serialization"); + + // Test that serialization is equivalent to stringifying. + assert_eq!( + format!("\"{as_str}\""), + ser, + "{kind} does not match stringification/serialization" + ); + } + } } diff --git a/common/src/disk.rs b/common/src/disk.rs index b9a259574e..86ee48c0f6 100644 --- a/common/src/disk.rs +++ b/common/src/disk.rs @@ -8,6 +8,7 @@ use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::ZpoolUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; use uuid::Uuid; use crate::{ @@ -180,12 +181,12 @@ pub struct DatasetsConfig { /// for a sled before any requests have been made. pub generation: Generation, - pub datasets: Vec, + pub datasets: BTreeMap, } impl Default for DatasetsConfig { fn default() -> Self { - Self { generation: Generation::new(), datasets: vec![] } + Self { generation: Generation::new(), datasets: BTreeMap::new() } } } diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 2bd7172bf3..cfbe028f9c 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2599,7 +2599,7 @@ "type": { "type": "string", "enum": [ - "cockroach_db" + "cockroachdb" ] } }, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index c9e5c709e3..82f3c6c3ae 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2374,8 +2374,8 @@ "type": "object", "properties": { "datasets": { - "type": "array", - "items": { + "type": "object", + "additionalProperties": { "$ref": "#/components/schemas/DatasetConfig" } }, diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index 1821eddac7..b675432172 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -8,8 +8,8 @@ ], "properties": { "datasets": { - "type": "array", - "items": { + "type": "object", + "additionalProperties": { "$ref": "#/definitions/DatasetConfig" } }, @@ -86,7 +86,7 @@ "type": { "type": "string", "enum": [ - "cockroach_db" + "cockroachdb" ] } } diff --git a/sled-storage/src/error.rs b/sled-storage/src/error.rs index 96fca65f57..988f7f363a 100644 --- a/sled-storage/src/error.rs +++ b/sled-storage/src/error.rs @@ -84,6 +84,9 @@ pub enum Error { current: Generation, }, + #[error("Invalid configuration (UUID mismatch in arguments)")] + ConfigUuidMismatch, + #[error("Dataset configuration out-of-date (asked for {requested}, but latest is {current})")] DatasetConfigurationOutdated { requested: Generation, current: Generation }, diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index c086d656d1..bc0cac2014 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -674,12 +674,19 @@ impl StorageManager { async fn datasets_ensure( &mut self, - mut config: DatasetsConfig, + config: DatasetsConfig, ) -> Result { let log = self.log.new(o!("request" => "datasets_ensure")); - // Ensure that the datasets arrive in a consistent order - config.datasets.sort_by(|a, b| a.id.partial_cmp(&b.id).unwrap()); + // As a small input-check, confirm that the UUID of the map of inputs + // matches the DatasetConfig. + // + // The dataset configs are sorted by UUID so they always appear in the + // same order, but this check prevents adding an entry of: + // - (UUID: X, Config(UUID: Y)), for X != Y + if !config.datasets.iter().all(|(id, config)| *id == config.id) { + return Err(Error::ConfigUuidMismatch); + } // We rely on the schema being stable across reboots -- observe // "test_datasets_schema" below for that property guarantee. @@ -764,7 +771,7 @@ impl StorageManager { config: &DatasetsConfig, ) -> DatasetsManagementResult { let mut status = vec![]; - for dataset in &config.datasets { + for dataset in config.datasets.values() { status.push(self.dataset_ensure_internal(log, dataset).await); } DatasetsManagementResult { status } @@ -1122,6 +1129,7 @@ mod tests { use omicron_common::ledger; use omicron_test_utils::dev::test_setup_log; use sled_hardware::DiskFirmware; + use std::collections::BTreeMap; use std::sync::atomic::Ordering; use uuid::Uuid; @@ -1621,13 +1629,16 @@ mod tests { let id = DatasetUuid::new_v4(); let zpool_name = ZpoolName::new_external(config.disks[0].pool_id); let name = DatasetName::new(zpool_name.clone(), DatasetKind::Crucible); - let datasets = vec![DatasetConfig { + let datasets = BTreeMap::from([( id, - name, - compression: None, - quota: None, - reservation: None, - }]; + DatasetConfig { + id, + name, + compression: None, + quota: None, + reservation: None, + }, + )]); // "Generation = 1" is reserved as "no requests seen yet", so we jump // past it. let generation = Generation::new().next(); @@ -1659,7 +1670,7 @@ mod tests { // However, calling it with a different input and the same generation // number should fail. config.generation = current_config_generation; - config.datasets[0].reservation = Some(1024); + config.datasets.values_mut().next().unwrap().reservation = Some(1024); let err = harness.handle().datasets_ensure(config.clone()).await.unwrap_err(); assert!(matches!(err, Error::DatasetConfigurationChanged { .. })); From 7156ba06870c10ea01df546f10d9f7b40f07a5a1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 17:18:40 -0700 Subject: [PATCH 23/36] safer API for Dataset::new --- nexus/db-model/src/dataset.rs | 13 ++++++++----- nexus/db-model/src/dataset_kind.rs | 8 ++++++-- nexus/db-queries/src/db/datastore/dataset.rs | 13 +++++-------- nexus/db-queries/src/db/datastore/mod.rs | 13 +++++-------- nexus/reconfigurator/execution/src/datasets.rs | 3 +-- .../execution/src/omicron_physical_disks.rs | 3 +-- .../background/tasks/decommissioned_disk_cleaner.rs | 3 +-- nexus/src/app/rack.rs | 3 +-- nexus/src/app/sagas/region_replacement_start.rs | 6 +----- nexus/src/app/sled.rs | 4 ++-- 10 files changed, 31 insertions(+), 38 deletions(-) diff --git a/nexus/db-model/src/dataset.rs b/nexus/db-model/src/dataset.rs index d525b80241..f896f11c5b 100644 --- a/nexus/db-model/src/dataset.rs +++ b/nexus/db-model/src/dataset.rs @@ -8,6 +8,7 @@ use crate::ipv6; use crate::schema::{dataset, region}; use chrono::{DateTime, Utc}; use db_macros::Asset; +use omicron_common::api::internal::shared::DatasetKind as ApiDatasetKind; use serde::{Deserialize, Serialize}; use std::net::{Ipv6Addr, SocketAddrV6}; use uuid::Uuid; @@ -49,13 +50,15 @@ impl Dataset { id: Uuid, pool_id: Uuid, addr: Option, - kind: DatasetKind, - zone_name: Option, + api_kind: ApiDatasetKind, ) -> Self { - let size_used = match kind { - DatasetKind::Crucible => Some(0), - _ => None, + let kind = DatasetKind::from(&api_kind); + let (size_used, zone_name) = match api_kind { + ApiDatasetKind::Crucible => (Some(0), None), + ApiDatasetKind::Zone { name } => (None, Some(name)), + _ => (None, None), }; + Self { identity: DatasetIdentity::new(id), time_deleted: None, diff --git a/nexus/db-model/src/dataset_kind.rs b/nexus/db-model/src/dataset_kind.rs index 2e71b96a41..856b340a11 100644 --- a/nexus/db-model/src/dataset_kind.rs +++ b/nexus/db-model/src/dataset_kind.rs @@ -27,8 +27,8 @@ impl_enum_type!( Debug => b"debug" ); -impl From for DatasetKind { - fn from(k: internal::shared::DatasetKind) -> Self { +impl From<&internal::shared::DatasetKind> for DatasetKind { + fn from(k: &internal::shared::DatasetKind) -> Self { match k { internal::shared::DatasetKind::Crucible => DatasetKind::Crucible, internal::shared::DatasetKind::Cockroach => DatasetKind::Cockroach, @@ -45,6 +45,10 @@ impl From for DatasetKind { DatasetKind::InternalDns } internal::shared::DatasetKind::ZoneRoot => DatasetKind::ZoneRoot, + // Enums in the database do not have associated data, so this drops + // the "name" of the zone and only considers the type. + // + // The zone name, if it exists, is stored in a separate column. internal::shared::DatasetKind::Zone { .. } => DatasetKind::Zone, internal::shared::DatasetKind::Debug => DatasetKind::Debug, } diff --git a/nexus/db-queries/src/db/datastore/dataset.rs b/nexus/db-queries/src/db/datastore/dataset.rs index 8a814aea80..0fe1c7912e 100644 --- a/nexus/db-queries/src/db/datastore/dataset.rs +++ b/nexus/db-queries/src/db/datastore/dataset.rs @@ -241,6 +241,7 @@ mod test { use nexus_db_model::SledSystemHardware; use nexus_db_model::SledUpdate; use nexus_test_utils::db::test_setup_database; + use omicron_common::api::internal::shared::DatasetKind as ApiDatasetKind; use omicron_test_utils::dev; #[tokio::test] @@ -291,8 +292,7 @@ mod test { Uuid::new_v4(), zpool_id, Some("[::1]:0".parse().unwrap()), - DatasetKind::Crucible, - None, + ApiDatasetKind::Crucible, )) .await .expect("failed to insert dataset") @@ -325,8 +325,7 @@ mod test { dataset1.id(), zpool_id, Some("[::1]:12345".parse().unwrap()), - DatasetKind::Cockroach, - None, + ApiDatasetKind::Cockroach, )) .await .expect("failed to do-nothing insert dataset"); @@ -342,8 +341,7 @@ mod test { Uuid::new_v4(), zpool_id, Some("[::1]:0".parse().unwrap()), - DatasetKind::Cockroach, - None, + ApiDatasetKind::Cockroach, )) .await .expect("failed to upsert dataset"); @@ -375,8 +373,7 @@ mod test { dataset1.id(), zpool_id, Some("[::1]:12345".parse().unwrap()), - DatasetKind::Cockroach, - None, + ApiDatasetKind::Cockroach, )) .await .expect("failed to do-nothing insert dataset"); diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 10c812de89..881f0d4aa5 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -392,10 +392,10 @@ mod test { use crate::db::identity::Asset; use crate::db::lookup::LookupPath; use crate::db::model::{ - BlockSize, ConsoleSession, Dataset, DatasetKind, ExternalIp, - PhysicalDisk, PhysicalDiskKind, PhysicalDiskPolicy, PhysicalDiskState, - Project, Rack, Region, SiloUser, SledBaseboard, SledSystemHardware, - SledUpdate, SshKey, Zpool, + BlockSize, ConsoleSession, Dataset, ExternalIp, PhysicalDisk, + PhysicalDiskKind, PhysicalDiskPolicy, PhysicalDiskState, Project, Rack, + Region, SiloUser, SledBaseboard, SledSystemHardware, SledUpdate, + SshKey, Zpool, }; use crate::db::queries::vpc_subnet::InsertVpcSubnetQuery; use chrono::{Duration, Utc}; @@ -411,6 +411,7 @@ mod test { use omicron_common::api::external::{ ByteCount, Error, IdentityMetadataCreateParams, LookupType, Name, }; + use omicron_common::api::internal::shared::DatasetKind; use omicron_test_utils::dev; use omicron_uuid_kinds::CollectionUuid; use omicron_uuid_kinds::GenericUuid; @@ -908,7 +909,6 @@ mod test { zpool.pool_id, bogus_addr, DatasetKind::Crucible, - None, ); let datastore = datastore.clone(); @@ -1281,7 +1281,6 @@ mod test { zpool_id, bogus_addr, DatasetKind::Crucible, - None, ); let datastore = datastore.clone(); async move { @@ -1382,7 +1381,6 @@ mod test { zpool_id, bogus_addr, DatasetKind::Crucible, - None, ); let datastore = datastore.clone(); async move { @@ -1458,7 +1456,6 @@ mod test { zpool_id, bogus_addr, DatasetKind::Crucible, - None, ); datastore.dataset_upsert(dataset).await.unwrap(); physical_disk_ids.push(physical_disk_id); diff --git a/nexus/reconfigurator/execution/src/datasets.rs b/nexus/reconfigurator/execution/src/datasets.rs index d20de82cef..2f84378a13 100644 --- a/nexus/reconfigurator/execution/src/datasets.rs +++ b/nexus/reconfigurator/execution/src/datasets.rs @@ -67,8 +67,7 @@ pub(crate) async fn ensure_dataset_records_exist( id.into_untyped_uuid(), pool_id.into_untyped_uuid(), Some(address), - kind.clone().into(), - kind.zone_name().map(String::from), + kind.clone(), ); let maybe_inserted = datastore .dataset_insert_if_not_exists(dataset) diff --git a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs index 9dcaa098d5..380448acf2 100644 --- a/nexus/reconfigurator/execution/src/omicron_physical_disks.rs +++ b/nexus/reconfigurator/execution/src/omicron_physical_disks.rs @@ -124,7 +124,6 @@ mod test { use httptest::responders::status_code; use httptest::Expectation; use nexus_db_model::Dataset; - use nexus_db_model::DatasetKind; use nexus_db_model::PhysicalDisk; use nexus_db_model::PhysicalDiskKind; use nexus_db_model::PhysicalDiskPolicy; @@ -142,6 +141,7 @@ mod test { use nexus_types::identity::Asset; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Generation; + use omicron_common::api::internal::shared::DatasetKind; use omicron_common::disk::DiskIdentity; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PhysicalDiskUuid; @@ -435,7 +435,6 @@ mod test { 0, )), DatasetKind::Crucible, - None, )) .await .unwrap(); diff --git a/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs b/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs index 0e90ed84ab..6e49ddc7f0 100644 --- a/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs +++ b/nexus/src/app/background/tasks/decommissioned_disk_cleaner.rs @@ -179,13 +179,13 @@ mod tests { use diesel::ExpressionMethods; use diesel::QueryDsl; use nexus_db_model::Dataset; - use nexus_db_model::DatasetKind; use nexus_db_model::PhysicalDisk; use nexus_db_model::PhysicalDiskKind; use nexus_db_model::PhysicalDiskPolicy; use nexus_db_model::Region; use nexus_test_utils::SLED_AGENT_UUID; use nexus_test_utils_macros::nexus_test; + use omicron_common::api::internal::shared::DatasetKind; use omicron_uuid_kinds::{ DatasetUuid, PhysicalDiskUuid, RegionUuid, SledUuid, }; @@ -246,7 +246,6 @@ mod tests { 0, )), DatasetKind::Crucible, - None, )) .await .unwrap(); diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 432c44bea4..b289e871eb 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -146,8 +146,7 @@ impl super::Nexus { dataset.dataset_id, dataset.zpool_id, Some(dataset.request.address), - dataset.request.kind.clone().into(), - dataset.request.kind.zone_name().map(String::from), + dataset.request.kind, ) }) .collect(); diff --git a/nexus/src/app/sagas/region_replacement_start.rs b/nexus/src/app/sagas/region_replacement_start.rs index 60c35781f1..c2b886938a 100644 --- a/nexus/src/app/sagas/region_replacement_start.rs +++ b/nexus/src/app/sagas/region_replacement_start.rs @@ -776,7 +776,6 @@ pub(crate) mod test { }; use chrono::Utc; use nexus_db_model::Dataset; - use nexus_db_model::DatasetKind; use nexus_db_model::Region; use nexus_db_model::RegionReplacement; use nexus_db_model::RegionReplacementState; @@ -787,6 +786,7 @@ pub(crate) mod test { use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils_macros::nexus_test; use nexus_types::identity::Asset; + use omicron_common::api::internal::shared::DatasetKind; use sled_agent_client::types::VolumeConstructionRequest; use uuid::Uuid; @@ -903,28 +903,24 @@ pub(crate) mod test { Uuid::new_v4(), Some("[fd00:1122:3344:101::1]:12345".parse().unwrap()), DatasetKind::Crucible, - None, ), Dataset::new( Uuid::new_v4(), Uuid::new_v4(), Some("[fd00:1122:3344:102::1]:12345".parse().unwrap()), DatasetKind::Crucible, - None, ), Dataset::new( Uuid::new_v4(), Uuid::new_v4(), Some("[fd00:1122:3344:103::1]:12345".parse().unwrap()), DatasetKind::Crucible, - None, ), Dataset::new( Uuid::new_v4(), Uuid::new_v4(), Some("[fd00:1122:3344:104::1]:12345".parse().unwrap()), DatasetKind::Crucible, - None, ), ]; diff --git a/nexus/src/app/sled.rs b/nexus/src/app/sled.rs index a0e1cc3526..9c21ca73a1 100644 --- a/nexus/src/app/sled.rs +++ b/nexus/src/app/sled.rs @@ -12,7 +12,6 @@ use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; use nexus_db_queries::db::lookup; -use nexus_db_queries::db::model::DatasetKind; use nexus_sled_agent_shared::inventory::SledRole; use nexus_types::deployment::DiskFilter; use nexus_types::deployment::SledFilter; @@ -23,6 +22,7 @@ use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; +use omicron_common::api::internal::shared::DatasetKind; use omicron_uuid_kinds::{GenericUuid, SledUuid}; use sled_agent_client::Client as SledAgentClient; use std::net::SocketAddrV6; @@ -308,7 +308,7 @@ impl super::Nexus { ); let kind = DatasetKind::Crucible; let dataset = - db::model::Dataset::new(id, zpool_id, Some(address), kind, None); + db::model::Dataset::new(id, zpool_id, Some(address), kind); self.db_datastore.dataset_upsert(dataset).await?; Ok(()) } From aa6025421da9bfc1f6872ed45c50d46d1bf9816d Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 8 Aug 2024 15:24:13 -0700 Subject: [PATCH 24/36] update schema JSONs, add replace --- clients/sled-agent-client/src/lib.rs | 1 + openapi/nexus-internal.json | 135 +-------------------------- openapi/sled-agent.json | 135 +-------------------------- 3 files changed, 5 insertions(+), 266 deletions(-) diff --git a/clients/sled-agent-client/src/lib.rs b/clients/sled-agent-client/src/lib.rs index 4e7a4a72db..d65174ed3b 100644 --- a/clients/sled-agent-client/src/lib.rs +++ b/clients/sled-agent-client/src/lib.rs @@ -38,6 +38,7 @@ progenitor::generate_api!( replace = { Baseboard = nexus_sled_agent_shared::inventory::Baseboard, ByteCount = omicron_common::api::external::ByteCount, + DatasetKind = omicron_common::api::internal::shared::DatasetKind, DiskIdentity = omicron_common::disk::DiskIdentity, DiskVariant = omicron_common::disk::DiskVariant, Generation = omicron_common::api::external::Generation, diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index cfbe028f9c..7cb401d4bc 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2591,139 +2591,8 @@ ] }, "DatasetKind": { - "description": "Describes the purpose of the dataset.", - "oneOf": [ - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "cockroachdb" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "crucible" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "clickhouse" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "clickhouse_keeper" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "external_dns" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "internal_dns" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "zone_root" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "type": { - "type": "string", - "enum": [ - "zone" - ] - } - }, - "required": [ - "name", - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "debug" - ] - } - }, - "required": [ - "type" - ] - } - ] + "description": "The kind of dataset. See the `DatasetKind` enum in omicron-common for possible values.", + "type": "string" }, "DatasetPutRequest": { "description": "Describes a dataset within a pool.", diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index 82f3c6c3ae..f2c5d127c1 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2205,139 +2205,8 @@ ] }, "DatasetKind": { - "description": "Describes the purpose of the dataset.", - "oneOf": [ - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "cockroachdb" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "crucible" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "clickhouse" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "clickhouse_keeper" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "external_dns" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "internal_dns" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "zone_root" - ] - } - }, - "required": [ - "type" - ] - }, - { - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "type": { - "type": "string", - "enum": [ - "zone" - ] - } - }, - "required": [ - "name", - "type" - ] - }, - { - "type": "object", - "properties": { - "type": { - "type": "string", - "enum": [ - "debug" - ] - } - }, - "required": [ - "type" - ] - } - ] + "description": "The kind of dataset. See the `DatasetKind` enum in omicron-common for possible values.", + "type": "string" }, "DatasetManagementStatus": { "description": "Identifies how a single dataset management operation may have succeeded or failed.", From 3e14cc4602935b07515b7355cec51caf7e0ec8fc Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 9 Aug 2024 18:54:03 -0700 Subject: [PATCH 25/36] Fix schema --- schema/omicron-datasets.json | 135 +---------------------------------- 1 file changed, 2 insertions(+), 133 deletions(-) diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index b675432172..8b4bf59ae9 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -75,139 +75,8 @@ } }, "DatasetKind": { - "description": "Describes the purpose of the dataset.", - "oneOf": [ - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "cockroachdb" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "crucible" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "clickhouse" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "clickhouse_keeper" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "external_dns" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "internal_dns" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "zone_root" - ] - } - } - }, - { - "type": "object", - "required": [ - "name", - "type" - ], - "properties": { - "name": { - "type": "string" - }, - "type": { - "type": "string", - "enum": [ - "zone" - ] - } - } - }, - { - "type": "object", - "required": [ - "type" - ], - "properties": { - "type": { - "type": "string", - "enum": [ - "debug" - ] - } - } - } - ] + "description": "The kind of dataset. See the `DatasetKind` enum in omicron-common for possible values.", + "type": "string" }, "DatasetName": { "type": "object", From 077908d02873848f025b88173eb09b5c4e7050f0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 26 Aug 2024 08:46:30 -0700 Subject: [PATCH 26/36] Fix clickhouseserver datasetkind serialization --- common/src/api/internal/shared.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index a81e21e05b..4826292863 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -979,6 +979,7 @@ impl FromStr for DatasetKind { "crucible" => Crucible, "clickhouse" => Clickhouse, "clickhouse_keeper" => ClickhouseKeeper, + "clickhouse_server" => ClickhouseServer, "external_dns" => ExternalDns, "internal_dns" => InternalDns, "zone" => ZoneRoot, @@ -1068,10 +1069,11 @@ mod tests { #[test] fn test_dataset_kind_serialization() { let kinds = [ - DatasetKind::Crucible, DatasetKind::Cockroach, + DatasetKind::Crucible, DatasetKind::Clickhouse, DatasetKind::ClickhouseKeeper, + DatasetKind::ClickhouseServer, DatasetKind::ExternalDns, DatasetKind::InternalDns, DatasetKind::ZoneRoot, From a83450e7061537d6ac0495eb35f1b2749ce9e2a9 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 26 Aug 2024 16:42:50 -0700 Subject: [PATCH 27/36] Add docs, use ByteCount more, better simulated inventory --- Cargo.lock | 1 + common/src/api/external/mod.rs | 13 +++- common/src/disk.rs | 6 +- dev-tools/omdb/src/bin/omdb/sled_agent.rs | 6 +- illumos-utils/src/zfs.rs | 8 +-- sled-agent/src/backing_fs.rs | 15 ++-- sled-agent/src/sim/sled_agent.rs | 31 +++++++- sled-storage/Cargo.toml | 1 + sled-storage/src/dataset.rs | 87 +++++++++++++---------- 9 files changed, 112 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d254480ac4..cd9efeb4cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9656,6 +9656,7 @@ dependencies = [ "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", + "once_cell", "rand", "schemars", "serde", diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 07e4fd0b83..fbd5f3450b 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -593,7 +593,18 @@ impl JsonSchema for RoleName { // // TODO: custom JsonSchema impl to describe i64::MAX limit; this is blocked by // https://github.com/oxidecomputer/typify/issues/589 -#[derive(Copy, Clone, Debug, Serialize, JsonSchema, PartialEq, Eq)] +#[derive( + Copy, + Clone, + Debug, + Serialize, + JsonSchema, + Hash, + PartialEq, + Eq, + PartialOrd, + Ord, +)] pub struct ByteCount(u64); impl<'de> Deserialize<'de> for ByteCount { diff --git a/common/src/disk.rs b/common/src/disk.rs index a2016a9442..b60957fc95 100644 --- a/common/src/disk.rs +++ b/common/src/disk.rs @@ -14,7 +14,7 @@ use std::fmt; use uuid::Uuid; use crate::{ - api::external::Generation, + api::external::{ByteCount, Generation}, ledger::Ledgerable, zpool_name::{ZpoolKind, ZpoolName}, }; @@ -159,10 +159,10 @@ pub struct DatasetConfig { pub compression: Option, /// The upper bound on the amount of storage used by this dataset - pub quota: Option, + pub quota: Option, /// The lower bound on the amount of storage usable by this dataset - pub reservation: Option, + pub reservation: Option, } #[derive( diff --git a/dev-tools/omdb/src/bin/omdb/sled_agent.rs b/dev-tools/omdb/src/bin/omdb/sled_agent.rs index b97fb35e8c..44adbf4763 100644 --- a/dev-tools/omdb/src/bin/omdb/sled_agent.rs +++ b/dev-tools/omdb/src/bin/omdb/sled_agent.rs @@ -61,7 +61,11 @@ enum ZpoolCommands { #[derive(Debug, Subcommand)] enum DatasetCommands { - /// Print list of all datasets managed by the sled agent + /// Print list of all datasets the sled agent is configured to manage + /// + /// Note that the set of actual datasets on the sled may be distinct, + /// use the `omdb db inventory collections show` command to see the latest + /// set of datasets collected from sleds. List, } diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 5968569ebe..18e16f4ec7 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -206,8 +206,8 @@ pub struct EncryptionDetails { #[derive(Debug, Default)] pub struct SizeDetails { - pub quota: Option, - pub reservation: Option, + pub quota: Option, + pub reservation: Option, pub compression: Option, } @@ -507,8 +507,8 @@ impl Zfs { fn apply_properties( name: &str, mountpoint: &Mountpoint, - quota: Option, - reservation: Option, + quota: Option, + reservation: Option, compression: Option, ) -> Result<(), EnsureFilesystemError> { let quota = quota diff --git a/sled-agent/src/backing_fs.rs b/sled-agent/src/backing_fs.rs index 48002a8841..a207668a91 100644 --- a/sled-agent/src/backing_fs.rs +++ b/sled-agent/src/backing_fs.rs @@ -25,6 +25,8 @@ use camino::Utf8PathBuf; use illumos_utils::zfs::{ EnsureFilesystemError, GetValueError, Mountpoint, SizeDetails, Zfs, }; +use omicron_common::api::external::ByteCount; +use once_cell::sync::Lazy; use std::io; #[derive(Debug, thiserror::Error)] @@ -48,7 +50,7 @@ struct BackingFs<'a> { // Mountpoint mountpoint: &'static str, // Optional quota, in _bytes_ - quota: Option, + quota: Option, // Optional compression mode compression: Option<&'static str>, // Linked service @@ -74,7 +76,7 @@ impl<'a> BackingFs<'a> { self } - const fn quota(mut self, quota: usize) -> Self { + const fn quota(mut self, quota: ByteCount) -> Self { self.quota = Some(quota); self } @@ -99,18 +101,19 @@ const BACKING_FMD_DATASET: &'static str = "fmd"; const BACKING_FMD_MOUNTPOINT: &'static str = "/var/fm/fmd"; const BACKING_FMD_SUBDIRS: [&'static str; 3] = ["rsrc", "ckpt", "xprt"]; const BACKING_FMD_SERVICE: &'static str = "svc:/system/fmd:default"; -const BACKING_FMD_QUOTA: usize = 500 * (1 << 20); // 500 MiB +const BACKING_FMD_QUOTA: u64 = 500 * (1 << 20); // 500 MiB const BACKING_COMPRESSION: &'static str = "on"; const BACKINGFS_COUNT: usize = 1; -static BACKINGFS: [BackingFs; BACKINGFS_COUNT] = +static BACKINGFS: Lazy<[BackingFs; BACKINGFS_COUNT]> = Lazy::new(|| { [BackingFs::new(BACKING_FMD_DATASET) .mountpoint(BACKING_FMD_MOUNTPOINT) .subdirs(&BACKING_FMD_SUBDIRS) - .quota(BACKING_FMD_QUOTA) + .quota(ByteCount::try_from(BACKING_FMD_QUOTA).unwrap()) .compression(BACKING_COMPRESSION) - .service(BACKING_FMD_SERVICE)]; + .service(BACKING_FMD_SERVICE)] +}); /// Ensure that the backing filesystems are mounted. /// If the underlying dataset for a backing fs does not exist on the specified diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index bc1de2e6b5..786ec51bc1 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/sled-agent/src/sim/sled_agent.rs @@ -18,7 +18,8 @@ use anyhow::Context; use dropshot::{HttpError, HttpServer}; use futures::lock::Mutex; use nexus_sled_agent_shared::inventory::{ - Inventory, InventoryDisk, InventoryZpool, OmicronZonesConfig, SledRole, + Inventory, InventoryDataset, InventoryDisk, InventoryZpool, + OmicronZonesConfig, SledRole, }; use omicron_common::api::external::{ ByteCount, DiskState, Error, Generation, ResourceType, @@ -902,8 +903,32 @@ impl SledAgent { }) }) .collect::, anyhow::Error>>()?, - // TODO: Make this more real? - datasets: vec![], + // NOTE: We report the "configured" datasets as the "real" datasets + // unconditionally here. No real datasets exist, so we're free + // to lie here, but this information should be taken with a + // particularly careful grain-of-salt -- it's supposed to + // represent the "real" datasets the sled agent can observe. + datasets: storage + .datasets_config_list() + .await + .map(|config| { + config + .datasets + .into_iter() + .map(|(id, config)| InventoryDataset { + id: Some(id), + name: config.name.full_name(), + available: ByteCount::from_kibibytes_u32(0), + used: ByteCount::from_kibibytes_u32(0), + quota: config.quota, + reservation: config.reservation, + compression: config + .compression + .unwrap_or_else(|| String::new()), + }) + .collect::>() + }) + .unwrap_or_else(|_| vec![]), }) } diff --git a/sled-storage/Cargo.toml b/sled-storage/Cargo.toml index 2439c52aa7..27555ce96d 100644 --- a/sled-storage/Cargo.toml +++ b/sled-storage/Cargo.toml @@ -20,6 +20,7 @@ illumos-utils.workspace = true key-manager.workspace = true omicron-common.workspace = true omicron-uuid-kinds.workspace = true +once_cell.workspace = true rand.workspace = true schemars = { workspace = true, features = [ "chrono", "uuid1" ] } serde.workspace = true diff --git a/sled-storage/src/dataset.rs b/sled-storage/src/dataset.rs index b95877418e..f487460a6c 100644 --- a/sled-storage/src/dataset.rs +++ b/sled-storage/src/dataset.rs @@ -14,8 +14,10 @@ use illumos_utils::zfs::{ }; use illumos_utils::zpool::ZpoolName; use key_manager::StorageKeyRequester; +use omicron_common::api::external::ByteCount; use omicron_common::api::internal::shared::DatasetKind; use omicron_common::disk::{DatasetName, DiskIdentity, DiskVariant}; +use once_cell::sync::Lazy; use rand::distributions::{Alphanumeric, DistString}; use slog::{debug, info, Logger}; use std::process::Stdio; @@ -32,16 +34,16 @@ pub const M2_BACKING_DATASET: &'static str = "backing"; cfg_if! { if #[cfg(any(test, feature = "testing"))] { // Tuned for zone_bundle tests - pub const DEBUG_DATASET_QUOTA: usize = 1 << 20; + pub const DEBUG_DATASET_QUOTA: u64 = 1 << 20; } else { // TODO-correctness: This value of 100GiB is a pretty wild guess, and should be // tuned as needed. - pub const DEBUG_DATASET_QUOTA: usize = 100 * (1 << 30); + pub const DEBUG_DATASET_QUOTA: u64 = 100 * (1 << 30); } } // TODO-correctness: This value of 100GiB is a pretty wild guess, and should be // tuned as needed. -pub const DUMP_DATASET_QUOTA: usize = 100 * (1 << 30); +pub const DUMP_DATASET_QUOTA: u64 = 100 * (1 << 30); // passed to zfs create -o compression= pub const DUMP_DATASET_COMPRESSION: &'static str = "gzip-9"; @@ -54,41 +56,50 @@ pub const U2_DEBUG_DATASET: &'static str = "crypt/debug"; pub const CRYPT_DATASET: &'static str = "crypt"; const U2_EXPECTED_DATASET_COUNT: usize = 2; -static U2_EXPECTED_DATASETS: [ExpectedDataset; U2_EXPECTED_DATASET_COUNT] = [ - // Stores filesystems for zones - ExpectedDataset::new(ZONE_DATASET).wipe(), - // For storing full kernel RAM dumps - ExpectedDataset::new(DUMP_DATASET) - .quota(DUMP_DATASET_QUOTA) - .compression(DUMP_DATASET_COMPRESSION), -]; +static U2_EXPECTED_DATASETS: Lazy< + [ExpectedDataset; U2_EXPECTED_DATASET_COUNT], +> = Lazy::new(|| { + [ + // Stores filesystems for zones + ExpectedDataset::new(ZONE_DATASET).wipe(), + // For storing full kernel RAM dumps + ExpectedDataset::new(DUMP_DATASET) + .quota(ByteCount::try_from(DUMP_DATASET_QUOTA).unwrap()) + .compression(DUMP_DATASET_COMPRESSION), + ] +}); const M2_EXPECTED_DATASET_COUNT: usize = 6; -static M2_EXPECTED_DATASETS: [ExpectedDataset; M2_EXPECTED_DATASET_COUNT] = [ - // Stores software images. - // - // Should be duplicated to both M.2s. - ExpectedDataset::new(INSTALL_DATASET), - // Stores crash dumps. - ExpectedDataset::new(CRASH_DATASET), - // Backing store for OS data that should be persisted across reboots. - // Its children are selectively overlay mounted onto parts of the ramdisk - // root. - ExpectedDataset::new(M2_BACKING_DATASET), - // Stores cluter configuration information. - // - // Should be duplicated to both M.2s. - ExpectedDataset::new(CLUSTER_DATASET), - // Stores configuration data, including: - // - What services should be launched on this sled - // - Information about how to initialize the Sled Agent - // - (For scrimlets) RSS setup information - // - // Should be duplicated to both M.2s. - ExpectedDataset::new(CONFIG_DATASET), - // Store debugging data, such as service bundles. - ExpectedDataset::new(M2_DEBUG_DATASET).quota(DEBUG_DATASET_QUOTA), -]; +static M2_EXPECTED_DATASETS: Lazy< + [ExpectedDataset; M2_EXPECTED_DATASET_COUNT], +> = Lazy::new(|| { + [ + // Stores software images. + // + // Should be duplicated to both M.2s. + ExpectedDataset::new(INSTALL_DATASET), + // Stores crash dumps. + ExpectedDataset::new(CRASH_DATASET), + // Backing store for OS data that should be persisted across reboots. + // Its children are selectively overlay mounted onto parts of the ramdisk + // root. + ExpectedDataset::new(M2_BACKING_DATASET), + // Stores cluter configuration information. + // + // Should be duplicated to both M.2s. + ExpectedDataset::new(CLUSTER_DATASET), + // Stores configuration data, including: + // - What services should be launched on this sled + // - Information about how to initialize the Sled Agent + // - (For scrimlets) RSS setup information + // + // Should be duplicated to both M.2s. + ExpectedDataset::new(CONFIG_DATASET), + // Store debugging data, such as service bundles. + ExpectedDataset::new(M2_DEBUG_DATASET) + .quota(ByteCount::try_from(DEBUG_DATASET_QUOTA).unwrap()), + ] +}); // Helper type for describing expected datasets and their optional quota. #[derive(Clone, Copy, Debug)] @@ -96,7 +107,7 @@ struct ExpectedDataset { // Name for the dataset name: &'static str, // Optional quota, in _bytes_ - quota: Option, + quota: Option, // Identifies if the dataset should be deleted on boot wipe: bool, // Optional compression mode @@ -108,7 +119,7 @@ impl ExpectedDataset { ExpectedDataset { name, quota: None, wipe: false, compression: None } } - const fn quota(mut self, quota: usize) -> Self { + fn quota(mut self, quota: ByteCount) -> Self { self.quota = Some(quota); self } From e8504a58f3d89437837f2756bb843d5dc3690f02 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 27 Aug 2024 09:49:52 -0700 Subject: [PATCH 28/36] schema and openapi --- openapi/sled-agent.json | 16 ++++++++++------ schema/omicron-datasets.json | 34 ++++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index fdc87296b9..d7c4315a63 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2143,16 +2143,20 @@ "quota": { "nullable": true, "description": "The upper bound on the amount of storage used by this dataset", - "type": "integer", - "format": "uint", - "minimum": 0 + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] }, "reservation": { "nullable": true, "description": "The lower bound on the amount of storage usable by this dataset", - "type": "integer", - "format": "uint", - "minimum": 0 + "allOf": [ + { + "$ref": "#/components/schemas/ByteCount" + } + ] } }, "required": [ diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index 8b4bf59ae9..b6cd7da508 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -23,6 +23,12 @@ } }, "definitions": { + "ByteCount": { + "description": "Byte count to express memory or storage capacity.", + "type": "integer", + "format": "uint64", + "minimum": 0.0 + }, "DatasetConfig": { "description": "Configuration information necessary to request a single dataset", "type": "object", @@ -56,21 +62,25 @@ }, "quota": { "description": "The upper bound on the amount of storage used by this dataset", - "type": [ - "integer", - "null" - ], - "format": "uint", - "minimum": 0.0 + "anyOf": [ + { + "$ref": "#/definitions/ByteCount" + }, + { + "type": "null" + } + ] }, "reservation": { "description": "The lower bound on the amount of storage usable by this dataset", - "type": [ - "integer", - "null" - ], - "format": "uint", - "minimum": 0.0 + "anyOf": [ + { + "$ref": "#/definitions/ByteCount" + }, + { + "type": "null" + } + ] } } }, From 28e6497a75dd8152e0b9a2bc24e848f7793baf77 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 27 Aug 2024 16:31:20 -0700 Subject: [PATCH 29/36] Fix helios tests --- sled-agent/src/zone_bundle.rs | 13 ++++++++----- sled-storage/src/manager.rs | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/sled-agent/src/zone_bundle.rs b/sled-agent/src/zone_bundle.rs index 46cee1c415..4f5c7f4101 100644 --- a/sled-agent/src/zone_bundle.rs +++ b/sled-agent/src/zone_bundle.rs @@ -1698,6 +1698,8 @@ mod illumos_tests { use chrono::TimeZone; use chrono::Timelike; use chrono::Utc; + use omicron_common::api::external::ByteCount; + use once_cell::sync::Lazy; use rand::RngCore; use sled_storage::manager_test_harness::StorageManagerTestHarness; use slog::Drain; @@ -1884,7 +1886,9 @@ mod illumos_tests { // i.e., the "ashift" value. An empty dataset is unlikely to contain more // than one megabyte of overhead, so use that as a conservative test size to // avoid issues. - const TEST_QUOTA: usize = sled_storage::dataset::DEBUG_DATASET_QUOTA; + static TEST_QUOTA: Lazy = Lazy::new(|| { + sled_storage::dataset::DEBUG_DATASET_QUOTA.try_into().unwrap() + }); async fn run_test_with_zfs_dataset(test: T) where @@ -1932,18 +1936,17 @@ mod illumos_tests { // If this needs to change, go modify the "add_vdevs" call in // "setup_storage". assert!( - TEST_QUOTA + *TEST_QUOTA < StorageManagerTestHarness::DEFAULT_VDEV_SIZE .try_into() .unwrap(), - "Quota larger than underlying device (quota: {}, device size: {})", + "Quota larger than underlying device (quota: {:?}, device size: {})", TEST_QUOTA, StorageManagerTestHarness::DEFAULT_VDEV_SIZE, ); anyhow::ensure!( - bundle_utilization.dataset_quota - == u64::try_from(TEST_QUOTA).unwrap(), + bundle_utilization.dataset_quota == TEST_QUOTA.to_bytes(), "computed incorrect dataset quota" ); diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 747ac77823..e6f74afc8e 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -1668,7 +1668,7 @@ mod tests { // However, calling it with a different input and the same generation // number should fail. config.generation = current_config_generation; - config.datasets.values_mut().next().unwrap().reservation = Some(1024); + config.datasets.values_mut().next().unwrap().reservation = Some(1024.into()); let err = harness.handle().datasets_ensure(config.clone()).await.unwrap_err(); assert!(matches!(err, Error::DatasetConfigurationChanged { .. })); From b9d3dcc43d493f4de29f6e60483d87e3e89d2124 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 27 Aug 2024 16:38:34 -0700 Subject: [PATCH 30/36] fmt --- sled-storage/src/manager.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index e6f74afc8e..02789bfe76 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -1668,7 +1668,8 @@ mod tests { // However, calling it with a different input and the same generation // number should fail. config.generation = current_config_generation; - config.datasets.values_mut().next().unwrap().reservation = Some(1024.into()); + config.datasets.values_mut().next().unwrap().reservation = + Some(1024.into()); let err = harness.handle().datasets_ensure(config.clone()).await.unwrap_err(); assert!(matches!(err, Error::DatasetConfigurationChanged { .. })); From c9f170eb365902fb5eeebac40ef9bfaefa2a5651 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 27 Aug 2024 17:51:48 -0700 Subject: [PATCH 31/36] Make CompressionAlgorithm strongly typed --- common/src/disk.rs | 89 +++++++++++++++++++++++++++++++++++- illumos-utils/src/zfs.rs | 7 +-- sled-agent/src/backing_fs.rs | 13 +++--- sled-storage/src/dataset.rs | 22 ++++++--- 4 files changed, 113 insertions(+), 18 deletions(-) diff --git a/common/src/disk.rs b/common/src/disk.rs index a2016a9442..ed0bf8666e 100644 --- a/common/src/disk.rs +++ b/common/src/disk.rs @@ -135,6 +135,91 @@ impl DatasetName { } } +#[derive( + Copy, + Clone, + Debug, + Deserialize, + Serialize, + JsonSchema, + PartialEq, + Eq, + Hash, + PartialOrd, + Ord, +)] +pub struct GzipLevel(u8); + +// Fastest compression level +const GZIP_LEVEL_MIN: u8 = 1; + +// Best compression ratio +const GZIP_LEVEL_MAX: u8 = 9; + +impl GzipLevel { + pub const fn new() -> Self { + assert!(N >= GZIP_LEVEL_MIN, "Compression level too small"); + assert!(N <= GZIP_LEVEL_MAX, "Compression level too large"); + Self(N) + } +} + +#[derive( + Copy, + Clone, + Debug, + Default, + Deserialize, + Serialize, + JsonSchema, + PartialEq, + Eq, + Hash, + PartialOrd, + Ord, +)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum CompressionAlgorithm { + // Selects a default compression algorithm. This is dependent on both the + // zpool and OS version. + On, + + // Disables compression. + #[default] + Off, + + // Selects the default Gzip compression level. + // + // According to the ZFS docs, this is "gzip-6", but that's a default value, + // which may change with OS updates. + Gzip, + + GzipN { + level: GzipLevel, + }, + Lz4, + Lzjb, + Zle, +} + +impl fmt::Display for CompressionAlgorithm { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use CompressionAlgorithm::*; + let s = match self { + On => "on", + Off => "off", + Gzip => "gzip", + GzipN { level } => { + return write!(f, "gzip-{}", level.0); + } + Lz4 => "lz4", + Lzjb => "lzjb", + Zle => "zle", + }; + write!(f, "{}", s) + } +} + /// Configuration information necessary to request a single dataset #[derive( Clone, @@ -155,8 +240,8 @@ pub struct DatasetConfig { /// The dataset's name pub name: DatasetName, - /// The compression mode to be supplied, if any - pub compression: Option, + /// The compression mode to be used by the dataset + pub compression: CompressionAlgorithm, /// The upper bound on the amount of storage used by this dataset pub quota: Option, diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 5df1b73c07..5d512677f8 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -6,6 +6,7 @@ use crate::{execute, PFEXEC}; use camino::{Utf8Path, Utf8PathBuf}; +use omicron_common::disk::CompressionAlgorithm; use omicron_common::disk::DiskIdentity; use std::fmt; @@ -204,7 +205,7 @@ pub struct EncryptionDetails { pub struct SizeDetails { pub quota: Option, pub reservation: Option, - pub compression: Option, + pub compression: CompressionAlgorithm, } #[cfg_attr(any(test, feature = "testing"), mockall::automock, allow(dead_code))] @@ -403,7 +404,7 @@ impl Zfs { mountpoint: &Mountpoint, quota: Option, reservation: Option, - compression: Option, + compression: CompressionAlgorithm, ) -> Result<(), EnsureFilesystemError> { let quota = quota .map(|q| q.to_string()) @@ -411,7 +412,7 @@ impl Zfs { let reservation = reservation .map(|r| r.to_string()) .unwrap_or_else(|| String::from("none")); - let compression = compression.unwrap_or_else(|| String::from("off")); + let compression = compression.to_string(); if let Err(err) = Self::set_value(name, "quota", "a) { return Err(EnsureFilesystemError { diff --git a/sled-agent/src/backing_fs.rs b/sled-agent/src/backing_fs.rs index 48002a8841..a0f7826db3 100644 --- a/sled-agent/src/backing_fs.rs +++ b/sled-agent/src/backing_fs.rs @@ -25,6 +25,7 @@ use camino::Utf8PathBuf; use illumos_utils::zfs::{ EnsureFilesystemError, GetValueError, Mountpoint, SizeDetails, Zfs, }; +use omicron_common::disk::CompressionAlgorithm; use std::io; #[derive(Debug, thiserror::Error)] @@ -50,7 +51,7 @@ struct BackingFs<'a> { // Optional quota, in _bytes_ quota: Option, // Optional compression mode - compression: Option<&'static str>, + compression: CompressionAlgorithm, // Linked service service: Option<&'static str>, // Subdirectories to ensure @@ -63,7 +64,7 @@ impl<'a> BackingFs<'a> { name, mountpoint: "legacy", quota: None, - compression: None, + compression: CompressionAlgorithm::Off, service: None, subdirs: None, } @@ -79,8 +80,8 @@ impl<'a> BackingFs<'a> { self } - const fn compression(mut self, compression: &'static str) -> Self { - self.compression = Some(compression); + const fn compression(mut self, compression: CompressionAlgorithm) -> Self { + self.compression = compression; self } @@ -101,7 +102,7 @@ const BACKING_FMD_SUBDIRS: [&'static str; 3] = ["rsrc", "ckpt", "xprt"]; const BACKING_FMD_SERVICE: &'static str = "svc:/system/fmd:default"; const BACKING_FMD_QUOTA: usize = 500 * (1 << 20); // 500 MiB -const BACKING_COMPRESSION: &'static str = "on"; +const BACKING_COMPRESSION: CompressionAlgorithm = CompressionAlgorithm::On; const BACKINGFS_COUNT: usize = 1; static BACKINGFS: [BackingFs; BACKINGFS_COUNT] = @@ -138,7 +139,7 @@ pub(crate) fn ensure_backing_fs( let size_details = Some(SizeDetails { quota: bfs.quota, reservation: None, - compression: bfs.compression.map(|s| s.to_string()), + compression: bfs.compression, }); Zfs::ensure_filesystem( diff --git a/sled-storage/src/dataset.rs b/sled-storage/src/dataset.rs index b95877418e..e2b024db11 100644 --- a/sled-storage/src/dataset.rs +++ b/sled-storage/src/dataset.rs @@ -15,7 +15,9 @@ use illumos_utils::zfs::{ use illumos_utils::zpool::ZpoolName; use key_manager::StorageKeyRequester; use omicron_common::api::internal::shared::DatasetKind; -use omicron_common::disk::{DatasetName, DiskIdentity, DiskVariant}; +use omicron_common::disk::{ + CompressionAlgorithm, DatasetName, DiskIdentity, DiskVariant, GzipLevel, +}; use rand::distributions::{Alphanumeric, DistString}; use slog::{debug, info, Logger}; use std::process::Stdio; @@ -43,7 +45,8 @@ cfg_if! { // tuned as needed. pub const DUMP_DATASET_QUOTA: usize = 100 * (1 << 30); // passed to zfs create -o compression= -pub const DUMP_DATASET_COMPRESSION: &'static str = "gzip-9"; +pub const DUMP_DATASET_COMPRESSION: CompressionAlgorithm = + CompressionAlgorithm::GzipN { level: GzipLevel::new::<9>() }; // U.2 datasets live under the encrypted dataset and inherit encryption pub const ZONE_DATASET: &'static str = "crypt/zone"; @@ -100,12 +103,17 @@ struct ExpectedDataset { // Identifies if the dataset should be deleted on boot wipe: bool, // Optional compression mode - compression: Option<&'static str>, + compression: CompressionAlgorithm, } impl ExpectedDataset { const fn new(name: &'static str) -> Self { - ExpectedDataset { name, quota: None, wipe: false, compression: None } + ExpectedDataset { + name, + quota: None, + wipe: false, + compression: CompressionAlgorithm::Off, + } } const fn quota(mut self, quota: usize) -> Self { @@ -118,8 +126,8 @@ impl ExpectedDataset { self } - const fn compression(mut self, compression: &'static str) -> Self { - self.compression = Some(compression); + const fn compression(mut self, compression: CompressionAlgorithm) -> Self { + self.compression = compression; self } } @@ -291,7 +299,7 @@ pub(crate) async fn ensure_zpool_has_datasets( let size_details = Some(SizeDetails { quota: dataset.quota, reservation: None, - compression: dataset.compression.map(|s| s.to_string()), + compression: dataset.compression, }); Zfs::ensure_filesystem( name, From f5bc35ad1fb8aa0ce48f6713e1640aeea04e3705 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 27 Aug 2024 17:53:38 -0700 Subject: [PATCH 32/36] Fixing helios-only tests, clippy --- sled-storage/src/manager.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index 747ac77823..88e1bbaa34 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -1028,7 +1028,7 @@ impl StorageManager { let size_details = Some(illumos_utils::zfs::SizeDetails { quota: config.quota, reservation: config.reservation, - compression: config.compression.clone(), + compression: config.compression, }); Zfs::ensure_filesystem( fs_name, @@ -1122,6 +1122,7 @@ mod tests { use super::*; use camino_tempfile::tempdir_in; use omicron_common::api::external::Generation; + use omicron_common::disk::CompressionAlgorithm; use omicron_common::disk::DatasetKind; use omicron_common::disk::DiskManagementError; use omicron_common::ledger; @@ -1632,7 +1633,7 @@ mod tests { DatasetConfig { id, name, - compression: None, + compression: CompressionAlgorithm::Off, quota: None, reservation: None, }, From 474ec133d0cc105366632249e5bc93cad4fe517f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 28 Aug 2024 10:11:22 -0700 Subject: [PATCH 33/36] schemas --- openapi/sled-agent.json | 121 ++++++++++++++++++++++++++++++++++- schema/omicron-datasets.json | 121 +++++++++++++++++++++++++++++++++-- 2 files changed, 235 insertions(+), 7 deletions(-) diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index c314504745..bb8e4e0b87 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2061,6 +2061,112 @@ } ] }, + "CompressionAlgorithm": { + "oneOf": [ + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "on" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "off" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "gzip" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "level": { + "$ref": "#/components/schemas/GzipLevel" + }, + "type": { + "type": "string", + "enum": [ + "gzip_n" + ] + } + }, + "required": [ + "level", + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "lz4" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "lzjb" + ] + } + }, + "required": [ + "type" + ] + }, + { + "type": "object", + "properties": { + "type": { + "type": "string", + "enum": [ + "zle" + ] + } + }, + "required": [ + "type" + ] + } + ] + }, "CrucibleOpts": { "description": "CrucibleOpts\n\n
JSON schema\n\n```json { \"type\": \"object\", \"required\": [ \"id\", \"lossy\", \"read_only\", \"target\" ], \"properties\": { \"cert_pem\": { \"type\": [ \"string\", \"null\" ] }, \"control\": { \"type\": [ \"string\", \"null\" ] }, \"flush_timeout\": { \"type\": [ \"number\", \"null\" ], \"format\": \"float\" }, \"id\": { \"type\": \"string\", \"format\": \"uuid\" }, \"key\": { \"type\": [ \"string\", \"null\" ] }, \"key_pem\": { \"type\": [ \"string\", \"null\" ] }, \"lossy\": { \"type\": \"boolean\" }, \"read_only\": { \"type\": \"boolean\" }, \"root_cert_pem\": { \"type\": [ \"string\", \"null\" ] }, \"target\": { \"type\": \"array\", \"items\": { \"type\": \"string\" } } } } ```
", "type": "object", @@ -2119,9 +2225,12 @@ "type": "object", "properties": { "compression": { - "nullable": true, - "description": "The compression mode to be supplied, if any", - "type": "string" + "description": "The compression mode to be used by the dataset", + "allOf": [ + { + "$ref": "#/components/schemas/CompressionAlgorithm" + } + ] }, "id": { "description": "The UUID of the dataset being requested", @@ -2155,6 +2264,7 @@ } }, "required": [ + "compression", "id", "name" ] @@ -2874,6 +2984,11 @@ "format": "uint64", "minimum": 0 }, + "GzipLevel": { + "type": "integer", + "format": "uint8", + "minimum": 0 + }, "HostIdentifier": { "description": "A `HostIdentifier` represents either an IP host or network (v4 or v6), or an entire VPC (identified by its VNI). It is used in firewall rule host filters.", "oneOf": [ diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index 8b4bf59ae9..07fc2cfb13 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -23,19 +23,127 @@ } }, "definitions": { + "CompressionAlgorithm": { + "oneOf": [ + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "on" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "off" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "gzip" + ] + } + } + }, + { + "type": "object", + "required": [ + "level", + "type" + ], + "properties": { + "level": { + "$ref": "#/definitions/GzipLevel" + }, + "type": { + "type": "string", + "enum": [ + "gzip_n" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "lz4" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "lzjb" + ] + } + } + }, + { + "type": "object", + "required": [ + "type" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "zle" + ] + } + } + } + ] + }, "DatasetConfig": { "description": "Configuration information necessary to request a single dataset", "type": "object", "required": [ + "compression", "id", "name" ], "properties": { "compression": { - "description": "The compression mode to be supplied, if any", - "type": [ - "string", - "null" + "description": "The compression mode to be used by the dataset", + "allOf": [ + { + "$ref": "#/definitions/CompressionAlgorithm" + } ] }, "id": { @@ -99,6 +207,11 @@ "format": "uint64", "minimum": 0.0 }, + "GzipLevel": { + "type": "integer", + "format": "uint8", + "minimum": 0.0 + }, "TypedUuidForDatasetKind": { "type": "string", "format": "uuid" From 77b3721600198ca815727f7d3ab2760ac3cd56d1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 28 Aug 2024 14:08:25 -0700 Subject: [PATCH 34/36] Pass byte values as single integers (no 'GiB' suffixes) --- illumos-utils/src/zfs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 64226c1fa1..71c22425fa 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -513,10 +513,10 @@ impl Zfs { compression: CompressionAlgorithm, ) -> Result<(), EnsureFilesystemError> { let quota = quota - .map(|q| q.to_string()) + .map(|q| q.to_bytes().to_string()) .unwrap_or_else(|| String::from("none")); let reservation = reservation - .map(|r| r.to_string()) + .map(|r| r.to_bytes().to_string()) .unwrap_or_else(|| String::from("none")); let compression = compression.to_string(); From 67c218186903a1e89370111bc8849fde0b9fdf76 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 5 Sep 2024 19:57:38 -0700 Subject: [PATCH 35/36] review feedback --- dev-tools/omdb/src/bin/omdb/db.rs | 9 ++- illumos-utils/src/zfs.rs | 101 ++++++++++++++++-------------- schema/crdb/inv-dataset/up01.sql | 1 - 3 files changed, 63 insertions(+), 48 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index 3cea514d6d..642a4fb31b 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -4777,7 +4777,14 @@ fn inv_collection_print_sleds(collection: &Collection) { reservation, compression, } = dataset; - println!(" {name} - id: {id:?}, compression: {compression}"); + + let id = if let Some(id) = id { + id.to_string() + } else { + String::from("none") + }; + + println!(" {name} - id: {id}, compression: {compression}"); println!(" available: {available}, used: {used}"); println!(" reservation: {reservation:?}, quota: {quota:?}"); } diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index 71c22425fa..d711ca4f9d 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -218,65 +218,74 @@ pub struct DatasetProperties { pub id: Option, /// The full name of the dataset. pub name: String, - /// Remaining space in the dataset and descendents. + /// Remaining space in the dataset and descendants. pub avail: ByteCount, - /// Space used by dataset and descendents. + /// Space used by dataset and descendants. pub used: ByteCount, - /// Maximum space usable by dataset and descendents. + /// Maximum space usable by dataset and descendants. pub quota: Option, - /// Minimum space guaranteed to dataset and descendents. + /// Minimum space guaranteed to dataset and descendants. pub reservation: Option, /// The compression algorithm used for this dataset. + /// + /// This probably aligns with a value from + /// [omicron_common::disk::CompressionAlgorithm], but is left as an untyped + /// string so that unexpected compression formats don't prevent inventory + /// from being collected. pub compression: String, } -impl FromStr for DatasetProperties { - type Err = anyhow::Error; +impl DatasetProperties { + // care about. + const ZFS_LIST_STR: &'static str = + "oxide:uuid,name,avail,used,quota,reservation,compression"; +} - fn from_str(s: &str) -> Result { - let mut iter = s.split_whitespace(); +// 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(); - let id = match iter.next().context("Missing UUID")? { - "-" => None, - anything_else => Some(anything_else.parse::()?), - }; + let id = match iter.next().context("Missing UUID")? { + "-" => None, + anything_else => Some(anything_else.parse::()?), + }; - 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::()? - { + 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(); + let compression = iter.next().context("Missing 'compression'")?.to_string(); + + Ok(DatasetProperties { + id, + name, + avail, + used, + quota, + reservation, + compression, + }) +} - Ok(DatasetProperties { - id, - name, - avail, - used, - quota, - reservation, - compression, - }) +impl FromStr for DatasetProperties { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + dataset_properties_parse(s) + .with_context(|| format!("Failed to parse: {s}")) } } @@ -316,13 +325,13 @@ impl Zfs { let cmd = command.args(&["list", "-d", "1", "-rHpo"]); // Note: this is tightly coupled with the layout of DatasetProperties - cmd.arg("oxide:uuid,name,avail,used,quota,reservation,compression"); + cmd.arg(DatasetProperties::ZFS_LIST_STR); cmd.args(datasets); let output = execute(cmd).with_context(|| { format!("Failed to get dataset properties for {datasets:?}") })?; - let stdout = String::from_utf8_lossy(&output.stdout); + let stdout = String::from_utf8(output.stdout)?; let mut datasets = stdout .trim() .split('\n') diff --git a/schema/crdb/inv-dataset/up01.sql b/schema/crdb/inv-dataset/up01.sql index 4504768c40..d3d21d16ae 100644 --- a/schema/crdb/inv-dataset/up01.sql +++ b/schema/crdb/inv-dataset/up01.sql @@ -13,4 +13,3 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_dataset ( PRIMARY KEY (inv_collection_id, sled_id, name) ); - From 557dc222dc61b8d3c6f571efeac17f7430f3def5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 5 Sep 2024 22:53:32 -0700 Subject: [PATCH 36/36] Fix error wrapping in tests --- illumos-utils/src/zfs.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/illumos-utils/src/zfs.rs b/illumos-utils/src/zfs.rs index d711ca4f9d..f92fd5d60f 100644 --- a/illumos-utils/src/zfs.rs +++ b/illumos-utils/src/zfs.rs @@ -859,7 +859,7 @@ mod test { let err = DatasetProperties::from_str(&input) .expect_err("Should have failed to parse"); assert!( - err.to_string().contains("error parsing UUID (dataset)"), + format!("{err:#}").contains("error parsing UUID (dataset)"), "{err}" ); } @@ -870,7 +870,7 @@ mod test { let err = DatasetProperties::from_str(&input) .expect_err("Should have failed to parse"); assert!( - err.to_string().contains("invalid digit found in string"), + format!("{err:#}").contains("invalid digit found in string"), "{err}" ); } @@ -881,7 +881,7 @@ mod test { let err = DatasetProperties::from_str(&input) .expect_err("Should have failed to parse"); assert!( - err.to_string().contains("invalid digit found in string"), + format!("{err:#}").contains("invalid digit found in string"), "{err}" ); } @@ -892,7 +892,7 @@ mod test { let err = DatasetProperties::from_str(&input) .expect_err("Should have failed to parse"); assert!( - err.to_string().contains("invalid digit found in string"), + format!("{err:#}").contains("invalid digit found in string"), "{err}" ); } @@ -903,7 +903,7 @@ mod test { let err = DatasetProperties::from_str(&input) .expect_err("Should have failed to parse"); assert!( - err.to_string().contains("invalid digit found in string"), + format!("{err:#}").contains("invalid digit found in string"), "{err}" ); } @@ -913,7 +913,8 @@ mod test { let expect_missing = |input: &str, what: &str| { let err = DatasetProperties::from_str(input) .expect_err("Should have failed to parse"); - assert!(err.to_string().contains(&format!("Missing {what}"))); + let err = format!("{err:#}"); + assert!(err.contains(&format!("Missing {what}")), "{err}"); }; expect_missing(