From 3091070a06ccd8c4c782d017095f09ca4fbed71a Mon Sep 17 00:00:00 2001 From: iximeow Date: Thu, 17 Oct 2024 16:22:17 +0000 Subject: [PATCH 1/8] check exit status of commands, have tests expect success/failure --- phd-tests/framework/src/test_vm/mod.rs | 79 ++++++++++++++++++++- phd-tests/tests/src/boot_order.rs | 8 ++- phd-tests/tests/src/boot_order/efi_utils.rs | 7 +- phd-tests/tests/src/cpuid.rs | 3 +- phd-tests/tests/src/crucible/migrate.rs | 30 +++++--- phd-tests/tests/src/crucible/smoke.rs | 11 +-- phd-tests/tests/src/disk.rs | 17 +++-- phd-tests/tests/src/framework.rs | 3 +- phd-tests/tests/src/hw.rs | 16 +++-- phd-tests/tests/src/migrate.rs | 42 +++++++---- phd-tests/tests/src/smoke.rs | 2 +- 11 files changed, 169 insertions(+), 49 deletions(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 2fd454e3e..c4a965d1c 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -142,6 +142,51 @@ enum VmState { Ensured { serial: SerialConsole }, } +/// Both the output and status of a command. +pub struct ShellOutput { + status: u16, + output: String, +} + +impl ShellOutput { + /// Consume this [`ShellOutput`], returning the command's output as text + /// if the command completed successfully, or an error if it did not. + pub fn expect_ok(self) -> Result { + if self.status == 0 { + Ok(self.output) + } else { + Err(anyhow!("command exited with non-zero status: {}", self.status)) + } + } + + /// Consume this [`ShellOutput`], returning the command's output as text + /// if the command exited with non-zero status, or an error if it exited + /// with status zero. + pub fn expect_err(self) -> Result { + if self.status != 0 { + Ok(self.output) + } else { + Err(anyhow!("command unexpectedly succeeded")) + } + } + + pub fn ignore_status(self) -> String { + self.output + } + + pub fn status(&self) -> u16 { + self.status + } + + /// Get the textual output of the command. You probably should use + /// [`ShellOutput::expect_ok`] or [`ShellOutput::expect_err`] to ensure + /// the command was processed as expected before asserting on its + /// output. + pub fn output(&self) -> &str { + self.output.as_str() + } +} + /// A virtual machine running in a Propolis server. Test cases create these VMs /// using the `factory::VmFactory` embedded in their test contexts. /// @@ -868,7 +913,21 @@ impl TestVm { /// waits for another shell prompt to appear using /// [`Self::wait_for_serial_output`] and returns any text that was buffered /// to the serial console after the command was sent. - pub async fn run_shell_command(&self, cmd: &str) -> Result { + /// + /// After running the shell command, sends `echo $?` to query and return the + /// command's return status as well. + // TO REVIEWERS: it would be really nice to write this as a function + // returning a `struct ShellCommandExecutor` that impls + // `Future` where the underlying ShellOutput is automatically + // `.expect_ok()`'d. In such a case it would be possible for the struct to + // have an `.expect_err()` that replaces teh default `.expect_ok()` + // behavior, so that the likely case doesn't need any change in PHD tests. + // + // unfortunately I don't know how to plumb the futures for that, since we'd + // have to close over `&self`, so doing any Boxing to hold an + // `async move {}` immediately causes issues. + #[must_use] + pub async fn run_shell_command(&self, cmd: &str) -> Result { // Allow the guest OS to transform the input command into a // guest-specific command sequence. This accounts for the guest's shell // type (which affects e.g. affects how it displays multi-line commands) @@ -881,7 +940,18 @@ impl TestVm { // before actually issuing the final '\n' that issues the command. // This ensures that the buffer contents returned by this call contain // only the command's output. - let out = self + let output = self + .wait_for_serial_output( + self.guest_os.get_shell_prompt(), + Duration::from_secs(300), + ) + .await?; + + let status_command_sequence = + self.guest_os.shell_command_sequence("echo $?"); + self.run_command_sequence(status_command_sequence).await?; + + let status = self .wait_for_serial_output( self.guest_os.get_shell_prompt(), Duration::from_secs(300), @@ -891,7 +961,10 @@ impl TestVm { // Trim any leading newlines inserted when the command was issued and // any trailing whitespace that isn't actually part of the command // output. Any other embedded whitespace is the caller's problem. - Ok(out.trim().to_string()) + let output = output.trim().to_string(); + let status = status.trim().parse::()?; + + Ok(ShellOutput { status, output }) } pub async fn graceful_reboot(&self) -> Result<()> { diff --git a/phd-tests/tests/src/boot_order.rs b/phd-tests/tests/src/boot_order.rs index 89f640390..5f7424c15 100644 --- a/phd-tests/tests/src/boot_order.rs +++ b/phd-tests/tests/src/boot_order.rs @@ -5,7 +5,7 @@ use anyhow::{bail, Error}; use phd_framework::{ disk::{fat::FatFilesystem, DiskSource}, - test_vm::{DiskBackend, DiskInterface}, + test_vm::{DiskBackend, DiskInterface, ShellOutput}, }; use phd_testcase::*; use std::io::Cursor; @@ -23,7 +23,7 @@ use efi_utils::{ pub(crate) async fn run_long_command( vm: &phd_framework::TestVm, cmd: &str, -) -> Result { +) -> Result { // Ok, this is a bit whacky: something about the line wrapping for long // commands causes `run_shell_command` to hang instead of ever actually // seeing a response prompt. @@ -284,7 +284,8 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) { // If the guest doesn't have an EFI partition then there's no way for boot // order preferences to be persisted. - let mountline = vm.run_shell_command("mount | grep efivarfs").await?; + let mountline = + vm.run_shell_command("mount | grep efivarfs").await?.ignore_status(); if !mountline.starts_with("efivarfs on ") { warn!( @@ -298,6 +299,7 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) { // reboot, and make sure they're all as we set them. if !run_long_command(&vm, &format!("ls {}", efipath(&bootvar(0xffff)))) .await? + .expect_err()? .is_empty() { warn!( diff --git a/phd-tests/tests/src/boot_order/efi_utils.rs b/phd-tests/tests/src/boot_order/efi_utils.rs index adf1c1f50..fabe77e87 100644 --- a/phd-tests/tests/src/boot_order/efi_utils.rs +++ b/phd-tests/tests/src/boot_order/efi_utils.rs @@ -326,7 +326,7 @@ pub(crate) async fn read_efivar( efipath(varname) ); - let hex = run_long_command(vm, &cmd).await?; + let hex = run_long_command(vm, &cmd).await?.expect_ok()?; Ok(unhex(&hex)) } @@ -345,7 +345,8 @@ pub(crate) async fn write_efivar( efipath(varname) ); - let attr_read_bytes = run_long_command(vm, &attr_cmd).await?; + let attr_read_bytes = + run_long_command(vm, &attr_cmd).await?.ignore_status(); let attrs = if attr_read_bytes.ends_with(": No such file or directory") { // Default attributes if the variable does not exist yet. We expect it // to be non-volatile because we are writing it, we expect it to be @@ -390,7 +391,7 @@ pub(crate) async fn write_efivar( efipath(varname) ); - let res = run_long_command(vm, &cmd).await?; + let res = run_long_command(vm, &cmd).await?.expect_ok()?; // If something went sideways and the write failed with something like // `invalid argument`... if !res.is_empty() { diff --git a/phd-tests/tests/src/cpuid.rs b/phd-tests/tests/src/cpuid.rs index 3ec773761..1205ce5a6 100644 --- a/phd-tests/tests/src/cpuid.rs +++ b/phd-tests/tests/src/cpuid.rs @@ -169,7 +169,8 @@ async fn cpuid_boot_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let cpuinfo = vm.run_shell_command("cat /proc/cpuinfo").await?; + let cpuinfo = + vm.run_shell_command("cat /proc/cpuinfo").await?.expect_ok()?; info!(cpuinfo, "/proc/cpuinfo output"); assert!(cpuinfo.contains( std::str::from_utf8(BRAND_STRING).unwrap().trim_matches('\0') diff --git a/phd-tests/tests/src/crucible/migrate.rs b/phd-tests/tests/src/crucible/migrate.rs index bb48c6150..681fcc8c6 100644 --- a/phd-tests/tests/src/crucible/migrate.rs +++ b/phd-tests/tests/src/crucible/migrate.rs @@ -19,10 +19,13 @@ async fn smoke_test(ctx: &Framework) { source.launch().await?; source.wait_to_boot().await?; - let lsout = source.run_shell_command("ls foo.bar 2> /dev/null").await?; + let lsout = source + .run_shell_command("ls foo.bar 2> /dev/null") + .await? + .expect_err()?; assert_eq!(lsout, ""); - source.run_shell_command("touch ./foo.bar").await?; - source.run_shell_command("sync ./foo.bar").await?; + source.run_shell_command("touch ./foo.bar").await?.expect_ok()?; + source.run_shell_command("sync ./foo.bar").await?.expect_ok()?; disk.set_generation(2); let mut target = ctx @@ -32,7 +35,7 @@ async fn smoke_test(ctx: &Framework) { target .migrate_from(&source, Uuid::new_v4(), MigrationTimeout::default()) .await?; - let lsout = target.run_shell_command("ls foo.bar").await?; + let lsout = target.run_shell_command("ls foo.bar").await?.expect_ok()?; assert_eq!(lsout, "foo.bar"); } @@ -63,17 +66,22 @@ async fn load_test(ctx: &Framework) { ) .as_str(), ) - .await?; + .await? + .expect_ok()?; assert!(ddout.contains(format!("{}+0 records in", block_count).as_str())); // Compute the data's hash. - let sha256sum_out = source.run_shell_command("sha256sum rand.txt").await?; + let sha256sum_out = + source.run_shell_command("sha256sum rand.txt").await?.expect_ok()?; let checksum = sha256sum_out.split_whitespace().next().unwrap(); info!("Generated SHA256 checksum: {}", checksum); // Start copying the generated file into a second file, then start a // migration while that copy is in progress. - source.run_shell_command("dd if=./rand.txt of=./rand_new.txt &").await?; + source + .run_shell_command("dd if=./rand.txt of=./rand_new.txt &") + .await? + .expect_ok()?; target .migrate_from(&source, Uuid::new_v4(), MigrationTimeout::default()) .await?; @@ -81,9 +89,11 @@ async fn load_test(ctx: &Framework) { // Wait for the background command to finish running, then compute the // hash of the copied file. If all went well this will match the hash of // the source file. - target.run_shell_command("wait $!").await?; - let sha256sum_target = - target.run_shell_command("sha256sum rand_new.txt").await?; + target.run_shell_command("wait $!").await?.expect_ok()?; + let sha256sum_target = target + .run_shell_command("sha256sum rand_new.txt") + .await? + .expect_ok()?; let checksum_target = sha256sum_target.split_whitespace().next().unwrap(); assert_eq!(checksum, checksum_target); } diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs index 2955d8bdc..d675b8439 100644 --- a/phd-tests/tests/src/crucible/smoke.rs +++ b/phd-tests/tests/src/crucible/smoke.rs @@ -37,6 +37,7 @@ async fn guest_reboot_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; + // XXX: use graceful_reboot() now. // Don't use `run_shell_command` because the guest won't echo another prompt // after this. vm.send_serial_str("reboot\n").await?; @@ -64,10 +65,11 @@ async fn shutdown_persistence_test(ctx: &Framework) { // Verify that the test file doesn't exist yet, then touch it, flush it, and // shut down the VM. - let lsout = vm.run_shell_command("ls foo.bar 2> /dev/null").await?; + let lsout = + vm.run_shell_command("ls foo.bar 2> /dev/null").await?.expect_err()?; assert_eq!(lsout, ""); - vm.run_shell_command("touch ./foo.bar").await?; - vm.run_shell_command("sync ./foo.bar").await?; + vm.run_shell_command("touch ./foo.bar").await?.expect_ok()?; + vm.run_shell_command("sync ./foo.bar").await?.expect_ok()?; vm.stop().await?; vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(60)) .await?; @@ -82,6 +84,7 @@ async fn shutdown_persistence_test(ctx: &Framework) { vm.wait_to_boot().await?; // The touched file from the previous VM should be present in the new one. - let lsout = vm.run_shell_command("ls foo.bar 2> /dev/null").await?; + let lsout = + vm.run_shell_command("ls foo.bar 2> /dev/null").await?.expect_ok()?; assert_eq!(lsout, "foo.bar"); } diff --git a/phd-tests/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index 1db141c90..ba73caf9b 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -37,9 +37,12 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) { // try to check that the disk is located there and fail the test early if // it's not. If the by-path directory is missing, put up a warning and hope // for the best. - let dev_disk = vm.run_shell_command("ls /dev/disk").await?; + let dev_disk = vm.run_shell_command("ls /dev/disk").await?.ignore_status(); if dev_disk.contains("by-path") { - let ls = vm.run_shell_command("ls -la /dev/disk/by-path").await?; + let ls = vm + .run_shell_command("ls -la /dev/disk/by-path") + .await? + .expect_ok()?; info!(%ls, "guest disk device paths"); assert!(ls.contains("virtio-pci-0000:00:18.0 -> ../../vda")); } else { @@ -49,17 +52,19 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) { ); } - vm.run_shell_command("mkdir /phd").await?; + vm.run_shell_command("mkdir /phd").await?.expect_ok()?; // The disk is read-only, so pass the `ro` option to `mount` so that it // doesn't complain about not being able to mount for writing. - let mount = vm.run_shell_command("mount -o ro /dev/vda /phd").await?; + let mount = + vm.run_shell_command("mount -o ro /dev/vda /phd").await?.expect_ok()?; assert_eq!(mount, ""); // The file should be there and have the expected contents. - let ls = vm.run_shell_command("ls /phd").await?; + let ls = vm.run_shell_command("ls /phd").await?.expect_ok()?; assert_eq!(ls, "hello_oxide.txt"); - let cat = vm.run_shell_command("cat /phd/hello_oxide.txt").await?; + let cat = + vm.run_shell_command("cat /phd/hello_oxide.txt").await?.expect_ok()?; assert_eq!(cat, HELLO_MSG); } diff --git a/phd-tests/tests/src/framework.rs b/phd-tests/tests/src/framework.rs index 3f1cb8cab..f9ae012d8 100644 --- a/phd-tests/tests/src/framework.rs +++ b/phd-tests/tests/src/framework.rs @@ -13,6 +13,7 @@ async fn multiline_serial_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let out = vm.run_shell_command("echo \\\nhello \\\nworld").await?; + let out = + vm.run_shell_command("echo \\\nhello \\\nworld").await?.expect_ok()?; assert_eq!(out, "hello world"); } diff --git a/phd-tests/tests/src/hw.rs b/phd-tests/tests/src/hw.rs index b322d63d8..4ce27bea9 100644 --- a/phd-tests/tests/src/hw.rs +++ b/phd-tests/tests/src/hw.rs @@ -17,15 +17,23 @@ async fn lspci_lifecycle_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let lspci = vm.run_shell_command(LSPCI).await?; - let lshw = vm.run_shell_command(LSHW).await?; + // XXX: do not `ignore_status()` on these commands! They fail for any number + // of reasons on different guests: + // * sudo may not exist (some Alpine) + // * lshw may not exist (Debian) + // * we may not input a sudo password (Ubuntu) + + let lspci = vm.run_shell_command(LSPCI).await?.ignore_status(); + let lshw = vm.run_shell_command(LSHW).await?.ignore_status(); ctx.lifecycle_test(vm, &[Action::StopAndStart], move |vm| { let lspci = lspci.clone(); let lshw = lshw.clone(); Box::pin(async move { - let new_lspci = vm.run_shell_command(LSPCI).await.unwrap(); + let new_lspci = + vm.run_shell_command(LSPCI).await.unwrap().ignore_status(); assert_eq!(new_lspci, lspci); - let new_lshw = vm.run_shell_command(LSHW).await.unwrap(); + let new_lshw = + vm.run_shell_command(LSHW).await.unwrap().ignore_status(); assert_eq!(new_lshw, lshw); }) }) diff --git a/phd-tests/tests/src/migrate.rs b/phd-tests/tests/src/migrate.rs index 78438be79..e8b0898f1 100644 --- a/phd-tests/tests/src/migrate.rs +++ b/phd-tests/tests/src/migrate.rs @@ -55,12 +55,16 @@ mod from_base { spawn_base_vm(ctx, "migration_from_base_and_back").await?; source.launch().await?; source.wait_to_boot().await?; - let lsout = source.run_shell_command("ls foo.bar 2> /dev/null").await?; + // `ls` with no results exits non-zero, so expect an error here. + let lsout = source + .run_shell_command("ls foo.bar 2> /dev/null") + .await? + .expect_err()?; assert_eq!(lsout, ""); // create an empty file on the source VM. - source.run_shell_command("touch ./foo.bar").await?; - source.run_shell_command("sync ./foo.bar").await?; + source.run_shell_command("touch ./foo.bar").await?.expect_ok()?; + source.run_shell_command("sync ./foo.bar").await?.expect_ok()?; ctx.lifecycle_test( source, @@ -75,7 +79,8 @@ mod from_base { let lsout = target .run_shell_command("ls foo.bar") .await - .expect("`ls foo.bar` should succeed"); + .expect("`ls foo.bar` should succeed") + .ignore_status(); assert_eq!(lsout, "foo.bar"); }) }, @@ -256,8 +261,10 @@ mod running_process { "\nEOF" )) .await?; - vm.run_shell_command("chmod +x dirt.sh").await?; - let run_dirt = vm.run_shell_command("./dirt.sh").await?; + vm.run_shell_command("chmod +x dirt.sh").await?.expect_ok()?; + // When dirt.sh suspends itself, the parent shell will report a non-zero + // status (148, in particular: 128 + SIGTSTP aka 20 for Linux guests). + let run_dirt = vm.run_shell_command("./dirt.sh").await?.expect_err()?; assert!(run_dirt.contains("made dirt"), "dirt.sh failed: {run_dirt:?}"); assert!( run_dirt.contains("Stopped"), @@ -268,7 +275,7 @@ mod running_process { } async fn check_dirt(vm: &TestVm) -> phd_testcase::Result<()> { - let output = vm.run_shell_command("fg").await?; + let output = vm.run_shell_command("fg").await?.expect_ok()?; assert!(output.contains("all good"), "dirt.sh failed: {output:?}"); Ok(()) } @@ -335,10 +342,13 @@ async fn multiple_migrations(ctx: &Framework) { vm0.launch().await?; vm0.wait_to_boot().await?; vm1.migrate_from(&vm0, Uuid::new_v4(), MigrationTimeout::default()).await?; - assert_eq!(vm1.run_shell_command("echo Hello world").await?, "Hello world"); + assert_eq!( + vm1.run_shell_command("echo Hello world").await?.ignore_status(), + "Hello world" + ); vm2.migrate_from(&vm1, Uuid::new_v4(), MigrationTimeout::default()).await?; assert_eq!( - vm2.run_shell_command("echo I have migrated!").await?, + vm2.run_shell_command("echo I have migrated!").await?.ignore_status(), "I have migrated!" ); } @@ -346,7 +356,10 @@ async fn multiple_migrations(ctx: &Framework) { async fn run_smoke_test(ctx: &Framework, mut source: TestVm) -> Result<()> { source.launch().await?; source.wait_to_boot().await?; - let lsout = source.run_shell_command("ls foo.bar 2> /dev/null").await?; + let lsout = source + .run_shell_command("ls foo.bar 2> /dev/null") + .await? + .expect_err()?; assert_eq!(lsout, ""); // create an empty file on the source VM. @@ -362,7 +375,8 @@ async fn run_smoke_test(ctx: &Framework, mut source: TestVm) -> Result<()> { let lsout = target .run_shell_command("ls foo.bar") .await - .expect("`ls foo.bar` should succeed after migration"); + .expect("`ls foo.bar` should succeed after migration") + .ignore_status(); assert_eq!(lsout, "foo.bar"); }) }, @@ -377,8 +391,10 @@ async fn run_serial_history_test( source.launch().await?; source.wait_to_boot().await?; - let out = - source.run_shell_command("echo hello from the source VM!").await?; + let out = source + .run_shell_command("echo hello from the source VM!") + .await? + .ignore_status(); assert_eq!(out, "hello from the source VM!"); let serial_hist_pre = source.get_serial_console_history(0).await?; diff --git a/phd-tests/tests/src/smoke.rs b/phd-tests/tests/src/smoke.rs index 5537e19eb..aecbeaad3 100644 --- a/phd-tests/tests/src/smoke.rs +++ b/phd-tests/tests/src/smoke.rs @@ -11,7 +11,7 @@ async fn nproc_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let nproc = vm.run_shell_command("nproc").await?; + let nproc = vm.run_shell_command("nproc").await?.expect_ok()?; assert_eq!(nproc.parse::().unwrap(), 6); } From 2b5fa0a55c2b3585f84e2fd61561fda0e5fe6bc4 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 18:43:21 +0000 Subject: [PATCH 2/8] now i gave up making run_shell_command nice, the must_use is redundant --- phd-tests/framework/src/test_vm/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index c4a965d1c..a18be6eb4 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -926,7 +926,6 @@ impl TestVm { // unfortunately I don't know how to plumb the futures for that, since we'd // have to close over `&self`, so doing any Boxing to hold an // `async move {}` immediately causes issues. - #[must_use] pub async fn run_shell_command(&self, cmd: &str) -> Result { // Allow the guest OS to transform the input command into a // guest-specific command sequence. This accounts for the guest's shell From 6c725def6f9c2cd675fab54f8eaba152b1bae200 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 20:14:55 +0000 Subject: [PATCH 3/8] this doesnt work but we can talk about it! --- phd-tests/framework/src/test_vm/mod.rs | 38 +++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index a18be6eb4..87e27d08f 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -148,6 +148,36 @@ pub struct ShellOutput { output: String, } +pub struct ShellOutputExecutor { + fut: Pin>>> +} + +pub struct ShellOutputOkExecutor { + fut: Pin>>> +} + +use std::pin::Pin; +use std::task::Poll; + +impl std::future::Future for ShellOutputExecutor { + type Output = Result; + + fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { + self.fut.as_mut().poll(cx) + } +} + +impl std::future::Future for ShellOutputOkExecutor { + type Output = Result; + + fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { + match self.fut.as_mut().poll(cx) { + Poll::Ready(t) => Poll::Ready(t.and_then(|res| res.expect_ok())), + Poll::Pending => Poll::Pending, + } + } +} + impl ShellOutput { /// Consume this [`ShellOutput`], returning the command's output as text /// if the command completed successfully, or an error if it did not. @@ -926,7 +956,8 @@ impl TestVm { // unfortunately I don't know how to plumb the futures for that, since we'd // have to close over `&self`, so doing any Boxing to hold an // `async move {}` immediately causes issues. - pub async fn run_shell_command(&self, cmd: &str) -> Result { + pub fn run_shell_command<'a>(&'a self, cmd: &'a str) -> impl std::future::Future> + 'a { + let fut = Box::pin(async move { // Allow the guest OS to transform the input command into a // guest-specific command sequence. This accounts for the guest's shell // type (which affects e.g. affects how it displays multi-line commands) @@ -964,6 +995,11 @@ impl TestVm { let status = status.trim().parse::()?; Ok(ShellOutput { status, output }) + }); + + ShellOutputExecutor { + fut, + } } pub async fn graceful_reboot(&self) -> Result<()> { From 507929c3a34cf238779feb92a769dc099dc1be38 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 20:14:55 +0000 Subject: [PATCH 4/8] this doesnt work but we can talk about it! --- phd-tests/framework/src/test_vm/mod.rs | 46 +++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index a18be6eb4..c251d7f8d 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -148,6 +148,44 @@ pub struct ShellOutput { output: String, } +pub struct ShellOutputExecutor { + fut: Pin>>> +} + +impl ShellOutputExecutor { + fn require_ok(self) -> ShellOutputOkExecutor { + ShellOutputOkExecutor { + fut: self.fut + } + } +} + +pub struct ShellOutputOkExecutor { + fut: Pin>>> +} + +use std::pin::Pin; +use std::task::Poll; + +impl std::future::Future for ShellOutputExecutor { + type Output = Result; + + fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { + self.fut.as_mut().poll(cx) + } +} + +impl std::future::Future for ShellOutputOkExecutor { + type Output = Result; + + fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { + match self.fut.as_mut().poll(cx) { + Poll::Ready(t) => Poll::Ready(t.and_then(|res| res.expect_ok())), + Poll::Pending => Poll::Pending, + } + } +} + impl ShellOutput { /// Consume this [`ShellOutput`], returning the command's output as text /// if the command completed successfully, or an error if it did not. @@ -926,7 +964,8 @@ impl TestVm { // unfortunately I don't know how to plumb the futures for that, since we'd // have to close over `&self`, so doing any Boxing to hold an // `async move {}` immediately causes issues. - pub async fn run_shell_command(&self, cmd: &str) -> Result { + pub fn run_shell_command<'a>(&'a self, cmd: &'a str) -> impl std::future::Future> + 'a { + let fut = Box::pin(async move { // Allow the guest OS to transform the input command into a // guest-specific command sequence. This accounts for the guest's shell // type (which affects e.g. affects how it displays multi-line commands) @@ -964,6 +1003,11 @@ impl TestVm { let status = status.trim().parse::()?; Ok(ShellOutput { status, output }) + }); + + ShellOutputExecutor { + fut, + } } pub async fn graceful_reboot(&self) -> Result<()> { From 35b1ac916cbeabc530240cf1ae25fadeaa0ae238 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 20:42:34 +0000 Subject: [PATCH 5/8] if a shell command wore futures would it wear them like this? --- phd-tests/framework/src/test_vm/mod.rs | 105 ++++++++++++++----------- 1 file changed, 60 insertions(+), 45 deletions(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index c251d7f8d..d21d14a32 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -148,10 +148,12 @@ pub struct ShellOutput { output: String, } -pub struct ShellOutputExecutor { - fut: Pin>>> +pub struct ShellOutputExecutor<'ctx> { + vm: &'ctx TestVm, + cmd: &'ctx str, } +/* impl ShellOutputExecutor { fn require_ok(self) -> ShellOutputOkExecutor { ShellOutputOkExecutor { @@ -159,6 +161,56 @@ impl ShellOutputExecutor { } } } +*/ + +impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> { + type Output = Result; + type IntoFuture = Pin>>; + + fn into_future(self) -> Self::IntoFuture { + Box::pin(async move { + // Allow the guest OS to transform the input command into a + // guest-specific command sequence. This accounts for the guest's shell + // type (which affects e.g. affects how it displays multi-line commands) + // and serial console buffering discipline. + let command_sequence = self.vm.guest_os.shell_command_sequence(self.cmd); + self.vm.run_command_sequence(command_sequence).await?; + + // `shell_command_sequence` promises that the generated command sequence + // clears buffer of everything up to and including the input command + // before actually issuing the final '\n' that issues the command. + // This ensures that the buffer contents returned by this call contain + // only the command's output. + let output = self + .vm + .wait_for_serial_output( + self.vm.guest_os.get_shell_prompt(), + Duration::from_secs(300), + ) + .await?; + + let status_command_sequence = + self.vm.guest_os.shell_command_sequence("echo $?"); + self.vm.run_command_sequence(status_command_sequence).await?; + + let status = self + .vm + .wait_for_serial_output( + self.vm.guest_os.get_shell_prompt(), + Duration::from_secs(300), + ) + .await?; + + // Trim any leading newlines inserted when the command was issued and + // any trailing whitespace that isn't actually part of the command + // output. Any other embedded whitespace is the caller's problem. + let output = output.trim().to_string(); + let status = status.trim().parse::()?; + + Ok(ShellOutput { status, output }) + }) + } +} pub struct ShellOutputOkExecutor { fut: Pin>>> @@ -167,13 +219,15 @@ pub struct ShellOutputOkExecutor { use std::pin::Pin; use std::task::Poll; -impl std::future::Future for ShellOutputExecutor { +/* +impl<' std::future::Future for ShellOutputExecutor { type Output = Result; fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { self.fut.as_mut().poll(cx) } } +*/ impl std::future::Future for ShellOutputOkExecutor { type Output = Result; @@ -964,49 +1018,10 @@ impl TestVm { // unfortunately I don't know how to plumb the futures for that, since we'd // have to close over `&self`, so doing any Boxing to hold an // `async move {}` immediately causes issues. - pub fn run_shell_command<'a>(&'a self, cmd: &'a str) -> impl std::future::Future> + 'a { - let fut = Box::pin(async move { - // Allow the guest OS to transform the input command into a - // guest-specific command sequence. This accounts for the guest's shell - // type (which affects e.g. affects how it displays multi-line commands) - // and serial console buffering discipline. - let command_sequence = self.guest_os.shell_command_sequence(cmd); - self.run_command_sequence(command_sequence).await?; - - // `shell_command_sequence` promises that the generated command sequence - // clears buffer of everything up to and including the input command - // before actually issuing the final '\n' that issues the command. - // This ensures that the buffer contents returned by this call contain - // only the command's output. - let output = self - .wait_for_serial_output( - self.guest_os.get_shell_prompt(), - Duration::from_secs(300), - ) - .await?; - - let status_command_sequence = - self.guest_os.shell_command_sequence("echo $?"); - self.run_command_sequence(status_command_sequence).await?; - - let status = self - .wait_for_serial_output( - self.guest_os.get_shell_prompt(), - Duration::from_secs(300), - ) - .await?; - - // Trim any leading newlines inserted when the command was issued and - // any trailing whitespace that isn't actually part of the command - // output. Any other embedded whitespace is the caller's problem. - let output = output.trim().to_string(); - let status = status.trim().parse::()?; - - Ok(ShellOutput { status, output }) - }); - + pub fn run_shell_command<'a>(&'a self, cmd: &'a str) -> ShellOutputExecutor<'a> { ShellOutputExecutor { - fut, + vm: self, + cmd, } } From 3c5f16ccec7eac6703802e06ad85770d3c89c785 Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 23:09:18 +0000 Subject: [PATCH 6/8] leave the typical/implicit "command exits 0" behavior intact this makes `run_shell_command` a bit more magic but keeps tests less boilerplate-y. tyvm Eliza for helping me figure out how to put the future together... --- phd-tests/framework/src/test_vm/mod.rs | 226 +++++++++----------- phd-tests/tests/src/boot_order.rs | 18 +- phd-tests/tests/src/boot_order/efi_utils.rs | 7 +- phd-tests/tests/src/cpuid.rs | 3 +- phd-tests/tests/src/crucible/migrate.rs | 31 +-- phd-tests/tests/src/crucible/smoke.rs | 9 +- phd-tests/tests/src/disk.rs | 17 +- phd-tests/tests/src/framework.rs | 3 +- phd-tests/tests/src/hw.rs | 8 +- phd-tests/tests/src/migrate.rs | 39 ++-- phd-tests/tests/src/smoke.rs | 2 +- 11 files changed, 162 insertions(+), 201 deletions(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index d21d14a32..d23d48f2b 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -18,7 +18,7 @@ use crate::{ Framework, }; -use anyhow::{anyhow, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use camino::Utf8PathBuf; use core::result::Result as StdResult; use propolis_client::{ @@ -148,134 +148,113 @@ pub struct ShellOutput { output: String, } +/// Description of the acceptable status codes from executing a command in a +/// [`TestVm::run_shell_command`]. +// This could reasonably have a `Status(u16)` variant to check specific non-zero +// statuses, but specific codes are not terribly portable! In the few cases we +// can expect a specific status for errors, those specific codes change between +// f.ex illumos and Linux guests. +enum StatusCheck { + Ok, + NotOk, +} + pub struct ShellOutputExecutor<'ctx> { vm: &'ctx TestVm, cmd: &'ctx str, + status_check: Option, } -/* -impl ShellOutputExecutor { - fn require_ok(self) -> ShellOutputOkExecutor { - ShellOutputOkExecutor { - fut: self.fut - } +impl<'a> ShellOutputExecutor<'a> { + pub fn ignore_status(mut self) -> ShellOutputExecutor<'a> { + self.status_check = None; + self } -} -*/ - -impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> { - type Output = Result; - type IntoFuture = Pin>>; - fn into_future(self) -> Self::IntoFuture { - Box::pin(async move { - // Allow the guest OS to transform the input command into a - // guest-specific command sequence. This accounts for the guest's shell - // type (which affects e.g. affects how it displays multi-line commands) - // and serial console buffering discipline. - let command_sequence = self.vm.guest_os.shell_command_sequence(self.cmd); - self.vm.run_command_sequence(command_sequence).await?; - - // `shell_command_sequence` promises that the generated command sequence - // clears buffer of everything up to and including the input command - // before actually issuing the final '\n' that issues the command. - // This ensures that the buffer contents returned by this call contain - // only the command's output. - let output = self - .vm - .wait_for_serial_output( - self.vm.guest_os.get_shell_prompt(), - Duration::from_secs(300), - ) - .await?; - - let status_command_sequence = - self.vm.guest_os.shell_command_sequence("echo $?"); - self.vm.run_command_sequence(status_command_sequence).await?; - - let status = self - .vm - .wait_for_serial_output( - self.vm.guest_os.get_shell_prompt(), - Duration::from_secs(300), - ) - .await?; - - // Trim any leading newlines inserted when the command was issued and - // any trailing whitespace that isn't actually part of the command - // output. Any other embedded whitespace is the caller's problem. - let output = output.trim().to_string(); - let status = status.trim().parse::()?; - - Ok(ShellOutput { status, output }) - }) + pub fn check_ok(mut self) -> ShellOutputExecutor<'a> { + self.status_check = Some(StatusCheck::Ok); + self } -} - -pub struct ShellOutputOkExecutor { - fut: Pin>>> -} - -use std::pin::Pin; -use std::task::Poll; - -/* -impl<' std::future::Future for ShellOutputExecutor { - type Output = Result; - fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { - self.fut.as_mut().poll(cx) + pub fn check_err(mut self) -> ShellOutputExecutor<'a> { + self.status_check = Some(StatusCheck::NotOk); + self } } -*/ +use futures::FutureExt; -impl std::future::Future for ShellOutputOkExecutor { +impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> { type Output = Result; + type IntoFuture = futures::future::BoxFuture<'a, Result>; - fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll { - match self.fut.as_mut().poll(cx) { - Poll::Ready(t) => Poll::Ready(t.and_then(|res| res.expect_ok())), - Poll::Pending => Poll::Pending, - } - } -} - -impl ShellOutput { - /// Consume this [`ShellOutput`], returning the command's output as text - /// if the command completed successfully, or an error if it did not. - pub fn expect_ok(self) -> Result { - if self.status == 0 { - Ok(self.output) - } else { - Err(anyhow!("command exited with non-zero status: {}", self.status)) - } - } - - /// Consume this [`ShellOutput`], returning the command's output as text - /// if the command exited with non-zero status, or an error if it exited - /// with status zero. - pub fn expect_err(self) -> Result { - if self.status != 0 { - Ok(self.output) - } else { - Err(anyhow!("command unexpectedly succeeded")) - } - } - - pub fn ignore_status(self) -> String { - self.output - } + fn into_future(self) -> Self::IntoFuture { + let cmd = Box::pin(async move { + // Allow the guest OS to transform the input command into a + // guest-specific command sequence. This accounts for the guest's + // shell type (which affects e.g. affects how it displays multi-line + // commands) and serial console buffering discipline. + let command_sequence = + self.vm.guest_os.shell_command_sequence(self.cmd); + self.vm.run_command_sequence(command_sequence).await?; + + // `shell_command_sequence` promises that the generated command + // sequence clears buffer of everything up to and including the + // input command before actually issuing the final '\n' that issues + // the command. This ensures that the buffer contents returned by + // this call contain only the command's output. + let output = self + .vm + .wait_for_serial_output( + self.vm.guest_os.get_shell_prompt(), + Duration::from_secs(300), + ) + .await?; - pub fn status(&self) -> u16 { - self.status - } + let status_command_sequence = + self.vm.guest_os.shell_command_sequence("echo $?"); + self.vm.run_command_sequence(status_command_sequence).await?; - /// Get the textual output of the command. You probably should use - /// [`ShellOutput::expect_ok`] or [`ShellOutput::expect_err`] to ensure - /// the command was processed as expected before asserting on its - /// output. - pub fn output(&self) -> &str { - self.output.as_str() + let status = self + .vm + .wait_for_serial_output( + self.vm.guest_os.get_shell_prompt(), + Duration::from_secs(300), + ) + .await?; + + // Trim any leading newlines inserted when the command was issued + // and any trailing whitespace that isn't actually part of the + // command output. Any other embedded whitespace is the caller's + // problem. + let output = output.trim().to_string(); + let status = status.trim().parse::()?; + + Ok(ShellOutput { status, output }) + }); + cmd.map(move |res| { + res.and_then(|out| { + match self.status_check { + Some(StatusCheck::Ok) => { + if out.status != 0 { + bail!("expected status 0, got {}", out.status); + } + } + Some(StatusCheck::NotOk) => { + if out.status != 0 { + bail!( + "expected non-zero status, got {}", + out.status + ); + } + } + None => { + // No check, always a success regardless of exit status + } + } + Ok(out.output) + }) + }) + .boxed() } } @@ -1008,20 +987,19 @@ impl TestVm { /// /// After running the shell command, sends `echo $?` to query and return the /// command's return status as well. - // TO REVIEWERS: it would be really nice to write this as a function - // returning a `struct ShellCommandExecutor` that impls - // `Future` where the underlying ShellOutput is automatically - // `.expect_ok()`'d. In such a case it would be possible for the struct to - // have an `.expect_err()` that replaces teh default `.expect_ok()` - // behavior, so that the likely case doesn't need any change in PHD tests. - // - // unfortunately I don't know how to plumb the futures for that, since we'd - // have to close over `&self`, so doing any Boxing to hold an - // `async move {}` immediately causes issues. - pub fn run_shell_command<'a>(&'a self, cmd: &'a str) -> ShellOutputExecutor<'a> { + /// + /// This will return an error if the command returns a non-zero status by + /// default; to ignore the status or expect a non-zero as a positive + /// condition, see [`ShellOutputExecutor::ignore_status`] or + /// [`ShellOutputExecutor::check_err`]. + pub fn run_shell_command<'a>( + &'a self, + cmd: &'a str, + ) -> ShellOutputExecutor<'a> { ShellOutputExecutor { vm: self, cmd, + status_check: Some(StatusCheck::Ok), } } diff --git a/phd-tests/tests/src/boot_order.rs b/phd-tests/tests/src/boot_order.rs index 5f7424c15..ce69ca5a4 100644 --- a/phd-tests/tests/src/boot_order.rs +++ b/phd-tests/tests/src/boot_order.rs @@ -5,7 +5,7 @@ use anyhow::{bail, Error}; use phd_framework::{ disk::{fat::FatFilesystem, DiskSource}, - test_vm::{DiskBackend, DiskInterface, ShellOutput}, + test_vm::{DiskBackend, DiskInterface}, }; use phd_testcase::*; use std::io::Cursor; @@ -20,10 +20,15 @@ use efi_utils::{ EDK2_FIRMWARE_VOL_GUID, EDK2_UI_APP_GUID, }; +// NOTE: This function differs from `run_shell_command` in that it implicitly +// ignores the status of executed commands. When +// https://github.com/oxidecomputer/propolis/issues/773 is fixed and this is +// deleted, callers of this function may need to be updated to call +// `.ignore_status` or `.check_err` pub(crate) async fn run_long_command( vm: &phd_framework::TestVm, cmd: &str, -) -> Result { +) -> Result { // Ok, this is a bit whacky: something about the line wrapping for long // commands causes `run_shell_command` to hang instead of ever actually // seeing a response prompt. @@ -31,7 +36,7 @@ pub(crate) async fn run_long_command( // I haven't gone and debugged that; instead, chunk the input command up // into segments short enough to not wrap when input, put them all in a // file, then run the file. - vm.run_shell_command("rm cmd").await?; + vm.run_shell_command("rm cmd").ignore_status().await?; let mut offset = 0; // Escape any internal `\`. This isn't comprehensive escaping (doesn't // handle \n, for example).. @@ -48,7 +53,9 @@ pub(crate) async fn run_long_command( vm.run_shell_command(&format!("echo -n \'{}\' >>cmd", chunk)).await?; } - vm.run_shell_command(". cmd").await + // `ignore_status` because it's a bit cumbersome to wrap this whole thing in + // a way that checks statuses, + vm.run_shell_command(". cmd").ignore_status().await } // This test checks that with a specified boot order, the guest boots whichever @@ -285,7 +292,7 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) { // If the guest doesn't have an EFI partition then there's no way for boot // order preferences to be persisted. let mountline = - vm.run_shell_command("mount | grep efivarfs").await?.ignore_status(); + vm.run_shell_command("mount | grep efivarfs").ignore_status().await?; if !mountline.starts_with("efivarfs on ") { warn!( @@ -299,7 +306,6 @@ async fn guest_can_adjust_boot_order(ctx: &Framework) { // reboot, and make sure they're all as we set them. if !run_long_command(&vm, &format!("ls {}", efipath(&bootvar(0xffff)))) .await? - .expect_err()? .is_empty() { warn!( diff --git a/phd-tests/tests/src/boot_order/efi_utils.rs b/phd-tests/tests/src/boot_order/efi_utils.rs index fabe77e87..adf1c1f50 100644 --- a/phd-tests/tests/src/boot_order/efi_utils.rs +++ b/phd-tests/tests/src/boot_order/efi_utils.rs @@ -326,7 +326,7 @@ pub(crate) async fn read_efivar( efipath(varname) ); - let hex = run_long_command(vm, &cmd).await?.expect_ok()?; + let hex = run_long_command(vm, &cmd).await?; Ok(unhex(&hex)) } @@ -345,8 +345,7 @@ pub(crate) async fn write_efivar( efipath(varname) ); - let attr_read_bytes = - run_long_command(vm, &attr_cmd).await?.ignore_status(); + let attr_read_bytes = run_long_command(vm, &attr_cmd).await?; let attrs = if attr_read_bytes.ends_with(": No such file or directory") { // Default attributes if the variable does not exist yet. We expect it // to be non-volatile because we are writing it, we expect it to be @@ -391,7 +390,7 @@ pub(crate) async fn write_efivar( efipath(varname) ); - let res = run_long_command(vm, &cmd).await?.expect_ok()?; + let res = run_long_command(vm, &cmd).await?; // If something went sideways and the write failed with something like // `invalid argument`... if !res.is_empty() { diff --git a/phd-tests/tests/src/cpuid.rs b/phd-tests/tests/src/cpuid.rs index 1205ce5a6..3ec773761 100644 --- a/phd-tests/tests/src/cpuid.rs +++ b/phd-tests/tests/src/cpuid.rs @@ -169,8 +169,7 @@ async fn cpuid_boot_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let cpuinfo = - vm.run_shell_command("cat /proc/cpuinfo").await?.expect_ok()?; + let cpuinfo = vm.run_shell_command("cat /proc/cpuinfo").await?; info!(cpuinfo, "/proc/cpuinfo output"); assert!(cpuinfo.contains( std::str::from_utf8(BRAND_STRING).unwrap().trim_matches('\0') diff --git a/phd-tests/tests/src/crucible/migrate.rs b/phd-tests/tests/src/crucible/migrate.rs index 681fcc8c6..0fcceda87 100644 --- a/phd-tests/tests/src/crucible/migrate.rs +++ b/phd-tests/tests/src/crucible/migrate.rs @@ -19,13 +19,11 @@ async fn smoke_test(ctx: &Framework) { source.launch().await?; source.wait_to_boot().await?; - let lsout = source - .run_shell_command("ls foo.bar 2> /dev/null") - .await? - .expect_err()?; + let lsout = + source.run_shell_command("ls foo.bar 2> /dev/null").check_err().await?; assert_eq!(lsout, ""); - source.run_shell_command("touch ./foo.bar").await?.expect_ok()?; - source.run_shell_command("sync ./foo.bar").await?.expect_ok()?; + source.run_shell_command("touch ./foo.bar").await?; + source.run_shell_command("sync ./foo.bar").await?; disk.set_generation(2); let mut target = ctx @@ -35,7 +33,7 @@ async fn smoke_test(ctx: &Framework) { target .migrate_from(&source, Uuid::new_v4(), MigrationTimeout::default()) .await?; - let lsout = target.run_shell_command("ls foo.bar").await?.expect_ok()?; + let lsout = target.run_shell_command("ls foo.bar").await?; assert_eq!(lsout, "foo.bar"); } @@ -66,22 +64,17 @@ async fn load_test(ctx: &Framework) { ) .as_str(), ) - .await? - .expect_ok()?; + .await?; assert!(ddout.contains(format!("{}+0 records in", block_count).as_str())); // Compute the data's hash. - let sha256sum_out = - source.run_shell_command("sha256sum rand.txt").await?.expect_ok()?; + let sha256sum_out = source.run_shell_command("sha256sum rand.txt").await?; let checksum = sha256sum_out.split_whitespace().next().unwrap(); info!("Generated SHA256 checksum: {}", checksum); // Start copying the generated file into a second file, then start a // migration while that copy is in progress. - source - .run_shell_command("dd if=./rand.txt of=./rand_new.txt &") - .await? - .expect_ok()?; + source.run_shell_command("dd if=./rand.txt of=./rand_new.txt &").await?; target .migrate_from(&source, Uuid::new_v4(), MigrationTimeout::default()) .await?; @@ -89,11 +82,9 @@ async fn load_test(ctx: &Framework) { // Wait for the background command to finish running, then compute the // hash of the copied file. If all went well this will match the hash of // the source file. - target.run_shell_command("wait $!").await?.expect_ok()?; - let sha256sum_target = target - .run_shell_command("sha256sum rand_new.txt") - .await? - .expect_ok()?; + target.run_shell_command("wait $!").await?; + let sha256sum_target = + target.run_shell_command("sha256sum rand_new.txt").await?; let checksum_target = sha256sum_target.split_whitespace().next().unwrap(); assert_eq!(checksum, checksum_target); } diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs index d675b8439..25d69b1c0 100644 --- a/phd-tests/tests/src/crucible/smoke.rs +++ b/phd-tests/tests/src/crucible/smoke.rs @@ -66,10 +66,10 @@ async fn shutdown_persistence_test(ctx: &Framework) { // Verify that the test file doesn't exist yet, then touch it, flush it, and // shut down the VM. let lsout = - vm.run_shell_command("ls foo.bar 2> /dev/null").await?.expect_err()?; + vm.run_shell_command("ls foo.bar 2> /dev/null").check_err().await?; assert_eq!(lsout, ""); - vm.run_shell_command("touch ./foo.bar").await?.expect_ok()?; - vm.run_shell_command("sync ./foo.bar").await?.expect_ok()?; + vm.run_shell_command("touch ./foo.bar").await?; + vm.run_shell_command("sync ./foo.bar").await?; vm.stop().await?; vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(60)) .await?; @@ -84,7 +84,6 @@ async fn shutdown_persistence_test(ctx: &Framework) { vm.wait_to_boot().await?; // The touched file from the previous VM should be present in the new one. - let lsout = - vm.run_shell_command("ls foo.bar 2> /dev/null").await?.expect_ok()?; + let lsout = vm.run_shell_command("ls foo.bar 2> /dev/null").await?; assert_eq!(lsout, "foo.bar"); } diff --git a/phd-tests/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index ba73caf9b..572e5e6af 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -37,12 +37,9 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) { // try to check that the disk is located there and fail the test early if // it's not. If the by-path directory is missing, put up a warning and hope // for the best. - let dev_disk = vm.run_shell_command("ls /dev/disk").await?.ignore_status(); + let dev_disk = vm.run_shell_command("ls /dev/disk").ignore_status().await?; if dev_disk.contains("by-path") { - let ls = vm - .run_shell_command("ls -la /dev/disk/by-path") - .await? - .expect_ok()?; + let ls = vm.run_shell_command("ls -la /dev/disk/by-path").await?; info!(%ls, "guest disk device paths"); assert!(ls.contains("virtio-pci-0000:00:18.0 -> ../../vda")); } else { @@ -52,19 +49,17 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) { ); } - vm.run_shell_command("mkdir /phd").await?.expect_ok()?; + vm.run_shell_command("mkdir /phd").await?; // The disk is read-only, so pass the `ro` option to `mount` so that it // doesn't complain about not being able to mount for writing. - let mount = - vm.run_shell_command("mount -o ro /dev/vda /phd").await?.expect_ok()?; + let mount = vm.run_shell_command("mount -o ro /dev/vda /phd").await?; assert_eq!(mount, ""); // The file should be there and have the expected contents. - let ls = vm.run_shell_command("ls /phd").await?.expect_ok()?; + let ls = vm.run_shell_command("ls /phd").await?; assert_eq!(ls, "hello_oxide.txt"); - let cat = - vm.run_shell_command("cat /phd/hello_oxide.txt").await?.expect_ok()?; + let cat = vm.run_shell_command("cat /phd/hello_oxide.txt").await?; assert_eq!(cat, HELLO_MSG); } diff --git a/phd-tests/tests/src/framework.rs b/phd-tests/tests/src/framework.rs index f9ae012d8..3f1cb8cab 100644 --- a/phd-tests/tests/src/framework.rs +++ b/phd-tests/tests/src/framework.rs @@ -13,7 +13,6 @@ async fn multiline_serial_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let out = - vm.run_shell_command("echo \\\nhello \\\nworld").await?.expect_ok()?; + let out = vm.run_shell_command("echo \\\nhello \\\nworld").await?; assert_eq!(out, "hello world"); } diff --git a/phd-tests/tests/src/hw.rs b/phd-tests/tests/src/hw.rs index 4ce27bea9..c6477a27f 100644 --- a/phd-tests/tests/src/hw.rs +++ b/phd-tests/tests/src/hw.rs @@ -23,17 +23,17 @@ async fn lspci_lifecycle_test(ctx: &Framework) { // * lshw may not exist (Debian) // * we may not input a sudo password (Ubuntu) - let lspci = vm.run_shell_command(LSPCI).await?.ignore_status(); - let lshw = vm.run_shell_command(LSHW).await?.ignore_status(); + let lspci = vm.run_shell_command(LSPCI).ignore_status().await?; + let lshw = vm.run_shell_command(LSHW).ignore_status().await?; ctx.lifecycle_test(vm, &[Action::StopAndStart], move |vm| { let lspci = lspci.clone(); let lshw = lshw.clone(); Box::pin(async move { let new_lspci = - vm.run_shell_command(LSPCI).await.unwrap().ignore_status(); + vm.run_shell_command(LSPCI).ignore_status().await.unwrap(); assert_eq!(new_lspci, lspci); let new_lshw = - vm.run_shell_command(LSHW).await.unwrap().ignore_status(); + vm.run_shell_command(LSHW).ignore_status().await.unwrap(); assert_eq!(new_lshw, lshw); }) }) diff --git a/phd-tests/tests/src/migrate.rs b/phd-tests/tests/src/migrate.rs index e8b0898f1..3690ea595 100644 --- a/phd-tests/tests/src/migrate.rs +++ b/phd-tests/tests/src/migrate.rs @@ -58,13 +58,13 @@ mod from_base { // `ls` with no results exits non-zero, so expect an error here. let lsout = source .run_shell_command("ls foo.bar 2> /dev/null") - .await? - .expect_err()?; + .check_err() + .await?; assert_eq!(lsout, ""); // create an empty file on the source VM. - source.run_shell_command("touch ./foo.bar").await?.expect_ok()?; - source.run_shell_command("sync ./foo.bar").await?.expect_ok()?; + source.run_shell_command("touch ./foo.bar").await?; + source.run_shell_command("sync ./foo.bar").await?; ctx.lifecycle_test( source, @@ -78,9 +78,9 @@ mod from_base { // the file should still exist on the target VM after migration. let lsout = target .run_shell_command("ls foo.bar") + .ignore_status() .await - .expect("`ls foo.bar` should succeed") - .ignore_status(); + .expect("can try to run `ls foo.bar`"); assert_eq!(lsout, "foo.bar"); }) }, @@ -261,10 +261,10 @@ mod running_process { "\nEOF" )) .await?; - vm.run_shell_command("chmod +x dirt.sh").await?.expect_ok()?; + vm.run_shell_command("chmod +x dirt.sh").await?; // When dirt.sh suspends itself, the parent shell will report a non-zero // status (148, in particular: 128 + SIGTSTP aka 20 for Linux guests). - let run_dirt = vm.run_shell_command("./dirt.sh").await?.expect_err()?; + let run_dirt = vm.run_shell_command("./dirt.sh").check_err().await?; assert!(run_dirt.contains("made dirt"), "dirt.sh failed: {run_dirt:?}"); assert!( run_dirt.contains("Stopped"), @@ -275,7 +275,7 @@ mod running_process { } async fn check_dirt(vm: &TestVm) -> phd_testcase::Result<()> { - let output = vm.run_shell_command("fg").await?.expect_ok()?; + let output = vm.run_shell_command("fg").await?; assert!(output.contains("all good"), "dirt.sh failed: {output:?}"); Ok(()) } @@ -342,13 +342,10 @@ async fn multiple_migrations(ctx: &Framework) { vm0.launch().await?; vm0.wait_to_boot().await?; vm1.migrate_from(&vm0, Uuid::new_v4(), MigrationTimeout::default()).await?; - assert_eq!( - vm1.run_shell_command("echo Hello world").await?.ignore_status(), - "Hello world" - ); + assert_eq!(vm1.run_shell_command("echo Hello world").await?, "Hello world"); vm2.migrate_from(&vm1, Uuid::new_v4(), MigrationTimeout::default()).await?; assert_eq!( - vm2.run_shell_command("echo I have migrated!").await?.ignore_status(), + vm2.run_shell_command("echo I have migrated!").await?, "I have migrated!" ); } @@ -356,10 +353,8 @@ async fn multiple_migrations(ctx: &Framework) { async fn run_smoke_test(ctx: &Framework, mut source: TestVm) -> Result<()> { source.launch().await?; source.wait_to_boot().await?; - let lsout = source - .run_shell_command("ls foo.bar 2> /dev/null") - .await? - .expect_err()?; + let lsout = + source.run_shell_command("ls foo.bar 2> /dev/null").check_err().await?; assert_eq!(lsout, ""); // create an empty file on the source VM. @@ -374,9 +369,9 @@ async fn run_smoke_test(ctx: &Framework, mut source: TestVm) -> Result<()> { // the file should still exist on the target VM after migration. let lsout = target .run_shell_command("ls foo.bar") + .ignore_status() .await - .expect("`ls foo.bar` should succeed after migration") - .ignore_status(); + .expect("can try to run `ls foo.bar`"); assert_eq!(lsout, "foo.bar"); }) }, @@ -393,8 +388,8 @@ async fn run_serial_history_test( let out = source .run_shell_command("echo hello from the source VM!") - .await? - .ignore_status(); + .ignore_status() + .await?; assert_eq!(out, "hello from the source VM!"); let serial_hist_pre = source.get_serial_console_history(0).await?; diff --git a/phd-tests/tests/src/smoke.rs b/phd-tests/tests/src/smoke.rs index aecbeaad3..5537e19eb 100644 --- a/phd-tests/tests/src/smoke.rs +++ b/phd-tests/tests/src/smoke.rs @@ -11,7 +11,7 @@ async fn nproc_test(ctx: &Framework) { vm.launch().await?; vm.wait_to_boot().await?; - let nproc = vm.run_shell_command("nproc").await?.expect_ok()?; + let nproc = vm.run_shell_command("nproc").await?; assert_eq!(nproc.parse::().unwrap(), 6); } From 324bb5259b807bdcbc4359da0507ea77660f5dae Mon Sep 17 00:00:00 2001 From: iximeow Date: Fri, 18 Oct 2024 23:18:08 +0000 Subject: [PATCH 7/8] tidy --- phd-tests/tests/src/migrate.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/phd-tests/tests/src/migrate.rs b/phd-tests/tests/src/migrate.rs index 3690ea595..e98bf5129 100644 --- a/phd-tests/tests/src/migrate.rs +++ b/phd-tests/tests/src/migrate.rs @@ -263,7 +263,7 @@ mod running_process { .await?; vm.run_shell_command("chmod +x dirt.sh").await?; // When dirt.sh suspends itself, the parent shell will report a non-zero - // status (148, in particular: 128 + SIGTSTP aka 20 for Linux guests). + // status (one example is 148: 128 + SIGTSTP aka 20 on Linux). let run_dirt = vm.run_shell_command("./dirt.sh").check_err().await?; assert!(run_dirt.contains("made dirt"), "dirt.sh failed: {run_dirt:?}"); assert!( @@ -386,10 +386,8 @@ async fn run_serial_history_test( source.launch().await?; source.wait_to_boot().await?; - let out = source - .run_shell_command("echo hello from the source VM!") - .ignore_status() - .await?; + let out = + source.run_shell_command("echo hello from the source VM!").await?; assert_eq!(out, "hello from the source VM!"); let serial_hist_pre = source.get_serial_console_history(0).await?; From db4e1ebfef76eab29c9fa541b77e6e1bd2c351bb Mon Sep 17 00:00:00 2001 From: iximeow Date: Sat, 19 Oct 2024 00:09:19 +0000 Subject: [PATCH 8/8] ShellOutput is no longer relevant really... --- phd-tests/framework/src/test_vm/mod.rs | 65 ++++++++++---------------- 1 file changed, 25 insertions(+), 40 deletions(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index d23d48f2b..f5464f15e 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -142,12 +142,6 @@ enum VmState { Ensured { serial: SerialConsole }, } -/// Both the output and status of a command. -pub struct ShellOutput { - status: u16, - output: String, -} - /// Description of the acceptable status codes from executing a command in a /// [`TestVm::run_shell_command`]. // This could reasonably have a `Status(u16)` variant to check specific non-zero @@ -188,7 +182,7 @@ impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> { type IntoFuture = futures::future::BoxFuture<'a, Result>; fn into_future(self) -> Self::IntoFuture { - let cmd = Box::pin(async move { + Box::pin(async move { // Allow the guest OS to transform the input command into a // guest-specific command sequence. This accounts for the guest's // shell type (which affects e.g. affects how it displays multi-line @@ -210,49 +204,40 @@ impl<'a> std::future::IntoFuture for ShellOutputExecutor<'a> { ) .await?; - let status_command_sequence = - self.vm.guest_os.shell_command_sequence("echo $?"); - self.vm.run_command_sequence(status_command_sequence).await?; - - let status = self - .vm - .wait_for_serial_output( - self.vm.guest_os.get_shell_prompt(), - Duration::from_secs(300), - ) - .await?; - // Trim any leading newlines inserted when the command was issued // and any trailing whitespace that isn't actually part of the // command output. Any other embedded whitespace is the caller's // problem. let output = output.trim().to_string(); - let status = status.trim().parse::()?; - - Ok(ShellOutput { status, output }) - }); - cmd.map(move |res| { - res.and_then(|out| { - match self.status_check { - Some(StatusCheck::Ok) => { - if out.status != 0 { - bail!("expected status 0, got {}", out.status); + + if let Some(check) = self.status_check { + let status_command_sequence = + self.vm.guest_os.shell_command_sequence("echo $?"); + self.vm.run_command_sequence(status_command_sequence).await?; + let status = self + .vm + .wait_for_serial_output( + self.vm.guest_os.get_shell_prompt(), + Duration::from_secs(300), + ) + .await?; + let status = status.trim().parse::()?; + + match check { + StatusCheck::Ok => { + if status != 0 { + bail!("expected status 0, got {}", status); } } - Some(StatusCheck::NotOk) => { - if out.status != 0 { - bail!( - "expected non-zero status, got {}", - out.status - ); + StatusCheck::NotOk => { + if status == 0 { + bail!("expected non-zero status, got {}", status); } } - None => { - // No check, always a success regardless of exit status - } } - Ok(out.output) - }) + } + + Ok(output) }) .boxed() }