From 6da665954c90ae06fa4ae1d0ca3848ddd566254b Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 31 Jan 2024 19:54:21 +0000 Subject: [PATCH 01/12] PHD: Use ZFS clones for file-backed disks When creating a file-backed disk from an artifact, create a ZFS snapshot and of the dataset containing the artifact, then create and mount a clone of the snapshot and back the disk from the clone. This is much, much less expensive than copying the underlying file and markedly improves performance for test runs with disks of nontrivial size. (On my local test machine, a full test run with a 2 GiB Debian nocloud image took 688 seconds before this change and 610 seconds after, about a 10% speedup.) This is unfortunately not likely to move the needle much on CI times because the Alpine test image CI uses is very small. However, this should make it much easier to switch to non-Alpine CI guest(s) without needlessly bloating CI times. Tested with full runs against Debian 11 and Alpine guests. --- phd-tests/framework/src/disk/file.rs | 42 +-- phd-tests/framework/src/disk/mod.rs | 13 +- phd-tests/framework/src/lib.rs | 1 + phd-tests/framework/src/test_vm/mod.rs | 150 +++++----- phd-tests/framework/src/test_vm/server.rs | 34 ++- phd-tests/framework/src/zfs.rs | 319 ++++++++++++++++++++++ 6 files changed, 436 insertions(+), 123 deletions(-) create mode 100644 phd-tests/framework/src/zfs.rs diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs index 817b777bc..037712d48 100644 --- a/phd-tests/framework/src/disk/file.rs +++ b/phd-tests/framework/src/disk/file.rs @@ -4,13 +4,10 @@ //! Abstractions for disks with a raw file backend. -use std::path::{Path, PathBuf}; - +use camino::Utf8Path; use propolis_client::types::{FileStorageBackend, StorageBackendV0}; -use tracing::{error, info}; -use uuid::Uuid; -use crate::guest_os::GuestOsKind; +use crate::{guest_os::GuestOsKind, zfs::ClonedFile as ZfsClonedFile}; /// An RAII wrapper for a disk wrapped by a file. #[derive(Debug)] @@ -18,8 +15,8 @@ pub struct FileBackedDisk { /// The name to use for instance spec backends that refer to this disk. backend_name: String, - /// The path at which the disk is stored. - disk_path: PathBuf, + /// The backing file for this disk. + file: ZfsClonedFile, /// The kind of guest OS image this guest contains, or `None` if the disk /// was not initialized from a guest OS artifact. @@ -31,23 +28,15 @@ impl FileBackedDisk { /// the specified artifact on the host file system. pub(crate) fn new_from_artifact( backend_name: String, - artifact_path: &impl AsRef, - data_dir: &impl AsRef, + artifact_path: &impl AsRef, guest_os: Option, ) -> Result { - let mut disk_path = data_dir.as_ref().to_path_buf(); - disk_path.push(format!("{}.phd_disk", Uuid::new_v4())); - info!( - source = %artifact_path.as_ref().display(), - disk_path = %disk_path.display(), - "Copying source image to create temporary disk", - ); - - std::fs::copy(artifact_path, &disk_path)?; + let artifact = ZfsClonedFile::create_from_path(artifact_path.as_ref())?; + let disk_path = artifact.path().into_std_path_buf(); // Make sure the disk is writable (the artifact may have been // read-only). - let disk_file = std::fs::File::open(&disk_path)?; + let disk_file = std::fs::File::open(disk_path)?; let mut permissions = disk_file.metadata()?.permissions(); // TODO: Clippy is upset that `set_readonly(false)` results in @@ -57,7 +46,7 @@ impl FileBackedDisk { permissions.set_readonly(false); disk_file.set_permissions(permissions)?; - Ok(Self { backend_name, disk_path, guest_os }) + Ok(Self { backend_name, file: artifact, guest_os }) } } @@ -66,7 +55,7 @@ impl super::DiskConfig for FileBackedDisk { ( self.backend_name.clone(), StorageBackendV0::File(FileStorageBackend { - path: self.disk_path.to_string_lossy().to_string(), + path: self.file.path().to_string(), readonly: false, }), ) @@ -76,14 +65,3 @@ impl super::DiskConfig for FileBackedDisk { self.guest_os } } - -impl Drop for FileBackedDisk { - fn drop(&mut self) { - info!(path = %self.disk_path.display(), "Deleting guest disk image"); - if let Err(e) = std::fs::remove_file(&self.disk_path) { - error!(?e, - path = %self.disk_path.display(), - "Failed to delete guest disk image"); - } - } -} diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index d59a95397..46579e5f7 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/phd-tests/framework/src/disk/mod.rs @@ -14,6 +14,7 @@ use std::{ }; use anyhow::Context; +use camino::Utf8PathBuf; use propolis_client::types::StorageBackendV0; use thiserror::Error; @@ -145,11 +146,10 @@ impl DiskFactory { async fn get_guest_artifact_info( &self, artifact_name: &str, - ) -> Result<(PathBuf, GuestOsKind), DiskError> { + ) -> Result<(Utf8PathBuf, GuestOsKind), DiskError> { self.artifact_store .get_guest_os_image(artifact_name) .await - .map(|(utf8, kind)| (utf8.into_std_path_buf(), kind)) .with_context(|| { format!("failed to get guest OS artifact '{}'", artifact_name) }) @@ -167,13 +167,8 @@ impl DiskFactory { let (artifact_path, guest_os) = self.get_guest_artifact_info(artifact_name).await?; - FileBackedDisk::new_from_artifact( - name, - &artifact_path, - &self.storage_dir, - Some(guest_os), - ) - .map(Arc::new) + FileBackedDisk::new_from_artifact(name, &artifact_path, Some(guest_os)) + .map(Arc::new) } /// Creates a new Crucible-backed disk by creating three region files to diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index 0aaaa358a..872889971 100644 --- a/phd-tests/framework/src/lib.rs +++ b/phd-tests/framework/src/lib.rs @@ -55,6 +55,7 @@ mod port_allocator; mod serial; pub mod server_log_mode; pub mod test_vm; +pub(crate) mod zfs; /// An instance of the PHD test framework. pub struct Framework { diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 18c862d33..76b687572 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -824,78 +824,39 @@ impl Drop for TestVm { // `block_on` here to guarantee that VMMs are synchronously cleaned up // when a `TestVm` is dropped. // - // As an alternative, destructure the client and server and hand them - // off to their own separate task. (The server needs to be moved into - // the task explicitly so that its `Drop` impl, which terminates the - // server process, won't run until this work is complete.) - // - // Send the task back to the framework so that the test runner can wait - // for all under-destruction VMs to be reaped before exiting. + // To drop the VMM safely, destructure this VM into its client, server, + // and attached disk objects, and hand them all off to a separate + // destructor task. Once the task is spawned, send it back to the + // framework so that the test runner can wait for all the VMs destroyed + // by a test case to be reaped before starting another test. let client = self.client.clone(); - let server = self.server.take(); - let task = tokio::spawn( - async move { - let _server = server; - match client - .instance_get() - .send() - .await - .map(|r| r.instance.state) - { - Ok(InstanceState::Destroyed) => return, - Err(e) => warn!( - ?e, - "error getting instance state from dropped VM" - ), - Ok(_) => {} - } - - debug!("stopping test VM on drop"); - if let Err(e) = client - .instance_state_put() - .body(InstanceStateRequested::Stop) - .send() - .await - { - error!(?e, "error stopping dropping VM"); - return; - } - - let check_destroyed = || async { - match client - .instance_get() - .send() - .await - .map(|r| r.instance.state) - { - Ok(InstanceState::Destroyed) => Ok(()), - Ok(state) => { - Err(backoff::Error::transient(anyhow::anyhow!( - "instance not destroyed yet (state: {:?})", - state - ))) - } - Err(e) => { - error!(%e, "failed to get state of dropping VM"); - Err(backoff::Error::permanent(e.into())) - } - } - }; + let mut server = self.server.take().expect( + "TestVm should always have a valid server until it's dropped", + ); - let destroyed = backoff::future::retry( - backoff::ExponentialBackoff { - max_elapsed_time: Some(std::time::Duration::from_secs( - 5, - )), - ..Default::default() - }, - check_destroyed, - ) - .await; + let disks: Vec<_> = self.vm_spec().disk_handles.drain(..).collect(); - if let Err(e) = destroyed { - error!(%e, "dropped VM not destroyed after 5 seconds"); - } + // The order in which the task destroys objects is important: the server + // can't be killed until the client has gotten a chance to shut down + // the VM, and the disks can't be destroyed until the server process has + // been killed. + let task = tokio::spawn( + async move { + // The task doesn't use the disks directly, but they need to be + // kept alive until the server process is gone. + let _disks = disks; + + // Try to make sure the server's kernel VMM is cleaned up before + // killing the server process. This is best-effort; if it fails, + // the kernel VMM is leaked. This generally indicates a bug in + // Propolis (e.g. a VMM reference leak or an instance taking an + // unexpectedly long time to stop). + try_ensure_vm_destroyed(&client).await; + + // Make sure the server process is dead before trying to clean + // up any disks. Otherwise, ZFS may refuse to delete a cloned + // disk because the server process still has it open. + server.kill(); } .instrument( info_span!("VM cleanup", vm = self.spec.vm_name, vm_id = %self.id), @@ -905,3 +866,54 @@ impl Drop for TestVm { let _ = self.cleanup_task_tx.send(task); } } + +/// Attempts to ensure that the Propolis server referred to by `client` is in +/// the `Destroyed` state by stopping any VM that happens to be running in that +/// server. +/// +/// This function is best-effort. +async fn try_ensure_vm_destroyed(client: &Client) { + match client.instance_get().send().await.map(|r| r.instance.state) { + Ok(InstanceState::Destroyed) => return, + Err(e) => warn!(?e, "error getting instance state from dropped VM"), + Ok(_) => {} + } + + debug!("trying to ensure Propolis server VM is destroyed"); + if let Err(e) = client + .instance_state_put() + .body(InstanceStateRequested::Stop) + .send() + .await + { + error!(%e, "error stopping VM to move it to Destroyed"); + return; + } + + let check_destroyed = || async { + match client.instance_get().send().await.map(|r| r.instance.state) { + Ok(InstanceState::Destroyed) => Ok(()), + Ok(state) => Err(backoff::Error::transient(anyhow::anyhow!( + "instance not destroyed yet (state: {:?})", + state + ))), + Err(e) => { + error!(%e, "failed to get state of VM being destroyed"); + Err(backoff::Error::permanent(e.into())) + } + } + }; + + let destroyed = backoff::future::retry( + backoff::ExponentialBackoff { + max_elapsed_time: Some(std::time::Duration::from_secs(5)), + ..Default::default() + }, + check_destroyed, + ) + .await; + + if let Err(e) = destroyed { + error!(%e, "VM not destroyed after 5 seconds"); + } +} diff --git a/phd-tests/framework/src/test_vm/server.rs b/phd-tests/framework/src/test_vm/server.rs index 0e6596c28..92f5fb18b 100644 --- a/phd-tests/framework/src/test_vm/server.rs +++ b/phd-tests/framework/src/test_vm/server.rs @@ -34,7 +34,7 @@ pub struct ServerProcessParameters<'a> { } pub struct PropolisServer { - server: std::process::Child, + server: Option, address: SocketAddrV4, } @@ -100,39 +100,47 @@ impl PropolisServer { } let server = PropolisServer { - server: server_cmd.spawn()?, + server: Some(server_cmd.spawn()?), address: server_addr, }; - info!("Launched server with pid {}", server.server.id()); + info!( + "Launched server with pid {}", + server.server.as_ref().unwrap().id() + ); Ok(server) } pub(crate) fn server_addr(&self) -> SocketAddrV4 { self.address } -} -impl Drop for PropolisServer { - fn drop(&mut self) { - let pid = self.server.id().to_string(); + /// Kills this server process if it hasn't been killed already. + pub(super) fn kill(&mut self) { + let Some(mut server) = self.server.take() else { + return; + }; + + let pid = server.id(); debug!( pid, %self.address, - "Killing Propolis server that was dropped" + "Killing Propolis server process" ); std::process::Command::new("pfexec") - .args(["kill", &pid]) + .args(["kill", &pid.to_string()]) .spawn() .expect("should be able to kill a phd-spawned propolis"); - self.server + server .wait() .expect("should be able to wait on a phd-spawned propolis"); + } +} - debug!(pid, - %self.address, - "Successfully waited for demise of Propolis server that was dropped"); +impl Drop for PropolisServer { + fn drop(&mut self) { + self.kill(); } } diff --git a/phd-tests/framework/src/zfs.rs b/phd-tests/framework/src/zfs.rs new file mode 100644 index 000000000..b42c5f921 --- /dev/null +++ b/phd-tests/framework/src/zfs.rs @@ -0,0 +1,319 @@ +// 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/. + +//! Support functions for working with ZFS snapshots and clones. + +use anyhow::Context; +use camino::{Utf8Path, Utf8PathBuf}; +use tracing::{debug, error}; +use uuid::Uuid; + +#[derive(Debug)] +struct DatasetName(String); + +#[derive(Debug)] +struct SnapshotName(String); + +#[derive(Debug)] +struct CloneName(String); + +/// Describes a dataset that's mounted at a specific point in the global +/// directory hierarchy. +#[derive(Debug)] +struct Dataset { + /// The name of this dataset, used to refer to it as a subject of a ZFS + /// operation. + name: DatasetName, + + /// The mount point of this dataset. Stripping this prefix from the absolute + /// path of a file that lies in this dataset yields the path to the file + /// relative to the dataset root. This is needed to find the file if the + /// dataset (or a clone of it) is mounted someplace else. + mount_point: Utf8PathBuf, +} + +/// Describes a snapshot of a specific dataset. When dropped, attempts to delete +/// itself using `zfs destroy`. +#[derive(Debug)] +struct Snapshot { + /// The name of this snapshot, used to refer to it as a subject of a ZFS + /// operation. + name: SnapshotName, +} + +impl Snapshot { + /// Takes a snapshot of the supplied `dataset`. + fn create_from_dataset(dataset: &DatasetName) -> anyhow::Result { + let snapshot_name = format!("{}@phd-{}", dataset.0, Uuid::new_v4()); + zfs_command("snapshot", &[&snapshot_name])?; + + Ok(Self { name: SnapshotName(snapshot_name) }) + } +} + +impl Drop for Snapshot { + fn drop(&mut self) { + debug!(name = self.name.0, "zfs snapshot dropped"); + let _ = zfs_command("destroy", &[&self.name.0]); + } +} + +/// Describes a clone of a specific snapshot. When dropped, attempts to delete +/// itself using `zfs destroy`. +#[derive(Debug)] +struct Clone { + /// The name of this clone, used to refer to it as a subject of a ZFS + /// operation. + name: CloneName, + + /// The point at which this clone is mounted in the global directory + /// hierarchy. + mount_point: Utf8PathBuf, + + /// The snapshot this clone derives from. Snapshots can't be deleted until + /// all their clones are gone; this reference helps to ensure that clones + /// and snapshots are deleted in the correct order irrespective of when the + /// clones and the [`CloneManager`] are dropped. + _snapshot: Snapshot, +} + +impl Drop for Clone { + fn drop(&mut self) { + debug!(name = self.name.0, "zfs clone dropped"); + let _ = zfs_command("destroy", &[&self.name.0]); + } +} + +/// Represents a specific copy-on-write file within a ZFS clone. When this is +/// dropped, attempts to delete the associated clone. +#[derive(Debug)] +pub struct ClonedFile { + /// The clone to which this file belongs. + clone: Clone, + + /// The path to this file relative to the mount point of the clone. + relative_path: Utf8PathBuf, +} + +impl ClonedFile { + /// Creates a snapshot and clone of the dataset that contains the canonical + /// location of the file indicated by `path`. + pub fn create_from_path(path: &Utf8Path) -> anyhow::Result { + // Canonicalize the path to resolve any symbolic links before doing any + // prefix matching. + let canonical_path = path.canonicalize_utf8()?; + + let containing_dataset = Dataset::from_path(&canonical_path) + .with_context(|| format!("getting dataset containing {path}"))?; + + let relative_file_path = canonical_path + .strip_prefix(&containing_dataset.mount_point) + .context("getting relative path to file to clone")?; + + let snapshot = Snapshot::create_from_dataset(&containing_dataset.name)?; + Self::create_from_paths_and_snapshot(relative_file_path, snapshot) + .context("creating zfs clone") + } + + /// Yields the absolute path to this cloned file in the global directory + /// hierarchy. + pub fn path(&self) -> Utf8PathBuf { + let mut path = self.clone.mount_point.clone(); + path.push(&self.relative_path); + path + } + + /// Given a path to a file relative to the root of its (mounted) dataset, + /// and the name of a snapshot of that dataset, clones the snapshot and + /// returns a handle to the clone. The [`path`] method can be used to find + /// the absolute path to the file within the clone. + fn create_from_paths_and_snapshot( + relative_file_path: &Utf8Path, + snapshot: Snapshot, + ) -> anyhow::Result { + // Naively assume the existence of a root pool named `rpool` in lieu of + // trying to parse the output of `zpool list`. + const ZFS_ROOT_POOL: &str = "rpool"; + let clone_name = + format!("{ZFS_ROOT_POOL}/phd-clone-{}", Uuid::new_v4()); + + zfs_command("clone", &[&snapshot.name.0, &clone_name])?; + + fn get_clone_mount_point( + clone_name: &str, + ) -> anyhow::Result { + let output = zfs_command("list", &[&clone_name])?; + let (object_name, mount_point) = parse_zfs_list_output(output)?; + + anyhow::ensure!( + object_name == clone_name, + "zfs list returned object {object_name} when asked about clone \ + {clone_name}" + ); + + let Some(mount_point) = mount_point else { + anyhow::bail!("new zfs clone {clone_name} not mounted"); + }; + + Ok(mount_point) + } + + let mount_point = match get_clone_mount_point(&clone_name) { + Ok(mount_point) => mount_point, + Err(e) => { + let _ = zfs_command("destroy", &[&clone_name]); + return Err(e); + } + }; + + Ok(Self { + clone: Clone { + name: CloneName(clone_name), + mount_point, + _snapshot: snapshot, + }, + relative_path: relative_file_path.to_path_buf(), + }) + } +} + +impl Dataset { + /// Looks up the dataset containing `path`. + /// + /// This routine fails if any `zfs` command line operations fail or return + /// no output. It also fails if the found dataset's mount point is not a + /// prefix of the supplied `path`, e.g. because of a symbolic link somewhere + /// in the path. + fn from_path(path: &Utf8Path) -> anyhow::Result { + let output = zfs_command("list", &[path.as_str()])?; + let (name, mount_point) = parse_zfs_list_output(output) + .with_context(|| format!("parsing output from zfs list {path}"))?; + + let Some(mount_point) = mount_point else { + anyhow::bail!( + "`zfs list {path}` produced a dataset with no mount point" + ); + }; + + // The rest of this module needs to be able to strip the mount point + // from the original path to get a dataset-relative path to the target + // file. If the file path isn't prefixed by the mount point, this won't + // work. This should generally not happen if the caller was diligent + // about providing canonicalized paths. + anyhow::ensure!( + path.starts_with(&mount_point), + "zfs dataset containing '{path}' is not prefixed by dataset mount + point {mount_point} (is the path canonicalized?)" + ); + + Ok(Self { name: DatasetName(name.to_owned()), mount_point }) + } +} + +/// Parses the output returned from a `zfs list` command into an object name and +/// a mountpoint. +/// +/// This routine assumes the caller scoped its `zfs list` command so that it +/// returns exactly one (non-header) line of output. If it finds more, this +/// routine fails. +fn parse_zfs_list_output( + output: std::process::Output, +) -> anyhow::Result<(String, Option)> { + let output = String::from_utf8(output.stdout) + .context("converting `zfs list` output to string")?; + + debug!(stdout = output, "parsing zfs list output"); + + // The expected output format from this command is + // + // NAME USED AVAIL REFER MOUNTPOINT + // rpool/home/user 263G 549G 263G /home/user + let mut lines = output.lines(); + + // Consume the header line and make sure it looks like it's sensibly + // formatted. In particular, if the supplied path isn't part of a dataset, + // `zfs` will return `cannot open 'path'`. + let header = lines.next().ok_or_else(|| { + anyhow::anyhow!("`zfs list` unexpectedly printed nothing") + })?; + + anyhow::ensure!( + header.starts_with("NAME"), + "expected first line of `zfs list` output to start with NAME \ + (got '{header}')" + ); + + // Capture the first line of actual output for splitting and parsing. If + // there are more output lines than this, fail instead of ignoring some + // output. + let answer = lines.next().ok_or_else(|| { + anyhow::anyhow!("`zfs list` didn't have an output line") + })?; + + if lines.next().is_some() { + anyhow::bail!("`zfs list` returned more than one output line"); + } + + // `zfs list` output looks something like this (with a header line for + // reference): + // + // NAME USED AVAIL REFER MOUNTPOINT + // rpool/home/user 263G 549G 263G /home/user + // + // The object name is the first token and the mount point is the fifth + // (fourth after consuming the name). + let mut words = answer.split_whitespace(); + let name = words.next().ok_or_else(|| { + anyhow::anyhow!("`zfs list` didn't produce a dataset name") + })?; + + // An unmounted object's mount point displays as "-", so this token should + // always be present, even for unmounted objects. + let mount_point = words.nth(3).ok_or_else(|| { + anyhow::anyhow!("`zfs list` didn't produce a mount point") + })?; + + let mount_point = mount_point + .starts_with('/') + .then_some(mount_point) + .map(Utf8PathBuf::from); + + Ok((name.to_owned(), mount_point)) +} + +/// Executes `zfs ` with the supplied `args` as trailing arguments. +/// Returns the full command output on success. Fails if `zfs` returned a +/// nonzero error code. +fn zfs_command( + verb: &str, + args: &[&str], +) -> anyhow::Result { + debug!(verb, ?args, "executing ZFS command"); + + let output = std::process::Command::new("pfexec") + .arg("zfs") + .arg(verb) + .args(args) + .output() + .with_context(|| format!("running `zfs {verb}` with args {args:?}"))?; + + if !output.status.success() { + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + error!( + verb, + ?args, + error_code = output.status.code(), + %stdout, + %stderr, + "zfs command failed" + ); + anyhow::bail!( + "`zfs {verb}` with args {args:?} returned error {:?}", + output.status.code() + ); + } + + Ok(output) +} From 3ffab722b0358cef5a32f6f2a8c515574fe94b0b Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 8 Feb 2024 17:19:26 +0000 Subject: [PATCH 02/12] fall back to copying artifacts that aren't on a ZFS dataset This keeps tests from falling over if the user chose an artifact directory that isn't on a ZFS dataset (e.g. everything is in tmpfs). This should fix PHD CI, but I'm also going to try to make another change to allow CI runs to take advantage of the cloning feature. --- phd-tests/framework/src/disk/file.rs | 75 ++++++++++++++++++++++++++-- phd-tests/framework/src/disk/mod.rs | 20 ++++---- 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs index 037712d48..86d069cb6 100644 --- a/phd-tests/framework/src/disk/file.rs +++ b/phd-tests/framework/src/disk/file.rs @@ -4,11 +4,76 @@ //! Abstractions for disks with a raw file backend. -use camino::Utf8Path; +use camino::{Utf8Path, Utf8PathBuf}; use propolis_client::types::{FileStorageBackend, StorageBackendV0}; +use tracing::{debug, error, warn}; +use uuid::Uuid; use crate::{guest_os::GuestOsKind, zfs::ClonedFile as ZfsClonedFile}; +/// Describes the method used to create the backing file for a file-backed disk. +#[derive(Debug)] +enum BackingFile { + /// The disk is a ZFS clone of the original artifact. + Zfs(ZfsClonedFile), + + /// The disk is a hard copy of the original artifact. + HardCopy(Utf8PathBuf), +} + +impl BackingFile { + /// Creates a new backing file from the artifact with the supplied + /// `artifact_path`. If possible, this routine will create a ZFS clone of + /// the dataset containing the file; otherwise it will fall back to creating + /// a hard copy of the original artifact. + fn create_from_source( + artifact_path: &Utf8Path, + data_dir: &Utf8Path, + ) -> anyhow::Result { + if let Ok(file) = ZfsClonedFile::create_from_path(artifact_path) { + return Ok(Self::Zfs(file)); + } + + warn!(%artifact_path, + "failed to make ZFS clone of backing artifact, will copy it"); + + let mut disk_path = data_dir.to_path_buf(); + disk_path.push(format!("{}.phd_disk", Uuid::new_v4())); + debug!( + source = %artifact_path, + disk_path = %disk_path, + "Copying source image to create temporary disk", + ); + + std::fs::copy(artifact_path, &disk_path)?; + Ok(Self::HardCopy(disk_path)) + } + + /// Yields the path to this disk's backing file. + fn path(&self) -> Utf8PathBuf { + match self { + BackingFile::Zfs(zfs) => zfs.path(), + BackingFile::HardCopy(path) => path.clone(), + } + } +} + +impl Drop for BackingFile { + fn drop(&mut self) { + // ZFS clones are cleaned up by their own drop impls. + if let BackingFile::HardCopy(path) = self { + debug!(%path, "deleting hard copy of guest disk image"); + if let Err(e) = std::fs::remove_file(&path) { + error!( + ?e, + %path, + "failed to delete hard copy of guest disk image" + ); + } + } + } +} + /// An RAII wrapper for a disk wrapped by a file. #[derive(Debug)] pub struct FileBackedDisk { @@ -16,7 +81,7 @@ pub struct FileBackedDisk { backend_name: String, /// The backing file for this disk. - file: ZfsClonedFile, + file: BackingFile, /// The kind of guest OS image this guest contains, or `None` if the disk /// was not initialized from a guest OS artifact. @@ -29,9 +94,13 @@ impl FileBackedDisk { pub(crate) fn new_from_artifact( backend_name: String, artifact_path: &impl AsRef, + data_dir: &impl AsRef, guest_os: Option, ) -> Result { - let artifact = ZfsClonedFile::create_from_path(artifact_path.as_ref())?; + let artifact = BackingFile::create_from_source( + artifact_path.as_ref(), + data_dir.as_ref(), + )?; let disk_path = artifact.path().into_std_path_buf(); // Make sure the disk is writable (the artifact may have been diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index 46579e5f7..2fb09e623 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/phd-tests/framework/src/disk/mod.rs @@ -8,13 +8,10 @@ //! They can then pass these disks to the VM factory to connect them to a //! specific guest VM. -use std::{ - path::{Path, PathBuf}, - sync::Arc, -}; +use std::sync::Arc; use anyhow::Context; -use camino::Utf8PathBuf; +use camino::{Utf8Path, Utf8PathBuf}; use propolis_client::types::StorageBackendV0; use thiserror::Error; @@ -109,7 +106,7 @@ pub enum DiskSource<'a> { /// Propolis backend implementations interact with the disk. pub(crate) struct DiskFactory { /// The directory in which disk files should be stored. - storage_dir: PathBuf, + storage_dir: Utf8PathBuf, /// A reference to the artifact store to use to look up guest OS artifacts /// when those are used as a disk source. @@ -128,7 +125,7 @@ impl DiskFactory { /// their data in `storage_dir` and will look up guest OS images in the /// supplied `artifact_store`. pub fn new( - storage_dir: &impl AsRef, + storage_dir: &impl AsRef, artifact_store: Arc, port_allocator: Arc, log_mode: ServerLogMode, @@ -167,8 +164,13 @@ impl DiskFactory { let (artifact_path, guest_os) = self.get_guest_artifact_info(artifact_name).await?; - FileBackedDisk::new_from_artifact(name, &artifact_path, Some(guest_os)) - .map(Arc::new) + FileBackedDisk::new_from_artifact( + name, + &artifact_path, + &self.storage_dir, + Some(guest_os), + ) + .map(Arc::new) } /// Creates a new Crucible-backed disk by creating three region files to From d65911d9ebc0bb1e300479cd5a5b645dfdecbe53 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 8 Feb 2024 17:53:35 +0000 Subject: [PATCH 03/12] make phd-run put artifacts on a ZFS filesystem This is needed to use ZFS clones for file-backed disks. --- .github/buildomat/jobs/phd-run.sh | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/buildomat/jobs/phd-run.sh b/.github/buildomat/jobs/phd-run.sh index 6117dc900..e5faa1610 100644 --- a/.github/buildomat/jobs/phd-run.sh +++ b/.github/buildomat/jobs/phd-run.sh @@ -21,12 +21,19 @@ indir="/input" indir_suffix="phd-build/out/*.tar.gz" phddir="$PWD/phd-test" +# Download artifacts to /work so that the runner can create ZFS clones of them +# to reduce the number of files it needs to copy repeatedly. +artifactdir="/work/phd-artifacts" + banner 'Inputs' find $indir -ls rm -rf "$phddir" mkdir "$phddir" +rm -rf "$artifactdir" +mkdir "$artifactdir" + for p in $indir/$indir_suffix; do tar xzvf $p -C $phddir for f in $(tar tf "$p"); do @@ -63,7 +70,7 @@ set +e --crucible-downstairs-commit auto \ --artifact-toml-path $artifacts \ --tmp-directory $tmpdir \ - --artifact-directory $tmpdir | \ + --artifact-directory $artifactdir | \ tee /tmp/phd-runner.log) failcount=$? set -e From abdd3eebd16a08930e168d4f70a1aeb2db53a257 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 8 Feb 2024 19:35:44 +0000 Subject: [PATCH 04/12] stick zfs clones under the dataset containing the desired source file --- phd-tests/framework/src/zfs.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/phd-tests/framework/src/zfs.rs b/phd-tests/framework/src/zfs.rs index b42c5f921..14abea018 100644 --- a/phd-tests/framework/src/zfs.rs +++ b/phd-tests/framework/src/zfs.rs @@ -112,8 +112,12 @@ impl ClonedFile { .context("getting relative path to file to clone")?; let snapshot = Snapshot::create_from_dataset(&containing_dataset.name)?; - Self::create_from_paths_and_snapshot(relative_file_path, snapshot) - .context("creating zfs clone") + Self::create_from_paths_and_snapshot( + containing_dataset, + relative_file_path, + snapshot, + ) + .context("creating zfs clone") } /// Yields the absolute path to this cloned file in the global directory @@ -129,14 +133,12 @@ impl ClonedFile { /// returns a handle to the clone. The [`path`] method can be used to find /// the absolute path to the file within the clone. fn create_from_paths_and_snapshot( + dataset: Dataset, relative_file_path: &Utf8Path, snapshot: Snapshot, ) -> anyhow::Result { - // Naively assume the existence of a root pool named `rpool` in lieu of - // trying to parse the output of `zpool list`. - const ZFS_ROOT_POOL: &str = "rpool"; let clone_name = - format!("{ZFS_ROOT_POOL}/phd-clone-{}", Uuid::new_v4()); + format!("{}/phd-clone-{}", dataset.name.0, Uuid::new_v4()); zfs_command("clone", &[&snapshot.name.0, &clone_name])?; From 53d47564102d283be2304b8e60985a1f0b78f59a Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 8 Feb 2024 19:44:02 +0000 Subject: [PATCH 05/12] ci: put artifacts on the runner's SSDs, not the ramdisk --- .github/buildomat/jobs/phd-run.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/buildomat/jobs/phd-run.sh b/.github/buildomat/jobs/phd-run.sh index e5faa1610..5f57e33ac 100644 --- a/.github/buildomat/jobs/phd-run.sh +++ b/.github/buildomat/jobs/phd-run.sh @@ -21,9 +21,10 @@ indir="/input" indir_suffix="phd-build/out/*.tar.gz" phddir="$PWD/phd-test" -# Download artifacts to /work so that the runner can create ZFS clones of them -# to reduce the number of files it needs to copy repeatedly. -artifactdir="/work/phd-artifacts" +# Put artifacts on the runner's SSDs (the /work ramdisk is small by design, too +# small for images of any appreciable size). +pfexec zpool create -f phd-artifacts c1t1d0 c2t1d0 +artifactdir="/phd-artifacts" banner 'Inputs' find $indir -ls From 41548656797e7276ae786d6baaefa72d78bdb111 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 8 Feb 2024 20:07:31 +0000 Subject: [PATCH 06/12] gosh dang it --- .github/buildomat/jobs/phd-run.sh | 3 --- 1 file changed, 3 deletions(-) diff --git a/.github/buildomat/jobs/phd-run.sh b/.github/buildomat/jobs/phd-run.sh index 5f57e33ac..a12671438 100644 --- a/.github/buildomat/jobs/phd-run.sh +++ b/.github/buildomat/jobs/phd-run.sh @@ -32,9 +32,6 @@ find $indir -ls rm -rf "$phddir" mkdir "$phddir" -rm -rf "$artifactdir" -mkdir "$artifactdir" - for p in $indir/$indir_suffix; do tar xzvf $p -C $phddir for f in $(tar tf "$p"); do From 409c7bd292f6302f0d42779dad06b850a42eef3f Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 14 Feb 2024 20:47:47 +0000 Subject: [PATCH 07/12] include error in warning message for failure to clone --- phd-tests/framework/src/disk/file.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs index 86d069cb6..1cd75798e 100644 --- a/phd-tests/framework/src/disk/file.rs +++ b/phd-tests/framework/src/disk/file.rs @@ -30,13 +30,15 @@ impl BackingFile { artifact_path: &Utf8Path, data_dir: &Utf8Path, ) -> anyhow::Result { - if let Ok(file) = ZfsClonedFile::create_from_path(artifact_path) { - return Ok(Self::Zfs(file)); + match ZfsClonedFile::create_from_path(artifact_path) { + Ok(file) => return Ok(Self::Zfs(file)), + Err(error) => warn!( + %artifact_path, + %error, + "failed to make ZFS clone of backing artifact, will copy it" + ), } - warn!(%artifact_path, - "failed to make ZFS clone of backing artifact, will copy it"); - let mut disk_path = data_dir.to_path_buf(); disk_path.push(format!("{}.phd_disk", Uuid::new_v4())); debug!( From c2133469b0695d4eebd40f2c180a838080f48cfc Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 14 Feb 2024 20:49:19 +0000 Subject: [PATCH 08/12] remove unneeded type conversion --- phd-tests/framework/src/disk/file.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs index 1cd75798e..e5fe6fef9 100644 --- a/phd-tests/framework/src/disk/file.rs +++ b/phd-tests/framework/src/disk/file.rs @@ -103,11 +103,10 @@ impl FileBackedDisk { artifact_path.as_ref(), data_dir.as_ref(), )?; - let disk_path = artifact.path().into_std_path_buf(); // Make sure the disk is writable (the artifact may have been // read-only). - let disk_file = std::fs::File::open(disk_path)?; + let disk_file = std::fs::File::open(artifact.path())?; let mut permissions = disk_file.metadata()?.permissions(); // TODO: Clippy is upset that `set_readonly(false)` results in From 93b17b559a3ef7f9087c02e11c5b78c346aa5cfe Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 14 Feb 2024 20:50:11 +0000 Subject: [PATCH 09/12] tidy up field name in logs --- phd-tests/framework/src/test_vm/mod.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 76b687572..f0db51151 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -875,18 +875,24 @@ impl Drop for TestVm { async fn try_ensure_vm_destroyed(client: &Client) { match client.instance_get().send().await.map(|r| r.instance.state) { Ok(InstanceState::Destroyed) => return, - Err(e) => warn!(?e, "error getting instance state from dropped VM"), + Err(error) => warn!( + %error, + "error getting instance state from dropped VM" + ), Ok(_) => {} } debug!("trying to ensure Propolis server VM is destroyed"); - if let Err(e) = client + if let Err(error) = client .instance_state_put() .body(InstanceStateRequested::Stop) .send() .await { - error!(%e, "error stopping VM to move it to Destroyed"); + error!( + %error, + "error stopping VM to move it to Destroyed" + ); return; } @@ -897,9 +903,12 @@ async fn try_ensure_vm_destroyed(client: &Client) { "instance not destroyed yet (state: {:?})", state ))), - Err(e) => { - error!(%e, "failed to get state of VM being destroyed"); - Err(backoff::Error::permanent(e.into())) + Err(error) => { + error!( + %error, + "failed to get state of VM being destroyed" + ); + Err(backoff::Error::permanent(error.into())) } } }; @@ -913,7 +922,7 @@ async fn try_ensure_vm_destroyed(client: &Client) { ) .await; - if let Err(e) = destroyed { - error!(%e, "VM not destroyed after 5 seconds"); + if let Err(error) = destroyed { + error!(%error, "VM not destroyed after 5 seconds"); } } From fd6ac0b970881b59355988add1fc763813fc9dcf Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 14 Feb 2024 21:07:16 +0000 Subject: [PATCH 10/12] add path to error context --- phd-tests/framework/src/zfs.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/phd-tests/framework/src/zfs.rs b/phd-tests/framework/src/zfs.rs index 14abea018..3aba56f6b 100644 --- a/phd-tests/framework/src/zfs.rs +++ b/phd-tests/framework/src/zfs.rs @@ -117,7 +117,12 @@ impl ClonedFile { relative_file_path, snapshot, ) - .context("creating zfs clone") + .with_context(|| { + format!( + "creating zfs clone of {canonical_path} with original path \ + {path}" + ) + }) } /// Yields the absolute path to this cloned file in the global directory From dece78ce2834adc23fb0413dd4c4ca53e7585001 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 14 Feb 2024 21:07:41 +0000 Subject: [PATCH 11/12] note that manual cleanup is required --- phd-tests/framework/src/zfs.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/phd-tests/framework/src/zfs.rs b/phd-tests/framework/src/zfs.rs index 3aba56f6b..2fd709f1a 100644 --- a/phd-tests/framework/src/zfs.rs +++ b/phd-tests/framework/src/zfs.rs @@ -147,6 +147,11 @@ impl ClonedFile { zfs_command("clone", &[&snapshot.name.0, &clone_name])?; + // If any errors occur between this point and the construction of a + // `Clone` wrapper, this function needs to destroy the new clone + // manually. The only thing needed to construct a `Clone` is its mount + // point, so put that logic in a function and clean up manually if it + // fails. fn get_clone_mount_point( clone_name: &str, ) -> anyhow::Result { From 818efb28046de8243e025497033c57af62c93a02 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 14 Feb 2024 21:57:03 +0000 Subject: [PATCH 12/12] CloneManager is no longer a thing --- phd-tests/framework/src/zfs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phd-tests/framework/src/zfs.rs b/phd-tests/framework/src/zfs.rs index 2fd709f1a..9415fe360 100644 --- a/phd-tests/framework/src/zfs.rs +++ b/phd-tests/framework/src/zfs.rs @@ -74,7 +74,7 @@ struct Clone { /// The snapshot this clone derives from. Snapshots can't be deleted until /// all their clones are gone; this reference helps to ensure that clones /// and snapshots are deleted in the correct order irrespective of when the - /// clones and the [`CloneManager`] are dropped. + /// clones are dropped. _snapshot: Snapshot, }