From 516dabe473cecdc3baea1db98a80441968c144cf Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 18 Sep 2024 11:50:55 -0700 Subject: [PATCH] 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/compat.rs | 795 ++++++++++++++++++ .../src/lib/migrate/destination.rs | 9 +- bin/propolis-server/src/lib/migrate/mod.rs | 11 + .../src/lib/migrate/preamble.rs | 74 +- bin/propolis-server/src/lib/spec/mod.rs | 51 ++ .../src/instance_spec/components/backends.rs | 102 --- .../src/instance_spec/components/board.rs | 120 --- .../src/instance_spec/components/devices.rs | 303 +------ .../src/instance_spec/migration.rs | 210 ----- .../src/instance_spec/mod.rs | 1 - .../src/instance_spec/v0.rs | 108 +-- 11 files changed, 888 insertions(+), 896 deletions(-) create mode 100644 bin/propolis-server/src/lib/migrate/compat.rs delete mode 100644 crates/propolis-api-types/src/instance_spec/migration.rs diff --git a/bin/propolis-server/src/lib/migrate/compat.rs b/bin/propolis-server/src/lib/migrate/compat.rs new file mode 100644 index 000000000..2da008b5a --- /dev/null +++ b/bin/propolis-server/src/lib/migrate/compat.rs @@ -0,0 +1,795 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +//! Checks for compatibility of two instance specs. + +use std::collections::HashMap; + +use crate::spec::{self, SerialPortDevice}; + +use propolis_api_types::instance_spec::{ + components::{ + board::Chipset, + devices::{PciPciBridge, SerialPortNumber}, + }, + PciPath, +}; +use thiserror::Error; + +trait CompatCheck { + type Error; + + fn is_compatible_with(&self, other: &Self) -> Result<(), Self::Error>; +} + +#[derive(Debug, Error)] +pub enum CompatibilityError { + #[error(transparent)] + Board(#[from] BoardIncompatibility), + + #[error(transparent)] + Pvpanic(#[from] PvpanicIncompatibility), + + #[error("collection {0} incompatible")] + Collection(String, #[source] CollectionIncompatibility), + + #[cfg(feature = "falcon")] + #[error("can't migrate instances containing softnpu devices")] + SoftNpu, +} + +#[derive(Debug, Error)] +pub enum BoardIncompatibility { + #[error("boards have different CPU counts (self: {this}, other: {other})")] + CpuCount { this: u8, other: u8 }, + + #[error( + "boards have different memory sizes (self: {this}, other: {other})" + )] + MemorySize { this: u64, other: u64 }, + + #[error( + "chipsets have different PCIe settings (self: {this}, other: {other})" + )] + PcieEnabled { this: bool, other: bool }, +} + +#[derive(Debug, Error)] +pub enum DiskIncompatibility { + #[error( + "disks have different device interfaces (self: {this}, other: {other})" + )] + Interface { this: &'static str, other: &'static str }, + + #[error("disks have different PCI paths (self: {this}, other: {other})")] + PciPath { this: PciPath, other: PciPath }, + + #[error( + "disks have different backend names (self: {this:?}, other: {other:?})" + )] + BackendName { this: String, other: String }, + + #[error( + "disks have different backend kinds (self: {this}, other: {other})" + )] + BackendKind { this: &'static str, other: &'static str }, + + #[error( + "disks have different read-only settings (self: {this}, other: {other})" + )] + ReadOnly { this: bool, other: bool }, +} + +#[derive(Debug, Error)] +pub enum NicIncompatibility { + #[error("NICs have different PCI paths (self: {this}, other: {other})")] + PciPath { this: PciPath, other: PciPath }, + + #[error( + "NICs have different backend names (self: {this}, other: {other})" + )] + BackendName { this: String, other: String }, +} + +#[derive(Debug, Error)] +pub enum SerialPortIncompatibility { + #[error("ports have different numbers (self: {this:?}, other: {other:?})")] + Number { this: SerialPortNumber, other: SerialPortNumber }, + + #[error("ports have different devices (self: {this}, other: {other})")] + Device { this: SerialPortDevice, other: SerialPortDevice }, +} + +#[derive(Debug, Error)] +pub enum BridgeIncompatibility { + #[error("bridges have different PCI paths (self: {this}, other: {other})")] + PciPath { this: PciPath, other: PciPath }, + + #[error("bridges have different downstream buses (self: {this}, other: {other})")] + DownstreamBus { this: u8, other: u8 }, +} + +#[derive(Debug, Error)] +pub enum PvpanicIncompatibility { + #[error("pvpanic presence differs (self: {this}, other: {other})")] + Presence { this: bool, other: bool }, + + #[error( + "pvpanic devices have different names (self: {this:?}, other: {other:?})" + )] + Name { this: String, other: String }, + + #[error( + "pvpanic devices have different ISA settings (self: {this}, other: {other})" + )] + EnableIsa { this: bool, other: bool }, +} + +#[derive(Debug, Error)] +pub enum ComponentIncompatibility { + #[error(transparent)] + Board(#[from] BoardIncompatibility), + + #[error(transparent)] + Disk(#[from] DiskIncompatibility), + + #[error(transparent)] + Nic(#[from] NicIncompatibility), + + #[error(transparent)] + SerialPort(#[from] SerialPortIncompatibility), + + #[error(transparent)] + PciPciBridge(#[from] BridgeIncompatibility), +} + +#[derive(Debug, Error)] +pub enum CollectionIncompatibility { + #[error( + "collections have different lengths (self: {this}, other: {other})" + )] + Length { this: usize, other: usize }, + + #[error("collection key {0} present in self but not other")] + KeyAbsent(String), + + #[error("component {0} incompatible")] + Component(String, #[source] ComponentIncompatibility), +} + +impl> CompatCheck + for HashMap +{ + type Error = CollectionIncompatibility; + + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), CollectionIncompatibility> { + if self.len() != other.len() { + return Err(CollectionIncompatibility::Length { + this: self.len(), + other: other.len(), + }); + } + + for (key, this_val) in self.iter() { + let other_val = other.get(key).ok_or_else(|| { + CollectionIncompatibility::KeyAbsent(key.clone()) + })?; + + this_val.is_compatible_with(other_val).map_err(|e| { + CollectionIncompatibility::Component(key.clone(), e) + })?; + } + + Ok(()) + } +} + +impl spec::Spec { + fn is_board_compatible( + &self, + other: &Self, + ) -> Result<(), BoardIncompatibility> { + self.is_chipset_compatible(other)?; + + let this = &self.board; + let other = &other.board; + if this.cpus != other.cpus { + Err(BoardIncompatibility::CpuCount { + this: this.cpus, + other: other.cpus, + }) + } else if this.memory_mb != other.memory_mb { + Err(BoardIncompatibility::MemorySize { + this: this.memory_mb, + other: other.memory_mb, + }) + } else { + Ok(()) + } + } + + fn is_chipset_compatible( + &self, + other: &Self, + ) -> Result<(), BoardIncompatibility> { + let Chipset::I440Fx(this) = self.board.chipset; + let Chipset::I440Fx(other) = other.board.chipset; + + if this.enable_pcie != other.enable_pcie { + Err(BoardIncompatibility::PcieEnabled { + this: this.enable_pcie, + other: other.enable_pcie, + }) + } else { + Ok(()) + } + } + + fn is_pvpanic_compatible( + &self, + other: &Self, + ) -> Result<(), PvpanicIncompatibility> { + match (&self.pvpanic, &other.pvpanic) { + (None, None) => Ok(()), + (Some(this), Some(other)) if this.name != other.name => { + Err(PvpanicIncompatibility::Name { + this: this.name.clone(), + other: other.name.clone(), + }) + } + (Some(this), Some(other)) + if this.spec.enable_isa != other.spec.enable_isa => + { + Err(PvpanicIncompatibility::EnableIsa { + this: this.spec.enable_isa, + other: other.spec.enable_isa, + }) + } + (Some(_), Some(_)) => Ok(()), + (this, other) => Err(PvpanicIncompatibility::Presence { + this: this.is_some(), + other: other.is_some(), + }), + } + } + + pub(super) fn is_migration_compatible( + &self, + other: &Self, + ) -> Result<(), CompatibilityError> { + self.is_board_compatible(other)?; + self.disks.is_compatible_with(&other.disks).map_err(|e| { + CompatibilityError::Collection("disks".to_string(), e) + })?; + + self.nics.is_compatible_with(&other.nics).map_err(|e| { + CompatibilityError::Collection("nics".to_string(), e) + })?; + + self.serial.is_compatible_with(&other.serial).map_err(|e| { + CompatibilityError::Collection("serial ports".to_string(), e) + })?; + + self.pci_pci_bridges + .is_compatible_with(&other.pci_pci_bridges) + .map_err(|e| { + CompatibilityError::Collection("PCI bridges".to_string(), e) + })?; + + self.is_pvpanic_compatible(other)?; + + #[cfg(feature = "falcon")] + if self.softnpu.has_components() || other.softnpu.has_components() { + return Err(CompatibilityError::SoftNpu); + } + + Ok(()) + } +} + +impl spec::StorageDevice { + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), DiskIncompatibility> { + if std::mem::discriminant(self) != std::mem::discriminant(other) { + Err(DiskIncompatibility::Interface { + this: self.kind(), + other: other.kind(), + }) + } else if self.pci_path() != other.pci_path() { + Err(DiskIncompatibility::PciPath { + this: self.pci_path(), + other: other.pci_path(), + }) + } else if self.backend_name() != other.backend_name() { + Err(DiskIncompatibility::BackendName { + this: self.backend_name().to_owned(), + other: other.backend_name().to_owned(), + }) + } else { + Ok(()) + } + } +} + +impl spec::StorageBackend { + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), DiskIncompatibility> { + if std::mem::discriminant(self) != std::mem::discriminant(other) { + Err(DiskIncompatibility::BackendKind { + this: self.kind(), + other: other.kind(), + }) + } else if self.read_only() != other.read_only() { + Err(DiskIncompatibility::ReadOnly { + this: self.read_only(), + other: other.read_only(), + }) + } else { + Ok(()) + } + } +} + +impl CompatCheck for spec::Disk { + type Error = ComponentIncompatibility; + + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), ComponentIncompatibility> { + self.device_spec.is_compatible_with(&other.device_spec)?; + self.backend_spec.is_compatible_with(&other.backend_spec)?; + Ok(()) + } +} + +impl CompatCheck for spec::Nic { + type Error = ComponentIncompatibility; + + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), ComponentIncompatibility> { + if self.device_spec.pci_path != other.device_spec.pci_path { + Err(NicIncompatibility::PciPath { + this: self.device_spec.pci_path, + other: other.device_spec.pci_path, + }) + } else if self.device_spec.backend_name + != other.device_spec.backend_name + { + Err(NicIncompatibility::BackendName { + this: self.device_spec.backend_name.clone(), + other: other.device_spec.backend_name.clone(), + }) + } else { + Ok(()) + } + .map_err(ComponentIncompatibility::Nic) + } +} + +impl CompatCheck for spec::SerialPort { + type Error = ComponentIncompatibility; + + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), ComponentIncompatibility> { + if self.num != other.num { + Err(SerialPortIncompatibility::Number { + this: self.num, + other: other.num, + }) + } else if std::mem::discriminant(&self.device) + != std::mem::discriminant(&other.device) + { + Err(SerialPortIncompatibility::Device { + this: self.device, + other: other.device, + }) + } else { + Ok(()) + } + .map_err(ComponentIncompatibility::SerialPort) + } +} + +impl CompatCheck for PciPciBridge { + type Error = ComponentIncompatibility; + + fn is_compatible_with( + &self, + other: &Self, + ) -> Result<(), ComponentIncompatibility> { + if self.pci_path != other.pci_path { + Err(BridgeIncompatibility::PciPath { + this: self.pci_path, + other: other.pci_path, + }) + } else if self.downstream_bus != other.downstream_bus { + Err(BridgeIncompatibility::DownstreamBus { + this: self.downstream_bus, + other: other.downstream_bus, + }) + } else { + Ok(()) + } + .map_err(ComponentIncompatibility::PciPciBridge) + } +} + +#[cfg(test)] +mod test { + use propolis_api_types::instance_spec::components::{ + backends::{ + CrucibleStorageBackend, FileStorageBackend, VirtioNetworkBackend, + }, + board::I440Fx, + devices::{ + NvmeDisk, QemuPvpanic as QemuPvpanicDesc, VirtioDisk, VirtioNic, + }, + }; + use spec::{QemuPvpanic, StorageDevice}; + use uuid::Uuid; + + use super::*; + + fn new_spec() -> spec::Spec { + let mut spec = spec::Spec::default(); + spec.board.cpus = 2; + spec.board.memory_mb = 512; + spec + } + + fn file_backend() -> spec::StorageBackend { + spec::StorageBackend::File(FileStorageBackend { + path: "/tmp/file.raw".to_owned(), + readonly: false, + }) + } + + fn crucible_backend() -> spec::StorageBackend { + spec::StorageBackend::Crucible(CrucibleStorageBackend { + request_json: "{}".to_owned(), + readonly: false, + }) + } + + fn nic() -> spec::Nic { + spec::Nic { + device_spec: VirtioNic { + pci_path: PciPath::new(0, 16, 0).unwrap(), + interface_id: Uuid::new_v4(), + backend_name: "vnic".to_owned(), + }, + backend_spec: VirtioNetworkBackend { + vnic_name: "vnic0".to_owned(), + }, + } + } + + fn serial_port() -> spec::SerialPort { + spec::SerialPort { + num: SerialPortNumber::Com1, + device: SerialPortDevice::Uart, + } + } + + fn bridge() -> PciPciBridge { + PciPciBridge { + downstream_bus: 1, + pci_path: PciPath::new(0, 24, 0).unwrap(), + } + } + + #[test] + fn cpu_mismatch() { + let s1 = new_spec(); + let mut s2 = s1.clone(); + s2.board.cpus += 1; + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn memory_mismatch() { + let s1 = new_spec(); + let mut s2 = s1.clone(); + s2.board.memory_mb *= 2; + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn pcie_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + s1.board.chipset = Chipset::I440Fx(I440Fx { enable_pcie: false }); + s2.board.chipset = Chipset::I440Fx(I440Fx { enable_pcie: true }); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn pvpanic_name_mismatch() { + let mut s1 = new_spec(); + s1.pvpanic = Some(QemuPvpanic { + name: "pvpanic1".to_string(), + spec: QemuPvpanicDesc { enable_isa: true }, + }); + let mut s2 = s1.clone(); + s2.pvpanic.as_mut().unwrap().name = "pvpanic2".to_string(); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn pvpanic_enable_isa_mismatch() { + let mut s1 = new_spec(); + s1.pvpanic = Some(QemuPvpanic { + name: "pvpanic".to_string(), + spec: QemuPvpanicDesc { enable_isa: true }, + }); + let mut s2 = s1.clone(); + s2.pvpanic.as_mut().unwrap().spec.enable_isa = false; + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn compatible_disks() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let disk = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_string(), + }), + backend_spec: crucible_backend(), + }; + + s1.disks.insert("disk".to_owned(), disk.clone()); + s2.disks.insert("disk".to_owned(), disk); + assert!(s1.is_migration_compatible(&s2).is_ok()); + } + + #[test] + fn disk_name_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: crucible_backend(), + }; + + s1.disks.insert("disk1".to_owned(), d1.clone()); + s2.disks.insert("disk2".to_owned(), d1); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn disk_length_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: crucible_backend(), + }; + + s1.disks.insert("disk1".to_owned(), d1.clone()); + s2.disks.insert("disk1".to_owned(), d1.clone()); + s2.disks.insert("disk2".to_owned(), d1); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn disk_interface_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: crucible_backend(), + }; + + let mut d2 = d1.clone(); + d2.device_spec = StorageDevice::Nvme(NvmeDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }); + + s1.disks.insert("disk".to_owned(), d1); + s2.disks.insert("disk".to_owned(), d2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn disk_pci_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: crucible_backend(), + }; + + let mut d2 = d1.clone(); + d2.device_spec = StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 5, 0).unwrap(), + backend_name: "backend".to_owned(), + }); + + s1.disks.insert("disk".to_owned(), d1); + s2.disks.insert("disk".to_owned(), d2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn disk_backend_name_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: crucible_backend(), + }; + + let mut d2 = d1.clone(); + d2.device_spec = StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "other_backend".to_owned(), + }); + + s1.disks.insert("disk".to_owned(), d1); + s2.disks.insert("disk".to_owned(), d2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn disk_backend_kind_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: file_backend(), + }; + + let mut d2 = d1.clone(); + d2.backend_spec = crucible_backend(); + s1.disks.insert("disk".to_owned(), d1); + s2.disks.insert("disk".to_owned(), d2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn disk_backend_readonly_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let d1 = spec::Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + pci_path: PciPath::new(0, 4, 0).unwrap(), + backend_name: "backend".to_owned(), + }), + backend_spec: file_backend(), + }; + + let mut d2 = d1.clone(); + d2.backend_spec = spec::StorageBackend::File(FileStorageBackend { + path: "/tmp/file.raw".to_owned(), + readonly: true, + }); + + s1.disks.insert("disk".to_owned(), d1); + s2.disks.insert("disk".to_owned(), d2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn compatible_nics() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let nic = nic(); + s1.nics.insert("nic".to_owned(), nic.clone()); + s2.nics.insert("nic".to_owned(), nic); + assert!(s1.is_migration_compatible(&s2).is_ok()); + } + + #[test] + fn nic_pci_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let n1 = nic(); + let mut n2 = n1.clone(); + n2.device_spec.pci_path = PciPath::new(0, 24, 0).unwrap(); + s1.nics.insert("nic".to_owned(), n1); + s2.nics.insert("nic".to_owned(), n2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn nic_backend_name_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let n1 = nic(); + let mut n2 = n1.clone(); + "other_backend".clone_into(&mut n2.device_spec.backend_name); + s1.nics.insert("nic".to_owned(), n1); + s2.nics.insert("nic".to_owned(), n2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn compatible_serial_ports() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let serial = serial_port(); + s1.serial.insert("com1".to_owned(), serial.clone()); + s2.serial.insert("com1".to_owned(), serial); + assert!(s1.is_migration_compatible(&s2).is_ok()); + } + + #[test] + fn serial_port_number_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let serial1 = serial_port(); + let mut serial2 = serial1.clone(); + serial2.num = SerialPortNumber::Com2; + s1.serial.insert("com1".to_owned(), serial1); + s2.serial.insert("com1".to_owned(), serial2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn compatible_bridges() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let bridge = bridge(); + s1.pci_pci_bridges.insert("bridge1".to_owned(), bridge); + s2.pci_pci_bridges.insert("bridge1".to_owned(), bridge); + assert!(s1.is_migration_compatible(&s2).is_ok()); + } + + #[test] + fn bridge_downstream_bus_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let b1 = bridge(); + let mut b2 = b1; + b2.downstream_bus += 1; + s1.pci_pci_bridges.insert("bridge1".to_owned(), b1); + s2.pci_pci_bridges.insert("bridge1".to_owned(), b2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } + + #[test] + fn bridge_pci_path_mismatch() { + let mut s1 = new_spec(); + let mut s2 = s1.clone(); + let b1 = bridge(); + let mut b2 = b1; + b2.pci_path = PciPath::new(0, 30, 0).unwrap(); + s1.pci_pci_bridges.insert("bridge1".to_owned(), b1); + s2.pci_pci_bridges.insert("bridge1".to_owned(), b2); + assert!(s1.is_migration_compatible(&s2).is_err()); + } +} diff --git a/bin/propolis-server/src/lib/migrate/destination.rs b/bin/propolis-server/src/lib/migrate/destination.rs index 21b12f09c..eb5982f15 100644 --- a/bin/propolis-server/src/lib/migrate/destination.rs +++ b/bin/propolis-server/src/lib/migrate/destination.rs @@ -346,14 +346,15 @@ impl RonV0 { }?; 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 diff --git a/bin/propolis-server/src/lib/migrate/mod.rs b/bin/propolis-server/src/lib/migrate/mod.rs index a59e8c85c..402adf30e 100644 --- a/bin/propolis-server/src/lib/migrate/mod.rs +++ b/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 for HttpError { | MigrateError::ProtocolParse(_, _) | MigrateError::NoMatchingProtocol(_, _) | MigrateError::TargetInstanceInitializationFailed(_) + | MigrateError::PreambleParse(_) + | MigrateError::InstanceSpecsIncompatible(_) | MigrateError::InvalidInstanceState | MigrateError::Codec(_) | MigrateError::UnexpectedMessage diff --git a/bin/propolis-server/src/lib/migrate/preamble.rs b/bin/propolis-server/src/lib/migrate/preamble.rs index b45a0d9ac..f80c4a767 100644 --- a/bin/propolis-server/src/lib/migrate/preamble.rs +++ b/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, + pub instance_spec: VersionedInstanceSpec, pub blobs: Vec>, } -fn get_spec_backend_keys(spec: &InstanceSpecV0) -> BTreeSet { - 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. diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index e26f51757..189463a6c 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/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 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, } +#[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, diff --git a/crates/propolis-api-types/src/instance_spec/components/backends.rs b/crates/propolis-api-types/src/instance_spec/components/backends.rs index c61778b51..79fc161cf 100644 --- a/crates/propolis-api-types/src/instance_spec/components/backends.rs +++ b/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,20 +78,6 @@ 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)] @@ -167,23 +85,3 @@ 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), -} 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 71370fea9..e9c3f59de 100644 --- a/crates/propolis-api-types/src/instance_spec/components/board.rs +++ b/crates/propolis-api-types/src/instance_spec/components/board.rs @@ -7,9 +7,6 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use thiserror::Error; - -use crate::instance_spec::migration::MigrationElement; /// An Intel 440FX-compatible chipset. #[derive( @@ -22,28 +19,6 @@ pub struct I440Fx { pub enable_pcie: bool, } -impl MigrationElement for I440Fx { - fn kind(&self) -> &'static str { - "i440fx" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - if self.enable_pcie != other.enable_pcie { - Err(MigrationCompatibilityError::PcieMismatch( - self.enable_pcie, - other.enable_pcie, - ) - .into()) - } else { - Ok(()) - } - } -} - /// A kind of virtual chipset. #[derive( Clone, Copy, Deserialize, Serialize, Debug, PartialEq, Eq, JsonSchema, @@ -59,23 +34,6 @@ pub enum Chipset { I440Fx(I440Fx), } -impl MigrationElement for Chipset { - fn kind(&self) -> &'static str { - match self { - Self::I440Fx(_) => "i440fx", - } - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - let (Self::I440Fx(this), Self::I440Fx(other)) = (self, other); - this.can_migrate_from_element(other) - } -} - /// A VM's mainboard. #[derive(Clone, Deserialize, Serialize, Debug, PartialEq, Eq, JsonSchema)] #[serde(deny_unknown_fields)] @@ -101,81 +59,3 @@ impl Default for Board { } } } - -impl MigrationElement for Board { - fn kind(&self) -> &'static str { - "Board" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - if self.cpus != other.cpus { - Err(MigrationCompatibilityError::CpuCount(self.cpus, other.cpus) - .into()) - } else if self.memory_mb != other.memory_mb { - Err(MigrationCompatibilityError::MemorySize( - self.memory_mb, - other.memory_mb, - ) - .into()) - } else if let Err(e) = - self.chipset.can_migrate_from_element(&other.chipset) - { - Err(e) - } else { - Ok(()) - } - } -} - -#[derive(Debug, Error)] -pub enum MigrationCompatibilityError { - #[error("Boards have different CPU counts (self: {0}, other: {1})")] - CpuCount(u8, u8), - - #[error("Boards have different memory amounts (self: {0}, other: {1})")] - MemorySize(u64, u64), - - #[error("Chipsets have different PCIe settings (self: {0}, other: {1})")] - PcieMismatch(bool, bool), -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn compatible_boards() { - let b1 = Board { - cpus: 8, - memory_mb: 8192, - chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }), - }; - - assert!(b1.can_migrate_from_element(&b1).is_ok()); - } - - #[test] - fn incompatible_boards() { - let b1 = Board { - cpus: 4, - memory_mb: 4096, - chipset: Chipset::I440Fx(I440Fx { enable_pcie: true }), - }; - - let b2 = Board { cpus: 8, ..b1 }; - assert!(b1.can_migrate_from_element(&b2).is_err()); - - let b2 = Board { memory_mb: b1.memory_mb * 2, ..b1 }; - assert!(b1.can_migrate_from_element(&b2).is_err()); - - let b2 = Board { - chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }), - ..b1 - }; - assert!(b1.can_migrate_from_element(&b2).is_err()); - } -} 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 b99e5c3b9..1aa9b7225 100644 --- a/crates/propolis-api-types/src/instance_spec/components/devices.rs +++ b/crates/propolis-api-types/src/instance_spec/components/devices.rs @@ -5,35 +5,9 @@ //! Device configuration data: components that define VM properties that are //! visible to a VM's guest software. -use crate::instance_spec::{migration::MigrationElement, PciPath}; +use crate::instance_spec::PciPath; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use thiserror::Error; - -fn backend_name_matches( - this: &str, - other: &str, -) -> Result<(), MigrationCompatibilityError> { - if this != other { - Err(MigrationCompatibilityError::BackendNameMismatch( - this.to_owned(), - other.to_owned(), - )) - } else { - Ok(()) - } -} - -fn pci_path_matches( - this: &PciPath, - other: &PciPath, -) -> Result<(), MigrationCompatibilityError> { - if this != other { - Err(MigrationCompatibilityError::PciPath(*this, *other)) - } else { - Ok(()) - } -} /// A disk that presents a virtio-block interface to the guest. #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] @@ -46,22 +20,6 @@ pub struct VirtioDisk { pub pci_path: PciPath, } -impl MigrationElement for VirtioDisk { - fn kind(&self) -> &'static str { - "VirtioDisk" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - backend_name_matches(&self.backend_name, &other.backend_name)?; - pci_path_matches(&self.pci_path, &other.pci_path)?; - Ok(()) - } -} - /// A disk that presents an NVMe interface to the guest. #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] #[serde(deny_unknown_fields)] @@ -73,22 +31,6 @@ pub struct NvmeDisk { pub pci_path: PciPath, } -impl MigrationElement for NvmeDisk { - fn kind(&self) -> &'static str { - "NvmeDisk" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - backend_name_matches(&self.backend_name, &other.backend_name)?; - pci_path_matches(&self.pci_path, &other.pci_path)?; - Ok(()) - } -} - /// A network card that presents a virtio-net interface to the guest. #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] #[serde(deny_unknown_fields)] @@ -106,22 +48,6 @@ pub struct VirtioNic { pub pci_path: PciPath, } -impl MigrationElement for VirtioNic { - fn kind(&self) -> &'static str { - "VirtioNic" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - backend_name_matches(&self.backend_name, &other.backend_name)?; - pci_path_matches(&self.pci_path, &other.pci_path)?; - Ok(()) - } -} - /// A serial port identifier, which determines what I/O ports a guest can use to /// access a port. #[derive( @@ -145,28 +71,6 @@ pub struct SerialPort { pub num: SerialPortNumber, } -impl MigrationElement for SerialPort { - fn kind(&self) -> &'static str { - "SerialPort" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - if self != other { - Err(MigrationCompatibilityError::ComponentConfiguration(format!( - "serial port number mismatch (self: {0:?}, other: {1:?})", - self, other - )) - .into()) - } else { - Ok(()) - } - } -} - /// A PCI-PCI bridge. #[derive( Clone, Copy, Deserialize, Serialize, Debug, PartialEq, Eq, JsonSchema, @@ -182,44 +86,6 @@ pub struct PciPciBridge { pub pci_path: PciPath, } -impl MigrationElement for PciPciBridge { - fn kind(&self) -> &'static str { - "PciPciBridge" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - pci_path_matches(&self.pci_path, &other.pci_path)?; - if self.downstream_bus != other.downstream_bus { - Err(MigrationCompatibilityError::ComponentConfiguration(format!( - "bridge downstream bus mismatch (self: {0}, other: {1})", - self.downstream_bus, other.downstream_bus - )) - .into()) - } else { - Ok(()) - } - } -} - -#[derive(Debug, Error)] -pub enum MigrationCompatibilityError { - /// The two devices have mismatched backend names. This means that migration - /// might fail to copy the source device's backend's state to the - /// corresponding target backend. - #[error("devices have different backend names (self: {0}, other: {1})")] - BackendNameMismatch(String, String), - - #[error("PCI devices have different paths (self: {0:?}, other: {1:?})")] - PciPath(PciPath, PciPath), - - #[error("component configurations incompatible: {0}")] - ComponentConfiguration(String), -} - #[derive( Clone, Copy, @@ -238,28 +104,6 @@ pub struct QemuPvpanic { // TODO(eliza): add support for the PCI PVPANIC device... } -impl MigrationElement for Option { - fn kind(&self) -> &'static str { - "QemuPvpanic" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), crate::instance_spec::migration::ElementCompatibilityError> - { - if self != other { - Err(MigrationCompatibilityError::ComponentConfiguration(format!( - "pvpanic configuration mismatch (self: {0:?}, other: {1:?})", - self, other - )) - .into()) - } else { - Ok(()) - } - } -} - // // Structs for Falcon devices. These devices don't support live migration. // @@ -308,148 +152,3 @@ pub struct P9fs { /// The PCI path at which to attach the guest to this P9 filesystem. pub pci_path: PciPath, } - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn compatible_virtio_disk() { - let d1 = VirtioDisk { - backend_name: "storage_backend".to_string(), - pci_path: PciPath::new(0, 5, 0).unwrap(), - }; - assert!(d1.can_migrate_from_element(&d1).is_ok()); - } - - #[test] - fn incompatible_virtio_disk() { - let d1 = VirtioDisk { - backend_name: "storage_backend".to_string(), - pci_path: PciPath::new(0, 5, 0).unwrap(), - }; - - let d2 = VirtioDisk { backend_name: "other_backend".to_string(), ..d1 }; - assert!(d1.can_migrate_from_element(&d2).is_err()); - - let d2 = VirtioDisk { - pci_path: PciPath::new(0, 6, 0).unwrap(), - ..d1.clone() - }; - assert!(d1.can_migrate_from_element(&d2).is_err()); - } - - #[test] - fn compatible_nvme_disk() { - let d1 = NvmeDisk { - backend_name: "storage_backend".to_string(), - pci_path: PciPath::new(0, 5, 0).unwrap(), - }; - assert!(d1.can_migrate_from_element(&d1).is_ok()); - } - - #[test] - fn incompatible_nvme_disk() { - let d1 = NvmeDisk { - backend_name: "storage_backend".to_string(), - pci_path: PciPath::new(0, 5, 0).unwrap(), - }; - - let d2 = NvmeDisk { backend_name: "other_backend".to_string(), ..d1 }; - assert!(d1.can_migrate_from_element(&d2).is_err()); - - let d2 = - NvmeDisk { pci_path: PciPath::new(0, 6, 0).unwrap(), ..d1.clone() }; - assert!(d1.can_migrate_from_element(&d2).is_err()); - } - - #[test] - fn compatible_virtio_nic() { - let d1 = VirtioNic { - backend_name: "storage_backend".to_string(), - interface_id: uuid::Uuid::new_v4(), - pci_path: PciPath::new(0, 5, 0).unwrap(), - }; - assert!(d1.can_migrate_from_element(&d1).is_ok()); - } - - #[test] - fn incompatible_virtio_nic() { - let d1 = VirtioNic { - backend_name: "storage_backend".to_string(), - interface_id: uuid::Uuid::new_v4(), - pci_path: PciPath::new(0, 5, 0).unwrap(), - }; - - let d2 = VirtioNic { backend_name: "other_backend".to_string(), ..d1 }; - assert!(d1.can_migrate_from_element(&d2).is_err()); - - let d2 = VirtioNic { - pci_path: PciPath::new(0, 6, 0).unwrap(), - ..d1.clone() - }; - assert!(d1.can_migrate_from_element(&d2).is_err()); - } - - #[test] - fn serial_port_compatibility() { - let ports = [ - SerialPortNumber::Com1, - SerialPortNumber::Com2, - SerialPortNumber::Com3, - SerialPortNumber::Com4, - ]; - - for (p1, p2) in - ports.into_iter().flat_map(|p| std::iter::repeat(p).zip(ports)) - { - let can_migrate = SerialPort { num: p1 } - .can_migrate_from_element(&SerialPort { num: p2 }); - - assert_eq!( - p1 == p2, - can_migrate.is_ok(), - "p1: {:?}, p2: {:?}", - p1, - p2 - ); - } - } - - #[test] - fn pci_bridge_compatibility() { - let b1 = PciPciBridge { - downstream_bus: 1, - pci_path: PciPath::new(1, 2, 3).unwrap(), - }; - - let mut b2 = b1; - assert!(b1.can_migrate_from_element(&b2).is_ok()); - - b2.downstream_bus += 1; - assert!(b1.can_migrate_from_element(&b2).is_err()); - b2.downstream_bus = b1.downstream_bus; - - b2.pci_path = PciPath::new(4, 5, 6).unwrap(); - assert!(b1.can_migrate_from_element(&b2).is_err()); - } - - #[test] - fn incompatible_qemu_pvpanic() { - let d1 = Some(QemuPvpanic { enable_isa: true }); - let d2 = Some(QemuPvpanic { enable_isa: false }); - assert!(d1.can_migrate_from_element(&d2).is_err()); - assert!(d1.can_migrate_from_element(&None).is_err()); - } - - #[test] - fn compatible_qemu_pvpanic() { - let d1 = Some(QemuPvpanic { enable_isa: true }); - let d2 = Some(QemuPvpanic { enable_isa: true }); - assert!(d1.can_migrate_from_element(&d2).is_ok()); - - let d1 = Some(QemuPvpanic { enable_isa: false }); - let d2 = Some(QemuPvpanic { enable_isa: false }); - assert!(d1.can_migrate_from_element(&d2).is_ok()); - } -} diff --git a/crates/propolis-api-types/src/instance_spec/migration.rs b/crates/propolis-api-types/src/instance_spec/migration.rs deleted file mode 100644 index 061e81473..000000000 --- a/crates/propolis-api-types/src/instance_spec/migration.rs +++ /dev/null @@ -1,210 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// 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/. - -//! Traits for determining whether two instance specs describe VM configurations -//! that allow live migration from a Propolis using one spec to a Propolis using -//! the other. - -use std::collections::{HashMap, HashSet}; - -use crate::instance_spec::{components, SpecKey}; -use thiserror::Error; - -/// An error type describing mismatches between two spec elements--i.e., -/// descriptions of individual devices or components--that block migration. -#[derive(Debug, Error)] -pub enum ElementCompatibilityError { - #[error("component types aren't comparable (self: {0}, other: {1})")] - ComponentsIncomparable(&'static str, &'static str), - - #[error("boards not compatible: {0}")] - BoardsIncompatible(#[from] components::board::MigrationCompatibilityError), - - #[error("devices not compatible: {0}")] - DevicesIncompatible( - #[from] components::devices::MigrationCompatibilityError, - ), - - #[error("backends not compatible: {0}")] - BackendsIncompatible( - #[from] components::backends::MigrationCompatibilityError, - ), - - #[cfg(test)] - #[error("Test components differ")] - TestComponents(), -} - -/// An error type describing a mismatch between collections of elements that -/// blocks migration. -#[derive(Debug, Error)] -pub enum CollectionCompatibilityError { - #[error( - "Specs have collections with different lengths (self: {0}, other: {1})" - )] - CollectionSize(usize, usize), - - #[error("Collection key {0} present in self but absent from other")] - CollectionKeyAbsent(SpecKey), - - #[error("Spec element {0} mismatched: {1:?}")] - SpecElementMismatch(String, ElementCompatibilityError), -} - -/// The top-level migration compatibility error type. -#[derive(Debug, Error)] -pub enum MigrationCompatibilityError { - #[error("Collection {0} not compatible: {1}")] - CollectionMismatch(String, CollectionCompatibilityError), - - #[error("Spec element {0} not compatible: {1}")] - ElementMismatch(String, ElementCompatibilityError), -} - -/// Implementors of this trait are individual devices or VMM components who can -/// describe inconsistencies using an [`ElementCompatibilityError`] variant. -pub(crate) trait MigrationElement { - /// Returns a string indicating the kind of component this is for diagnostic - /// purposes. - fn kind(&self) -> &'static str; - - /// Returns true if `self` and `other` describe spec elements that are - /// similar enough to permit migration of this element from one VMM to - /// another. - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), ElementCompatibilityError>; -} - -/// This trait implements migration compatibility checks for collection types, -/// which can be incompatible either because of a problem with the collection -/// itself or because of problems with one of the collection's members. -pub(crate) trait MigrationCollection { - fn can_migrate_from_collection( - &self, - other: &Self, - ) -> Result<(), CollectionCompatibilityError>; -} - -impl MigrationCollection for HashMap { - // Two keyed maps of components are compatible if they contain all the same - // keys and if, for each key, the corresponding values are - // migration-compatible. - fn can_migrate_from_collection( - &self, - other: &Self, - ) -> Result<(), CollectionCompatibilityError> { - // If the two maps have different sizes, then they have different key - // sets. - if self.len() != other.len() { - return Err(CollectionCompatibilityError::CollectionSize( - self.len(), - other.len(), - )); - } - - // Each key in `self`'s map must be present in `other`'s map, and the - // corresponding values must be compatible with one another. - for (key, this_val) in self.iter() { - let other_val = other.get(key).ok_or_else(|| { - CollectionCompatibilityError::CollectionKeyAbsent(key.clone()) - })?; - - this_val.can_migrate_from_element(other_val).map_err(|e| { - CollectionCompatibilityError::SpecElementMismatch( - key.clone(), - e, - ) - })?; - } - - Ok(()) - } -} - -impl MigrationCollection for HashSet { - // Two sets of spec keys are compatible if they have all the same members. - fn can_migrate_from_collection( - &self, - other: &Self, - ) -> Result<(), CollectionCompatibilityError> { - if self.len() != other.len() { - return Err(CollectionCompatibilityError::CollectionSize( - self.len(), - other.len(), - )); - } - - for key in self.iter() { - if !other.contains(key) { - return Err(CollectionCompatibilityError::CollectionKeyAbsent( - key.clone(), - )); - } - } - - Ok(()) - } -} - -#[cfg(test)] -mod test { - use super::*; - - #[derive(Clone, Copy, PartialEq, Eq)] - enum TestComponent { - Widget, - Gizmo, - Contraption, - } - - impl MigrationElement for TestComponent { - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), ElementCompatibilityError> { - if self != other { - Err(ElementCompatibilityError::TestComponents()) - } else { - Ok(()) - } - } - - fn kind(&self) -> &'static str { - "TestComponent" - } - } - - // Verifies that the generic compatibility check for maps - // works correctly with a simple test type. - #[test] - fn generic_map_compatibility() { - let m1: HashMap = HashMap::from([ - ("widget".to_string(), TestComponent::Widget), - ("gizmo".to_string(), TestComponent::Gizmo), - ("contraption".to_string(), TestComponent::Contraption), - ]); - - let mut m2 = m1.clone(); - assert!(m1.can_migrate_from_collection(&m2).is_ok()); - - // Mismatched key counts make two maps incompatible. - m2.insert("second_widget".to_string(), TestComponent::Widget); - assert!(m1.can_migrate_from_collection(&m2).is_err()); - m2.remove("second_widget"); - - // Two maps are incompatible if their keys refer to components that are - // not compatible with each other. - *m2.get_mut("gizmo").unwrap() = TestComponent::Contraption; - assert!(m1.can_migrate_from_collection(&m2).is_err()); - *m2.get_mut("gizmo").unwrap() = TestComponent::Gizmo; - - // Two maps are incompatible if they have the same number of keys and - // values, but different sets of key names. - m2.remove("gizmo"); - m2.insert("other_gizmo".to_string(), TestComponent::Gizmo); - assert!(m1.can_migrate_from_collection(&m2).is_err()); - } -} diff --git a/crates/propolis-api-types/src/instance_spec/mod.rs b/crates/propolis-api-types/src/instance_spec/mod.rs index 643eb6d9a..a907eb01f 100644 --- a/crates/propolis-api-types/src/instance_spec/mod.rs +++ b/crates/propolis-api-types/src/instance_spec/mod.rs @@ -159,7 +159,6 @@ use serde::{Deserialize, Serialize}; pub use propolis_types::PciPath; pub mod components; -pub mod migration; pub mod v0; /// Type alias for keys in the instance spec's maps. diff --git a/crates/propolis-api-types/src/instance_spec/v0.rs b/crates/propolis-api-types/src/instance_spec/v0.rs index 344f83fcb..13b5cedbb 100644 --- a/crates/propolis-api-types/src/instance_spec/v0.rs +++ b/crates/propolis-api-types/src/instance_spec/v0.rs @@ -21,14 +21,7 @@ use std::collections::HashMap; -use crate::instance_spec::{ - components, - migration::{ - ElementCompatibilityError, MigrationCollection, - MigrationCompatibilityError, MigrationElement, - }, - SpecKey, -}; +use crate::instance_spec::{components, SpecKey}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -39,53 +32,12 @@ pub enum StorageDeviceV0 { NvmeDisk(components::devices::NvmeDisk), } -impl MigrationElement for StorageDeviceV0 { - fn kind(&self) -> &'static str { - match self { - StorageDeviceV0::VirtioDisk(_) => "StorageDevice(VirtioDisk)", - StorageDeviceV0::NvmeDisk(_) => "StorageDevice(NvmeDisk)", - } - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), super::migration::ElementCompatibilityError> { - match (self, other) { - (Self::VirtioDisk(this), Self::VirtioDisk(other)) => { - this.can_migrate_from_element(other) - } - (Self::NvmeDisk(this), Self::NvmeDisk(other)) => { - this.can_migrate_from_element(other) - } - (_, _) => Err(ElementCompatibilityError::ComponentsIncomparable( - self.kind(), - other.kind(), - )), - } - } -} - #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] #[serde(deny_unknown_fields, tag = "type", content = "component")] pub enum NetworkDeviceV0 { VirtioNic(components::devices::VirtioNic), } -impl MigrationElement for NetworkDeviceV0 { - fn kind(&self) -> &'static str { - "NetworkDevice(VirtioNic)" - } - - fn can_migrate_from_element( - &self, - other: &Self, - ) -> Result<(), ElementCompatibilityError> { - let (Self::VirtioNic(this), Self::VirtioNic(other)) = (self, other); - this.can_migrate_from_element(other) - } -} - #[derive(Default, Clone, Deserialize, Serialize, Debug, JsonSchema)] #[serde(deny_unknown_fields)] pub struct DeviceSpecV0 { @@ -119,64 +71,6 @@ pub struct DeviceSpecV0 { pub p9fs: Option, } -impl DeviceSpecV0 { - pub fn can_migrate_devices_from( - &self, - other: &Self, - ) -> Result<(), MigrationCompatibilityError> { - self.board.can_migrate_from_element(&other.board).map_err(|e| { - MigrationCompatibilityError::ElementMismatch("board".to_string(), e) - })?; - - self.storage_devices - .can_migrate_from_collection(&other.storage_devices) - .map_err(|e| { - MigrationCompatibilityError::CollectionMismatch( - "storage devices".to_string(), - e, - ) - })?; - - self.network_devices - .can_migrate_from_collection(&other.network_devices) - .map_err(|e| { - MigrationCompatibilityError::CollectionMismatch( - "storage devices".to_string(), - e, - ) - })?; - - self.serial_ports - .can_migrate_from_collection(&other.serial_ports) - .map_err(|e| { - MigrationCompatibilityError::CollectionMismatch( - "serial ports".to_string(), - e, - ) - })?; - - self.pci_pci_bridges - .can_migrate_from_collection(&other.pci_pci_bridges) - .map_err(|e| { - MigrationCompatibilityError::CollectionMismatch( - "PCI bridges".to_string(), - e, - ) - })?; - - self.qemu_pvpanic - .can_migrate_from_element(&other.qemu_pvpanic) - .map_err(|e| { - MigrationCompatibilityError::ElementMismatch( - "QEMU PVPANIC device".to_string(), - e, - ) - })?; - - Ok(()) - } -} - #[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] #[serde(deny_unknown_fields, tag = "type", content = "component")] pub enum StorageBackendV0 {