From 7c73f713f35080f9df4e9e89ea5c32ac20325ecf Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 14 Nov 2024 17:24:53 +0000 Subject: [PATCH 1/3] api: one ensure API to rule them all Remove the existing instance ensure APIs and collapse them into a single API that takes a set of instance properties and a mechanism for creating the instance, which can be either creating a new VM from a spec or initializing as a live migration target. In the latter case, the client can also supply a list of instance spec components that the client wishes to override the components in the source's spec. This gives Omicron a way to replace backend information (Crucible generation number, host VNIC names) that changes over live migration. Remove all of the instance ensure types that supported the old ensure interface. One notable consequence of this is that the `DiskRequest` type also caused propolis-server's versions of Crucible's `VolumeConstructionRequest` and `CrucibleOpts` types to appear in the generated client. Omicron code tends to pull these types in from propolis-client (often transitively through sled-agent-client). This is a useful way of making sure Omicron components serialize/deserialize VCRs the same way Propolis does, so preserve it by re-exporting these types from propolis-client. Remove the parts of the mock server that validated disk, NIC, and cloud-init volume requests. The Omicron tests that use the mock server primarily check that its serial console interface works as expected and don't heavily exercise these checks. (Omicron CI's end-to-end test will fail if sled-agent mishandles the Propolis ensure API.) Finally, remove a few other unused types from the propolis_api_types crate. Tests: cargo test, PHD, driving ad hoc VMs with propolis-cli. --- Cargo.lock | 1 + bin/mock-server/src/lib/api_types.rs | 1 - bin/mock-server/src/lib/copied.rs | 60 --- bin/mock-server/src/lib/lib.rs | 68 +-- bin/propolis-cli/src/main.rs | 155 ++++-- .../src/lib/migrate/destination.rs | 18 +- .../src/lib/migrate/preamble.rs | 119 ++-- bin/propolis-server/src/lib/server.rs | 146 ++--- .../src/lib/spec/api_request.rs | 138 ----- bin/propolis-server/src/lib/spec/builder.rs | 178 ++---- bin/propolis-server/src/lib/spec/mod.rs | 26 +- bin/propolis-server/src/lib/vm/ensure.rs | 59 +- bin/propolis-server/src/lib/vm/mod.rs | 15 +- .../src/lib/vm/state_driver.rs | 5 +- crates/propolis-api-types/src/lib.rs | 134 ++--- lib/propolis-client/Cargo.toml | 1 + lib/propolis-client/src/lib.rs | 17 +- openapi/propolis-server.json | 507 +++++------------- phd-tests/framework/src/disk/crucible.rs | 13 +- phd-tests/framework/src/test_vm/mod.rs | 158 +++--- phd-tests/framework/src/test_vm/spec.rs | 81 +-- phd-tests/tests/src/ensure_api.rs | 33 -- phd-tests/tests/src/lib.rs | 1 - 23 files changed, 578 insertions(+), 1356 deletions(-) delete mode 100644 bin/mock-server/src/lib/copied.rs delete mode 100644 bin/propolis-server/src/lib/spec/api_request.rs delete mode 100644 phd-tests/tests/src/ensure_api.rs diff --git a/Cargo.lock b/Cargo.lock index a4282d133..30c378b60 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4298,6 +4298,7 @@ version = "0.1.0" dependencies = [ "async-trait", "base64 0.21.7", + "crucible-client-types", "futures", "progenitor", "propolis_api_types", diff --git a/bin/mock-server/src/lib/api_types.rs b/bin/mock-server/src/lib/api_types.rs index 0f9586007..c39b8ea33 100644 --- a/bin/mock-server/src/lib/api_types.rs +++ b/bin/mock-server/src/lib/api_types.rs @@ -22,7 +22,6 @@ impl TryFrom for propolis_types::PciPath { .map_err(|e| e.to_string()) } } -pub use propolis_types::PciPath; // Duplicate the parameter types for the endpoints related to the serial console diff --git a/bin/mock-server/src/lib/copied.rs b/bin/mock-server/src/lib/copied.rs deleted file mode 100644 index 8692394c6..000000000 --- a/bin/mock-server/src/lib/copied.rs +++ /dev/null @@ -1,60 +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/. - -//! Bits copied from propolis-server, rather than splitting them out into some -//! shared dependency - -use crate::api_types::{types as api, PciPath}; - -use thiserror::Error; - -#[derive(Clone, Copy, Debug)] -pub(crate) enum SlotType { - Nic, - Disk, - CloudInit, -} - -#[allow(unused)] -#[derive(Debug, Error)] -pub(crate) enum ServerSpecBuilderError { - #[error("The string {0} could not be converted to a PCI path")] - PciPathNotParseable(String), - - #[error( - "Could not translate PCI slot {0} for device type {1:?} to a PCI path" - )] - PciSlotInvalid(u8, SlotType), - - #[error("Unrecognized storage device interface {0}")] - UnrecognizedStorageDevice(String), - - #[error("Unrecognized storage backend type {0}")] - UnrecognizedStorageBackend(String), - - #[error("Device {0} requested missing backend {1}")] - DeviceMissingBackend(String, String), - - #[error("Error in server config TOML: {0}")] - ConfigTomlError(String), - - #[error("Error serializing {0} into spec element: {1}")] - SerializationError(String, serde_json::error::Error), -} - -pub(crate) fn slot_to_pci_path( - slot: api::Slot, - ty: SlotType, -) -> Result { - match ty { - // Slots for NICS: 0x08 -> 0x0F - SlotType::Nic if slot.0 <= 7 => PciPath::new(0, slot.0 + 0x8, 0), - // Slots for Disks: 0x10 -> 0x17 - SlotType::Disk if slot.0 <= 7 => PciPath::new(0, slot.0 + 0x10, 0), - // Slot for CloudInit - SlotType::CloudInit if slot.0 == 0 => PciPath::new(0, slot.0 + 0x18, 0), - _ => return Err(ServerSpecBuilderError::PciSlotInvalid(slot.0, ty)), - } - .map_err(|_| ServerSpecBuilderError::PciSlotInvalid(slot.0, ty)) -} diff --git a/bin/mock-server/src/lib/lib.rs b/bin/mock-server/src/lib/lib.rs index 9603fb882..bd245b86b 100644 --- a/bin/mock-server/src/lib/lib.rs +++ b/bin/mock-server/src/lib/lib.rs @@ -4,17 +4,15 @@ //! Implementation of a mock Propolis server -use std::io::{Error as IoError, ErrorKind}; use std::sync::Arc; -use base64::Engine; use dropshot::{ channel, endpoint, ApiDescription, HttpError, HttpResponseCreated, HttpResponseOk, HttpResponseUpdatedNoContent, Query, RequestContext, TypedBody, WebsocketConnection, }; use futures::SinkExt; -use slog::{error, info, o, Logger}; +use slog::{error, o, Logger}; use thiserror::Error; use tokio::sync::{watch, Mutex}; use tokio_tungstenite::tungstenite::protocol::{Role, WebSocketConfig}; @@ -22,10 +20,7 @@ use tokio_tungstenite::tungstenite::Message; use tokio_tungstenite::WebSocketStream; mod api_types; -mod copied; - -use crate::copied::{slot_to_pci_path, SlotType}; -use api_types::types as api; +use api_types::types::{self as api, InstanceEnsureRequest}; #[derive(Debug, Eq, PartialEq, Error)] pub enum Error { @@ -141,12 +136,7 @@ async fn instance_ensure( ) -> Result, HttpError> { let server_context = rqctx.context(); let request = request.into_inner(); - let (properties, nics, disks, cloud_init_bytes) = ( - request.properties, - request.nics, - request.disks, - request.cloud_init_bytes, - ); + let InstanceEnsureRequest { properties, .. } = request; // Handle an already-initialized instance let mut instance = server_context.instance.lock().await; @@ -160,58 +150,6 @@ async fn instance_ensure( migrate: None, })); } - - // Perform some basic validation of the requested properties - for nic in &nics { - info!(server_context.log, "Creating NIC: {:#?}", nic); - slot_to_pci_path(nic.slot, SlotType::Nic).map_err(|e| { - let err = IoError::new( - ErrorKind::InvalidData, - format!("Cannot parse vnic PCI: {}", e), - ); - HttpError::for_internal_error(format!( - "Cannot build instance: {}", - err - )) - })?; - } - - for disk in &disks { - info!(server_context.log, "Creating Disk: {:#?}", disk); - slot_to_pci_path(disk.slot, SlotType::Disk).map_err(|e| { - let err = IoError::new( - ErrorKind::InvalidData, - format!("Cannot parse disk PCI: {}", e), - ); - HttpError::for_internal_error(format!( - "Cannot build instance: {}", - err - )) - })?; - info!(server_context.log, "Disk {} created successfully", disk.name); - } - - if let Some(cloud_init_bytes) = &cloud_init_bytes { - info!(server_context.log, "Creating cloud-init disk"); - slot_to_pci_path(api::Slot(0), SlotType::CloudInit).map_err(|e| { - let err = IoError::new(ErrorKind::InvalidData, e.to_string()); - HttpError::for_internal_error(format!( - "Cannot build instance: {}", - err - )) - })?; - base64::engine::general_purpose::STANDARD - .decode(cloud_init_bytes) - .map_err(|e| { - let err = IoError::new(ErrorKind::InvalidInput, e.to_string()); - HttpError::for_internal_error(format!( - "Cannot build instance: {}", - err - )) - })?; - info!(server_context.log, "cloud-init disk created"); - } - *instance = Some(InstanceContext::new(properties, &server_context.log)); Ok(HttpResponseCreated(api::InstanceEnsureResponse { migrate: None })) } diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 78c8069ec..543bb6b82 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -2,6 +2,7 @@ // 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::HashMap; use std::fs::File; use std::io::BufReader; use std::path::{Path, PathBuf}; @@ -17,13 +18,14 @@ use futures::{future, SinkExt}; use newtype_uuid::{GenericUuid, TypedUuid, TypedUuidKind, TypedUuidTag}; use propolis_client::types::{ BlobStorageBackend, Board, Chipset, ComponentV0, CrucibleStorageBackend, - I440Fx, InstanceMetadata, InstanceSpecEnsureRequest, - InstanceSpecGetResponse, InstanceSpecStatus, InstanceSpecV0, NvmeDisk, - QemuPvpanic, SerialPort, SerialPortNumber, VersionedInstanceSpec, + I440Fx, InstanceEnsureRequest, InstanceInitializationMethod, + InstanceMetadata, InstanceSpecGetResponse, InstanceSpecV0, NvmeDisk, + QemuPvpanic, ReplacementComponent, SerialPort, SerialPortNumber, VirtioDisk, }; use propolis_client::PciPath; use propolis_config_toml::spec::SpecConfig; +use serde::{Deserialize, Serialize}; use slog::{o, Drain, Level, Logger}; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio_tungstenite::tungstenite::{ @@ -35,8 +37,8 @@ use uuid::Uuid; use propolis_client::{ support::{InstanceSerialConsoleHelper, WSClientOffset}, types::{ - DiskRequest, InstanceMigrateInitiateRequest, InstanceProperties, - InstanceStateRequested, InstanceVcrReplace, MigrationState, + InstanceProperties, InstanceStateRequested, InstanceVcrReplace, + MigrationState, }, Client, }; @@ -199,40 +201,67 @@ fn add_component_to_spec( Ok(()) } -fn add_disk_request_to_spec( - spec: &mut InstanceSpecV0, - req: &DiskRequest, -) -> anyhow::Result<()> { - let slot = req.slot.0 + 0x10; - let backend_name = format!("{}-backend", req.name); - let pci_path = PciPath::new(0, slot, 0) - .with_context(|| format!("processing disk request {:?}", req.name))?; - let device_spec = match req.device.as_ref() { - "virtio" => ComponentV0::VirtioDisk(VirtioDisk { - backend_name: backend_name.clone(), - pci_path, - }), - "nvme" => ComponentV0::NvmeDisk(NvmeDisk { - backend_name: backend_name.clone(), - pci_path, - }), - _ => anyhow::bail!( - "invalid device type in disk request: {:?}", - req.device - ), - }; +/// A legacy Propolis API disk request, preserved here for compatibility with +/// the `--crucible-disks` option. +#[derive(Clone, Debug, Deserialize, Serialize)] +struct DiskRequest { + name: String, + slot: u8, + read_only: bool, + device: String, + volume_construction_request: propolis_client::VolumeConstructionRequest, +} + +#[derive(Clone, Debug)] +struct ParsedDiskRequest { + device_name: String, + device_spec: ComponentV0, + backend_name: String, + backend_spec: CrucibleStorageBackend, +} + +impl DiskRequest { + fn parse(&self) -> anyhow::Result { + // Preserve compatibility with the old Propolis API by adding 16 to the + // slot number, which must be between 0 and 7 inclusive. + if !(0..8).contains(&self.slot) { + anyhow::bail!("disk request slots must be in [0..7]"); + } - let backend_spec = - ComponentV0::CrucibleStorageBackend(CrucibleStorageBackend { - readonly: req.read_only, + let slot = self.slot + 0x10; + let backend_name = format!("{}-backend", self.name); + let pci_path = PciPath::new(0, slot, 0).with_context(|| { + format!("processing disk request {:?}", self.name) + })?; + let device_spec = match self.device.as_ref() { + "virtio" => ComponentV0::VirtioDisk(VirtioDisk { + backend_name: backend_name.clone(), + pci_path, + }), + "nvme" => ComponentV0::NvmeDisk(NvmeDisk { + backend_name: backend_name.clone(), + pci_path, + }), + _ => anyhow::bail!( + "invalid device type in disk request: {:?}", + self.device + ), + }; + + let backend_spec = CrucibleStorageBackend { + readonly: self.read_only, request_json: serde_json::to_string( - &req.volume_construction_request, + &self.volume_construction_request, )?, - }); + }; - add_component_to_spec(spec, req.name.clone(), device_spec)?; - add_component_to_spec(spec, backend_name, backend_spec)?; - Ok(()) + Ok(ParsedDiskRequest { + device_name: self.name.clone(), + device_spec, + backend_name, + backend_spec, + }) + } } impl VmConfig { @@ -284,7 +313,18 @@ impl VmConfig { .iter() .flatten() { - add_disk_request_to_spec(&mut spec, disk_request)?; + let ParsedDiskRequest { + device_name, + device_spec, + backend_name, + backend_spec, + } = disk_request.parse()?; + add_component_to_spec(&mut spec, device_name, device_spec)?; + add_component_to_spec( + &mut spec, + backend_name, + ComponentV0::CrucibleStorageBackend(backend_spec), + )?; } if let Some(cloud_init) = self.cloud_init.as_ref() { @@ -437,15 +477,14 @@ async fn new_instance( metadata, }; - let request = InstanceSpecEnsureRequest { - instance_spec: VersionedInstanceSpec::V0(spec), - migrate: None, + let request = InstanceEnsureRequest { properties, + init: InstanceInitializationMethod::Spec { spec }, }; // Try to create the instance client - .instance_spec_ensure() + .instance_ensure() .body(request) .send() .await @@ -675,7 +714,7 @@ async fn migrate_instance( disks: Vec, ) -> anyhow::Result<()> { // Grab the instance details - let InstanceSpecGetResponse { mut properties, spec, .. } = src_client + let InstanceSpecGetResponse { mut properties, .. } = src_client .instance_spec_get() .send() .await @@ -683,29 +722,37 @@ async fn migrate_instance( .into_inner(); let src_uuid = properties.id; properties.id = dst_uuid; - let InstanceSpecStatus::Present(spec) = spec else { - anyhow::bail!("source instance doesn't have a spec yet"); - }; - let VersionedInstanceSpec::V0(mut spec) = spec; - spec.components.clear(); - for disk in disks.iter() { - add_disk_request_to_spec(&mut spec, disk)?; + let mut replace_components = HashMap::new(); + for disk in disks { + let ParsedDiskRequest { backend_name, backend_spec, .. } = + disk.parse()?; + + let old = replace_components.insert( + backend_name.clone(), + ReplacementComponent::CrucibleStorageBackend(backend_spec), + ); + + if old.is_some() { + anyhow::bail!( + "duplicate backend name {backend_name} in replacement disk \ + list" + ); + } } - let request = InstanceSpecEnsureRequest { + let request = InstanceEnsureRequest { properties, - migrate: Some(InstanceMigrateInitiateRequest { + init: InstanceInitializationMethod::MigrationTarget { migration_id: Uuid::new_v4(), src_addr: src_addr.to_string(), - src_uuid, - }), - instance_spec: VersionedInstanceSpec::V0(spec), + replace_components, + }, }; // Initiate the migration via the destination instance let migration_res = - dst_client.instance_spec_ensure().body(request).send().await?; + dst_client.instance_ensure().body(request).send().await?; let migration_id = migration_res .migrate .as_ref() diff --git a/bin/propolis-server/src/lib/migrate/destination.rs b/bin/propolis-server/src/lib/migrate/destination.rs index 9156a02b2..6d29e1115 100644 --- a/bin/propolis-server/src/lib/migrate/destination.rs +++ b/bin/propolis-server/src/lib/migrate/destination.rs @@ -9,8 +9,9 @@ use propolis::migrate::{ MigrateCtx, MigrateStateError, Migrator, PayloadOffer, PayloadOffers, }; use propolis::vmm; -use propolis_api_types::InstanceMigrateInitiateRequest; +use propolis_api_types::ReplacementComponent; use slog::{error, info, trace, warn}; +use std::collections::BTreeMap; use std::convert::TryInto; use std::io; use std::net::SocketAddr; @@ -36,6 +37,12 @@ use crate::vm::state_publisher::{ use super::protocol::Protocol; use super::MigrateConn; +pub(crate) struct MigrationTargetInfo { + pub migration_id: Uuid, + pub src_addr: SocketAddr, + pub replace_components: BTreeMap, +} + /// The interface to an arbitrary version of the target half of the live /// migration protocol. // @@ -58,7 +65,7 @@ pub(crate) trait DestinationProtocol { /// that the caller can use to run the migration. pub(crate) async fn initiate( log: &slog::Logger, - migrate_info: &InstanceMigrateInitiateRequest, + migrate_info: &MigrationTargetInfo, local_addr: SocketAddr, ) -> Result { let migration_id = migrate_info.migration_id; @@ -350,7 +357,12 @@ impl RonV0 { }?; info!(self.log(), "Destination read Preamble: {:?}", preamble); - let spec = match preamble.amend_spec(ensure_ctx.instance_spec()) { + let spec = match preamble.amend_spec( + &ensure_ctx + .migration_info() + .expect("migration in was requested") + .replace_components, + ) { Ok(spec) => spec, Err(e) => { error!( diff --git a/bin/propolis-server/src/lib/migrate/preamble.rs b/bin/propolis-server/src/lib/migrate/preamble.rs index a2946c556..6729f92e2 100644 --- a/bin/propolis-server/src/lib/migrate/preamble.rs +++ b/bin/propolis-server/src/lib/migrate/preamble.rs @@ -2,12 +2,15 @@ // 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 propolis_api_types::instance_spec::{ - v0::ComponentV0, VersionedInstanceSpec, +use std::collections::BTreeMap; + +use propolis_api_types::{ + instance_spec::{v0::ComponentV0, VersionedInstanceSpec}, + ReplacementComponent, }; use serde::{Deserialize, Serialize}; -use crate::spec::{api_spec_v0::ApiSpecError, Spec, StorageBackend}; +use crate::spec::{api_spec_v0::ApiSpecError, Spec}; use super::MigrateError; @@ -30,73 +33,65 @@ impl Preamble { /// Any such backends will replace the corresponding backend entries in the /// source spec. If the target spec contains a replacement backend that is /// not present in the source spec, this routine fails. - pub fn amend_spec(self, target_spec: &Spec) -> Result { - let VersionedInstanceSpec::V0(mut source_spec) = self.instance_spec; - for disk in target_spec.disks.values() { - let StorageBackend::Crucible(crucible) = &disk.backend_spec else { - continue; - }; - - let Some(to_amend) = - source_spec.components.get_mut(disk.device_spec.backend_name()) - else { - return Err(MigrateError::InstanceSpecsIncompatible(format!( - "replacement component {} not in source spec", - disk.device_spec.backend_name() - ))); - }; - - if !matches!(to_amend, ComponentV0::CrucibleStorageBackend(_)) { - return Err(MigrateError::InstanceSpecsIncompatible(format!( - "component {} is not a Crucible backend in the source spec", - disk.device_spec.backend_name() - ))); - } - - *to_amend = ComponentV0::CrucibleStorageBackend(crucible.clone()); - } - - for nic in target_spec.nics.values() { - let Some(to_amend) = - source_spec.components.get_mut(&nic.device_spec.backend_name) - else { - return Err(MigrateError::InstanceSpecsIncompatible(format!( - "replacement component {} not in source spec", - nic.device_spec.backend_name - ))); - }; - - if !matches!(to_amend, ComponentV0::VirtioNetworkBackend(_)) { - return Err(MigrateError::InstanceSpecsIncompatible(format!( - "component {} is not a virtio network backend \ - in the source spec", - nic.device_spec.backend_name - ))); - } - - *to_amend = - ComponentV0::VirtioNetworkBackend(nic.backend_spec.clone()); + pub fn amend_spec( + self, + replacements: &BTreeMap, + ) -> Result { + fn wrong_type_error(id: &str, kind: &str) -> MigrateError { + let msg = + format!("component {id} is not a {kind} in the source spec"); + MigrateError::InstanceSpecsIncompatible(msg) } - #[cfg(not(feature = "omicron-build"))] - if let Some(mig) = &target_spec.migration_failure { - let Some(to_amend) = source_spec.components.get_mut(&mig.name) + let VersionedInstanceSpec::V0(mut source_spec) = self.instance_spec; + for (id, comp) in replacements { + let Some(to_amend) = source_spec.components.get_mut(id.as_str()) else { return Err(MigrateError::InstanceSpecsIncompatible(format!( - "replacement component {} not in source spec", - mig.name + "replacement component {id} not in source spec", ))); }; - if !matches!(to_amend, ComponentV0::MigrationFailureInjector(_)) { - return Err(MigrateError::InstanceSpecsIncompatible(format!( - "component {} is not a migration failure injector \ - in the source spec", - mig.name - ))); + match comp { + #[cfg(feature = "omicron-build")] + ReplacementComponent::MigrationFailureInjector(_) => { + return Err(MigrateError::InstanceSpecsIncompatible( + format!( + "replacing migration failure injector {id} is \ + impossible because the feature is compiled out" + ), + )); + } + + #[cfg(not(feature = "omicron-build"))] + ReplacementComponent::MigrationFailureInjector(comp) => { + let ComponentV0::MigrationFailureInjector(src) = to_amend + else { + return Err(wrong_type_error( + id, + "migration failure injector", + )); + }; + + *src = comp.clone(); + } + ReplacementComponent::CrucibleStorageBackend(comp) => { + let ComponentV0::CrucibleStorageBackend(src) = to_amend + else { + return Err(wrong_type_error(id, "crucible backend")); + }; + + *src = comp.clone(); + } + ReplacementComponent::VirtioNetworkBackend(comp) => { + let ComponentV0::VirtioNetworkBackend(src) = to_amend + else { + return Err(wrong_type_error(id, "viona backend")); + }; + + *src = comp.clone(); + } } - - *to_amend = ComponentV0::MigrationFailureInjector(mig.spec.clone()); } let amended_spec = diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index 0a32d4464..00059b501 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -17,14 +17,10 @@ use std::net::SocketAddrV6; use std::path::PathBuf; use std::sync::Arc; +use crate::migrate::destination::MigrationTargetInfo; +use crate::vm::ensure::VmInitializationMethod; use crate::{ serial::history_buffer::SerialHistoryOffset, - spec::{ - self, - api_spec_v0::ApiSpecError, - builder::{SpecBuilder, SpecBuilderError}, - Spec, - }, vm::{ensure::VmEnsureRequest, VmError}, vnc::{self, VncServer}, }; @@ -40,9 +36,7 @@ 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::{ - self, components::devices::QemuPvpanic, VersionedInstanceSpec, -}; +use propolis_api_types::InstanceInitializationMethod; use rfb::tungstenite::BinaryWs; use slog::{error, warn, Logger}; use tokio::sync::MutexGuard; @@ -117,55 +111,6 @@ impl DropshotEndpointContext { } } -/// Creates an instance spec from an ensure request. (Both types are foreign to -/// this crate, so implementing TryFrom for them is not allowed.) -fn instance_spec_from_request( - request: &api::InstanceEnsureRequest, -) -> Result { - let mut spec_builder = SpecBuilder::new(request.vcpus, request.memory); - - for nic in &request.nics { - spec_builder.add_nic_from_request(nic)?; - } - - for disk in &request.disks { - spec_builder.add_disk_from_request(disk)?; - } - - if let Some(boot_settings) = request.boot_settings.as_ref() { - 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 { - spec_builder.add_cloud_init_from_request(base64.clone())?; - } - - for (name, port) in [ - ("com1", instance_spec::components::devices::SerialPortNumber::Com1), - ("com2", instance_spec::components::devices::SerialPortNumber::Com2), - ("com3", instance_spec::components::devices::SerialPortNumber::Com3), - // SoftNpu uses this port for ASIC management. - #[cfg(not(feature = "falcon"))] - ("com4", instance_spec::components::devices::SerialPortNumber::Com4), - ] { - spec_builder.add_serial_port(name.to_owned(), port)?; - } - - #[cfg(feature = "falcon")] - spec_builder.set_softnpu_com4("com4".to_owned())?; - - 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 /// the DNS lookup until accessed. /// @@ -242,13 +187,16 @@ async fn find_local_nexus_client( } } -async fn instance_ensure_common( +#[endpoint { + method = PUT, + path = "/instance", +}] +async fn instance_ensure( rqctx: RequestContext>, - properties: api::InstanceProperties, - migrate: Option, - instance_spec: Spec, + request: TypedBody, ) -> Result, HttpError> { let server_context = rqctx.context(); + let api::InstanceEnsureRequest { properties, init } = request.into_inner(); let oximeter_registry = server_context .static_config .metrics @@ -270,7 +218,29 @@ async fn instance_ensure_common( local_server_addr: rqctx.server.local_addr, }; - let request = VmEnsureRequest { properties, migrate, instance_spec }; + let vm_init = match init { + InstanceInitializationMethod::Spec { spec } => spec + .try_into() + .map(VmInitializationMethod::Spec) + .map_err(|e| e.to_string()), + InstanceInitializationMethod::MigrationTarget { + migration_id, + src_addr, + replace_components, + } => Ok(VmInitializationMethod::Migration(MigrationTargetInfo { + migration_id, + src_addr, + replace_components, + })), + } + .map_err(|e| { + HttpError::for_bad_request( + None, + format!("failed to generate internal instance spec: {e}"), + ) + })?; + + let request = VmEnsureRequest { properties, init: vm_init }; server_context .vm .ensure(&server_context.log, request, ensure_options) @@ -296,55 +266,6 @@ async fn instance_ensure_common( }) } -#[endpoint { - method = PUT, - path = "/instance", -}] -async fn instance_ensure( - rqctx: RequestContext>, - request: TypedBody, -) -> Result, HttpError> { - let request = request.into_inner(); - let instance_spec = instance_spec_from_request(&request).map_err(|e| { - HttpError::for_bad_request( - None, - format!("failed to generate instance spec from request: {:#?}", e), - ) - })?; - - instance_ensure_common( - rqctx, - request.properties, - request.migrate, - instance_spec, - ) - .await -} - -#[endpoint { - method = PUT, - path = "/instance/spec", -}] -async fn instance_spec_ensure( - rqctx: RequestContext>, - request: TypedBody, -) -> Result, HttpError> { - 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, - format!( - "failed to create internal instance spec from API spec: {:#?}", - e - ), - ) - })?; - - instance_ensure_common(rqctx, request.properties, request.migrate, spec) - .await -} - async fn instance_get_common( rqctx: &RequestContext>, ) -> Result { @@ -718,7 +639,6 @@ async fn instance_issue_nmi( pub fn api() -> ApiDescription> { let mut api = ApiDescription::new(); api.register(instance_ensure).unwrap(); - api.register(instance_spec_ensure).unwrap(); api.register(instance_get).unwrap(); api.register(instance_spec_get).unwrap(); api.register(instance_state_monitor).unwrap(); diff --git a/bin/propolis-server/src/lib/spec/api_request.rs b/bin/propolis-server/src/lib/spec/api_request.rs deleted file mode 100644 index 3cc115976..000000000 --- a/bin/propolis-server/src/lib/spec/api_request.rs +++ /dev/null @@ -1,138 +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/. - -//! Converts device descriptions from an -//! [`propolis_api_types::InstanceEnsureRequest`] into elements that can be -//! added to a spec. - -use propolis_api_types::{ - instance_spec::{ - components::{ - backends::{ - BlobStorageBackend, CrucibleStorageBackend, - VirtioNetworkBackend, - }, - devices::{NvmeDisk, VirtioDisk, VirtioNic}, - }, - PciPath, - }, - DiskRequest, NetworkInterfaceRequest, Slot, -}; -use thiserror::Error; - -use super::{ - Disk, Nic, ParsedDiskRequest, ParsedNicRequest, StorageBackend, - StorageDevice, -}; - -#[derive(Debug, Error)] -pub(crate) enum DeviceRequestError { - #[error("invalid storage interface {0} for disk in slot {1}")] - InvalidStorageInterface(String, u8), - - #[error("invalid PCI slot {0} for device type {1:?}")] - PciSlotInvalid(u8, SlotType), - - #[error("error serializing {0}")] - SerializationError(String, #[source] serde_json::error::Error), -} - -/// A type of PCI device. Device numbers on the PCI bus are partitioned by slot -/// type. If a client asks to attach a device of type X to PCI slot Y, the -/// server will assign the Yth device number in X's partition. The partitioning -/// scheme is defined by the implementation of the `slot_to_pci_path` utility -/// function. -#[derive(Clone, Copy, Debug)] -pub(crate) enum SlotType { - Nic, - Disk, - CloudInit, -} - -/// Translates a device type and PCI slot (as presented in an instance creation -/// request) into a concrete PCI path. See the documentation for [`SlotType`]. -fn slot_to_pci_path( - slot: Slot, - ty: SlotType, -) -> Result { - match ty { - // Slots for NICS: 0x08 -> 0x0F - SlotType::Nic if slot.0 <= 7 => PciPath::new(0, slot.0 + 0x8, 0), - // Slots for Disks: 0x10 -> 0x17 - SlotType::Disk if slot.0 <= 7 => PciPath::new(0, slot.0 + 0x10, 0), - // Slot for CloudInit - SlotType::CloudInit if slot.0 == 0 => PciPath::new(0, slot.0 + 0x18, 0), - _ => return Err(DeviceRequestError::PciSlotInvalid(slot.0, ty)), - } - .map_err(|_| DeviceRequestError::PciSlotInvalid(slot.0, ty)) -} - -pub(super) fn parse_disk_from_request( - disk: &DiskRequest, -) -> Result { - let pci_path = slot_to_pci_path(disk.slot, SlotType::Disk)?; - let device_name = disk.name.clone(); - let backend_name = format!("{}-backend", disk.name); - let device_spec = match disk.device.as_ref() { - "virtio" => { - StorageDevice::Virtio(VirtioDisk { backend_name, pci_path }) - } - "nvme" => StorageDevice::Nvme(NvmeDisk { backend_name, pci_path }), - _ => { - return Err(DeviceRequestError::InvalidStorageInterface( - disk.device.clone(), - disk.slot.0, - )) - } - }; - - 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(ParsedDiskRequest { - name: device_name, - disk: Disk { device_spec, backend_spec }, - }) -} - -pub(super) fn parse_cloud_init_from_request( - base64: String, -) -> Result { - let name = "cloud-init"; - let pci_path = slot_to_pci_path(Slot(0), SlotType::CloudInit)?; - let backend_name = "cloud-init-backend".to_string(); - let backend_spec = - StorageBackend::Blob(BlobStorageBackend { base64, readonly: true }); - - let device_spec = - StorageDevice::Virtio(VirtioDisk { backend_name, pci_path }); - - Ok(ParsedDiskRequest { - name: name.to_owned(), - disk: Disk { device_spec, backend_spec }, - }) -} - -pub(super) fn parse_nic_from_request( - nic: &NetworkInterfaceRequest, -) -> Result { - 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 = VirtioNic { - backend_name: backend_name.clone(), - interface_id: nic.interface_id, - pci_path, - }; - - let backend_spec = VirtioNetworkBackend { vnic_name: nic.name.to_string() }; - Ok(ParsedNicRequest { - name: device_name, - nic: Nic { device_spec, backend_spec }, - }) -} diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index 877317b9a..8bbd4e195 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -7,15 +7,12 @@ use std::collections::{BTreeSet, HashSet}; use cpuid_utils::CpuidMapConversionError; -use propolis_api_types::{ - instance_spec::{ - components::{ - board::{Board as InstanceSpecBoard, Chipset, I440Fx}, - devices::{PciPciBridge, SerialPortNumber}, - }, - PciPath, +use propolis_api_types::instance_spec::{ + components::{ + board::Board as InstanceSpecBoard, + devices::{PciPciBridge, SerialPortNumber}, }, - DiskRequest, NetworkInterfaceRequest, + PciPath, }; use thiserror::Error; @@ -27,7 +24,6 @@ use propolis_api_types::instance_spec::components::devices::{ use crate::spec::SerialPortDevice; use super::{ - api_request::{self, DeviceRequestError}, Board, BootOrderEntry, BootSettings, Disk, Nic, QemuPvpanic, SerialPort, }; @@ -40,9 +36,6 @@ use super::SoftNpuPort; /// Errors that can arise while building an instance spec from component parts. #[derive(Debug, Error)] pub(crate) enum SpecBuilderError { - #[error("error parsing device in ensure request")] - DeviceRequest(#[from] DeviceRequestError), - #[error("device {0} has the same name as its backend")] DeviceAndBackendNamesIdentical(String), @@ -81,19 +74,6 @@ pub(crate) struct SpecBuilder { } impl SpecBuilder { - pub fn new(cpus: u8, memory_mb: u64) -> Self { - let board = Board { - cpus, - memory_mb, - chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }), - }; - - Self { - spec: super::Spec { board, ..Default::default() }, - ..Default::default() - } - } - pub(super) fn with_instance_spec_board( board: InstanceSpecBoard, ) -> Result { @@ -121,28 +101,6 @@ impl SpecBuilder { }) } - /// Converts an HTTP API request to add a NIC to an instance into - /// device/backend entries in the spec under construction. - pub fn add_nic_from_request( - &mut self, - nic: &NetworkInterfaceRequest, - ) -> Result<(), SpecBuilderError> { - let parsed = api_request::parse_nic_from_request(nic)?; - self.add_network_device(parsed.name, parsed.nic)?; - Ok(()) - } - - /// Converts an HTTP API request to add a disk to an instance into - /// device/backend entries in the spec under construction. - pub fn add_disk_from_request( - &mut self, - disk: &DiskRequest, - ) -> Result<(), SpecBuilderError> { - let parsed = api_request::parse_disk_from_request(disk)?; - self.add_storage_device(parsed.name, parsed.disk)?; - Ok(()) - } - /// Sets the spec's boot order to the list of disk devices specified in /// `boot_options`. /// @@ -177,17 +135,6 @@ impl SpecBuilder { Ok(()) } - /// Converts an HTTP API request to add a cloud-init disk to an instance - /// into device/backend entries in the spec under construction. - pub fn add_cloud_init_from_request( - &mut self, - base64: String, - ) -> Result<(), SpecBuilderError> { - let parsed = api_request::parse_cloud_init_from_request(base64)?; - self.add_storage_device(parsed.name, parsed.disk)?; - Ok(()) - } - /// Adds a PCI path to this builder's record of PCI locations with an /// attached device. If the path is already in use, returns an error. fn register_pci_device( @@ -336,33 +283,26 @@ impl SpecBuilder { } #[cfg(feature = "falcon")] - pub fn set_softnpu_com4( + pub fn set_softnpu_pci_port( &mut self, - name: String, + pci_port: SoftNpuPciPort, ) -> Result<&Self, SpecBuilderError> { - if self.component_names.contains(&name) { - return Err(SpecBuilderError::ComponentNameInUse(name)); + // SoftNPU squats on COM4. + let id = "com4".to_string(); + let num = SerialPortNumber::Com4; + if self.component_names.contains(&id) { + return Err(SpecBuilderError::ComponentNameInUse(id)); } - let num = SerialPortNumber::Com4; if self.serial_ports.contains(&num) { return Err(SpecBuilderError::SerialPortInUse(num)); } - let desc = SerialPort { num, device: SerialPortDevice::SoftNpu }; - self.spec.serial.insert(name.clone(), desc); - self.component_names.insert(name); - self.serial_ports.insert(num); - Ok(self) - } - - #[cfg(feature = "falcon")] - pub fn set_softnpu_pci_port( - &mut self, - pci_port: SoftNpuPciPort, - ) -> Result<&Self, SpecBuilderError> { self.register_pci_device(pci_port.pci_path)?; self.spec.softnpu.pci_port = Some(pci_port); + self.spec + .serial + .insert(id, SerialPort { num, device: SerialPortDevice::SoftNpu }); Ok(self) } @@ -418,12 +358,10 @@ impl SpecBuilder { #[cfg(test)] mod test { - use propolis_api_types::{ - instance_spec::components::{ - backends::{BlobStorageBackend, VirtioNetworkBackend}, - devices::{VirtioDisk, VirtioNic}, - }, - Slot, VolumeConstructionRequest, + use propolis_api_types::instance_spec::components::{ + backends::{BlobStorageBackend, VirtioNetworkBackend}, + board::{Chipset, I440Fx}, + devices::{VirtioDisk, VirtioNic}, }; use uuid::Uuid; @@ -432,39 +370,51 @@ mod test { use super::*; fn test_builder() -> SpecBuilder { - SpecBuilder::new(4, 512) + let board = Board { + cpus: 4, + memory_mb: 512, + chipset: Chipset::I440Fx(I440Fx { enable_pcie: false }), + }; + + SpecBuilder { + spec: crate::spec::Spec { board, ..Default::default() }, + ..Default::default() + } } #[test] fn duplicate_pci_slot() { let mut builder = test_builder(); - // Adding the same disk device twice should fail. assert!(builder - .add_disk_from_request(&DiskRequest { - name: "disk1".to_string(), - slot: Slot(0), - read_only: true, - device: "nvme".to_string(), - volume_construction_request: VolumeConstructionRequest::File { - id: Uuid::new_v4(), - block_size: 512, - path: "disk1.img".to_string() - }, - }) + .add_storage_device( + "storage".to_owned(), + Disk { + device_spec: StorageDevice::Virtio(VirtioDisk { + backend_name: "storage-backend".to_owned(), + pci_path: PciPath::new(0, 4, 0).unwrap() + }), + backend_spec: StorageBackend::Blob(BlobStorageBackend { + base64: "".to_string(), + readonly: false + }) + } + ) .is_ok()); assert!(builder - .add_disk_from_request(&DiskRequest { - name: "disk2".to_string(), - slot: Slot(0), - read_only: true, - device: "virtio".to_string(), - volume_construction_request: VolumeConstructionRequest::File { - id: Uuid::new_v4(), - block_size: 512, - path: "disk2.img".to_string() - }, - }) + .add_network_device( + "network".to_owned(), + Nic { + device_spec: VirtioNic { + backend_name: "network-backend".to_owned(), + interface_id: Uuid::nil(), + pci_path: PciPath::new(0, 4, 0).unwrap() + }, + backend_spec: VirtioNetworkBackend { + vnic_name: "vnic0".to_owned() + } + } + ) .is_err()); } @@ -488,24 +438,6 @@ mod test { .is_err()); } - #[test] - fn unknown_storage_device_type() { - let mut builder = test_builder(); - assert!(builder - .add_disk_from_request(&DiskRequest { - name: "disk3".to_string(), - slot: Slot(0), - read_only: true, - device: "virtio-scsi".to_string(), - volume_construction_request: VolumeConstructionRequest::File { - id: Uuid::new_v4(), - block_size: 512, - path: "disk3.img".to_string() - }, - }) - .is_err()); - } - #[test] fn device_with_same_name_as_backend() { let mut builder = test_builder(); diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index c211693fd..10258265c 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -43,7 +43,7 @@ use propolis_api_types::instance_spec::components::{ devices::{P9fs, SoftNpuP9, SoftNpuPciPort}, }; -mod api_request; +// mod api_request; pub(crate) mod api_spec_v0; pub(crate) mod builder; @@ -306,27 +306,3 @@ pub struct SoftNpu { pub p9_device: Option, pub p9fs: Option, } - -struct ParsedDiskRequest { - name: String, - disk: Disk, -} - -struct ParsedNicRequest { - name: String, - nic: Nic, -} - -/// Generates NIC device and backend names from the NIC's PCI path. This is -/// needed because the `name` field in a propolis-client -/// `NetworkInterfaceRequest` is actually the name of the host vNIC to bind to, -/// and that can change between incarnations of an instance. The PCI path is -/// unique to each NIC but must remain stable over a migration, so it's suitable -/// for use in this naming scheme. -/// -/// N.B. Migrating a NIC requires the source and target to agree on these names, -/// so changing this routine's behavior will prevent Propolis processes -/// with the old behavior from migrating processes with the new behavior. -fn pci_path_to_nic_names(path: PciPath) -> (String, String) { - (format!("vnic-{}", path), format!("vnic-{}-backend", path)) -} diff --git a/bin/propolis-server/src/lib/vm/ensure.rs b/bin/propolis-server/src/lib/vm/ensure.rs index ae6040f4d..1c0a2080d 100644 --- a/bin/propolis-server/src/lib/vm/ensure.rs +++ b/bin/propolis-server/src/lib/vm/ensure.rs @@ -30,8 +30,8 @@ use std::sync::Arc; use oximeter::types::ProducerRegistry; use oximeter_instruments::kstat::KstatSampler; use propolis_api_types::{ - InstanceEnsureResponse, InstanceMigrateInitiateRequest, - InstanceMigrateInitiateResponse, InstanceProperties, InstanceState, + InstanceEnsureResponse, InstanceMigrateInitiateResponse, + InstanceProperties, InstanceState, }; use slog::{debug, info}; @@ -39,6 +39,7 @@ use crate::{ initializer::{ build_instance, MachineInitializer, MachineInitializerState, }, + migrate::destination::MigrationTargetInfo, spec::Spec, stats::{create_kstat_sampler, VirtualMachine}, vm::{ @@ -55,10 +56,34 @@ use super::{ EnsureOptions, InstanceEnsureResponseTx, VmError, }; +pub(crate) enum VmInitializationMethod { + Spec(Spec), + Migration(MigrationTargetInfo), +} + pub(crate) struct VmEnsureRequest { pub(crate) properties: InstanceProperties, - pub(crate) migrate: Option, - pub(crate) instance_spec: Spec, + pub(crate) init: VmInitializationMethod, +} + +impl VmEnsureRequest { + pub(crate) fn is_migration(&self) -> bool { + matches!(self.init, VmInitializationMethod::Migration(_)) + } + + pub(crate) fn migration_info(&self) -> Option<&MigrationTargetInfo> { + match &self.init { + VmInitializationMethod::Spec(_) => None, + VmInitializationMethod::Migration(info) => Some(info), + } + } + + pub(crate) fn spec(&self) -> Option<&Spec> { + match &self.init { + VmInitializationMethod::Spec(spec) => Some(spec), + VmInitializationMethod::Migration(_) => None, + } + } } /// Holds state about an instance ensure request that has not yet produced any @@ -97,18 +122,25 @@ impl<'a> VmEnsureNotStarted<'a> { } } - pub(crate) fn instance_spec(&self) -> &Spec { - &self.ensure_request.instance_spec - } - pub(crate) fn state_publisher(&mut self) -> &mut StatePublisher { self.state_publisher } + pub(crate) fn migration_info(&self) -> Option<&MigrationTargetInfo> { + self.ensure_request.migration_info() + } + pub(crate) async fn create_objects_from_request( self, ) -> anyhow::Result> { - let spec = self.ensure_request.instance_spec.clone(); + let spec = self + .ensure_request + .spec() + .expect( + "create_objects_from_request is called with an explicit spec", + ) + .clone(); + self.create_objects(spec).await } @@ -129,9 +161,10 @@ impl<'a> VmEnsureNotStarted<'a> { let input_queue = Arc::new(InputQueue::new( self.log.new(slog::o!("component" => "request_queue")), - match &self.ensure_request.migrate { - Some(_) => InstanceAutoStart::Yes, - None => InstanceAutoStart::No, + if self.ensure_request.is_migration() { + InstanceAutoStart::Yes + } else { + InstanceAutoStart::No }, )); @@ -267,7 +300,7 @@ impl<'a> VmEnsureObjectsCreated<'a> { // state and using the state change API to send commands to the state // driver. let _ = self.ensure_response_tx.send(Ok(InstanceEnsureResponse { - migrate: self.ensure_request.migrate.as_ref().map(|req| { + migrate: self.ensure_request.migration_info().map(|req| { InstanceMigrateInitiateResponse { migration_id: req.migration_id, } diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index c820b7dea..f680702fc 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -562,13 +562,13 @@ impl Vm { &log_for_driver, InstanceStateMonitorResponse { gen: 1, - state: if ensure_request.migrate.is_some() { + state: if ensure_request.is_migration() { InstanceState::Migrating } else { InstanceState::Creating }, migration: InstanceMigrateStatusResponse { - migration_in: ensure_request.migrate.as_ref().map(|req| { + migration_in: ensure_request.migration_info().map(|req| { InstanceMigrationStatus { id: req.migration_id, state: MigrationState::Sync, @@ -598,10 +598,13 @@ impl Vm { }; let properties = ensure_request.properties.clone(); - let spec = if ensure_request.migrate.is_some() { - MaybeSpec::WaitingForMigrationSource - } else { - MaybeSpec::Present(ensure_request.instance_spec.clone()) + let spec = match &ensure_request.init { + ensure::VmInitializationMethod::Spec(s) => { + MaybeSpec::Present(s.clone()) + } + ensure::VmInitializationMethod::Migration(_) => { + MaybeSpec::WaitingForMigrationSource + } }; let vm_for_driver = self.clone(); diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index 73e1768be..e565f96da 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -304,8 +304,7 @@ pub(super) async fn run_state_driver( // new external callers can access its objects or services. match vmm_rt_hdl .spawn(async move { - let output = - state_driver.run(ensure_request.migrate.is_some()).await; + let output = state_driver.run(ensure_request.is_migration()).await; vm.set_rundown().await; output }) @@ -335,7 +334,7 @@ async fn create_and_activate_vm<'a>( state_publisher, ); - if let Some(migrate_request) = ensure_request.migrate.as_ref() { + if let Some(migrate_request) = ensure_request.migration_info() { let migration = match crate::migrate::destination::initiate( log, migrate_request, diff --git a/crates/propolis-api-types/src/lib.rs b/crates/propolis-api-types/src/lib.rs index db007667c..1944b3b13 100644 --- a/crates/propolis-api-types/src/lib.rs +++ b/crates/propolis-api-types/src/lib.rs @@ -4,8 +4,9 @@ //! Definitions for types exposed by the propolis-server API -use std::{fmt, net::SocketAddr}; +use std::{collections::BTreeMap, fmt, net::SocketAddr}; +use instance_spec::v0::InstanceSpecV0; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -15,7 +16,9 @@ use uuid::Uuid; pub use crate::instance_spec::components::devices::{ BootOrderEntry, BootSettings, }; -use crate::instance_spec::VersionedInstanceSpec; +use crate::instance_spec::{ + components, v0::ComponentV0, VersionedInstanceSpec, +}; // Re-export volume construction requests since they're part of a disk request. pub use crucible_client_types::VolumeConstructionRequest; @@ -48,36 +51,49 @@ pub struct InstanceMetadata { pub sled_model: String, } +/// Describes an instance spec component that should be replaced during a live +/// migration. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct InstanceEnsureRequest { - pub properties: InstanceProperties, - - /// Number of vCPUs to be allocated to the Instance. - pub vcpus: u8, - - /// Size of memory allocated to the Instance, in MiB. - pub memory: u64, - - #[serde(default)] - pub nics: Vec, - - #[serde(default)] - pub disks: Vec, - - #[serde(default)] - pub boot_settings: Option, - - pub migrate: Option, +#[serde(deny_unknown_fields, tag = "component", content = "spec")] +pub enum ReplacementComponent { + MigrationFailureInjector(components::devices::MigrationFailureInjector), + CrucibleStorageBackend(components::backends::CrucibleStorageBackend), + VirtioNetworkBackend(components::backends::VirtioNetworkBackend), +} + +impl From for instance_spec::v0::ComponentV0 { + fn from(value: ReplacementComponent) -> Self { + match value { + ReplacementComponent::MigrationFailureInjector(c) => { + ComponentV0::MigrationFailureInjector(c) + } + ReplacementComponent::CrucibleStorageBackend(c) => { + ComponentV0::CrucibleStorageBackend(c) + } + ReplacementComponent::VirtioNetworkBackend(c) => { + ComponentV0::VirtioNetworkBackend(c) + } + } + } +} - // base64 encoded cloud-init ISO - pub cloud_init_bytes: Option, +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +#[serde(tag = "method", content = "value")] +pub enum InstanceInitializationMethod { + Spec { + spec: InstanceSpecV0, + }, + MigrationTarget { + migration_id: Uuid, + src_addr: SocketAddr, + replace_components: BTreeMap, + }, } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct InstanceSpecEnsureRequest { +pub struct InstanceEnsureRequest { pub properties: InstanceProperties, - pub instance_spec: VersionedInstanceSpec, - pub migrate: Option, + pub init: InstanceInitializationMethod, } #[derive(Clone, Deserialize, Serialize, JsonSchema)] @@ -301,72 +317,6 @@ pub enum InstanceSerialConsoleControlMessage { Migrating { destination: SocketAddr, from_start: u64 }, } -/// Describes how to connect to one or more storage agent services. -#[derive(Clone, Deserialize, Serialize, JsonSchema)] -pub struct StorageAgentDescription { - /// Addresses of storage agents. - pub agents: Vec, - - /// Opaque key material for encryption and decryption. - /// May become more structured as encryption scheme is solidified. - pub key: Vec, - - /// Minimum number of redundant copies of a block which must - /// be written until data is considered "persistent". - pub write_redundancy_threshold: u32, -} - -/// Refer to RFD 135 for more information on Virtual Storage Interfaces. -/// This describes the type of disk which should be exposed to the guest VM. -#[derive(Clone, Copy, Deserialize, Serialize, JsonSchema)] -pub enum DiskType { - NVMe, - VirtioBlock, -} - -/// Describes a virtual disk. -#[derive(Clone, Deserialize, Serialize, JsonSchema)] -pub struct Disk { - /// Unique identifier for this disk. - pub id: Uuid, - - /// Storage agents which implement networked block device servers. - pub storage_agents: StorageAgentDescription, - - /// Size of the disk (blocks). - pub block_count: u64, - - /// Block size (bytes). - pub block_size: u32, - - /// Storage interface. - pub interface: DiskType, -} - -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct DiskRequest { - pub name: String, - pub slot: Slot, - pub read_only: bool, - pub device: String, - - // Crucible related opts - pub volume_construction_request: - crucible_client_types::VolumeConstructionRequest, -} - -/// A stable index which is translated by Propolis -/// into a PCI BDF, visible to the guest. -#[derive(Copy, Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct Slot(pub u8); - -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct NetworkInterfaceRequest { - pub interface_id: Uuid, - pub name: String, - pub slot: Slot, -} - #[derive(Deserialize, JsonSchema)] pub struct SnapshotRequestPathParams { pub id: Uuid, diff --git a/lib/propolis-client/Cargo.toml b/lib/propolis-client/Cargo.toml index 355efa94a..7bf325034 100644 --- a/lib/propolis-client/Cargo.toml +++ b/lib/propolis-client/Cargo.toml @@ -9,6 +9,7 @@ doctest = false [dependencies] async-trait.workspace = true base64.workspace = true +crucible-client-types.workspace = true futures.workspace = true progenitor.workspace = true propolis_api_types.workspace = true diff --git a/lib/propolis-client/src/lib.rs b/lib/propolis-client/src/lib.rs index 03305aeaf..2ec411af6 100644 --- a/lib/propolis-client/src/lib.rs +++ b/lib/propolis-client/src/lib.rs @@ -8,6 +8,12 @@ // its constructor and From impls. pub use propolis_api_types::instance_spec::PciPath; +// Re-export Crucible client types that appear in their serialized forms in +// instance specs. This allows clients to ensure they serialize/deserialize +// these types using the same versions as the Propolis client associated with +// the server they want to talk to. +pub use crucible_client_types::{CrucibleOpts, VolumeConstructionRequest}; + progenitor::generate_api!( spec = "../../openapi/propolis-server.json", interface = Builder, @@ -16,20 +22,9 @@ progenitor::generate_api!( PciPath = crate::PciPath, }, patch = { - // Some Crucible-related bits are re-exported through simulated - // sled-agent and thus require JsonSchema BootOrderEntry = { derives = [schemars::JsonSchema] }, BootSettings = { derives = [Default, schemars::JsonSchema] }, CpuidEntry = { derives = [PartialEq, Eq, Copy] }, - DiskRequest = { derives = [schemars::JsonSchema] }, - VolumeConstructionRequest = { derives = [schemars::JsonSchema] }, - CrucibleOpts = { derives = [schemars::JsonSchema] }, - Slot = { derives = [schemars::JsonSchema] }, - - PciPath = { derives = [ - Copy, Ord, Eq, PartialEq, PartialOrd - ] }, - InstanceMetadata = { derives = [ PartialEq ] }, } ); diff --git a/openapi/propolis-server.json b/openapi/propolis-server.json index b02e9036c..ff27cd36b 100644 --- a/openapi/propolis-server.json +++ b/openapi/propolis-server.json @@ -362,37 +362,6 @@ "$ref": "#/components/responses/Error" } } - }, - "put": { - "operationId": "instance_spec_ensure", - "requestBody": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/InstanceSpecEnsureRequest" - } - } - }, - "required": true - }, - "responses": { - "201": { - "description": "successful creation", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/InstanceEnsureResponse" - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - } } }, "/instance/state": { @@ -984,58 +953,6 @@ "Intel" ] }, - "CrucibleOpts": { - "type": "object", - "properties": { - "cert_pem": { - "nullable": true, - "type": "string" - }, - "control": { - "nullable": true, - "type": "string" - }, - "flush_timeout": { - "nullable": true, - "type": "number", - "format": "float" - }, - "id": { - "type": "string", - "format": "uuid" - }, - "key": { - "nullable": true, - "type": "string" - }, - "key_pem": { - "nullable": true, - "type": "string" - }, - "lossy": { - "type": "boolean" - }, - "read_only": { - "type": "boolean" - }, - "root_cert_pem": { - "nullable": true, - "type": "string" - }, - "target": { - "type": "array", - "items": { - "type": "string" - } - } - }, - "required": [ - "id", - "lossy", - "read_only", - "target" - ] - }, "CrucibleStorageBackend": { "description": "A Crucible storage backend.", "type": "object", @@ -1055,33 +972,6 @@ ], "additionalProperties": false }, - "DiskRequest": { - "type": "object", - "properties": { - "device": { - "type": "string" - }, - "name": { - "type": "string" - }, - "read_only": { - "type": "boolean" - }, - "slot": { - "$ref": "#/components/schemas/Slot" - }, - "volume_construction_request": { - "$ref": "#/components/schemas/VolumeConstructionRequest" - } - }, - "required": [ - "device", - "name", - "read_only", - "slot", - "volume_construction_request" - ] - }, "DlpiNetworkBackend": { "description": "A network backend associated with a DLPI VNIC on the host.", "type": "object", @@ -1166,61 +1056,16 @@ "InstanceEnsureRequest": { "type": "object", "properties": { - "boot_settings": { - "nullable": true, - "default": null, - "allOf": [ - { - "$ref": "#/components/schemas/BootSettings" - } - ] - }, - "cloud_init_bytes": { - "nullable": true, - "type": "string" - }, - "disks": { - "default": [], - "type": "array", - "items": { - "$ref": "#/components/schemas/DiskRequest" - } - }, - "memory": { - "description": "Size of memory allocated to the Instance, in MiB.", - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "migrate": { - "nullable": true, - "allOf": [ - { - "$ref": "#/components/schemas/InstanceMigrateInitiateRequest" - } - ] - }, - "nics": { - "default": [], - "type": "array", - "items": { - "$ref": "#/components/schemas/NetworkInterfaceRequest" - } + "init": { + "$ref": "#/components/schemas/InstanceInitializationMethod" }, "properties": { "$ref": "#/components/schemas/InstanceProperties" - }, - "vcpus": { - "description": "Number of vCPUs to be allocated to the Instance.", - "type": "integer", - "format": "uint8", - "minimum": 0 } }, "required": [ - "memory", - "properties", - "vcpus" + "init", + "properties" ] }, "InstanceEnsureResponse": { @@ -1247,6 +1092,74 @@ "instance" ] }, + "InstanceInitializationMethod": { + "oneOf": [ + { + "type": "object", + "properties": { + "method": { + "type": "string", + "enum": [ + "Spec" + ] + }, + "value": { + "type": "object", + "properties": { + "spec": { + "$ref": "#/components/schemas/InstanceSpecV0" + } + }, + "required": [ + "spec" + ] + } + }, + "required": [ + "method", + "value" + ] + }, + { + "type": "object", + "properties": { + "method": { + "type": "string", + "enum": [ + "MigrationTarget" + ] + }, + "value": { + "type": "object", + "properties": { + "migration_id": { + "type": "string", + "format": "uuid" + }, + "replace_components": { + "type": "object", + "additionalProperties": { + "$ref": "#/components/schemas/ReplacementComponent" + } + }, + "src_addr": { + "type": "string" + } + }, + "required": [ + "migration_id", + "replace_components", + "src_addr" + ] + } + }, + "required": [ + "method", + "value" + ] + } + ] + }, "InstanceMetadata": { "type": "object", "properties": { @@ -1283,27 +1196,6 @@ "sled_serial" ] }, - "InstanceMigrateInitiateRequest": { - "type": "object", - "properties": { - "migration_id": { - "type": "string", - "format": "uuid" - }, - "src_addr": { - "type": "string" - }, - "src_uuid": { - "type": "string", - "format": "uuid" - } - }, - "required": [ - "migration_id", - "src_addr", - "src_uuid" - ] - }, "InstanceMigrateInitiateResponse": { "type": "object", "properties": { @@ -1420,29 +1312,6 @@ "last_byte_offset" ] }, - "InstanceSpecEnsureRequest": { - "type": "object", - "properties": { - "instance_spec": { - "$ref": "#/components/schemas/VersionedInstanceSpec" - }, - "migrate": { - "nullable": true, - "allOf": [ - { - "$ref": "#/components/schemas/InstanceMigrateInitiateRequest" - } - ] - }, - "properties": { - "$ref": "#/components/schemas/InstanceProperties" - } - }, - "required": [ - "instance_spec", - "properties" - ] - }, "InstanceSpecGetResponse": { "type": "object", "properties": { @@ -1628,26 +1497,6 @@ "Error" ] }, - "NetworkInterfaceRequest": { - "type": "object", - "properties": { - "interface_id": { - "type": "string", - "format": "uuid" - }, - "name": { - "type": "string" - }, - "slot": { - "$ref": "#/components/schemas/Slot" - } - }, - "required": [ - "interface_id", - "name", - "slot" - ] - }, "NvmeDisk": { "description": "A disk that presents an NVMe interface to the guest.", "type": "object", @@ -1780,6 +1629,68 @@ "vcr_matches" ] }, + "ReplacementComponent": { + "description": "Describes an instance spec component that should be replaced during a live migration.", + "oneOf": [ + { + "type": "object", + "properties": { + "component": { + "type": "string", + "enum": [ + "MigrationFailureInjector" + ] + }, + "spec": { + "$ref": "#/components/schemas/MigrationFailureInjector" + } + }, + "required": [ + "component", + "spec" + ], + "additionalProperties": false + }, + { + "type": "object", + "properties": { + "component": { + "type": "string", + "enum": [ + "CrucibleStorageBackend" + ] + }, + "spec": { + "$ref": "#/components/schemas/CrucibleStorageBackend" + } + }, + "required": [ + "component", + "spec" + ], + "additionalProperties": false + }, + { + "type": "object", + "properties": { + "component": { + "type": "string", + "enum": [ + "VirtioNetworkBackend" + ] + }, + "spec": { + "$ref": "#/components/schemas/VirtioNetworkBackend" + } + }, + "required": [ + "component", + "spec" + ], + "additionalProperties": false + } + ] + }, "SerialPort": { "description": "A serial port device.", "type": "object", @@ -1808,12 +1719,6 @@ "com4" ] }, - "Slot": { - "description": "A stable index which is translated by Propolis into a PCI BDF, visible to the guest.", - "type": "integer", - "format": "uint8", - "minimum": 0 - }, "SoftNpuP9": { "description": "Describes a PCI device that shares host files with the guest using the P9 protocol.\n\nThis is only supported by Propolis servers compiled with the `falcon` feature.", "type": "object", @@ -1959,150 +1864,6 @@ ], "additionalProperties": false }, - "VolumeConstructionRequest": { - "oneOf": [ - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "read_only_parent": { - "nullable": true, - "allOf": [ - { - "$ref": "#/components/schemas/VolumeConstructionRequest" - } - ] - }, - "sub_volumes": { - "type": "array", - "items": { - "$ref": "#/components/schemas/VolumeConstructionRequest" - } - }, - "type": { - "type": "string", - "enum": [ - "volume" - ] - } - }, - "required": [ - "block_size", - "id", - "sub_volumes", - "type" - ] - }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "type": { - "type": "string", - "enum": [ - "url" - ] - }, - "url": { - "type": "string" - } - }, - "required": [ - "block_size", - "id", - "type", - "url" - ] - }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "blocks_per_extent": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "extent_count": { - "type": "integer", - "format": "uint32", - "minimum": 0 - }, - "gen": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "opts": { - "$ref": "#/components/schemas/CrucibleOpts" - }, - "type": { - "type": "string", - "enum": [ - "region" - ] - } - }, - "required": [ - "block_size", - "blocks_per_extent", - "extent_count", - "gen", - "opts", - "type" - ] - }, - { - "type": "object", - "properties": { - "block_size": { - "type": "integer", - "format": "uint64", - "minimum": 0 - }, - "id": { - "type": "string", - "format": "uuid" - }, - "path": { - "type": "string" - }, - "type": { - "type": "string", - "enum": [ - "file" - ] - } - }, - "required": [ - "block_size", - "id", - "path", - "type" - ] - } - ] - }, "VolumeStatus": { "type": "object", "properties": { diff --git a/phd-tests/framework/src/disk/crucible.rs b/phd-tests/framework/src/disk/crucible.rs index 244f87309..36fec53c7 100644 --- a/phd-tests/framework/src/disk/crucible.rs +++ b/phd-tests/framework/src/disk/crucible.rs @@ -12,9 +12,9 @@ use std::{ }; use anyhow::Context; -use propolis_client::types::{ - ComponentV0, CrucibleOpts, CrucibleStorageBackend, - VolumeConstructionRequest, +use propolis_client::{ + types::{ComponentV0, CrucibleStorageBackend}, + CrucibleOpts, VolumeConstructionRequest, }; use rand::{rngs::StdRng, RngCore, SeedableRng}; use tracing::{error, info}; @@ -288,11 +288,8 @@ impl super::DiskConfig for CrucibleDisk { fn backend_spec(&self) -> ComponentV0 { let gen = self.generation.load(Ordering::Relaxed); - let downstairs_addrs = self - .downstairs_instances - .iter() - .map(|ds| ds.address.to_string()) - .collect(); + let downstairs_addrs = + self.downstairs_instances.iter().map(|ds| ds.address).collect(); let vcr = VolumeConstructionRequest::Volume { id: self.id, diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index c8f419dcc..872856592 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -5,7 +5,10 @@ //! Routines for starting VMs, changing their states, and interacting with their //! guest OSes. -use std::{fmt::Debug, sync::Arc, time::Duration}; +use std::{ + collections::HashMap, fmt::Debug, net::SocketAddr, sync::Arc, + time::Duration, +}; use crate::{ guest_os::{ @@ -24,11 +27,11 @@ use core::result::Result as StdResult; use propolis_client::{ support::{InstanceSerialConsoleHelper, WSClientOffset}, types::{ - InstanceEnsureRequest, InstanceGetResponse, - InstanceMigrateInitiateRequest, InstanceMigrateStatusResponse, + ComponentV0, InstanceEnsureRequest, InstanceGetResponse, + InstanceInitializationMethod, InstanceMigrateStatusResponse, InstanceProperties, InstanceSerialConsoleHistoryResponse, - InstanceSpecEnsureRequest, InstanceSpecGetResponse, InstanceState, - InstanceStateRequested, MigrationState, VersionedInstanceSpec, + InstanceSpecGetResponse, InstanceState, InstanceStateRequested, + MigrationState, ReplacementComponent, }, }; use propolis_client::{Client, ResponseValue}; @@ -66,6 +69,15 @@ pub enum VmStateError { InstanceAlreadyEnsured, } +type ReplacementComponents = HashMap; + +#[derive(Clone, Debug)] +struct MigrationInfo { + migration_id: Uuid, + src_addr: SocketAddr, + replace_components: ReplacementComponents, +} + /// Specifies the timeout to apply to an attempt to migrate. pub enum MigrationTimeout { /// Time out after the specified duration. @@ -128,15 +140,6 @@ enum InstanceConsoleSource<'a> { InheritFrom(&'a TestVm), } -/// Specifies the propolis-server interface to use when starting a VM. -enum InstanceEnsureApi { - /// Use the `instance_spec_ensure` interface. - SpecEnsure, - - /// Use the `instance_ensure` interface. - Ensure, -} - enum VmState { New, Ensured { serial: SerialConsole }, @@ -271,19 +274,12 @@ impl TestVm { #[instrument(skip_all, fields(vm = self.spec.vm_name, vm_id = %self.id))] async fn instance_ensure_internal<'a>( &self, - api: InstanceEnsureApi, - migrate: Option, + migrate: Option, console_source: InstanceConsoleSource<'a>, ) -> Result { - let (vcpus, memory_mib) = match self.state { - VmState::New => ( - self.spec.instance_spec.board.cpus, - self.spec.instance_spec.board.memory_mb, - ), - VmState::Ensured { .. } => { - return Err(VmStateError::InstanceAlreadyEnsured.into()) - } - }; + if let VmState::Ensured { .. } = self.state { + return Err(VmStateError::InstanceAlreadyEnsured.into()); + } let properties = InstanceProperties { id: self.id, @@ -292,14 +288,18 @@ impl TestVm { description: "Pheidippides-managed VM".to_string(), }; - // The non-spec ensure interface requires a set of `DiskRequest` - // structures to specify disks. Create those once and clone them if the - // ensure call needs to be retried. - let disk_reqs = if let InstanceEnsureApi::Ensure = api { - Some(self.spec.make_disk_requests()?) - } else { - None + let init = match migrate { + None => InstanceInitializationMethod::Spec { + spec: self.spec.instance_spec.clone(), + }, + Some(info) => InstanceInitializationMethod::MigrationTarget { + migration_id: info.migration_id, + replace_components: info.replace_components, + src_addr: info.src_addr.to_string(), + }, }; + let ensure_req = + InstanceEnsureRequest { properties: properties.clone(), init }; // There is a brief period where the Propolis server process has begun // to run but hasn't started its Dropshot server yet. Ensure requests @@ -310,39 +310,8 @@ impl TestVm { // it's possible to create a boxed future that abstracts over the // caller's chosen endpoint. let ensure_fn = || async { - let result = match api { - InstanceEnsureApi::SpecEnsure => { - let versioned_spec = VersionedInstanceSpec::V0( - self.spec.instance_spec.clone(), - ); - - let ensure_req = InstanceSpecEnsureRequest { - properties: properties.clone(), - instance_spec: versioned_spec, - migrate: migrate.clone(), - }; - - self.client - .instance_spec_ensure() - .body(&ensure_req) - .send() - .await - } - InstanceEnsureApi::Ensure => { - let ensure_req = InstanceEnsureRequest { - cloud_init_bytes: None, - vcpus, - memory: memory_mib, - disks: disk_reqs.clone().unwrap(), - migrate: migrate.clone(), - nics: vec![], - boot_settings: None, - properties: properties.clone(), - }; - - self.client.instance_ensure().body(&ensure_req).send().await - } - }; + let result = + self.client.instance_ensure().body(&ensure_req).send().await; if let Err(e) = result { match e { propolis_client::Error::CommunicationError(_) => { @@ -433,29 +402,7 @@ impl TestVm { match self.state { VmState::New => { let console = self - .instance_ensure_internal( - InstanceEnsureApi::SpecEnsure, - None, - InstanceConsoleSource::New, - ) - .await?; - self.state = VmState::Ensured { serial: console }; - } - VmState::Ensured { .. } => {} - } - - Ok(()) - } - - pub async fn instance_ensure_using_api_request(&mut self) -> Result<()> { - match self.state { - VmState::New => { - let console = self - .instance_ensure_internal( - InstanceEnsureApi::Ensure, - None, - InstanceConsoleSource::New, - ) + .instance_ensure_internal(None, InstanceConsoleSource::New) .await?; self.state = VmState::Ensured { serial: console }; } @@ -558,11 +505,11 @@ impl TestVm { let serial = self .instance_ensure_internal( - InstanceEnsureApi::SpecEnsure, - Some(InstanceMigrateInitiateRequest { + Some(MigrationInfo { migration_id, - src_addr: server_addr.to_string(), - src_uuid: Uuid::default(), + src_addr: SocketAddr::V4(server_addr), + replace_components: self + .generate_replacement_components(), }), InstanceConsoleSource::InheritFrom(source), ) @@ -616,6 +563,33 @@ impl TestVm { } } + fn generate_replacement_components(&self) -> ReplacementComponents { + let mut map = ReplacementComponents::new(); + for (id, comp) in &self.spec.instance_spec.components { + match comp { + ComponentV0::MigrationFailureInjector(inj) => { + map.insert( + id.clone(), + ReplacementComponent::MigrationFailureInjector( + inj.clone(), + ), + ); + } + ComponentV0::CrucibleStorageBackend(be) => { + map.insert( + id.clone(), + ReplacementComponent::CrucibleStorageBackend( + be.clone(), + ), + ); + } + _ => {} + } + } + + map + } + pub async fn get_migration_state( &self, ) -> Result { diff --git a/phd-tests/framework/src/test_vm/spec.rs b/phd-tests/framework/src/test_vm/spec.rs index dc276b2df..573537c08 100644 --- a/phd-tests/framework/src/test_vm/spec.rs +++ b/phd-tests/framework/src/test_vm/spec.rs @@ -9,10 +9,7 @@ use crate::{ guest_os::GuestOsKind, }; use camino::Utf8PathBuf; -use propolis_client::{ - types::{ComponentV0, DiskRequest, InstanceMetadata, InstanceSpecV0, Slot}, - PciPath, -}; +use propolis_client::types::{ComponentV0, InstanceMetadata, InstanceSpecV0}; use uuid::Uuid; /// The set of objects needed to start and run a guest in a `TestVm`. @@ -76,80 +73,4 @@ impl VmSpec { self.metadata.sled_id = id; self.metadata.sled_serial = id.to_string(); } - - /// Generates a set of [`propolis_client::types::DiskRequest`] structures - /// corresponding to the disks in this VM spec. - /// - /// All of the disks in the spec must be Crucible disks. If one is not, this - /// routine returns an error. - pub(crate) fn make_disk_requests( - &self, - ) -> anyhow::Result> { - struct DeviceInfo<'a> { - backend_name: &'a str, - interface: &'static str, - slot: Slot, - } - - fn convert_to_slot(pci_path: PciPath) -> anyhow::Result { - match pci_path.device() { - dev @ 0x10..=0x17 => Ok(Slot(dev - 0x10)), - _ => Err(anyhow::anyhow!( - "PCI device number {} out of range", - pci_path.device() - )), - } - } - - fn get_device_info(device: &ComponentV0) -> anyhow::Result { - match device { - ComponentV0::VirtioDisk(d) => Ok(DeviceInfo { - backend_name: &d.backend_name, - interface: "virtio", - slot: convert_to_slot(d.pci_path)?, - }), - ComponentV0::NvmeDisk(d) => Ok(DeviceInfo { - backend_name: &d.backend_name, - interface: "nvme", - slot: convert_to_slot(d.pci_path)?, - }), - _ => { - panic!("asked to get device info for a non-storage device") - } - } - } - - let mut reqs = vec![]; - for (name, device) in - self.instance_spec.components.iter().filter(|(_, c)| { - matches!( - c, - ComponentV0::VirtioDisk(_) | ComponentV0::NvmeDisk(_) - ) - }) - { - let info = get_device_info(device)?; - let backend = self - .instance_spec - .components - .get(info.backend_name) - .expect("storage device should have a matching backend"); - - let ComponentV0::CrucibleStorageBackend(backend) = backend else { - anyhow::bail!("disk {name} does not have a Crucible backend"); - }; - - reqs.push(DiskRequest { - device: info.interface.to_owned(), - name: name.clone(), - read_only: backend.readonly, - slot: info.slot, - volume_construction_request: serde_json::from_str( - &backend.request_json, - )?, - }) - } - - Ok(reqs) - } } diff --git a/phd-tests/tests/src/ensure_api.rs b/phd-tests/tests/src/ensure_api.rs deleted file mode 100644 index 8407a1360..000000000 --- a/phd-tests/tests/src/ensure_api.rs +++ /dev/null @@ -1,33 +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/. - -//! Tests that explicitly exercise the `instance_ensure` form of the VM start -//! API. - -use phd_framework::{ - disk::BlockSize, - test_vm::{DiskBackend, DiskInterface}, -}; -use phd_testcase::*; - -#[phd_testcase] -async fn instance_ensure_api_test(ctx: &Framework) { - if !ctx.crucible_enabled() { - phd_skip!("test requires Crucible to be enabled"); - } - - let mut config = ctx.vm_config_builder("instance_ensure_api_test"); - config.boot_disk( - ctx.default_guest_os_artifact(), - DiskInterface::Nvme, - DiskBackend::Crucible { - min_disk_size_gib: 10, - block_size: BlockSize::Bytes512, - }, - 0x10, - ); - - let mut vm = ctx.spawn_vm(&config, None).await?; - vm.instance_ensure_using_api_request().await?; -} diff --git a/phd-tests/tests/src/lib.rs b/phd-tests/tests/src/lib.rs index e2f872f11..b76f0bec1 100644 --- a/phd-tests/tests/src/lib.rs +++ b/phd-tests/tests/src/lib.rs @@ -8,7 +8,6 @@ mod boot_order; mod cpuid; mod crucible; mod disk; -mod ensure_api; mod framework; mod hw; mod migrate; From 5354f856738e4eccf0b4d1bf1d5057d640a329e8 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Mon, 2 Dec 2024 21:33:33 +0000 Subject: [PATCH 2/3] explain replacement component enum --- crates/propolis-api-types/src/lib.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/crates/propolis-api-types/src/lib.rs b/crates/propolis-api-types/src/lib.rs index 1944b3b13..c24fdd3e5 100644 --- a/crates/propolis-api-types/src/lib.rs +++ b/crates/propolis-api-types/src/lib.rs @@ -51,8 +51,31 @@ pub struct InstanceMetadata { pub sled_model: String, } -/// Describes an instance spec component that should be replaced during a live -/// migration. +/// An instance spec component that should be replaced during a live migration. +/// +/// When a caller asks Propolis to initialize via live migration, the target VM +/// inherits the migration source's current instance spec. For the most part, +/// the target can (and indeed in some cases must) use this spec without +/// modifying it; this helps Propolis ensure that guest-visible configuration +/// remains unchanged when a VM migrates. However, there are some components +/// with no guest-visible state that may need to be reconfigured when a VM +/// migrates. These include the following: +/// +/// - Crucible disks: After migrating, the target Propolis presents itself as a +/// new client of the Crucible downstairs servers backing the VM's disks. +/// Crucible requires the target to present a newer client generation number +/// to allow the target to connect. In a full Oxide deployment, these numbers +/// are managed by the control plane (i.e. it is not safe for Propolis to +/// manage these values directly--new Crucible volume connection information +/// must always come from Nexus). +/// - Virtio network devices: Each virtio NIC in the guest needs to bind to a +/// named VNIC object on the host. These names can change when a VM migrates +/// from host to host. +/// +/// Each component that can be reconfigured this way has a variant in this enum; +/// components not in the enum can't be reconfigured during migration. This +/// saves the initialization API from having to reason about requests to replace +/// a component that can't legally be replaced. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(deny_unknown_fields, tag = "component", content = "spec")] pub enum ReplacementComponent { From 7a9bbc0671bcacbe14253506b0910653e3cc2faa Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 3 Dec 2024 18:41:05 +0000 Subject: [PATCH 3/3] openapi update --- crates/propolis-api-types/src/lib.rs | 48 ++++++++++++++-------------- openapi/propolis-server.json | 2 +- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/crates/propolis-api-types/src/lib.rs b/crates/propolis-api-types/src/lib.rs index c24fdd3e5..f36eb0b14 100644 --- a/crates/propolis-api-types/src/lib.rs +++ b/crates/propolis-api-types/src/lib.rs @@ -52,30 +52,30 @@ pub struct InstanceMetadata { } /// An instance spec component that should be replaced during a live migration. -/// -/// When a caller asks Propolis to initialize via live migration, the target VM -/// inherits the migration source's current instance spec. For the most part, -/// the target can (and indeed in some cases must) use this spec without -/// modifying it; this helps Propolis ensure that guest-visible configuration -/// remains unchanged when a VM migrates. However, there are some components -/// with no guest-visible state that may need to be reconfigured when a VM -/// migrates. These include the following: -/// -/// - Crucible disks: After migrating, the target Propolis presents itself as a -/// new client of the Crucible downstairs servers backing the VM's disks. -/// Crucible requires the target to present a newer client generation number -/// to allow the target to connect. In a full Oxide deployment, these numbers -/// are managed by the control plane (i.e. it is not safe for Propolis to -/// manage these values directly--new Crucible volume connection information -/// must always come from Nexus). -/// - Virtio network devices: Each virtio NIC in the guest needs to bind to a -/// named VNIC object on the host. These names can change when a VM migrates -/// from host to host. -/// -/// Each component that can be reconfigured this way has a variant in this enum; -/// components not in the enum can't be reconfigured during migration. This -/// saves the initialization API from having to reason about requests to replace -/// a component that can't legally be replaced. +// +// When a caller asks Propolis to initialize via live migration, the target VM +// inherits the migration source's current instance spec. For the most part, +// the target can (and indeed in some cases must) use this spec without +// modifying it; this helps Propolis ensure that guest-visible configuration +// remains unchanged when a VM migrates. However, there are some components +// with no guest-visible state that may need to be reconfigured when a VM +// migrates. These include the following: +// +// - Crucible disks: After migrating, the target Propolis presents itself as a +// new client of the Crucible downstairs servers backing the VM's disks. +// Crucible requires the target to present a newer client generation number +// to allow the target to connect. In a full Oxide deployment, these numbers +// are managed by the control plane (i.e. it is not safe for Propolis to +// manage these values directly--new Crucible volume connection information +// must always come from Nexus). +// - Virtio network devices: Each virtio NIC in the guest needs to bind to a +// named VNIC object on the host. These names can change when a VM migrates +// from host to host. +// +// Each component that can be reconfigured this way has a variant in this enum; +// components not in the enum can't be reconfigured during migration. This +// saves the initialization API from having to reason about requests to replace +// a component that can't legally be replaced. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(deny_unknown_fields, tag = "component", content = "spec")] pub enum ReplacementComponent { diff --git a/openapi/propolis-server.json b/openapi/propolis-server.json index ff27cd36b..876d0a10e 100644 --- a/openapi/propolis-server.json +++ b/openapi/propolis-server.json @@ -1630,7 +1630,7 @@ ] }, "ReplacementComponent": { - "description": "Describes an instance spec component that should be replaced during a live migration.", + "description": "An instance spec component that should be replaced during a live migration.", "oneOf": [ { "type": "object",