try making boot settings a component instead

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 {

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<Spec> 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<Spec> 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<Spec> for InstanceSpecV0 {
             );
         }
 
-        for (name, desc) in val.serial {
+        for (name, desc) in serial {
             if desc.device == SerialPortDevice::Uart {
                 insert_component(
                     &mut spec,
@@ -112,49 +117,59 @@ impl From<Spec> 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,
                 ComponentV0::PciPciBridge(bridge),
             );
         }
 
-        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),
                     ComponentV0::SoftNpuPciPort(softnpu_pci),
                 );
             }
 
-            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),
                     ComponentV0::SoftNpuP9(p9),
                 );
             }
 
-            if let Some(p9fs) = val.softnpu.p9fs {
+            if let Some(p9fs) = softnpu.p9fs {
                 insert_component(
                     &mut spec,
                     format!("p9fs-{}", p9fs.pci_path),
                     ComponentV0::P9fs(p9fs),
                 );
             }
 
-            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<InstanceSpecV0> for Spec {
     type Error = ApiSpecError;
 
     fn try_from(value: InstanceSpecV0) -> Result<Self, Self::Error> {
-        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<String, StorageBackend> =
             HashMap::new();
         let mut viona_backends: HashMap<String, VirtioNetworkBackend> =
@@ -260,6 +276,14 @@ impl TryFrom<InstanceSpecV0> 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<InstanceSpecV0> 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() {

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<Item = BootOrderEntry>,
+    ) -> 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(())
     }

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<BootOrderEntry>,
 }
 
-impl From<propolis_api_types::BootSettings> for BootSettings {
-    fn from(value: propolis_api_types::BootSettings) -> Self {
-        Self { order: value.order.into_iter().map(Into::into).collect() }
-    }
-}
-
-impl From<BootSettings> 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<propolis_api_types::BootOrderEntry> for BootOrderEntry {
-    fn from(value: propolis_api_types::BootOrderEntry) -> Self {
-        Self { name: value.name }
+impl
+    From<propolis_api_types::instance_spec::components::devices::BootOrderEntry>
+    for BootOrderEntry
+{
+    fn from(
+        value: propolis_api_types::instance_spec::components::devices::BootOrderEntry,
+    ) -> Self {
+        Self { name: value.name.clone() }
     }
 }
 
-impl From<BootOrderEntry> for propolis_api_types::BootOrderEntry {
+impl From<BootOrderEntry>
+    for propolis_api_types::instance_spec::components::devices::BootOrderEntry
+{
     fn from(value: BootOrderEntry) -> Self {
         Self { name: value.name }
     }