Test oximeter metrics end to end (#855)

Other tests in Propolis validate that the Oximeter translation of kstats
for a VM are coherent but do so from stubbed kstat readings. Those tests
are fine (and quick!), but don't cover the risk of a translation error
in Propolis reading from a real system, nor the possibility of reading
kstats incorrectly in the first place.

... good news: test doesn't catch anything interesting! The bug I was
looking at is somewhere else.

Author: iximeow

Cargo.lock

@@ -16,13 +16,16 @@ bhyve_api.workspace = true
 camino = { workspace = true, features = ["serde1"] }
 cfg-if.workspace = true
 cpuid_utils.workspace = true
+dropshot.workspace = true
 errno.workspace = true
 fatfs.workspace = true
 futures.workspace = true
 flate2.workspace = true
 hex.workspace = true
 libc.workspace = true
 newtype_derive.workspace = true
+omicron-common.workspace = true
+oximeter.workspace = true
 propolis-client.workspace = true
 reqwest = { workspace = true, features = ["blocking"] }
 ring.workspace = true
@@ -31,6 +34,7 @@ serde_derive.workspace = true
 serde_json.workspace = true
 slog.workspace = true
 slog-async.workspace = true
+slog-bunyan.workspace = true
 slog-term.workspace = true
 tar.workspace = true
 termwiz.workspace = true

phd-tests/framework/Cargo.toml

@@ -21,9 +21,7 @@ use tracing::{error, info};
 use uuid::Uuid;
 
 use super::BlockSize;
-use crate::{
-    disk::DeviceName, guest_os::GuestOsKind, server_log_mode::ServerLogMode,
-};
+use crate::{disk::DeviceName, guest_os::GuestOsKind, log_config::LogConfig};
 
 /// An RAII wrapper around a directory containing Crucible data files. Deletes
 /// the directory and its contents when dropped.
@@ -93,7 +91,7 @@ impl CrucibleDisk {
         data_dir_root: &impl AsRef<Path>,
         read_only_parent: Option<&impl AsRef<Path>>,
         guest_os: Option<GuestOsKind>,
-        log_mode: ServerLogMode,
+        log_config: LogConfig,
     ) -> anyhow::Result<Self> {
         Ok(Self {
             device_name,
@@ -106,7 +104,7 @@ impl CrucibleDisk {
                 downstairs_ports,
                 data_dir_root,
                 read_only_parent,
-                log_mode,
+                log_config,
             )?),
         })
     }
@@ -210,7 +208,7 @@ impl Inner {
         downstairs_ports: &[u16],
         data_dir_root: &impl AsRef<Path>,
         read_only_parent: Option<&impl AsRef<Path>>,
-        log_mode: ServerLogMode,
+        log_config: LogConfig,
     ) -> 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
@@ -335,7 +333,12 @@ impl Inner {
                 dir_arg.as_ref(),
             ];
 
-            let (stdout, stderr) = log_mode.get_handles(
+            // NOTE: `log_format` is ignored here because Crucible determines
+            // Bunyan or plain formatting based on `atty::is()`. In practice
+            // this is fine, and matches what we want right now, but it might be
+            // 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,
                 &format!("crucible_{}_{}", disk_uuid, port),
             )?;

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

@@ -19,8 +19,8 @@ use thiserror::Error;
 use crate::{
     artifacts::ArtifactStore,
     guest_os::GuestOsKind,
+    log_config::LogConfig,
     port_allocator::{PortAllocator, PortAllocatorError},
-    server_log_mode::ServerLogMode,
 };
 
 use self::{crucible::CrucibleDisk, file::FileBackedDisk};
@@ -190,7 +190,7 @@ pub(crate) struct DiskFactory {
     port_allocator: Arc<PortAllocator>,
 
     /// The logging discipline to use for Crucible server processes.
-    log_mode: ServerLogMode,
+    log_config: LogConfig,
 }
 
 impl DiskFactory {
@@ -201,13 +201,13 @@ impl DiskFactory {
         storage_dir: &impl AsRef<Utf8Path>,
         artifact_store: Arc<ArtifactStore>,
         port_allocator: Arc<PortAllocator>,
-        log_mode: ServerLogMode,
+        log_config: LogConfig,
     ) -> Self {
         Self {
             storage_dir: storage_dir.as_ref().to_path_buf(),
             artifact_store,
             port_allocator,
-            log_mode,
+            log_config,
         }
     }
 }
