From 4da775b0e0a76dd0dd0742326b3a87082417fa19 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 19 Nov 2024 00:19:09 +0000 Subject: [PATCH] convert in-repo clients to use SpecKey --- bin/propolis-cli/src/main.rs | 50 ++++++------ crates/propolis-config-toml/src/spec.rs | 47 +++++------ lib/propolis-client/src/lib.rs | 7 +- openapi/propolis-server.json | 95 ++++++++++++++++------- phd-tests/framework/src/test_vm/config.rs | 14 +++- 5 files changed, 133 insertions(+), 80 deletions(-) diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 543bb6b82..5b9735658 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -23,7 +23,7 @@ use propolis_client::types::{ QemuPvpanic, ReplacementComponent, SerialPort, SerialPortNumber, VirtioDisk, }; -use propolis_client::PciPath; +use propolis_client::{PciPath, SpecKey}; use propolis_config_toml::spec::SpecConfig; use serde::{Deserialize, Serialize}; use slog::{o, Drain, Level, Logger}; @@ -192,11 +192,11 @@ struct VmConfig { fn add_component_to_spec( spec: &mut InstanceSpecV0, - name: String, + id: SpecKey, component: ComponentV0, ) -> anyhow::Result<()> { - if spec.components.insert(name.clone(), component).is_some() { - anyhow::bail!("duplicate component ID {name:?}"); + if spec.components.insert(id.to_string(), component).is_some() { + anyhow::bail!("duplicate component ID {id:?}"); } Ok(()) } @@ -214,9 +214,9 @@ struct DiskRequest { #[derive(Clone, Debug)] struct ParsedDiskRequest { - device_name: String, + device_id: SpecKey, device_spec: ComponentV0, - backend_name: String, + backend_id: SpecKey, backend_spec: CrucibleStorageBackend, } @@ -229,17 +229,17 @@ impl DiskRequest { } let slot = self.slot + 0x10; - let backend_name = format!("{}-backend", self.name); + let backend_id = SpecKey::Name(format!("{}-backend", self.name)); let pci_path = PciPath::new(0, slot, 0).with_context(|| { format!("processing disk request {:?}", self.name) })?; let device_spec = match self.device.as_ref() { "virtio" => ComponentV0::VirtioDisk(VirtioDisk { - backend_name: backend_name.clone(), + backend_id: backend_id.clone(), pci_path, }), "nvme" => ComponentV0::NvmeDisk(NvmeDisk { - backend_name: backend_name.clone(), + backend_id: backend_id.clone(), pci_path, }), _ => anyhow::bail!( @@ -256,9 +256,9 @@ impl DiskRequest { }; Ok(ParsedDiskRequest { - device_name: self.name.clone(), + device_id: SpecKey::Name(self.name.clone()), device_spec, - backend_name, + backend_id, backend_spec, }) } @@ -314,15 +314,15 @@ impl VmConfig { .flatten() { let ParsedDiskRequest { - device_name, + device_id, device_spec, - backend_name, + backend_id, backend_spec, } = disk_request.parse()?; - add_component_to_spec(&mut spec, device_name, device_spec)?; + add_component_to_spec(&mut spec, device_id, device_spec)?; add_component_to_spec( &mut spec, - backend_name, + backend_id, ComponentV0::CrucibleStorageBackend(backend_spec), )?; } @@ -338,16 +338,18 @@ impl VmConfig { add_component_to_spec( &mut spec, - CLOUD_INIT_NAME.to_owned(), + SpecKey::Name(CLOUD_INIT_NAME.to_owned()), ComponentV0::VirtioDisk(VirtioDisk { - backend_name: CLOUD_INIT_BACKEND_NAME.to_owned(), + backend_id: SpecKey::Name( + CLOUD_INIT_BACKEND_NAME.to_owned(), + ), pci_path: PciPath::new(0, 0x18, 0).unwrap(), }), )?; add_component_to_spec( &mut spec, - CLOUD_INIT_BACKEND_NAME.to_owned(), + SpecKey::Name(CLOUD_INIT_BACKEND_NAME.to_owned()), ComponentV0::BlobStorageBackend(BlobStorageBackend { base64: bytes, readonly: true, @@ -362,7 +364,7 @@ impl VmConfig { ] { add_component_to_spec( &mut spec, - name.to_owned(), + SpecKey::Name(name.to_owned()), ComponentV0::SerialPort(SerialPort { num: port }), )?; } @@ -375,7 +377,7 @@ impl VmConfig { { add_component_to_spec( &mut spec, - "com4".to_owned(), + SpecKey::Name("com4".to_owned()), ComponentV0::SerialPort(SerialPort { num: SerialPortNumber::Com4, }), @@ -384,7 +386,7 @@ impl VmConfig { add_component_to_spec( &mut spec, - "pvpanic".to_owned(), + SpecKey::Name("pvpanic".to_owned()), ComponentV0::QemuPvpanic(QemuPvpanic { enable_isa: true }), )?; @@ -725,17 +727,17 @@ async fn migrate_instance( let mut replace_components = HashMap::new(); for disk in disks { - let ParsedDiskRequest { backend_name, backend_spec, .. } = + let ParsedDiskRequest { backend_id, backend_spec, .. } = disk.parse()?; let old = replace_components.insert( - backend_name.clone(), + backend_id.to_string(), ReplacementComponent::CrucibleStorageBackend(backend_spec), ); if old.is_some() { anyhow::bail!( - "duplicate backend name {backend_name} in replacement disk \ + "duplicate backend name {backend_id} in replacement disk \ list" ); } diff --git a/crates/propolis-config-toml/src/spec.rs b/crates/propolis-config-toml/src/spec.rs index 73584afdf..07954c326 100644 --- a/crates/propolis-config-toml/src/spec.rs +++ b/crates/propolis-config-toml/src/spec.rs @@ -16,14 +16,12 @@ use propolis_client::{ SoftNpuPciPort, SoftNpuPort, VirtioDisk, VirtioNetworkBackend, VirtioNic, }, - PciPath, + PciPath, SpecKey, }; use thiserror::Error; pub const MIGRATION_FAILURE_DEVICE_NAME: &str = "test-migration-failure"; -type SpecKey = String; - #[derive(Debug, Error)] pub enum TomlToSpecError { #[error("unrecognized device type {0:?}")] @@ -111,7 +109,7 @@ impl TryFrom<&super::Config> for SpecConfig { .max(0) as u32; spec.components.insert( - MIGRATION_FAILURE_DEVICE_NAME.to_owned(), + SpecKey::Name(MIGRATION_FAILURE_DEVICE_NAME.to_owned()), ComponentV0::MigrationFailureInjector( MigrationFailureInjector { fail_exports, fail_imports }, ), @@ -177,13 +175,14 @@ impl TryFrom<&super::Config> for SpecConfig { TomlToSpecError::NoVnicName(device_name.to_owned()) })?; - let backend_name = format!("{}:backend", device_id); + let backend_name = + SpecKey::Name(format!("{device_id}:backend")); spec.components.insert( device_id, ComponentV0::SoftNpuPort(SoftNpuPort { link_name: device_name.to_string(), - backend_name: backend_name.clone(), + backend_id: backend_name.clone(), }), ); @@ -234,7 +233,7 @@ impl TryFrom<&super::Config> for SpecConfig { })?; spec.components.insert( - format!("pci-bridge-{}", bridge.pci_path), + SpecKey::Name(format!("pci-bridge-{}", bridge.pci_path)), ComponentV0::PciPciBridge(PciPciBridge { downstream_bus: bridge.downstream_bus, pci_path, @@ -266,30 +265,32 @@ fn parse_storage_device_from_config( } }; - let backend_name = device - .options - .get("block_dev") - .ok_or_else(|| { - TomlToSpecError::NoBackendNameForStorageDevice(name.to_owned()) - })? - .as_str() - .ok_or_else(|| { - TomlToSpecError::NoBackendNameForStorageDevice(name.to_owned()) - })? - .to_string(); + let backend_id = SpecKey::from_str( + device + .options + .get("block_dev") + .ok_or_else(|| { + TomlToSpecError::NoBackendNameForStorageDevice(name.to_owned()) + })? + .as_str() + .ok_or_else(|| { + TomlToSpecError::NoBackendNameForStorageDevice(name.to_owned()) + })?, + ) + .expect("SpecKey::from_str is infallible"); let pci_path: PciPath = device .get("pci-path") .ok_or_else(|| TomlToSpecError::InvalidPciPath(name.to_owned()))?; - let id_to_return = backend_name.clone(); + let id_to_return = backend_id.clone(); Ok(( match interface { Interface::Virtio => { - ComponentV0::VirtioDisk(VirtioDisk { backend_name, pci_path }) + ComponentV0::VirtioDisk(VirtioDisk { backend_id, pci_path }) } Interface::Nvme => { - ComponentV0::NvmeDisk(NvmeDisk { backend_name, pci_path }) + ComponentV0::NvmeDisk(NvmeDisk { backend_id, pci_path }) } }, id_to_return, @@ -356,10 +357,10 @@ fn parse_network_device_from_config( .get("pci-path") .ok_or_else(|| TomlToSpecError::InvalidPciPath(name.to_owned()))?; - let backend_id = format!("{name}-backend"); + let backend_id = SpecKey::Name(format!("{name}-backend")); Ok(ParsedNic { device_spec: VirtioNic { - backend_name: backend_id.clone(), + backend_id: backend_id.clone(), interface_id: uuid::Uuid::nil(), pci_path, }, diff --git a/lib/propolis-client/src/lib.rs b/lib/propolis-client/src/lib.rs index 2ec411af6..7aa803093 100644 --- a/lib/propolis-client/src/lib.rs +++ b/lib/propolis-client/src/lib.rs @@ -4,9 +4,9 @@ //! A client for the Propolis hypervisor frontend's server API. -// Re-export the PCI path type from propolis_api_types so that users can access -// its constructor and From impls. -pub use propolis_api_types::instance_spec::PciPath; +// Re-export types from propolis_api_types where callers may want to use +// constructors or From impls. +pub use propolis_api_types::instance_spec::{PciPath, SpecKey}; // Re-export Crucible client types that appear in their serialized forms in // instance specs. This allows clients to ensure they serialize/deserialize @@ -20,6 +20,7 @@ progenitor::generate_api!( tags = Separate, replace = { PciPath = crate::PciPath, + SpecKey = crate::SpecKey, }, patch = { BootOrderEntry = { derives = [schemars::JsonSchema] }, diff --git a/openapi/propolis-server.json b/openapi/propolis-server.json index ff27cd36b..7ea2e6f77 100644 --- a/openapi/propolis-server.json +++ b/openapi/propolis-server.json @@ -74,8 +74,7 @@ "name": "id", "required": true, "schema": { - "type": "string", - "format": "uuid" + "type": "string" } }, { @@ -122,8 +121,7 @@ "name": "id", "required": true, "schema": { - "type": "string", - "format": "uuid" + "type": "string" } } ], @@ -157,8 +155,7 @@ "name": "id", "required": true, "schema": { - "type": "string", - "format": "uuid" + "type": "string" } } ], @@ -490,13 +487,17 @@ "description": "An entry in the boot order stored in a [`BootSettings`] component.", "type": "object", "properties": { - "name": { - "description": "The name of another component in the spec that Propolis should try to boot from.\n\nCurrently, only disk device components are supported.", - "type": "string" + "id": { + "description": "The ID of another component in the spec that Propolis should try to boot from.\n\nCurrently, only disk device components are supported.", + "allOf": [ + { + "$ref": "#/components/schemas/SpecKey" + } + ] } }, "required": [ - "name" + "id" ] }, "BootSettings": { @@ -1447,15 +1448,11 @@ "InstanceVCRReplace": { "type": "object", "properties": { - "name": { - "type": "string" - }, "vcr_json": { "type": "string" } }, "required": [ - "name", "vcr_json" ] }, @@ -1501,9 +1498,13 @@ "description": "A disk that presents an NVMe interface to the guest.", "type": "object", "properties": { - "backend_name": { + "backend_id": { "description": "The name of the disk's backend component.", - "type": "string" + "allOf": [ + { + "$ref": "#/components/schemas/SpecKey" + } + ] }, "pci_path": { "description": "The PCI bus/device/function at which this disk should be attached.", @@ -1515,7 +1516,7 @@ } }, "required": [ - "backend_name", + "backend_id", "pci_path" ], "additionalProperties": false @@ -1759,9 +1760,13 @@ "description": "Describes a port in a SoftNPU emulated ASIC.\n\nThis is only supported by Propolis servers compiled with the `falcon` feature.", "type": "object", "properties": { - "backend_name": { + "backend_id": { "description": "The name of the port's associated DLPI backend.", - "type": "string" + "allOf": [ + { + "$ref": "#/components/schemas/SpecKey" + } + ] }, "link_name": { "description": "The data link name for this port.", @@ -1769,11 +1774,41 @@ } }, "required": [ - "backend_name", + "backend_id", "link_name" ], "additionalProperties": false }, + "SpecKey": { + "description": "A key identifying a component in an instance spec.", + "oneOf": [ + { + "type": "object", + "properties": { + "Uuid": { + "type": "string", + "format": "uuid" + } + }, + "required": [ + "Uuid" + ], + "additionalProperties": false + }, + { + "type": "object", + "properties": { + "Name": { + "type": "string" + } + }, + "required": [ + "Name" + ], + "additionalProperties": false + } + ] + }, "VersionedInstanceSpec": { "description": "A versioned instance spec.", "oneOf": [ @@ -1802,9 +1837,13 @@ "description": "A disk that presents a virtio-block interface to the guest.", "type": "object", "properties": { - "backend_name": { + "backend_id": { "description": "The name of the disk's backend component.", - "type": "string" + "allOf": [ + { + "$ref": "#/components/schemas/SpecKey" + } + ] }, "pci_path": { "description": "The PCI bus/device/function at which this disk should be attached.", @@ -1816,7 +1855,7 @@ } }, "required": [ - "backend_name", + "backend_id", "pci_path" ], "additionalProperties": false @@ -1839,9 +1878,13 @@ "description": "A network card that presents a virtio-net interface to the guest.", "type": "object", "properties": { - "backend_name": { + "backend_id": { "description": "The name of the device's backend.", - "type": "string" + "allOf": [ + { + "$ref": "#/components/schemas/SpecKey" + } + ] }, "interface_id": { "description": "A caller-defined correlation identifier for this interface. If Propolis is configured to collect network interface kstats in its Oximeter metrics, the metric series for this interface will be associated with this identifier.", @@ -1858,7 +1901,7 @@ } }, "required": [ - "backend_name", + "backend_id", "interface_id", "pci_path" ], diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 9c3497dbd..1ea5ec14b 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -13,7 +13,7 @@ use propolis_client::{ MigrationFailureInjector, NvmeDisk, SerialPort, SerialPortNumber, VirtioDisk, }, - PciPath, + PciPath, SpecKey, }; use uuid::Uuid; @@ -303,11 +303,15 @@ impl<'dr> VmConfig<'dr> { let backend_name = device_name.clone().into_backend_name(); let device_spec = match req.interface { DiskInterface::Virtio => ComponentV0::VirtioDisk(VirtioDisk { - backend_name: backend_name.clone().into_string(), + backend_id: SpecKey::from( + backend_name.clone().into_string(), + ), pci_path, }), DiskInterface::Nvme => ComponentV0::NvmeDisk(NvmeDisk { - backend_name: backend_name.clone().into_string(), + backend_id: SpecKey::from( + backend_name.clone().into_string(), + ), pci_path, }), }; @@ -333,7 +337,9 @@ impl<'dr> VmConfig<'dr> { ComponentV0::BootSettings(BootSettings { order: boot_order .iter() - .map(|item| BootOrderEntry { name: item.to_string() }) + .map(|item| BootOrderEntry { + id: SpecKey::from(item.to_string()), + }) .collect(), }), );