instance spec rework: remove most dependencies on InstanceSpecV0 fr…

…om propolis-server (#757)

Define propolis-server's internal representation of a VM component manifest
(the `Spec` type) and switch almost everyone in the server over to it. (The
exception is the live migration preamble; this will be dealt with in a future
PR that moves migration compatibility checks from api-types to server.)

- Define the internal `spec::Spec` type and some helper types, like internal
  representations of a `Disk` and a `Nic`.
  - This PR starts to show some of the benefits of this series of changes
    (finally!): now a regular guest VNIC, which is only ever supposed to have a
    viona backend, is guaranteed by construction to have one, so the machine
    initializer doesn't need to handle the "oops wait you specified an invalid
    backend type here" case.
- Add conversion functions to/from the `InstanceSpecV0` public-API type.
  Converting to a versioned spec is infallible, but conversion from a versioned
  spec (which can be well-formed but not valid) is fallible.
- Represent Falcon devices with a little more care:
  - The parsed SoftNPU device collection now uses `Option`s for components (the
    main PCI device, the P9 device, and the P9 filesystem adapter) with a
    maximum cardinality of 1.
  - When enabling SoftNPU, explicitly denote in the instance spec that SoftNPU
    expects to use COM4 instead of excluding it from the spec entirely.

Known warts in this change that are on deck to be fixed later in this series:

* In the new internal spec type, serial devices don't have names yet (they
* still use port numbers as a key and convert these to implicit names). The new
* `Disk` and `Nic` types duplicate backend names: both the types themselves and
* their `device_spec` members contain `backend_name` fields, and these need to
* be identical or hilarity will ensue. Subsequent changes will remove the
* `backend_name` field from these types and just rely on the backend name
* that's in the device spec.

Tests: cargo test, PHD, Omicron dev cluster.

bin/propolis-server/src/lib/migrate/destination.rs

@@ -346,8 +346,8 @@ impl<T: MigrateConn> RonV0<T> {
         }?;
         info!(self.log(), "Destination read Preamble: {:?}", preamble);
 
