From 9cdd1e9ea887ab21e01239fab0683b04025a6bb2 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 7 Jun 2024 22:00:23 +0000 Subject: [PATCH 1/4] PHD: properly consume characters from raw serial buffer after matching --- phd-tests/framework/src/serial/raw_buffer.rs | 66 +++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/phd-tests/framework/src/serial/raw_buffer.rs b/phd-tests/framework/src/serial/raw_buffer.rs index d432dfebe..01fcda562 100644 --- a/phd-tests/framework/src/serial/raw_buffer.rs +++ b/phd-tests/framework/src/serial/raw_buffer.rs @@ -95,7 +95,8 @@ impl RawBuffer { "checking wait on raw serial buffer" ); if let Some(idx) = self.wait_buffer.rfind(&waiter.wanted) { - let out = self.wait_buffer[..idx].to_owned(); + let out = self.wait_buffer.drain(..idx).collect(); + self.wait_buffer = self.wait_buffer.split_off(waiter.wanted.len()); // Because incoming bytes from Propolis may be processed on a // separate task than the task that registered the wait, this @@ -152,3 +153,66 @@ impl Drop for RawBuffer { } } } + +#[cfg(test)] +mod test { + use tokio::sync::oneshot; + + use super::*; + + fn make_buffer() -> RawBuffer { + let file = + std::fs::OpenOptions::new().write(true).open("/dev/null").unwrap(); + + RawBuffer { + log: std::io::BufWriter::new(file), + line_buffer: String::new(), + wait_buffer: String::new(), + waiter: None, + parser: Parser::new(), + } + } + + #[tokio::test] + async fn successful_wait_consumes_buffer_contents() { + let mut buf = make_buffer(); + let (tx, mut rx) = oneshot::channel(); + buf.push_str("the quick brown fox jumped over the lazy propolis"); + buf.satisfy_or_set_wait(OutputWaiter { + wanted: "jumped over".to_string(), + preceding_tx: tx, + }); + assert_eq!(rx.try_recv().unwrap(), "the quick brown fox "); + assert_eq!(buf.wait_buffer, " the lazy propolis"); + + // Repeat the test, but register the wait before the characters are + // pushed. + buf.clear(); + let (tx, mut rx) = oneshot::channel(); + buf.satisfy_or_set_wait(OutputWaiter { + wanted: "jumped over".to_string(), + preceding_tx: tx, + }); + buf.push_str("the quick brown fox jumped over the lazy propolis"); + assert_eq!(rx.try_recv().unwrap(), "the quick brown fox "); + assert_eq!(buf.wait_buffer, " the lazy propolis"); + } + + #[tokio::test] + async fn successful_wait_consumes_last_match() { + let mut buf = make_buffer(); + let (tx, mut rx) = oneshot::channel(); + buf.push_str( + "I put some Oxide in your Oxide so you can Oxide while you Oxide", + ); + buf.satisfy_or_set_wait(OutputWaiter { + wanted: "you".to_string(), + preceding_tx: tx, + }); + assert_eq!( + rx.try_recv().unwrap(), + "I put some Oxide in your Oxide so you can Oxide while " + ); + assert_eq!(buf.wait_buffer, " Oxide"); + } +} From 8d592c9284b216376fb862f361873d1f34dbd5b5 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 7 Jun 2024 22:00:23 +0000 Subject: [PATCH 2/4] add PHD tests to exercise reboot from within a guest --- phd-tests/framework/src/test_vm/mod.rs | 2 +- phd-tests/tests/src/crucible/smoke.rs | 27 ++++++++++++++++++++++++++ phd-tests/tests/src/smoke.rs | 21 ++++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 03b8f24f9..a4b16f4b3 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -849,7 +849,7 @@ impl TestVm { /// Sends `string` to the guest's serial console worker, then waits for the /// entire string to be sent to the guest before returning. - pub(crate) async fn send_serial_str(&self, string: &str) -> Result<()> { + pub async fn send_serial_str(&self, string: &str) -> Result<()> { if !string.is_empty() { self.send_serial_bytes(Vec::from(string.as_bytes()))?.await?; } diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs index 44eafc052..2955d8bdc 100644 --- a/phd-tests/tests/src/crucible/smoke.rs +++ b/phd-tests/tests/src/crucible/smoke.rs @@ -16,6 +16,33 @@ async fn boot_test(ctx: &Framework) { vm.wait_to_boot().await?; } +#[phd_testcase] +async fn api_reboot_test(ctx: &Framework) { + let mut config = ctx.vm_config_builder("crucible_guest_reboot_test"); + super::add_default_boot_disk(ctx, &mut config)?; + + let mut vm = ctx.spawn_vm(&config, None).await?; + vm.launch().await?; + vm.wait_to_boot().await?; + vm.reset().await?; + vm.wait_to_boot().await?; +} + +#[phd_testcase] +async fn guest_reboot_test(ctx: &Framework) { + let mut config = ctx.vm_config_builder("crucible_guest_reboot_test"); + super::add_default_boot_disk(ctx, &mut config)?; + + let mut vm = ctx.spawn_vm(&config, None).await?; + vm.launch().await?; + vm.wait_to_boot().await?; + + // Don't use `run_shell_command` because the guest won't echo another prompt + // after this. + vm.send_serial_str("reboot\n").await?; + vm.wait_to_boot().await?; +} + #[phd_testcase] async fn shutdown_persistence_test(ctx: &Framework) { let mut config = diff --git a/phd-tests/tests/src/smoke.rs b/phd-tests/tests/src/smoke.rs index 8a797afae..9095e0c6d 100644 --- a/phd-tests/tests/src/smoke.rs +++ b/phd-tests/tests/src/smoke.rs @@ -15,6 +15,27 @@ async fn nproc_test(ctx: &Framework) { assert_eq!(nproc.parse::().unwrap(), 6); } +#[phd_testcase] +async fn api_reboot_test(ctx: &Framework) { + let mut vm = ctx.spawn_default_vm("api_reboot_test").await?; + vm.launch().await?; + vm.wait_to_boot().await?; + vm.reset().await?; + vm.wait_to_boot().await?; +} + +#[phd_testcase] +async fn guest_reboot_test(ctx: &Framework) { + let mut vm = ctx.spawn_default_vm("guest_reboot_test").await?; + vm.launch().await?; + vm.wait_to_boot().await?; + + // Don't use `run_shell_command` because the guest won't echo another prompt + // after this. + vm.send_serial_str("reboot\n").await?; + vm.wait_to_boot().await?; +} + #[phd_testcase] async fn instance_spec_get_test(ctx: &Framework) { let mut vm = ctx From 42e4412f27b692145ff8f6a5b05de341811d1d8b Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 14 Jun 2024 00:26:50 +0000 Subject: [PATCH 3/4] don't bail from block_for_req if queue is still paused or empty --- lib/propolis/src/block/attachment.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/propolis/src/block/attachment.rs b/lib/propolis/src/block/attachment.rs index 790ad14cd..05f54891b 100644 --- a/lib/propolis/src/block/attachment.rs +++ b/lib/propolis/src/block/attachment.rs @@ -66,15 +66,20 @@ impl BlockData { } Err(err) => match err { ReqError::NonePending | ReqError::Paused => { - let guard = self.lock.lock().unwrap(); // Double-check for attachment-related "error" // conditions under protection of the lock before // finally blocking. - if check_state(att_state).is_err() { - return None; + let guard = self.lock.lock().unwrap(); + match check_state(att_state) { + Err(ReqError::Stopped | ReqError::Detached) => { + return None; + } + Ok(()) + | Err(ReqError::Paused | ReqError::NonePending) => { + let _guard = self.cv.wait(guard).unwrap(); + continue; + } } - let _guard = self.cv.wait(guard).unwrap(); - continue; } _ => { return None; From 099e2beea6eaebbbedbbcffe46b3ef30e622e6e2 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Fri, 14 Jun 2024 18:43:28 +0000 Subject: [PATCH 4/4] explicitly check for defunct attachment --- lib/propolis/src/block/attachment.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/propolis/src/block/attachment.rs b/lib/propolis/src/block/attachment.rs index 05f54891b..2e521049f 100644 --- a/lib/propolis/src/block/attachment.rs +++ b/lib/propolis/src/block/attachment.rs @@ -70,16 +70,12 @@ impl BlockData { // conditions under protection of the lock before // finally blocking. let guard = self.lock.lock().unwrap(); - match check_state(att_state) { - Err(ReqError::Stopped | ReqError::Detached) => { - return None; - } - Ok(()) - | Err(ReqError::Paused | ReqError::NonePending) => { - let _guard = self.cv.wait(guard).unwrap(); - continue; - } + if !att_state.is_attached() || att_state.is_stopped() { + return None; } + + let _guard = self.cv.wait(guard).unwrap(); + continue; } _ => { return None;