From 42b87359b4511476f741e8e9484a8f42c9246125 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 26 Feb 2025 16:18:29 -0800 Subject: [PATCH] phd: add smoke test for VCR replacement (#872) Add a smoke test for Crucible VCR replacement: - Add a `CrucibleDisk` function to get a disk's current VCR. - Add a `TestVm` framework to send a VCR replacement request. - Fix a couple of bugs in Crucible disk setup: - The `id` field in the VCR's `CrucibleOpts` needs to be the same in the old and new VCRs, so use the disk ID for it instead of calling `Uuid::new_v4()` every time the VCR is generated. - When using a `Blank` disk source, properly account for the fact that the disk source size is given in bytes, not gibibytes. Also add a couple of bells and whistles to allow this test to be transformed into a test of VCR replacement during VM startup: - Make PHD's `VmSpec` type a public type and amend `Framework` to allow tests to create a VM from a spec. This gives tests a way to access a config's Crucible disks before actually launching a VM (and sending an instance spec to Propolis). - Reorganize the `CrucibleDisk` types to wrap the disk innards in a `Mutex`, allowing them to be mutated through the `Arc` references that tests get. This will eventually be used to allow tests to override the downstairs addresses in a disk's VCRs before launching a VM that uses that disk, which will be used to test #841. In the meantime, use the mutex to protect the VCR generation number, which no longer needs to be an `AtomicU64`. --- phd-tests/framework/src/disk/crucible.rs | 126 ++++++++++++++-------- phd-tests/framework/src/disk/mod.rs | 11 +- phd-tests/framework/src/lib.rs | 21 +++- phd-tests/framework/src/test_vm/config.rs | 2 +- phd-tests/framework/src/test_vm/mod.rs | 34 ++++++ phd-tests/framework/src/test_vm/spec.rs | 9 ++ phd-tests/tests/src/crucible/smoke.rs | 37 +++++++ 7 files changed, 189 insertions(+), 51 deletions(-) diff --git a/phd-tests/framework/src/disk/crucible.rs b/phd-tests/framework/src/disk/crucible.rs index 36fec53c7..5ac2f2747 100644 --- a/phd-tests/framework/src/disk/crucible.rs +++ b/phd-tests/framework/src/disk/crucible.rs @@ -8,7 +8,7 @@ use std::{ net::{Ipv4Addr, SocketAddr, SocketAddrV4}, path::{Path, PathBuf}, process::Stdio, - sync::atomic::{AtomicU64, Ordering}, + sync::Mutex, }; use anyhow::Context; @@ -62,12 +62,73 @@ impl Drop for Downstairs { /// An RAII wrapper around a Crucible disk. #[derive(Debug)] pub struct CrucibleDisk { - /// The name to use in instance specs that include this disk. device_name: DeviceName, + disk_id: Uuid, + guest_os: Option, + inner: Mutex, +} + +impl CrucibleDisk { + #[allow(clippy::too_many_arguments)] + pub(crate) fn new( + device_name: DeviceName, + min_disk_size_gib: u64, + block_size: BlockSize, + downstairs_binary_path: &impl AsRef, + downstairs_ports: &[u16], + data_dir_root: &impl AsRef, + read_only_parent: Option<&impl AsRef>, + guest_os: Option, + log_mode: ServerLogMode, + ) -> anyhow::Result { + Ok(Self { + device_name, + disk_id: Uuid::new_v4(), + guest_os, + inner: Mutex::new(Inner::new( + min_disk_size_gib, + block_size, + downstairs_binary_path, + downstairs_ports, + data_dir_root, + read_only_parent, + log_mode, + )?), + }) + } - /// The UUID to insert into this disk's `VolumeConstructionRequest`s. - id: Uuid, + /// Obtains the current volume construction request for this disk. + pub fn vcr(&self) -> VolumeConstructionRequest { + self.inner.lock().unwrap().vcr(self.disk_id) + } + /// Sets the generation number to use in subsequent calls to create a + /// backend spec for this disk. + pub fn set_generation(&self, generation: u64) { + self.inner.lock().unwrap().generation = generation; + } +} + +impl super::DiskConfig for CrucibleDisk { + fn device_name(&self) -> &DeviceName { + &self.device_name + } + + fn backend_spec(&self) -> ComponentV0 { + self.inner.lock().unwrap().backend_spec(self.disk_id) + } + + fn guest_os(&self) -> Option { + self.guest_os + } + + fn as_crucible(&self) -> Option<&CrucibleDisk> { + Some(self) + } +} + +#[derive(Debug)] +struct Inner { /// The disk's block size. block_size: BlockSize, @@ -83,30 +144,25 @@ pub struct CrucibleDisk { /// An optional path to a file to use as a read-only parent for this disk. read_only_parent: Option, - /// The kind of guest OS that can be found on this disk, if there is one. - guest_os: Option, - /// The base64-encoded encryption key to use for this disk. encryption_key: String, /// The generation number to insert into this disk's /// `VolumeConstructionRequest`s. - generation: AtomicU64, + generation: u64, } -impl CrucibleDisk { +impl Inner { /// Constructs a new Crucible disk that stores its files in the supplied /// `data_dir`. #[allow(clippy::too_many_arguments)] pub(crate) fn new( - device_name: DeviceName, min_disk_size_gib: u64, block_size: BlockSize, downstairs_binary_path: &impl AsRef, downstairs_ports: &[u16], data_dir_root: &impl AsRef, read_only_parent: Option<&impl AsRef>, - guest_os: Option, log_mode: ServerLogMode, ) -> anyhow::Result { // To create a region, Crucible requires a block size, an extent size @@ -253,8 +309,6 @@ impl CrucibleDisk { } Ok(Self { - device_name, - id: disk_uuid, block_size, blocks_per_extent, extent_count: extents_in_disk as u32, @@ -269,37 +323,33 @@ impl CrucibleDisk { bytes }, ), - guest_os, - generation: AtomicU64::new(1), + generation: 1, }) } - /// Sets the generation number to use in subsequent calls to create a - /// backend spec for this disk. - pub fn set_generation(&self, gen: u64) { - self.generation.store(gen, Ordering::Relaxed); - } -} + fn backend_spec(&self, disk_id: Uuid) -> ComponentV0 { + let vcr = self.vcr(disk_id); -impl super::DiskConfig for CrucibleDisk { - fn device_name(&self) -> &DeviceName { - &self.device_name + ComponentV0::CrucibleStorageBackend(CrucibleStorageBackend { + request_json: serde_json::to_string(&vcr) + .expect("VolumeConstructionRequest should serialize"), + readonly: false, + }) } - fn backend_spec(&self) -> ComponentV0 { - let gen = self.generation.load(Ordering::Relaxed); + fn vcr(&self, disk_id: Uuid) -> VolumeConstructionRequest { let downstairs_addrs = self.downstairs_instances.iter().map(|ds| ds.address).collect(); - let vcr = VolumeConstructionRequest::Volume { - id: self.id, + VolumeConstructionRequest::Volume { + id: disk_id, block_size: self.block_size.bytes(), sub_volumes: vec![VolumeConstructionRequest::Region { block_size: self.block_size.bytes(), blocks_per_extent: self.blocks_per_extent, extent_count: self.extent_count, opts: CrucibleOpts { - id: Uuid::new_v4(), + id: disk_id, target: downstairs_addrs, lossy: false, flush_timeout: None, @@ -310,7 +360,7 @@ impl super::DiskConfig for CrucibleDisk { control: None, read_only: false, }, - gen, + r#gen: self.generation, }], read_only_parent: self.read_only_parent.as_ref().map(|p| { Box::new(VolumeConstructionRequest::File { @@ -319,21 +369,7 @@ impl super::DiskConfig for CrucibleDisk { path: p.to_string_lossy().to_string(), }) }), - }; - - ComponentV0::CrucibleStorageBackend(CrucibleStorageBackend { - request_json: serde_json::to_string(&vcr) - .expect("VolumeConstructionRequest should serialize"), - readonly: false, - }) - } - - fn guest_os(&self) -> Option { - self.guest_os - } - - fn as_crucible(&self) -> Option<&CrucibleDisk> { - Some(self) + } } } diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index 5d91e74cf..0fbb542c4 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/phd-tests/framework/src/disk/mod.rs @@ -279,6 +279,8 @@ impl DiskFactory { mut min_disk_size_gib: u64, block_size: BlockSize, ) -> Result, DiskError> { + const BYTES_PER_GIB: u64 = 1024 * 1024 * 1024; + let binary_path = self.artifact_store.get_crucible_downstairs().await?; let (artifact_path, guest_os) = match source { @@ -287,8 +289,13 @@ impl DiskFactory { (Some(path), Some(os)) } DiskSource::Blank(size) => { - min_disk_size_gib = min_disk_size_gib - .max(u64::try_from(*size).map_err(anyhow::Error::from)?); + let blank_size = + u64::try_from(*size).map_err(anyhow::Error::from)?; + + let min_disk_size_b = + (min_disk_size_gib * BYTES_PER_GIB).max(blank_size); + + min_disk_size_gib = min_disk_size_b.div_ceil(BYTES_PER_GIB); (None, None) } // It's possible in theory to have a Crucible-backed disk with diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index 912ff397c..327b1804b 100644 --- a/phd-tests/framework/src/lib.rs +++ b/phd-tests/framework/src/lib.rs @@ -252,12 +252,27 @@ impl Framework { config: &VmConfig<'_>, environment: Option<&EnvironmentSpec>, ) -> anyhow::Result { - TestVm::new( - self, + self.spawn_vm_with_spec( config .vm_spec(self) .await - .context("building VM config for test VM")?, + .context("building VM spec from VmConfig")?, + environment, + ) + .await + } + + /// Spawns a new test VM using the supplied `spec`. If `environment` is + /// `Some`, the VM is spawned using the supplied environment; otherwise it + /// is spawned using the default `environment_builder`. + pub async fn spawn_vm_with_spec( + &self, + spec: VmSpec, + environment: Option<&EnvironmentSpec>, + ) -> anyhow::Result { + TestVm::new( + self, + spec, environment.unwrap_or(&self.environment_builder()), ) .await diff --git a/phd-tests/framework/src/test_vm/config.rs b/phd-tests/framework/src/test_vm/config.rs index cb586fd8a..06a0c7c2d 100644 --- a/phd-tests/framework/src/test_vm/config.rs +++ b/phd-tests/framework/src/test_vm/config.rs @@ -208,7 +208,7 @@ impl<'dr> VmConfig<'dr> { self } - pub(crate) async fn vm_spec( + pub async fn vm_spec( &self, framework: &Framework, ) -> anyhow::Result { diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index caa17360e..88fcfb2d9 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -11,6 +11,7 @@ use std::{ }; use crate::{ + disk::{crucible::CrucibleDisk, DiskConfig}, guest_os::{ self, windows::WindowsVm, CommandSequence, CommandSequenceEntry, GuestOs, GuestOsKind, @@ -603,6 +604,39 @@ impl TestVm { Ok(self.client.instance_migrate_status().send().await?.into_inner()) } + pub async fn replace_crucible_vcr( + &self, + disk: &CrucibleDisk, + ) -> anyhow::Result<()> { + let vcr = disk.vcr(); + let body = propolis_client::types::InstanceVcrReplace { + vcr_json: serde_json::to_string(&vcr) + .with_context(|| format!("serializing VCR {vcr:?}"))?, + }; + + info!( + disk_name = disk.device_name().as_str(), + vcr = ?vcr, + "issuing Crucible VCR replacement request" + ); + + let response_value = self + .client + .instance_issue_crucible_vcr_request() + .id(disk.device_name().clone().into_backend_name().into_string()) + .body(body) + .send() + .await?; + + anyhow::ensure!( + response_value.status().is_success(), + "VCR replacement request returned an error value: \ + {response_value:?}" + ); + + Ok(()) + } + pub async fn get_serial_console_history( &self, from_start: u64, diff --git a/phd-tests/framework/src/test_vm/spec.rs b/phd-tests/framework/src/test_vm/spec.rs index 573537c08..970e4424d 100644 --- a/phd-tests/framework/src/test_vm/spec.rs +++ b/phd-tests/framework/src/test_vm/spec.rs @@ -34,6 +34,15 @@ pub struct VmSpec { } impl VmSpec { + pub fn get_disk_by_device_name( + &self, + name: &str, + ) -> Option<&Arc> { + self.disk_handles + .iter() + .find(|disk| disk.device_name().as_str() == name) + } + /// Update the Crucible backend specs in the instance spec to match the /// current backend specs given by this specification's disk handles. pub(crate) fn refresh_crucible_backends(&mut self) { diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs index a27134579..fd34e34b8 100644 --- a/phd-tests/tests/src/crucible/smoke.rs +++ b/phd-tests/tests/src/crucible/smoke.rs @@ -4,6 +4,10 @@ use std::time::Duration; +use phd_framework::{ + disk::{BlockSize, DiskSource}, + test_vm::{DiskBackend, DiskInterface}, +}; use phd_testcase::*; use propolis_client::types::InstanceState; @@ -82,3 +86,36 @@ async fn shutdown_persistence_test(ctx: &Framework) { let lsout = vm.run_shell_command("ls foo.bar 2> /dev/null").await?; assert_eq!(lsout, "foo.bar"); } + +#[phd_testcase] +async fn vcr_replace_test(ctx: &Framework) { + let mut config = ctx.vm_config_builder("crucible_vcr_replace_test"); + + // Create a blank data disk on which to perform VCR replacement. This is + // necessary because Crucible doesn't permit VCR replacements for volumes + // whose read-only parents are local files (which is true for artifact-based + // Crucible disks). + const DATA_DISK_NAME: &str = "vcr-replacement-target"; + config.data_disk( + DATA_DISK_NAME, + DiskSource::Blank(1024 * 1024 * 1024), + DiskInterface::Nvme, + DiskBackend::Crucible { + min_disk_size_gib: 1, + block_size: BlockSize::Bytes512, + }, + 5, + ); + + let spec = config.vm_spec(ctx).await?; + let disk_hdl = + spec.get_disk_by_device_name(DATA_DISK_NAME).cloned().unwrap(); + let disk = disk_hdl.as_crucible().unwrap(); + + let mut vm = ctx.spawn_vm_with_spec(spec, None).await?; + vm.launch().await?; + vm.wait_to_boot().await?; + + disk.set_generation(2); + vm.replace_crucible_vcr(disk).await?; +}