@@ -325,7 +325,7 @@ impl DiskFactory {
             &self.storage_dir,
             artifact_path.as_ref(),
             guest_os,
-            self.log_mode,
+            self.log_config,
         )
         .map(Arc::new)
         .map_err(Into::into)

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

@@ -36,8 +36,8 @@ use camino::Utf8PathBuf;
 use disk::DiskFactory;
 use futures::{stream::FuturesUnordered, StreamExt};
 use guest_os::GuestOsKind;
+use log_config::LogConfig;
 use port_allocator::PortAllocator;
-use server_log_mode::ServerLogMode;
 pub use test_vm::TestVm;
 use test_vm::{
     environment::EnvironmentSpec, spec::VmSpec, VmConfig, VmLocation,
@@ -52,16 +52,16 @@ pub mod disk;
 pub mod guest_os;
 pub mod host_api;
 pub mod lifecycle;
+pub mod log_config;
 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 {
     pub(crate) tmp_directory: Utf8PathBuf,
-    pub(crate) server_log_mode: ServerLogMode,
+    pub(crate) log_config: LogConfig,
 
     pub(crate) default_guest_cpus: u8,
     pub(crate) default_guest_memory_mib: u64,
@@ -99,7 +99,7 @@ pub struct FrameworkParameters<'a> {
     pub tmp_directory: Utf8PathBuf,
     pub artifact_directory: Utf8PathBuf,
     pub artifact_toml: Utf8PathBuf,
-    pub server_log_mode: ServerLogMode,
+    pub log_config: LogConfig,
 
     pub default_guest_cpus: u8,
     pub default_guest_memory_mib: u64,
@@ -187,14 +187,14 @@ impl Framework {
             &params.tmp_directory,
             artifact_store.clone(),
             port_allocator.clone(),
-            params.server_log_mode,
+            params.log_config,
         );
 
         let (cleanup_task_tx, cleanup_task_rx) =
             tokio::sync::mpsc::unbounded_channel();
         Ok(Self {
             tmp_directory: params.tmp_directory,
-            server_log_mode: params.server_log_mode,
+            log_config: params.log_config,
             default_guest_cpus: params.default_guest_cpus,
             default_guest_memory_mib: params.default_guest_memory_mib,
             default_guest_os_artifact: params.default_guest_os_artifact,

phd-tests/framework/src/lib.rs

@@ -2,16 +2,23 @@
 // 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/.
 
-//! Types and helpers specifying where a server process's stdout/stderr should
-//! be recorded.
+//! Types and helpers specifying how logs should be formatted and where they
+//! should be directed.
 
 use std::{path::Path, process::Stdio, str::FromStr};
 
 use tracing::info;
 
-/// Specifies where a process's output should be written.
+/// Specifies how a test's logging should be managed.
 #[derive(Debug, Clone, Copy)]
-pub enum ServerLogMode {
+pub struct LogConfig {
+    pub output_mode: OutputMode,
+    pub log_format: LogFormat,
+}
+
+/// Specifies where a output for a test's processes should be written.
+#[derive(Debug, Clone, Copy)]
+pub enum OutputMode {
     /// Write to files in the server's factory's temporary directory.
     TmpFile,
 
@@ -22,14 +29,14 @@ pub enum ServerLogMode {
     Null,
 }
 
-impl FromStr for ServerLogMode {
+impl FromStr for OutputMode {
     type Err = std::io::Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         match s.to_lowercase().as_str() {
-            "file" | "tmpfile" => Ok(ServerLogMode::TmpFile),
-            "stdio" => Ok(ServerLogMode::Stdio),
-            "null" => Ok(ServerLogMode::Null),
+            "file" | "tmpfile" => Ok(OutputMode::TmpFile),
+            "stdio" => Ok(OutputMode::Stdio),
+            "null" => Ok(OutputMode::Null),
             _ => Err(std::io::Error::new(
                 std::io::ErrorKind::InvalidData,
                 s.to_string(),
@@ -38,7 +45,7 @@ impl FromStr for ServerLogMode {
     }
 }
 
-impl ServerLogMode {
+impl OutputMode {
     /// Returns the stdout/stderr handles to pass to processes using the
     /// specified logging mode.
     ///
@@ -54,7 +61,7 @@ impl ServerLogMode {
         file_prefix: &str,
     ) -> anyhow::Result<(Stdio, Stdio)> {
         match self {
-            ServerLogMode::TmpFile => {
+            OutputMode::TmpFile => {
                 let mut stdout_path = directory.as_ref().to_path_buf();
                 stdout_path.push(format!("{}.stdout.log", file_prefix));
 
@@ -67,8 +74,19 @@ impl ServerLogMode {
                     std::fs::File::create(stderr_path)?.into(),
                 ))
             }
-            ServerLogMode::Stdio => Ok((Stdio::inherit(), Stdio::inherit())),
-            ServerLogMode::Null => Ok((Stdio::null(), Stdio::null())),
+            OutputMode::Stdio => Ok((Stdio::inherit(), Stdio::inherit())),
+            OutputMode::Null => Ok((Stdio::null(), Stdio::null())),
         }
     }
 }
+
+/// Specifies how output for a test's processes should be structured.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum LogFormat {
+    /// Format logs as plain hopefully human-readable output.
+    Plain,
+
+    /// Format logs as Bunyan output, more suitable for machine processing (such
+    /// as in CI).
+    Bunyan,
+}

phd-tests/framework/src/server_log_mode.rs phd-tests/framework/src/log_config.rs

@@ -16,15 +16,31 @@ pub enum VmLocation {
     // TODO: Support remote VMs.
 }
 
+/// Specifies where test VMs should report metrics to, if anywhere.
+#[derive(Clone, Copy, Debug)]
+pub enum MetricsLocation {
+    /// Oximeter metrics should be reported to a server colocated with the test
+    /// VM to be started.
+    Local,
+    // When the time comes to support remote VMs, it will presumably be useful
+    // to have local and (perhaps multiple) remote VMs report metrics to the
+    // same server. But we don't support remote VMs yet.
+}
+
 #[derive(Clone, Debug)]
 pub struct EnvironmentSpec {
     pub(crate) location: VmLocation,
     pub(crate) propolis_artifact: String,
+    pub(crate) metrics: Option<MetricsLocation>,
 }
 
 impl EnvironmentSpec {
     pub(crate) fn new(location: VmLocation, propolis_artifact: &str) -> Self {
-        Self { location, propolis_artifact: propolis_artifact.to_owned() }
+        Self {
+            location,
+            propolis_artifact: propolis_artifact.to_owned(),
+            metrics: None,
+        }
     }
 
     pub fn location(&mut self, location: VmLocation) -> &mut Self {
@@ -37,6 +53,11 @@ impl EnvironmentSpec {
         self
     }
 
+    pub fn metrics(&mut self, metrics: Option<MetricsLocation>) -> &mut Self {
+        self.metrics = metrics;
+        self
+    }
+
     pub(crate) async fn build<'a>(
         &self,
         framework: &'a Framework,
@@ -75,18 +96,31 @@ impl<'a> Environment<'a> {
                     .port_allocator
                     .next()
                     .context("getting VNC server port")?;
+                let metrics_addr = builder.metrics.and_then(|m| match m {
+                    MetricsLocation::Local => {
+                        // If the test requests metrics are local, we'll start
+                        // an Oximeter stand-in for this VM when setting up this
+                        // environment later. `start_local_vm` will patch in the
+                        // actual server address when it has created one.
+                        //
+                        // If the VM is to be started remotely but requests
+                        // "Local" metrics, that's probably an error.
+                        None
+                    }
+                });
                 let params = ServerProcessParameters {
                     server_path: propolis_server,
                     data_dir: framework.tmp_directory.as_path(),
                     server_addr: SocketAddrV4::new(
                         Ipv4Addr::new(127, 0, 0, 1),
                         server_port,
                     ),
+                    metrics_addr,
                     vnc_addr: SocketAddrV4::new(
                         Ipv4Addr::new(127, 0, 0, 1),
                         vnc_port,
                     ),
-                    log_mode: framework.server_log_mode,
+                    log_config: framework.log_config,
                 };
                 Ok(Self::Local(params))
             }