PHD: Use ZFS clones for file-backed disks (#640)

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.

Author: gjcolombo

.github/buildomat/jobs/phd-run.sh

@@ -21,6 +21,11 @@ indir="/input"
 indir_suffix="phd-build/out/*.tar.gz"
 phddir="$PWD/phd-test"
 
+# 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
 
@@ -63,7 +68,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

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

@@ -4,22 +4,86 @@
 
 //! Abstractions for disks with a raw file backend.
 
-use std::path::{Path, PathBuf};
-
+use camino::{Utf8Path, Utf8PathBuf};
 use propolis_client::types::{FileStorageBackend, StorageBackendV0};
-use tracing::{error, info};
+use tracing::{debug, error, warn};
 use uuid::Uuid;
 
-use crate::guest_os::GuestOsKind;
+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<Self> {
+        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"
+            ),
+        }
+
+        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 {
     /// 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: BackingFile,
 
     /// The kind of guest OS image this guest contains, or `None` if the disk
     /// was not initialized from a guest OS artifact.
@@ -31,23 +95,18 @@ impl FileBackedDisk {
     /// the specified artifact on the host file system.
     pub(crate) fn new_from_artifact(
         backend_name: String,
-        artifact_path: &impl AsRef<Path>,
-        data_dir: &impl AsRef<Path>,
+        artifact_path: &impl AsRef<Utf8Path>,
+        data_dir: &impl AsRef<Utf8Path>,
         guest_os: Option<GuestOsKind>,
     ) -> Result<Self, super::DiskError> {
-        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 = BackingFile::create_from_source(
+            artifact_path.as_ref(),
+            data_dir.as_ref(),
+        )?;
 
         // 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
@@ -57,7 +116,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 +125,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 +135,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");
-        }
-    }
-}

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

@@ -8,12 +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::{Utf8Path, Utf8PathBuf};
 use propolis_client::types::StorageBackendV0;
 use thiserror::Error;
 
@@ -108,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.
@@ -127,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<Path>,
+        storage_dir: &impl AsRef<Utf8Path>,
         artifact_store: Arc<ArtifactStore>,
         port_allocator: Arc<PortAllocator>,
         log_mode: ServerLogMode,
@@ -145,11 +143,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)
             })

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 {