From b2781933119fbbabf8282eacd62e80792874a233 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 16 Aug 2024 13:49:21 -0700 Subject: [PATCH] instance spec refactor: move some impls out of api_types (#741) Move the server instance spec builder out of propolis_api_types and into propolis-server. Do the same for the `pci_path` associated functions on instance spec types. Also patch up a couple of error variants that weren't using debug format specifiers for strings. Tests: cargo test, PHD. --- .../propolis-server/src/lib/spec}/builder.rs | 127 +++++++++++------- .../src/lib/spec/config_toml.rs | 6 +- bin/propolis-server/src/lib/spec/mod.rs | 117 ++++++++-------- .../src/instance_spec/v0/mod.rs | 21 +-- 4 files changed, 132 insertions(+), 139 deletions(-) rename {crates/propolis-api-types/src/instance_spec/v0 => bin/propolis-server/src/lib/spec}/builder.rs (76%) diff --git a/crates/propolis-api-types/src/instance_spec/v0/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs similarity index 76% rename from crates/propolis-api-types/src/instance_spec/v0/builder.rs rename to bin/propolis-server/src/lib/spec/builder.rs index 14765f12f..fb3871ee1 100644 --- a/crates/propolis-api-types/src/instance_spec/v0/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -2,16 +2,33 @@ // 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/. -//! A builder for V0 instance specs. +//! A builder for instance specs. use std::collections::BTreeSet; -use crate::instance_spec::{components, v0::*, PciPath}; +use propolis_api_types::instance_spec::{ + components::{ + board::Board, + devices::{PciPciBridge, QemuPvpanic, SerialPort, SerialPortNumber}, + }, + v0::{ + DeviceSpecV0, InstanceSpecV0, NetworkBackendV0, NetworkDeviceV0, + StorageBackendV0, StorageDeviceV0, + }, + PciPath, +}; use thiserror::Error; +#[cfg(feature = "falcon")] +use propolis_api_types::instance_spec::components::{ + backends::DlpiNetworkBackend, + devices::{P9fs, SoftNpuP9, SoftNpuPciPort, SoftNpuPort}, +}; + /// Errors that can arise while building an instance spec from component parts. +#[allow(clippy::enum_variant_names)] #[derive(Debug, Error)] -pub enum SpecBuilderError { +pub(crate) enum SpecBuilderError { #[error("A device with name {0} already exists")] DeviceNameInUse(String), @@ -22,35 +39,45 @@ pub enum SpecBuilderError { PciPathInUse(PciPath), #[error("Serial port {0:?} is already specified")] - SerialPortInUse(components::devices::SerialPortNumber), + SerialPortInUse(SerialPortNumber), + #[cfg(feature = "falcon")] #[error("SoftNpu port {0:?} is already specified")] SoftNpuPortInUse(String), } -/// A builder that constructs instance specs incrementally and catches basic -/// errors, such as specifying duplicate component names or specifying multiple -/// devices with the same PCI path. -pub struct SpecBuilder { +pub(super) struct SpecBuilder { spec: InstanceSpecV0, pci_paths: BTreeSet, } -impl SpecBuilder { - /// Creates a new instance spec with the supplied board configuration. - pub fn new(cpus: u8, memory_mb: u64, enable_pcie: bool) -> Self { - let board = components::board::Board { - cpus, - memory_mb, - chipset: components::board::Chipset::I440Fx( - components::board::I440Fx { enable_pcie }, - ), - }; +trait PciComponent { + fn pci_path(&self) -> PciPath; +} + +impl PciComponent for StorageDeviceV0 { + fn pci_path(&self) -> PciPath { + match self { + StorageDeviceV0::VirtioDisk(disk) => disk.pci_path, + StorageDeviceV0::NvmeDisk(disk) => disk.pci_path, + } + } +} + +impl PciComponent for NetworkDeviceV0 { + fn pci_path(&self) -> PciPath { + match self { + NetworkDeviceV0::VirtioNic(nic) => nic.pci_path, + } + } +} +impl SpecBuilder { + pub(super) fn new(board: Board) -> Self { Self { spec: InstanceSpecV0 { devices: DeviceSpecV0 { board, ..Default::default() }, - ..Default::default() + backends: Default::default(), }, pci_paths: Default::default(), } @@ -71,7 +98,7 @@ impl SpecBuilder { } /// Adds a storage device with an associated backend. - pub fn add_storage_device( + pub(super) fn add_storage_device( &mut self, device_name: String, device_spec: StorageDeviceV0, @@ -135,7 +162,7 @@ impl SpecBuilder { pub fn add_pci_bridge( &mut self, bridge_name: String, - bridge_spec: components::devices::PciPciBridge, + bridge_spec: PciPciBridge, ) -> Result<&Self, SpecBuilderError> { if self.spec.devices.pci_pci_bridges.contains_key(&bridge_name) { return Err(SpecBuilderError::DeviceNameInUse(bridge_name)); @@ -152,7 +179,7 @@ impl SpecBuilder { /// Adds a serial port. pub fn add_serial_port( &mut self, - port: components::devices::SerialPortNumber, + port: SerialPortNumber, ) -> Result<&Self, SpecBuilderError> { if self .spec @@ -160,13 +187,13 @@ impl SpecBuilder { .serial_ports .insert( match port { - components::devices::SerialPortNumber::Com1 => "com1", - components::devices::SerialPortNumber::Com2 => "com2", - components::devices::SerialPortNumber::Com3 => "com3", - components::devices::SerialPortNumber::Com4 => "com4", + SerialPortNumber::Com1 => "com1", + SerialPortNumber::Com2 => "com2", + SerialPortNumber::Com3 => "com3", + SerialPortNumber::Com4 => "com4", } .to_string(), - components::devices::SerialPort { num: port }, + SerialPort { num: port }, ) .is_some() { @@ -176,10 +203,9 @@ impl SpecBuilder { } } - /// Adds a QEMU pvpanic device. pub fn add_pvpanic_device( &mut self, - pvpanic: components::devices::QemuPvpanic, + pvpanic: QemuPvpanic, ) -> Result<&Self, SpecBuilderError> { if self.spec.devices.qemu_pvpanic.is_some() { return Err(SpecBuilderError::DeviceNameInUse( @@ -195,22 +221,39 @@ impl SpecBuilder { #[cfg(feature = "falcon")] pub fn set_softnpu_pci_port( &mut self, - pci_port: components::devices::SoftNpuPciPort, + pci_port: SoftNpuPciPort, ) -> Result<&Self, SpecBuilderError> { self.register_pci_device(pci_port.pci_path)?; self.spec.devices.softnpu_pci_port = Some(pci_port); Ok(self) } + #[cfg(feature = "falcon")] + pub fn set_softnpu_p9( + &mut self, + p9: SoftNpuP9, + ) -> Result<&Self, SpecBuilderError> { + self.register_pci_device(p9.pci_path)?; + self.spec.devices.softnpu_p9 = Some(p9); + Ok(self) + } + + #[cfg(feature = "falcon")] + pub fn set_p9fs(&mut self, p9fs: P9fs) -> Result<&Self, SpecBuilderError> { + self.register_pci_device(p9fs.pci_path)?; + self.spec.devices.p9fs = Some(p9fs); + Ok(self) + } + #[cfg(feature = "falcon")] pub fn add_softnpu_port( &mut self, key: String, - port: components::devices::SoftNpuPort, + port: SoftNpuPort, ) -> Result<&Self, SpecBuilderError> { let _old = self.spec.backends.network_backends.insert( port.backend_name.clone(), - NetworkBackendV0::Dlpi(components::backends::DlpiNetworkBackend { + NetworkBackendV0::Dlpi(DlpiNetworkBackend { vnic_name: port.backend_name.clone(), }), ); @@ -222,26 +265,6 @@ impl SpecBuilder { } } - #[cfg(feature = "falcon")] - pub fn set_softnpu_p9( - &mut self, - p9: components::devices::SoftNpuP9, - ) -> Result<&Self, SpecBuilderError> { - self.register_pci_device(p9.pci_path)?; - self.spec.devices.softnpu_p9 = Some(p9); - Ok(self) - } - - #[cfg(feature = "falcon")] - pub fn set_p9fs( - &mut self, - p9fs: components::devices::P9fs, - ) -> Result<&Self, SpecBuilderError> { - self.register_pci_device(p9fs.pci_path)?; - self.spec.devices.p9fs = Some(p9fs); - Ok(self) - } - /// Yields the completed spec, consuming the builder. pub fn finish(self) -> InstanceSpecV0 { self.spec diff --git a/bin/propolis-server/src/lib/spec/config_toml.rs b/bin/propolis-server/src/lib/spec/config_toml.rs index 0e8c3e129..0163bd914 100644 --- a/bin/propolis-server/src/lib/spec/config_toml.rs +++ b/bin/propolis-server/src/lib/spec/config_toml.rs @@ -26,7 +26,7 @@ use propolis_api_types::instance_spec::components::devices::{ use crate::config; #[derive(Debug, Error)] -pub enum ConfigTomlError { +pub(crate) enum ConfigTomlError { #[error("unrecognized device type {0:?}")] UnrecognizedDeviceType(String), @@ -39,13 +39,13 @@ pub enum ConfigTomlError { #[error("failed to parse PCI path string {0:?}")] PciPathParseFailed(String, #[source] std::io::Error), - #[error("invalid storage device kind {kind:?} for device {name}")] + #[error("invalid storage device kind {kind:?} for device {name:?}")] InvalidStorageDeviceType { kind: String, name: String }, #[error("no backend name for storage device {0:?}")] NoBackendNameForStorageDevice(String), - #[error("invalid storage backend kind {kind:?} for backend {name}")] + #[error("invalid storage backend kind {kind:?} for backend {name:?}")] InvalidStorageBackendType { kind: String, name: String }, #[error("couldn't get path for file backend {0:?}")] diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index a8c70090a..a13bf7501 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -5,27 +5,31 @@ //! Helper functions for building instance specs from server parameters. use crate::config; +use builder::SpecBuilder; use config_toml::ConfigTomlError; +use propolis_api_types::instance_spec::components::backends::{ + BlobStorageBackend, CrucibleStorageBackend, VirtioNetworkBackend, +}; +use propolis_api_types::instance_spec::components::board::{Chipset, I440Fx}; +use propolis_api_types::instance_spec::components::devices::{ + NvmeDisk, QemuPvpanic, SerialPortNumber, VirtioDisk, VirtioNic, +}; use propolis_api_types::instance_spec::{ - components, - v0::{ - builder::{SpecBuilder, SpecBuilderError}, - *, - }, - PciPath, + components::board::Board, v0::*, PciPath, }; use propolis_api_types::{ self as api, DiskRequest, InstanceProperties, NetworkInterfaceRequest, }; use thiserror::Error; +mod builder; mod config_toml; /// Errors that can occur while building an instance spec from component parts. #[derive(Debug, Error)] -pub enum ServerSpecBuilderError { +pub(crate) enum ServerSpecBuilderError { #[error(transparent)] - InnerBuilderError(#[from] SpecBuilderError), + InnerBuilderError(#[from] builder::SpecBuilderError), #[error( "Could not translate PCI slot {0} for device type {1:?} to a PCI path" @@ -115,12 +119,13 @@ impl ServerSpecBuilder { }, )?; - let mut builder = - SpecBuilder::new(properties.vcpus, properties.memory, enable_pcie); + let mut builder = SpecBuilder::new(Board { + cpus: properties.vcpus, + memory_mb: properties.memory, + chipset: Chipset::I440Fx(I440Fx { enable_pcie }), + }); - builder.add_pvpanic_device(components::devices::QemuPvpanic { - enable_isa: true, - })?; + builder.add_pvpanic_device(QemuPvpanic { enable_isa: true })?; Ok(Self { builder }) } @@ -133,17 +138,14 @@ impl ServerSpecBuilder { ) -> Result<(), ServerSpecBuilderError> { let pci_path = slot_to_pci_path(nic.slot, SlotType::Nic)?; let (device_name, backend_name) = pci_path_to_nic_names(pci_path); - let device_spec = - NetworkDeviceV0::VirtioNic(components::devices::VirtioNic { - backend_name: backend_name.clone(), - pci_path, - }); + let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic { + backend_name: backend_name.clone(), + pci_path, + }); - let backend_spec = NetworkBackendV0::Virtio( - components::backends::VirtioNetworkBackend { - vnic_name: nic.name.to_string(), - }, - ); + let backend_spec = NetworkBackendV0::Virtio(VirtioNetworkBackend { + vnic_name: nic.name.to_string(), + }); self.builder.add_network_device( device_name, @@ -164,35 +166,26 @@ impl ServerSpecBuilder { let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?; let backend_name = disk.name.clone(); - let backend_spec = StorageBackendV0::Crucible( - components::backends::CrucibleStorageBackend { - request_json: serde_json::to_string( - &disk.volume_construction_request, - ) - .map_err(|e| { - ServerSpecBuilderError::SerializationError( - disk.name.clone(), - e, - ) - })?, - readonly: disk.read_only, - }, - ); + let backend_spec = StorageBackendV0::Crucible(CrucibleStorageBackend { + request_json: serde_json::to_string( + &disk.volume_construction_request, + ) + .map_err(|e| { + ServerSpecBuilderError::SerializationError(disk.name.clone(), e) + })?, + readonly: disk.read_only, + }); let device_name = disk.name.clone(); let device_spec = match disk.device.as_ref() { - "virtio" => { - StorageDeviceV0::VirtioDisk(components::devices::VirtioDisk { - backend_name: disk.name.to_string(), - pci_path, - }) - } - "nvme" => { - StorageDeviceV0::NvmeDisk(components::devices::NvmeDisk { - backend_name: disk.name.to_string(), - pci_path, - }) - } + "virtio" => StorageDeviceV0::VirtioDisk(VirtioDisk { + backend_name: disk.name.to_string(), + pci_path, + }), + "nvme" => StorageDeviceV0::NvmeDisk(NvmeDisk { + backend_name: disk.name.to_string(), + pci_path, + }), _ => { return Err(ServerSpecBuilderError::UnrecognizedStorageDevice( disk.device.clone(), @@ -219,18 +212,16 @@ impl ServerSpecBuilder { let name = "cloud-init"; let pci_path = slot_to_pci_path(api::Slot(0), SlotType::CloudInit)?; let backend_name = name.to_string(); - let backend_spec = - StorageBackendV0::Blob(components::backends::BlobStorageBackend { - base64, - readonly: true, - }); + let backend_spec = StorageBackendV0::Blob(BlobStorageBackend { + base64, + readonly: true, + }); let device_name = name.to_string(); - let device_spec = - StorageDeviceV0::VirtioDisk(components::devices::VirtioDisk { - backend_name: name.to_string(), - pci_path, - }); + let device_spec = StorageDeviceV0::VirtioDisk(VirtioDisk { + backend_name: name.to_string(), + pci_path, + }); self.builder.add_storage_device( device_name, @@ -407,7 +398,7 @@ impl ServerSpecBuilder { /// Adds a serial port specification to the spec under construction. pub fn add_serial_port( &mut self, - port: components::devices::SerialPortNumber, + port: SerialPortNumber, ) -> Result<(), ServerSpecBuilderError> { self.builder.add_serial_port(port)?; Ok(()) @@ -495,15 +486,13 @@ mod test { }) .err(), Some(ServerSpecBuilderError::InnerBuilderError( - SpecBuilderError::PciPathInUse(_) + builder::SpecBuilderError::PciPathInUse(_) )) )); } #[test] fn duplicate_serial_port() { - use components::devices::SerialPortNumber; - let mut builder = default_spec_builder().unwrap(); assert!(builder.add_serial_port(SerialPortNumber::Com1).is_ok()); assert!(builder.add_serial_port(SerialPortNumber::Com2).is_ok()); @@ -512,7 +501,7 @@ mod test { assert!(matches!( builder.add_serial_port(SerialPortNumber::Com1).err(), Some(ServerSpecBuilderError::InnerBuilderError( - SpecBuilderError::SerialPortInUse(_) + builder::SpecBuilderError::SerialPortInUse(_) )) )); } diff --git a/crates/propolis-api-types/src/instance_spec/v0/mod.rs b/crates/propolis-api-types/src/instance_spec/v0/mod.rs index 03cab5db8..344f83fcb 100644 --- a/crates/propolis-api-types/src/instance_spec/v0/mod.rs +++ b/crates/propolis-api-types/src/instance_spec/v0/mod.rs @@ -27,13 +27,11 @@ use crate::instance_spec::{ ElementCompatibilityError, MigrationCollection, MigrationCompatibilityError, MigrationElement, }, - PciPath, SpecKey, + SpecKey, }; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -pub mod builder; - #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] #[serde(deny_unknown_fields, tag = "type", content = "component")] pub enum StorageDeviceV0 { @@ -41,15 +39,6 @@ pub enum StorageDeviceV0 { NvmeDisk(components::devices::NvmeDisk), } -impl StorageDeviceV0 { - fn pci_path(&self) -> PciPath { - match self { - Self::VirtioDisk(disk) => disk.pci_path, - Self::NvmeDisk(disk) => disk.pci_path, - } - } -} - impl MigrationElement for StorageDeviceV0 { fn kind(&self) -> &'static str { match self { @@ -83,14 +72,6 @@ pub enum NetworkDeviceV0 { VirtioNic(components::devices::VirtioNic), } -impl NetworkDeviceV0 { - fn pci_path(&self) -> PciPath { - match self { - Self::VirtioNic(nic) => nic.pci_path, - } - } -} - impl MigrationElement for NetworkDeviceV0 { fn kind(&self) -> &'static str { "NetworkDevice(VirtioNic)"