-        if let Err(e) =
-            preamble.is_migration_compatible(ensure_ctx.instance_spec())
+        if let Err(e) = preamble
+            .is_migration_compatible(&ensure_ctx.instance_spec().clone().into())
         {
             error!(
                 self.log(),

bin/propolis-server/src/lib/migrate/source.rs

@@ -468,7 +468,7 @@ impl<'vm, T: MigrateConn> RonV0Runner<'vm, T> {
     async fn sync(&mut self) -> Result<(), MigrateError> {
         self.update_state(MigrationState::Sync);
         let preamble = Preamble::new(VersionedInstanceSpec::V0(
-            self.vm.lock_shared().await.instance_spec().clone(),
+            self.vm.lock_shared().await.instance_spec().clone().into(),
         ));
         let s = ron::ser::to_string(&preamble)
             .map_err(codec::ProtocolError::from)?;

bin/propolis-server/src/lib/server.rs

@@ -16,8 +16,18 @@ use std::net::SocketAddr;
 use std::net::SocketAddrV6;
 use std::sync::Arc;
 
-use crate::serial::history_buffer::SerialHistoryOffset;
-use crate::vm::VmError;
+use crate::{
+    serial::history_buffer::SerialHistoryOffset,
+    spec::{
+        self,
+        api_spec_v0::ApiSpecError,
+        builder::{SpecBuilder, SpecBuilderError},
+        Spec,
+    },
+    vm::{ensure::VmEnsureRequest, VmError},
+    vnc::{self, VncServer},
+};
+
 use dropshot::{
     channel, endpoint, ApiDescription, HttpError, HttpResponseCreated,
     HttpResponseOk, HttpResponseUpdatedNoContent, Path, Query, RequestContext,
@@ -29,18 +39,17 @@ 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::components::devices::QemuPvpanic;
-use propolis_api_types::instance_spec::{self, VersionedInstanceSpec};
-
+use propolis_api_types::instance_spec::{
+    self, components::devices::QemuPvpanic, VersionedInstanceSpec,
+};
 pub use propolis_server_config::Config as VmTomlConfig;
 use rfb::tungstenite::BinaryWs;
 use slog::{error, warn, Logger};
 use tokio::sync::MutexGuard;
-use tokio_tungstenite::tungstenite::protocol::{Role, WebSocketConfig};
-use tokio_tungstenite::WebSocketStream;
-
-use crate::spec::builder::{SpecBuilder, SpecBuilderError};
-use crate::vnc::{self, VncServer};
+use tokio_tungstenite::{
+    tungstenite::protocol::{Role, WebSocketConfig},
+    WebSocketStream,
+};
 
 /// Configuration used to set this server up to provide Oximeter metrics.
 #[derive(Debug, Clone)]
@@ -107,7 +116,7 @@ impl DropshotEndpointContext {
 fn instance_spec_from_request(
     request: &api::InstanceEnsureRequest,
     toml_config: &VmTomlConfig,
-) -> Result<VersionedInstanceSpec, SpecBuilderError> {
+) -> Result<Spec, SpecBuilderError> {
     let mut spec_builder = SpecBuilder::new(&request.properties);
 
     spec_builder.add_devices_from_config(toml_config)?;
@@ -135,8 +144,15 @@ fn instance_spec_from_request(
         spec_builder.add_serial_port(port)?;
     }
 
-    spec_builder.add_pvpanic_device(QemuPvpanic { enable_isa: true })?;
-    Ok(VersionedInstanceSpec::V0(spec_builder.finish()))
+    #[cfg(feature = "falcon")]
+    spec_builder.set_softnpu_com4()?;
+
+    spec_builder.add_pvpanic_device(spec::QemuPvpanic {
+        name: "pvpanic".to_string(),
+        spec: QemuPvpanic { enable_isa: true },
+    })?;
+
+    Ok(spec_builder.finish())
 }
 
 /// Wrapper around a [`NexusClient`] object, which allows deferring
@@ -217,14 +233,16 @@ async fn find_local_nexus_client(
 
 async fn instance_ensure_common(
     rqctx: RequestContext<Arc<DropshotEndpointContext>>,
-    request: api::InstanceSpecEnsureRequest,
+    properties: api::InstanceProperties,
+    migrate: Option<api::InstanceMigrateInitiateRequest>,
+    instance_spec: Spec,
 ) -> Result<HttpResponseCreated<api::InstanceEnsureResponse>, HttpError> {
     let server_context = rqctx.context();
     let oximeter_registry = server_context
         .static_config
         .metrics
         .as_ref()
-        .map(|_| ProducerRegistry::with_id(request.properties.id));
+        .map(|_| ProducerRegistry::with_id(properties.id));
 
     let nexus_client =
         find_local_nexus_client(rqctx.server.local_addr, rqctx.log.clone())
@@ -240,6 +258,7 @@ async fn instance_ensure_common(
         local_server_addr: rqctx.server.local_addr,
     };
 
+    let request = VmEnsureRequest { properties, migrate, instance_spec };
     server_context
         .vm
         .ensure(&server_context.log, request, ensure_options)
@@ -289,11 +308,9 @@ async fn instance_ensure(
 
     instance_ensure_common(
         rqctx,
-        api::InstanceSpecEnsureRequest {
-            properties: request.properties,
-            instance_spec,
-            migrate: request.migrate,
-        },
+        request.properties,
+        request.migrate,
+        instance_spec,
     )
     .await
 }
@@ -306,7 +323,14 @@ async fn instance_spec_ensure(
     rqctx: RequestContext<Arc<DropshotEndpointContext>>,
     request: TypedBody<api::InstanceSpecEnsureRequest>,
 ) -> Result<HttpResponseCreated<api::InstanceEnsureResponse>, HttpError> {
-    instance_ensure_common(rqctx, request.into_inner()).await
+    let request = request.into_inner();
+    let VersionedInstanceSpec::V0(v0_spec) = request.instance_spec;
+    let spec = Spec::try_from(v0_spec).map_err(|e: ApiSpecError| {
+        HttpError::for_bad_request(None, e.to_string())
+    })?;
+
+    instance_ensure_common(rqctx, request.properties, request.migrate, spec)
+        .await
 }
 
 async fn instance_get_common(

bin/propolis-server/src/lib/spec/api_request.rs

@@ -15,17 +15,16 @@ use propolis_api_types::{
             },
             devices::{NvmeDisk, VirtioDisk, VirtioNic},
         },
-        v0::{
-            NetworkBackendV0, NetworkDeviceV0, StorageBackendV0,
-            StorageDeviceV0,
-        },
         PciPath,
     },
     DiskRequest, NetworkInterfaceRequest, Slot,
 };
 use thiserror::Error;
 
-use super::{ParsedNetworkDevice, ParsedStorageDevice};
+use super::{
+    Disk, Nic, ParsedDiskRequest, ParsedNicRequest, StorageBackend,
+    StorageDevice,
+};
 
 #[derive(Debug, Error)]
 pub(crate) enum DeviceRequestError {
@@ -71,15 +70,15 @@ fn slot_to_pci_path(
 
 pub(super) fn parse_disk_from_request(
     disk: &DiskRequest,
-) -> Result<ParsedStorageDevice, DeviceRequestError> {
+) -> Result<ParsedDiskRequest, DeviceRequestError> {
     let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?;
     let backend_name = format!("{}-backend", disk.name);
     let device_spec = match disk.device.as_ref() {
-        "virtio" => StorageDeviceV0::VirtioDisk(VirtioDisk {
+        "virtio" => StorageDevice::Virtio(VirtioDisk {
             backend_name: backend_name.clone(),
             pci_path,
         }),
-        "nvme" => StorageDeviceV0::NvmeDisk(NvmeDisk {
+        "nvme" => StorageDevice::Nvme(NvmeDisk {
             backend_name: backend_name.clone(),
             pci_path,
         }),
@@ -92,65 +91,55 @@ pub(super) fn parse_disk_from_request(
     };
 
     let device_name = disk.name.clone();
-    let backend_spec = StorageBackendV0::Crucible(CrucibleStorageBackend {
+    let backend_spec = StorageBackend::Crucible(CrucibleStorageBackend {
         request_json: serde_json::to_string(&disk.volume_construction_request)
             .map_err(|e| {
                 DeviceRequestError::SerializationError(disk.name.clone(), e)
             })?,
         readonly: disk.read_only,
     });
 
-    Ok(ParsedStorageDevice {
-        device_name,
-        device_spec,
-        backend_name,
-        backend_spec,
+    Ok(ParsedDiskRequest {
+        name: device_name,
+        disk: Disk { device_spec, backend_name, backend_spec },
     })
 }
 
 pub(super) fn parse_cloud_init_from_request(
     base64: String,
-) -> Result<ParsedStorageDevice, DeviceRequestError> {
+) -> Result<ParsedDiskRequest, DeviceRequestError> {
     let name = "cloud-init";
     let pci_path = slot_to_pci_path(Slot(0), SlotType::CloudInit)?;
     let backend_name = name.to_string();
     let backend_spec =
-        StorageBackendV0::Blob(BlobStorageBackend { base64, readonly: true });
+        StorageBackend::Blob(BlobStorageBackend { base64, readonly: true });
 
-    let device_name = name.to_string();
-    let device_spec = StorageDeviceV0::VirtioDisk(VirtioDisk {
+    let device_spec = StorageDevice::Virtio(VirtioDisk {
         backend_name: name.to_string(),
         pci_path,
     });
 
-    Ok(ParsedStorageDevice {
-        device_name,
-        device_spec,
-        backend_name,
-        backend_spec,
+    Ok(ParsedDiskRequest {
+        name: name.to_owned(),
+        disk: Disk { device_spec, backend_name, backend_spec },
     })
 }
 
 pub(super) fn parse_nic_from_request(
     nic: &NetworkInterfaceRequest,
-) -> Result<ParsedNetworkDevice, DeviceRequestError> {
+) -> Result<ParsedNicRequest, DeviceRequestError> {
     let pci_path = slot_to_pci_path(nic.slot, SlotType::Nic)?;
     let (device_name, backend_name) = super::pci_path_to_nic_names(pci_path);
-    let device_spec = NetworkDeviceV0::VirtioNic(VirtioNic {
+    let device_spec = VirtioNic {
         backend_name: backend_name.clone(),
         interface_id: nic.interface_id,
         pci_path,
-    });
-
-    let backend_spec = NetworkBackendV0::Virtio(VirtioNetworkBackend {
-        vnic_name: nic.name.to_string(),
-    });
+    };
 
-    Ok(ParsedNetworkDevice {
-        device_name,
-        device_spec,
-        backend_name,
-        backend_spec,
+    let backend_spec = VirtioNetworkBackend { vnic_name: nic.name.to_string() };
+    Ok(ParsedNicRequest {
+        name: device_name,
+        nic: Nic { device_spec, backend_name, backend_spec },
     })
 }
 
@@ -161,15 +150,13 @@ mod test {
 
     use super::*;
 
-    fn check_parsed_storage_device_backend_pointer(
-        parsed: &ParsedStorageDevice,
-    ) {
-        let device_to_backend = match &parsed.device_spec {
-            StorageDeviceV0::VirtioDisk(d) => d.backend_name.clone(),
-            StorageDeviceV0::NvmeDisk(d) => d.backend_name.clone(),
+    fn check_parsed_storage_device_backend_pointer(parsed: &ParsedDiskRequest) {
+        let device_to_backend = match &parsed.disk.device_spec {
+            StorageDevice::Virtio(d) => d.backend_name.clone(),
+            StorageDevice::Nvme(d) => d.backend_name.clone(),
         };
 
-        assert_eq!(device_to_backend, parsed.backend_name);
+        assert_eq!(device_to_backend, parsed.disk.backend_name);
     }
 
     #[test]
@@ -201,8 +188,8 @@ mod test {
         };
 
         let parsed = parse_nic_from_request(&req).unwrap();
-        let NetworkDeviceV0::VirtioNic(nic) = &parsed.device_spec;
-        assert_eq!(nic.backend_name, parsed.backend_name);
+        let VirtioNic { backend_name, .. } = &parsed.nic.device_spec;
+        assert_eq!(*backend_name, parsed.nic.backend_name);
     }
 
     #[test]