[sled-agent] PUT zones doesn't create datasets

common/src/ledger.rs

@@ -130,6 +130,7 @@ impl<T: Ledgerable> Ledger<T> {
                 warn!(self.log, "Failed to write ledger"; "path" => ?path, "err" => ?e);
                 failed_paths.push((path.to_path_buf(), e));
             } else {
+                info!(self.log, "Successfully wrote to ledger"; "path" => ?path);
                 one_successful_write = true;
             }
         }

sled-agent/src/services.rs

@@ -275,6 +275,12 @@ pub enum Error {
     #[error("Error querying simnet devices")]
     Simnet(#[from] GetSimnetError),
 
+    #[error("Failed to access storage")]
+    Storage(#[from] sled_storage::error::Error),
+
+    #[error("Missing datasets: {datasets:?}")]
+    MissingDatasets { datasets: HashSet<DatasetName> },
+
     #[error(
         "Requested generation ({requested}) is older than current ({current})"
     )]
@@ -3470,6 +3476,59 @@ impl ServiceManager {
         Ok(())
     }
 
+    // Compares the incoming set of zones with the datasets this sled is
+    // configured to use.
+    //
+    // If the requested zones contain any datasets not configured on this sled,
+    // an error is returned.
+    async fn check_requested_zone_datasets_exist(
+        &self,
+        request: &OmicronZonesConfig,
+    ) -> Result<(), Error> {
+        // Before we provision these zones, inspect the request, and
+        // check all the datasets we're trying to make.
+        let mut requested_datasets = HashSet::new();
+        for zone in &request.zones {
+            if let Some(dataset) = zone.dataset_name() {
+                requested_datasets.insert(dataset);
+            }
+
+            if let Some(pool) = zone.filesystem_pool.as_ref() {
+                requested_datasets.insert(DatasetName::new(
+                    pool.clone(),
+                    DatasetKind::TransientZone {
+                        name: illumos_utils::zone::zone_name(
+                            zone.zone_type.kind().zone_prefix(),
+                            Some(zone.id),
+                        ),
+                    },
+                ));
+            }
+        }
+
+        // These datasets are configured to be part of the control plane.
+        let datasets_config = self.inner.storage.datasets_config_list().await?;
+        let existing_datasets: HashSet<_> = datasets_config
+            .datasets
+            .into_values()
+            .map(|config| config.name)
+            .collect();
+
+        // Check that the request is no larger than the configured set.
+        //
+        // (It's fine to have "existing_datasets" not contained in the
+        // request set).
+        let missing_datasets: HashSet<_> = requested_datasets
+            .difference(&existing_datasets)
+            .cloned()
+            .collect();
+        if !missing_datasets.is_empty() {
+            warn!(self.inner.log, "Missing datasets: {missing_datasets:?}");
+            return Err(Error::MissingDatasets { datasets: missing_datasets });
+        }
+        Ok(())
+    }
+
     // Ensures that only the following Omicron zones are running.
     //
     // This method strives to be idempotent.
@@ -3493,6 +3552,8 @@ impl ServiceManager {
         new_request: OmicronZonesConfig,
         fake_install_dir: Option<&String>,
     ) -> Result<(), Error> {
+        self.check_requested_zone_datasets_exist(&new_request).await?;
+
         // Do some data-normalization to ensure we can compare the "requested
         // set" vs the "existing set" as HashSets.
         let old_zone_configs: HashSet<OmicronZoneConfig> = existing_zones
@@ -4694,6 +4755,7 @@ mod illumos_tests {
         zone::MockZones,
     };
 
+    use omicron_common::disk::DatasetsConfig;
     use omicron_uuid_kinds::OmicronZoneUuid;
     use sled_storage::manager_test_harness::StorageManagerTestHarness;
     use std::os::unix::process::ExitStatusExt;
@@ -5039,6 +5101,18 @@ mod illumos_tests {
             .await
             .expect("Failed to ensure disks");
         assert!(!result.has_error(), "{:?}", result);
+        let result = harness
+            .handle()
+            .datasets_ensure(DatasetsConfig {
+                generation: Generation::new(),
+                datasets: BTreeMap::new(),
+            })
+            .await
+            .unwrap();
+        assert!(
+            !result.has_error(),
+            "Failed to ensure empty dataset ledger: {result:?}"
+        );
         harness
     }
 

sled-agent/src/sled_agent.rs

@@ -15,7 +15,6 @@ use crate::metrics::MetricsManager;
 use crate::nexus::{
     NexusClient, NexusNotifierHandle, NexusNotifierInput, NexusNotifierTask,
 };
-use crate::params::OmicronZoneTypeExt;
 use crate::probe_manager::ProbeManager;
 use crate::services::{self, ServiceManager};
 use crate::storage_monitor::StorageMonitorHandle;
@@ -943,24 +942,6 @@ impl SledAgent {
         &self,
         requested_zones: OmicronZonesConfig,
     ) -> Result<(), Error> {
-        // 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,
-        // rather than requesting individual datasets.
-        for zone in &requested_zones.zones {
-            let Some(dataset_name) = zone.dataset_name() else {
-                continue;
-            };
-
-            // First, ensure the dataset exists
-            let dataset_id = zone.id.into_untyped_uuid();
-            self.inner
-                .storage
-                .upsert_filesystem(dataset_id, dataset_name)
-                .await?;
-        }
-
         self.inner
             .services
             .ensure_all_omicron_zones_persistent(requested_zones, None)

sled-storage/src/manager.rs

@@ -856,6 +856,7 @@ impl StorageManager {
         let ledger_paths = self.all_omicron_dataset_ledgers().await;
         let maybe_ledger =
             Ledger::<DatasetsConfig>::new(&log, ledger_paths.clone()).await;
+        let had_old_ledger = maybe_ledger.is_some();
 
         let mut ledger = match maybe_ledger {
             Some(ledger) => {
@@ -914,7 +915,11 @@ impl StorageManager {
         let result = self.datasets_ensure_internal(&log, &config).await;
 
         let ledger_data = ledger.data_mut();
-        if *ledger_data == config {
+        if had_old_ledger && *ledger_data == config {
+            info!(
+                log,
+                "Skipping dataset ledger update because nothing changed"
+            );
             return Ok(result);
         }
         *ledger_data = config;
@@ -1142,6 +1147,7 @@ impl StorageManager {
             ledger_paths.clone(),
         )
         .await;
+        let had_old_ledger = maybe_ledger.is_some();
 
         let mut ledger = match maybe_ledger {
             Some(ledger) => {
@@ -1181,7 +1187,7 @@ impl StorageManager {
             self.omicron_physical_disks_ensure_internal(&log, &config).await?;
 
         let ledger_data = ledger.data_mut();
-        if *ledger_data == config {
+        if had_old_ledger && *ledger_data == config {
             return Ok(result);
         }
         *ledger_data = config;