From d4529fd8247386b422b78e1203315d5baea5ea8b Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 18 Dec 2024 10:46:46 -0800 Subject: [PATCH] api: allow use of UUIDs as component IDs in instance specs (#816) Change the map key type in instance specs from String to a SpecKey enum whose variants are Uuid and Name(String). This type serializes as a string (so it can be used as a JSON dictionary key) and will opportunistically deserialize into the Uuid variant. This allows the control plane to use UUIDs to identify VM objects wherever it makes sense while allowing it (and other users) to continue to use strings as component IDs (e.g. because no UUID is available or because the relevant UUID is being used by some other spec element). Index Propolis servers' VM objects using their SpecKeys. Note that the CrucibleBackendMap used to use UUIDs as keys, obtaining these from `Volume::get_uuid` after instantiating a Crucible upstairs. This UUID happens to be the control plane disk UUID (because of the way the control plane constructs its volume records); the intention is that Omicron will use disk IDs as its backend IDs so that its existing calls to Propolis's snapshot and volume-replace endpoints will Just Work. Change NVMe disk specifications to specify disk serial numbers explicitly instead of inferring them from a disk name or spec key. The intent is to use spec keys only to identify components and not to determine how they behave or how guests perceive them. --- Cargo.lock | 10 +- Cargo.toml | 1 + bin/propolis-cli/src/main.rs | 52 ++--- bin/propolis-server/src/lib/initializer.rs | 189 +++++++++++------- .../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 +++-- bin/propolis-standalone/src/main.rs | 9 +- crates/propolis-api-types/Cargo.toml | 4 + .../src/instance_spec/components/devices.rs | 20 +- .../src/instance_spec/mod.rs | 138 ++++++++++++- .../src/instance_spec/v0.rs | 4 +- crates/propolis-api-types/src/lib.rs | 11 +- crates/propolis-config-toml/src/spec.rs | 54 ++--- lib/propolis-client/src/lib.rs | 7 +- lib/propolis-client/src/support.rs | 45 +++++ lib/propolis/src/hw/nvme/mod.rs | 8 +- openapi/propolis-server.json | 109 +++++++--- phd-tests/framework/src/disk/mod.rs | 4 + phd-tests/framework/src/test_vm/config.rs | 24 ++- 28 files changed, 742 insertions(+), 413 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 30c378b60..146e1f064 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4478,6 +4478,8 @@ dependencies = [ "propolis_types", "schemars", "serde", + "serde_json", + "serde_with", "thiserror", "uuid", ] @@ -5383,9 +5385,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 +5403,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-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 543bb6b82..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, @@ -23,7 +24,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 +193,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 +215,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,18 +230,19 @@ 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, + serial_number: nvme_serial_from_str(&self.name, b' '), }), _ => anyhow::bail!( "invalid device type in disk request: {:?}", @@ -256,9 +258,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 +316,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 +340,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 +366,7 @@ impl VmConfig { ] { add_component_to_spec( &mut spec, - name.to_owned(), + SpecKey::Name(name.to_owned()), ComponentV0::SerialPort(SerialPort { num: port }), )?; } @@ -375,7 +379,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 +388,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 +729,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/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 5bb3213ee..666e0f549 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,74 +617,93 @@ 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 } 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-{}", disk_name); + let component = format!("nvme-{}", device_id); let nvme = nvme::PciNvme::create( - disk_name.to_owned(), + &nvme_spec.serial_number, 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 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) = block_size else { + let Some(block_size) = crucible.block_size().await 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; + }; + + 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; }; - // 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 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; @@ -749,8 +775,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 +829,7 @@ impl<'a> MachineInitializer<'a> { }, ); - self.devices.insert(mig.name.clone(), dev); + self.devices.insert(mig.id.clone(), dev); } } @@ -831,7 +856,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 +868,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 +885,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 +907,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 +936,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 +1134,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 +1154,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 +1169,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 +1230,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 +1267,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/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/Cargo.toml b/crates/propolis-api-types/Cargo.toml index 5deedb3dd..55874a4e8 100644 --- a/crates/propolis-api-types/Cargo.toml +++ b/crates/propolis-api-types/Cargo.toml @@ -12,5 +12,9 @@ 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 + +[dev-dependencies] +serde_json.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..9dc86d031 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,10 +25,14 @@ 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, + + /// 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. @@ -36,7 +40,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 +119,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 +154,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..9cef19d53 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 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 +// 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)] @@ -170,3 +250,57 @@ type SpecKey = String; 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); + } +} 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)] diff --git a/crates/propolis-config-toml/src/spec.rs b/crates/propolis-config-toml/src/spec.rs index 73584afdf..08baa7ee3 100644 --- a/crates/propolis-config-toml/src/spec.rs +++ b/crates/propolis-config-toml/src/spec.rs @@ -10,20 +10,19 @@ use std::{ }; use propolis_client::{ + support::nvme_serial_from_str, types::{ ComponentV0, DlpiNetworkBackend, FileStorageBackend, MigrationFailureInjector, NvmeDisk, P9fs, PciPciBridge, SoftNpuP9, 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 +110,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 +176,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 +234,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,31 +266,35 @@ 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 }) - } - Interface::Nvme => { - ComponentV0::NvmeDisk(NvmeDisk { backend_name, pci_path }) + ComponentV0::VirtioDisk(VirtioDisk { backend_id, pci_path }) } + Interface::Nvme => ComponentV0::NvmeDisk(NvmeDisk { + backend_id, + pci_path, + serial_number: nvme_serial_from_str(name, b' '), + }), }, id_to_return, )) @@ -356,10 +360,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/lib/propolis-client/src/support.rs b/lib/propolis-client/src/support.rs index 228d309f3..1f47da8c1 100644 --- a/lib/propolis-client/src/support.rs +++ b/lib/propolis-client/src/support.rs @@ -27,6 +27,31 @@ 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]; + + 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 +597,24 @@ 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 + ); + + let expected = b"nonvolatile EFI\0\0\0\0\0"; + assert_eq!(nvme_serial_from_str("nonvolatile EFI", 0), *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 876d0a10e..4e23f600c 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.", @@ -1512,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_name", - "pci_path" + "backend_id", + "pci_path", + "serial_number" ], "additionalProperties": false }, @@ -1759,9 +1772,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 +1786,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 +1849,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 +1867,7 @@ } }, "required": [ - "backend_name", + "backend_id", "pci_path" ], "additionalProperties": false @@ -1839,9 +1890,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 +1913,7 @@ } }, "required": [ - "backend_name", + "backend_id", "interface_id", "pci_path" ], 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 9c3497dbd..752c25178 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -7,13 +7,14 @@ 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, MigrationFailureInjector, NvmeDisk, SerialPort, SerialPortNumber, VirtioDisk, }, - PciPath, + PciPath, SpecKey, }; use uuid::Uuid; @@ -303,12 +304,25 @@ 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, + serial_number: nvme_serial_from_str( + device_name.as_str(), + // 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, + ), }), }; @@ -333,7 +347,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(), }), );