Serialize via string, not tag

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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        serializer.serialize_str(&self.to_string())
+    }
+}
+
+impl<'de> Deserialize<'de> for DatasetKind {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    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<Self, Self::Err> {
         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"
+            );
+        }
+    }
 }

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<DatasetConfig>,
+    pub datasets: BTreeMap<DatasetUuid, DatasetConfig>,
 }
 
 impl Default for DatasetsConfig {
     fn default() -> Self {
-        Self { generation: Generation::new(), datasets: vec![] }
+        Self { generation: Generation::new(), datasets: BTreeMap::new() }
     }
 }
 

openapi/nexus-internal.json

@@ -2599,7 +2599,7 @@
               "type": {
                 "type": "string",
                 "enum": [
-                  "cockroach_db"
+                  "cockroachdb"
                 ]
               }
             },

openapi/sled-agent.json

@@ -2374,8 +2374,8 @@
         "type": "object",
         "properties": {
           "datasets": {
-            "type": "array",
-            "items": {
+            "type": "object",
+            "additionalProperties": {
               "$ref": "#/components/schemas/DatasetConfig"
             }
           },

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"
               ]
             }
           }

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 },
 

sled-storage/src/manager.rs

@@ -674,12 +674,19 @@ impl StorageManager {
 
     async fn datasets_ensure(
         &mut self,
-        mut config: DatasetsConfig,
+        config: DatasetsConfig,
     ) -> Result<DatasetsManagementResult, Error> {
         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 { .. }));