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`.

Author: gjcolombo

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<GuestOsKind>,
+    inner: Mutex<Inner>,
+}
+
+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<std::ffi::OsStr>,
+        downstairs_ports: &[u16],
+        data_dir_root: &impl AsRef<Path>,
+        read_only_parent: Option<&impl AsRef<Path>>,
+        guest_os: Option<GuestOsKind>,
+        log_mode: ServerLogMode,
+    ) -> anyhow::Result<Self> {
+        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<GuestOsKind> {
+        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<PathBuf>,
 
-    /// The kind of guest OS that can be found on this disk, if there is one.
-    guest_os: Option<GuestOsKind>,
-
     /// 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<std::ffi::OsStr>,
         downstairs_ports: &[u16],
         data_dir_root: &impl AsRef<Path>,
         read_only_parent: Option<&impl AsRef<Path>>,
-        guest_os: Option<GuestOsKind>,
         log_mode: ServerLogMode,
     ) -> anyhow::Result<Self> {
         // 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<GuestOsKind> {
-        self.guest_os
-    }
-
-    fn as_crucible(&self) -> Option<&CrucibleDisk> {
-        Some(self)
+        }
     }
 }
 

phd-tests/framework/src/disk/mod.rs

@@ -279,6 +279,8 @@ impl DiskFactory {
         mut min_disk_size_gib: u64,
         block_size: BlockSize,
     ) -> Result<Arc<CrucibleDisk>, 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

phd-tests/framework/src/lib.rs

@@ -252,12 +252,27 @@ impl Framework {
         config: &VmConfig<'_>,
         environment: Option<&EnvironmentSpec>,
     ) -> anyhow::Result<TestVm> {
-        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> {
+        TestVm::new(
+            self,
+            spec,
             environment.unwrap_or(&self.environment_builder()),
         )
         .await

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<VmSpec> {

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,

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<dyn disk::DiskConfig>> {
+        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) {

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?;
+}