diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index d4b22f9a8..10a649986 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -263,6 +263,11 @@ impl DiskRequest { backend_id: backend_id.clone(), pci_path, serial_number: nvme_serial_from_str(&self.name, b' '), + // TODO: `DiskRequest` implies this is the disk-side interface + // of a Crucible storage backend, so we report a write cache. + // but this probably should be configurable more directly in the + // limit + has_write_cache: true, }), _ => anyhow::bail!( "invalid device type in disk request: {:?}", diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index cada3a60e..0da580375 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -909,6 +909,7 @@ impl MachineInitializer<'_> { let nvme = nvme::PciNvme::create( &nvme_spec.serial_number, mdts, + nvme_spec.has_write_cache, self.log.new(slog::o!("component" => component)), ); self.devices.insert(device_id.clone(), nvme.clone()); diff --git a/bin/propolis-server/src/lib/migrate/mod.rs b/bin/propolis-server/src/lib/migrate/mod.rs index 2145dd3c3..8979f99e6 100644 --- a/bin/propolis-server/src/lib/migrate/mod.rs +++ b/bin/propolis-server/src/lib/migrate/mod.rs @@ -2,9 +2,14 @@ // 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/. +//! This module and its children define the migration protocol for +//! `propolis-server`. Generally the structures and state machine here are +//! consistent with the discussion in RFD 71. + use bit_field::BitField; use dropshot::HttpError; use propolis::migrate::MigrateStateError; +use propolis_api_types::instance_spec::SpecKey; use propolis_api_types::migration::MigrationState; use serde::{Deserialize, Serialize}; use slog::error; @@ -17,6 +22,7 @@ mod memx; mod preamble; pub mod protocol; pub mod source; +mod types; /// Trait bounds for connection objects used in live migrations. pub(crate) trait MigrateConn: @@ -153,6 +159,13 @@ pub enum MigrateError { RemoteError(MigrateRole, String), } +impl MigrateError { + pub(crate) fn wrong_type(id: &SpecKey, kind: &str) -> MigrateError { + let msg = format!("component {id} is not a {kind} in the source spec"); + MigrateError::InstanceSpecsIncompatible(msg) + } +} + impl From for MigrateError { fn from(err: tokio_tungstenite::tungstenite::Error) -> MigrateError { MigrateError::Websocket(err.to_string()) diff --git a/bin/propolis-server/src/lib/migrate/preamble.rs b/bin/propolis-server/src/lib/migrate/preamble.rs index 3c205f8ef..03b4ecde8 100644 --- a/bin/propolis-server/src/lib/migrate/preamble.rs +++ b/bin/propolis-server/src/lib/migrate/preamble.rs @@ -8,19 +8,20 @@ use propolis_api_types::instance::ReplacementComponent; use propolis_api_types_versions::v1; use serde::{Deserialize, Serialize}; -use crate::spec::{api_spec_v0::ApiSpecError, Spec}; +use crate::migrate; +use crate::spec::Spec; use super::MigrateError; #[derive(Deserialize, Serialize, Debug)] pub(crate) struct Preamble { - pub instance_spec: v1::instance_spec::VersionedInstanceSpec, + pub instance_spec: migrate::types::VersionedInstanceSpec, pub blobs: Vec>, } impl Preamble { pub fn new( - instance_spec: v1::instance_spec::VersionedInstanceSpec, + instance_spec: migrate::types::VersionedInstanceSpec, ) -> Preamble { Preamble { instance_spec, blobs: Vec::new() } } @@ -40,75 +41,8 @@ impl Preamble { ReplacementComponent, >, ) -> Result { - fn wrong_type_error( - id: &v1::instance_spec::SpecKey, - kind: &str, - ) -> MigrateError { - let msg = - format!("component {id} is not a {kind} in the source spec"); - MigrateError::InstanceSpecsIncompatible(msg) - } - - let v1::instance_spec::VersionedInstanceSpec::V0(mut source_spec) = - self.instance_spec; - for (id, comp) in replacements { - let Some(to_amend) = source_spec.components.get_mut(id) else { - return Err(MigrateError::InstanceSpecsIncompatible(format!( - "replacement component {id} not in source spec", - ))); - }; - - match comp { - #[cfg(not(feature = "failure-injection"))] - ReplacementComponent::MigrationFailureInjector(_) => { - return Err(MigrateError::InstanceSpecsIncompatible( - format!( - "replacing migration failure injector {id} is \ - impossible because the feature is compiled out" - ), - )); - } - - #[cfg(feature = "failure-injection")] - ReplacementComponent::MigrationFailureInjector(comp) => { - let v1::instance_spec::Component::MigrationFailureInjector( - src, - ) = to_amend - else { - return Err(wrong_type_error( - id, - "migration failure injector", - )); - }; - - *src = comp.clone(); - } - ReplacementComponent::CrucibleStorageBackend(comp) => { - let v1::instance_spec::Component::CrucibleStorageBackend( - src, - ) = to_amend - else { - return Err(wrong_type_error(id, "crucible backend")); - }; - - *src = comp.clone(); - } - ReplacementComponent::VirtioNetworkBackend(comp) => { - let v1::instance_spec::Component::VirtioNetworkBackend(src) = - to_amend - else { - return Err(wrong_type_error(id, "viona backend")); - }; - - *src = comp.clone(); - } - } - } - let amended_spec = - source_spec.try_into().map_err(|e: ApiSpecError| { - MigrateError::PreambleParse(e.to_string()) - })?; + self.instance_spec.into_amended_spec(replacements)?; // TODO: Compare opaque blobs. diff --git a/bin/propolis-server/src/lib/migrate/source.rs b/bin/propolis-server/src/lib/migrate/source.rs index df9545753..0d34ca192 100644 --- a/bin/propolis-server/src/lib/migrate/source.rs +++ b/bin/propolis-server/src/lib/migrate/source.rs @@ -9,7 +9,6 @@ use propolis::migrate::{ MigrateCtx, MigrateStateError, Migrator, PayloadOutputs, }; use propolis::vmm; -use propolis_api_types_versions::v1; use slog::{debug, error, info, trace, warn}; use std::collections::HashMap; use std::convert::TryInto; @@ -25,6 +24,7 @@ use crate::migrate::memx; use crate::migrate::preamble::Preamble; use crate::migrate::probes; use crate::migrate::protocol::Protocol; +use crate::migrate::types::VersionedInstanceSpec; use crate::migrate::{codec, protocol}; use crate::migrate::{ Device, DevicePayload, MigrateError, MigratePhase, MigrateRole, @@ -467,10 +467,10 @@ impl RonV0Runner<'_, T> { async fn sync(&mut self) -> Result<(), MigrateError> { self.update_state(MigrationState::Sync); - let preamble = - Preamble::new(v1::instance_spec::VersionedInstanceSpec::V0( - self.vm.lock_shared().await.instance_spec().clone().into(), - )); + let versioned = VersionedInstanceSpec::from_spec( + self.vm.lock_shared().await.instance_spec(), + )?; + let preamble = Preamble::new(versioned); let s = ron::ser::to_string(&preamble) .map_err(codec::ProtocolError::from)?; self.send_msg(codec::Message::Serialized(s)).await?; diff --git a/bin/propolis-server/src/lib/migrate/types.rs b/bin/propolis-server/src/lib/migrate/types.rs new file mode 100644 index 000000000..d88e24b15 --- /dev/null +++ b/bin/propolis-server/src/lib/migrate/types.rs @@ -0,0 +1,193 @@ +// 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/. + +//! ## Types to describe a VM which is being migrated. +//! +//! The API types that describe a VM are converted by `propolis-server` into a +//! [`struct Spec`][crate::lib::spec::Spec] as a "how this version happens to +//! describe VMs" internal structure. Early in migration we must convert this to +//! some format that a `propolis-server` of a different version can instantiate +//! an equivalent VM from, which CPUs, memory, and device state will be imported +//! into. We *kind of* use API types here, and the rest of this section gets +//! into why and what one should consider in adding future versions. +//! +//! Even for VMs that have been migrated many times, `propolis-server` must +//! incarnate a VM that has been[1] described by *some* HTTP API `InstanceSpec` +//! version at some point in the past. We'll call this "oldest possible VM spec" +//! the _import horizon_ that `propolis-server` supports. Further, the tooling +//! for Dropshot (/OpenAPI) version management is quite good, and provides +//! guardrails against old versions' API types having structural changes. +//! +//! So, the strategy we use for communicating `struct Spec` is to convert it +//! back into *some* `InstanceSpec` that faithfully describes the Spec from +//! whence it came, send that on the wire to a destination `propolis-server`, +//! and expect it'll produce an equivalent VM to import state into. +//! +//! What complicates this is that it may not in general be possible to translate +//! a `struct Spec` into one of the most recent HTTP API `InstanceSpec` types. +//! We may have decided to split a setting into multiple fields, add a device +//! setting, or may have even added a field which cannot be simply "updated +//! into" from an old `InstanceSpec` in isolation (see: SMBIOS Type 1 tables!) +//! This points us away from the naive "`propolis-server` simply produces an +//! InstanceSpec from this version or last version" strategy one may imagine, +//! and towards keeping HTTP API types around well after those HTTP API +//! *endpoints* have been retired! +//! +//! So, we're finally at why the existing mechanics of getting a `Spec` out of +//! `propolis-server` and transmitted are what they are: go through a list of +//! `TryInto for v*::instance_spec::InstanceSpec`, one of them will +//! succeed, and send that over. This is the implementation you'll find in +//! [`RonV0Runner::sync`](crate::lib::migrate::source::RonV0Runner::sync). +//! +//! ### [`VersionedInstanceSpec`] +//! +//! `fn sync` mentions `VersionedInstanceSpec`, which is where things get weirder +//! than simple HTTP API types. +//! +//! `propolis-server` supports more flexibility than the control plane is +//! expected to need in the immediate future. Given the current control plane +//! (update, live migration) plans, the source and destination `propolis-server` +//! may either be the same version, or the destination may be one release newer. +//! +//! Since we have to support HTTP API types as far back as `propolis-server`'s +//! import horizon, it's not much additional work to at least try supporting +//! migration across downgrades of `propolis-server`. If we try converting to +//! all `v1, v2, v3 ..` forms of `InstanceSpec` in *ascending* order, the only +//! time conversion will fail to be downgradeable is if a VM has been created +//! using only-in-newest API language. This means that some VMs created using a +//! `latest::instance_spec::InstanceSpec` could end up with even `v1` types on +//! the wire for migration, but as long as `From/TryFrom` use is correct and +//! *not lossy*, that's fine! +//! +//! So, `VersionedInstanceSpec` is a container that is outside the HTTP API but +//! only contains OpenAPI-described API types. A destination `propolis-server` +//! is expected to gracefully reject new variants, and a source +//! `propolis-server` is expected to emit oldest-supported forms of instances. +//! +//! ### What if we didn't do all that? +//! +//! Another option to negotiate one `propolis-server`'s `struct Spec` into +//! another process would be to have some set of structs and functions to move +//! to and from totally-unrelated-to-HTTP-API wire format with more +//! forward-compatible device types. This would probably work! It would also +//! require testing work to check we don't inadvertently change the current +//! canonical definition of an "old version" which should never change. +//! +//! In either case we need testing that old device descriptions don't +//! *semantically* change, so it doesn't save effort there either. +//! +//! [1]: Technically, being able to describe a VM faithfully using the language +//! of an old API version does not tell you if the VM actually *was* created +//! using that API description. It's possible a VM was provided to Propolis in +//! the form of a `v6::instance_spec::InstanceSpec` which _happens_ to be +//! expressible as a `v1::instance_spec::InstanceSpec`. For the purposes of the +//! discussion above this a boring nitpick; a V1-compatible instance spec that +//! happened to come to us in V6-form can be imagined as anything that it can +//! convert back to (be that V3, V2, V1, ...). + +use serde::{Deserialize, Serialize}; + +use propolis_api_types_versions::v1::instance::ReplacementComponent; +use propolis_api_types_versions::{v1, v2, v3, v6}; + +use std::collections::BTreeMap; + +use crate::migrate::MigrateError; +use crate::spec::{api_spec_v1, api_spec_v2, api_spec_v3, api_spec_v6, Spec}; + +/// A wrapper for one of any supported `InstanceSpec` that describe a +/// to-be-migrated VM. +/// +/// Architecturally, this bridges the very fixed HTTP API types and the +/// possibility of having to migrate an arbitrarily old VM. See the doc comments +/// on [`migrate`][crate::lib::migrate] for more about how this all fits +/// together. +// +// If you're adding (or removing!?) API versions, you'll just want to adjust the +// variants here, plus uses in `VersionedInstanceSpec::from_spec` and +// `VersionedInstanceSpec::into_amended_spec`. shrimple as that. +#[derive(Deserialize, Serialize, Debug)] +pub(crate) enum VersionedInstanceSpec { + V1(v1::instance_spec::InstanceSpec), + V2(v2::instance_spec::InstanceSpec), + V3(v3::instance_spec::InstanceSpec), + V6(v6::instance_spec::InstanceSpec), +} + +impl VersionedInstanceSpec { + pub(crate) fn from_spec( + spec: &Spec, + ) -> Result { + // Try conversions in oldest-to-newest order in support of + // migration-to-older-version. As long as the VM doesn't use a new + // feature or setting, we'll pick a version the older Propolis should + // know about, and everything else will "just work". + // + // When adding a new API version, the previous latest version will + // probably have gone from having an `Into` to instead having + // `TryInto`, which fails for a `Spec` describing whatever new + // features have been added. The new latest version, hopefully, will + // have an `Into`. Those two versions should be the only ones that + // need attention. + let versioned = if let Ok(v1_spec) = + TryInto::::try_into(spec.clone()) + { + VersionedInstanceSpec::V1(v1_spec) + } else if let Ok(v2_spec) = + TryInto::::try_into(spec.clone()) + { + VersionedInstanceSpec::V2(v2_spec) + } else if let Ok(v3_spec) = + TryInto::::try_into(spec.clone()) + { + VersionedInstanceSpec::V3(v3_spec) + } else { + VersionedInstanceSpec::V6( + Into::::into(spec.clone()), + ) + }; + Ok(versioned) + } + + pub(crate) fn into_amended_spec( + self, + replacements: &BTreeMap< + v1::instance_spec::SpecKey, + ReplacementComponent, + >, + ) -> Result { + let amended_spec = match self { + VersionedInstanceSpec::V1(mut source_spec) => { + api_spec_v1::amend(&mut source_spec, replacements)?; + + api_spec_v1::v1_to_spec_builder(source_spec) + .map_err(|e| MigrateError::PreambleParse(e.to_string()))? + .finish() + } + VersionedInstanceSpec::V2(mut source_spec) => { + api_spec_v2::amend(&mut source_spec, replacements)?; + + api_spec_v2::v2_to_spec_builder(source_spec) + .map_err(|e| MigrateError::PreambleParse(e.to_string()))? + .finish() + } + VersionedInstanceSpec::V3(mut source_spec) => { + api_spec_v3::amend(&mut source_spec, replacements)?; + + api_spec_v3::v3_to_spec_builder(source_spec) + .map_err(|e| MigrateError::PreambleParse(e.to_string()))? + .finish() + } + VersionedInstanceSpec::V6(mut source_spec) => { + api_spec_v6::amend(&mut source_spec, replacements)?; + + api_spec_v6::v6_to_spec_builder(source_spec) + .map_err(|e| MigrateError::PreambleParse(e.to_string()))? + .finish() + } + }; + + Ok(amended_spec) + } +} diff --git a/bin/propolis-server/src/lib/spec/api_spec_latest.rs b/bin/propolis-server/src/lib/spec/api_spec_latest.rs new file mode 100644 index 000000000..b3772e5ec --- /dev/null +++ b/bin/propolis-server/src/lib/spec/api_spec_latest.rs @@ -0,0 +1,218 @@ +// 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/. + +//! Conversions from types in [`propolis_api_types`] - that is, the latest +//! Propolis API version - to the internal [`super::Spec`] representation. +//! +//! `propolis_api_types` is a re-export of the latest versions of types out of +//! `propolis_api_types_versions`. The types that tend to change across files +//! are referred to here as `latest::` for similarity to other +//! version-specific code. Types that do not tend to change (say, `SpecKey`) are +//! just taken from the re-exported path because that's how they're used +//! everywhere (including other `api_spec_*` files.) + +use std::collections::BTreeMap; + +use propolis_api_types::instance_spec::{ + components::backends::{DlpiNetworkBackend, VirtioNetworkBackend}, + SpecKey, +}; +use propolis_api_types_versions::latest; + +use super::{ + builder::SpecBuilder, ApiSpecError, Disk, Nic, QemuPvpanic, StorageBackend, + StorageDevice, +}; + +#[cfg(feature = "failure-injection")] +use super::MigrationFailure; + +#[cfg(feature = "falcon")] +use super::SoftNpuPort; + +/// Parses the latest form of `InstanceSpec` into a [`SpecBuilder`], validating +/// component names, PCI paths, and backend references along the way. Callers +/// can add additional components to the builder before calling `finish()`. +pub(crate) fn latest_to_spec_builder( + value: latest::instance_spec::InstanceSpec, +) -> Result { + let mut builder = SpecBuilder::with_instance_spec_board(value.board)?; + let mut devices: Vec<(SpecKey, latest::instance_spec::Component)> = vec![]; + let mut boot_settings = None; + let mut storage_backends: BTreeMap = + BTreeMap::new(); + let mut viona_backends: BTreeMap = + BTreeMap::new(); + let mut dlpi_backends: BTreeMap = + BTreeMap::new(); + + for (id, component) in value.components.into_iter() { + match component { + latest::instance_spec::Component::CrucibleStorageBackend(_) + | latest::instance_spec::Component::FileStorageBackend(_) + | latest::instance_spec::Component::BlobStorageBackend(_) => { + storage_backends.insert( + id, + component + .try_into() + .expect("component is known to be a storage backend"), + ); + } + latest::instance_spec::Component::VirtioNetworkBackend(viona) => { + viona_backends.insert(id, viona); + } + latest::instance_spec::Component::DlpiNetworkBackend(dlpi) => { + dlpi_backends.insert(id, dlpi); + } + device => { + devices.push((id, device)); + } + } + } + + for (device_id, device_spec) in devices { + match device_spec { + latest::instance_spec::Component::VirtioDisk(_) + | latest::instance_spec::Component::NvmeDisk(_) => { + let device_spec = StorageDevice::try_from(device_spec) + .expect("component is known to be a disk"); + + let (_, backend_spec) = storage_backends + .remove_entry(device_spec.backend_id()) + .ok_or_else(|| ApiSpecError::StorageBackendNotFound { + backend: device_spec.backend_id().to_owned(), + device: device_id.clone(), + })?; + + builder.add_storage_device( + device_id, + Disk { device_spec, backend_spec }, + )?; + } + latest::instance_spec::Component::VirtioNic(nic) => { + let (_, backend_spec) = viona_backends + .remove_entry(&nic.backend_id) + .ok_or_else(|| ApiSpecError::NetworkBackendNotFound { + backend: nic.backend_id.clone(), + device: device_id.clone(), + })?; + + builder.add_network_device( + device_id, + Nic { device_spec: nic, backend_spec }, + )?; + } + latest::instance_spec::Component::SerialPort(port) => { + builder.add_serial_port(device_id, port.num)?; + } + latest::instance_spec::Component::PciPciBridge(bridge) => { + builder.add_pci_bridge(device_id, bridge)?; + } + latest::instance_spec::Component::QemuPvpanic(pvpanic) => { + builder.add_pvpanic_device(QemuPvpanic { + id: device_id, + spec: pvpanic, + })?; + } + latest::instance_spec::Component::BootSettings(settings) => { + // The builder returns an error if its caller tries to add + // a boot option that isn't in the set of attached disks. + // Since there may be more disk devices left in the + // component map, just capture the boot order for now and + // apply it to the builder later. + boot_settings = Some((device_id, settings)); + } + latest::instance_spec::Component::VirtioSocket(vsock) => { + let vsock_device = crate::spec::VirtioSocket { + id: device_id.clone(), + spec: vsock, + }; + builder.add_vsock_device(vsock_device)?; + } + #[cfg(not(feature = "failure-injection"))] + latest::instance_spec::Component::MigrationFailureInjector(_) => { + return Err(ApiSpecError::FeatureCompiledOut { + component: device_id, + feature: "failure-injection", + }); + } + #[cfg(feature = "failure-injection")] + latest::instance_spec::Component::MigrationFailureInjector(mig) => { + builder.add_migration_failure_device(MigrationFailure { + id: device_id, + spec: mig, + })?; + } + #[cfg(not(feature = "falcon"))] + latest::instance_spec::Component::SoftNpuPciPort(_) + | latest::instance_spec::Component::SoftNpuPort(_) + | latest::instance_spec::Component::SoftNpuP9(_) + | latest::instance_spec::Component::P9fs(_) => { + return Err(ApiSpecError::FeatureCompiledOut { + component: device_id, + feature: "falcon", + }); + } + #[cfg(feature = "falcon")] + latest::instance_spec::Component::SoftNpuPciPort(port) => { + builder.set_softnpu_pci_port(port)?; + } + #[cfg(feature = "falcon")] + latest::instance_spec::Component::SoftNpuPort(port) => { + let (_, backend_spec) = dlpi_backends + .remove_entry(&port.backend_id) + .ok_or_else(|| ApiSpecError::NetworkBackendNotFound { + backend: port.backend_id.clone(), + device: device_id.clone(), + })?; + + let port = SoftNpuPort { + link_name: port.link_name, + backend_name: port.backend_id, + backend_spec, + }; + + builder.add_softnpu_port(device_id, port)?; + } + #[cfg(feature = "falcon")] + latest::instance_spec::Component::SoftNpuP9(p9) => { + builder.set_softnpu_p9(p9)?; + } + #[cfg(feature = "falcon")] + latest::instance_spec::Component::P9fs(p9fs) => { + builder.set_p9fs(p9fs)?; + } + latest::instance_spec::Component::CrucibleStorageBackend(_) + | latest::instance_spec::Component::FileStorageBackend(_) + | latest::instance_spec::Component::BlobStorageBackend(_) + | latest::instance_spec::Component::VirtioNetworkBackend(_) + | latest::instance_spec::Component::DlpiNetworkBackend(_) => { + unreachable!("already filtered out backends") + } + } + } + + // Now that all disks have been attached, try to establish the boot + // order if one was supplied. + if let Some(settings) = boot_settings { + builder.add_boot_order( + settings.0, + settings.1.order.into_iter().map(Into::into), + )?; + } + + if let Some(backend) = storage_backends.into_keys().next() { + return Err(ApiSpecError::BackendNotUsed(backend)); + } + + if let Some(backend) = viona_backends.into_keys().next() { + return Err(ApiSpecError::BackendNotUsed(backend)); + } + + if let Some(backend) = dlpi_backends.into_keys().next() { + return Err(ApiSpecError::BackendNotUsed(backend)); + } + + Ok(builder) +} diff --git a/bin/propolis-server/src/lib/spec/api_spec_v0.rs b/bin/propolis-server/src/lib/spec/api_spec_v0.rs deleted file mode 100644 index 4aa0d3e21..000000000 --- a/bin/propolis-server/src/lib/spec/api_spec_v0.rs +++ /dev/null @@ -1,428 +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/. - -//! Conversions from version-0 instance specs in the [`propolis_api_types`] -//! crate to the internal [`super::Spec`] representation. - -use std::collections::BTreeMap; - -use propolis_api_types::instance_spec::{ - components::{ - backends::{DlpiNetworkBackend, VirtioNetworkBackend}, - board::Board as InstanceSpecBoard, - devices::{BootSettings, SerialPort as SerialPortDesc}, - }, - SpecKey, -}; -use propolis_api_types_versions::v1; -use thiserror::Error; - -#[cfg(feature = "falcon")] -use propolis_api_types::instance_spec::components::devices::SoftNpuPort as SoftNpuPortSpec; - -use super::{ - builder::{SpecBuilder, SpecBuilderError}, - Disk, Nic, QemuPvpanic, SerialPortDevice, Spec, StorageBackend, - StorageDevice, -}; - -#[cfg(feature = "failure-injection")] -use super::MigrationFailure; - -#[cfg(feature = "falcon")] -use super::SoftNpuPort; - -#[derive(Debug, Error)] -pub(crate) enum ApiSpecError { - #[error(transparent)] - Builder(#[from] SpecBuilderError), - - #[error("storage backend {backend} not found for device {device}")] - StorageBackendNotFound { backend: SpecKey, device: SpecKey }, - - #[error("network backend {backend} not found for device {device}")] - NetworkBackendNotFound { backend: SpecKey, device: SpecKey }, - - #[allow(dead_code)] - #[error("support for component {component} compiled out via {feature}")] - FeatureCompiledOut { component: SpecKey, feature: &'static str }, - - #[error("backend {0} not used by any device")] - BackendNotUsed(SpecKey), -} - -impl From for v1::instance_spec::InstanceSpec { - fn from(val: Spec) -> Self { - // Exhaustively destructure the input spec so that adding a new field - // without considering it here will break the build. - let Spec { - board, - cpuid, - disks, - nics, - boot_settings, - serial, - pci_pci_bridges, - pvpanic, - #[cfg(feature = "failure-injection")] - migration_failure, - #[cfg(feature = "falcon")] - softnpu, - - // Not part of `v1::instance_spec::InstanceSpec`. Added in - // `InstanceSpec` in API Version 2.0.0. - smbios_type1_input: _, - - // Not part of `v1::instance_spec::InstanceSpec`. Added in - // `InstanceSpec` in API Version 3.0.0. - vsock: _, - } = val; - - // Inserts a component entry into the supplied map, asserting first that - // the supplied key is not present in that map. - // - // This assertion is valid because internal instance specs should assign - // a unique name to each component they describe. The spec builder - // upholds this invariant at spec creation time. - #[track_caller] - fn insert_component( - spec: &mut v1::instance_spec::InstanceSpec, - key: SpecKey, - val: v1::instance_spec::Component, - ) { - assert!( - !spec.components.contains_key(&key), - "component name {} already exists in output spec", - &key - ); - spec.components.insert(key, val); - } - - let board = InstanceSpecBoard { - cpus: board.cpus, - memory_mb: board.memory_mb, - chipset: board.chipset, - guest_hv_interface: board.guest_hv_interface, - cpuid: Some(cpuid.into_instance_spec_cpuid()), - }; - let mut spec = v1::instance_spec::InstanceSpec { - board, - components: Default::default(), - }; - - for (disk_id, disk) in disks { - let backend_id = disk.device_spec.backend_id().to_owned(); - insert_component(&mut spec, disk_id, disk.device_spec.into()); - insert_component(&mut spec, backend_id, disk.backend_spec.into()); - } - - for (nic_id, nic) in nics { - let backend_id = nic.device_spec.backend_id.clone(); - insert_component( - &mut spec, - nic_id, - v1::instance_spec::Component::VirtioNic(nic.device_spec), - ); - - insert_component( - &mut spec, - backend_id, - v1::instance_spec::Component::VirtioNetworkBackend( - nic.backend_spec, - ), - ); - } - - for (name, desc) in serial { - if desc.device == SerialPortDevice::Uart { - insert_component( - &mut spec, - name, - v1::instance_spec::Component::SerialPort(SerialPortDesc { - num: desc.num, - }), - ); - } - } - - for (bridge_name, bridge) in pci_pci_bridges { - insert_component( - &mut spec, - bridge_name, - v1::instance_spec::Component::PciPciBridge(bridge), - ); - } - - if let Some(pvpanic) = pvpanic { - insert_component( - &mut spec, - pvpanic.id, - v1::instance_spec::Component::QemuPvpanic(pvpanic.spec), - ); - } - - if let Some(settings) = boot_settings { - insert_component( - &mut spec, - settings.name, - v1::instance_spec::Component::BootSettings(BootSettings { - order: settings.order.into_iter().map(Into::into).collect(), - }), - ); - } - - #[cfg(feature = "failure-injection")] - if let Some(mig) = migration_failure { - insert_component( - &mut spec, - mig.id, - v1::instance_spec::Component::MigrationFailureInjector( - mig.spec, - ), - ); - } - - #[cfg(feature = "falcon")] - { - if let Some(softnpu_pci) = softnpu.pci_port { - insert_component( - &mut spec, - SpecKey::Name(format!( - "softnpu-pci-{}", - softnpu_pci.pci_path - )), - v1::instance_spec::Component::SoftNpuPciPort(softnpu_pci), - ); - } - - if let Some(p9) = softnpu.p9_device { - insert_component( - &mut spec, - SpecKey::Name(format!("softnpu-p9-{}", p9.pci_path)), - v1::instance_spec::Component::SoftNpuP9(p9), - ); - } - - if let Some(p9fs) = softnpu.p9fs { - insert_component( - &mut spec, - SpecKey::Name(format!("p9fs-{}", p9fs.pci_path)), - v1::instance_spec::Component::P9fs(p9fs), - ); - } - - for (port_name, port) in softnpu.ports { - insert_component( - &mut spec, - port_name.clone(), - v1::instance_spec::Component::SoftNpuPort( - SoftNpuPortSpec { - link_name: port.link_name, - backend_id: port.backend_name.clone(), - }, - ), - ); - - insert_component( - &mut spec, - port.backend_name, - v1::instance_spec::Component::DlpiNetworkBackend( - port.backend_spec, - ), - ); - } - } - - spec - } -} - -impl TryFrom for Spec { - type Error = ApiSpecError; - - fn try_from( - value: v1::instance_spec::InstanceSpec, - ) -> Result { - Ok(v1_to_spec_builder(value)?.finish()) - } -} - -/// Parses a v1 instance spec into a [`SpecBuilder`], validating component -/// names, PCI paths, and backend references along the way. Callers can add -/// additional (non-v1) components to the builder before calling `finish()`. -pub(crate) fn v1_to_spec_builder( - value: v1::instance_spec::InstanceSpec, -) -> Result { - let mut builder = SpecBuilder::with_instance_spec_board(value.board)?; - let mut devices: Vec<(SpecKey, v1::instance_spec::Component)> = vec![]; - let mut boot_settings = None; - let mut storage_backends: BTreeMap = - BTreeMap::new(); - let mut viona_backends: BTreeMap = - BTreeMap::new(); - let mut dlpi_backends: BTreeMap = - BTreeMap::new(); - - for (id, component) in value.components.into_iter() { - match component { - v1::instance_spec::Component::CrucibleStorageBackend(_) - | v1::instance_spec::Component::FileStorageBackend(_) - | v1::instance_spec::Component::BlobStorageBackend(_) => { - storage_backends.insert( - id, - component - .try_into() - .expect("component is known to be a storage backend"), - ); - } - v1::instance_spec::Component::VirtioNetworkBackend(viona) => { - viona_backends.insert(id, viona); - } - v1::instance_spec::Component::DlpiNetworkBackend(dlpi) => { - dlpi_backends.insert(id, dlpi); - } - device => { - devices.push((id, device)); - } - } - } - - for (device_id, device_spec) in devices { - match device_spec { - v1::instance_spec::Component::VirtioDisk(_) - | v1::instance_spec::Component::NvmeDisk(_) => { - let device_spec = StorageDevice::try_from(device_spec) - .expect("component is known to be a disk"); - - let (_, backend_spec) = storage_backends - .remove_entry(device_spec.backend_id()) - .ok_or_else(|| ApiSpecError::StorageBackendNotFound { - backend: device_spec.backend_id().to_owned(), - device: device_id.clone(), - })?; - - builder.add_storage_device( - device_id, - Disk { device_spec, backend_spec }, - )?; - } - v1::instance_spec::Component::VirtioNic(nic) => { - let (_, backend_spec) = viona_backends - .remove_entry(&nic.backend_id) - .ok_or_else(|| ApiSpecError::NetworkBackendNotFound { - backend: nic.backend_id.clone(), - device: device_id.clone(), - })?; - - builder.add_network_device( - device_id, - Nic { device_spec: nic, backend_spec }, - )?; - } - v1::instance_spec::Component::SerialPort(port) => { - builder.add_serial_port(device_id, port.num)?; - } - v1::instance_spec::Component::PciPciBridge(bridge) => { - builder.add_pci_bridge(device_id, bridge)?; - } - v1::instance_spec::Component::QemuPvpanic(pvpanic) => { - builder.add_pvpanic_device(QemuPvpanic { - id: device_id, - spec: pvpanic, - })?; - } - v1::instance_spec::Component::BootSettings(settings) => { - // The builder returns an error if its caller tries to add - // a boot option that isn't in the set of attached disks. - // Since there may be more disk devices left in the - // component map, just capture the boot order for now and - // apply it to the builder later. - boot_settings = Some((device_id, settings)); - } - #[cfg(not(feature = "failure-injection"))] - v1::instance_spec::Component::MigrationFailureInjector(_) => { - return Err(ApiSpecError::FeatureCompiledOut { - component: device_id, - feature: "failure-injection", - }); - } - #[cfg(feature = "failure-injection")] - v1::instance_spec::Component::MigrationFailureInjector(mig) => { - builder.add_migration_failure_device(MigrationFailure { - id: device_id, - spec: mig, - })?; - } - #[cfg(not(feature = "falcon"))] - v1::instance_spec::Component::SoftNpuPciPort(_) - | v1::instance_spec::Component::SoftNpuPort(_) - | v1::instance_spec::Component::SoftNpuP9(_) - | v1::instance_spec::Component::P9fs(_) => { - return Err(ApiSpecError::FeatureCompiledOut { - component: device_id, - feature: "falcon", - }); - } - #[cfg(feature = "falcon")] - v1::instance_spec::Component::SoftNpuPciPort(port) => { - builder.set_softnpu_pci_port(port)?; - } - #[cfg(feature = "falcon")] - v1::instance_spec::Component::SoftNpuPort(port) => { - let (_, backend_spec) = dlpi_backends - .remove_entry(&port.backend_id) - .ok_or_else(|| ApiSpecError::NetworkBackendNotFound { - backend: port.backend_id.clone(), - device: device_id.clone(), - })?; - - let port = SoftNpuPort { - link_name: port.link_name, - backend_name: port.backend_id, - backend_spec, - }; - - builder.add_softnpu_port(device_id, port)?; - } - #[cfg(feature = "falcon")] - v1::instance_spec::Component::SoftNpuP9(p9) => { - builder.set_softnpu_p9(p9)?; - } - #[cfg(feature = "falcon")] - v1::instance_spec::Component::P9fs(p9fs) => { - builder.set_p9fs(p9fs)?; - } - v1::instance_spec::Component::CrucibleStorageBackend(_) - | v1::instance_spec::Component::FileStorageBackend(_) - | v1::instance_spec::Component::BlobStorageBackend(_) - | v1::instance_spec::Component::VirtioNetworkBackend(_) - | v1::instance_spec::Component::DlpiNetworkBackend(_) => { - unreachable!("already filtered out backends") - } - } - } - - // Now that all disks have been attached, try to establish the boot - // order if one was supplied. - if let Some(settings) = boot_settings { - builder.add_boot_order( - settings.0, - settings.1.order.into_iter().map(Into::into), - )?; - } - - if let Some(backend) = storage_backends.into_keys().next() { - return Err(ApiSpecError::BackendNotUsed(backend)); - } - - if let Some(backend) = viona_backends.into_keys().next() { - return Err(ApiSpecError::BackendNotUsed(backend)); - } - - if let Some(backend) = dlpi_backends.into_keys().next() { - return Err(ApiSpecError::BackendNotUsed(backend)); - } - - Ok(builder) -} diff --git a/bin/propolis-server/src/lib/spec/api_spec_v1.rs b/bin/propolis-server/src/lib/spec/api_spec_v1.rs new file mode 100644 index 000000000..0337b717a --- /dev/null +++ b/bin/propolis-server/src/lib/spec/api_spec_v1.rs @@ -0,0 +1,376 @@ +// 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/. + +//! Conversions between [`propolis_api_types_versions::v1`] (aka "V0" in some +//! parts of propolis-server) instance specs and the internal [`super::Spec`] +//! representation. + +use std::collections::BTreeMap; + +use propolis_api_types::instance_spec::{ + components::{ + board::Board as InstanceSpecBoard, + devices::{BootSettings, SerialPort as SerialPortDesc}, + }, + SpecKey, +}; +use propolis_api_types_versions::{ + v1, v1::instance::ReplacementComponent, v2, v3, v6, +}; + +#[cfg(feature = "falcon")] +use propolis_api_types::instance_spec::components::devices::SoftNpuPort as SoftNpuPortSpec; + +use super::{ + builder::SpecBuilder, LegacyApiSpecError, SerialPortDevice, Spec, + StorageBackend, StorageDevice, +}; +use crate::migrate::MigrateError; + +#[cfg(feature = "failure-injection")] +use super::MigrationFailure; + +// Woah! It's strange to have a conversion to a *v1* type which has an error +// from *v6* about *v3*. Not as bad as it seems though: v6 is when this +// component changed, and v3 is next-most-recent version of +// `instance_spec::Component`. So in v6, the error is "I can't convert this to +// v3::instance_spec::Component". +// +// We're converting to v1, though, which has a further-different `Component` +// type. If we had a fallible operation converting a storage device all the way +// down, that might produce an error about failing to convert a V3 type to V1 - +// it turns out that's infallible here. +impl TryFrom for v1::instance_spec::Component { + type Error = v6::instance_spec::InvalidV3Component; + + fn try_from(value: StorageDevice) -> Result { + match value { + StorageDevice::Virtio(d) => Ok(Self::VirtioDisk(d)), + StorageDevice::Nvme(d) => Ok(Self::NvmeDisk(d.try_into()?)), + } + } +} + +impl From for v1::instance_spec::Component { + fn from(value: StorageBackend) -> Self { + match value { + StorageBackend::Crucible(be) => Self::CrucibleStorageBackend(be), + StorageBackend::File(be) => Self::FileStorageBackend(be), + StorageBackend::Blob(be) => Self::BlobStorageBackend(be), + } + } +} + +impl TryFrom for v1::instance_spec::InstanceSpec { + type Error = LegacyApiSpecError; + + fn try_from(val: Spec) -> Result { + // Exhaustively destructure the input spec so that adding a new field + // without considering it here will break the build. + let Spec { + board, + cpuid, + disks, + nics, + boot_settings, + serial, + pci_pci_bridges, + pvpanic, + #[cfg(feature = "failure-injection")] + migration_failure, + #[cfg(feature = "falcon")] + softnpu, + + // Not part of `v1::instance_spec::InstanceSpec`. Added in + // `InstanceSpec` in API Version 2.0.0. + smbios_type1_input, + + // Not part of `v1::instance_spec::InstanceSpec`. Added in + // `InstanceSpec` in API Version 3.0.0. + vsock, + } = val; + + if smbios_type1_input.is_some() { + // NOTE: This is overly strict. There is one specific SMBIOS Type 1 + // table that could be expressed previously, and that is the one + // where the instance serial is set to the instance UUID. + // + // This is the Type 1 table provided by Nexus as of specs later than + // V1, so by bailing here we're effectively blocking migration from + // new Propolises to old Propolises. This is acceptable for a few + // reasons: + // * the control plane is not expected to migrate VMs to down-rev + // Propolises + // * V1 specs are from before live migration was done outside + // ad-hoc/CI environments - such an old Propolis will never exist + // as a migration target in the field. + return Err(LegacyApiSpecError::IncompatibleComponent( + "cannot express explicit SMBIOS tables in v1 instance spec" + .to_string(), + )); + } + + if vsock.is_some() { + return Err(LegacyApiSpecError::IncompatibleComponent( + "cannot convert virtio-socket to v1 instance spec".to_string(), + )); + } + + // Inserts a component entry into the supplied map, asserting first that + // the supplied key is not present in that map. + // + // This assertion is valid because internal instance specs should assign + // a unique name to each component they describe. The spec builder + // upholds this invariant at spec creation time. + #[track_caller] + fn insert_component( + spec: &mut v1::instance_spec::InstanceSpec, + key: SpecKey, + val: v1::instance_spec::Component, + ) { + assert!( + !spec.components.contains_key(&key), + "component name {} already exists in output spec", + &key + ); + spec.components.insert(key, val); + } + + let board = InstanceSpecBoard { + cpus: board.cpus, + memory_mb: board.memory_mb, + chipset: board.chipset, + guest_hv_interface: board.guest_hv_interface, + cpuid: Some(cpuid.into_instance_spec_cpuid()), + }; + let mut spec = v1::instance_spec::InstanceSpec { + board, + components: Default::default(), + }; + + for (disk_id, disk) in disks { + let backend_id = disk.device_spec.backend_id().to_owned(); + let device_component: v1::instance_spec::Component = disk + .device_spec + .try_into() + .map_err(|e: v6::instance_spec::InvalidV3Component| { + LegacyApiSpecError::IncompatibleComponent(e.to_string()) + })?; + let backend_component: v1::instance_spec::Component = + disk.backend_spec.into(); + insert_component(&mut spec, disk_id, device_component); + insert_component(&mut spec, backend_id, backend_component); + } + + for (nic_id, nic) in nics { + let backend_id = nic.device_spec.backend_id.clone(); + insert_component( + &mut spec, + nic_id, + v1::instance_spec::Component::VirtioNic(nic.device_spec), + ); + + insert_component( + &mut spec, + backend_id, + v1::instance_spec::Component::VirtioNetworkBackend( + nic.backend_spec, + ), + ); + } + + for (name, desc) in serial { + if desc.device == SerialPortDevice::Uart { + insert_component( + &mut spec, + name, + v1::instance_spec::Component::SerialPort(SerialPortDesc { + num: desc.num, + }), + ); + } + } + + for (bridge_name, bridge) in pci_pci_bridges { + insert_component( + &mut spec, + bridge_name, + v1::instance_spec::Component::PciPciBridge(bridge), + ); + } + + if let Some(pvpanic) = pvpanic { + insert_component( + &mut spec, + pvpanic.id, + v1::instance_spec::Component::QemuPvpanic(pvpanic.spec), + ); + } + + if let Some(settings) = boot_settings { + insert_component( + &mut spec, + settings.name, + v1::instance_spec::Component::BootSettings(BootSettings { + order: settings.order.into_iter().map(Into::into).collect(), + }), + ); + } + + #[cfg(feature = "failure-injection")] + if let Some(mig) = migration_failure { + insert_component( + &mut spec, + mig.id, + v1::instance_spec::Component::MigrationFailureInjector( + mig.spec, + ), + ); + } + + #[cfg(feature = "falcon")] + { + if let Some(softnpu_pci) = softnpu.pci_port { + insert_component( + &mut spec, + SpecKey::Name(format!( + "softnpu-pci-{}", + softnpu_pci.pci_path + )), + v1::instance_spec::Component::SoftNpuPciPort(softnpu_pci), + ); + } + + if let Some(p9) = softnpu.p9_device { + insert_component( + &mut spec, + SpecKey::Name(format!("softnpu-p9-{}", p9.pci_path)), + v1::instance_spec::Component::SoftNpuP9(p9), + ); + } + + if let Some(p9fs) = softnpu.p9fs { + insert_component( + &mut spec, + SpecKey::Name(format!("p9fs-{}", p9fs.pci_path)), + v1::instance_spec::Component::P9fs(p9fs), + ); + } + + for (port_name, port) in softnpu.ports { + insert_component( + &mut spec, + port_name.clone(), + v1::instance_spec::Component::SoftNpuPort( + SoftNpuPortSpec { + link_name: port.link_name, + backend_id: port.backend_name.clone(), + }, + ), + ); + + insert_component( + &mut spec, + port.backend_name, + v1::instance_spec::Component::DlpiNetworkBackend( + port.backend_spec, + ), + ); + } + } + + Ok(spec) + } +} + +impl TryFrom for Spec { + type Error = LegacyApiSpecError; + + fn try_from( + value: v1::instance_spec::InstanceSpec, + ) -> Result { + Ok(v1_to_spec_builder(value)?.finish()) + } +} + +/// Parses a v1 instance spec into a [`SpecBuilder`], validating component +/// names, PCI paths, and backend references along the way. Callers can add +/// additional (non-v1) components to the builder before calling `finish()`. +pub(crate) fn v1_to_spec_builder( + value: v1::instance_spec::InstanceSpec, +) -> Result { + let v2_spec: v2::instance_spec::InstanceSpec = value.into(); + let v3_spec: v3::instance_spec::InstanceSpec = v2_spec.into(); + + crate::spec::api_spec_v3::v3_to_spec_builder(v3_spec) +} + +// `amend_component` is suitable for (and used in) amending a v2 InstanceSpec, +// so this one is pub(crate) unlike other `api_spec_v*`. +pub(crate) fn amend_component( + id: &SpecKey, + to_amend: &mut v1::instance_spec::Component, + replacement: &ReplacementComponent, +) -> Result<(), MigrateError> { + match replacement { + #[cfg(not(feature = "failure-injection"))] + ReplacementComponent::MigrationFailureInjector(_) => { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacing migration failure injector {id} is \ + impossible because the feature is compiled out" + ))); + } + + #[cfg(feature = "failure-injection")] + ReplacementComponent::MigrationFailureInjector(comp) => { + let v1::instance_spec::Component::MigrationFailureInjector(src) = + to_amend + else { + return Err(MigrateError::wrong_type( + id, + "migration failure injector", + )); + }; + + *src = comp.clone(); + } + ReplacementComponent::CrucibleStorageBackend(comp) => { + let v1::instance_spec::Component::CrucibleStorageBackend(src) = + to_amend + else { + return Err(MigrateError::wrong_type(id, "crucible backend")); + }; + + *src = comp.clone(); + } + ReplacementComponent::VirtioNetworkBackend(comp) => { + let v1::instance_spec::Component::VirtioNetworkBackend(src) = + to_amend + else { + return Err(MigrateError::wrong_type(id, "viona backend")); + }; + + *src = comp.clone(); + } + } + + Ok(()) +} + +pub(crate) fn amend( + spec: &mut v1::instance_spec::InstanceSpec, + replacements: &BTreeMap, +) -> Result<(), MigrateError> { + for (id, replacement) in replacements { + let Some(to_amend) = spec.components.get_mut(id) else { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacement component {id} not in source spec", + ))); + }; + + amend_component(id, to_amend, replacement)?; + } + + Ok(()) +} diff --git a/bin/propolis-server/src/lib/spec/api_spec_v2.rs b/bin/propolis-server/src/lib/spec/api_spec_v2.rs new file mode 100644 index 000000000..d7f764177 --- /dev/null +++ b/bin/propolis-server/src/lib/spec/api_spec_v2.rs @@ -0,0 +1,75 @@ +// 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/. + +//! Conversions between [`propolis_api_types_versions::v2`] instance specs and +//! the internal [`super::Spec`] representation. + +use std::collections::BTreeMap; + +use propolis_api_types::instance_spec::SpecKey; +use propolis_api_types_versions::{ + v1, v1::instance::ReplacementComponent, v2, v3, +}; + +use super::{builder::SpecBuilder, LegacyApiSpecError, Spec}; +use crate::migrate::MigrateError; + +#[cfg(feature = "failure-injection")] +use super::MigrationFailure; + +impl TryFrom for v2::instance_spec::InstanceSpec { + type Error = LegacyApiSpecError; + + fn try_from(mut val: Spec) -> Result { + // A V2 InstanceSpec is just a V1 InstanceSpec with an optional + // `smbios_type1_input`. Emptying out the SMBIOS Type 1 input means + // this either can be converted to a V1 spec which we can losslessly + // make V2 by adding the SMBIOS table input back in, or we wouldn't be + // able to get to a V2 InstanceSpec either way. + let smbios = val.smbios_type1_input.take(); + + let v1::instance_spec::InstanceSpec { board, components } = + val.try_into()?; + + Ok(v2::instance_spec::InstanceSpec { board, smbios, components }) + } +} + +impl TryFrom for Spec { + type Error = LegacyApiSpecError; + + fn try_from( + value: v2::instance_spec::InstanceSpec, + ) -> Result { + Ok(v2_to_spec_builder(value)?.finish()) + } +} + +/// Parses a v2 instance spec into a [`SpecBuilder`], validating component +/// names, PCI paths, and backend references along the way. Callers can add +/// additional (non-v2) components to the builder before calling `finish()`. +pub(crate) fn v2_to_spec_builder( + value: v2::instance_spec::InstanceSpec, +) -> Result { + let v3_spec: v3::instance_spec::InstanceSpec = value.into(); + + crate::spec::api_spec_v3::v3_to_spec_builder(v3_spec) +} + +pub(crate) fn amend( + spec: &mut v2::instance_spec::InstanceSpec, + replacements: &BTreeMap, +) -> Result<(), MigrateError> { + for (id, replacement) in replacements { + let Some(to_amend) = spec.components.get_mut(id) else { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacement component {id} not in source spec", + ))); + }; + + super::api_spec_v1::amend_component(id, to_amend, replacement)?; + } + + Ok(()) +} diff --git a/bin/propolis-server/src/lib/spec/api_spec_v3.rs b/bin/propolis-server/src/lib/spec/api_spec_v3.rs new file mode 100644 index 000000000..d55cc609b --- /dev/null +++ b/bin/propolis-server/src/lib/spec/api_spec_v3.rs @@ -0,0 +1,127 @@ +// 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/. + +//! Conversions between [`propolis_api_types_versions::v3`] instance specs and +//! the internal [`super::Spec`] representation. + +use std::collections::BTreeMap; + +use propolis_api_types::instance_spec::SpecKey; +use propolis_api_types_versions::{ + v1::instance::ReplacementComponent, v2, v3, v6, +}; + +use super::{api_spec_v6, builder::SpecBuilder, LegacyApiSpecError, Spec}; +use crate::migrate::MigrateError; + +impl TryFrom for v3::instance_spec::InstanceSpec { + type Error = LegacyApiSpecError; + + fn try_from(mut val: Spec) -> Result { + // v3 added only the `vsock` component, which is expressed only as the + // `vsock` field on `Spec` here. Either we can remove it and this is a + // Spec that can be interpreted as v2, or this spec is not valid as + // either. + let vsock = val.vsock.take(); + + let v2_instance_spec: v2::instance_spec::InstanceSpec = + val.try_into()?; + let mut instance_spec: v3::instance_spec::InstanceSpec = + v2_instance_spec.into(); + + if let Some(vsock) = vsock { + let existing = instance_spec.components.insert( + vsock.id.clone(), + v3::instance_spec::Component::VirtioSocket(vsock.spec), + ); + assert!( + existing.is_none(), + "there was already a component named {} in the spec?!", + vsock.id + ); + } + + Ok(instance_spec) + } +} + +/// Parses a v3 instance spec into a [`SpecBuilder`], validating component +/// names, PCI paths, and backend references along the way. Callers can add +/// additional (non-v3) components to the builder before calling `finish()`. +pub(crate) fn v3_to_spec_builder( + value: v3::instance_spec::InstanceSpec, +) -> Result { + // Converting v3 to v6 is lossless so just do that and piggyback on the + // v6 `InstanceSpec->SpecBuilder`. + let v6_spec: v6::instance_spec::InstanceSpec = value.into(); + + api_spec_v6::v6_to_spec_builder(v6_spec).map_err(|e| e.into()) +} + +fn amend_component( + id: &SpecKey, + to_amend: &mut v3::instance_spec::Component, + replacement: &ReplacementComponent, +) -> Result<(), MigrateError> { + match replacement { + #[cfg(not(feature = "failure-injection"))] + ReplacementComponent::MigrationFailureInjector(_) => { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacing migration failure injector {id} is \ + impossible because the feature is compiled out" + ))); + } + + #[cfg(feature = "failure-injection")] + ReplacementComponent::MigrationFailureInjector(comp) => { + let v3::instance_spec::Component::MigrationFailureInjector(src) = + to_amend + else { + return Err(MigrateError::wrong_type( + id, + "migration failure injector", + )); + }; + + *src = comp.clone(); + } + ReplacementComponent::CrucibleStorageBackend(comp) => { + let v3::instance_spec::Component::CrucibleStorageBackend(src) = + to_amend + else { + return Err(MigrateError::wrong_type(id, "crucible backend")); + }; + + *src = comp.clone(); + } + ReplacementComponent::VirtioNetworkBackend(comp) => { + let v3::instance_spec::Component::VirtioNetworkBackend(src) = + to_amend + else { + return Err(MigrateError::wrong_type(id, "viona backend")); + }; + + *src = comp.clone(); + } + } + + Ok(()) +} + +pub(crate) fn amend( + spec: &mut v3::instance_spec::InstanceSpec, + replacements: &BTreeMap, +) -> Result<(), MigrateError> { + for (id, replacement) in replacements { + let Some(to_amend) = spec.components.get_mut(id) else { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacement component {id} not in source spec", + ))); + }; + + amend_component(id, to_amend, replacement)?; + } + + Ok(()) +} diff --git a/bin/propolis-server/src/lib/spec/api_spec_v6.rs b/bin/propolis-server/src/lib/spec/api_spec_v6.rs new file mode 100644 index 000000000..3cdadcb10 --- /dev/null +++ b/bin/propolis-server/src/lib/spec/api_spec_v6.rs @@ -0,0 +1,158 @@ +// 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/. + +//! Conversions between [`propolis_api_types_versions::v6`] instance specs and +//! the internal [`super::Spec`] representation. + +use std::collections::BTreeMap; + +use propolis_api_types::instance_spec::SpecKey; +use propolis_api_types_versions::{v1::instance::ReplacementComponent, v3, v6}; + +use super::{builder::SpecBuilder, ApiSpecError, Disk, Spec, StorageDevice}; +use crate::migrate::MigrateError; +use crate::spec::api_spec_latest; + +impl From for v6::instance_spec::InstanceSpec { + fn from(mut val: Spec) -> Self { + // v6 adds a new field on NvmeDisk. Such disks probably can't be + // converted to v3 components and would cause a conversion from + // Spec->v3::instance_spec::InstanceSpec to fail. So, extract those + // disks and convert the rest of the Spec to a + // v3::instance_spec::InstanceSpec. If this fails, we wouldn't have been + // able to get a v6 spec anyway. If it succeeds, we can add the disks + // back in here. + // + // TODO: could be extract_if once we're on a Rust >= 1.91.0. + let mut nvme_disks = Vec::new(); + let v6_only_disk = + |disk: &Disk| matches!(disk.device_spec, StorageDevice::Nvme(_)); + for (key, disk) in val.disks.iter() { + if v6_only_disk(disk) { + nvme_disks.push((key.clone(), disk.clone())); + } + } + val.disks.retain(|_, disk| !v6_only_disk(disk)); + + let v3_spec: v3::instance_spec::InstanceSpec = + val.try_into().unwrap_or_else(|e| { + unreachable!( + "Converting to Spec without v6 bits to v3 failed: {e}. \ + This is currently impossible. When Spec to \ + v6::instance_spec::InstanceSpec becomes fallible, \ + this should `?`." + ); + }); + + let mut spec: v6::instance_spec::InstanceSpec = v3_spec.into(); + + // Inserts a component entry into the supplied map, asserting first that + // the supplied key is not present in that map. + // + // This assertion is valid because internal instance specs should assign + // a unique name to each component they describe. The spec builder + // upholds this invariant at spec creation time. + #[track_caller] + fn insert_component( + spec: &mut v6::instance_spec::InstanceSpec, + key: SpecKey, + val: v6::instance_spec::Component, + ) { + assert!( + !spec.components.contains_key(&key), + "component name {} already exists in output spec", + &key + ); + spec.components.insert(key, val); + } + + for (disk_id, disk) in nvme_disks { + let backend_id = disk.device_spec.backend_id().to_owned(); + let device_component: v6::instance_spec::Component = + disk.device_spec.into(); + let backend_component: v6::instance_spec::Component = + disk.backend_spec.into(); + insert_component(&mut spec, disk_id, device_component); + insert_component(&mut spec, backend_id, backend_component); + } + + spec + } +} + +/// Parses a v6 instance spec into a [`SpecBuilder`], validating component +/// names, PCI paths, and backend references along the way. Callers can add +/// additional (non-v6) components to the builder before calling `finish()`. +pub(crate) fn v6_to_spec_builder( + value: v6::instance_spec::InstanceSpec, +) -> Result { + api_spec_latest::latest_to_spec_builder(value) +} + +fn amend_component( + id: &SpecKey, + to_amend: &mut v6::instance_spec::Component, + replacement: &ReplacementComponent, +) -> Result<(), MigrateError> { + match replacement { + #[cfg(not(feature = "failure-injection"))] + ReplacementComponent::MigrationFailureInjector(_) => { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacing migration failure injector {id} is \ + impossible because the feature is compiled out" + ))); + } + + #[cfg(feature = "failure-injection")] + ReplacementComponent::MigrationFailureInjector(comp) => { + let v6::instance_spec::Component::MigrationFailureInjector(src) = + to_amend + else { + return Err(MigrateError::wrong_type( + id, + "migration failure injector", + )); + }; + + *src = comp.clone(); + } + ReplacementComponent::CrucibleStorageBackend(comp) => { + let v6::instance_spec::Component::CrucibleStorageBackend(src) = + to_amend + else { + return Err(MigrateError::wrong_type(id, "crucible backend")); + }; + + *src = comp.clone(); + } + ReplacementComponent::VirtioNetworkBackend(comp) => { + let v6::instance_spec::Component::VirtioNetworkBackend(src) = + to_amend + else { + return Err(MigrateError::wrong_type(id, "viona backend")); + }; + + *src = comp.clone(); + } + } + + Ok(()) +} + +pub(crate) fn amend( + spec: &mut v6::instance_spec::InstanceSpec, + replacements: &BTreeMap, +) -> Result<(), MigrateError> { + for (id, replacement) in replacements { + let Some(to_amend) = spec.components.get_mut(id) else { + return Err(MigrateError::InstanceSpecsIncompatible(format!( + "replacement component {id} not in source spec", + ))); + }; + + amend_component(id, to_amend, replacement)?; + } + + Ok(()) +} diff --git a/bin/propolis-server/src/lib/spec/builder.rs b/bin/propolis-server/src/lib/spec/builder.rs index 993697259..78560da8e 100644 --- a/bin/propolis-server/src/lib/spec/builder.rs +++ b/bin/propolis-server/src/lib/spec/builder.rs @@ -71,6 +71,18 @@ pub(crate) enum SpecBuilderError { DefaultCpuidReadFailed(#[from] cpuid_utils::host::GetHostCpuidError), } +/// A builder onto which devices and other components are added as an +/// `InstanceSpec` is interpreted. Among other things, this services as a +/// forcing function to canonicalize VM descriptions, where we can enforce +/// invariants about components (such as "disk devices must reference backends +/// that exist"). +/// +/// Note that the API type `Component` itself does not appear here: the +/// expectation is that individual components' definitions change relatively +/// rarely, so callers do the work of mapping components to the +/// closer-to-internal definitions that `SpecBuilder` accepts. In theory, +/// hopefully, this means `SpecBuilder` itself changes rarely and can be more +/// readily audited for semantic drift. #[derive(Debug, Default)] pub(crate) struct SpecBuilder { spec: super::Spec, diff --git a/bin/propolis-server/src/lib/spec/mod.rs b/bin/propolis-server/src/lib/spec/mod.rs index b6110e16a..79c613650 100644 --- a/bin/propolis-server/src/lib/spec/mod.rs +++ b/bin/propolis-server/src/lib/spec/mod.rs @@ -13,10 +13,26 @@ //! wire-format types in the [`propolis_api_types`] crate. This, in turn, allows //! [`Spec`] and its component types to take forms that might otherwise be hard //! to change in a backward-compatible way. +//! +//! Types and operations here are copied as-needed into new version-specific +//! modules, as new versions of the `propolis-server` HTTP API are added. +//! +//! ## New Versions +//! +//! When adding a new API version, particularly when those API versions inclue a +//! new defintion for `InstanceSpec`, you will probably want to: +//! +//! * add a new `api_spec_vN` module for the new version, with conversions from +//! `Spec` to the `vN::instance_spec::InstanceSpec` via the previous version +//! and from `vN::instance_spec::InstanceSpec` to `Spec` via +//! `latest_to_spec_builder()` +//! * adjust the formerly-latest module's `vN_to_spec_builder` to call +//! `vN_to_spec_builder` and (try) converting `InstanceSpec` upwards to match. +//! * adjust the formerly-latest module's `Spec` to `InstanceSpec` conversion to +//! be fallible, using `LegacyApiSpecError` instead of `ApiSpecError` use std::collections::BTreeMap; -use crate::spec::api_spec_v0::ApiSpecError; use cpuid_utils::CpuidSet; use propolis_api_types::instance_spec::{ components::{ @@ -36,7 +52,7 @@ use propolis_api_types::instance_spec::{ use propolis_api_types::instance_spec::{ Component, InstanceSpec, SmbiosType1Input, }; -use propolis_api_types_versions::{v1, v2}; +use propolis_api_types_versions::latest; use thiserror::Error; #[cfg(feature = "failure-injection")] @@ -49,55 +65,69 @@ use propolis_api_types::instance_spec::components::{ devices::{P9fs, SoftNpuP9, SoftNpuPciPort}, }; -// mod api_request; -pub(crate) mod api_spec_v0; +pub(crate) mod api_spec_latest; +pub(crate) mod api_spec_v1; +pub(crate) mod api_spec_v2; +pub(crate) mod api_spec_v3; +pub(crate) mod api_spec_v6; pub(crate) mod builder; -/// The code related to latest types does not go into a versioned module -impl From for InstanceSpec { - fn from(val: Spec) -> Self { - let smbios = val.smbios_type1_input.clone(); - let vsock = val.vsock.clone(); +/// An error that can arise in converting +/// `propolis_api_types::instance_spec::InstanceSpec` to a propolis-server +/// `Spec`. To date, this is a subset of the errors that can arise in converting +/// older versions of `InstanceSpec` as well. +/// +/// When adding a new `api_spec_v*`, it may be appropriate to either move this +/// error type into the formerly-latest module and add a new top-level type, or +/// to simply rename uses of this error in the formerly-latest module to +/// `LegacyApiSpecError`. Whichever is appropriate depends on how similar the +/// errors in the newly version are to the errors described here. +#[derive(Debug, Error)] +pub(crate) enum ApiSpecError { + #[error(transparent)] + Builder(#[from] builder::SpecBuilderError), - let v1_spec: v1::instance_spec::InstanceSpec = val.into(); - let v2_spec = - v2::instance_spec::InstanceSpec { smbios, ..v1_spec.into() }; - let mut spec: InstanceSpec = v2_spec.into(); + #[error("storage backend {backend} not found for device {device}")] + StorageBackendNotFound { backend: SpecKey, device: SpecKey }, - if let Some(vsock) = vsock { - spec.components - .insert(vsock.id, Component::VirtioSocket(vsock.spec)); - } - spec - } + #[error("network backend {backend} not found for device {device}")] + NetworkBackendNotFound { backend: SpecKey, device: SpecKey }, + + #[allow(dead_code)] + #[error("support for component {component} compiled out via {feature}")] + FeatureCompiledOut { component: SpecKey, feature: &'static str }, + + #[error("backend {0} not used by any device")] + BackendNotUsed(SpecKey), +} + +/// An error that can arise in converting any older versions of `InstanceSpec` +/// to a propolis-server `Spec`. To date, this may be any of the errors that can +/// occur in converting the most recent version of the spec, plus errors in +/// converting components to older API forms. +/// +/// This type, as well as `ApiSpecError` are best-effort attempts to describe +/// the error space as we've seen it so far; if the "legacy" kinds of errors end +/// up variable it may make sense to revisit even having a "shared" error type +/// for these conversions. +#[derive(Debug, Error)] +pub(crate) enum LegacyApiSpecError { + #[error(transparent)] + SpecError(#[from] ApiSpecError), + + #[error("spec contains v1-incompatible component: {0}")] + IncompatibleComponent(String), } -/// The code related to latest types does not go into a versioned module +/// `propolis-server` relies on `TryInto` to convert the API-provided +/// `InstanceSpec` to an internal `Spec`. When adding a new API version to +/// `propolis-server` you will probably want to take this implementation and +/// copy it into the no-longer-latest `api_spec_v*` module. impl TryFrom for Spec { type Error = ApiSpecError; fn try_from(value: InstanceSpec) -> Result { - // Extract vsock before conversion since it's v3-only and will be - // filtered out during the v3→v2→v1 chain. - let mut vsock_entry = None; - for (id, component) in &value.components { - if let Component::VirtioSocket(v) = component { - vsock_entry = Some(VirtioSocket { id: id.clone(), spec: *v }); - break; - } - } - - let v2_spec: v2::instance_spec::InstanceSpec = value.into(); - let smbios = v2_spec.smbios.clone(); - let v1_spec: v1::instance_spec::InstanceSpec = v2_spec.into(); - - let mut builder = api_spec_v0::v1_to_spec_builder(v1_spec)?; - if let Some(vsock) = vsock_entry { - builder.add_vsock_device(vsock)?; - } - let mut spec = builder.finish(); - spec.smbios_type1_input = smbios; - Ok(spec) + Ok(api_spec_v6::v6_to_spec_builder(value)?.finish()) } } @@ -114,6 +144,14 @@ pub struct ComponentTypeMismatch; /// device paths, etc.). When constructing a new spec, use the /// [`builder::SpecBuilder`] struct to catch requests that violate these /// invariants. +/// +/// ### Relationship to migration +/// +/// As Propolis' internal representation of a VM, conversion to/from `Spec` is a +/// front-and-center concern for migrating from a current Propolis to some other +/// newer or older `propolis-server`. See the module comment in +/// [`lib/migrate/mod.rs`](crate::lib::migrate) for more about the relationships +/// between these types. #[derive(Clone, Debug, Default)] pub(crate) struct Spec { pub board: Board, @@ -135,16 +173,19 @@ pub(crate) struct Spec { #[cfg(feature = "falcon")] pub softnpu: SoftNpu, - // TODO: This is an option because there is no good way to generate a - // default implementation of `SmbiosType1Input`. The default `serial_number` - // field of `SmbiosType1Input` should be equivalent to the VM UUID for - // backwards compatibility, but that isn't currently possible. + // This is an option because in v1::instance_spec::InstanceSpec the defaults + // `None` would imply came from the data outside the `InstanceSpec` itself; + // instance properties, for the instance's UUID, in particular. Two + // options here are to have `Builder` take the instance's UUID at all times + // and only sometimes synthesize `SmbiosType1Input` if nothing else is + // provided, or allow this to be `None` and interpret that in "the old way" + // when instantiating the SMBIOS tables. We've gone with the latter. + // Alternatively, we could scratch `Builder` entirely and have open-coded + // functions to translate `InstanceSpec` to a `Spec`, and have v1 of *those* + // take the requisite ancillary data. // - // One way to fix this would be to remove the `Builder` and directly - // construct `Spec` from a function that takes an `v1::instance_spec::InstanceSpec` and the - // VM UUID. This would replace `impl TryFrom for Spec`, and - // would allow removing the `Default` derive on `Spec`, and the `Option` - // from the `smbios_type1_input` field. + // If (when!) we remove `v1` types - they are wholly from before any kind of + // live migration was supported - this can be de-Option'd. pub smbios_type1_input: Option, } @@ -238,7 +279,7 @@ impl StorageDevice { } } -impl From for v1::instance_spec::Component { +impl From for latest::instance_spec::Component { fn from(value: StorageDevice) -> Self { match value { StorageDevice::Virtio(d) => Self::VirtioDisk(d), @@ -247,15 +288,13 @@ impl From for v1::instance_spec::Component { } } -impl TryFrom for StorageDevice { +impl TryFrom for StorageDevice { type Error = ComponentTypeMismatch; - fn try_from( - value: v1::instance_spec::Component, - ) -> Result { + fn try_from(value: Component) -> Result { match value { - v1::instance_spec::Component::VirtioDisk(d) => Ok(Self::Virtio(d)), - v1::instance_spec::Component::NvmeDisk(d) => Ok(Self::Nvme(d)), + Component::VirtioDisk(d) => Ok(Self::Virtio(d)), + Component::NvmeDisk(d) => Ok(Self::Nvme(d)), _ => Err(ComponentTypeMismatch), } } @@ -287,7 +326,7 @@ impl StorageBackend { } } -impl From for v1::instance_spec::Component { +impl From for Component { fn from(value: StorageBackend) -> Self { match value { StorageBackend::Crucible(be) => Self::CrucibleStorageBackend(be), @@ -297,22 +336,14 @@ impl From for v1::instance_spec::Component { } } -impl TryFrom for StorageBackend { +impl TryFrom for StorageBackend { type Error = ComponentTypeMismatch; - fn try_from( - value: v1::instance_spec::Component, - ) -> Result { + fn try_from(value: Component) -> Result { match value { - v1::instance_spec::Component::CrucibleStorageBackend(be) => { - Ok(Self::Crucible(be)) - } - v1::instance_spec::Component::FileStorageBackend(be) => { - Ok(Self::File(be)) - } - v1::instance_spec::Component::BlobStorageBackend(be) => { - Ok(Self::Blob(be)) - } + Component::CrucibleStorageBackend(be) => Ok(Self::Crucible(be)), + Component::FileStorageBackend(be) => Ok(Self::File(be)), + Component::BlobStorageBackend(be) => Ok(Self::Blob(be)), _ => Err(ComponentTypeMismatch), } } diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index f83bf8d29..9f3c98e7f 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -89,7 +89,7 @@ use propolis_api_types::instance::{ InstanceStateMonitorResponse, }; use propolis_api_types::instance_spec::{ - InstanceSpecGetResponse, InstanceSpecStatus, SpecKey, + InstanceSpec, InstanceSpecGetResponse, InstanceSpecStatus, SpecKey, }; use propolis_api_types::migration::{ InstanceMigrateStatusResponse, InstanceMigrationStatus, MigrationState, @@ -355,9 +355,10 @@ impl Vm { let spec = vm.objects().lock_shared().await.instance_spec().clone(); let state = vm.external_state_rx.borrow().clone(); + let external_spec: InstanceSpec = spec.into(); Some(InstanceSpecGetResponse { properties: vm.properties.clone(), - spec: InstanceSpecStatus::Present(spec.into()), + spec: InstanceSpecStatus::Present(external_spec), state: state.state, }) } @@ -369,13 +370,14 @@ impl Vm { spec: spec.clone().into(), }) } - VmState::Rundown { vm, spec } => Some(InstanceSpecGetResponse { - properties: vm.properties.clone(), - state: vm.external_state_rx.borrow().state, - spec: InstanceSpecStatus::Present( - spec.as_ref().to_owned().into(), - ), - }), + VmState::Rundown { vm, spec } => { + let external_spec: InstanceSpec = (*spec.to_owned()).into(); + Some(InstanceSpecGetResponse { + properties: vm.properties.clone(), + state: vm.external_state_rx.borrow().state, + spec: InstanceSpecStatus::Present(external_spec), + }) + } } } diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs index f520380dc..8ce40f064 100644 --- a/bin/propolis-standalone/src/main.rs +++ b/bin/propolis-standalone/src/main.rs @@ -1355,6 +1355,11 @@ fn setup_instance( .to_string(); let log = log.new(slog::o!("dev" => format!("nvme-{}", name))); + let has_write_cache = dev + .options + .get("has_write_cache") + .map(|v| v.as_bool().unwrap()) + .unwrap_or(false); // Limit data transfers to 1MiB (2^8 * 4k) in size let mdts = Some(8); @@ -1363,8 +1368,12 @@ fn setup_instance( serial_number[..sz] .clone_from_slice(&dev_serial.as_bytes()[..sz]); - let nvme = - hw::nvme::PciNvme::create(&serial_number, mdts, log); + let nvme = hw::nvme::PciNvme::create( + &serial_number, + mdts, + has_write_cache, + log, + ); guard.inventory.register_instance(&nvme, &bdf.to_string()); guard.inventory.register_block(&backend, name); diff --git a/crates/propolis-api-types-versions/src/add_vsock/instance_spec.rs b/crates/propolis-api-types-versions/src/add_vsock/instance_spec.rs index 08caf469d..5072232bf 100644 --- a/crates/propolis-api-types-versions/src/add_vsock/instance_spec.rs +++ b/crates/propolis-api-types-versions/src/add_vsock/instance_spec.rs @@ -112,19 +112,17 @@ impl TryFrom for V1Component { } } -impl From for v2::instance_spec::InstanceSpec { - fn from(new: InstanceSpec) -> Self { - Self { - board: new.board, - components: new - .components - .into_iter() - .filter_map(|(k, v)| { - V1Component::try_from(v).ok().map(|c| (k, c)) - }) - .collect(), - smbios: new.smbios, - } +impl TryFrom for v2::instance_spec::InstanceSpec { + type Error = InvalidV1Component; + + fn try_from(new: InstanceSpec) -> Result { + let components: BTreeMap<_, _> = new + .components + .into_iter() + .map(|(k, v)| V1Component::try_from(v).map(|c| (k, c))) + .collect::, _>>()?; + + Ok(Self { board: new.board, components, smbios: new.smbios }) } } @@ -164,26 +162,34 @@ impl From for Component { } } -impl From for v2::instance_spec::InstanceSpecStatus { - fn from(new: InstanceSpecStatus) -> Self { +impl TryFrom for v2::instance_spec::InstanceSpecStatus { + type Error = InvalidV1Component; + + fn try_from(new: InstanceSpecStatus) -> Result { match new { InstanceSpecStatus::WaitingForMigrationSource => { - Self::WaitingForMigrationSource + Ok(Self::WaitingForMigrationSource) + } + InstanceSpecStatus::Present(spec) => { + let v2_spec: v2::instance_spec::InstanceSpec = + spec.try_into()?; + Ok(Self::Present(v2_spec)) } - InstanceSpecStatus::Present(spec) => Self::Present(spec.into()), } } } -impl From +impl TryFrom for v2::instance_spec::InstanceSpecGetResponse { - fn from(new: InstanceSpecGetResponse) -> Self { - Self { + type Error = InvalidV1Component; + + fn try_from(new: InstanceSpecGetResponse) -> Result { + Ok(Self { properties: new.properties, state: new.state, - spec: new.spec.into(), - } + spec: new.spec.try_into()?, + }) } } diff --git a/crates/propolis-api-types-versions/src/latest.rs b/crates/propolis-api-types-versions/src/latest.rs index 0d9675aeb..5e64474b0 100644 --- a/crates/propolis-api-types-versions/src/latest.rs +++ b/crates/propolis-api-types-versions/src/latest.rs @@ -30,7 +30,6 @@ pub mod components { pub use crate::v1::components::devices::BootOrderEntry; pub use crate::v1::components::devices::BootSettings; pub use crate::v1::components::devices::MigrationFailureInjector; - pub use crate::v1::components::devices::NvmeDisk; pub use crate::v1::components::devices::P9fs; pub use crate::v1::components::devices::PciPciBridge; pub use crate::v1::components::devices::QemuPvpanic; @@ -43,6 +42,8 @@ pub mod components { pub use crate::v1::components::devices::VirtioNic; pub use crate::v3::components::devices::VirtioSocket; + + pub use crate::v6::components::devices::NvmeDisk; } } @@ -70,8 +71,8 @@ pub mod instance { pub use crate::v1::instance::InstanceStateRequested; pub use crate::v1::instance::ReplacementComponent; - pub use crate::v3::api::InstanceEnsureRequest; - pub use crate::v3::api::InstanceInitializationMethod; + pub use crate::v6::api::InstanceEnsureRequest; + pub use crate::v6::api::InstanceInitializationMethod; } pub mod instance_spec { @@ -84,10 +85,10 @@ pub mod instance_spec { pub use crate::v2::instance_spec::SmbiosType1Input; - pub use crate::v3::instance_spec::Component; - pub use crate::v3::instance_spec::InstanceSpec; - pub use crate::v3::instance_spec::InstanceSpecGetResponse; - pub use crate::v3::instance_spec::InstanceSpecStatus; + pub use crate::v6::instance_spec::Component; + pub use crate::v6::instance_spec::InstanceSpec; + pub use crate::v6::instance_spec::InstanceSpecGetResponse; + pub use crate::v6::instance_spec::InstanceSpecStatus; } pub mod migration { diff --git a/crates/propolis-api-types-versions/src/lib.rs b/crates/propolis-api-types-versions/src/lib.rs index 77dd5e31c..aecc1a645 100644 --- a/crates/propolis-api-types-versions/src/lib.rs +++ b/crates/propolis-api-types-versions/src/lib.rs @@ -39,3 +39,5 @@ pub mod v2; pub mod v3; #[path = "crucible_volume_info/mod.rs"] pub mod v5; +#[path = "nvme_write_cache/mod.rs"] +pub mod v6; diff --git a/crates/propolis-api-types-versions/src/nvme_write_cache/api.rs b/crates/propolis-api-types-versions/src/nvme_write_cache/api.rs new file mode 100644 index 000000000..651896b26 --- /dev/null +++ b/crates/propolis-api-types-versions/src/nvme_write_cache/api.rs @@ -0,0 +1,62 @@ +// 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/. + +//! API request and response types for the ADD_VSOCK API version. + +use std::{collections::BTreeMap, net::SocketAddr}; + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use uuid::Uuid; + +use super::instance_spec::InstanceSpec; +use crate::v1::instance::{InstanceProperties, ReplacementComponent}; +use crate::v1::instance_spec::SpecKey; +use crate::v3; + +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +#[serde(tag = "method", content = "value")] +pub enum InstanceInitializationMethod { + Spec { + spec: InstanceSpec, + }, + MigrationTarget { + migration_id: Uuid, + src_addr: SocketAddr, + replace_components: BTreeMap, + }, +} + +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct InstanceEnsureRequest { + pub properties: InstanceProperties, + pub init: InstanceInitializationMethod, +} + +impl From + for InstanceInitializationMethod +{ + fn from(old: v3::api::InstanceInitializationMethod) -> Self { + match old { + v3::api::InstanceInitializationMethod::Spec { spec } => { + Self::Spec { spec: spec.into() } + } + v3::api::InstanceInitializationMethod::MigrationTarget { + migration_id, + src_addr, + replace_components, + } => Self::MigrationTarget { + migration_id, + src_addr, + replace_components, + }, + } + } +} + +impl From for InstanceEnsureRequest { + fn from(old: v3::api::InstanceEnsureRequest) -> Self { + Self { properties: old.properties, init: old.init.into() } + } +} diff --git a/crates/propolis-api-types-versions/src/nvme_write_cache/components/devices.rs b/crates/propolis-api-types-versions/src/nvme_write_cache/components/devices.rs new file mode 100644 index 000000000..cfe04ed2a --- /dev/null +++ b/crates/propolis-api-types-versions/src/nvme_write_cache/components/devices.rs @@ -0,0 +1,65 @@ +// 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/. + +use super::super::instance_spec; +use crate::v1::components::devices::NvmeDisk as V1NvmeDisk; +use crate::v1::instance_spec::{PciPath, SpecKey}; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +/// A disk that presents an NVMe interface to the guest. +#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct NvmeDisk { + /// The name of the disk's backend component. + pub backend_id: SpecKey, + + /// The PCI bus/device/function at which this disk should be attached. + pub pci_path: PciPath, + + /// The serial number to return in response to an NVMe Identify Controller + /// command. + pub serial_number: [u8; 20], + + /// Control if the NVMe disk reports the presence of a volatile write cache. + /// + /// This generally should be configured in consideration of the storage + /// backend for the NVMe device. "true" is a safe default, and was + /// historically the only configurable value. If the storage backend will + /// not lose data once writes are accepted, even in the face of unplanned + /// crashes or power loss (or, if you really want to lie to guests), setting + /// this to "false" can advise guests they may skip issuing flushes to the + /// device. + pub has_write_cache: bool, +} + +impl TryFrom for V1NvmeDisk { + type Error = instance_spec::InvalidV3Component; + + fn try_from(disk: NvmeDisk) -> Result { + let NvmeDisk { backend_id, pci_path, serial_number, has_write_cache } = + disk; + + if !has_write_cache { + return Err(instance_spec::InvalidV3Component { + reason: + "NvmeDisk with has_write_cache=false cannot be downgraded", + }); + } + + Ok(V1NvmeDisk { backend_id, pci_path, serial_number }) + } +} + +impl From for NvmeDisk { + fn from(v1_disk: V1NvmeDisk) -> Self { + let V1NvmeDisk { backend_id, pci_path, serial_number } = v1_disk; + + // API version `nvme_write_cache` pairs with a Propolis change to make + // the former-default of NVMe devices offering `VWC=1` into a + // configurable option. So the historical default and effect of any + // previous APIs was to `has_write_cache: true`. + Self { backend_id, pci_path, serial_number, has_write_cache: true } + } +} diff --git a/crates/propolis-api-types-versions/src/nvme_write_cache/components/mod.rs b/crates/propolis-api-types-versions/src/nvme_write_cache/components/mod.rs new file mode 100644 index 000000000..0b45a47b1 --- /dev/null +++ b/crates/propolis-api-types-versions/src/nvme_write_cache/components/mod.rs @@ -0,0 +1,5 @@ +// 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/. + +pub mod devices; diff --git a/crates/propolis-api-types-versions/src/nvme_write_cache/instance_spec.rs b/crates/propolis-api-types-versions/src/nvme_write_cache/instance_spec.rs new file mode 100644 index 000000000..cfe8d491e --- /dev/null +++ b/crates/propolis-api-types-versions/src/nvme_write_cache/instance_spec.rs @@ -0,0 +1,204 @@ +// 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/. + +use std::collections::BTreeMap; + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::v1::components::backends; +use crate::v1::components::board; +use crate::v1::components::devices as v1_devices; +use crate::v1::instance::{InstanceProperties, InstanceState}; +use crate::v1::instance_spec::SpecKey; +use crate::v2::instance_spec::SmbiosType1Input; +use crate::v3; +use crate::v3::components::devices as v3_devices; +use crate::v3::instance_spec::Component as V3Component; + +pub use super::components::devices::NvmeDisk; + +#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] +#[serde( + deny_unknown_fields, + tag = "type", + content = "component", + rename_all = "snake_case" +)] +pub enum Component { + VirtioDisk(v1_devices::VirtioDisk), + NvmeDisk(NvmeDisk), + VirtioNic(v1_devices::VirtioNic), + SerialPort(v1_devices::SerialPort), + PciPciBridge(v1_devices::PciPciBridge), + QemuPvpanic(v1_devices::QemuPvpanic), + BootSettings(v1_devices::BootSettings), + VirtioSocket(v3_devices::VirtioSocket), + SoftNpuPciPort(v1_devices::SoftNpuPciPort), + SoftNpuPort(v1_devices::SoftNpuPort), + SoftNpuP9(v1_devices::SoftNpuP9), + P9fs(v1_devices::P9fs), + MigrationFailureInjector(v1_devices::MigrationFailureInjector), + CrucibleStorageBackend(backends::CrucibleStorageBackend), + FileStorageBackend(backends::FileStorageBackend), + BlobStorageBackend(backends::BlobStorageBackend), + VirtioNetworkBackend(backends::VirtioNetworkBackend), + DlpiNetworkBackend(backends::DlpiNetworkBackend), +} + +#[derive(Clone, Deserialize, Serialize, Debug, JsonSchema)] +pub struct InstanceSpec { + pub board: board::Board, + pub components: BTreeMap, + pub smbios: Option, +} + +#[derive(Clone, Deserialize, Serialize, JsonSchema)] +#[serde(tag = "type", content = "value")] +pub enum InstanceSpecStatus { + WaitingForMigrationSource, + Present(InstanceSpec), +} + +#[derive(Clone, Deserialize, Serialize, JsonSchema)] +pub struct InstanceSpecGetResponse { + pub properties: InstanceProperties, + pub state: InstanceState, + pub spec: InstanceSpecStatus, +} + +#[derive(thiserror::Error, Debug)] +#[error("cannot convert component to v3: {reason}")] +pub struct InvalidV3Component { + pub(crate) reason: &'static str, +} + +impl TryFrom for V3Component { + type Error = InvalidV3Component; + + fn try_from(value: Component) -> Result { + Ok(match value { + Component::VirtioDisk(c) => V3Component::VirtioDisk(c), + Component::NvmeDisk(c) => V3Component::NvmeDisk(c.try_into()?), + Component::VirtioNic(c) => V3Component::VirtioNic(c), + Component::SerialPort(c) => V3Component::SerialPort(c), + Component::PciPciBridge(c) => V3Component::PciPciBridge(c), + Component::QemuPvpanic(c) => V3Component::QemuPvpanic(c), + Component::BootSettings(c) => V3Component::BootSettings(c), + Component::VirtioSocket(c) => V3Component::VirtioSocket(c), + Component::SoftNpuPciPort(c) => V3Component::SoftNpuPciPort(c), + Component::SoftNpuPort(c) => V3Component::SoftNpuPort(c), + Component::SoftNpuP9(c) => V3Component::SoftNpuP9(c), + Component::P9fs(c) => V3Component::P9fs(c), + Component::MigrationFailureInjector(c) => { + V3Component::MigrationFailureInjector(c) + } + Component::CrucibleStorageBackend(c) => { + V3Component::CrucibleStorageBackend(c) + } + Component::FileStorageBackend(c) => { + V3Component::FileStorageBackend(c) + } + Component::BlobStorageBackend(c) => { + V3Component::BlobStorageBackend(c) + } + Component::VirtioNetworkBackend(c) => { + V3Component::VirtioNetworkBackend(c) + } + Component::DlpiNetworkBackend(c) => { + V3Component::DlpiNetworkBackend(c) + } + }) + } +} + +impl From for v3::instance_spec::InstanceSpec { + fn from(new: InstanceSpec) -> Self { + Self { + board: new.board, + components: new + .components + .into_iter() + .filter_map(|(k, v)| { + V3Component::try_from(v).ok().map(|c| (k, c)) + }) + .collect(), + smbios: new.smbios, + } + } +} + +impl From for Component { + fn from(old: V3Component) -> Self { + match old { + V3Component::VirtioDisk(c) => Component::VirtioDisk(c), + V3Component::VirtioSocket(c) => Component::VirtioSocket(c), + V3Component::NvmeDisk(c) => Component::NvmeDisk(c.into()), + V3Component::VirtioNic(c) => Component::VirtioNic(c), + V3Component::SerialPort(c) => Component::SerialPort(c), + V3Component::PciPciBridge(c) => Component::PciPciBridge(c), + V3Component::QemuPvpanic(c) => Component::QemuPvpanic(c), + V3Component::BootSettings(c) => Component::BootSettings(c), + V3Component::SoftNpuPciPort(c) => Component::SoftNpuPciPort(c), + V3Component::SoftNpuPort(c) => Component::SoftNpuPort(c), + V3Component::SoftNpuP9(c) => Component::SoftNpuP9(c), + V3Component::P9fs(c) => Component::P9fs(c), + V3Component::MigrationFailureInjector(c) => { + Component::MigrationFailureInjector(c) + } + V3Component::CrucibleStorageBackend(c) => { + Component::CrucibleStorageBackend(c) + } + V3Component::FileStorageBackend(c) => { + Component::FileStorageBackend(c) + } + V3Component::BlobStorageBackend(c) => { + Component::BlobStorageBackend(c) + } + V3Component::VirtioNetworkBackend(c) => { + Component::VirtioNetworkBackend(c) + } + V3Component::DlpiNetworkBackend(c) => { + Component::DlpiNetworkBackend(c) + } + } + } +} + +impl From for v3::instance_spec::InstanceSpecStatus { + fn from(new: InstanceSpecStatus) -> Self { + match new { + InstanceSpecStatus::WaitingForMigrationSource => { + Self::WaitingForMigrationSource + } + InstanceSpecStatus::Present(spec) => Self::Present(spec.into()), + } + } +} + +impl From + for v3::instance_spec::InstanceSpecGetResponse +{ + fn from(new: InstanceSpecGetResponse) -> Self { + Self { + properties: new.properties, + state: new.state, + spec: new.spec.into(), + } + } +} + +impl From for InstanceSpec { + fn from(old: v3::instance_spec::InstanceSpec) -> Self { + Self { + board: old.board, + components: old + .components + .into_iter() + .map(|(k, v)| (k, Component::from(v))) + .collect(), + smbios: old.smbios, + } + } +} diff --git a/crates/propolis-api-types-versions/src/nvme_write_cache/mod.rs b/crates/propolis-api-types-versions/src/nvme_write_cache/mod.rs new file mode 100644 index 000000000..29cf1e870 --- /dev/null +++ b/crates/propolis-api-types-versions/src/nvme_write_cache/mod.rs @@ -0,0 +1,12 @@ +// 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/. + +//! Version `NVME_WRITE_CACHE` of the Propolis Server API. +//! +//! This version adds an API field to control if an NVMe device reports having a +//! volatile write cache. + +pub mod api; +pub mod components; +pub mod instance_spec; diff --git a/crates/propolis-config-toml/src/lib.rs b/crates/propolis-config-toml/src/lib.rs index 583fb6591..b9ed33995 100644 --- a/crates/propolis-config-toml/src/lib.rs +++ b/crates/propolis-config-toml/src/lib.rs @@ -93,8 +93,15 @@ pub struct Device { } impl Device { + pub fn get_toml_value>( + &self, + key: S, + ) -> Option<&toml::Value> { + self.options.get(key.as_ref()) + } + pub fn get_string>(&self, key: S) -> Option<&str> { - self.options.get(key.as_ref())?.as_str() + self.get_toml_value(key)?.as_str() } pub fn get>(&self, key: S) -> Option { diff --git a/crates/propolis-config-toml/src/spec.rs b/crates/propolis-config-toml/src/spec.rs index 2b6371892..96b98b490 100644 --- a/crates/propolis-config-toml/src/spec.rs +++ b/crates/propolis-config-toml/src/spec.rs @@ -4,10 +4,7 @@ //! Functions for converting a [`super::Config`] into instance spec elements. -use std::{ - collections::BTreeMap, - str::{FromStr, ParseBoolError}, -}; +use std::{collections::BTreeMap, str::FromStr}; use propolis_client::{ instance_spec::{ @@ -54,8 +51,14 @@ pub enum TomlToSpecError { #[error("couldn't get path for file backend {0:?}")] InvalidFileBackendPath(String), - #[error("failed to parse read-only option for file backend {0:?}")] - FileBackendReadonlyParseFailed(String, #[source] ParseBoolError), + #[error("failed to parse option \"{field}\" for {name}: {error}")] + FieldParseError { + field: &'static str, + name: String, + // "String" is just a lowest common denominator for the different kinds + // of parse errors we might see. + error: String, + }, #[error("failed to get VNIC name for device {0:?}")] NoVnicName(String), @@ -76,7 +79,7 @@ pub struct SpecConfig { pub components: BTreeMap, } -// Inspired by `api_spec_v0.rs`'s `insert_component` and +// Inspired by `api_spec_v1.rs`'s `insert_component` and // `propolis-cli/src/main.rs`'s `add_component_to_spec`. Same purpose as both of // them. // @@ -335,11 +338,36 @@ fn parse_storage_device_from_config( Interface::Virtio => { Component::VirtioDisk(VirtioDisk { backend_id, pci_path }) } - Interface::Nvme => Component::NvmeDisk(NvmeDisk { - backend_id, - pci_path, - serial_number: nvme_serial_from_str(name, b' '), - }), + Interface::Nvme => { + let write_cache_opt = device + .get_toml_value("has_write_cache") + .map(|v: &toml::Value| { + v.as_bool().ok_or_else(|| { + TomlToSpecError::FieldParseError { + field: "has_write_cache", + name: name.to_owned(), + error: format!( + "field must be a boolean, was {:?}", + v + ), + } + }) + }) + .transpose()?; + + // Reporting a write cache when the underlying medium does not + // causes unnecessary guest work, but is not a correctness + // issue. The converse can be. Default to reporting write caches + // if we're not instructed otherwise. + let has_write_cache = write_cache_opt.unwrap_or(true); + + Component::NvmeDisk(NvmeDisk { + backend_id, + pci_path, + serial_number: nvme_serial_from_str(name, b' '), + has_write_cache, + }) + } }, id_to_return, )) @@ -366,10 +394,11 @@ fn parse_storage_backend_from_config( Some(toml::Value::Boolean(ro)) => Some(*ro), Some(toml::Value::String(v)) => { Some(v.parse::().map_err(|e| { - TomlToSpecError::FileBackendReadonlyParseFailed( - name.to_owned(), - e, - ) + TomlToSpecError::FieldParseError { + field: "readonly", + name: name.to_owned(), + error: e.to_string(), + } })?) } _ => None, diff --git a/crates/propolis-server-api/src/lib.rs b/crates/propolis-server-api/src/lib.rs index 8670ab02d..298d0d33e 100644 --- a/crates/propolis-server-api/src/lib.rs +++ b/crates/propolis-server-api/src/lib.rs @@ -3,12 +3,12 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use dropshot::{ - HttpError, HttpResponseCreated, HttpResponseOk, + ClientErrorStatusCode, HttpError, HttpResponseCreated, HttpResponseOk, HttpResponseUpdatedNoContent, Path, Query, RequestContext, TypedBody, WebsocketChannelResult, WebsocketConnection, }; use dropshot_api_manager_types::api_versions; -use propolis_api_types_versions::{latest, v1, v2}; +use propolis_api_types_versions::{latest, v1, v2, v3, v6}; api_versions!([ // WHEN CHANGING THE API (part 1 of 2): @@ -22,6 +22,7 @@ api_versions!([ // | example for the next person. // v // (next_int, IDENT), + (6, NVME_WRITE_CACHE), (5, CRUCIBLE_VOLUME_INFO), (4, DROPSHOT_BUMP_WEBSOCKET), (3, ADD_VSOCK), @@ -48,7 +49,7 @@ pub trait PropolisServerApi { #[endpoint { method = PUT, path = "/instance", - versions = VERSION_ADD_VSOCK.. + versions = VERSION_NVME_WRITE_CACHE.. }] async fn instance_ensure( rqctx: RequestContext, @@ -58,6 +59,26 @@ pub trait PropolisServerApi { HttpError, >; + #[endpoint { + operation_id = "instance_ensure", + method = PUT, + path = "/instance", + versions = VERSION_ADD_VSOCK..VERSION_NVME_WRITE_CACHE + }] + async fn instance_ensure_v3( + rqctx: RequestContext, + request: TypedBody, + ) -> Result< + HttpResponseCreated, + HttpError, + > { + Self::instance_ensure( + rqctx, + request.map(v6::api::InstanceEnsureRequest::from), + ) + .await + } + #[endpoint { operation_id = "instance_ensure", method = PUT, @@ -68,12 +89,12 @@ pub trait PropolisServerApi { rqctx: RequestContext, request: TypedBody, ) -> Result< - HttpResponseCreated, + HttpResponseCreated, HttpError, > { - Self::instance_ensure( + Self::instance_ensure_v3( rqctx, - request.map(latest::instance::InstanceEnsureRequest::from), + request.map(v3::api::InstanceEnsureRequest::from), ) .await } @@ -88,7 +109,7 @@ pub trait PropolisServerApi { rqctx: RequestContext, request: TypedBody, ) -> Result< - HttpResponseCreated, + HttpResponseCreated, HttpError, > { Self::instance_ensure_v2( @@ -101,7 +122,7 @@ pub trait PropolisServerApi { #[endpoint { method = GET, path = "/instance/spec", - versions = VERSION_ADD_VSOCK.. + versions = VERSION_NVME_WRITE_CACHE.. }] async fn instance_spec_get( rqctx: RequestContext, @@ -110,6 +131,23 @@ pub trait PropolisServerApi { HttpError, >; + #[endpoint { + operation_id = "instance_spec_get", + method = GET, + path = "/instance/spec", + versions = VERSION_ADD_VSOCK..VERSION_NVME_WRITE_CACHE + }] + async fn instance_spec_get_v3( + rqctx: RequestContext, + ) -> Result< + HttpResponseOk, + HttpError, + > { + Ok(Self::instance_spec_get(rqctx) + .await? + .map(v3::instance_spec::InstanceSpecGetResponse::from)) + } + #[endpoint { operation_id = "instance_spec_get", method = GET, @@ -122,9 +160,17 @@ pub trait PropolisServerApi { HttpResponseOk, HttpError, > { - Ok(Self::instance_spec_get(rqctx) - .await? - .map(v2::instance_spec::InstanceSpecGetResponse::from)) + let v3_response = Self::instance_spec_get_v3(rqctx).await?.0; + let v2_response: v2::instance_spec::InstanceSpecGetResponse = + v3_response.try_into().map_err(|_e| { + HttpError::for_client_error( + None, + ClientErrorStatusCode::BAD_REQUEST, + "instance spec cannot be expressed to v2 clients" + .to_owned(), + ) + })?; + Ok(HttpResponseOk(v2_response)) } #[endpoint { diff --git a/lib/propolis/src/hw/nvme/mod.rs b/lib/propolis/src/hw/nvme/mod.rs index 7f8f639b1..a9360383f 100644 --- a/lib/propolis/src/hw/nvme/mod.rs +++ b/lib/propolis/src/hw/nvme/mod.rs @@ -147,6 +147,13 @@ struct CtrlState { /// Controller Capabilities cap: Capabilities, + /// Version + /// + /// A bare u32 as we expect to support specific NVMe major/minor versions + /// if/when we move beyond simply 1.0. The field is constant after the + /// controller is instantiated and returned to the guest in one read. + vs: u32, + /// Controller Configuration cc: Configuration, @@ -811,6 +818,7 @@ impl PciNvme { pub fn create( serial_number: &[u8; 20], mdts: Option, + has_write_cache: bool, log: slog::Logger, ) -> Arc { let builder = pci::Builder::new(pci::Ident { @@ -831,6 +839,24 @@ impl PciNvme { let cqes = size_of::().trailing_zeros() as u8; let sqes = size_of::().trailing_zeros() as u8; + // The controller's supported NVMe version is not (yet) configurable, + // but making it available to ourselves is useful to ensure we handle + // new versions correctly. + let vs = NVME_VER_1_0; + + // In NVMe 1.0e, VWC defines only bit 0, the presence of a volatile + // write cache. + let vwc = if has_write_cache { + // XXX: Later NVMe revisions define upper bits and only allow bits + // 1-2 to be zero in controllers implementing older versions of + // NVMe. This will need more precise handling when the supported + // NVMe version is configurable. + assert!(vs == NVME_VER_1_0); + 1 + } else { + 0 + }; + // Initialize the Identify structure returned when the host issues // an Identify Controller command. let ctrl_ident = bits::IdentifyController { @@ -839,8 +865,8 @@ impl PciNvme { sn: *serial_number, ieee: OXIDE_OUI, mdts: mdts.unwrap_or(0), - // We use standard Completion/Submission Queue Entry structures with no extra - // data, so required (minimum) == maximum + // We use standard Completion/Submission Queue Entry structures with + // no extra data, so required (minimum) == maximum sqes: NvmQueueEntrySize(0).with_maximum(sqes).with_required(sqes), cqes: NvmQueueEntrySize(0).with_maximum(cqes).with_required(cqes), // Supporting multiple namespaces complicates I/O dispatching, @@ -848,8 +874,7 @@ impl PciNvme { nn: 1, // bit 2 indicates support for the Dataset Management command oncs: (1 << 2), - // bit 0 indicates volatile write cache is present - vwc: 1, + vwc, // bit 8 indicates Doorbell Buffer support oacs: (1 << 8), ..Default::default() @@ -898,7 +923,7 @@ impl PciNvme { let state = NvmeCtrl { device_id: DeviceId::new(), - ctrl: CtrlState { cap, cc, csts, ..Default::default() }, + ctrl: CtrlState { cap, vs, cc, csts, ..Default::default() }, doorbell_buf: None, msix_hdl: None, cqs: Default::default(), @@ -1001,7 +1026,8 @@ impl PciNvme { ro.write_u64(state.ctrl.cap.0); } CtrlrReg::Version => { - ro.write_u32(NVME_VER_1_0); + let state = self.state.lock().unwrap(); + ro.write_u32(state.ctrl.vs); } CtrlrReg::IntrMaskSet | CtrlrReg::IntrMaskClear => { diff --git a/openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json.gitstub b/openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json.gitstub new file mode 100644 index 000000000..2c4d4cfc8 --- /dev/null +++ b/openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json.gitstub @@ -0,0 +1 @@ +9343361f922d10c5825906a69c7bb05d139a3662:openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json diff --git a/openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json b/openapi/propolis-server/propolis-server-6.0.0-b5b984.json similarity index 99% rename from openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json rename to openapi/propolis-server/propolis-server-6.0.0-b5b984.json index 94d0a20d8..1bfc621c4 100644 --- a/openapi/propolis-server/propolis-server-5.0.0-0c6dd9.json +++ b/openapi/propolis-server/propolis-server-6.0.0-b5b984.json @@ -7,7 +7,7 @@ "url": "https://oxide.computer", "email": "api@oxide.computer" }, - "version": "5.0.0" + "version": "6.0.0" }, "paths": { "/instance": { @@ -1757,6 +1757,10 @@ } ] }, + "has_write_cache": { + "description": "Control if the NVMe disk reports the presence of a volatile write cache.\n\nThis generally should be configured in consideration of the storage backend for the NVMe device. \"true\" is a safe default, and was historically the only configurable value. If the storage backend will not lose data once writes are accepted, even in the face of unplanned crashes or power loss (or, if you really want to lie to guests), setting this to \"false\" can advise guests they may skip issuing flushes to the device.", + "type": "boolean" + }, "pci_path": { "description": "The PCI bus/device/function at which this disk should be attached.", "allOf": [ @@ -1779,6 +1783,7 @@ }, "required": [ "backend_id", + "has_write_cache", "pci_path", "serial_number" ], diff --git a/openapi/propolis-server/propolis-server-latest.json b/openapi/propolis-server/propolis-server-latest.json index 452ab4db5..114ec385b 120000 --- a/openapi/propolis-server/propolis-server-latest.json +++ b/openapi/propolis-server/propolis-server-latest.json @@ -1 +1 @@ -propolis-server-5.0.0-0c6dd9.json \ No newline at end of file +propolis-server-6.0.0-b5b984.json \ No newline at end of file diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index f65643c80..887f17244 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -27,7 +27,21 @@ use crate::{ #[derive(Clone, Copy, Debug)] pub enum DiskInterface { Virtio, - Nvme, + Nvme { has_write_cache: bool }, +} + +impl DiskInterface { + pub fn virtio() -> Self { + DiskInterface::Virtio + } + + pub fn nvme() -> Self { + // Default to reporting a write cache for the same reason as + // propolis-cli. Some tests want to see that we can actually tell a + // guest that there's no write cache, though, so it's configurable and + // may lie with respect to backend's actual cachefulness. + DiskInterface::Nvme { has_write_cache: true } + } } #[derive(Clone, Copy, Debug)] @@ -82,7 +96,7 @@ impl<'dr> VmConfig<'dr> { config.boot_disk( guest_artifact, - DiskInterface::Nvme, + DiskInterface::nvme(), DiskBackend::File, 4, ); @@ -330,21 +344,24 @@ impl<'dr> VmConfig<'dr> { ), pci_path, }), - DiskInterface::Nvme => Component::NvmeDisk(NvmeDisk { - backend_id: SpecKey::Name( - backend_name.clone().into_string(), - ), - pci_path, - serial_number: nvme_serial_from_str( - device_name.as_str(), - // Omicron supplies (or will supply, as of this writing) - // 0 as the padding byte to maintain compatibility for - // existing disks. Match that behavior here so that PHD - // and Omicron VM configurations are as similar as - // possible. - 0, - ), - }), + DiskInterface::Nvme { has_write_cache } => { + Component::NvmeDisk(NvmeDisk { + backend_id: SpecKey::Name( + backend_name.clone().into_string(), + ), + pci_path, + serial_number: nvme_serial_from_str( + device_name.as_str(), + // Omicron supplies (or will supply, as of this writing) + // 0 as the padding byte to maintain compatibility for + // existing disks. Match that behavior here so that PHD + // and Omicron VM configurations are as similar as + // possible. + 0, + ), + has_write_cache, + }) + } }; let _old = spec diff --git a/phd-tests/tests/src/boot_order.rs b/phd-tests/tests/src/boot_order.rs index 6ae79ff0a..e2e1f1c3c 100644 --- a/phd-tests/tests/src/boot_order.rs +++ b/phd-tests/tests/src/boot_order.rs @@ -59,7 +59,7 @@ async fn configurable_boot_order(ctx: &TestCtx) { cfg.data_disk( "alt-boot", DiskSource::Artifact(ctx.default_guest_os_artifact()), - DiskInterface::Virtio, + DiskInterface::virtio(), DiskBackend::File, 24, ); @@ -121,7 +121,7 @@ async fn unbootable_disk_skipped(ctx: &TestCtx) { cfg.data_disk( "unbootable", DiskSource::FatFilesystem(FatFilesystem::new()), - DiskInterface::Virtio, + DiskInterface::virtio(), DiskBackend::InMemory { readonly: true }, 16, ); @@ -240,7 +240,7 @@ async fn guest_can_adjust_boot_order(ctx: &TestCtx) { cfg.data_disk( "unbootable", DiskSource::FatFilesystem(FatFilesystem::new()), - DiskInterface::Virtio, + DiskInterface::virtio(), DiskBackend::InMemory { readonly: true }, 16, ); @@ -407,7 +407,7 @@ async fn boot_order_source_priority(ctx: &TestCtx) { cfg.data_disk( "unbootable", DiskSource::FatFilesystem(FatFilesystem::new()), - DiskInterface::Virtio, + DiskInterface::virtio(), DiskBackend::InMemory { readonly: true }, 16, ); @@ -415,7 +415,7 @@ async fn boot_order_source_priority(ctx: &TestCtx) { cfg.data_disk( "unbootable-2", DiskSource::FatFilesystem(FatFilesystem::new()), - DiskInterface::Virtio, + DiskInterface::virtio(), DiskBackend::InMemory { readonly: true }, 20, ); @@ -514,7 +514,7 @@ async fn nvme_boot_option_description(ctx: &TestCtx) { cfg.data_disk( "nvme-test-disk", DiskSource::Artifact(ctx.default_guest_os_artifact()), - DiskInterface::Nvme, + DiskInterface::nvme(), DiskBackend::File, 8, ); diff --git a/phd-tests/tests/src/crucible/mod.rs b/phd-tests/tests/src/crucible/mod.rs index ce6ba856e..9e84b0940 100644 --- a/phd-tests/tests/src/crucible/mod.rs +++ b/phd-tests/tests/src/crucible/mod.rs @@ -44,7 +44,7 @@ fn add_default_boot_disk<'a>( ctx, config, ctx.default_guest_os_artifact(), - DiskInterface::Nvme, + DiskInterface::nvme(), 4, 10, BlockSize::Bytes512, diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs index 815e2782c..fc535b460 100644 --- a/phd-tests/tests/src/crucible/smoke.rs +++ b/phd-tests/tests/src/crucible/smoke.rs @@ -105,7 +105,7 @@ async fn vcr_replace_during_start_test(ctx: &TestCtx) { config.data_disk( DATA_DISK_NAME, DiskSource::Blank(1024 * 1024 * 1024), - DiskInterface::Nvme, + DiskInterface::nvme(), DiskBackend::Crucible { min_disk_size_gib: 1, block_size: BlockSize::Bytes512, diff --git a/phd-tests/tests/src/server_state_machine.rs b/phd-tests/tests/src/server_state_machine.rs index 9b9421610..dba7006ca 100644 --- a/phd-tests/tests/src/server_state_machine.rs +++ b/phd-tests/tests/src/server_state_machine.rs @@ -127,7 +127,7 @@ async fn stop_while_blocked_on_start_test(ctx: &TestCtx) { config.data_disk( DATA_DISK_NAME, DiskSource::Blank(1024 * 1024 * 1024), - DiskInterface::Nvme, + DiskInterface::nvme(), DiskBackend::Crucible { min_disk_size_gib: 1, block_size: BlockSize::Bytes512,