From d42625c37986fa11ee5b6b25b56f037b0b3f41f2 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 28 Jul 2022 10:41:27 -0700 Subject: [PATCH 1/5] add affordance to allow multiple PHD VMs to be created in one test case --- phd-tests/framework/src/test_vm/factory.rs | 35 +++++++++++++++++++++- phd-tests/framework/src/test_vm/mod.rs | 17 +++++------ phd-tests/runner/src/main.rs | 1 + 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/phd-tests/framework/src/test_vm/factory.rs b/phd-tests/framework/src/test_vm/factory.rs index 11ad24c9b..3309c213c 100644 --- a/phd-tests/framework/src/test_vm/factory.rs +++ b/phd-tests/framework/src/test_vm/factory.rs @@ -2,8 +2,10 @@ use std::{ net::{Ipv4Addr, SocketAddrV4}, + ops::Range, path::PathBuf, str::FromStr, + sync::atomic::{AtomicU16, Ordering}, }; use anyhow::Result; @@ -93,6 +95,9 @@ pub struct FactoryOptions { /// The default amount of memory to set in [`vm_config::VmConfig`] structs /// generated by this factory. pub default_guest_memory_mib: u64, + + /// The range of ports to assign to servers created by this factory. + pub server_port_range: Range, } /// A VM factory that provides routines to generate new test VMs. @@ -101,6 +106,19 @@ pub struct VmFactory { default_guest_image_path: String, default_guest_kind: GuestOsKind, default_bootrom_path: String, + + // The test runner executes test cases in a `catch_unwind` block to allow + // those cases to use macros like `assert!` and `panic!`. This requires that + // arguments to test cases (like the `TestContext` and, by extension, the VM + // factory) be `UnwindSafe`. This in turn means that the factory cannot be + // passed by mutable reference or contain any interiorly mutable types that + // are not themselves `UnwindSafe` (e.g. `RefCell`). + // + // `AtomicU16` is unwind-safe and less expensive to manipulate than a full + // `Mutex`. If and when the factory needs more complex mutable state, this + // should move to an `Inner` struct guarded by an unwind-safe primitive like + // Mutex. + next_port: AtomicU16, } impl VmFactory { @@ -120,14 +138,23 @@ impl VmFactory { opts.default_bootrom_artifact.clone(), ))?; + let first_port = opts.server_port_range.start; Ok(Self { opts, default_guest_image_path: guest_path.to_string_lossy().to_string(), default_guest_kind: kind, default_bootrom_path: bootrom_path.to_string_lossy().to_string(), + next_port: AtomicU16::new(first_port), }) } + /// Resets this factory to the state it had when it was created, preparing + /// it for use in a new test case. + pub fn reset(&mut self) { + self.next_port + .store(self.opts.server_port_range.start, Ordering::Relaxed); + } + /// Creates a VM configuration that specifies this factory's defaults for /// CPUs, memory, bootrom, and guest image. /// @@ -183,10 +210,16 @@ impl VmFactory { } }; + let server_port = self.next_port.fetch_add(1, Ordering::Relaxed); + let vnc_port = self.next_port.fetch_add(1, Ordering::Relaxed); let server_params = ServerProcessParameters { server_path: &self.opts.propolis_server_path, config_toml_path: &config_toml_path.as_os_str().to_string_lossy(), - server_addr: SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 9000), + server_addr: SocketAddrV4::new( + Ipv4Addr::new(127, 0, 0, 1), + server_port, + ), + vnc_addr: SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), vnc_port), server_stdout, server_stderr, }; diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 1d1e986bb..976f82bf7 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -1,12 +1,7 @@ //! Routines for starting VMs, changing their states, and interacting with their //! guest OSes. -use std::{ - fmt::Debug, - net::{Ipv4Addr, SocketAddrV4}, - process::Stdio, - time::Duration, -}; +use std::{fmt::Debug, net::SocketAddrV4, process::Stdio, time::Duration}; use crate::guest_os::{self, CommandSequenceEntry, GuestOs, GuestOsKind}; use crate::serial::SerialConsole; @@ -71,6 +66,9 @@ pub struct ServerProcessParameters<'a, T: Into + Debug> { /// The address at which the server should serve. pub server_addr: SocketAddrV4, + /// The address at which the server should offer its VNC server. + pub vnc_addr: SocketAddrV4, + /// The [`Stdio`] descriptor to which the server's stdout should be /// directed. pub server_stdout: T, @@ -116,6 +114,7 @@ impl TestVm { server_path, config_toml_path, server_addr, + vnc_addr, server_stdout, server_stderr, } = process_params; @@ -134,6 +133,7 @@ impl TestVm { "run", config_toml_path, server_addr.to_string().as_str(), + vnc_addr.to_string().as_str(), ]) .stdout(server_stdout) .stderr(server_stderr) @@ -148,10 +148,7 @@ impl TestVm { let client_async_drain = slog_async::Async::new(client_drain).build().fuse(); let client = Client::new( - std::net::SocketAddr::V4(SocketAddrV4::new( - Ipv4Addr::new(127, 0, 0, 1), - 9000, - )), + server_addr.into(), slog::Logger::root(client_async_drain, slog::o!()), ); diff --git a/phd-tests/runner/src/main.rs b/phd-tests/runner/src/main.rs index bbb48d13b..a6e693bce 100644 --- a/phd-tests/runner/src/main.rs +++ b/phd-tests/runner/src/main.rs @@ -51,6 +51,7 @@ fn run_tests(run_opts: &RunOptions) { default_bootrom_artifact: run_opts.default_bootrom_artifact.clone(), default_guest_cpus: run_opts.default_guest_cpus, default_guest_memory_mib: run_opts.default_guest_memory_mib, + server_port_range: 9000..10000, }; // The VM factory config and artifact store are enough to create a test From a86d67857587deb856c704ffadb255e88d1c0de3 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 28 Jul 2022 11:17:57 -0700 Subject: [PATCH 2/5] Reset the VM factory in a fixture --- phd-tests/framework/src/test_vm/factory.rs | 19 +++++++------------ phd-tests/runner/src/execute.rs | 2 +- phd-tests/runner/src/fixtures.rs | 7 ++++++- phd-tests/runner/src/main.rs | 5 +++-- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/phd-tests/framework/src/test_vm/factory.rs b/phd-tests/framework/src/test_vm/factory.rs index 3309c213c..70364d9ff 100644 --- a/phd-tests/framework/src/test_vm/factory.rs +++ b/phd-tests/framework/src/test_vm/factory.rs @@ -107,17 +107,12 @@ pub struct VmFactory { default_guest_kind: GuestOsKind, default_bootrom_path: String, - // The test runner executes test cases in a `catch_unwind` block to allow - // those cases to use macros like `assert!` and `panic!`. This requires that - // arguments to test cases (like the `TestContext` and, by extension, the VM - // factory) be `UnwindSafe`. This in turn means that the factory cannot be - // passed by mutable reference or contain any interiorly mutable types that - // are not themselves `UnwindSafe` (e.g. `RefCell`). - // - // `AtomicU16` is unwind-safe and less expensive to manipulate than a full - // `Mutex`. If and when the factory needs more complex mutable state, this - // should move to an `Inner` struct guarded by an unwind-safe primitive like - // Mutex. + // Mutable state in the VM factory must be unwind-safe because the runner + // passes the factory to test cases (via their test contexts), and test + // cases run in a `catch_unwind` block to enable the use of `assert!` and + // `panic!`. For assigning sequential port numbers, `AtomicU16` fits the + // bill without requiring the extra interlocked operations needed to acquire + // and release an entire `Mutex`. next_port: AtomicU16, } @@ -150,7 +145,7 @@ impl VmFactory { /// Resets this factory to the state it had when it was created, preparing /// it for use in a new test case. - pub fn reset(&mut self) { + pub fn reset(&self) { self.next_port .store(self.opts.server_port_range.start, Ordering::Relaxed); } diff --git a/phd-tests/runner/src/execute.rs b/phd-tests/runner/src/execute.rs index 995fc5c57..6d3fb8e35 100644 --- a/phd-tests/runner/src/execute.rs +++ b/phd-tests/runner/src/execute.rs @@ -53,7 +53,7 @@ thread_local! { /// Executes a set of tests using the supplied test context. pub fn run_tests_with_ctx<'fix>( - ctx: TestContext, + ctx: &TestContext, mut fixtures: TestFixtures, run_opts: &RunOptions, ) -> ExecutionStats { diff --git a/phd-tests/runner/src/fixtures.rs b/phd-tests/runner/src/fixtures.rs index e044f8cb4..9cb47469d 100644 --- a/phd-tests/runner/src/fixtures.rs +++ b/phd-tests/runner/src/fixtures.rs @@ -2,12 +2,15 @@ use anyhow::Result; use phd_framework::artifacts::ArtifactStore; use tracing::instrument; +use crate::TestContext; + use super::config; use super::zfs::ZfsFixture; /// A wrapper containing the objects needed to run the executor's test fixtures. pub struct TestFixtures<'a> { artifact_store: &'a ArtifactStore, + test_context: &'a TestContext, zfs: Option, } @@ -17,6 +20,7 @@ impl<'a> TestFixtures<'a> { pub fn new( run_opts: &config::RunOptions, artifact_store: &'a ArtifactStore, + test_context: &'a TestContext, ) -> Result { let zfs = run_opts .zfs_fs_name @@ -30,7 +34,7 @@ impl<'a> TestFixtures<'a> { }) .transpose()?; - Ok(Self { artifact_store, zfs }) + Ok(Self { artifact_store, test_context, zfs }) } /// Calls fixture routines that need to run before any tests run. @@ -73,6 +77,7 @@ impl<'a> TestFixtures<'a> { /// corresponding setup fixture has run. #[instrument(skip_all)] pub fn test_cleanup(&mut self) -> Result<()> { + self.test_context.vm_factory.reset(); if let Some(zfs) = &mut self.zfs { zfs.rollback_to_artifact_snapshot() } else { diff --git a/phd-tests/runner/src/main.rs b/phd-tests/runner/src/main.rs index a6e693bce..6112bb446 100644 --- a/phd-tests/runner/src/main.rs +++ b/phd-tests/runner/src/main.rs @@ -63,10 +63,11 @@ fn run_tests(run_opts: &RunOptions) { ) .unwrap(), }; - let fixtures = TestFixtures::new(&run_opts, &artifact_store).unwrap(); + let fixtures = TestFixtures::new(&run_opts, &artifact_store, &ctx).unwrap(); // Run the tests and print results. - let execution_stats = execute::run_tests_with_ctx(ctx, fixtures, &run_opts); + let execution_stats = + execute::run_tests_with_ctx(&ctx, fixtures, &run_opts); if execution_stats.failed_test_cases.len() != 0 { println!("\nfailures:"); for tc in execution_stats.failed_test_cases { From c33888792bf97172b21cbcfee992c9e683cf2246 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 28 Jul 2022 11:34:08 -0700 Subject: [PATCH 3/5] Replace uname test with smoke test for running multiple VMs --- phd-tests/tests/src/smoke.rs | 41 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/phd-tests/tests/src/smoke.rs b/phd-tests/tests/src/smoke.rs index 242576ad1..c6bd1dc21 100644 --- a/phd-tests/tests/src/smoke.rs +++ b/phd-tests/tests/src/smoke.rs @@ -1,24 +1,4 @@ -use phd_testcase::{phd_framework::guest_os::GuestOsKind, *}; - -#[phd_testcase] -fn uname_test(ctx: &TestContext) { - let vm = ctx - .vm_factory - .new_vm("uname_test", ctx.vm_factory.default_vm_config())?; - vm.run()?; - vm.wait_to_boot()?; - - vm.run_shell_command("echo $SHELL")?; - - let uname = vm.run_shell_command("uname -r")?; - assert_eq!( - uname, - match vm.guest_os_kind() { - GuestOsKind::Alpine => "5.15.41-0-virt", - GuestOsKind::Debian11NoCloud => "5.10.0-16-amd64", - } - ); -} +use phd_testcase::*; #[phd_testcase] fn nproc_test(ctx: &TestContext) { @@ -31,3 +11,22 @@ fn nproc_test(ctx: &TestContext) { let nproc = vm.run_shell_command("nproc")?; assert_eq!(nproc.parse::().unwrap(), 6); } + +#[phd_testcase] +fn multiple_vms_test(ctx: &TestContext) { + let vms = (0..5) + .into_iter() + .map(|i| { + let name = format!("vm{}", i); + ctx.vm_factory.new_vm(&name, ctx.vm_factory.default_vm_config()) + }) + .collect::, _>>()?; + + for vm in &vms { + vm.run()?; + } + + for vm in &vms { + vm.wait_to_boot()?; + } +} From d2eab0d732dbcb7bb7d0863136cecd4931e59e33 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 28 Jul 2022 11:54:35 -0700 Subject: [PATCH 4/5] Clean up some span misuse revealed by multi-VM test --- phd-tests/framework/src/serial/vt100.rs | 17 +++++++++++------ phd-tests/framework/src/test_vm/mod.rs | 7 +++---- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/phd-tests/framework/src/serial/vt100.rs b/phd-tests/framework/src/serial/vt100.rs index 457ce70a6..436d888a8 100644 --- a/phd-tests/framework/src/serial/vt100.rs +++ b/phd-tests/framework/src/serial/vt100.rs @@ -6,7 +6,7 @@ use tokio::{ sync::{mpsc, Mutex}, task::JoinHandle, }; -use tracing::{info, info_span, Instrument}; +use tracing::{debug, info, info_span, Instrument}; #[derive(Error, Debug)] pub enum Vt100Error { @@ -29,11 +29,15 @@ pub struct Vt100Processor { impl Vt100Processor { pub fn new(vt_rx: mpsc::Receiver>) -> Self { - let state = Arc::new(SharedState::default()); - let state_for_task = state.clone(); - let vt_span = info_span!("Serial"); vt_span.follows_from(tracing::Span::current()); + + let state = Arc::new(SharedState { + span: vt_span.clone(), + inner: Mutex::default(), + }); + + let state_for_task = state.clone(); let task = tokio::spawn( async move { vt100_handler(state_for_task, vt_rx).await; @@ -98,8 +102,8 @@ struct Waiter { /// State shared between the receiver task and the public interface to the VT /// processor. -#[derive(Default)] struct SharedState { + span: tracing::Span, inner: Mutex, } @@ -140,6 +144,7 @@ impl SharedState { wanted: String, output_tx: mpsc::Sender, ) -> Result<()> { + let _span = self.span.enter(); info!(wanted, "Registering wait for serial console output"); let mut guard = self.inner.lock().await; @@ -177,7 +182,7 @@ struct Inner { impl Drop for Inner { fn drop(&mut self) { - info!( + debug!( self.next_output_line, "Dropped serial console with partial line" ); diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 976f82bf7..f070f0f6b 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -247,9 +247,8 @@ impl TestVm { /// initial login prompt and the login prompt itself. pub fn wait_to_boot(&self) -> Result<()> { let timeout_duration = Duration::from_secs(300); - let wait_span = - info_span!("Waiting {} for guest to boot", ?timeout_duration); - wait_span.follows_from(&self.tracing_span); + let _span = self.tracing_span.enter(); + info!("Waiting {:?} for guest to boot", timeout_duration); let boot_sequence = self.guest_os.get_login_sequence(); let _ = self.rt.block_on(async { @@ -272,7 +271,7 @@ impl TestVm { } Ok::<(), anyhow::Error>(()) } - .instrument(wait_span), + .instrument(info_span!("wait_to_boot")), ) .await .map_err(|e| anyhow!(e)) From a81c829b7e4f2c252231a1bdd07a14db806200f9 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 28 Jul 2022 11:56:47 -0700 Subject: [PATCH 5/5] Use a somewhat more descriptive VM name in the multi-VM test --- phd-tests/tests/src/smoke.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phd-tests/tests/src/smoke.rs b/phd-tests/tests/src/smoke.rs index c6bd1dc21..3779d9120 100644 --- a/phd-tests/tests/src/smoke.rs +++ b/phd-tests/tests/src/smoke.rs @@ -17,7 +17,7 @@ fn multiple_vms_test(ctx: &TestContext) { let vms = (0..5) .into_iter() .map(|i| { - let name = format!("vm{}", i); + let name = format!("multiple_vms_test_vm{}", i); ctx.vm_factory.new_vm(&name, ctx.vm_factory.default_vm_config()) }) .collect::, _>>()?;