diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index d4561d824..ce0ea860a 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -130,9 +130,11 @@ fn instance_spec_from_request( } if let Some(boot_settings) = request.boot_settings.as_ref() { - for item in boot_settings.order.iter() { - spec_builder.add_boot_option(item.clone())?; - } + let order = boot_settings.order.clone(); + spec_builder.add_boot_order( + "boot-settings".to_string(), + order.into_iter().map(Into::into), + )?; } if let Some(base64) = &request.cloud_init_bytes { 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 0ea2a360d..5ae9af5ee 100644 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ b/bin/propolis-server/src/lib/spec/api_spec_v0.rs @@ -10,8 +10,7 @@ use std::collections::HashMap; use propolis_api_types::instance_spec::{ components::{ backends::{DlpiNetworkBackend, VirtioNetworkBackend}, - board::Board as ApiBoard, - devices::SerialPort as SerialPortDesc, + devices::{BootSettings, SerialPort as SerialPortDesc}, }, v0::{ComponentV0, InstanceSpecV0}, }; @@ -50,6 +49,20 @@ pub(crate) enum ApiSpecError { impl From for InstanceSpecV0 { fn from(val: Spec) -> Self { + // Exhaustively destructure the input spec so that adding a new field + // without considering it here will break the build. + let Spec { + board, + disks, + nics, + boot_settings, + serial, + pci_pci_bridges, + pvpanic, + #[cfg(feature = "falcon")] + softnpu, + } = val; + // Inserts a component entry into the supplied map, asserting first that // the supplied key is not present in that map. // @@ -70,24 +83,16 @@ impl From for InstanceSpecV0 { spec.components.insert(key, val); } - let board = ApiBoard { - cpus: val.board.cpus, - memory_mb: val.board.memory_mb, - chipset: val.board.chipset, - boot_settings: val.boot_settings.map(Into::into).unwrap_or( - propolis_api_types::BootSettings { order: Vec::new() }, - ), - }; let mut spec = InstanceSpecV0 { board, ..Default::default() }; - for (disk_name, disk) in val.disks { + 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 (nic_name, nic) in val.nics { + for (nic_name, nic) in nics { let backend_name = nic.device_spec.backend_name.clone(); insert_component( &mut spec, @@ -102,7 +107,7 @@ impl From for InstanceSpecV0 { ); } - for (name, desc) in val.serial { + for (name, desc) in serial { if desc.device == SerialPortDevice::Uart { insert_component( &mut spec, @@ -112,7 +117,7 @@ impl From for InstanceSpecV0 { } } - for (bridge_name, bridge) in val.pci_pci_bridges { + for (bridge_name, bridge) in pci_pci_bridges { insert_component( &mut spec, bridge_name, @@ -120,17 +125,27 @@ impl From for InstanceSpecV0 { ); } - if let Some(pvpanic) = val.pvpanic { + if let Some(pvpanic) = pvpanic { insert_component( &mut spec, - pvpanic.name.clone(), + pvpanic.name, ComponentV0::QemuPvpanic(pvpanic.spec), ); } + if let Some(settings) = boot_settings { + insert_component( + &mut spec, + settings.name, + ComponentV0::BootSettings(BootSettings { + order: settings.order.into_iter().map(Into::into).collect(), + }), + ); + } + #[cfg(feature = "falcon")] { - if let Some(softnpu_pci) = val.softnpu.pci_port { + if let Some(softnpu_pci) = softnpu.pci_port { insert_component( &mut spec, format!("softnpu-pci-{}", softnpu_pci.pci_path), @@ -138,7 +153,7 @@ impl From for InstanceSpecV0 { ); } - if let Some(p9) = val.softnpu.p9_device { + if let Some(p9) = softnpu.p9_device { insert_component( &mut spec, format!("softnpu-p9-{}", p9.pci_path), @@ -146,7 +161,7 @@ impl From for InstanceSpecV0 { ); } - if let Some(p9fs) = val.softnpu.p9fs { + if let Some(p9fs) = softnpu.p9fs { insert_component( &mut spec, format!("p9fs-{}", p9fs.pci_path), @@ -154,7 +169,7 @@ impl From for InstanceSpecV0 { ); } - for (port_name, port) in val.softnpu.ports { + for (port_name, port) in softnpu.ports { insert_component( &mut spec, port_name.clone(), @@ -180,8 +195,9 @@ impl TryFrom for Spec { type Error = ApiSpecError; fn try_from(value: InstanceSpecV0) -> Result { - let mut builder = SpecBuilder::with_board(&value.board); + let mut builder = SpecBuilder::with_board(value.board); let mut devices: Vec<(String, ComponentV0)> = vec![]; + let mut boot_settings = None; let mut storage_backends: HashMap = HashMap::new(); let mut viona_backends: HashMap = @@ -260,6 +276,14 @@ impl TryFrom for Spec { spec: pvpanic, })?; } + ComponentV0::BootSettings(settings) => { + // The builder returns an error if its caller tries to add + // a boot option that isn't in the set of attached disks. + // 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)); + } #[cfg(not(feature = "falcon"))] ComponentV0::SoftNpuPciPort(_) | ComponentV0::SoftNpuPort(_) @@ -307,8 +331,13 @@ impl TryFrom for Spec { } } - for item in value.board.boot_settings.order.into_iter() { - builder.add_boot_option(item)?; + // Now that all disks have been attached, try to establish the boot + // order if one was supplied. + if let Some(settings) = boot_settings { + builder.add_boot_order( + settings.0, + settings.1.order.into_iter().map(Into::into), + )?; } if let Some(backend) = storage_backends.into_keys().next() { diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index acbfe90e5..227d001ce 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -9,12 +9,12 @@ use std::collections::{BTreeSet, HashSet}; use propolis_api_types::{ instance_spec::{ components::{ - board::{Board as ApiBoard, Chipset, I440Fx}, + board::{Board, Chipset, I440Fx}, devices::{PciPciBridge, SerialPortNumber}, }, PciPath, }, - BootOrderEntry, DiskRequest, InstanceProperties, NetworkInterfaceRequest, + DiskRequest, InstanceProperties, NetworkInterfaceRequest, }; use thiserror::Error; @@ -28,7 +28,7 @@ use crate::{config, spec::SerialPortDevice}; use super::{ api_request::{self, DeviceRequestError}, config_toml::{ConfigTomlError, ParsedConfig}, - Board, BootSettings, Disk, Nic, QemuPvpanic, SerialPort, + BootOrderEntry, BootSettings, Disk, Nic, QemuPvpanic, SerialPort, }; #[cfg(feature = "falcon")] @@ -46,19 +46,22 @@ pub(crate) enum SpecBuilderError { #[error("device {0} has the same name as its backend")] DeviceAndBackendNamesIdentical(String), - #[error("A component with name {0} already exists")] + #[error("a component with name {0} already exists")] ComponentNameInUse(String), - #[error("A PCI device is already attached at {0:?}")] + #[error("a PCI device is already attached at {0:?}")] PciPathInUse(PciPath), - #[error("Serial port {0:?} is already specified")] + #[error("serial port {0:?} is already specified")] SerialPortInUse(SerialPortNumber), #[error("pvpanic device already specified")] PvpanicInUse, - #[error("Boot option {0} is not an attached device")] + #[error("boot settings were already specified")] + BootSettingsInUse, + + #[error("boot option {0} is not an attached device")] BootOptionMissing(String), } @@ -84,9 +87,9 @@ impl SpecBuilder { } } - pub(super) fn with_board(api_board: &ApiBoard) -> Self { + pub(super) fn with_board(board: Board) -> Self { Self { - spec: super::Spec { board: api_board.into(), ..Default::default() }, + spec: super::Spec { board, ..Default::default() }, ..Default::default() } } @@ -113,8 +116,36 @@ impl SpecBuilder { Ok(()) } + /// Sets the spec's boot order to the list of disk devices specified in + /// `boot_options`. + /// + /// All of the items in the supplied `boot_options` must already be present + /// in the spec's disk map. + pub fn add_boot_order( + &mut self, + component_name: String, + boot_options: impl Iterator, + ) -> Result<(), SpecBuilderError> { + if self.component_names.contains(&component_name) { + return Err(SpecBuilderError::ComponentNameInUse(component_name)); + } + + if self.spec.boot_settings.is_some() { + return Err(SpecBuilderError::BootSettingsInUse); + } + + self.spec.boot_settings = + Some(BootSettings { name: component_name, order: vec![] }); + + for item in boot_options { + self.add_boot_option(item)?; + } + + Ok(()) + } + /// Add a boot option to the boot option list of the spec under construction. - pub fn add_boot_option( + fn add_boot_option( &mut self, item: BootOrderEntry, ) -> Result<(), SpecBuilderError> { @@ -125,9 +156,12 @@ impl SpecBuilder { let boot_settings = self .spec .boot_settings - .get_or_insert(BootSettings { order: Vec::new() }); + .as_mut() + .expect("boot settings must already exist"); - boot_settings.order.push(item.into()); + boot_settings + .order + .push(crate::spec::BootOrderEntry { name: item.name.clone() }); Ok(()) } diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index 0245afb16..4efe20488 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -22,7 +22,7 @@ use propolis_api_types::instance_spec::{ BlobStorageBackend, CrucibleStorageBackend, FileStorageBackend, VirtioNetworkBackend, }, - board::{Chipset, I440Fx}, + board::Board, devices::{ NvmeDisk, PciPciBridge, QemuPvpanic as QemuPvpanicDesc, SerialPortNumber, VirtioDisk, VirtioNic, @@ -74,65 +74,30 @@ pub(crate) struct Spec { } #[derive(Clone, Debug)] -pub(crate) struct Board { - pub cpus: u8, - pub memory_mb: u64, - pub chipset: Chipset, -} - -impl Default for Board { - fn default() -> Self { - Self { - cpus: 0, - memory_mb: 0, - chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }), - } - } -} - -impl From<&propolis_api_types::instance_spec::components::board::Board> - for Board -{ - fn from( - value: &propolis_api_types::instance_spec::components::board::Board, - ) -> Self { - Board { - cpus: value.cpus, - memory_mb: value.memory_mb, - chipset: value.chipset, - } - } -} - -#[derive(Clone, Debug, Default)] pub(crate) struct BootSettings { + pub name: String, pub order: Vec, } -impl From for BootSettings { - fn from(value: propolis_api_types::BootSettings) -> Self { - Self { order: value.order.into_iter().map(Into::into).collect() } - } -} - -impl From for propolis_api_types::BootSettings { - fn from(value: BootSettings) -> Self { - Self { order: value.order.into_iter().map(Into::into).collect() } - } -} - #[derive(Clone, Debug, Default)] pub(crate) struct BootOrderEntry { pub name: String, } -impl From for BootOrderEntry { - fn from(value: propolis_api_types::BootOrderEntry) -> Self { - Self { name: value.name } +impl + From + for BootOrderEntry +{ + fn from( + value: propolis_api_types::instance_spec::components::devices::BootOrderEntry, + ) -> Self { + Self { name: value.name.clone() } } } -impl From for propolis_api_types::BootOrderEntry { +impl From + for propolis_api_types::instance_spec::components::devices::BootOrderEntry +{ fn from(value: BootOrderEntry) -> Self { Self { name: value.name } } diff --git a/crates/propolis-api-types/src/instance_spec/components/board.rs b/crates/propolis-api-types/src/instance_spec/components/board.rs index f8713949b..df8d9e24c 100644 --- a/crates/propolis-api-types/src/instance_spec/components/board.rs +++ b/crates/propolis-api-types/src/instance_spec/components/board.rs @@ -8,8 +8,6 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use crate::BootSettings; - /// An Intel 440FX-compatible chipset. #[derive( Clone, Copy, Deserialize, Serialize, Debug, PartialEq, Eq, JsonSchema, @@ -48,9 +46,6 @@ pub struct Board { /// The chipset to expose to guest software. pub chipset: Chipset, - - /// The boot device order to supply to the guest. - pub boot_settings: BootSettings, // TODO: Guest platform and CPU feature identification. // TODO: NUMA topology. } @@ -61,7 +56,6 @@ impl Default for Board { cpus: 0, memory_mb: 0, chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }), - boot_settings: BootSettings { order: vec![] }, } } } 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 27defac39..f40a595d6 100644 --- a/crates/propolis-api-types/src/instance_spec/components/devices.rs +++ b/crates/propolis-api-types/src/instance_spec/components/devices.rs @@ -104,6 +104,25 @@ pub struct QemuPvpanic { // TODO(eliza): add support for the PCI PVPANIC device... } +/// A set of settings that determines what Propolis tells guest firmware about +/// how to boot a guest operating system. +#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema, Default)] +#[serde(deny_unknown_fields)] +pub struct BootSettings { + /// An ordered list of components to attempt to boot from. + pub order: Vec, +} + +/// An entry in the boot order stored in a [`BootSettings`] component. +#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema, Default)] +pub struct BootOrderEntry { + /// The name of another component in the spec that Propolis should try to + /// boot from. + /// + /// Currently, only disk device components are supported. + pub name: String, +} + // // Structs for Falcon devices. These devices don't support live migration. // diff --git a/crates/propolis-api-types/src/instance_spec/v0.rs b/crates/propolis-api-types/src/instance_spec/v0.rs index 93b8e545d..4fcdd7f54 100644 --- a/crates/propolis-api-types/src/instance_spec/v0.rs +++ b/crates/propolis-api-types/src/instance_spec/v0.rs @@ -17,6 +17,7 @@ pub enum ComponentV0 { SerialPort(components::devices::SerialPort), PciPciBridge(components::devices::PciPciBridge), QemuPvpanic(components::devices::QemuPvpanic), + BootSettings(components::devices::BootSettings), SoftNpuPciPort(components::devices::SoftNpuPciPort), SoftNpuPort(components::devices::SoftNpuPort), SoftNpuP9(components::devices::SoftNpuP9), diff --git a/crates/propolis-api-types/src/lib.rs b/crates/propolis-api-types/src/lib.rs index 593a2ec4b..a6de2d547 100644 --- a/crates/propolis-api-types/src/lib.rs +++ b/crates/propolis-api-types/src/lib.rs @@ -10,8 +10,14 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use uuid::Uuid; -// Re-export types that are of a public struct +// Re-export the instance spec boot settings types so they can also be used in +// legacy instance ensure requests. +pub use crate::instance_spec::components::devices::{ + BootOrderEntry, BootSettings, +}; use crate::instance_spec::VersionedInstanceSpec; + +// Re-export volume construction requests since they're part of a disk request. pub use crucible_client_types::VolumeConstructionRequest; pub mod instance_spec; @@ -388,18 +394,6 @@ pub struct DiskAttachment { pub state: DiskAttachmentState, } -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct BootSettings { - pub order: Vec, -} - -/// An entry in a list of boot options. -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct BootOrderEntry { - /// The name of the device to attempt booting from. - pub name: String, -} - /// A stable index which is translated by Propolis /// into a PCI BDF, visible to the guest. #[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] diff --git a/openapi/propolis-server.json b/openapi/propolis-server.json index 3650c4b35..6f834a6c5 100644 --- a/openapi/propolis-server.json +++ b/openapi/propolis-server.json @@ -480,14 +480,6 @@ "description": "A VM's mainboard.", "type": "object", "properties": { - "boot_settings": { - "description": "The boot device order to supply to the guest.", - "allOf": [ - { - "$ref": "#/components/schemas/BootSettings" - } - ] - }, "chipset": { "description": "The chipset to expose to guest software.", "allOf": [ @@ -510,7 +502,6 @@ } }, "required": [ - "boot_settings", "chipset", "cpus", "memory_mb" @@ -518,11 +509,11 @@ "additionalProperties": false }, "BootOrderEntry": { - "description": "An entry in a list of boot options.", + "description": "An entry in the boot order stored in a [`BootSettings`] component.", "type": "object", "properties": { "name": { - "description": "The name of the device to attempt booting from.", + "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" } }, @@ -531,9 +522,11 @@ ] }, "BootSettings": { + "description": "A set of settings that determines what Propolis tells guest firmware about how to boot a guest operating system.", "type": "object", "properties": { "order": { + "description": "An ordered list of components to attempt to boot from.", "type": "array", "items": { "$ref": "#/components/schemas/BootOrderEntry" @@ -542,7 +535,8 @@ }, "required": [ "order" - ] + ], + "additionalProperties": false }, "Chipset": { "description": "A kind of virtual chipset.", @@ -685,6 +679,25 @@ ], "additionalProperties": false }, + { + "type": "object", + "properties": { + "component": { + "$ref": "#/components/schemas/BootSettings" + }, + "type": { + "type": "string", + "enum": [ + "BootSettings" + ] + } + }, + "required": [ + "component", + "type" + ], + "additionalProperties": false + }, { "type": "object", "properties": { diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index 22f29d8c6..241180665 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -261,7 +261,6 @@ impl<'dr> VmConfig<'dr> { cpus: self.cpus, memory_mb: self.memory_mib, chipset: Chipset::default(), - boot_settings: BootSettings::default(), }, ..Default::default() }; @@ -303,12 +302,16 @@ impl<'dr> VmConfig<'dr> { assert!(_old.is_none()); if let Some(boot_order) = self.boot_order.as_ref() { - spec.board.boot_settings = BootSettings { - order: boot_order - .iter() - .map(|item| BootOrderEntry { name: item.to_string() }) - .collect(), - }; + let _old = spec.components.insert( + "boot-settings".to_string(), + ComponentV0::BootSettings(BootSettings { + order: boot_order + .iter() + .map(|item| BootOrderEntry { name: item.to_string() }) + .collect(), + }), + ); + assert!(_old.is_none()); } // Generate random identifiers for this instance's timeseries metadata.