instance spec rework: move migration compat checks to propolis-server (…

…#766)

Transfer Propolis's component-level live migration compatibility checks
from the API types crate to propolis-server. This is the last big chunk of
application code that lives in the api-types crate. This is not straight-up
code movement because the internal Spec type doesn't have the same notion
of "device" and "backend" collections that the wire type has. This means
that the hierarchy of errors in the new propolis-server compat.rs is
different (but hopefully simpler) than what was in the old
propolis-api-types migration.rs. The actual compatibility rules should be
the same as they were before, however, and I've tried to bring over
wholesale the api-types crate's compatibility unit tests.

Also, adjust how compatibility checks work in the migration protocol.
Before, the migration preamble sent a DeviceSpecV0 and a list of backend
component names; the destination checked that the devices were compatible
and that the backend name sets matched (but, owing to the absence of the
actual backend descriptors, did not check that the backends themselves were
migration-compatible). With this change, the preamble contains the source's
entire InstanceSpecV0, though the compatibility checks are still
(currently) limited to the VM's devices. This breaks the protocol's
dependency on the notion that instance specs necessarily have collections
of "devices" and "backends" as opposed to simply having a set of
components. This is a prerequisite for the next change in this series, which
removes this distinction from the InstanceSpecV0 type itself.

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

@@ -346,14 +346,15 @@ impl<T: MigrateConn> RonV0<T> {
         }?;
         info!(self.log(), "Destination read Preamble: {:?}", preamble);
 
-        if let Err(e) = preamble
-            .is_migration_compatible(&ensure_ctx.instance_spec().clone().into())
+        if let Err(e) =
+            preamble.check_compatibility(&ensure_ctx.instance_spec().clone())
         {
             error!(
                 self.log(),
-                "Source and destination instance specs incompatible: {}", e
+                "source and destination instance specs incompatible";
+                "error" => #%e
             );
-            return Err(MigrateError::InvalidInstanceState);
+            return Err(MigrateError::InstanceSpecsIncompatible(e.to_string()));
         }
 
         self.send_msg(codec::Message::Okay).await

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

@@ -12,6 +12,7 @@ use thiserror::Error;
 use tokio::io::{AsyncRead, AsyncWrite};
 
 mod codec;
+mod compat;
 pub mod destination;
 mod memx;
 mod preamble;
@@ -88,6 +89,14 @@ pub enum MigrateError {
     #[error("expected connection upgrade")]
     UpgradeExpected,
 
+    /// Error parsing the contents of the preamble
+    #[error("failed to parse preamble: {0}")]
+    PreambleParse(String),
+
+    /// Source and target decided their configurations are incompatible
+    #[error("instance specs incompatible: {0}")]
+    InstanceSpecsIncompatible(String),
+
     /// Attempted to migrate an uninitialized instance
     #[error("failed to initialize the target VM: {0}")]
     TargetInstanceInitializationFailed(String),
@@ -172,6 +181,8 @@ impl From<MigrateError> for HttpError {
             | MigrateError::ProtocolParse(_, _)
             | MigrateError::NoMatchingProtocol(_, _)
             | MigrateError::TargetInstanceInitializationFailed(_)
+            | MigrateError::PreambleParse(_)
+            | MigrateError::InstanceSpecsIncompatible(_)
             | MigrateError::InvalidInstanceState
             | MigrateError::Codec(_)
             | MigrateError::UnexpectedMessage

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

@@ -2,67 +2,41 @@
 // 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::BTreeSet;
-
-use propolis_api_types::instance_spec::{
-    migration::{CollectionCompatibilityError, MigrationCompatibilityError},
-    v0::{DeviceSpecV0, InstanceSpecV0},
-    VersionedInstanceSpec,
-};
+use propolis_api_types::instance_spec::VersionedInstanceSpec;
 use serde::{Deserialize, Serialize};
 
+use crate::spec::{self, api_spec_v0::ApiSpecError};
+
+use super::MigrateError;
+
 #[derive(Deserialize, Serialize, Debug)]
 pub(crate) struct Preamble {
-    pub device_spec: DeviceSpecV0,
-    pub backend_keys: BTreeSet<String>,
+    pub instance_spec: VersionedInstanceSpec,
     pub blobs: Vec<Vec<u8>>,
 }
 
-fn get_spec_backend_keys(spec: &InstanceSpecV0) -> BTreeSet<String> {
-    spec.backends
-        .storage_backends
-        .keys()
-        .chain(spec.backends.network_backends.keys())
-        .cloned()
-        .collect()
-}
-
 impl Preamble {
     pub fn new(instance_spec: VersionedInstanceSpec) -> Preamble {
-        let VersionedInstanceSpec::V0(instance_spec) = instance_spec;
-        Preamble {
-            device_spec: instance_spec.devices.clone(),
-            backend_keys: get_spec_backend_keys(&instance_spec),
-            blobs: Vec::new(),
-        }
+        Preamble { instance_spec, blobs: Vec::new() }
     }
 
-    pub fn is_migration_compatible(
-        &self,
-        other_spec: &InstanceSpecV0,
-    ) -> Result<(), MigrationCompatibilityError> {
-        self.device_spec.can_migrate_devices_from(&other_spec.devices)?;
-        let other_keys = get_spec_backend_keys(other_spec);
-        if self.backend_keys.len() != other_keys.len() {
-            return Err(MigrationCompatibilityError::CollectionMismatch(
-                "backends".to_string(),
-                CollectionCompatibilityError::CollectionSize(
-                    self.backend_keys.len(),
-                    other_keys.len(),
-                ),
-            ));
-        }
-
-        for this_key in self.backend_keys.iter() {
-            if !other_keys.contains(this_key) {
-                return Err(MigrationCompatibilityError::CollectionMismatch(
-                    "backends".to_string(),
-                    CollectionCompatibilityError::CollectionKeyAbsent(
-                        this_key.clone(),
-                    ),
-                ));
-            }
-        }
+    /// Checks to see if the serialized spec in this preamble is compatible with
+    /// the supplied `other_spec`.
+    ///
+    /// This check runs on the destination.
+    pub fn check_compatibility(
+        self,
+        other_spec: &spec::Spec,
+    ) -> Result<(), MigrateError> {
+        let VersionedInstanceSpec::V0(v0_spec) = self.instance_spec;
+        let this_spec: spec::Spec =
+            v0_spec.try_into().map_err(|e: ApiSpecError| {
+                MigrateError::PreambleParse(e.to_string())
+            })?;
+
+        this_spec.is_migration_compatible(other_spec).map_err(|e| {
+            MigrateError::InstanceSpecsIncompatible(e.to_string())
+        })?;
 
         // TODO: Compare opaque blobs.
 

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

@@ -75,6 +75,13 @@ pub enum StorageDevice {
 }
 
 impl StorageDevice {
+    pub fn kind(&self) -> &'static str {
+        match self {
+            StorageDevice::Virtio(_) => "virtio",
+            StorageDevice::Nvme(_) => "nvme",
+        }
+    }
+
     pub fn pci_path(&self) -> PciPath {
         match self {
             StorageDevice::Virtio(disk) => disk.pci_path,
@@ -116,6 +123,24 @@ pub enum StorageBackend {
     Blob(BlobStorageBackend),
 }
 
+impl StorageBackend {
+    pub fn kind(&self) -> &'static str {
+        match self {
+            StorageBackend::Crucible(_) => "crucible",
+            StorageBackend::File(_) => "file",
+            StorageBackend::Blob(_) => "backend",
+        }
+    }
+
+    pub fn read_only(&self) -> bool {
+        match self {
+            StorageBackend::Crucible(be) => be.readonly,
+            StorageBackend::File(be) => be.readonly,
+            StorageBackend::Blob(be) => be.readonly,
+        }
+    }
+}
+
 impl From<StorageBackend> for StorageBackendV0 {
     fn from(value: StorageBackend) -> Self {
         match value {
@@ -157,6 +182,21 @@ pub enum SerialPortDevice {
     SoftNpu,
 }
 
+impl std::fmt::Display for SerialPortDevice {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "{}",
+            match self {
+                SerialPortDevice::Uart => "uart",
+
+                #[cfg(feature = "falcon")]
+                SerialPortDevice::SoftNpu => "softnpu",
+            }
+        )
+    }
+}
+
 #[derive(Clone, Debug)]
 pub struct SerialPort {
     pub num: SerialPortNumber,
@@ -186,6 +226,17 @@ pub struct SoftNpu {
     pub p9fs: Option<P9fs>,
 }
 
+#[cfg(feature = "falcon")]
+impl SoftNpu {
+    /// Returns `true` if this struct specifies at least one SoftNPU component.
+    pub fn has_components(&self) -> bool {
+        self.pci_port.is_some()
+            || self.p9_device.is_some()
+            || self.p9fs.is_some()
+            || !self.ports.is_empty()
+    }
+}
+
 struct ParsedDiskRequest {
     name: String,
     disk: Disk,

crates/propolis-api-types/src/instance_spec/components/backends.rs

@@ -6,10 +6,8 @@
 //! its components to talk to other services supplied by the host OS or the
 //! larger rack.
 
-use crate::instance_spec::migration::MigrationElement;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use thiserror::Error;
 
 /// A Crucible storage backend.
 #[derive(Clone, Deserialize, Serialize, JsonSchema)]
@@ -29,28 +27,6 @@ pub struct CrucibleStorageBackend {
     pub readonly: bool,
 }
 
-impl MigrationElement for CrucibleStorageBackend {
-    fn kind(&self) -> &'static str {
-        "CrucibleStorageBackend"
-    }
-
-    fn can_migrate_from_element(
-        &self,
-        other: &Self,
-    ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError>
-    {
-        if self.readonly != other.readonly {
-            Err(MigrationCompatibilityError::ComponentConfiguration(format!(
-                "read-only mismatch (self: {}, other: {})",
-                self.readonly, other.readonly,
-            ))
-            .into())
-        } else {
-            Ok(())
-        }
-    }
-}
-
 impl std::fmt::Debug for CrucibleStorageBackend {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         // Redact the contents of the VCR since they may contain volume
@@ -73,28 +49,6 @@ pub struct FileStorageBackend {
     pub readonly: bool,
 }
 
-impl MigrationElement for FileStorageBackend {
-    fn kind(&self) -> &'static str {
-        "FileStorageBackend"
-    }
-
-    fn can_migrate_from_element(
-        &self,
-        other: &Self,
-    ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError>
-    {
-        if self.readonly != other.readonly {
-            Err(MigrationCompatibilityError::ComponentConfiguration(format!(
-                "read-only mismatch (self: {}, other: {})",
-                self.readonly, other.readonly,
-            ))
-            .into())
-        } else {
-            Ok(())
-        }
-    }
-}
-
 /// A storage backend for a disk whose initial contents are given explicitly
 /// by the specification.
 #[derive(Clone, Deserialize, Serialize, JsonSchema)]
@@ -116,28 +70,6 @@ impl std::fmt::Debug for BlobStorageBackend {
     }
 }
 
-impl MigrationElement for BlobStorageBackend {
-    fn kind(&self) -> &'static str {
-        "BlobStorageBackend"
-    }
-
-    fn can_migrate_from_element(
-        &self,
-        other: &Self,
-    ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError>
-    {
-        if self.readonly != other.readonly {
-            Err(MigrationCompatibilityError::ComponentConfiguration(format!(
-                "read-only mismatch (self: {}, other: {})",
-                self.readonly, other.readonly,
-            ))
-            .into())
-        } else {
-            Ok(())
-        }
-    }
-}
-
 /// A network backend associated with a virtio-net (viona) VNIC on the host.
 #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
 #[serde(deny_unknown_fields)]
@@ -146,44 +78,10 @@ pub struct VirtioNetworkBackend {
     pub vnic_name: String,
 }
 
-impl MigrationElement for VirtioNetworkBackend {
-    fn kind(&self) -> &'static str {
-        "VirtioNetworkBackend"
-    }
-
-    fn can_migrate_from_element(
-        &self,
-        _other: &Self,
-    ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError>
-    {
-        Ok(())
-    }
-}
-
 /// A network backend associated with a DLPI VNIC on the host.
 #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)]
 #[serde(deny_unknown_fields)]
 pub struct DlpiNetworkBackend {
     /// The name of the VNIC to use as a backend.
     pub vnic_name: String,
 }
-
-impl MigrationElement for DlpiNetworkBackend {
-    fn kind(&self) -> &'static str {
-        "DlpiNetworkBackend"
-    }
-
-    fn can_migrate_from_element(
-        &self,
-        _other: &Self,
-    ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError>
-    {
-        Ok(())
-    }
-}
-
-#[derive(Debug, Error)]
-pub enum MigrationCompatibilityError {
-    #[error("component configurations incompatible: {0}")]
-    ComponentConfiguration(String),
-}