From a88cae977148d1a56540241e4a8b2c7b9d3ac4ce Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Mon, 24 Feb 2025 20:56:28 +0000
Subject: [PATCH 1/4] phd: smoke test VCR replacement
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 | 110 +++++++++++++++-------
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 | 28 ++++++
phd-tests/framework/src/test_vm/spec.rs | 9 ++
phd-tests/tests/src/crucible/smoke.rs | 35 +++++++
7 files changed, 178 insertions(+), 38 deletions(-)
diff --git a/phd-tests/framework/src/disk/crucible.rs b/phd-tests/framework/src/disk/crucible.rs
index 36fec53c7..4df13f903 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,9 +62,70 @@ 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,
+ 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,
+ inner: Mutex::new(Inner::new(
+ min_disk_size_gib,
+ block_size,
+ downstairs_binary_path,
+ downstairs_ports,
+ data_dir_root,
+ read_only_parent,
+ guest_os,
+ log_mode,
+ )?),
+ })
+ }
+
+ /// Obtains the current volume construction request for this disk.
+ pub fn vcr(&self) -> VolumeConstructionRequest {
+ self.inner.lock().unwrap().vcr()
+ }
+
+ /// 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()
+ }
+
+ fn guest_os(&self) -> Option<GuestOsKind> {
+ self.inner.lock().unwrap().guest_os()
+ }
+
+ fn as_crucible(&self) -> Option<&CrucibleDisk> {
+ Some(self)
+ }
+}
+#[derive(Debug)]
+struct Inner {
/// The UUID to insert into this disk's `VolumeConstructionRequest`s.
id: Uuid,
@@ -91,15 +152,14 @@ pub struct CrucibleDisk {
/// 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>,
@@ -253,7 +313,6 @@ impl CrucibleDisk {
}
Ok(Self {
- device_name,
id: disk_uuid,
block_size,
blocks_per_extent,
@@ -270,28 +329,25 @@ impl CrucibleDisk {
},
),
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) -> ComponentV0 {
+ let vcr = self.vcr();
-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) -> VolumeConstructionRequest {
let downstairs_addrs =
self.downstairs_instances.iter().map(|ds| ds.address).collect();
- let vcr = VolumeConstructionRequest::Volume {
+ VolumeConstructionRequest::Volume {
id: self.id,
block_size: self.block_size.bytes(),
sub_volumes: vec![VolumeConstructionRequest::Region {
@@ -299,7 +355,7 @@ impl super::DiskConfig for CrucibleDisk {
blocks_per_extent: self.blocks_per_extent,
extent_count: self.extent_count,
opts: CrucibleOpts {
- id: Uuid::new_v4(),
+ id: self.id,
target: downstairs_addrs,
lossy: false,
flush_timeout: None,
@@ -310,7 +366,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,22 +375,12 @@ 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)
- }
}
fn run_crucible_downstairs(
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<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
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> {
- 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
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<VmSpec> {
diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs
index caa17360e..08b57e889 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,33 @@ 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:?}"))?,
+ };
+
+ 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..6a30c3c9d 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 {
+ /// Yields an array of handles to this VM's Crucible disks.
+ pub fn get_crucible_disks(&self) -> Vec<Arc<dyn disk::DiskConfig>> {
+ self.disk_handles
+ .iter()
+ .filter(|d| d.as_crucible().is_some())
+ .cloned()
+ .collect()
+ }
+
/// 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..c2157475d 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,34 @@ 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).
+ config.data_disk(
+ "vcr-replacement-target",
+ 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 disks = spec.get_crucible_disks();
+ let disk = disks[0].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?;
+}
From 23e73f2752fc5291fe2683c66df50905b4375be9 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Tue, 25 Feb 2025 21:09:26 +0000
Subject: [PATCH 2/4] phd: better method for referencing a disk from a VmSpec
---
phd-tests/framework/src/test_vm/spec.rs | 10 +++++-----
phd-tests/tests/src/crucible/smoke.rs | 8 +++++---
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/phd-tests/framework/src/test_vm/spec.rs b/phd-tests/framework/src/test_vm/spec.rs
index 6a30c3c9d..970e4424d 100644
--- a/phd-tests/framework/src/test_vm/spec.rs
+++ b/phd-tests/framework/src/test_vm/spec.rs
@@ -34,13 +34,13 @@ pub struct VmSpec {
}
impl VmSpec {
- /// Yields an array of handles to this VM's Crucible disks.
- pub fn get_crucible_disks(&self) -> Vec<Arc<dyn disk::DiskConfig>> {
+ pub fn get_disk_by_device_name(
+ &self,
+ name: &str,
+ ) -> Option<&Arc<dyn disk::DiskConfig>> {
self.disk_handles
.iter()
- .filter(|d| d.as_crucible().is_some())
- .cloned()
- .collect()
+ .find(|disk| disk.device_name().as_str() == name)
}
/// Update the Crucible backend specs in the instance spec to match the
diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs
index c2157475d..fd34e34b8 100644
--- a/phd-tests/tests/src/crucible/smoke.rs
+++ b/phd-tests/tests/src/crucible/smoke.rs
@@ -95,8 +95,9 @@ async fn vcr_replace_test(ctx: &Framework) {
// 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(
- "vcr-replacement-target",
+ DATA_DISK_NAME,
DiskSource::Blank(1024 * 1024 * 1024),
DiskInterface::Nvme,
DiskBackend::Crucible {
@@ -107,8 +108,9 @@ async fn vcr_replace_test(ctx: &Framework) {
);
let spec = config.vm_spec(ctx).await?;
- let disks = spec.get_crucible_disks();
- let disk = disks[0].as_crucible().unwrap();
+ 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?;
From fa04d6f606e0b01381b088cdb7ebb93e470e14b2 Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Tue, 25 Feb 2025 21:14:22 +0000
Subject: [PATCH 3/4] phd: improve logging
---
phd-tests/framework/src/test_vm/mod.rs | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs
index 08b57e889..88fcfb2d9 100644
--- a/phd-tests/framework/src/test_vm/mod.rs
+++ b/phd-tests/framework/src/test_vm/mod.rs
@@ -614,6 +614,12 @@ impl TestVm {
.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()
From ca88843a08bd7246f1133377942a5076361b8a1e Mon Sep 17 00:00:00 2001
From: Greg Colombo <greg@oxidecomputer.com>
Date: Tue, 25 Feb 2025 21:14:22 +0000
Subject: [PATCH 4/4] phd: put less Crucible state into the inner struct
---
phd-tests/framework/src/disk/crucible.rs | 34 +++++++++---------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/phd-tests/framework/src/disk/crucible.rs b/phd-tests/framework/src/disk/crucible.rs
index 4df13f903..5ac2f2747 100644
--- a/phd-tests/framework/src/disk/crucible.rs
+++ b/phd-tests/framework/src/disk/crucible.rs
@@ -63,6 +63,8 @@ impl Drop for Downstairs {
#[derive(Debug)]
pub struct CrucibleDisk {
device_name: DeviceName,
+ disk_id: Uuid,
+ guest_os: Option<GuestOsKind>,
inner: Mutex<Inner>,
}
@@ -81,6 +83,8 @@ impl CrucibleDisk {
) -> 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,
@@ -88,7 +92,6 @@ impl CrucibleDisk {
downstairs_ports,
data_dir_root,
read_only_parent,
- guest_os,
log_mode,
)?),
})
@@ -96,7 +99,7 @@ impl CrucibleDisk {
/// Obtains the current volume construction request for this disk.
pub fn vcr(&self) -> VolumeConstructionRequest {
- self.inner.lock().unwrap().vcr()
+ self.inner.lock().unwrap().vcr(self.disk_id)
}
/// Sets the generation number to use in subsequent calls to create a
@@ -112,11 +115,11 @@ impl super::DiskConfig for CrucibleDisk {
}
fn backend_spec(&self) -> ComponentV0 {
- self.inner.lock().unwrap().backend_spec()
+ self.inner.lock().unwrap().backend_spec(self.disk_id)
}
fn guest_os(&self) -> Option<GuestOsKind> {
- self.inner.lock().unwrap().guest_os()
+ self.guest_os
}
fn as_crucible(&self) -> Option<&CrucibleDisk> {
@@ -126,9 +129,6 @@ impl super::DiskConfig for CrucibleDisk {
#[derive(Debug)]
struct Inner {
- /// The UUID to insert into this disk's `VolumeConstructionRequest`s.
- id: Uuid,
-
/// The disk's block size.
block_size: BlockSize,
@@ -144,9 +144,6 @@ struct Inner {
/// 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,
@@ -166,7 +163,6 @@ impl Inner {
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
@@ -313,7 +309,6 @@ impl Inner {
}
Ok(Self {
- id: disk_uuid,
block_size,
blocks_per_extent,
extent_count: extents_in_disk as u32,
@@ -328,13 +323,12 @@ impl Inner {
bytes
},
),
- guest_os,
generation: 1,
})
}
- fn backend_spec(&self) -> ComponentV0 {
- let vcr = self.vcr();
+ fn backend_spec(&self, disk_id: Uuid) -> ComponentV0 {
+ let vcr = self.vcr(disk_id);
ComponentV0::CrucibleStorageBackend(CrucibleStorageBackend {
request_json: serde_json::to_string(&vcr)
@@ -343,19 +337,19 @@ impl Inner {
})
}
- fn vcr(&self) -> VolumeConstructionRequest {
+ fn vcr(&self, disk_id: Uuid) -> VolumeConstructionRequest {
let downstairs_addrs =
self.downstairs_instances.iter().map(|ds| ds.address).collect();
VolumeConstructionRequest::Volume {
- id: self.id,
+ 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: self.id,
+ id: disk_id,
target: downstairs_addrs,
lossy: false,
flush_timeout: None,
@@ -377,10 +371,6 @@ impl Inner {
}),
}
}
-
- fn guest_os(&self) -> Option<GuestOsKind> {
- self.guest_os
- }
}
fn run_crucible_downstairs(