From 3a3e753ca28a383f48eea33673f339f835a98546 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 27 Sep 2024 23:33:07 +0000 Subject: [PATCH] use newtypes to delineate DeviceName/BackendName in PHD 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 --- phd-tests/framework/src/disk/crucible.rs | 31 ++++++++------ phd-tests/framework/src/disk/file.rs | 27 ++++++------ phd-tests/framework/src/disk/in_memory.rs | 24 ++++++----- phd-tests/framework/src/disk/mod.rs | 52 +++++++++++++++++++++-- phd-tests/framework/src/test_vm/config.rs | 29 ++++++------- phd-tests/framework/src/test_vm/spec.rs | 4 +- 6 files changed, 110 insertions(+), 57 deletions(-) diff --git a/phd-tests/framework/src/disk/crucible.rs b/phd-tests/framework/src/disk/crucible.rs index 04d71b595..c8b543450 100644 --- 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, @@ -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 { diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs index 76eb409b6..3fbf05308 100644 --- 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, data_dir: &impl AsRef, guest_os: Option, @@ -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 { diff --git a/phd-tests/framework/src/disk/in_memory.rs b/phd-tests/framework/src/disk/in_memory.rs index 51a57f680..5300c4f68 100644 --- 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, 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, 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 { diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index e3803e3da..35ec84af4 100644 --- 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, 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, DiskError> { diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 14d92fb32..ddfdb42f8 100644 --- 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,14 +331,16 @@ impl<'dr> VmConfig<'dr> { } async fn make_disk<'req>( - name: String, + device_name: String, framework: &Framework, req: &DiskRequest<'req>, ) -> anyhow::Result> { + 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,) @@ -349,7 +348,7 @@ async fn make_disk<'req>( 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, 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 index 841a05b7d..0d25dc271 100644 --- 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