From 5c9ca44f89b2fda75fcde4c96f3391bce8a85110 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 29 Aug 2024 09:11:48 -0700 Subject: [PATCH] Skip `test_disk_usage` if there's no `/usr/bin/du` The `zone_bundle::disk_usage` function in sled-agent executes `du(1)` to measure disk usage for a path. Presently, the path to the `du(1)` executable is hard-coded to `/usr/bin/du`. On some systems, such as NixOS, the `du(1)` executable may not live at the path `/usr/bin/du`. We could, potentially, change the `disk_usage` function to find a `du(1)` binary on the PATH, or fall back to other locations...but, on Helios, it is always at `/usr/bin/du`, so hardcoding it is a good solution for the environment that sled-agents are actually intended to run in. However, hardcoding the path means that this test will always fail on systems where there is no `/usr/bin/du`. Therefore, this commit changes the test to first check if `/usr/bin/du` exists, and return early if it does not, printing a warning indicating that the test is skipped. If, someday, Rust's libtest grows a mechanism for tests to mark themselves as skipped at runtime dynamically (rather than using the `#[ignore]` attribute at compile-time), we should probably switch to using that hypothetical mechanism here. See this [internals.rust-lang.org thread][1]. If folks aren't happy with making the test skip itself, we *could* change the implementation to worth with `du(1)` being at a different location by finding it from the PATH, potentially falling back to `/usr/bin/du` if it isn't on the PATH. My thought process was that it was better to avoid complicating the production sled-agent code just to support environments that the software will never actually run on for test purposes, but I'm open to being convinced otherwise. [1]: https://internals.rust-lang.org/t/pre-rfc-skippable-tests/14611 --- sled-agent/src/zone_bundle.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/sled-agent/src/zone_bundle.rs b/sled-agent/src/zone_bundle.rs index 46cee1c415..e0a3f8012e 100644 --- a/sled-agent/src/zone_bundle.rs +++ b/sled-agent/src/zone_bundle.rs @@ -62,6 +62,9 @@ const ARCHIVE_SNAPSHOT_PREFIX: &'static str = "zone-archives-"; const ZONE_BUNDLE_ZFS_PROPERTY_NAME: &'static str = "oxide:for-zone-bundle"; const ZONE_BUNDLE_ZFS_PROPERTY_VALUE: &'static str = "true"; +// Path to the `du(1)` executable. +const DU: &str = "/usr/bin/du"; + // Initialize the ZFS resources we need for zone bundling. // // This deletes any snapshots matching the names we expect to create ourselves @@ -1573,7 +1576,6 @@ async fn disk_usage(path: &Utf8PathBuf) -> Result { compile_error!("unsupported target OS"); } } - const DU: &str = "/usr/bin/du"; let args = &[DU_ARG, "-s", path.as_str()]; let output = Command::new(DU).args(args).output().await.map_err(|err| { BundleError::Command { cmd: format!("{DU} {}", args.join(" ")), err } @@ -1660,10 +1662,33 @@ async fn zfs_quota(path: &Utf8PathBuf) -> Result { #[cfg(test)] mod tests { use super::disk_usage; + use super::Utf8Path; use super::Utf8PathBuf; + use super::DU; #[tokio::test] async fn test_disk_usage() { + // On some systems, such as NixOS, the `du(1)` executable may not live + // at the path `/usr/bin/du``. We could, potentially, change the + // `disk_usage` function to find a du binary on the PATH, or fall back + // to other locations...but, on Helios, it is always at `/usr/bin/du`, + // so hardcoding it is a good solution for the environment that + // sled-agents are actually intended to run in. + // + // However, hardcoding the path means that this test will always fail on + // systems where there is no `/usr/bin/du`, so let's just return early + // on such systems. + // + // TODO(eliza): if, someday, Rust's libtest grows a mechanism for tests + // to mark themselves as skipped at runtime dynamically (rather than + // using the `#[ignore]` attribute at compile-time), we should use + // that here. See: + // https://internals.rust-lang.org/t/pre-rfc-skippable-tests/14611 + if !Utf8Path::new(DU).exists() { + eprintln!("No {DU} executable found, skipping disk usage test!"); + return; + } + let path = Utf8PathBuf::from(concat!(env!("CARGO_MANIFEST_DIR"), "/src")); let usage = disk_usage(&path).await.unwrap();