diff --git a/.github/buildomat/jobs/phd-run.sh b/.github/buildomat/jobs/phd-run.sh index 6117dc900..a12671438 100644 --- a/.github/buildomat/jobs/phd-run.sh +++ b/.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 diff --git a/phd-tests/framework/src/disk/file.rs b/phd-tests/framework/src/disk/file.rs index 817b777bc..e5fe6fef9 100644 --- a/phd-tests/framework/src/disk/file.rs +++ b/phd-tests/framework/src/disk/file.rs @@ -4,13 +4,77 @@ //! 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 { + 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)] @@ -18,8 +82,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: 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, - data_dir: &impl AsRef, + artifact_path: &impl AsRef, + data_dir: &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 = 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"); - } - } -} diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index d59a95397..2fb09e623 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/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, + storage_dir: &impl AsRef, artifact_store: Arc, port_allocator: Arc, 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) }) 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..f0db51151 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,63 @@ 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(error) => warn!( + %error, + "error getting instance state from dropped VM" + ), + Ok(_) => {} + } + + debug!("trying to ensure Propolis server VM is destroyed"); + if let Err(error) = client + .instance_state_put() + .body(InstanceStateRequested::Stop) + .send() + .await + { + error!( + %error, + "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(error) => { + error!( + %error, + "failed to get state of VM being destroyed" + ); + Err(backoff::Error::permanent(error.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(error) = destroyed { + error!(%error, "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..9415fe360 --- /dev/null +++ b/phd-tests/framework/src/zfs.rs @@ -0,0 +1,331 @@ +// 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 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( + containing_dataset, + relative_file_path, + snapshot, + ) + .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 + /// 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( + dataset: Dataset, + relative_file_path: &Utf8Path, + snapshot: Snapshot, + ) -> anyhow::Result { + let clone_name = + format!("{}/phd-clone-{}", dataset.name.0, Uuid::new_v4()); + + 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 { + 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) +}