From 79aa27ed70b18933d9849863ec63c791a94be78b Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 15 Nov 2024 23:38:11 +0000 Subject: [PATCH 1/7] define SpecKey type Instead of using Strings as keys in instance specs, define a SpecKey type that is a union of a UUID and a String. This type serializes as a string, and its `FromStr` and `From` impls will try to parse strings as UUIDs before storing keys in string format. This allows the control plane to use UUIDs wherever it makes sense while allowing it (and other users) to continue to use strings where a UUID component ID is unavailable or is being used by some other spec element (as might be the case for a disk device and disk backend). Index most objects in propolis-server using SpecKeys instead of Strings. This is mostly just a rote change-the-types, make-it-compile exercise, but it extends to Crucible backends, which deserve some additional commentary. In the old ensure API, sending a `DiskRequest` would register disk components with several different identifiers: - The disk's name, given by the `DiskRequest`'s `name` field, is used - as a key in the VM's `DeviceMap`, - as the disk device component's key in the VM's instance spec, and - to generate the disk backend component's key in the VM's instance spec. - The disk's ID, given as the `id` in the `Volume`-variant `VolumeConstructionRequest` in the `DiskRequest`, is reported by the Crucible backend's `get_uuid` function and used - to register a metrics producer for each disk, and - as the key in the VM's `CrucibleBackendMap`. Now that all new VMs are started using instance specs, use component keys for everything: - Entities in the `DeviceMap` are identified by their corresponding components' `SpecKey`s. - Crucible backends in the `CrucibleBackendMap` are also identified by their `SpecKey`s. - APIs that take a Crucible backend ID on their path now take a String and not a UUID. The server converts these to a SpecKey before looking up backends in the map. - When a Crucible backend is created, the machine initializer checks to see if its `SpecKey` was a `Uuid`. If so, it will register the backend to produce metrics. The intention is that Omicron will use disk IDs as the keys for its `CrucibleStorageBackend` components in the specs it generates. It can then also use these IDs as parameters when requesting snapshots or VCR replacements. The friendly names users give their Omicron disks no longer appear in the Propolis API at all. --- Cargo.lock | 9 +- Cargo.toml | 1 + bin/propolis-server/src/lib/initializer.rs | 171 ++++++++++-------- .../src/lib/migrate/destination.rs | 18 +- .../src/lib/migrate/preamble.rs | 9 +- bin/propolis-server/src/lib/server.rs | 38 ++-- .../src/lib/spec/api_spec_v0.rs | 113 ++++++------ bin/propolis-server/src/lib/spec/builder.rs | 143 ++++++++------- bin/propolis-server/src/lib/spec/mod.rs | 36 ++-- bin/propolis-server/src/lib/vm/active.rs | 8 +- bin/propolis-server/src/lib/vm/mod.rs | 15 +- bin/propolis-server/src/lib/vm/objects.rs | 21 ++- .../src/lib/vm/request_queue.rs | 14 +- .../src/lib/vm/state_driver.rs | 51 +++--- crates/propolis-api-types/Cargo.toml | 1 + .../src/instance_spec/components/devices.rs | 16 +- .../src/instance_spec/mod.rs | 84 ++++++++- .../src/instance_spec/v0.rs | 4 +- crates/propolis-api-types/src/lib.rs | 11 +- 19 files changed, 440 insertions(+), 323 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 30c378b60..e6a2d2109 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4478,6 +4478,7 @@ dependencies = [ "propolis_types", "schemars", "serde", + "serde_with", "thiserror", "uuid", ] @@ -5383,9 +5384,9 @@ dependencies = [ [[package]] name = "serde_with" -version = "3.9.0" +version = "3.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "69cecfa94848272156ea67b2b1a53f20fc7bc638c4a46d2f8abde08f05f4b857" +checksum = "8e28bdad6db2b8340e449f7108f020b3b092e8583a9e3fb82713e1d4e71fe817" dependencies = [ "base64 0.22.1", "chrono", @@ -5401,9 +5402,9 @@ dependencies = [ [[package]] name = "serde_with_macros" -version = "3.9.0" +version = "3.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a8fee4991ef4f274617a51ad4af30519438dacb2f56ac773b08a1922ff743350" +checksum = "9d846214a9854ef724f3da161b426242d8de7c1fc7de2f89bb1efcb154dca79d" dependencies = [ "darling", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index 79e6de9ad..a5692f767 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -148,6 +148,7 @@ serde_arrays = "0.1" serde_derive = "1.0" serde_json = "1.0" serde_test = "1.0.138" +serde_with = "3.11.0" slog = "2.7" slog-async = "2.8" slog-bunyan = "2.4.0" diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 5bb3213ee..eab11ccb2 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -44,14 +44,13 @@ use propolis::hw::uart::LpcUart; use propolis::hw::{nvme, virtio}; use propolis::intr_pins; use propolis::vmm::{self, Builder, Machine}; -use propolis_api_types::instance_spec; use propolis_api_types::instance_spec::components::devices::SerialPortNumber; +use propolis_api_types::instance_spec::{self, SpecKey}; use propolis_api_types::InstanceProperties; use propolis_types::{CpuidIdent, CpuidVendor}; use slog::info; use strum::IntoEnumIterator; use thiserror::Error; -use uuid::Uuid; /// An error that can arise while initializing a new machine. #[derive(Debug, Error)] @@ -83,14 +82,14 @@ pub enum MachineInitError { #[error("failed to decode in-memory storage backend contents")] InMemoryBackendDecodeFailed(#[from] base64::DecodeError), - #[error("multiple Crucible disks with ID {0}")] - DuplicateCrucibleBackendId(Uuid), + #[error("multiple Crucible disks with backend ID {0}")] + DuplicateCrucibleBackendId(SpecKey), #[error("boot order entry {0:?} does not refer to an attached disk")] - BootOrderEntryWithoutDevice(String), + BootOrderEntryWithoutDevice(SpecKey), #[error("boot entry {0:?} refers to a device on non-zero PCI bus {1}")] - BootDeviceOnDownstreamPciBus(String, u8), + BootDeviceOnDownstreamPciBus(SpecKey, u8), #[error("failed to insert {0} fwcfg entry")] FwcfgInsertFailed(&'static str, #[source] fwcfg::InsertError), @@ -171,7 +170,7 @@ impl RegisteredChipset { struct StorageBackendInstance { be: Arc, - crucible: Option<(uuid::Uuid, Arc)>, + crucible: Option>, } #[derive(Default)] @@ -266,7 +265,8 @@ impl<'a> MachineInitializer<'a> { pub fn initialize_hpet(&mut self) { let hpet = BhyveHpet::create(self.machine.hdl.clone()); - self.devices.insert(hpet.type_name().into(), hpet.clone()); + self.devices + .insert(SpecKey::Name(hpet.type_name().into()), hpet.clone()); } pub fn initialize_chipset( @@ -342,20 +342,31 @@ impl<'a> MachineInitializer<'a> { do_pci_attach(i440fx::DEFAULT_PM_BDF, chipset_pm.clone()); chipset_pm.attach(&self.machine.bus_pio); - self.devices - .insert(chipset_hb.type_name().into(), chipset_hb.clone()); self.devices.insert( - chipset_lpc.type_name().into(), + SpecKey::Name(chipset_hb.type_name().into()), + chipset_hb.clone(), + ); + self.devices.insert( + SpecKey::Name(chipset_lpc.type_name().into()), chipset_lpc.clone(), ); - self.devices.insert(chipset_pm.type_name().into(), chipset_pm); + self.devices.insert( + SpecKey::Name(chipset_pm.type_name().into()), + chipset_pm, + ); // Record attachment for any bridges in PCI topology too for (bdf, bridge) in bridges { - self.devices.insert( - format!("{}-{bdf}", bridge.type_name()), - bridge, - ); + let spec_element = self + .spec + .pci_pci_bridges + .iter() + .find(|(_, spec_bridge)| { + bdf == spec_bridge.pci_path.into() + }) + .expect("all PCI bridges are in the topology"); + + self.devices.insert(spec_element.0.clone(), bridge); } Ok(RegisteredChipset { chipset: chipset_hb, isa: chipset_lpc }) @@ -407,7 +418,10 @@ impl<'a> MachineInitializer<'a> { chipset.irq_pin(ibmpc::IRQ_PS2_AUX).unwrap(), chipset.reset_pin(), ); - self.devices.insert(ps2_ctrl.type_name().into(), ps2_ctrl.clone()); + self.devices.insert( + SpecKey::Name(ps2_ctrl.type_name().into()), + ps2_ctrl.clone(), + ); ps2_ctrl } @@ -421,7 +435,7 @@ impl<'a> MachineInitializer<'a> { let poller = chardev::BlockingFileOutput::new(debug_file); poller.attach(Arc::clone(&dbg) as Arc); - self.devices.insert(dbg.type_name().into(), dbg); + self.devices.insert(SpecKey::Name(dbg.type_name().into()), dbg); Ok(()) } @@ -432,17 +446,16 @@ impl<'a> MachineInitializer<'a> { ) -> Result<(), MachineInitError> { if let Some(pvpanic) = &self.spec.pvpanic { if pvpanic.spec.enable_isa { - let pvpanic = QemuPvpanic::create( + let device = QemuPvpanic::create( self.log.new(slog::o!("dev" => "qemu-pvpanic")), ); - pvpanic.attach_pio(&self.machine.bus_pio); - self.devices - .insert(pvpanic.type_name().into(), pvpanic.clone()); + device.attach_pio(&self.machine.bus_pio); + self.devices.insert(pvpanic.id.clone(), device.clone()); if let Some(ref registry) = self.producer_registry { let producer = crate::stats::PvpanicProducer::new( virtual_machine, - pvpanic, + device, ); registry.register_producer(producer).context( "failed to register PVPANIC Oximeter producer", @@ -457,13 +470,13 @@ impl<'a> MachineInitializer<'a> { async fn create_storage_backend_from_spec( &self, backend_spec: &StorageBackend, - backend_name: &str, + backend_id: &SpecKey, nexus_client: &Option, ) -> Result { match backend_spec { StorageBackend::Crucible(spec) => { info!(self.log, "Creating Crucible disk"; - "backend_name" => backend_name); + "backend_id" => %backend_id); let vcr: VolumeConstructionRequest = serde_json::from_str(&spec.request_json) @@ -497,13 +510,7 @@ impl<'a> MachineInitializer<'a> { .await .context("failed to create Crucible backend")?; - let crucible = Some(( - be.get_uuid() - .await - .context("failed to get Crucible backend ID")?, - be.clone(), - )); - + let crucible = Some(be.clone()); Ok(StorageBackendInstance { be, crucible }) } StorageBackend::File(spec) => { @@ -586,22 +593,22 @@ impl<'a> MachineInitializer<'a> { Nvme, } - for (disk_name, disk) in &self.spec.disks { + for (device_id, disk) in &self.spec.disks { info!( self.log, "Creating storage device"; - "name" => disk_name, + "device_id" => %device_id, "spec" => ?disk.device_spec ); - let (device_interface, backend_name, pci_path) = match &disk + let (device_interface, backend_id, pci_path) = match &disk .device_spec { spec::StorageDevice::Virtio(disk) => { - (DeviceInterface::Virtio, &disk.backend_name, disk.pci_path) + (DeviceInterface::Virtio, &disk.backend_id, disk.pci_path) } spec::StorageDevice::Nvme(disk) => { - (DeviceInterface::Nvme, &disk.backend_name, disk.pci_path) + (DeviceInterface::Nvme, &disk.backend_id, disk.pci_path) } }; @@ -610,18 +617,17 @@ impl<'a> MachineInitializer<'a> { let StorageBackendInstance { be: backend, crucible } = self .create_storage_backend_from_spec( &disk.backend_spec, - backend_name, + backend_id, &nexus_client, ) .await?; - self.block_backends.insert(backend_name.clone(), backend.clone()); + self.block_backends.insert(backend_id.clone(), backend.clone()); let block_dev: Arc = match device_interface { DeviceInterface::Virtio => { let vioblk = virtio::PciVirtioBlock::new(0x100); - self.devices - .insert(format!("pci-virtio-{}", bdf), vioblk.clone()); + self.devices.insert(device_id.clone(), vioblk.clone()); block::attach(vioblk.clone(), backend).unwrap(); chipset.pci_attach(bdf, vioblk.clone()); vioblk @@ -629,47 +635,49 @@ impl<'a> MachineInitializer<'a> { DeviceInterface::Nvme => { // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); - let component = format!("nvme-{}", disk_name); + let component = format!("nvme-{}", device_id); let nvme = nvme::PciNvme::create( - disk_name.to_owned(), + device_id.to_string(), mdts, self.log.new(slog::o!("component" => component)), ); - self.devices - .insert(format!("pci-nvme-{bdf}"), nvme.clone()); + self.devices.insert(device_id.clone(), nvme.clone()); block::attach(nvme.clone(), backend).unwrap(); chipset.pci_attach(bdf, nvme.clone()); nvme } }; - if let Some((disk_id, backend)) = crucible { - let block_size = backend.block_size().await; - let prev = self.crucible_backends.insert(disk_id, backend); - if prev.is_some() { - return Err(MachineInitError::DuplicateCrucibleBackendId( - disk_id, - )); - } - + if let Some(crucible) = crucible { + let block_size = crucible.block_size().await; let Some(block_size) = block_size else { slog::error!( self.log, "Could not get Crucible backend block size, \ virtual disk metrics can't be reported for it"; - "disk_id" => %disk_id, + "disk_id" => %backend_id, ); continue; }; - // Register the block device as a metric producer, if we've been - // setup to do so. Note we currently only do this for the Crucible - // backend, in which case we have the disk ID. - if let Some(registry) = &self.producer_registry { + let prev = + self.crucible_backends.insert(backend_id.clone(), crucible); + if prev.is_some() { + return Err(MachineInitError::DuplicateCrucibleBackendId( + backend_id.clone(), + )); + } + + // If metrics are enabled and this Crucible backend was + // identified with a UUID-format spec key, register this disk + // for metrics collection, using the key as the disk ID. + if let (Some(registry), SpecKey::Uuid(disk_id)) = + (&self.producer_registry, &backend_id) + { let stats = VirtualDiskProducer::new( block_size, self.properties.id, - disk_id, + *disk_id, &self.properties.metadata, ); if let Err(e) = registry.register_producer(stats.clone()) { @@ -749,8 +757,7 @@ impl<'a> MachineInitializer<'a> { format!("failed to create viona device {device_name:?}") })?; - self.devices - .insert(format!("pci-virtio-viona-{}", bdf), viona.clone()); + self.devices.insert(device_name.clone(), viona.clone()); // Only push to interface_ids if kstat_sampler exists if let Some(ref mut ids) = interface_ids { @@ -804,7 +811,7 @@ impl<'a> MachineInitializer<'a> { }, ); - self.devices.insert(mig.name.clone(), dev); + self.devices.insert(mig.id.clone(), dev); } } @@ -831,7 +838,7 @@ impl<'a> MachineInitializer<'a> { // SoftNpu ports are named __vnic by falcon, where // indicates the intended order. - ports.sort_by_key(|p| p.0.as_str()); + ports.sort_by_key(|p| p.0); let data_links = ports .iter() .map(|port| port.1.backend_spec.vnic_name.clone()) @@ -843,7 +850,8 @@ impl<'a> MachineInitializer<'a> { let uart = LpcUart::new(chipset.irq_pin(ibmpc::IRQ_COM4).unwrap()); uart.set_autodiscard(true); LpcUart::attach(&uart, &self.machine.bus_pio, ibmpc::PORT_COM4); - self.devices.insert("softnpu-uart".to_string(), uart.clone()); + self.devices + .insert(SpecKey::Name("softnpu-uart".to_string()), uart.clone()); // Start with no pipeline. The guest must load the initial P4 program. let pipeline = Arc::new(std::sync::Mutex::new(None)); @@ -859,7 +867,8 @@ impl<'a> MachineInitializer<'a> { ); let vio9p = virtio::p9fs::PciVirtio9pfs::new(0x40, Arc::new(p9_handler)); - self.devices.insert("softnpu-p9fs".to_string(), vio9p.clone()); + self.devices + .insert(SpecKey::Name("softnpu-p9fs".to_string()), vio9p.clone()); let bdf = softnpu .p9_device .as_ref() @@ -880,11 +889,14 @@ impl<'a> MachineInitializer<'a> { ) .context("failed to register softnpu")?; - self.devices.insert("softnpu-main".to_string(), softnpu.clone()); + self.devices + .insert(SpecKey::Name("softnpu-main".to_string()), softnpu.clone()); // Create the SoftNpu PCI port. - self.devices - .insert("softnpu-pciport".to_string(), softnpu.pci_port.clone()); + self.devices.insert( + SpecKey::Name("softnpu-pciport".to_string()), + softnpu.pci_port.clone(), + ); chipset.pci_attach(pci_port.pci_path.into(), softnpu.pci_port.clone()); Ok(()) @@ -906,7 +918,8 @@ impl<'a> MachineInitializer<'a> { self.log.clone(), ); let vio9p = virtio::p9fs::PciVirtio9pfs::new(0x40, Arc::new(handler)); - self.devices.insert("falcon-p9fs".to_string(), vio9p.clone()); + self.devices + .insert(SpecKey::Name("falcon-p9fs".to_string()), vio9p.clone()); chipset.pci_attach(p9fs.pci_path.into(), vio9p); } @@ -1103,14 +1116,14 @@ impl<'a> MachineInitializer<'a> { // Theoretically we could support booting from network devices by // matching them here and adding their PCI paths, but exactly what // would happen is ill-understood. So, only check disks here. - if let Some(spec) = self.spec.disks.get(boot_entry.name.as_str()) { + if let Some(spec) = self.spec.disks.get(&boot_entry.device_id) { match &spec.device_spec { StorageDevice::Virtio(disk) => { let bdf: pci::Bdf = disk.pci_path.into(); if bdf.bus.get() != 0 { return Err( MachineInitError::BootDeviceOnDownstreamPciBus( - boot_entry.name.clone(), + boot_entry.device_id.clone(), bdf.bus.get(), ), ); @@ -1123,7 +1136,7 @@ impl<'a> MachineInitializer<'a> { if bdf.bus.get() != 0 { return Err( MachineInitError::BootDeviceOnDownstreamPciBus( - boot_entry.name.clone(), + boot_entry.device_id.clone(), bdf.bus.get(), ), ); @@ -1138,7 +1151,7 @@ impl<'a> MachineInitializer<'a> { // This should be unreachable - we check that the boot disk is // valid when constructing the spec we're initializing from. return Err(MachineInitError::BootOrderEntryWithoutDevice( - boot_entry.name.clone(), + boot_entry.device_id.clone(), )); } } @@ -1199,8 +1212,9 @@ impl<'a> MachineInitializer<'a> { fwcfg.attach(&self.machine.bus_pio, &self.machine.acc_mem); - self.devices.insert(fwcfg.type_name().into(), fwcfg); - self.devices.insert(ramfb.type_name().into(), ramfb.clone()); + self.devices.insert(SpecKey::Name(fwcfg.type_name().into()), fwcfg); + self.devices + .insert(SpecKey::Name(ramfb.type_name().into()), ramfb.clone()); Ok(ramfb) } @@ -1235,7 +1249,10 @@ impl<'a> MachineInitializer<'a> { .context("failed to set vcpu capabilities")?; // The vCPUs behave like devices, so add them to the list as well - self.devices.insert(format!("vcpu-{}", vcpu.id), vcpu.clone()); + self.devices.insert( + SpecKey::Name(format!("vcpu-{}", vcpu.id)), + vcpu.clone(), + ); } if let Some(sampler) = self.kstat_sampler.as_ref() { track_vcpu_kstats(&self.log, sampler, &self.stats_vm).await; diff --git a/bin/propolis-server/src/lib/migrate/destination.rs b/bin/propolis-server/src/lib/migrate/destination.rs index 6d29e1115..49bb96b1f 100644 --- a/bin/propolis-server/src/lib/migrate/destination.rs +++ b/bin/propolis-server/src/lib/migrate/destination.rs @@ -9,6 +9,7 @@ use propolis::migrate::{ MigrateCtx, MigrateStateError, Migrator, PayloadOffer, PayloadOffers, }; use propolis::vmm; +use propolis_api_types::instance_spec::SpecKey; use propolis_api_types::ReplacementComponent; use slog::{error, info, trace, warn}; use std::collections::BTreeMap; @@ -40,7 +41,7 @@ use super::MigrateConn; pub(crate) struct MigrationTargetInfo { pub migration_id: Uuid, pub src_addr: SocketAddr, - pub replace_components: BTreeMap, + pub replace_components: BTreeMap, } /// The interface to an arbitrary version of the target half of the live @@ -519,16 +520,13 @@ impl RonV0 { let migrate_ctx = MigrateCtx { mem: &vm_objects.access_mem().unwrap() }; for device in devices { - info!( - self.log(), - "Applying state to device {}", device.instance_name - ); + let key = SpecKey::from(device.instance_name.clone()); + info!(self.log(), "Applying state to device {key}"); - let target = vm_objects - .device_by_name(&device.instance_name) - .ok_or_else(|| { - MigrateError::UnknownDevice(device.instance_name.clone()) - })?; + let target = + vm_objects.device_by_id(&key).ok_or_else(|| { + MigrateError::UnknownDevice(key.to_string()) + })?; self.import_device(&target, &device, &migrate_ctx)?; } } diff --git a/bin/propolis-server/src/lib/migrate/preamble.rs b/bin/propolis-server/src/lib/migrate/preamble.rs index 6729f92e2..4c6af125a 100644 --- a/bin/propolis-server/src/lib/migrate/preamble.rs +++ b/bin/propolis-server/src/lib/migrate/preamble.rs @@ -5,7 +5,7 @@ use std::collections::BTreeMap; use propolis_api_types::{ - instance_spec::{v0::ComponentV0, VersionedInstanceSpec}, + instance_spec::{v0::ComponentV0, SpecKey, VersionedInstanceSpec}, ReplacementComponent, }; use serde::{Deserialize, Serialize}; @@ -35,9 +35,9 @@ impl Preamble { /// not present in the source spec, this routine fails. pub fn amend_spec( self, - replacements: &BTreeMap, + replacements: &BTreeMap, ) -> Result { - fn wrong_type_error(id: &str, kind: &str) -> MigrateError { + fn wrong_type_error(id: &SpecKey, kind: &str) -> MigrateError { let msg = format!("component {id} is not a {kind} in the source spec"); MigrateError::InstanceSpecsIncompatible(msg) @@ -45,8 +45,7 @@ impl Preamble { let VersionedInstanceSpec::V0(mut source_spec) = self.instance_spec; for (id, comp) in replacements { - let Some(to_amend) = source_spec.components.get_mut(id.as_str()) - else { + let Some(to_amend) = source_spec.components.get_mut(id) else { return Err(MigrateError::InstanceSpecsIncompatible(format!( "replacement component {id} not in source spec", ))); diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index 00059b501..16ea39668 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -36,6 +36,7 @@ use internal_dns::ServiceName; pub use nexus_client::Client as NexusClient; use oximeter::types::ProducerRegistry; use propolis_api_types as api; +use propolis_api_types::instance_spec::SpecKey; use propolis_api_types::InstanceInitializationMethod; use rfb::tungstenite::BinaryWs; use slog::{error, warn, Logger}; @@ -543,8 +544,10 @@ async fn instance_issue_crucible_snapshot_request( let objects = vm.objects().lock_shared().await; let path_params = path_params.into_inner(); - let backend = - objects.crucible_backends().get(&path_params.id).ok_or_else(|| { + let backend = objects + .crucible_backends() + .get(&SpecKey::from(path_params.id.clone())) + .ok_or_else(|| { let s = format!("no disk with id {}!", path_params.id); HttpError::for_not_found(Some(s.clone()), s) })?; @@ -568,8 +571,10 @@ async fn disk_volume_status( let vm = rqctx.context().vm.active_vm().await.ok_or_else(not_created_error)?; let objects = vm.objects().lock_shared().await; - let backend = - objects.crucible_backends().get(&path_params.id).ok_or_else(|| { + let backend = objects + .crucible_backends() + .get(&SpecKey::from(path_params.id.clone())) + .ok_or_else(|| { let s = format!("No crucible backend for id {}", path_params.id); HttpError::for_not_found(Some(s.clone()), s) })?; @@ -594,22 +599,25 @@ async fn instance_issue_crucible_vcr_request( let path_params = path_params.into_inner(); let request = request.into_inner(); let new_vcr_json = request.vcr_json; - let disk_name = request.name; let (tx, rx) = tokio::sync::oneshot::channel(); let vm = rqctx.context().vm.active_vm().await.ok_or_else(not_created_error)?; - vm.reconfigure_crucible_volume(disk_name, path_params.id, new_vcr_json, tx) - .map_err(|e| match e { - VmError::ForbiddenStateChange(reason) => HttpError::for_status( - Some(format!("instance state change not allowed: {}", reason)), - hyper::StatusCode::FORBIDDEN, - ), - _ => HttpError::for_internal_error(format!( - "unexpected error from VM controller: {e}" - )), - })?; + vm.reconfigure_crucible_volume( + SpecKey::from(path_params.id), + new_vcr_json, + tx, + ) + .map_err(|e| match e { + VmError::ForbiddenStateChange(reason) => HttpError::for_status( + Some(format!("instance state change not allowed: {}", reason)), + hyper::StatusCode::FORBIDDEN, + ), + _ => HttpError::for_internal_error(format!( + "unexpected error from VM controller: {e}" + )), + })?; let result = rx.await.map_err(|_| { HttpError::for_internal_error( diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs index 6dca51918..837720e31 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -5,7 +5,7 @@ //! Conversions from version-0 instance specs in the [`propolis_api_types`] //! crate to the internal [`super::Spec`] representation. -use std::collections::HashMap; +use std::collections::BTreeMap; use propolis_api_types::instance_spec::{ components::{ @@ -14,6 +14,7 @@ use propolis_api_types::instance_spec::{ devices::{BootSettings, SerialPort as SerialPortDesc}, }, v0::{ComponentV0, InstanceSpecV0}, + SpecKey, }; use thiserror::Error; @@ -38,17 +39,17 @@ pub(crate) enum ApiSpecError { Builder(#[from] SpecBuilderError), #[error("storage backend {backend} not found for device {device}")] - StorageBackendNotFound { backend: String, device: String }, + StorageBackendNotFound { backend: SpecKey, device: SpecKey }, #[error("network backend {backend} not found for device {device}")] - NetworkBackendNotFound { backend: String, device: String }, + NetworkBackendNotFound { backend: SpecKey, device: SpecKey }, #[allow(dead_code)] #[error("support for component {component} compiled out via {feature}")] - FeatureCompiledOut { component: String, feature: &'static str }, + FeatureCompiledOut { component: SpecKey, feature: &'static str }, #[error("backend {0} not used by any device")] - BackendNotUsed(String), + BackendNotUsed(SpecKey), } impl From for InstanceSpecV0 { @@ -79,7 +80,7 @@ impl From for InstanceSpecV0 { #[track_caller] fn insert_component( spec: &mut InstanceSpecV0, - key: String, + key: SpecKey, val: ComponentV0, ) { assert!( @@ -98,24 +99,23 @@ impl From for InstanceSpecV0 { }; let mut spec = InstanceSpecV0 { board, components: Default::default() }; - for (disk_name, disk) in disks { - let backend_name = disk.device_spec.backend_name().to_owned(); - insert_component(&mut spec, disk_name, disk.device_spec.into()); - - insert_component(&mut spec, backend_name, disk.backend_spec.into()); + for (disk_id, disk) in disks { + let backend_id = disk.device_spec.backend_id().to_owned(); + insert_component(&mut spec, disk_id, disk.device_spec.into()); + insert_component(&mut spec, backend_id, disk.backend_spec.into()); } - for (nic_name, nic) in nics { - let backend_name = nic.device_spec.backend_name.clone(); + for (nic_id, nic) in nics { + let backend_id = nic.device_spec.backend_id.clone(); insert_component( &mut spec, - nic_name, + nic_id, ComponentV0::VirtioNic(nic.device_spec), ); insert_component( &mut spec, - backend_name, + backend_id, ComponentV0::VirtioNetworkBackend(nic.backend_spec), ); } @@ -141,7 +141,7 @@ impl From for InstanceSpecV0 { if let Some(pvpanic) = pvpanic { insert_component( &mut spec, - pvpanic.name, + pvpanic.id, ComponentV0::QemuPvpanic(pvpanic.spec), ); } @@ -160,7 +160,7 @@ impl From for InstanceSpecV0 { if let Some(mig) = migration_failure { insert_component( &mut spec, - mig.name, + mig.id, ComponentV0::MigrationFailureInjector(mig.spec), ); } @@ -170,7 +170,10 @@ impl From for InstanceSpecV0 { if let Some(softnpu_pci) = softnpu.pci_port { insert_component( &mut spec, - format!("softnpu-pci-{}", softnpu_pci.pci_path), + SpecKey::Name(format!( + "softnpu-pci-{}", + softnpu_pci.pci_path + )), ComponentV0::SoftNpuPciPort(softnpu_pci), ); } @@ -178,7 +181,7 @@ impl From for InstanceSpecV0 { if let Some(p9) = softnpu.p9_device { insert_component( &mut spec, - format!("softnpu-p9-{}", p9.pci_path), + SpecKey::Name(format!("softnpu-p9-{}", p9.pci_path)), ComponentV0::SoftNpuP9(p9), ); } @@ -186,7 +189,7 @@ impl From for InstanceSpecV0 { if let Some(p9fs) = softnpu.p9fs { insert_component( &mut spec, - format!("p9fs-{}", p9fs.pci_path), + SpecKey::Name(format!("p9fs-{}", p9fs.pci_path)), ComponentV0::P9fs(p9fs), ); } @@ -197,7 +200,7 @@ impl From for InstanceSpecV0 { port_name.clone(), ComponentV0::SoftNpuPort(SoftNpuPortSpec { link_name: port.link_name, - backend_name: port.backend_name.clone(), + backend_id: port.backend_name.clone(), }), ); @@ -218,83 +221,83 @@ impl TryFrom for Spec { fn try_from(value: InstanceSpecV0) -> Result { let mut builder = SpecBuilder::with_instance_spec_board(value.board)?; - let mut devices: Vec<(String, ComponentV0)> = vec![]; + let mut devices: Vec<(SpecKey, ComponentV0)> = vec![]; let mut boot_settings = None; - let mut storage_backends: HashMap = - HashMap::new(); - let mut viona_backends: HashMap = - HashMap::new(); - let mut dlpi_backends: HashMap = - HashMap::new(); - - for (name, component) in value.components.into_iter() { + let mut storage_backends: BTreeMap = + BTreeMap::new(); + let mut viona_backends: BTreeMap = + BTreeMap::new(); + let mut dlpi_backends: BTreeMap = + BTreeMap::new(); + + for (id, component) in value.components.into_iter() { match component { ComponentV0::CrucibleStorageBackend(_) | ComponentV0::FileStorageBackend(_) | ComponentV0::BlobStorageBackend(_) => { storage_backends.insert( - name, + id, component.try_into().expect( "component is known to be a storage backend", ), ); } ComponentV0::VirtioNetworkBackend(viona) => { - viona_backends.insert(name, viona); + viona_backends.insert(id, viona); } ComponentV0::DlpiNetworkBackend(dlpi) => { - dlpi_backends.insert(name, dlpi); + dlpi_backends.insert(id, dlpi); } device => { - devices.push((name, device)); + devices.push((id, device)); } } } - for (device_name, device_spec) in devices { + for (device_id, device_spec) in devices { match device_spec { ComponentV0::VirtioDisk(_) | ComponentV0::NvmeDisk(_) => { let device_spec = StorageDevice::try_from(device_spec) .expect("component is known to be a disk"); let (_, backend_spec) = storage_backends - .remove_entry(device_spec.backend_name()) + .remove_entry(device_spec.backend_id()) .ok_or_else(|| { ApiSpecError::StorageBackendNotFound { - backend: device_spec.backend_name().to_owned(), - device: device_name.clone(), + backend: device_spec.backend_id().to_owned(), + device: device_id.clone(), } })?; builder.add_storage_device( - device_name, + device_id, Disk { device_spec, backend_spec }, )?; } ComponentV0::VirtioNic(nic) => { let (_, backend_spec) = viona_backends - .remove_entry(&nic.backend_name) + .remove_entry(&nic.backend_id) .ok_or_else(|| { ApiSpecError::NetworkBackendNotFound { - backend: nic.backend_name.clone(), - device: device_name.clone(), + backend: nic.backend_id.clone(), + device: device_id.clone(), } })?; builder.add_network_device( - device_name, + device_id, Nic { device_spec: nic, backend_spec }, )?; } ComponentV0::SerialPort(port) => { - builder.add_serial_port(device_name, port.num)?; + builder.add_serial_port(device_id, port.num)?; } ComponentV0::PciPciBridge(bridge) => { - builder.add_pci_bridge(device_name, bridge)?; + builder.add_pci_bridge(device_id, bridge)?; } ComponentV0::QemuPvpanic(pvpanic) => { builder.add_pvpanic_device(QemuPvpanic { - name: device_name, + id: device_id, spec: pvpanic, })?; } @@ -304,19 +307,19 @@ impl TryFrom for Spec { // Since there may be more disk devices left in the // component map, just capture the boot order for now and // apply it to the builder later. - boot_settings = Some((device_name, settings)); + boot_settings = Some((device_id, settings)); } #[cfg(feature = "omicron-build")] ComponentV0::MigrationFailureInjector(_) => { return Err(ApiSpecError::FeatureCompiledOut { - component: device_name, + component: device_id, feature: "omicron-build", }); } #[cfg(not(feature = "omicron-build"))] ComponentV0::MigrationFailureInjector(mig) => { builder.add_migration_failure_device(MigrationFailure { - name: device_name, + id: device_id, spec: mig, })?; } @@ -326,7 +329,7 @@ impl TryFrom for Spec { | ComponentV0::SoftNpuP9(_) | ComponentV0::P9fs(_) => { return Err(ApiSpecError::FeatureCompiledOut { - component: device_name, + component: device_id, feature: "falcon", }); } @@ -337,21 +340,21 @@ impl TryFrom for Spec { #[cfg(feature = "falcon")] ComponentV0::SoftNpuPort(port) => { let (_, backend_spec) = dlpi_backends - .remove_entry(&port.backend_name) + .remove_entry(&port.backend_id) .ok_or_else(|| { ApiSpecError::NetworkBackendNotFound { - backend: port.backend_name.clone(), - device: device_name.clone(), + backend: port.backend_id.clone(), + device: device_id.clone(), } })?; let port = SoftNpuPort { link_name: port.link_name, - backend_name: port.backend_name, + backend_name: port.backend_id, backend_spec, }; - builder.add_softnpu_port(device_name, port)?; + builder.add_softnpu_port(device_id, port)?; } #[cfg(feature = "falcon")] ComponentV0::SoftNpuP9(p9) => { diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index 8bbd4e195..a417c2b2d 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -12,7 +12,7 @@ use propolis_api_types::instance_spec::{ board::Board as InstanceSpecBoard, devices::{PciPciBridge, SerialPortNumber}, }, - PciPath, + PciPath, SpecKey, }; use thiserror::Error; @@ -37,10 +37,10 @@ use super::SoftNpuPort; #[derive(Debug, Error)] pub(crate) enum SpecBuilderError { #[error("device {0} has the same name as its backend")] - DeviceAndBackendNamesIdentical(String), + DeviceAndBackendNamesIdentical(SpecKey), #[error("a component with name {0} already exists")] - ComponentNameInUse(String), + ComponentNameInUse(SpecKey), #[error("a PCI device is already attached at {0:?}")] PciPathInUse(PciPath), @@ -59,7 +59,7 @@ pub(crate) enum SpecBuilderError { BootSettingsInUse, #[error("boot option {0} is not an attached device")] - BootOptionMissing(String), + BootOptionMissing(SpecKey), #[error("instance spec's CPUID entries are invalid")] CpuidEntriesInvalid(#[from] cpuid_utils::CpuidMapConversionError), @@ -70,7 +70,7 @@ pub(crate) struct SpecBuilder { spec: super::Spec, pci_paths: BTreeSet, serial_ports: HashSet, - component_names: BTreeSet, + component_names: BTreeSet, } impl SpecBuilder { @@ -108,11 +108,11 @@ impl SpecBuilder { /// in the spec's disk map. pub fn add_boot_order( &mut self, - component_name: String, + component_id: SpecKey, boot_options: impl Iterator, ) -> Result<(), SpecBuilderError> { - if self.component_names.contains(&component_name) { - return Err(SpecBuilderError::ComponentNameInUse(component_name)); + if self.component_names.contains(&component_id) { + return Err(SpecBuilderError::ComponentNameInUse(component_id)); } if self.spec.boot_settings.is_some() { @@ -121,17 +121,19 @@ impl SpecBuilder { let mut order = vec![]; for item in boot_options { - if !self.spec.disks.contains_key(item.name.as_str()) { + if !self.spec.disks.contains_key(&item.device_id) { return Err(SpecBuilderError::BootOptionMissing( - item.name.clone(), + item.device_id.clone(), )); } - order.push(crate::spec::BootOrderEntry { name: item.name.clone() }); + order.push(crate::spec::BootOrderEntry { + device_id: item.device_id.clone(), + }); } self.spec.boot_settings = - Some(BootSettings { name: component_name, order }); + Some(BootSettings { name: component_id, order }); Ok(()) } @@ -152,29 +154,29 @@ impl SpecBuilder { /// Adds a storage device with an associated backend. pub(super) fn add_storage_device( &mut self, - disk_name: String, + disk_id: SpecKey, disk: Disk, ) -> Result<&Self, SpecBuilderError> { - if disk_name == disk.device_spec.backend_name() { + if disk_id == *disk.device_spec.backend_id() { return Err(SpecBuilderError::DeviceAndBackendNamesIdentical( - disk_name, + disk_id, )); } - if self.component_names.contains(&disk_name) { - return Err(SpecBuilderError::ComponentNameInUse(disk_name)); + if self.component_names.contains(&disk_id) { + return Err(SpecBuilderError::ComponentNameInUse(disk_id)); } - if self.component_names.contains(disk.device_spec.backend_name()) { + if self.component_names.contains(disk.device_spec.backend_id()) { return Err(SpecBuilderError::ComponentNameInUse( - disk.device_spec.backend_name().to_owned(), + disk.device_spec.backend_id().to_owned(), )); } self.register_pci_device(disk.device_spec.pci_path())?; - self.component_names.insert(disk_name.clone()); - self.component_names.insert(disk.device_spec.backend_name().to_owned()); - let _old = self.spec.disks.insert(disk_name, disk); + self.component_names.insert(disk_id.clone()); + self.component_names.insert(disk.device_spec.backend_id().to_owned()); + let _old = self.spec.disks.insert(disk_id, disk); assert!(_old.is_none()); Ok(self) } @@ -182,29 +184,29 @@ impl SpecBuilder { /// Adds a network device with an associated backend. pub(super) fn add_network_device( &mut self, - nic_name: String, + nic_id: SpecKey, nic: Nic, ) -> Result<&Self, SpecBuilderError> { - if nic_name == nic.device_spec.backend_name { + if nic_id == nic.device_spec.backend_id { return Err(SpecBuilderError::DeviceAndBackendNamesIdentical( - nic_name, + nic_id, )); } - if self.component_names.contains(&nic_name) { - return Err(SpecBuilderError::ComponentNameInUse(nic_name)); + if self.component_names.contains(&nic_id) { + return Err(SpecBuilderError::ComponentNameInUse(nic_id)); } - if self.component_names.contains(&nic.device_spec.backend_name) { + if self.component_names.contains(&nic.device_spec.backend_id) { return Err(SpecBuilderError::ComponentNameInUse( - nic.device_spec.backend_name, + nic.device_spec.backend_id, )); } self.register_pci_device(nic.device_spec.pci_path)?; - self.component_names.insert(nic_name.clone()); - self.component_names.insert(nic.device_spec.backend_name.clone()); - let _old = self.spec.nics.insert(nic_name, nic); + self.component_names.insert(nic_id.clone()); + self.component_names.insert(nic.device_spec.backend_id.clone()); + let _old = self.spec.nics.insert(nic_id, nic); assert!(_old.is_none()); Ok(self) } @@ -212,16 +214,16 @@ impl SpecBuilder { /// Adds a PCI-PCI bridge. pub fn add_pci_bridge( &mut self, - name: String, + id: SpecKey, bridge: PciPciBridge, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&name) { - return Err(SpecBuilderError::ComponentNameInUse(name)); + if self.component_names.contains(&id) { + return Err(SpecBuilderError::ComponentNameInUse(id)); } self.register_pci_device(bridge.pci_path)?; - self.component_names.insert(name.clone()); - let _old = self.spec.pci_pci_bridges.insert(name, bridge); + self.component_names.insert(id.clone()); + let _old = self.spec.pci_pci_bridges.insert(id, bridge); assert!(_old.is_none()); Ok(self) } @@ -229,11 +231,11 @@ impl SpecBuilder { /// Adds a serial port. pub fn add_serial_port( &mut self, - name: String, + id: SpecKey, num: SerialPortNumber, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&name) { - return Err(SpecBuilderError::ComponentNameInUse(name)); + if self.component_names.contains(&id) { + return Err(SpecBuilderError::ComponentNameInUse(id)); } if self.serial_ports.contains(&num) { @@ -241,8 +243,8 @@ impl SpecBuilder { } let desc = SerialPort { num, device: SerialPortDevice::Uart }; - self.spec.serial.insert(name.clone(), desc); - self.component_names.insert(name); + self.spec.serial.insert(id.clone(), desc); + self.component_names.insert(id); self.serial_ports.insert(num); Ok(self) } @@ -251,15 +253,15 @@ impl SpecBuilder { &mut self, pvpanic: QemuPvpanic, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&pvpanic.name) { - return Err(SpecBuilderError::ComponentNameInUse(pvpanic.name)); + if self.component_names.contains(&pvpanic.id) { + return Err(SpecBuilderError::ComponentNameInUse(pvpanic.id)); } if self.spec.pvpanic.is_some() { return Err(SpecBuilderError::PvpanicInUse); } - self.component_names.insert(pvpanic.name.clone()); + self.component_names.insert(pvpanic.id.clone()); self.spec.pvpanic = Some(pvpanic); Ok(self) } @@ -269,15 +271,15 @@ impl SpecBuilder { &mut self, mig: MigrationFailure, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&mig.name) { - return Err(SpecBuilderError::ComponentNameInUse(mig.name)); + if self.component_names.contains(&mig.id) { + return Err(SpecBuilderError::ComponentNameInUse(mig.id)); } if self.spec.migration_failure.is_some() { return Err(SpecBuilderError::MigrationFailureInjectionInUse); } - self.component_names.insert(mig.name.clone()); + self.component_names.insert(mig.id.clone()); self.spec.migration_failure = Some(mig); Ok(self) } @@ -288,7 +290,7 @@ impl SpecBuilder { pci_port: SoftNpuPciPort, ) -> Result<&Self, SpecBuilderError> { // SoftNPU squats on COM4. - let id = "com4".to_string(); + let id = SpecKey::Name("com4".to_string()); let num = SerialPortNumber::Com4; if self.component_names.contains(&id) { return Err(SpecBuilderError::ComponentNameInUse(id)); @@ -326,7 +328,7 @@ impl SpecBuilder { #[cfg(feature = "falcon")] pub fn add_softnpu_port( &mut self, - port_name: String, + port_name: SpecKey, port: SoftNpuPort, ) -> Result<&Self, SpecBuilderError> { if port_name == port.backend_name { @@ -387,10 +389,10 @@ mod test { let mut builder = test_builder(); assert!(builder .add_storage_device( - "storage".to_owned(), + SpecKey::Name("storage".to_owned()), Disk { device_spec: StorageDevice::Virtio(VirtioDisk { - backend_name: "storage-backend".to_owned(), + backend_id: SpecKey::Name("storage-backend".to_owned()), pci_path: PciPath::new(0, 4, 0).unwrap() }), backend_spec: StorageBackend::Blob(BlobStorageBackend { @@ -403,10 +405,10 @@ mod test { assert!(builder .add_network_device( - "network".to_owned(), + SpecKey::Name("network".to_owned()), Nic { device_spec: VirtioNic { - backend_name: "network-backend".to_owned(), + backend_id: SpecKey::Name("network-backend".to_owned()), interface_id: Uuid::nil(), pci_path: PciPath::new(0, 4, 0).unwrap() }, @@ -422,19 +424,34 @@ mod test { fn duplicate_serial_port() { let mut builder = test_builder(); assert!(builder - .add_serial_port("com1".to_owned(), SerialPortNumber::Com1) + .add_serial_port( + SpecKey::Name("com1".to_owned()), + SerialPortNumber::Com1 + ) .is_ok()); assert!(builder - .add_serial_port("com2".to_owned(), SerialPortNumber::Com2) + .add_serial_port( + SpecKey::Name("com2".to_owned()), + SerialPortNumber::Com2 + ) .is_ok()); assert!(builder - .add_serial_port("com3".to_owned(), SerialPortNumber::Com3) + .add_serial_port( + SpecKey::Name("com3".to_owned()), + SerialPortNumber::Com3 + ) .is_ok()); assert!(builder - .add_serial_port("com4".to_owned(), SerialPortNumber::Com4) + .add_serial_port( + SpecKey::Name("com4".to_owned()), + SerialPortNumber::Com4 + ) .is_ok()); assert!(builder - .add_serial_port("com1".to_owned(), SerialPortNumber::Com1) + .add_serial_port( + SpecKey::Name("com1".to_owned()), + SerialPortNumber::Com1 + ) .is_err()); } @@ -443,10 +460,10 @@ mod test { let mut builder = test_builder(); assert!(builder .add_storage_device( - "storage".to_owned(), + SpecKey::Name("storage".to_owned()), Disk { device_spec: StorageDevice::Virtio(VirtioDisk { - backend_name: "storage".to_owned(), + backend_id: SpecKey::Name("storage".to_owned()), pci_path: PciPath::new(0, 4, 0).unwrap() }), backend_spec: StorageBackend::Blob(BlobStorageBackend { @@ -459,10 +476,10 @@ mod test { assert!(builder .add_network_device( - "network".to_owned(), + SpecKey::Name("network".to_owned()), Nic { device_spec: VirtioNic { - backend_name: "network".to_owned(), + backend_id: SpecKey::Name("network".to_owned()), interface_id: Uuid::nil(), pci_path: PciPath::new(0, 5, 0).unwrap() }, diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 10258265c..b79ae2d68 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -14,7 +14,7 @@ //! [`Spec`] and its component types to take forms that might otherwise be hard //! to change in a backward-compatible way. -use std::collections::HashMap; +use std::collections::BTreeMap; use cpuid_utils::CpuidSet; use propolis_api_types::instance_spec::{ @@ -30,7 +30,7 @@ use propolis_api_types::instance_spec::{ }, }, v0::ComponentV0, - PciPath, + PciPath, SpecKey, }; use thiserror::Error; @@ -64,13 +64,13 @@ pub struct ComponentTypeMismatch; pub(crate) struct Spec { pub board: Board, pub cpuid: Option, - pub disks: HashMap, - pub nics: HashMap, + pub disks: BTreeMap, + pub nics: BTreeMap, pub boot_settings: Option, - pub serial: HashMap, + pub serial: BTreeMap, - pub pci_pci_bridges: HashMap, + pub pci_pci_bridges: BTreeMap, pub pvpanic: Option, #[cfg(not(feature = "omicron-build"))] @@ -106,13 +106,13 @@ impl Default for Board { #[derive(Clone, Debug)] pub(crate) struct BootSettings { - pub name: String, + pub name: SpecKey, pub order: Vec, } -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug)] pub(crate) struct BootOrderEntry { - pub name: String, + pub device_id: SpecKey, } impl @@ -122,7 +122,7 @@ impl fn from( value: propolis_api_types::instance_spec::components::devices::BootOrderEntry, ) -> Self { - Self { name: value.name.clone() } + Self { device_id: value.id.clone() } } } @@ -130,7 +130,7 @@ impl From for propolis_api_types::instance_spec::components::devices::BootOrderEntry { fn from(value: BootOrderEntry) -> Self { - Self { name: value.name } + Self { id: value.device_id } } } @@ -156,10 +156,10 @@ impl StorageDevice { } } - pub fn backend_name(&self) -> &str { + pub fn backend_id(&self) -> &SpecKey { match self { - StorageDevice::Virtio(disk) => &disk.backend_name, - StorageDevice::Nvme(disk) => &disk.backend_name, + StorageDevice::Virtio(disk) => &disk.backend_id, + StorageDevice::Nvme(disk) => &disk.backend_id, } } } @@ -279,14 +279,14 @@ pub struct SerialPort { #[derive(Clone, Debug)] pub struct QemuPvpanic { #[allow(dead_code)] - pub name: String, + pub id: SpecKey, pub spec: QemuPvpanicDesc, } #[cfg(not(feature = "omicron-build"))] #[derive(Clone, Debug)] pub struct MigrationFailure { - pub name: String, + pub id: SpecKey, pub spec: MigrationFailureInjector, } @@ -294,7 +294,7 @@ pub struct MigrationFailure { #[derive(Clone, Debug)] pub struct SoftNpuPort { pub link_name: String, - pub backend_name: String, + pub backend_name: SpecKey, pub backend_spec: DlpiNetworkBackend, } @@ -302,7 +302,7 @@ pub struct SoftNpuPort { #[derive(Clone, Debug, Default)] pub struct SoftNpu { pub pci_port: Option, - pub ports: HashMap, + pub ports: BTreeMap, pub p9_device: Option, pub p9fs: Option, } diff --git a/bin/propolis-server/src/lib/vm/active.rs b/bin/propolis-server/src/lib/vm/active.rs index 1274782f9..8a812bf27 100644 --- a/bin/propolis-server/src/lib/vm/active.rs +++ b/bin/propolis-server/src/lib/vm/active.rs @@ -6,7 +6,9 @@ use std::sync::Arc; -use propolis_api_types::{InstanceProperties, InstanceStateRequested}; +use propolis_api_types::{ + instance_spec::SpecKey, InstanceProperties, InstanceStateRequested, +}; use slog::info; use uuid::Uuid; @@ -99,15 +101,13 @@ impl ActiveVm { /// replacement result after it completes this operation. pub(crate) fn reconfigure_crucible_volume( &self, - disk_name: String, - backend_id: Uuid, + backend_id: SpecKey, new_vcr_json: String, result_tx: CrucibleReplaceResultTx, ) -> Result<(), VmError> { self.state_driver_queue .queue_external_request( ExternalRequest::ReconfigureCrucibleVolume { - disk_name, backend_id, new_vcr_json, result_tx, diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index f680702fc..debb9af78 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -85,10 +85,11 @@ use active::ActiveVm; use ensure::VmEnsureRequest; use oximeter::types::ProducerRegistry; use propolis_api_types::{ - instance_spec::VersionedInstanceSpec, InstanceEnsureResponse, - InstanceMigrateStatusResponse, InstanceMigrationStatus, InstanceProperties, - InstanceSpecGetResponse, InstanceSpecStatus, InstanceState, - InstanceStateMonitorResponse, MigrationState, + instance_spec::{SpecKey, VersionedInstanceSpec}, + InstanceEnsureResponse, InstanceMigrateStatusResponse, + InstanceMigrationStatus, InstanceProperties, InstanceSpecGetResponse, + InstanceSpecStatus, InstanceState, InstanceStateMonitorResponse, + MigrationState, }; use slog::info; use state_driver::StateDriverOutput; @@ -109,7 +110,7 @@ pub(crate) mod state_publisher; /// Maps component names to lifecycle trait objects that allow /// components to be started, paused, resumed, and halted. pub(crate) type DeviceMap = - BTreeMap>; + BTreeMap>; /// Mapping of NIC identifiers to viona device instance IDs. /// We use a Vec here due to the limited size of the NIC array. @@ -117,11 +118,11 @@ pub(crate) type NetworkInterfaceIds = Vec<(uuid::Uuid, KstatInstanceId)>; /// Maps component names to block backend trait objects. pub(crate) type BlockBackendMap = - BTreeMap>; + BTreeMap>; /// Maps component names to Crucible backend objects. pub(crate) type CrucibleBackendMap = - BTreeMap>; + BTreeMap>; /// Type alias for the sender side of the channel that receives /// externally-visible instance state updates. diff --git a/bin/propolis-server/src/lib/vm/objects.rs b/bin/propolis-server/src/lib/vm/objects.rs index a79cee1d7..2ade7dab1 100644 --- a/bin/propolis-server/src/lib/vm/objects.rs +++ b/bin/propolis-server/src/lib/vm/objects.rs @@ -17,6 +17,7 @@ use propolis::{ vmm::VmmHdl, Machine, }; +use propolis_api_types::instance_spec::SpecKey; use slog::{error, info}; use tokio::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; @@ -159,12 +160,12 @@ impl VmObjectsLocked { } /// Obtains a handle to the lifecycle trait object for the component with - /// the supplied `name`. - pub(crate) fn device_by_name( + /// the supplied `id`. + pub(crate) fn device_by_id( &self, - name: &str, + id: &SpecKey, ) -> Option> { - self.devices.get(name).cloned() + self.devices.get(id).cloned() } /// Yields the VM's current Crucible backend map. @@ -192,7 +193,7 @@ impl VmObjectsLocked { /// `func` on each one. pub(crate) fn for_each_device( &self, - mut func: impl FnMut(&str, &Arc), + mut func: impl FnMut(&SpecKey, &Arc), ) { for (name, dev) in self.devices.iter() { func(name, dev); @@ -205,7 +206,7 @@ impl VmObjectsLocked { pub(crate) fn for_each_device_fallible( &self, mut func: impl FnMut( - &str, + &SpecKey, &Arc, ) -> std::result::Result<(), E>, ) -> std::result::Result<(), E> { @@ -377,7 +378,7 @@ impl VmObjectsLocked { .iter() .map(|(name, dev)| { info!(self.log, "got paused future from dev {}", name); - NamedFuture { name: name.clone(), future: dev.paused() } + NamedFuture { name: name.to_string(), future: dev.paused() } }) .collect(); @@ -408,12 +409,12 @@ impl VmObjectsLocked { }); }); - for (name, backend) in self.block_backends.iter() { - info!(self.log, "stopping and detaching block backend {}", name); + for (id, backend) in self.block_backends.iter() { + info!(self.log, "stopping and detaching block backend {}", id); backend.stop().await; if let Err(err) = backend.detach() { error!(self.log, "error detaching block backend"; - "name" => name, + "id" => %id, "error" => ?err); } } diff --git a/bin/propolis-server/src/lib/vm/request_queue.rs b/bin/propolis-server/src/lib/vm/request_queue.rs index 6cea692a1..30d26f59c 100644 --- a/bin/propolis-server/src/lib/vm/request_queue.rs +++ b/bin/propolis-server/src/lib/vm/request_queue.rs @@ -23,6 +23,7 @@ use std::collections::VecDeque; +use propolis_api_types::instance_spec::SpecKey; use slog::{debug, info, Logger}; use thiserror::Error; use uuid::Uuid; @@ -78,11 +79,8 @@ pub enum ExternalRequest { /// is only allowed once the VM is started and the volume has activated, but /// it should be allowed even before the VM has started. ReconfigureCrucibleVolume { - /// The name of the Crucible backend component in the instance spec. - disk_name: String, - /// The ID of the Crucible backend in the VM's Crucible backend map. - backend_id: Uuid, + backend_id: SpecKey, /// The new volume construction request to supply to the Crucible /// upstairs. @@ -103,11 +101,8 @@ impl std::fmt::Debug for ExternalRequest { .finish(), Self::Reboot => write!(f, "Reboot"), Self::Stop => write!(f, "Stop"), - Self::ReconfigureCrucibleVolume { - disk_name, backend_id, .. - } => f + Self::ReconfigureCrucibleVolume { backend_id, .. } => f .debug_struct("ReconfigureCrucibleVolume") - .field("disk_name", disk_name) .field("backend_id", backend_id) .finish(), } @@ -473,8 +468,7 @@ mod test { fn make_reconfigure_crucible_request() -> ExternalRequest { let (tx, _rx) = tokio::sync::oneshot::channel(); ExternalRequest::ReconfigureCrucibleVolume { - disk_name: "".to_string(), - backend_id: Uuid::new_v4(), + backend_id: SpecKey::Uuid(Uuid::new_v4()), new_vcr_json: "".to_string(), result_tx: tx, } diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index e565f96da..0987699e9 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -10,9 +10,10 @@ use std::{ }; use anyhow::Context; +use dropshot::HttpError; use propolis_api_types::{ - instance_spec::components::backends::CrucibleStorageBackend, InstanceState, - MigrationState, + instance_spec::{components::backends::CrucibleStorageBackend, SpecKey}, + InstanceState, MigrationState, }; use slog::{error, info}; use tokio::sync::Notify; @@ -510,18 +511,13 @@ impl StateDriver { } } ExternalRequest::ReconfigureCrucibleVolume { - disk_name, backend_id, new_vcr_json, result_tx, } => { let _ = result_tx.send( - self.reconfigure_crucible_volume( - disk_name, - &backend_id, - new_vcr_json, - ) - .await, + self.reconfigure_crucible_volume(&backend_id, new_vcr_json) + .await, ); HandleEventOutcome::Continue } @@ -660,19 +656,12 @@ impl StateDriver { async fn reconfigure_crucible_volume( &self, - disk_name: String, - backend_id: &Uuid, + backend_id: &SpecKey, new_vcr_json: String, ) -> super::CrucibleReplaceResult { info!(self.log, "request to replace Crucible VCR"; - "disk_name" => %disk_name, "backend_id" => %backend_id); - fn spec_element_not_found(disk_name: &str) -> dropshot::HttpError { - let msg = format!("Crucible backend for {:?} not found", disk_name); - dropshot::HttpError::for_not_found(Some(msg.clone()), msg) - } - let mut objects = self.objects.lock_exclusive().await; let backend = objects .crucible_backends() @@ -683,21 +672,28 @@ impl StateDriver { })? .clone(); - let Some(disk) = objects.instance_spec_mut().disks.get_mut(&disk_name) - else { - return Err(spec_element_not_found(&disk_name)); + let Some(disk) = objects.instance_spec_mut().disks.iter_mut().find( + |(_id, device)| device.device_spec.backend_id() == backend_id, + ) else { + let msg = format!("no disk in spec with backend ID {backend_id}"); + return Err(HttpError::for_not_found(Some(msg.clone()), msg)); }; let StorageBackend::Crucible(CrucibleStorageBackend { request_json: old_vcr_json, readonly, - }) = &disk.backend_spec + }) = &disk.1.backend_spec else { - return Err(spec_element_not_found(&disk_name)); + let msg = format!( + "disk {} has backend {backend_id} but its kind is {}", + disk.0, + disk.1.backend_spec.kind() + ); + return Err(HttpError::for_not_found(Some(msg.clone()), msg)); }; let replace_result = backend - .vcr_replace(old_vcr_json, &new_vcr_json) + .vcr_replace(old_vcr_json.as_str(), &new_vcr_json) .await .map_err(|e| { dropshot::HttpError::for_bad_request( @@ -706,10 +702,11 @@ impl StateDriver { ) })?; - disk.backend_spec = StorageBackend::Crucible(CrucibleStorageBackend { - readonly: *readonly, - request_json: new_vcr_json, - }); + disk.1.backend_spec = + StorageBackend::Crucible(CrucibleStorageBackend { + readonly: *readonly, + request_json: new_vcr_json, + }); info!(self.log, "replaced Crucible VCR"; "backend_id" => %backend_id); diff --git a/crates/propolis-api-types/Cargo.toml b/crates/propolis-api-types/Cargo.toml index 5deedb3dd..6a510015d 100644 --- a/crates/propolis-api-types/Cargo.toml +++ b/crates/propolis-api-types/Cargo.toml @@ -12,5 +12,6 @@ crucible-client-types.workspace = true propolis_types.workspace = true schemars.workspace = true serde.workspace = true +serde_with.workspace = true thiserror.workspace = true uuid.workspace = true diff --git a/crates/propolis-api-types/src/instance_spec/components/devices.rs b/crates/propolis-api-types/src/instance_spec/components/devices.rs index dcbb8d7e4..0c0c7e686 100644 --- a/crates/propolis-api-types/src/instance_spec/components/devices.rs +++ b/crates/propolis-api-types/src/instance_spec/components/devices.rs @@ -5,7 +5,7 @@ //! Device configuration data: components that define VM properties that are //! visible to a VM's guest software. -use crate::instance_spec::PciPath; +use crate::instance_spec::{PciPath, SpecKey}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize}; #[serde(deny_unknown_fields)] pub struct VirtioDisk { /// The name of the disk's backend component. - pub backend_name: String, + pub backend_id: SpecKey, /// The PCI bus/device/function at which this disk should be attached. pub pci_path: PciPath, @@ -25,7 +25,7 @@ pub struct VirtioDisk { #[serde(deny_unknown_fields)] pub struct NvmeDisk { /// The name of the disk's backend component. - pub backend_name: String, + pub backend_id: SpecKey, /// The PCI bus/device/function at which this disk should be attached. pub pci_path: PciPath, @@ -36,7 +36,7 @@ pub struct NvmeDisk { #[serde(deny_unknown_fields)] pub struct VirtioNic { /// The name of the device's backend. - pub backend_name: String, + pub backend_id: SpecKey, /// A caller-defined correlation identifier for this interface. If Propolis /// is configured to collect network interface kstats in its Oximeter @@ -115,13 +115,13 @@ pub struct BootSettings { } /// An entry in the boot order stored in a [`BootSettings`] component. -#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema, Default)] +#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] pub struct BootOrderEntry { - /// The name of another component in the spec that Propolis should try to + /// The ID of another component in the spec that Propolis should try to /// boot from. /// /// Currently, only disk device components are supported. - pub name: String, + pub id: SpecKey, } // @@ -150,7 +150,7 @@ pub struct SoftNpuPort { pub link_name: String, /// The name of the port's associated DLPI backend. - pub backend_name: String, + pub backend_id: SpecKey, } /// Describes a PCI device that shares host files with the guest using the P9 diff --git a/crates/propolis-api-types/src/instance_spec/mod.rs b/crates/propolis-api-types/src/instance_spec/mod.rs index 53b234f5a..868c20dd0 100644 --- a/crates/propolis-api-types/src/instance_spec/mod.rs +++ b/crates/propolis-api-types/src/instance_spec/mod.rs @@ -155,14 +155,94 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use serde_with::{DeserializeFromStr, SerializeDisplay}; pub use propolis_types::{CpuidIdent, CpuidValues, CpuidVendor, PciPath}; +use uuid::Uuid; pub mod components; pub mod v0; -/// Type alias for keys in the instance spec's maps. -type SpecKey = String; +/// A key identifying a component in an instance spec. +// +// Some of the components Omicron attaches to Propolis VMs, like network +// interfaces and Crucible disks, are described by database records with UUID +// primary keys. It's natural to reuse these UUIDs as component identifiers in +// Propolis, especially because it lets Omicron functions that need to identify +// a specific component (e.g. a specific Crucible backend that should handle a +// disk snapshot request) pass that component's ID directly to Propolis. +// +// In some cases it's not desirable or possible to use UUIDs this way: +// +// - Some components (like the cloud-init disk) don't have their own rows in the +// database and so don't have obvious UUIDs to use. +// - Some objects (like Crucible disks) require both a device and a backend +// component in the spec, and these can't share the same key. +// - Propolis users outside the control plane may not have any component UUIDs +// at all and may just want to use strings to identify all their components. +// +// For these reasons, the key type is a union of a UUID and a String. This +// allows the more compact, more-easily-compared UUID format to be used wherever +// it is practical while still allowing callers to use strings as names if they +// have no UUIDs available or the most obvious UUID is in use elsewhere. The key +// type's From impls will try to parse strings into UUIDs before storing keys as +// strings. +// +// This type derives `SerializeDisplay` and `DeserializeFromStr` so that it can +// be used as a map key when serializing to JSON, which requires strings (and +// not objects) as keys. +#[derive( + Clone, + Debug, + SerializeDisplay, + DeserializeFromStr, + Eq, + PartialEq, + Ord, + PartialOrd, + JsonSchema, +)] +pub enum SpecKey { + Uuid(Uuid), + Name(String), +} + +impl std::fmt::Display for SpecKey { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Uuid(uuid) => write!(f, "{uuid}"), + Self::Name(name) => write!(f, "{name}"), + } + } +} + +impl std::str::FromStr for SpecKey { + // This conversion is infallible, but the error type needs to implement + // `Display` for `SpecKey` to derive `DeserializeFromStr`. + type Err = &'static str; + + fn from_str(s: &str) -> Result { + Ok(match Uuid::parse_str(s) { + Ok(uuid) => Self::Uuid(uuid), + Err(_) => Self::Name(s.to_owned()), + }) + } +} + +impl From for SpecKey { + fn from(value: String) -> Self { + match Uuid::parse_str(value.as_str()) { + Ok(uuid) => Self::Uuid(uuid), + Err(_) => Self::Name(value), + } + } +} + +impl From for SpecKey { + fn from(value: Uuid) -> Self { + Self::Uuid(value) + } +} /// A versioned instance spec. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] diff --git a/crates/propolis-api-types/src/instance_spec/v0.rs b/crates/propolis-api-types/src/instance_spec/v0.rs index 9c49176d5..cf2f4717f 100644 --- a/crates/propolis-api-types/src/instance_spec/v0.rs +++ b/crates/propolis-api-types/src/instance_spec/v0.rs @@ -2,7 +2,7 @@ // 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/. -use std::collections::HashMap; +use std::collections::BTreeMap; use crate::instance_spec::{components, SpecKey}; use schemars::JsonSchema; @@ -34,5 +34,5 @@ pub enum ComponentV0 { #[serde(deny_unknown_fields)] pub struct InstanceSpecV0 { pub board: components::board::Board, - pub components: HashMap, + pub components: BTreeMap, } diff --git a/crates/propolis-api-types/src/lib.rs b/crates/propolis-api-types/src/lib.rs index f36eb0b14..4296be0ac 100644 --- a/crates/propolis-api-types/src/lib.rs +++ b/crates/propolis-api-types/src/lib.rs @@ -6,7 +6,7 @@ use std::{collections::BTreeMap, fmt, net::SocketAddr}; -use instance_spec::v0::InstanceSpecV0; +use instance_spec::{v0::InstanceSpecV0, SpecKey}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -27,7 +27,6 @@ pub mod instance_spec; #[derive(Clone, Deserialize, Serialize, JsonSchema)] pub struct InstanceVCRReplace { - pub name: String, pub vcr_json: String, } @@ -109,7 +108,7 @@ pub enum InstanceInitializationMethod { MigrationTarget { migration_id: Uuid, src_addr: SocketAddr, - replace_components: BTreeMap, + replace_components: BTreeMap, }, } @@ -342,18 +341,18 @@ pub enum InstanceSerialConsoleControlMessage { #[derive(Deserialize, JsonSchema)] pub struct SnapshotRequestPathParams { - pub id: Uuid, + pub id: String, pub snapshot_id: Uuid, } #[derive(Deserialize, JsonSchema)] pub struct VCRRequestPathParams { - pub id: Uuid, + pub id: String, } #[derive(Deserialize, JsonSchema)] pub struct VolumeStatusPathParams { - pub id: Uuid, + pub id: String, } #[derive(Debug, Serialize, Deserialize, JsonSchema)] From 171fac7a9098eca5f387545b0326400c5ffbca84 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 19 Nov 2024 00:19:09 +0000 Subject: [PATCH 2/7] 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 876d0a10e..9e79d005f 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(), }), ); From 1d7b68e81f1593a9b035a52cd35079246d05c753 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 19 Nov 2024 00:55:42 +0000 Subject: [PATCH 3/7] add some serialization/deserialization tests --- Cargo.lock | 1 + crates/propolis-api-types/Cargo.toml | 3 ++ .../src/instance_spec/mod.rs | 54 +++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index e6a2d2109..146e1f064 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4478,6 +4478,7 @@ dependencies = [ "propolis_types", "schemars", "serde", + "serde_json", "serde_with", "thiserror", "uuid", diff --git a/crates/propolis-api-types/Cargo.toml b/crates/propolis-api-types/Cargo.toml index 6a510015d..55874a4e8 100644 --- a/crates/propolis-api-types/Cargo.toml +++ b/crates/propolis-api-types/Cargo.toml @@ -15,3 +15,6 @@ serde.workspace = true serde_with.workspace = true thiserror.workspace = true uuid.workspace = true + +[dev-dependencies] +serde_json.workspace = true diff --git a/crates/propolis-api-types/src/instance_spec/mod.rs b/crates/propolis-api-types/src/instance_spec/mod.rs index 868c20dd0..927c4ac78 100644 --- a/crates/propolis-api-types/src/instance_spec/mod.rs +++ b/crates/propolis-api-types/src/instance_spec/mod.rs @@ -250,3 +250,57 @@ impl From for SpecKey { pub enum VersionedInstanceSpec { V0(v0::InstanceSpecV0), } + +#[cfg(test)] +mod test { + use std::collections::BTreeMap; + + use uuid::Uuid; + + use super::{components::devices::QemuPvpanic, v0::ComponentV0, SpecKey}; + + type TestMap = BTreeMap; + + // Verifies that UUID-type spec keys that are serialized and deserialized + // continue to be interpreted as UUID-type spec keys. + #[test] + fn spec_key_uuid_roundtrip() { + let id = Uuid::new_v4(); + let mut map = TestMap::new(); + map.insert( + SpecKey::Uuid(id), + ComponentV0::QemuPvpanic(QemuPvpanic { enable_isa: true }), + ); + + let ser = serde_json::to_string(&map).unwrap(); + let unser: TestMap = serde_json::from_str(&ser).unwrap(); + let key = unser.keys().next().expect("one key in the map"); + let SpecKey::Uuid(got_id) = key else { + panic!("expected SpecKey::Uuid, got {}", key); + }; + + assert_eq!(*got_id, id); + } + + // Verifies that serializing a name-type spec key that happens to be the + // string representation of a UUID causes the key to deserialize as a + // UUID-type key. + #[test] + fn spec_key_uuid_string_deserializes_as_uuid_variant() { + let id = Uuid::new_v4(); + let mut map = TestMap::new(); + map.insert( + SpecKey::Name(id.to_string()), + ComponentV0::QemuPvpanic(QemuPvpanic { enable_isa: true }), + ); + + let ser = serde_json::to_string(&map).unwrap(); + let unser: TestMap = serde_json::from_str(&ser).unwrap(); + let key = unser.keys().next().expect("one key in the map"); + let SpecKey::Uuid(got_id) = key else { + panic!("expected SpecKey::Uuid, got {}", key); + }; + + assert_eq!(*got_id, id); + } +} From 152ccdfeaf6c6cd1a86f0594b66eef9d295ab450 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Mon, 2 Dec 2024 22:26:17 +0000 Subject: [PATCH 4/7] clean up terminology --- crates/propolis-api-types/src/instance_spec/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/propolis-api-types/src/instance_spec/mod.rs b/crates/propolis-api-types/src/instance_spec/mod.rs index 927c4ac78..9cef19d53 100644 --- a/crates/propolis-api-types/src/instance_spec/mod.rs +++ b/crates/propolis-api-types/src/instance_spec/mod.rs @@ -181,12 +181,12 @@ pub mod v0; // - Propolis users outside the control plane may not have any component UUIDs // at all and may just want to use strings to identify all their components. // -// For these reasons, the key type is a union of a UUID and a String. This -// allows the more compact, more-easily-compared UUID format to be used wherever -// it is practical while still allowing callers to use strings as names if they -// have no UUIDs available or the most obvious UUID is in use elsewhere. The key -// type's From impls will try to parse strings into UUIDs before storing keys as -// strings. +// For these reasons, the key type may be represented as either a UUID or a +// String. This allows the more compact, more-easily-compared UUID format to be +// used wherever it is practical while still allowing callers to use strings as +// names if they have no UUIDs available or the most obvious UUID is in use +// elsewhere. The key type's From impls will try to parse strings into UUIDs +// before storing keys as strings. // // This type derives `SerializeDisplay` and `DeserializeFromStr` so that it can // be used as a map key when serializing to JSON, which requires strings (and From 12bd6c7d0e7a6425fe103bef5bf7faadec9b42a1 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 3 Dec 2024 17:19:20 +0000 Subject: [PATCH 5/7] return to using get_uuid for stats ID --- bin/propolis-server/src/lib/initializer.rs | 46 ++++++++++++++-------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index eab11ccb2..583c441be 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -649,8 +649,21 @@ impl<'a> MachineInitializer<'a> { }; if let Some(crucible) = crucible { - let block_size = crucible.block_size().await; - let Some(block_size) = block_size else { + let crucible = + match self.crucible_backends.entry(backend_id.clone()) { + std::collections::btree_map::Entry::Occupied(_) => { + return Err( + MachineInitError::DuplicateCrucibleBackendId( + backend_id.clone(), + ), + ); + } + std::collections::btree_map::Entry::Vacant(e) => { + e.insert(crucible) + } + }; + + let Some(block_size) = crucible.block_size().await else { slog::error!( self.log, "Could not get Crucible backend block size, \ @@ -660,32 +673,31 @@ impl<'a> MachineInitializer<'a> { continue; }; - let prev = - self.crucible_backends.insert(backend_id.clone(), crucible); - if prev.is_some() { - return Err(MachineInitError::DuplicateCrucibleBackendId( - backend_id.clone(), - )); - } + let Ok(volume_id) = crucible.get_uuid().await else { + slog::error!( + self.log, + "Could not get Crucible volume ID, \ + virtual disk metrics can't be reported for it"; + "disk_id" => %backend_id, + ); + continue; + }; - // If metrics are enabled and this Crucible backend was - // identified with a UUID-format spec key, register this disk - // for metrics collection, using the key as the disk ID. - if let (Some(registry), SpecKey::Uuid(disk_id)) = - (&self.producer_registry, &backend_id) - { + if let Some(registry) = &self.producer_registry { let stats = VirtualDiskProducer::new( block_size, self.properties.id, - *disk_id, + volume_id, &self.properties.metadata, ); + if let Err(e) = registry.register_producer(stats.clone()) { slog::error!( self.log, "Could not register virtual disk producer, \ metrics will not be produced"; - "disk_id" => %disk_id, + "disk_id" => %backend_id, + "volume_id" => %volume_id, "error" => ?e, ); continue; From 857ae937e9d33e6866be78e226a4a3c2a35194fb Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 3 Dec 2024 18:17:32 +0000 Subject: [PATCH 6/7] take serial numbers in NVMe specs --- bin/propolis-cli/src/main.rs | 2 ++ bin/propolis-server/src/lib/initializer.rs | 8 ++++- bin/propolis-standalone/src/main.rs | 9 +++++- .../src/instance_spec/components/devices.rs | 4 +++ crates/propolis-config-toml/src/spec.rs | 9 ++++-- lib/propolis-client/src/support.rs | 29 +++++++++++++++++++ lib/propolis/src/hw/nvme/mod.rs | 8 ++--- openapi/propolis-server.json | 14 ++++++++- phd-tests/framework/src/disk/mod.rs | 4 +++ phd-tests/framework/src/test_vm/config.rs | 25 ++++++++++++++++ 10 files changed, 100 insertions(+), 12 deletions(-) diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 5b9735658..940e21b6b 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -16,6 +16,7 @@ use anyhow::{anyhow, Context}; use clap::{Args, Parser, Subcommand}; use futures::{future, SinkExt}; use newtype_uuid::{GenericUuid, TypedUuid, TypedUuidKind, TypedUuidTag}; +use propolis_client::support::nvme_serial_from_str; use propolis_client::types::{ BlobStorageBackend, Board, Chipset, ComponentV0, CrucibleStorageBackend, I440Fx, InstanceEnsureRequest, InstanceInitializationMethod, @@ -241,6 +242,7 @@ impl DiskRequest { "nvme" => ComponentV0::NvmeDisk(NvmeDisk { backend_id: backend_id.clone(), pci_path, + serial_number: nvme_serial_from_str(&self.name, b' '), }), _ => anyhow::bail!( "invalid device type in disk request: {:?}", diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 583c441be..666e0f549 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -633,11 +633,17 @@ impl<'a> MachineInitializer<'a> { vioblk } DeviceInterface::Nvme => { + let spec::StorageDevice::Nvme(nvme_spec) = + &disk.device_spec + else { + unreachable!("disk is known to be an NVMe disk"); + }; + // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); let component = format!("nvme-{}", device_id); let nvme = nvme::PciNvme::create( - device_id.to_string(), + &nvme_spec.serial_number, mdts, self.log.new(slog::o!("component" => component)), ); diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs index 702fb409f..4010bb096 100644 --- a/bin/propolis-standalone/src/main.rs +++ b/bin/propolis-standalone/src/main.rs @@ -1230,7 +1230,14 @@ fn setup_instance( log.new(slog::o!("dev" => format!("nvme-{}", name))); // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); - let nvme = hw::nvme::PciNvme::create(dev_serial, mdts, log); + + let mut serial_number = [0u8; 20]; + let sz = dev_serial.len().min(20); + serial_number[..sz] + .clone_from_slice(&dev_serial.as_bytes()[..sz]); + + let nvme = + hw::nvme::PciNvme::create(&serial_number, mdts, log); guard.inventory.register_instance(&nvme, &bdf.to_string()); guard.inventory.register_block(&backend, name); diff --git a/crates/propolis-api-types/src/instance_spec/components/devices.rs b/crates/propolis-api-types/src/instance_spec/components/devices.rs index 0c0c7e686..9dc86d031 100644 --- a/crates/propolis-api-types/src/instance_spec/components/devices.rs +++ b/crates/propolis-api-types/src/instance_spec/components/devices.rs @@ -29,6 +29,10 @@ pub struct NvmeDisk { /// The PCI bus/device/function at which this disk should be attached. pub pci_path: PciPath, + + /// The serial number to return in response to an NVMe Identify Controller + /// command. + pub serial_number: [u8; 20], } /// A network card that presents a virtio-net interface to the guest. diff --git a/crates/propolis-config-toml/src/spec.rs b/crates/propolis-config-toml/src/spec.rs index 07954c326..08baa7ee3 100644 --- a/crates/propolis-config-toml/src/spec.rs +++ b/crates/propolis-config-toml/src/spec.rs @@ -10,6 +10,7 @@ use std::{ }; use propolis_client::{ + support::nvme_serial_from_str, types::{ ComponentV0, DlpiNetworkBackend, FileStorageBackend, MigrationFailureInjector, NvmeDisk, P9fs, PciPciBridge, SoftNpuP9, @@ -289,9 +290,11 @@ fn parse_storage_device_from_config( Interface::Virtio => { ComponentV0::VirtioDisk(VirtioDisk { backend_id, pci_path }) } - Interface::Nvme => { - ComponentV0::NvmeDisk(NvmeDisk { backend_id, pci_path }) - } + Interface::Nvme => ComponentV0::NvmeDisk(NvmeDisk { + backend_id, + pci_path, + serial_number: nvme_serial_from_str(name, b' '), + }), }, id_to_return, )) diff --git a/lib/propolis-client/src/support.rs b/lib/propolis-client/src/support.rs index 228d309f3..071982d68 100644 --- a/lib/propolis-client/src/support.rs +++ b/lib/propolis-client/src/support.rs @@ -27,6 +27,18 @@ impl Default for Chipset { } } +/// Generates a 20-byte NVMe device serial number from the bytes in a string +/// slice. If the slice is too short to populate the entire serial number, the +/// remaining bytes are filled with `pad`. +pub fn nvme_serial_from_str(s: &str, pad: u8) -> [u8; 20] { + let mut sn = [0u8; 20]; + + let bytes_from_slice = sn.len().min(s.len()); + sn[..bytes_from_slice].copy_from_slice(&s.as_bytes()[..bytes_from_slice]); + sn[bytes_from_slice..].fill(pad); + sn +} + /// Clone of `InstanceSerialConsoleControlMessage` type defined in /// `propolis_api_types`, with which this must be kept in sync. /// @@ -572,4 +584,21 @@ mod test { { WebSocketStream::from_raw_socket(conn, Role::Server, None).await } + + #[test] + fn test_nvme_serial_from_str() { + use super::nvme_serial_from_str; + + let expected = b"hello world "; + assert_eq!(nvme_serial_from_str("hello world", b' '), *expected); + + let expected = b"enthusiasm!!!!!!!!!!"; + assert_eq!(nvme_serial_from_str("enthusiasm", b'!'), *expected); + + let expected = b"very_long_disk_name_"; + assert_eq!( + nvme_serial_from_str("very_long_disk_name_goes_here", b'?'), + *expected + ); + } } diff --git a/lib/propolis/src/hw/nvme/mod.rs b/lib/propolis/src/hw/nvme/mod.rs index 58450adc3..572291d05 100644 --- a/lib/propolis/src/hw/nvme/mod.rs +++ b/lib/propolis/src/hw/nvme/mod.rs @@ -505,7 +505,7 @@ pub struct PciNvme { impl PciNvme { /// Create a new pci-nvme device with the given values pub fn create( - serial_number: String, + serial_number: &[u8; 20], mdts: Option, log: slog::Logger, ) -> Arc { @@ -527,16 +527,12 @@ impl PciNvme { let cqes = size_of::().trailing_zeros() as u8; let sqes = size_of::().trailing_zeros() as u8; - let sz = std::cmp::min(20, serial_number.len()); - let mut sn: [u8; 20] = [0u8; 20]; - sn[..sz].clone_from_slice(&serial_number.as_bytes()[..sz]); - // Initialize the Identify structure returned when the host issues // an Identify Controller command. let ctrl_ident = bits::IdentifyController { vid: VENDOR_OXIDE, ssvid: VENDOR_OXIDE, - sn, + sn: *serial_number, ieee: OXIDE_OUI, mdts: mdts.unwrap_or(0), // We use standard Completion/Submission Queue Entry structures with no extra diff --git a/openapi/propolis-server.json b/openapi/propolis-server.json index 9e79d005f..4e23f600c 100644 --- a/openapi/propolis-server.json +++ b/openapi/propolis-server.json @@ -1513,11 +1513,23 @@ "$ref": "#/components/schemas/PciPath" } ] + }, + "serial_number": { + "description": "The serial number to return in response to an NVMe Identify Controller command.", + "type": "array", + "items": { + "type": "integer", + "format": "uint8", + "minimum": 0 + }, + "minItems": 20, + "maxItems": 20 } }, "required": [ "backend_id", - "pci_path" + "pci_path", + "serial_number" ], "additionalProperties": false }, diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index a0ca5c506..9458f4cd2 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/phd-tests/framework/src/disk/mod.rs @@ -95,6 +95,10 @@ impl DeviceName { pub fn into_string(self) -> String { self.0 } + + pub fn as_str(&self) -> &str { + self.0.as_str() + } } /// The name for a backend implementing storage for a disk. This is derived diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 1ea5ec14b..61d64a5b2 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use anyhow::Context; use cpuid_utils::CpuidIdent; use propolis_client::{ + support::nvme_serial_from_str, types::{ Board, BootOrderEntry, BootSettings, Chipset, ComponentV0, Cpuid, CpuidEntry, CpuidVendor, InstanceMetadata, InstanceSpecV0, @@ -313,6 +314,30 @@ impl<'dr> VmConfig<'dr> { backend_name.clone().into_string(), ), pci_path, + serial_number: nvme_serial_from_str( + device_name.as_str(), + // TODO(#790): Propolis's NVMe controller used to pad + // serial numbers with 0 bytes by default. This causes + // disk serial numbers to appear in the guest to be + // null-terminated strings. This, in turn, affects (or + // at least may affect) how guest firmware images + // identify disks when determining a VM's boot order: a + // disk whose serial number is padded to 20 bytes with + // spaces may not match a disk whose serial number was + // padded with \0 bytes. + // + // Unfortunately for us, guest firmware may persist disk + // identifiers to a VM's nonvolatile EFI variable store. + // This means that changing from zero-padded to + // space-padded serial numbers may break an existing + // VM's saved boot order. + // + // Until we decide whether and how to preserve + // compatibility for existing VMs that may have + // preserved a zero-padded disk ID, continue to zero-pad + // disk IDs in PHD to match previous behavior. + 0, + ), }), }; From 9a291d3e35147cb79679558dd4b350bfeab550fd Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 3 Dec 2024 20:52:05 +0000 Subject: [PATCH 7/7] rearrange explanatory comments --- lib/propolis-client/src/support.rs | 16 +++++++++++++++ phd-tests/framework/src/test_vm/config.rs | 25 +++++------------------ 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/propolis-client/src/support.rs b/lib/propolis-client/src/support.rs index 071982d68..1f47da8c1 100644 --- a/lib/propolis-client/src/support.rs +++ b/lib/propolis-client/src/support.rs @@ -30,6 +30,19 @@ impl Default for Chipset { /// Generates a 20-byte NVMe device serial number from the bytes in a string /// slice. If the slice is too short to populate the entire serial number, the /// remaining bytes are filled with `pad`. +/// +/// NOTE: Version 1.2.1 of the NVMe specification (June 5, 2016) specifies in +/// section 1.5 that ASCII data fields, including serial numbers, must be +/// left-justified and must use 0x20 bytes (spaces) as the padding value. This +/// function allows callers to choose a non-0x20 padding value to preserve the +/// serial numbers for existing disks, which serial numbers may have been used +/// previously and persisted into a VM's nonvolatile EFI variables (such as its +/// boot order variables). +// +// TODO(#790): Ideally, this routine would have no `pad` parameter at all and +// would always pad with spaces, but whether this is ultimately possible depends +// on whether Omicron can start space-padding serial numbers for disks that were +// attached to a Propolis VM that zero-padded their serial numbers. pub fn nvme_serial_from_str(s: &str, pad: u8) -> [u8; 20] { let mut sn = [0u8; 20]; @@ -600,5 +613,8 @@ mod test { nvme_serial_from_str("very_long_disk_name_goes_here", b'?'), *expected ); + + let expected = b"nonvolatile EFI\0\0\0\0\0"; + assert_eq!(nvme_serial_from_str("nonvolatile EFI", 0), *expected); } } diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 61d64a5b2..752c25178 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -316,26 +316,11 @@ impl<'dr> VmConfig<'dr> { pci_path, serial_number: nvme_serial_from_str( device_name.as_str(), - // TODO(#790): Propolis's NVMe controller used to pad - // serial numbers with 0 bytes by default. This causes - // disk serial numbers to appear in the guest to be - // null-terminated strings. This, in turn, affects (or - // at least may affect) how guest firmware images - // identify disks when determining a VM's boot order: a - // disk whose serial number is padded to 20 bytes with - // spaces may not match a disk whose serial number was - // padded with \0 bytes. - // - // Unfortunately for us, guest firmware may persist disk - // identifiers to a VM's nonvolatile EFI variable store. - // This means that changing from zero-padded to - // space-padded serial numbers may break an existing - // VM's saved boot order. - // - // Until we decide whether and how to preserve - // compatibility for existing VMs that may have - // preserved a zero-padded disk ID, continue to zero-pad - // disk IDs in PHD to match previous behavior. + // Omicron supplies (or will supply, as of this writing) + // 0 as the padding byte to maintain compatibility for + // existing disks. Match that behavior here so that PHD + // and Omicron VM configurations are as similar as + // possible. 0, ), }),