PHD: put log files in per-test subdirectories

.github/buildomat/phd-run-with-args.sh

@@ -81,7 +81,7 @@ failcount=$?
 set -e
 
 tar -czvf /tmp/phd-tmp-files.tar.gz \
-	-C /tmp/propolis-phd /tmp/propolis-phd/*.log
+	-C "$tmpdir" .
 
 exitcode=0
 if [ $failcount -eq 0 ]; then

phd-tests/README.md

@@ -227,7 +227,7 @@ the adapter to provide the correct credentials.
 ## Authoring tests
 
 PHD's test cases live in the `tests` crate. To write a new test, add a function
-of the form `fn my_test(ctx: &Framework)` and tag it with the
+of the form `fn my_test(ctx: &TestCtx)` and tag it with the
 `#[phd_testcase]` attribute macro. The framework will automatically register the
 test into the crate's test inventory for the runner to discover.
 

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

@@ -92,6 +92,7 @@ impl CrucibleDisk {
         read_only_parent: Option<&impl AsRef<Path>>,
         guest_os: Option<GuestOsKind>,
         log_config: LogConfig,
+        output_dir: &impl AsRef<Path>,
     ) -> anyhow::Result<Self> {
         Ok(Self {
             device_name,
@@ -105,6 +106,7 @@ impl CrucibleDisk {
                 data_dir_root,
                 read_only_parent,
                 log_config,
+                output_dir,
             )?),
         })
     }
@@ -209,6 +211,7 @@ impl Inner {
         data_dir_root: &impl AsRef<Path>,
         read_only_parent: Option<&impl AsRef<Path>>,
         log_config: LogConfig,
+        output_dir: &impl AsRef<Path>,
     ) -> anyhow::Result<Self> {
         // To create a region, Crucible requires a block size, an extent size
         // given as a number of blocks, and an extent count. Compute the latter
@@ -339,7 +342,7 @@ impl Inner {
             // nice to connect this more directly to the output desire expressed
             // by the test runner.
             let (stdout, stderr) = log_config.output_mode.get_handles(
-                data_dir_root,
+                output_dir,
                 &format!("crucible_{disk_uuid}_{port}"),
             )?;
 

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

@@ -8,6 +8,7 @@
 //! They can then pass these disks to the VM factory to connect them to a
 //! specific guest VM.
 
+use std::path::Path;
 use std::sync::Arc;
 
 use anyhow::Context;
@@ -278,6 +279,7 @@ impl DiskFactory {
         source: &DiskSource<'_>,
         mut min_disk_size_gib: u64,
         block_size: BlockSize,
+        output_dir: &impl AsRef<Path>,
     ) -> Result<Arc<CrucibleDisk>, DiskError> {
         const BYTES_PER_GIB: u64 = 1024 * 1024 * 1024;
 
@@ -326,6 +328,7 @@ impl DiskFactory {
             artifact_path.as_ref(),
             guest_os,
             self.log_config,
+            output_dir,
         )
         .map(Arc::new)
         .map_err(Into::into)

phd-tests/framework/src/lib.rs

@@ -58,6 +58,13 @@ mod serial;
 pub mod test_vm;
 pub(crate) mod zfs;
 
+/// A test context for an individual PHD test, containing a `Framework` plus
+/// test specific information.
+pub struct TestCtx {
+    pub(crate) framework: Arc<Framework>,
+    pub(crate) output_dir: Utf8PathBuf,
+}
+
 /// An instance of the PHD test framework.
 pub struct Framework {
     pub(crate) tmp_directory: Utf8PathBuf,
@@ -123,6 +130,117 @@ pub enum BasePropolisSource<'a> {
     Local(&'a Utf8PathBuf),
 }
 
+impl TestCtx {
+    /// Creates a new VM configuration builder using the default configuration
+    /// from this framework instance.
+    pub fn vm_config_builder(&self, vm_name: &str) -> VmConfig<'_> {
+        self.framework.vm_config_builder(vm_name)
+    }
+
+    /// Yields an environment builder with default settings (run the VM on the
+    /// test runner's machine using the default Propolis from the command line).
+    pub fn environment_builder(&self) -> EnvironmentSpec {
+        self.framework.environment_builder()
+    }
+
+    /// Yields this framework instance's default guest OS artifact name. This
+    /// can be used to configure boot disks with different parameters than the
+    /// builder defaults.
+    pub fn default_guest_os_artifact(&self) -> &str {
+        self.framework.default_guest_os_artifact()
+    }
+
+    /// Yields the guest OS adapter corresponding to the default guest OS
+    /// artifact.
+    pub async fn default_guest_os_kind(&self) -> anyhow::Result<GuestOsKind> {
+        self.framework.default_guest_os_kind().await
+    }
+
+    /// Indicates whether the disk factory in this framework supports the
+    /// creation of Crucible disks. This can be used to skip tests that require
+    /// Crucible support.
+    pub fn crucible_enabled(&self) -> bool {
+        self.framework.crucible_enabled
+    }
+
+    /// Indicates whether a "migration base" Propolis server artifact is
+    /// available for migration-from-base tests.
+    pub fn migration_base_enabled(&self) -> bool {
+        self.framework.migration_base_enabled
+    }
+    /// Spawns a test VM using the default configuration returned from
+    /// `vm_builder` and the default environment returned from
+    /// `environment_builder`.
+    pub async fn spawn_default_vm(
+        &self,
+        vm_name: &str,
+    ) -> anyhow::Result<TestVm> {
+        self.spawn_vm(&self.vm_config_builder(vm_name), None).await
+    }
+
+    /// Spawns a new test VM using the supplied `config`. 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(
+        &self,
+        config: &VmConfig<'_>,
+        environment: Option<&EnvironmentSpec>,
+    ) -> anyhow::Result<TestVm> {
+        self.spawn_vm_with_spec(
+            config
+                .vm_spec(self)
+                .await
+                .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
+        .context("constructing test VM")
+    }
+
+    /// Spawns a "successor" to the supplied `vm`. The successor has the same
+    /// configuration and takes additional references to all of its
+    /// predecessor's backing objects (e.g. disk handles). If `environment` is
+    /// `None`, the successor is launched using the predecessor's environment
+    /// spec.
+    pub async fn spawn_successor_vm(
+        &self,
+        vm_name: &str,
+        vm: &TestVm,
+        environment: Option<&EnvironmentSpec>,
+    ) -> anyhow::Result<TestVm> {
+        let mut vm_spec = vm.vm_spec().clone();
+        vm_spec.set_vm_name(vm_name.to_owned());
+
+        // Create new metadata for an instance based on this predecessor. It
+        // should have the same project and silo IDs, but the sled identifiers
+        // will be different.
+        vm_spec.refresh_sled_identifiers();
+
+        TestVm::new(
+            self,
+            vm_spec,
+            environment.unwrap_or(&vm.environment_spec()),
+        )
+        .await
+    }
+}
+
 // The framework implementation includes some "runner-only" functions
 // (constructing and resetting a framework) that are marked `pub`. This could be
 // improved by splitting the "test case" functions into a trait and giving test
@@ -209,6 +327,12 @@ impl Framework {
         })
     }
 
+    pub fn test_ctx(self: &Arc<Self>, fully_qualified_name: String) -> TestCtx {
+        let output_dir =
+            self.tmp_directory.as_path().join(&fully_qualified_name);
+        TestCtx { framework: self.clone(), output_dir }
+    }
+
     /// Resets the state of any stateful objects in the framework to prepare it
     /// to run a new test case.
     pub async fn reset(&self) {
@@ -227,85 +351,12 @@ impl Framework {
             &self.default_guest_os_artifact,
         )
     }
-
     /// Yields an environment builder with default settings (run the VM on the
     /// test runner's machine using the default Propolis from the command line).
     pub fn environment_builder(&self) -> EnvironmentSpec {
         EnvironmentSpec::new(VmLocation::Local, DEFAULT_PROPOLIS_ARTIFACT)
     }
 
-    /// Spawns a test VM using the default configuration returned from
-    /// `vm_builder` and the default environment returned from
-    /// `environment_builder`.
-    pub async fn spawn_default_vm(
-        &self,
-        vm_name: &str,
-    ) -> anyhow::Result<TestVm> {
-        self.spawn_vm(&self.vm_config_builder(vm_name), None).await
-    }
-
-    /// Spawns a new test VM using the supplied `config`. 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(
-        &self,
-        config: &VmConfig<'_>,
-        environment: Option<&EnvironmentSpec>,
-    ) -> anyhow::Result<TestVm> {
-        self.spawn_vm_with_spec(
-            config
-                .vm_spec(self)
-                .await
-                .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
-        .context("constructing test VM")
-    }
-
-    /// Spawns a "successor" to the supplied `vm`. The successor has the same
-    /// configuration and takes additional references to all of its
-    /// predecessor's backing objects (e.g. disk handles). If `environment` is
-    /// `None`, the successor is launched using the predecessor's environment
-    /// spec.
-    pub async fn spawn_successor_vm(
-        &self,
-        vm_name: &str,
-        vm: &TestVm,
-        environment: Option<&EnvironmentSpec>,
-    ) -> anyhow::Result<TestVm> {
-        let mut vm_spec = vm.vm_spec().clone();
-        vm_spec.set_vm_name(vm_name.to_owned());
-
-        // Create new metadata for an instance based on this predecessor. It
-        // should have the same project and silo IDs, but the sled identifiers
-        // will be different.
-        vm_spec.refresh_sled_identifiers();
-
-        TestVm::new(
-            self,
-            vm_spec,
-            environment.unwrap_or(&vm.environment_spec()),
-        )
-        .await
-    }
-
     /// Yields this framework instance's default guest OS artifact name. This
     /// can be used to configure boot disks with different parameters than the
     /// builder defaults.

phd-tests/framework/src/lifecycle.rs

@@ -7,7 +7,7 @@ use futures::future::BoxFuture;
 use tracing::info;
 use uuid::Uuid;
 
-use crate::{test_vm::MigrationTimeout, Framework, TestVm};
+use crate::{test_vm::MigrationTimeout, TestCtx, TestVm};
 
 /// The set of actions that can be taken on a VM undergoing lifecycle testing.
 pub enum Action<'a> {
@@ -31,11 +31,11 @@ pub enum Action<'a> {
     MigrateToPropolis(&'a str),
 }
 
-impl Framework {
+impl TestCtx {
     /// Runs a lifecycle test on the supplied `vm` by iterating over the
     /// `actions`, performing the specified action, and then calling `check_fn`
     /// on the resulting VM to verify invariants.
-    pub async fn lifecycle_test<'a>(
+    pub async fn lifecycle_test(
         &self,
         vm: TestVm,
         actions: &[Action<'_>],

phd-tests/framework/src/log_config.rs

@@ -7,6 +7,7 @@
 
 use std::{path::Path, process::Stdio, str::FromStr};
 
+use anyhow::Context;
 use tracing::info;
 
 /// Specifies how a test's logging should be managed.
@@ -51,9 +52,12 @@ impl OutputMode {
     ///
     /// # Parameters
     ///
-    /// - directory: The directory in which to store any files written under
+    /// - `directory`: The directory in which to store any files written under
     ///   the selected discipline.
-    /// - file_prefix: The prefix to add to the names of any files written
+    ///
+    ///   If this directory does not already exist, it (and any parents) will
+    ///   be created.
+    /// - `file_prefix`: The prefix to add to the names of any files written
     ///   under the selected discipline.
     pub(crate) fn get_handles(
         &self,
@@ -62,17 +66,26 @@ impl OutputMode {
     ) -> anyhow::Result<(Stdio, Stdio)> {
         match self {
             OutputMode::TmpFile => {
-                let mut stdout_path = directory.as_ref().to_path_buf();
-                stdout_path.push(format!("{file_prefix}.stdout.log"));
+                let directory = directory.as_ref();
 
-                let mut stderr_path = directory.as_ref().to_path_buf();
-                stderr_path.push(format!("{file_prefix}.stderr.log"));
+                // Make sure the output dir actually exists before creating log
+                // files.
+                std::fs::create_dir_all(directory).with_context(|| {
+                    format!(
+                        "failed to create log file directory {}",
+                        directory.display()
+                    )
+                })?;
 
+                let stdout_path =
+                    directory.join(format!("{file_prefix}.stdout.log"));
+                let stderr_path =
+                    directory.join(format!("{file_prefix}.stderr.log"));
                 info!(?stdout_path, ?stderr_path, "Opening server log files");
-                Ok((
-                    std::fs::File::create(stdout_path)?.into(),
-                    std::fs::File::create(stderr_path)?.into(),
-                ))
+                let stdout = create_file(&stdout_path)?.into();
+                let stderr = create_file(&stderr_path)?.into();
+
+                Ok((stdout, stderr))
             }
             OutputMode::Stdio => Ok((Stdio::inherit(), Stdio::inherit())),
             OutputMode::Null => Ok((Stdio::null(), Stdio::null())),
@@ -90,3 +103,9 @@ pub enum LogFormat {
     /// as in CI).
     Bunyan,
 }
+
+fn create_file(path: &impl AsRef<Path>) -> anyhow::Result<std::fs::File> {
+    let path = path.as_ref();
+    std::fs::File::create(path)
+        .with_context(|| format!("failed to create file {}", path.display()))
+}