[sled-agent-config-reconciler] Flesh out ExternalDisks type (#8111)

Author: jgallagher

common/src/disk.rs

@@ -518,6 +518,9 @@ pub enum DiskManagementError {
     #[error("Disk requested by control plane, but not found on device")]
     NotFound,
 
+    #[error("Disk requested by control plane is an internal disk: {0}")]
+    InternalDiskControlPlaneRequest(PhysicalDiskUuid),
+
     #[error("Expected zpool UUID of {expected}, but saw {observed}")]
     ZpoolUuidMismatch { expected: ZpoolUuid, observed: ZpoolUuid },
 

openapi/sled-agent.json

@@ -3793,6 +3793,24 @@
               "type"
             ]
           },
+          {
+            "type": "object",
+            "properties": {
+              "type": {
+                "type": "string",
+                "enum": [
+                  "internal_disk_control_plane_request"
+                ]
+              },
+              "value": {
+                "$ref": "#/components/schemas/TypedUuidForPhysicalDiskKind"
+              }
+            },
+            "required": [
+              "type",
+              "value"
+            ]
+          },
           {
             "type": "object",
             "properties": {

sled-agent/config-reconciler/src/disks_common.rs

@@ -0,0 +1,53 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//! Functionality common to both internal and external managed disk.
+
+use sled_storage::disk::Disk;
+use sled_storage::disk::RawDisk;
+use slog::Logger;
+use slog::info;
+use slog::warn;
+
+#[derive(Debug)]
+// `Disk` is ~200 bytes, but the callers wants ownership of them, so it'd be
+// annoying to box them. Suppress the clippy lint about the variants having
+// large size differences.
+#[allow(clippy::large_enum_variant)]
+pub(crate) enum MaybeUpdatedDisk {
+    Unchanged,
+    Updated(Disk),
+}
+
+pub(crate) fn update_properties_from_raw_disk(
+    disk: &Disk,
+    raw_disk: &RawDisk,
+    log: &Logger,
+) -> MaybeUpdatedDisk {
+    if *raw_disk == RawDisk::from(disk.clone()) {
+        return MaybeUpdatedDisk::Unchanged;
+    }
+
+    // The only property we expect to change is the firmware metadata. Update
+    // that and check again; if they're still not equal, something weird is
+    // going on. At least log a warning.
+    let mut disk = disk.clone();
+    disk.update_firmware_metadata(raw_disk);
+    if *raw_disk == RawDisk::from(disk.clone()) {
+        info!(
+            log, "Updated disk firmware metadata";
+            "firmware" => ?disk.firmware(),
+            "identity" => ?disk.identity(),
+        );
+    } else {
+        warn!(
+            log,
+            "Updated disk firmware metadata from raw disk properties, \
+             but other properties are different!";
+            "disk" => ?disk,
+            "raw_disk" => ?*raw_disk,
+        );
+    }
+    MaybeUpdatedDisk::Updated(disk)
+}

sled-agent/config-reconciler/src/external_disks.rs

@@ -45,6 +45,8 @@ use std::time::Duration;
 use tokio::sync::watch;
 use tokio::sync::watch::error::RecvError;
 
+use crate::disks_common::MaybeUpdatedDisk;
+use crate::disks_common::update_properties_from_raw_disk;
 use crate::raw_disks::RawDiskWithId;
 
 /// A thin wrapper around a [`watch::Receiver`] that presents a similar API.
@@ -728,36 +730,16 @@ impl InternalDisksTask {
                 let existing_disk = disks
                     .get(raw_disk.identity())
                     .expect("disk should still be present");
-                let existing_raw_disk =
-                    RawDisk::from(existing_disk.disk.clone());
-
-                if *raw_disk != existing_raw_disk {
-                    // The only property we expect to change is the firmware
-                    // metadata. Update that and check again; if they're still
-                    // not equal, something weird is going on. At least log a
-                    // warning.
-                    let mut updated_disk = existing_disk.clone();
-                    updated_disk.disk.update_firmware_metadata(&raw_disk);
-
-                    if *raw_disk == RawDisk::from(updated_disk.disk.clone()) {
-                        info!(
-                            self.log, "Updated disk firmware metadata";
-                            "old" => ?updated_disk.firmware(),
-                            "new" => ?raw_disk.firmware(),
-                            "identity" => ?updated_disk.id(),
-                        );
-                    } else {
-                        warn!(
-                            self.log,
-                            "Updated disk firmware metadata, \
-                             but other disk properties are different!";
-                            "old" => ?existing_raw_disk,
-                            "new" => ?*raw_disk,
-                        );
+                match update_properties_from_raw_disk(
+                    &existing_disk.disk,
+                    &raw_disk,
+                    &self.log,
+                ) {
+                    MaybeUpdatedDisk::Updated(new_disk) => {
+                        disks.insert(InternalDisk::from(new_disk));
+                        changed = true;
                     }
-
-                    disks.insert(updated_disk);
-                    changed = true;
+                    MaybeUpdatedDisk::Unchanged => (),
                 }
             }
 

sled-agent/config-reconciler/src/internal_disks.rs

@@ -50,7 +50,7 @@
 #![allow(dead_code)]
 
 mod dataset_serialization_task;
-mod external_disks;
+mod disks_common;
 mod handle;
 mod internal_disks;
 mod ledger;

sled-agent/config-reconciler/src/lib.rs

@@ -9,22 +9,24 @@ use chrono::Utc;
 use illumos_utils::dladm::EtherstubVnic;
 use illumos_utils::running_zone::RunCommandError;
 use illumos_utils::zpool::PathInPool;
-use illumos_utils::zpool::ZpoolName;
 use key_manager::StorageKeyRequester;
 use nexus_sled_agent_shared::inventory::OmicronSledConfig;
 use sled_agent_types::time_sync::TimeSync;
 use slog::Logger;
-use std::collections::BTreeSet;
 use std::sync::Arc;
 use std::time::Duration;
 use std::time::Instant;
 use tokio::sync::watch;
-use tokio::sync::watch::error::RecvError;
 
 use crate::TimeSyncConfig;
 use crate::ledger::CurrentSledConfig;
 use crate::sled_agent_facilities::SledAgentFacilities;
 
+mod external_disks;
+
+pub use self::external_disks::CurrentlyManagedZpools;
+pub use self::external_disks::CurrentlyManagedZpoolsReceiver;
+
 #[allow(clippy::too_many_arguments)]
 pub(crate) fn spawn<T: SledAgentFacilities>(
     key_requester: StorageKeyRequester,
@@ -133,114 +135,6 @@ pub enum TimeSyncError {
     FailedToParse { reason: &'static str, stdout: String },
 }
 
-#[derive(Debug, Clone)]
-pub struct CurrentlyManagedZpoolsReceiver {
-    inner: CurrentlyManagedZpoolsReceiverInner,
-}
-
-#[derive(Debug, Clone)]
-enum CurrentlyManagedZpoolsReceiverInner {
-    Real(watch::Receiver<Arc<CurrentlyManagedZpools>>),
-    #[cfg(any(test, feature = "testing"))]
-    FakeDynamic(watch::Receiver<BTreeSet<ZpoolName>>),
-    #[cfg(any(test, feature = "testing"))]
-    FakeStatic(BTreeSet<ZpoolName>),
-}
-
-impl CurrentlyManagedZpoolsReceiver {
-    #[cfg(any(test, feature = "testing"))]
-    pub fn fake_dynamic(rx: watch::Receiver<BTreeSet<ZpoolName>>) -> Self {
-        Self { inner: CurrentlyManagedZpoolsReceiverInner::FakeDynamic(rx) }
-    }
-
-    #[cfg(any(test, feature = "testing"))]
-    pub fn fake_static(zpools: impl Iterator<Item = ZpoolName>) -> Self {
-        Self {
-            inner: CurrentlyManagedZpoolsReceiverInner::FakeStatic(
-                zpools.collect(),
-            ),
-        }
-    }
-
-    pub(crate) fn new(
-        rx: watch::Receiver<Arc<CurrentlyManagedZpools>>,
-    ) -> Self {
-        Self { inner: CurrentlyManagedZpoolsReceiverInner::Real(rx) }
-    }
-
-    pub fn current(&self) -> Arc<CurrentlyManagedZpools> {
-        match &self.inner {
-            CurrentlyManagedZpoolsReceiverInner::Real(rx) => {
-                Arc::clone(&*rx.borrow())
-            }
-            #[cfg(any(test, feature = "testing"))]
-            CurrentlyManagedZpoolsReceiverInner::FakeDynamic(rx) => {
-                Arc::new(CurrentlyManagedZpools(rx.borrow().clone()))
-            }
-            #[cfg(any(test, feature = "testing"))]
-            CurrentlyManagedZpoolsReceiverInner::FakeStatic(zpools) => {
-                Arc::new(CurrentlyManagedZpools(zpools.clone()))
-            }
-        }
-    }
-
-    pub fn current_and_update(&mut self) -> Arc<CurrentlyManagedZpools> {
-        match &mut self.inner {
-            CurrentlyManagedZpoolsReceiverInner::Real(rx) => {
-                Arc::clone(&*rx.borrow_and_update())
-            }
-            #[cfg(any(test, feature = "testing"))]
-            CurrentlyManagedZpoolsReceiverInner::FakeDynamic(rx) => {
-                Arc::new(CurrentlyManagedZpools(rx.borrow_and_update().clone()))
-            }
-            #[cfg(any(test, feature = "testing"))]
-            CurrentlyManagedZpoolsReceiverInner::FakeStatic(zpools) => {
-                Arc::new(CurrentlyManagedZpools(zpools.clone()))
-            }
-        }
-    }
-
-    /// Wait for changes in the underlying watch channel.
-    ///
-    /// Cancel-safe.
-    pub async fn changed(&mut self) -> Result<(), RecvError> {
-        match &mut self.inner {
-            CurrentlyManagedZpoolsReceiverInner::Real(rx) => rx.changed().await,
-            #[cfg(any(test, feature = "testing"))]
-            CurrentlyManagedZpoolsReceiverInner::FakeDynamic(rx) => {
-                rx.changed().await
-            }
-            #[cfg(any(test, feature = "testing"))]
-            CurrentlyManagedZpoolsReceiverInner::FakeStatic(_) => {
-                // Static set of zpools never changes
-                std::future::pending().await
-            }
-        }
-    }
-}
-
-/// Set of currently managed zpools.
-///
-/// This handle should only be used to decide to _stop_ using a zpool (e.g., if
-/// a previously-launched zone is on a zpool that is no longer managed). It does
-/// not expose a means to list or choose from the currently-managed pools;
-/// instead, consumers should choose mounted datasets.
-///
-/// This level of abstraction even for "when to stop using a zpool" is probably
-/// wrong: if we choose a dataset on which to place a zone's root, we should
-/// shut that zone down if the _dataset_ goes away, not the zpool. For now we
-/// live with "assume the dataset bases we choose stick around as long as their
-/// parent zpool does".
-#[derive(Default, Debug, Clone)]
-pub struct CurrentlyManagedZpools(BTreeSet<ZpoolName>);
-
-impl CurrentlyManagedZpools {
-    /// Returns true if `zpool` is currently managed.
-    pub fn contains(&self, zpool: &ZpoolName) -> bool {
-        self.0.contains(zpool)
-    }
-}
-
 #[derive(Debug)]
 struct LatestReconcilerTaskResultInner {
     sled_config: OmicronSledConfig,