use newtypes to delineate DeviceName/BackendName in PHD

iximeow · iximeow · commit 3a3e753ca28a · 2024-09-28T01:00:46.000Z
these newtypes are useful for other kinds of devices, and in
propolis-api-types generally, but threading that through is more time
than i can spend on this right now
diff --git a/phd-tests/framework/src/disk/crucible.rs b/phd-tests/framework/src/disk/crucible.rs
@@ -21,7 +21,9 @@ use tracing::{error, info};
 use uuid::Uuid;
 
 use super::BlockSize;
-use crate::{guest_os::GuestOsKind, server_log_mode::ServerLogMode};
+use crate::{
+    disk::DeviceName, guest_os::GuestOsKind, server_log_mode::ServerLogMode,
+};
 
 /// An RAII wrapper around a directory containing Crucible data files. Deletes
 /// the directory and its contents when dropped.
@@ -60,8 +62,8 @@ impl Drop for Downstairs {
 /// An RAII wrapper around a Crucible disk.
 #[derive(Debug)]
 pub struct CrucibleDisk {
-    /// The name of the backend to use in instance specs that include this disk.
-    backend_name: String,
+    /// The name to use in instance specs that include this disk.
+    device_name: DeviceName,
 
     /// The UUID to insert into this disk's `VolumeConstructionRequest`s.
     id: Uuid,
@@ -97,7 +99,7 @@ impl CrucibleDisk {
     /// `data_dir`.
     #[allow(clippy::too_many_arguments)]
     pub(crate) fn new(
-        backend_name: String,
+        device_name: DeviceName,
         min_disk_size_gib: u64,
         block_size: BlockSize,
         downstairs_binary_path: &impl AsRef<std::ffi::OsStr>,
@@ -251,7 +253,7 @@ impl CrucibleDisk {
         }
 
         Ok(Self {
-            backend_name,
+            device_name,
             id: disk_uuid,
             block_size,
             blocks_per_extent,
@@ -280,7 +282,11 @@ impl CrucibleDisk {
 }
 
 impl super::DiskConfig for CrucibleDisk {
-    fn backend_spec(&self) -> (String, StorageBackendV0) {
+    fn device_name(&self) -> &DeviceName {
+        &self.device_name
+    }
+
+    fn backend_spec(&self) -> StorageBackendV0 {
         let gen = self.generation.load(Ordering::Relaxed);
         let downstairs_addrs = self
             .downstairs_instances
@@ -318,14 +324,11 @@ impl super::DiskConfig for CrucibleDisk {
             }),
         };
 
-        (
-            self.backend_name.clone(),
-            StorageBackendV0::Crucible(CrucibleStorageBackend {
-                request_json: serde_json::to_string(&vcr)
-                    .expect("VolumeConstructionRequest should serialize"),
-                readonly: false,
-            }),
-        )
+        StorageBackendV0::Crucible(CrucibleStorageBackend {
+            request_json: serde_json::to_string(&vcr)
+                .expect("VolumeConstructionRequest should serialize"),
+            readonly: false,
+        })
     }
 
     fn guest_os(&self) -> Option<GuestOsKind> {
diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs
@@ -9,7 +9,9 @@ use propolis_client::types::{FileStorageBackend, StorageBackendV0};
 use tracing::{debug, error, warn};
 use uuid::Uuid;
 
-use crate::{guest_os::GuestOsKind, zfs::ClonedFile as ZfsClonedFile};
+use crate::{
+    disk::DeviceName, guest_os::GuestOsKind, zfs::ClonedFile as ZfsClonedFile,
+};
 
 /// Describes the method used to create the backing file for a file-backed disk.
 #[derive(Debug)]
@@ -80,7 +82,7 @@ impl Drop for BackingFile {
 #[derive(Debug)]
 pub struct FileBackedDisk {
     /// The name to use for instance spec backends that refer to this disk.
-    backend_name: String,
+    device_name: DeviceName,
 
     /// The backing file for this disk.
     file: BackingFile,
@@ -94,7 +96,7 @@ impl FileBackedDisk {
     /// Creates a new file-backed disk whose initial contents are copied from
     /// the specified artifact on the host file system.
     pub(crate) fn new_from_artifact(
-        backend_name: String,
+        device_name: DeviceName,
         artifact_path: &impl AsRef<Utf8Path>,
         data_dir: &impl AsRef<Utf8Path>,
         guest_os: Option<GuestOsKind>,
@@ -116,19 +118,20 @@ impl FileBackedDisk {
         permissions.set_readonly(false);
         disk_file.set_permissions(permissions)?;
 
-        Ok(Self { backend_name, file: artifact, guest_os })
+        Ok(Self { device_name, file: artifact, guest_os })
     }
 }
 
 impl super::DiskConfig for FileBackedDisk {
-    fn backend_spec(&self) -> (String, StorageBackendV0) {
-        (
-            self.backend_name.clone(),
-            StorageBackendV0::File(FileStorageBackend {
-                path: self.file.path().to_string(),
-                readonly: false,
-            }),
-        )
+    fn device_name(&self) -> &DeviceName {
+        &self.device_name
+    }
+
+    fn backend_spec(&self) -> StorageBackendV0 {
+        StorageBackendV0::File(FileStorageBackend {
+            path: self.file.path().to_string(),
+            readonly: false,
+        })
     }
 
     fn guest_os(&self) -> Option<GuestOsKind> {
diff --git a/phd-tests/framework/src/disk/in_memory.rs b/phd-tests/framework/src/disk/in_memory.rs
@@ -7,11 +7,12 @@
 use propolis_client::types::{BlobStorageBackend, StorageBackendV0};
 
 use super::DiskConfig;
+use crate::disk::DeviceName;
 
 /// A disk with an in-memory backend.
 #[derive(Debug)]
 pub struct InMemoryDisk {
-    backend_name: String,
+    device_name: DeviceName,
     contents: Vec<u8>,
     readonly: bool,
 }
@@ -20,28 +21,29 @@ impl InMemoryDisk {
     /// Creates an in-memory backend that will present the supplied `contents`
     /// to the guest.
     pub fn new(
-        backend_name: String,
+        device_name: DeviceName,
         contents: Vec<u8>,
         readonly: bool,
     ) -> Self {
-        Self { backend_name, contents, readonly }
+        Self { device_name, contents, readonly }
     }
 }
 
 impl DiskConfig for InMemoryDisk {
-    fn backend_spec(&self) -> (String, StorageBackendV0) {
+    fn device_name(&self) -> &DeviceName {
+        &self.device_name
+    }
+
+    fn backend_spec(&self) -> StorageBackendV0 {
         let base64 = base64::Engine::encode(
             &base64::engine::general_purpose::STANDARD,
             self.contents.as_slice(),
         );
 
-        (
-            self.backend_name.clone(),
-            StorageBackendV0::Blob(BlobStorageBackend {
-                base64,
-                readonly: self.readonly,
-            }),
-        )
+        StorageBackendV0::Blob(BlobStorageBackend {
+            base64,
+            readonly: self.readonly,
+        })
     }
 
     fn guest_os(&self) -> Option<crate::guest_os::GuestOsKind> {
diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs
@@ -67,10 +67,54 @@ impl BlockSize {
     }
 }
 
+/// The name for the device implementing a disk. This is the name provided for a
+/// disk in constructing a VM spec for PHD tests. The disk by this name likely
+/// also has a [`BackendName`] derived from this device name.
+///
+/// TODO: This exists largely to ensure that PHD matches the same spec
+/// construction conventions as `propolis-server` when handling API requests: it
+/// is another piece of glue that could reasonably be deleted if/when PHD and
+/// sled-agent use the same code to build InstanceEnsureRequest. Until then,
+/// carefully match the relationship between names with these newtypes.
+///
+/// Alternatively, `DeviceName` and `BackendName` could be pulled into
+/// `propolis-api-types`.
+#[derive(Clone, Debug)]
+pub struct DeviceName(String);
+
+impl DeviceName {
+    pub fn new(name: String) -> Self {
+        DeviceName(name)
+    }
+
+    pub fn into_backend_name(self) -> BackendName {
+        // This must match `api_request.rs`' `parse_disk_from_request`.
+        BackendName(format!("{}-backend", self.0))
+    }
+
+    pub fn into_string(self) -> String {
+        self.0
+    }
+}
+
+/// The name for a backend implementing storage for a disk. This is derived
+/// exclusively from a corresponding [`DeviceName`].
+#[derive(Clone, Debug)]
+pub struct BackendName(String);
+
+impl BackendName {
+    pub fn into_string(self) -> String {
+        self.0
+    }
+}
+
 /// A trait for functions exposed by all disk backends (files, Crucible, etc.).
 pub trait DiskConfig: std::fmt::Debug + Send + Sync {
+    /// Yields the device name for this disk.
+    fn device_name(&self) -> &DeviceName;
+
     /// Yields the backend spec for this disk's storage backend.
-    fn backend_spec(&self) -> (String, StorageBackendV0);
+    fn backend_spec(&self) -> StorageBackendV0;
 
     /// Yields the guest OS kind of the guest image the disk was created from,
     /// or `None` if the disk was not created from a guest image.
@@ -182,7 +226,7 @@ impl DiskFactory {
     /// by `source`.
     pub(crate) async fn create_file_backed_disk<'d>(
         &self,
-        name: String,
+        name: DeviceName,
         source: &DiskSource<'d>,
     ) -> Result<Arc<FileBackedDisk>, DiskError> {
         let artifact_name = match source {
@@ -226,7 +270,7 @@ impl DiskFactory {
     /// - block_size: The disk's block size.
     pub(crate) async fn create_crucible_disk<'d>(
         &self,
-        name: String,
+        name: DeviceName,
         source: &DiskSource<'d>,
         mut min_disk_size_gib: u64,
         block_size: BlockSize,
@@ -278,7 +322,7 @@ impl DiskFactory {
 
     pub(crate) async fn create_in_memory_disk<'d>(
         &self,
-        name: String,
+        name: DeviceName,
         source: &DiskSource<'d>,
         readonly: bool,
     ) -> Result<Arc<InMemoryDisk>, DiskError> {
diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs
@@ -16,7 +16,7 @@ use propolis_client::{
 use uuid::Uuid;
 
 use crate::{
-    disk::{DiskConfig, DiskSource},
+    disk::{DeviceName, DiskConfig, DiskSource},
     test_vm::spec::VmSpec,
     Framework,
 };
@@ -268,30 +268,27 @@ impl<'dr> VmConfig<'dr> {
         let all_disks = self.disks.iter().zip(disk_handles.iter());
         for (req, hdl) in all_disks {
             let pci_path = PciPath::new(0, req.pci_device_num, 0).unwrap();
-            let (device_name, backend_spec) = hdl.backend_spec();
-            // propolis-server uses a disk's name from the API as its device
-            // name. It then derives a backend name from the disk name. Match
-            // that name derivation here so PHD tests match the API as used by
-            // Nexus.
-            let backend_name = format!("{}-backend", device_name);
+            let backend_spec = hdl.backend_spec();
+            let device_name = hdl.device_name().clone();
+            let backend_name = device_name.clone().into_backend_name();
             let device_spec = match req.interface {
                 DiskInterface::Virtio => {
                     StorageDeviceV0::VirtioDisk(VirtioDisk {
-                        backend_name: backend_name.clone(),
+                        backend_name: backend_name.clone().into_string(),
                         pci_path,
                     })
                 }
                 DiskInterface::Nvme => StorageDeviceV0::NvmeDisk(NvmeDisk {
-                    backend_name: backend_name.clone(),
+                    backend_name: backend_name.clone().into_string(),
                     pci_path,
                 }),
             };
 
             spec_builder
                 .add_storage_device(
-                    device_name,
+                    device_name.into_string(),
                     device_spec,
-                    backend_name,
+                    backend_name.into_string(),
                     backend_spec,
                 )
                 .context("adding storage device to spec")?;
@@ -334,22 +331,24 @@ impl<'dr> VmConfig<'dr> {
 }
 
 async fn make_disk<'req>(
-    name: String,
+    device_name: String,
     framework: &Framework,
     req: &DiskRequest<'req>,
 ) -> anyhow::Result<Arc<dyn DiskConfig>> {
+    let device_name = DeviceName::new(device_name);
+
     Ok(match req.backend {
         DiskBackend::File => framework
             .disk_factory
-            .create_file_backed_disk(name, &req.source)
+            .create_file_backed_disk(device_name, &req.source)
             .await
             .with_context(|| {
                 format!("creating new file-backed disk from {:?}", req.source,)
             })? as Arc<dyn DiskConfig>,
         DiskBackend::Crucible { min_disk_size_gib, block_size } => framework
             .disk_factory
             .create_crucible_disk(
-                name,
+                device_name,
                 &req.source,
                 min_disk_size_gib,
                 block_size,
@@ -364,7 +363,7 @@ async fn make_disk<'req>(
             as Arc<dyn DiskConfig>,
         DiskBackend::InMemory { readonly } => framework
             .disk_factory
-            .create_in_memory_disk(name, &req.source, readonly)
+            .create_in_memory_disk(device_name, &req.source, readonly)
             .await
             .with_context(|| {
                 format!("creating new in-memory disk from {:?}", req.source)
diff --git a/phd-tests/framework/src/test_vm/spec.rs b/phd-tests/framework/src/test_vm/spec.rs
@@ -46,7 +46,9 @@ impl VmSpec {
                 continue;
             };
 
-            let (backend_name, backend_spec) = disk.backend_spec();
+            let backend_spec = disk.backend_spec();
+            let backend_name =
+                disk.device_name().clone().into_backend_name().into_string();
             match self
                 .instance_spec
                 .backends
