From 0b3e6b4879496ae2abd9f3322c5b688d982c569c Mon Sep 17 00:00:00 2001 From: lif <> Date: Sat, 19 Sep 2026 08:12:00 +0000 Subject: [PATCH 1/6] Support soft shutdown of guest via ACPI PWRBTN_STS Implements behavior specified in RFD 622, based on earlier work on platform support for ACPI fixed power button by @lgfa29. This adds an optional `acpi_timeout_secs` parameter to instance state PUT, which if specified will cause propolis-server to send the guest an [ACPI fixed power button](https://uefi.org/specs/ACPI/6.6/04_ACPI_Hardware_Specification.html#fixed-power-button) signal. After sending this to the guest, propolis-server waits for the guest to shut itself down for the specified number of seconds before enqueueing the explicit hard-stop or hard-reset the request would have otherwise been without the timeout parameter specified. If an instance state PUT request to stop or reset is received while shutting down, it is honored immediately, possibly interrupting the guest's safer shutdown process (unmounting disks, et al). If an instance state PUT with a timeout is received while still waiting for a previously-requested ACPI shutdown, the newer request is ignored. If a guest which is expected to be shutting down sends propolis *any* CPU-halting event, be it the expected halt, a chipset-driven reset, or a triple-fault, the action which was specified in the pending shutdown request is taken, overriding what the guest's action would normally imply absent the pending shutdown with with the authority of the user's intent.\* (\* In essence, we expect that if the control plane tells us the instance should be turned off gently, it could well be because the sled is about to be upgraded, and the customer is likely to want their VM guests' filesystems to be left in as coherent a state as possible. An unclean unmount from a triple-fault followed shortly by the system rebooting, starting a filesystem repair, and then being interrupted during *that* by a hard-cut of its power is unlikely to be a positive customer experience) Co-authored-by: Luiz Aoqui --- bin/propolis-cli/src/main.rs | 15 +- bin/propolis-server/src/lib/initializer.rs | 13 +- bin/propolis-server/src/lib/server.rs | 10 +- bin/propolis-server/src/lib/vm/active.rs | 32 +- bin/propolis-server/src/lib/vm/ensure.rs | 1 + bin/propolis-server/src/lib/vm/mod.rs | 7 + bin/propolis-server/src/lib/vm/objects.rs | 15 +- .../src/lib/vm/request_queue.rs | 322 +++++++++++++++++- .../src/lib/vm/state_driver.rs | 170 +++++++-- bin/propolis-standalone/src/main.rs | 1 + .../src/acpi_shutdown/instance.rs | 19 ++ .../src/acpi_shutdown/mod.rs | 5 + .../propolis-api-types-versions/src/latest.rs | 3 +- crates/propolis-api-types-versions/src/lib.rs | 2 + crates/propolis-server-api/src/lib.rs | 25 +- lib/propolis/src/hw/chipset/i440fx.rs | 20 +- .../propolis-server-6.0.0-b5b984.json.gitstub | 1 + ...json => propolis-server-7.0.0-a344ff.json} | 28 +- .../propolis-server-latest.json | 2 +- phd-tests/framework/src/test_vm/mod.rs | 46 ++- phd-tests/tests/src/acpi_shutdown.rs | 144 ++++++++ phd-tests/tests/src/lib.rs | 1 + 22 files changed, 809 insertions(+), 73 deletions(-) create mode 100644 crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs create mode 100644 crates/propolis-api-types-versions/src/acpi_shutdown/mod.rs create mode 100644 openapi/propolis-server/propolis-server-6.0.0-b5b984.json.gitstub rename openapi/propolis-server/{propolis-server-6.0.0-b5b984.json => propolis-server-7.0.0-a344ff.json} (98%) create mode 100644 phd-tests/tests/src/acpi_shutdown.rs diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 72ba175b4..0690a4bcf 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -39,7 +39,10 @@ use uuid::Uuid; use propolis_client::{ support::{InstanceSerialConsoleHelper, WSClientOffset}, - types::{InstanceStateRequested, InstanceVcrReplace, MigrationState}, + types::{ + InstanceStateChange, InstanceStateRequested, InstanceVcrReplace, + MigrationState, + }, Client, }; @@ -118,6 +121,9 @@ enum Command { /// The requested state #[clap(value_parser = parse_state)] state: InstanceStateRequested, + /// The number of seconds to wait after sending ACPI PWRBTN_STS before + /// forcing stop/reset. (If omitted, stop/reset are forced immediately) + acpi_timeout_secs: Option, }, /// Drop to a Serial console connected to the instance @@ -572,10 +578,11 @@ async fn get_instance(client: &Client) -> anyhow::Result<()> { async fn put_instance( client: &Client, state: InstanceStateRequested, + acpi_timeout_secs: Option, ) -> anyhow::Result<()> { client .instance_state_put() - .body(state) + .body(InstanceStateChange { state, acpi_timeout_secs }) .send() .await .with_context(|| anyhow!("failed to set instance state"))?; @@ -958,7 +965,9 @@ async fn main() -> anyhow::Result<()> { .await? } Command::Get => get_instance(&client).await?, - Command::State { state } => put_instance(&client, state).await?, + Command::State { state, acpi_timeout_secs } => { + put_instance(&client, state, acpi_timeout_secs).await? + } Command::Serial { byte_offset } => { serial(addr, byte_offset, log).await? } diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 0da580375..bf55e072c 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -189,6 +189,7 @@ pub fn build_instance( pub struct RegisteredChipset { chipset: Arc, isa: Arc, + pm: Arc, } impl RegisteredChipset { pub fn pci_attach(&self, bdf: pci::Bdf, dev: Arc) { @@ -197,6 +198,9 @@ impl RegisteredChipset { pub fn irq_pin(&self, irq: u8) -> Option> { self.isa.irq_pin(irq) } + pub fn acpi_shutdown(&self) { + self.pm.acpi_shutdown(); + } fn reset_pin(&self) -> Arc { self.chipset.reset_pin() } @@ -356,6 +360,7 @@ impl MachineInitializer<'_> { let chipset_pm = i440fx::Piix3PM::create( self.machine.hdl.clone(), chipset_hb.power_pin(), + chipset_lpc.sci_pin(), self.log.new(slog::o!("device" => "piix3pm")), ); @@ -387,7 +392,7 @@ impl MachineInitializer<'_> { ); self.devices.insert( SpecKey::Name(chipset_pm.type_name().into()), - chipset_pm, + chipset_pm.clone(), ); // Record attachment for any bridges in PCI topology too @@ -404,7 +409,11 @@ impl MachineInitializer<'_> { self.devices.insert(spec_element.0.clone(), bridge); } - Ok(RegisteredChipset { chipset: chipset_hb, isa: chipset_lpc }) + Ok(RegisteredChipset { + chipset: chipset_hb, + isa: chipset_lpc, + pm: chipset_pm, + }) } } } diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index cba4f65cd..8501e51a4 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -43,7 +43,7 @@ use propolis_api_types::disk::{ }; use propolis_api_types::instance::{ ErrorCode, Instance, InstanceEnsureRequest, InstanceEnsureResponse, - InstanceGetResponse, InstanceInitializationMethod, + InstanceGetResponse, InstanceInitializationMethod, InstanceStateChange, InstanceStateMonitorRequest, InstanceStateMonitorResponse, InstanceStateRequested, }; @@ -364,13 +364,13 @@ impl PropolisServerApi for PropolisServerImpl { async fn instance_state_put( rqctx: RequestContext, - request: TypedBody, + request: TypedBody, ) -> Result { let ctx = rqctx.context(); - let requested_state = request.into_inner(); + let requested_change = request.into_inner(); let vm = ctx.vm.active_vm().await.ok_or_else(not_created_error)?; let result = vm - .put_state(requested_state) + .put_state(requested_change) .map(|_| HttpResponseUpdatedNoContent {}) .map_err(|e| match e { VmError::WaitingToInitialize => HttpError::for_unavail( @@ -391,7 +391,7 @@ impl PropolisServerApi for PropolisServerImpl { }); if result.is_ok() { - if let InstanceStateRequested::Reboot = requested_state { + if let InstanceStateRequested::Reboot = requested_change.state { let stats = MutexGuard::map( vm.services().oximeter.lock().await, |state| &mut state.stats, diff --git a/bin/propolis-server/src/lib/vm/active.rs b/bin/propolis-server/src/lib/vm/active.rs index 2abb93a0b..8f0cda22c 100644 --- a/bin/propolis-server/src/lib/vm/active.rs +++ b/bin/propolis-server/src/lib/vm/active.rs @@ -5,15 +5,17 @@ //! Implements a wrapper around an active VM. use std::sync::Arc; +use std::time::Duration; use propolis_api_types::instance::{ - InstanceProperties, InstanceStateRequested, + InstanceProperties, InstanceStateChange, InstanceStateRequested, }; use propolis_api_types::instance_spec::SpecKey; use slog::info; use uuid::Uuid; -use crate::vm::request_queue::ExternalRequest; +use crate::vm::request_queue::{ExternalRequest, RequestDeniedReason}; +use crate::vm::SoftShutdownFate; use super::{ objects::VmObjects, services::VmServices, CrucibleReplaceResultTx, @@ -57,17 +59,31 @@ impl ActiveVm { /// Pushes a state change request to the VM's state change queue. pub(crate) fn put_state( &self, - requested: InstanceStateRequested, + requested: InstanceStateChange, ) -> Result<(), VmError> { info!(self.log, "requested state via API"; - "state" => ?requested); - - self.state_driver_queue - .queue_external_request(match requested { + "state" => ?requested.state, + "timeout" => ?requested.acpi_timeout_secs); + + let ext_req = if let Some(secs) = requested.acpi_timeout_secs { + let timeout = Duration::from_secs(secs); + let fate = match requested.state { + InstanceStateRequested::Run => { + return Err(RequestDeniedReason::TimeoutOnRun.into()); + } + InstanceStateRequested::Stop => SoftShutdownFate::Stop, + InstanceStateRequested::Reboot => SoftShutdownFate::Reboot, + }; + ExternalRequest::acpi_shutdown(fate, timeout) + } else { + match requested.state { InstanceStateRequested::Run => ExternalRequest::start(), InstanceStateRequested::Stop => ExternalRequest::stop(), InstanceStateRequested::Reboot => ExternalRequest::reboot(), - }) + } + }; + self.state_driver_queue + .queue_external_request(ext_req) .map_err(Into::into) } diff --git a/bin/propolis-server/src/lib/vm/ensure.rs b/bin/propolis-server/src/lib/vm/ensure.rs index 3f7d50709..22b205084 100644 --- a/bin/propolis-server/src/lib/vm/ensure.rs +++ b/bin/propolis-server/src/lib/vm/ensure.rs @@ -656,6 +656,7 @@ async fn initialize_vm_objects( framebuffer: Some(ramfb), ps2ctrl, attest_handle, + chipset: chipset.into(), }; // Another really terrible hack. As we've found in Propolis#1008, brk() diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index 9f3c98e7f..ae95963fb 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -164,6 +164,13 @@ const VMM_MIN_RT_THREADS: usize = 8; /// is greater. const VMM_BASE_RT_THREADS: usize = 4; +/// The action to take after an ACPI shutdown either completes or times out. +#[derive(Clone, Copy, Debug)] +pub enum SoftShutdownFate { + Stop, + Reboot, +} + /// Errors generated by the VM controller and its subcomponents. #[derive(Debug, thiserror::Error)] pub(crate) enum VmError { diff --git a/bin/propolis-server/src/lib/vm/objects.rs b/bin/propolis-server/src/lib/vm/objects.rs index 49ba61c84..d0af0f6cf 100644 --- a/bin/propolis-server/src/lib/vm/objects.rs +++ b/bin/propolis-server/src/lib/vm/objects.rs @@ -22,7 +22,10 @@ use propolis_api_types::instance_spec::SpecKey; use slog::info; use tokio::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; -use crate::{serial::Serial, spec::Spec, vcpu_tasks::VcpuTaskController}; +use crate::{ + initializer::RegisteredChipset, serial::Serial, spec::Spec, + vcpu_tasks::VcpuTaskController, +}; use super::{BlockBackendMap, CrucibleBackendMap, DeviceMap}; @@ -53,6 +56,7 @@ pub(super) struct InputVmObjects { pub framebuffer: Option>, pub ps2ctrl: Arc, pub attest_handle: Option, + pub chipset: Arc, } /// The collection of objects and state that make up a Propolis instance. @@ -91,6 +95,9 @@ pub(crate) struct VmObjectsLocked { /// A handle to the VM's attestation server. attest_handle: Option, + + /// A handle to the VM's mainboard chipset. + chipset: Arc, } impl VmObjects { @@ -132,6 +139,7 @@ impl VmObjectsLocked { framebuffer: input.framebuffer, ps2ctrl: input.ps2ctrl, attest_handle: input.attest_handle, + chipset: input.chipset, } } @@ -194,6 +202,11 @@ impl VmObjectsLocked { &self.ps2ctrl } + /// Yields a clonable reference to this VM's mainboard chipset. + pub(crate) fn chipset(&self) -> &Arc { + &self.chipset + } + pub(crate) fn device_map(&self) -> &DeviceMap { &self.devices } diff --git a/bin/propolis-server/src/lib/vm/request_queue.rs b/bin/propolis-server/src/lib/vm/request_queue.rs index 2608b4933..86ff8c47c 100644 --- a/bin/propolis-server/src/lib/vm/request_queue.rs +++ b/bin/propolis-server/src/lib/vm/request_queue.rs @@ -21,6 +21,8 @@ use slog::{info, Logger}; use thiserror::Error; use uuid::Uuid; +use crate::vm::SoftShutdownFate; + /// Wraps a [`dropshot::WebsocketConnection`] for inclusion in an /// [`ExternalRequest`]. // @@ -60,6 +62,12 @@ pub(crate) enum StateChangeRequest { /// graceful reboot and does not coordinate with guest software. Reboot, + /// Sends an ACPI shutdown signal to the guest, and then either stops + /// or reboots the instance (depending on `fate`) once the guest halts + /// its CPU. If this does not occur within the `timeout`, the guest is + /// ungracefully stopped/rebooted. + ACPIShutdown { fate: SoftShutdownFate, timeout: std::time::Duration }, + /// Halts the VM. Note that this is not a graceful shutdown and does not /// coordinate with guest software. Stop, @@ -74,6 +82,11 @@ impl std::fmt::Debug for StateChangeRequest { .field("migration_id", migration_id) .finish(), Self::Reboot => write!(f, "Reboot"), + Self::ACPIShutdown { fate, timeout } => f + .debug_struct("ACPIShutdown") + .field("fate", fate) + .field("timeout", timeout) + .finish(), Self::Stop => write!(f, "Stop"), } } @@ -129,6 +142,13 @@ impl ExternalRequest { Self::State(StateChangeRequest::Start) } + pub const fn acpi_shutdown( + fate: SoftShutdownFate, + timeout: std::time::Duration, + ) -> Self { + Self::State(StateChangeRequest::ACPIShutdown { fate, timeout }) + } + /// Constructs a VM stop request. pub const fn stop() -> Self { Self::State(StateChangeRequest::Stop) @@ -166,7 +186,16 @@ impl ExternalRequest { } fn is_stop(&self) -> bool { - matches!(self, Self::State(StateChangeRequest::Stop)) + matches!( + self, + Self::State( + StateChangeRequest::Stop + | StateChangeRequest::ACPIShutdown { + fate: SoftShutdownFate::Stop, + .. + } + ) + ) } } @@ -197,6 +226,9 @@ pub(crate) enum RequestDeniedReason { #[error("Instance failed to start or halted due to a failure")] InstanceFailed, + + #[error("Cannot supply shutdown timeout to a Run request")] + TimeoutOnRun, } /// A kind of request that can be popped from the queue and then completed. @@ -270,6 +302,10 @@ pub(super) struct ExternalRequestQueue { /// completed by the state driver. awaiting_stop: bool, + /// True if this queue has enqueued an ACPI shutdown request that has not + /// been completed by the state driver. + awaiting_shutdown: bool, + /// The queue's logger. log: Logger, } @@ -297,6 +333,7 @@ impl ExternalRequestQueue { awaiting_reboot: false, awaiting_migration_out: false, awaiting_stop: false, + awaiting_shutdown: false, log, } } @@ -367,12 +404,20 @@ impl ExternalRequestQueue { assert!(!self.awaiting_migration_out); self.awaiting_migration_out = true; } + ExternalRequest::State(StateChangeRequest::ACPIShutdown { + .. + }) => { + assert!(!self.awaiting_shutdown); + self.awaiting_shutdown = true; + } ExternalRequest::State(StateChangeRequest::Reboot) => { assert!(!self.awaiting_reboot); + self.awaiting_shutdown = false; self.awaiting_reboot = true; } ExternalRequest::State(StateChangeRequest::Stop) => { assert!(!self.awaiting_stop); + self.awaiting_shutdown = false; self.awaiting_stop = true; } ExternalRequest::Component(_) => {} @@ -416,7 +461,7 @@ impl ExternalRequestQueue { // Interpret start requests as requests to reach the Running // state. ExternalRequest::State(StateChangeRequest::Start) => { - if self.awaiting_stop { + if self.awaiting_stop || self.awaiting_shutdown { return Err(RequestDeniedReason::HaltPending); } else if self.state != QueueState::NotStarted { return Ok(false); @@ -433,7 +478,7 @@ impl ExternalRequestQueue { return Err( RequestDeniedReason::AlreadyMigrationSource, ); - } else if self.awaiting_stop { + } else if self.awaiting_stop || self.awaiting_shutdown { return Err(RequestDeniedReason::HaltPending); } else if self.state == QueueState::NotStarted { return Err(RequestDeniedReason::InstanceNotActive); @@ -461,6 +506,37 @@ impl ExternalRequestQueue { } } + // Reject ACPI shutdown requests if the instance is not in a + // state where it could reasonably be expected to respond to + // a power button press (that is to say, Running), and also + // ignore further shutdown requests if one is already happening. + ExternalRequest::State(StateChangeRequest::ACPIShutdown { + .. + }) => match self.state { + QueueState::StartPending => { + return Err(RequestDeniedReason::StartInProgress) + } + QueueState::NotStarted => { + return Err(RequestDeniedReason::InstanceNotActive) + } + QueueState::Stopped => { + return Err(RequestDeniedReason::Halted) + } + QueueState::Failed => { + return Err(RequestDeniedReason::InstanceFailed) + } + QueueState::MigratedOut => { + return Err(RequestDeniedReason::MigratedOut) + } + QueueState::Running => { + if self.awaiting_stop || self.awaiting_reboot { + return Err(RequestDeniedReason::HaltPending); + } else if self.awaiting_shutdown { + return Ok(false); + } + } + }, + // Always queue requests to stop a VM unless one is already // present. // @@ -513,7 +589,8 @@ impl ExternalRequestQueue { } CompletedRequest::Reboot => { assert_eq!(self.state, QueueState::Running); - assert!(self.awaiting_reboot); + // (awaiting_reboot not asserted; would risk data race when a + // just-in-timely halt coincides with ACPI reset timeout task) self.awaiting_reboot = false; } CompletedRequest::MigrationOut { succeeded } => { @@ -539,6 +616,21 @@ impl ExternalRequestQueue { info!(&self.log, "queue notified that VM has stopped"); self.state = QueueState::Stopped; } + + /// Update the queue's `awaiting_shutdown` flag when the guest has an ACPI + /// shutdown request outstanding and a CPU-halting guest event occurs + pub(super) fn notify_shutdown(&mut self) { + info!(&self.log, "queue notified that VM guest has shut down"); + self.awaiting_shutdown = false; + } + + /// Update the queue's `awaiting_reboot` flag when the guest has rebooted for reasons + /// other than an explicit external hard reboot request (i.e. when an ACPI + /// reboot was in progress as a CPU-halting guest event occurred) + pub(super) fn notify_rebooted(&mut self) { + info!(&self.log, "queue notified that VM has rebooted"); + self.awaiting_reboot = false; + } } // It's possible for an external request queue to be dropped with outstanding @@ -583,6 +675,8 @@ impl Drop for ExternalRequestQueue { #[cfg(test)] mod test { + use std::time::Duration; + use super::*; use proptest::prelude::*; @@ -632,6 +726,34 @@ mod test { ); } + #[track_caller] + fn assert_acpi_shutdown(&self) { + assert!( + matches!( + self, + Self::State(StateChangeRequest::ACPIShutdown { + fate: SoftShutdownFate::Stop, + .. + }) + ), + "expected ACPI shutdown request, got {self:?}" + ); + } + + #[track_caller] + fn assert_acpi_reboot(&self) { + assert!( + matches!( + self, + Self::State(StateChangeRequest::ACPIShutdown { + fate: SoftShutdownFate::Reboot, + .. + }) + ), + "expected ACPI reboot request, got {self:?}" + ); + } + #[track_caller] fn assert_migrate_as_source(&self) { assert!( @@ -787,6 +909,198 @@ mod test { assert!(queue.try_queue(ExternalRequest::reboot()).is_err()); } + #[test] + fn acpi_shutdowns_ignored_after_first() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + queue.notify_request_completed(CompletedRequest::Start { + succeeded: true, + }); + + assert!(queue.is_empty()); + // enqueue an ACPI shutdown request with intent to stop + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_ok()); + + // all these further ACPI shutdown requests should be ignored + // (with intent sent to reboot, just so we can distinguish in this test + // that only the first request was enqueued) + for _ in 0..5 { + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Reboot, + Duration::from_secs(600) + )) + .is_ok()); + } + queue.pop_front().unwrap().assert_acpi_shutdown(); + assert!(queue.is_empty()); + } + + #[test] + fn acpi_shutdown_overridden_by_hard_stop_request() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + queue.notify_request_completed(CompletedRequest::Start { + succeeded: true, + }); + + assert!(queue.is_empty()); + // enqueue an ACPI shutdown request with intent to stop + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_ok()); + // enqueue a hard stop + assert!(queue.try_queue(ExternalRequest::stop()).is_ok()); + + queue.pop_front().unwrap().assert_acpi_shutdown(); + queue.pop_front().unwrap().assert_stop(); + + assert!(queue.is_empty()); + } + + #[test] + fn acpi_reboot_overridden_by_hard_stop_request() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + queue.notify_request_completed(CompletedRequest::Start { + succeeded: true, + }); + + assert!(queue.is_empty()); + // enqueue an ACPI shutdown request with intent to reboot + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Reboot, + Duration::from_secs(600) + )) + .is_ok()); + // enqueue a hard stop + assert!(queue.try_queue(ExternalRequest::stop()).is_ok()); + + queue.pop_front().unwrap().assert_acpi_reboot(); + queue.pop_front().unwrap().assert_stop(); + + assert!(queue.is_empty()); + } + + #[test] + fn acpi_shutdown_overridden_by_hard_reboot_request() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + queue.notify_request_completed(CompletedRequest::Start { + succeeded: true, + }); + + assert!(queue.is_empty()); + // enqueue an ACPI shutdown request with intent to reboot + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_ok()); + // enqueue a hard stop + assert!(queue.try_queue(ExternalRequest::reboot()).is_ok()); + + queue.pop_front().unwrap().assert_acpi_shutdown(); + queue.pop_front().unwrap().assert_reboot(); + + assert!(queue.is_empty()); + } + + #[test] + fn acpi_shutdown_requests_ignored_after_vm_failure() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + + queue.notify_request_completed(CompletedRequest::Start { + succeeded: false, + }); + + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_ok()); + assert!(queue.is_empty()); + } + + #[test] + fn acpi_shutdown_requests_disallowed_while_stopped() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + + queue.notify_request_completed(CompletedRequest::Start { + succeeded: true, + }); + + assert!(queue.try_queue(ExternalRequest::stop()).is_ok()); + + // attempts to shut down while processing a hard stop are an error + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_err()); + + queue.pop_front().unwrap().assert_stop(); + queue.notify_request_completed(CompletedRequest::Stop); + assert!(queue.is_empty()); + + // further attempts to shut down while stopped are simply ignored + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_ok()); + + assert!(queue.is_empty()); + } + + #[test] + fn acpi_shutdown_requests_disallowed_while_rebooting() { + let mut queue = + ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); + + queue.notify_request_completed(CompletedRequest::Start { + succeeded: true, + }); + + assert!(queue.try_queue(ExternalRequest::reboot()).is_ok()); + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_err()); + + queue.pop_front().unwrap().assert_reboot(); + queue.notify_request_completed(CompletedRequest::Reboot); + + // allowed again after reboot is complete + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Stop, + Duration::from_secs(600) + )) + .is_ok()); + + queue.pop_front().unwrap().assert_acpi_shutdown(); + + assert!(queue.is_empty()); + } + #[test] fn mutation_disallowed_after_stopped() { let mut queue = diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index 613f4f492..ea3899b01 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -110,7 +110,9 @@ use crate::{ destination::DestinationProtocol, source::SourceProtocol, MigrateRole, }, spec::StorageBackend, - vm::{state_publisher::ExternalStateUpdate, BlockBackendMap}, + vm::{ + state_publisher::ExternalStateUpdate, BlockBackendMap, SoftShutdownFate, + }, }; use super::{ @@ -273,6 +275,18 @@ impl InputQueue { guard.external_requests.notify_stopped(); } + /// Notifies the external request queue that the instance guest has + /// completed its externally-requested shutdown process. + fn notify_shutdown(&self) { + let mut guard = self.inner.lock().unwrap(); + guard.external_requests.notify_shutdown(); + } + + fn notify_rebooted(&self) { + let mut guard = self.inner.lock().unwrap(); + guard.external_requests.notify_rebooted(); + } + /// Submits an external state change request to the queue. pub(super) fn queue_external_request( &self, @@ -364,6 +378,12 @@ struct StateDriver { /// True if the VM is paused. paused: bool, + /// `Some` if an ACPI shutdown has been requested. + /// Contains a tuple of the action to take after the guest halts its CPUs, + /// the timeout task that will forcibly halt the guest's CPUs if it is not + /// aborted. + soft_off: Option<(SoftShutdownFate, tokio::task::JoinHandle<()>)>, + /// State persisted from previous attempts to migrate out of this VM. migration_src_state: crate::migrate::source::PersistentState, } @@ -428,6 +448,7 @@ pub(super) async fn ensure_vm_and_launch_driver( external_state: state_publisher, paused: false, migration_src_state: Default::default(), + soft_off: None, }; // Run the VM until it exits, then set rundown on the parent VM so that no @@ -759,11 +780,12 @@ impl StateDriver { // reported that it's fully started. Similarly, requests to // start a VM that's already starting are expected to be ignored // for idempotency. - r @ ExternalRequest::State(StateChangeRequest::Start) - | r @ ExternalRequest::State( - StateChangeRequest::MigrateAsSource { .. }, - ) - | r @ ExternalRequest::State(StateChangeRequest::Reboot) => { + r @ ExternalRequest::State( + StateChangeRequest::Start + | StateChangeRequest::MigrateAsSource { .. } + | StateChangeRequest::Reboot + | StateChangeRequest::ACPIShutdown { .. }, + ) => { unreachable!( "external request {r:?} shouldn't be queued while \ starting" @@ -773,22 +795,56 @@ impl StateDriver { } } + async fn stop_and_notify( + &mut self, + direct_external_req: bool, + ) -> HandleEventOutcome { + self.do_halt().await; + + self.external_state + .update(ExternalStateUpdate::Instance(InstanceState::Stopped)); + + if direct_external_req { + self.input_queue.notify_request_completed(CompletedRequest::Stop); + } else { + self.input_queue.notify_stopped(); + } + + HandleEventOutcome::Exit { final_state: InstanceState::Destroyed } + } + async fn handle_guest_event( &mut self, event: GuestEvent, ) -> HandleEventOutcome { + // if an ACPI shutdown request was in progress when a guest event comes, + // we trust the control plane's authority on the ultimate fate + // of the guest's running state -- even if e.g. guest BSoD'd during + // its shutdown process and tried to reboot afterward, when operator + // asked for the guest to be *off*. + if let Some((fate, _)) = &self.soft_off { + self.input_queue.notify_shutdown(); + return match fate { + SoftShutdownFate::Stop => { + info!( + self.log, + "Halting due to {event:?} following power button event" + ); + self.stop_and_notify(false).await + } + SoftShutdownFate::Reboot => { + info!(self.log, "Resetting due to {event:?} following power button event"); + self.do_reboot().await; + self.input_queue.notify_rebooted(); + HandleEventOutcome::Continue + } + }; + } + match event { GuestEvent::VcpuSuspendHalt(_when) => { info!(self.log, "Halting due to VM suspend event",); - self.do_halt().await; - self.external_state.update(ExternalStateUpdate::Instance( - InstanceState::Stopped, - )); - - self.input_queue.notify_stopped(); - HandleEventOutcome::Exit { - final_state: InstanceState::Destroyed, - } + self.stop_and_notify(false).await } GuestEvent::VcpuSuspendReset(_when) => { info!(self.log, "Resetting due to VM suspend event"); @@ -805,15 +861,7 @@ impl StateDriver { } GuestEvent::ChipsetHalt => { info!(self.log, "Halting due to chipset-driven halt"); - self.do_halt().await; - self.external_state.update(ExternalStateUpdate::Instance( - InstanceState::Stopped, - )); - - self.input_queue.notify_stopped(); - HandleEventOutcome::Exit { - final_state: InstanceState::Destroyed, - } + self.stop_and_notify(false).await } GuestEvent::ChipsetReset => { info!(self.log, "Resetting due to chipset-driven reset"); @@ -828,6 +876,49 @@ impl StateDriver { request: ExternalRequest, ) -> HandleEventOutcome { match request { + ExternalRequest::State(StateChangeRequest::ACPIShutdown { + fate, + timeout, + }) => { + // if timeout already in progress, ignore further soft-off reqs + if self.soft_off.is_none() { + let guard = self.objects.lock_exclusive().await; + let chipset = guard.chipset(); + + chipset.acpi_shutdown(); + // TODO? might watch for OSPM clearing the status bit to + // determine if guest acknowledges the button press + + // timeout to hard-stop/reset if guest takes too long + let input_queue_weak = Arc::downgrade(&self.input_queue); + // (philosophically, timeout behavior is part of what was + // externally requested...) + let quasi_ext_req = match fate { + SoftShutdownFate::Stop => ExternalRequest::stop(), + SoftShutdownFate::Reboot => ExternalRequest::reboot(), + }; + let log = self.log.clone(); + self.soft_off = Some(( + fate, + tokio::spawn(async move { + tokio::time::sleep(timeout).await; + if let Some(input_queue) = + input_queue_weak.upgrade() + { + if let Err(e) = input_queue + .queue_external_request(quasi_ext_req) + { + slog::error!( + log, + "Failed to enqueue {fate:?}: {e}" + ); + } + } + }), + )); + } + HandleEventOutcome::Continue + } ExternalRequest::State(StateChangeRequest::Start) => { // If this start attempt produces a terminal VM state, return it // to the driver and indicate that the driver should exit. @@ -846,7 +937,13 @@ impl StateDriver { migration_id, websock, }) => { - if self + if let Some((fate, _)) = &self.soft_off { + slog::warn!( + self.log, + "Ignored MigrateAsSource while ACPI {fate:?} in progress" + ); + HandleEventOutcome::Continue + } else if self .migrate_as_source(migration_id, websock.into_inner()) .await .is_ok() @@ -867,17 +964,7 @@ impl StateDriver { HandleEventOutcome::Continue } ExternalRequest::State(StateChangeRequest::Stop) => { - self.do_halt().await; - self.external_state.update(ExternalStateUpdate::Instance( - InstanceState::Stopped, - )); - - self.input_queue - .notify_request_completed(CompletedRequest::Stop); - - HandleEventOutcome::Exit { - final_state: InstanceState::Destroyed, - } + self.stop_and_notify(true).await } ExternalRequest::Component( ComponentChangeRequest::ReconfigureCrucibleVolume { @@ -898,6 +985,11 @@ impl StateDriver { async fn do_reboot(&mut self) { info!(self.log, "resetting instance"); + // first abandon any timeout task that may send a redundant request + if let Some((_, timeout_task)) = self.soft_off.take() { + timeout_task.abort(); + } + self.external_state .update(ExternalStateUpdate::Instance(InstanceState::Rebooting)); @@ -911,6 +1003,12 @@ impl StateDriver { async fn do_halt(&mut self) { info!(self.log, "stopping instance"); + + // first abandon any timeout task that may send a redundant request + if let Some((_, timeout_task)) = self.soft_off.take() { + timeout_task.abort(); + } + self.external_state .update(ExternalStateUpdate::Instance(InstanceState::Stopping)); diff --git a/bin/propolis-standalone/src/main.rs b/bin/propolis-standalone/src/main.rs index 57c290331..aa7f5b93b 100644 --- a/bin/propolis-standalone/src/main.rs +++ b/bin/propolis-standalone/src/main.rs @@ -1230,6 +1230,7 @@ fn setup_instance( let chipset_pm = i440fx::Piix3PM::create( machine.hdl.clone(), chipset_hb.power_pin(), + chipset_lpc.sci_pin(), log.new(slog::o!("device" => "piix3pm")), ); diff --git a/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs b/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs new file mode 100644 index 000000000..ed655c9c0 --- /dev/null +++ b/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs @@ -0,0 +1,19 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use crate::v1::instance::InstanceStateRequested; + +/// Requested state change of an Instance. +#[derive(Clone, Copy, Deserialize, Serialize, JsonSchema)] +pub struct InstanceStateChange { + /// The desired state for the Instance. + pub state: InstanceStateRequested, + /// The number of seconds to wait after sending ACPI `PWRBTN_STS` before + /// forcing stop/reset. (If omitted, stop/reset are forced immediately, + /// and the ACPI signal is not sent.) + pub acpi_timeout_secs: Option, +} diff --git a/crates/propolis-api-types-versions/src/acpi_shutdown/mod.rs b/crates/propolis-api-types-versions/src/acpi_shutdown/mod.rs new file mode 100644 index 000000000..fed465ed4 --- /dev/null +++ b/crates/propolis-api-types-versions/src/acpi_shutdown/mod.rs @@ -0,0 +1,5 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +pub mod instance; diff --git a/crates/propolis-api-types-versions/src/latest.rs b/crates/propolis-api-types-versions/src/latest.rs index 5e64474b0..b440742c5 100644 --- a/crates/propolis-api-types-versions/src/latest.rs +++ b/crates/propolis-api-types-versions/src/latest.rs @@ -65,7 +65,6 @@ pub mod instance { pub use crate::v1::instance::InstancePathParams; pub use crate::v1::instance::InstanceProperties; pub use crate::v1::instance::InstanceState; - pub use crate::v1::instance::InstanceStateChange; pub use crate::v1::instance::InstanceStateMonitorRequest; pub use crate::v1::instance::InstanceStateMonitorResponse; pub use crate::v1::instance::InstanceStateRequested; @@ -73,6 +72,8 @@ pub mod instance { pub use crate::v6::api::InstanceEnsureRequest; pub use crate::v6::api::InstanceInitializationMethod; + + pub use crate::v7::instance::InstanceStateChange; } pub mod instance_spec { diff --git a/crates/propolis-api-types-versions/src/lib.rs b/crates/propolis-api-types-versions/src/lib.rs index aecc1a645..eb2829df2 100644 --- a/crates/propolis-api-types-versions/src/lib.rs +++ b/crates/propolis-api-types-versions/src/lib.rs @@ -41,3 +41,5 @@ pub mod v3; pub mod v5; #[path = "nvme_write_cache/mod.rs"] pub mod v6; +#[path = "acpi_shutdown/mod.rs"] +pub mod v7; diff --git a/crates/propolis-server-api/src/lib.rs b/crates/propolis-server-api/src/lib.rs index 298d0d33e..684d9df26 100644 --- a/crates/propolis-server-api/src/lib.rs +++ b/crates/propolis-server-api/src/lib.rs @@ -22,6 +22,7 @@ api_versions!([ // | example for the next person. // v // (next_int, IDENT), + (7, ACPI_SHUTDOWN), (6, NVME_WRITE_CACHE), (5, CRUCIBLE_VOLUME_INFO), (4, DROPSHOT_BUMP_WEBSOCKET), @@ -213,12 +214,34 @@ pub trait PropolisServerApi { #[endpoint { method = PUT, path = "/instance/state", + versions = VERSION_ACPI_SHUTDOWN.. }] async fn instance_state_put( rqctx: RequestContext, - request: TypedBody, + request: TypedBody, ) -> Result; + #[endpoint { + operation_id = "instance_state_put", + method = PUT, + path = "/instance/state", + versions = ..VERSION_ACPI_SHUTDOWN + }] + async fn instance_state_put_v6( + rqctx: RequestContext, + request: TypedBody, + ) -> Result { + Self::instance_state_put( + rqctx, + latest::instance::InstanceStateChange { + state: request.into_inner(), + acpi_timeout_secs: None, + } + .into(), + ) + .await + } + #[endpoint { method = GET, path = "/instance/serial/history", diff --git a/lib/propolis/src/hw/chipset/i440fx.rs b/lib/propolis/src/hw/chipset/i440fx.rs index beafb08a1..2a1523ed6 100644 --- a/lib/propolis/src/hw/chipset/i440fx.rs +++ b/lib/propolis/src/hw/chipset/i440fx.rs @@ -103,7 +103,6 @@ struct IrqConfig { lnk_pins: [Arc; 4], - #[allow(unused)] // XXX: wire up SCI notifications sci_pin: Arc, } @@ -466,6 +465,10 @@ impl Piix3Lpc { .pin_handle(irq) .map(|pin| Box::new(pin) as Box) } + + pub fn sci_pin(&self) -> Arc { + Arc::clone(&self.irq_config.sci_pin) as Arc + } } impl pci::Device for Piix3Lpc { fn device_state(&self) -> &pci::DeviceState { @@ -803,12 +806,14 @@ pub struct Piix3PM { regs: Mutex, power_pin: Arc, + sci_pin: Arc, log: slog::Logger, } impl Piix3PM { pub fn create( hdl: Arc, power_pin: Arc, + sci_pin: Arc, log: slog::Logger, ) -> Arc { let pci_state = pci::Builder::new(pci::Ident { @@ -836,6 +841,7 @@ impl Piix3PM { regs: Mutex::new(regs), power_pin, + sci_pin, log, }) } @@ -848,6 +854,12 @@ impl Piix3PM { pio.register(PMBASE_DEFAULT, PMBASE_LEN, piofn).unwrap(); } + pub fn acpi_shutdown(&self) { + let mut regs = self.regs.lock().unwrap(); + regs.pm_status.insert(PmSts::PWRBTN_STS); + self.sci_pin.pulse(); + } + fn pio_rw(&self, _port: u16, mut rwo: RWOp) { PM_REGS.process(&mut rwo, |id, rwo| match rwo { RWOp::Read(ro) => self.pmreg_read(id, ro), @@ -1185,8 +1197,9 @@ mod test { let scaffold = Scaffold::new(); let log = Logger::root(Discard, slog::o!()); let power_pin = Arc::new(NoOpPin {}); + let sci_pin = Arc::new(NoOpPin {}); - let pm = Piix3PM::create(hdl, power_pin, log); + let pm = Piix3PM::create(hdl, power_pin, sci_pin, log); let _bus = setup_attach(&scaffold, pm.clone()); cfg_read(pm.as_ref() as &dyn Endpoint); @@ -1198,8 +1211,9 @@ mod test { let scaffold = Scaffold::new(); let log = Logger::root(Discard, slog::o!()); let power_pin = Arc::new(NoOpPin {}); + let sci_pin = Arc::new(NoOpPin {}); - let pm = Piix3PM::create(hdl, power_pin, log); + let pm = Piix3PM::create(hdl, power_pin, sci_pin, log); let _bus = setup_attach(&scaffold, pm.clone()); cfg_write(pm.as_ref() as &dyn Endpoint); diff --git a/openapi/propolis-server/propolis-server-6.0.0-b5b984.json.gitstub b/openapi/propolis-server/propolis-server-6.0.0-b5b984.json.gitstub new file mode 100644 index 000000000..a2c8f9747 --- /dev/null +++ b/openapi/propolis-server/propolis-server-6.0.0-b5b984.json.gitstub @@ -0,0 +1 @@ +046f74302e2ea09a75b0a6810645d42c7df6644a:openapi/propolis-server/propolis-server-6.0.0-b5b984.json diff --git a/openapi/propolis-server/propolis-server-6.0.0-b5b984.json b/openapi/propolis-server/propolis-server-7.0.0-a344ff.json similarity index 98% rename from openapi/propolis-server/propolis-server-6.0.0-b5b984.json rename to openapi/propolis-server/propolis-server-7.0.0-a344ff.json index 1bfc621c4..5718d81fe 100644 --- a/openapi/propolis-server/propolis-server-6.0.0-b5b984.json +++ b/openapi/propolis-server/propolis-server-7.0.0-a344ff.json @@ -7,7 +7,7 @@ "url": "https://oxide.computer", "email": "api@oxide.computer" }, - "version": "6.0.0" + "version": "7.0.0" }, "paths": { "/instance": { @@ -399,7 +399,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/InstanceStateRequested" + "$ref": "#/components/schemas/InstanceStateChange" } } }, @@ -1654,6 +1654,30 @@ "Destroyed" ] }, + "InstanceStateChange": { + "description": "Requested state change of an Instance.", + "type": "object", + "properties": { + "acpi_timeout_secs": { + "nullable": true, + "description": "The number of seconds to wait after sending ACPI `PWRBTN_STS` before forcing stop/reset. (If omitted, stop/reset are forced immediately, and the ACPI signal is not sent.)", + "type": "integer", + "format": "uint64", + "minimum": 0 + }, + "state": { + "description": "The desired state for the Instance.", + "allOf": [ + { + "$ref": "#/components/schemas/InstanceStateRequested" + } + ] + } + }, + "required": [ + "state" + ] + }, "InstanceStateMonitorRequest": { "type": "object", "properties": { diff --git a/openapi/propolis-server/propolis-server-latest.json b/openapi/propolis-server/propolis-server-latest.json index 114ec385b..b1cd71342 120000 --- a/openapi/propolis-server/propolis-server-latest.json +++ b/openapi/propolis-server/propolis-server-latest.json @@ -1 +1 @@ -propolis-server-6.0.0-b5b984.json \ No newline at end of file +propolis-server-7.0.0-a344ff.json \ No newline at end of file diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 6514d46c7..b2cec894d 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -37,7 +37,7 @@ use propolis_client::{ InstanceEnsureRequest, InstanceGetResponse, InstanceInitializationMethod, InstanceMigrateStatusResponse, InstanceSerialConsoleHistoryResponse, InstanceState, - InstanceStateRequested, MigrationState, + InstanceStateChange, InstanceStateRequested, MigrationState, }, }; use propolis_client::{Client, ResponseValue}; @@ -558,27 +558,58 @@ impl TestVm { /// Sets the VM to the running state without first sending an instance /// ensure request. pub async fn run(&self) -> PropolisClientResult<()> { - self.put_instance_state(InstanceStateRequested::Run).await + self.put_instance_state(InstanceStateRequested::Run, None).await } /// Stops the VM. pub async fn stop(&self) -> PropolisClientResult<()> { - self.put_instance_state(InstanceStateRequested::Stop).await + self.put_instance_state(InstanceStateRequested::Stop, None).await } /// Resets the VM by requesting the `Reboot` state from the server (as /// distinct from requesting a reboot from within the guest). pub async fn reset(&self) -> PropolisClientResult<()> { - self.put_instance_state(InstanceStateRequested::Reboot).await + self.put_instance_state(InstanceStateRequested::Reboot, None).await + } + + /// Sends a power button press, and forcefully stops the VM after the given + /// timeout if the guest hasn't shut itself down by then. + pub async fn acpi_shutdown( + &self, + timeout_secs: u64, + ) -> PropolisClientResult<()> { + self.put_instance_state( + InstanceStateRequested::Stop, + Some(timeout_secs), + ) + .await + } + + /// Reboots the VM after a guest shutdown triggered by a power button press, + /// or forcefully after the given timeout if the guest does not shut down. + pub async fn acpi_reset( + &self, + timeout_secs: u64, + ) -> PropolisClientResult<()> { + self.put_instance_state( + InstanceStateRequested::Reboot, + Some(timeout_secs), + ) + .await } #[instrument(skip_all, fields(vm = self.spec.vm_name, vm_id = %self.id))] async fn put_instance_state( &self, state: InstanceStateRequested, + acpi_timeout_secs: Option, ) -> PropolisClientResult<()> { info!(?state, "Requesting instance state change"); - self.client.instance_state_put().body(state).send().await + self.client + .instance_state_put() + .body(InstanceStateChange { state, acpi_timeout_secs }) + .send() + .await } /// Issues a Propolis client `instance_get` request. @@ -1195,7 +1226,10 @@ async fn try_ensure_vm_destroyed(client: &Client) { debug!("trying to ensure Propolis server VM is destroyed"); if let Err(error) = client .instance_state_put() - .body(InstanceStateRequested::Stop) + .body(InstanceStateChange { + state: InstanceStateRequested::Stop, + acpi_timeout_secs: None, + }) .send() .await { diff --git a/phd-tests/tests/src/acpi_shutdown.rs b/phd-tests/tests/src/acpi_shutdown.rs new file mode 100644 index 000000000..d31eb2600 --- /dev/null +++ b/phd-tests/tests/src/acpi_shutdown.rs @@ -0,0 +1,144 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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/. + +use std::time::Duration; + +use phd_framework::guest_os::GuestOsKind; +use phd_testcase::{phd_framework::test_vm::MigrationTimeout, *}; + +use propolis_client::types::InstanceState; +use uuid::Uuid; + +/// start an alpine TestVm that will *not* respond to pwrbtn events. +async fn non_acpid_vm( + ctx: &TestCtx, + vm_name: &str, +) -> phd_testcase::Result { + if ctx.default_guest_os_kind().await? != GuestOsKind::Alpine { + // other more heavyweight distros probably listen for pwrbtn events + phd_skip!("acpi_shutdown tests expect alpine default behaviors"); + } + + let mut vm = ctx.spawn_default_vm(vm_name).await?; + vm.launch().await?; + vm.wait_to_boot().await?; + Ok(vm) +} + +/// start an alpine TestVm that *will* respond to pwrbtn events. +async fn acpid_vm( + ctx: &TestCtx, + vm_name: &str, +) -> phd_testcase::Result { + let vm = non_acpid_vm(ctx, vm_name).await?; + + // alpine won't respond to acpi pwrbtn events without acpid running. + vm.run_shell_command("acpid").await?; + + Ok(vm) +} + +#[phd_testcase] +async fn acpi_shutdown_stop(ctx: &TestCtx) { + let vm = acpid_vm(ctx, "acpi_shutdown_stop").await?; + + // pwrbtn with intent to stop + vm.acpi_shutdown(60).await?; + + // note: timeout *before* the above elapsed, we specifically want to see + // that the guest reacted to the power button press by shutting itself down + vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(30)) + .await?; +} + +#[phd_testcase] +async fn acpi_shutdown_reboot(ctx: &TestCtx) { + let vm = acpid_vm(ctx, "acpi_shutdown_reboot").await?; + + for i in 0..5 { + // pwrbtn with intent to reboot, alternatingly being + // likely to be hard cut-off by the timeout + vm.acpi_reset(if i & 1 == 0 { 30 } else { 1 }).await?; + + // wait for the login prompt again + vm.wait_to_boot().await?; + } +} + +#[phd_testcase] +async fn acpi_shutdown_interject_with_hard_stop(ctx: &TestCtx) { + let vm = + non_acpid_vm(ctx, "acpi_shutdown_interject_with_hard_stop").await?; + + // note: *not* running acpid, so alpine will ignore pwrbtn. + // sending a 'reset' request so we can be sure it was ignored when the + // instance ends up destroyed + vm.acpi_reset(60).await?; + + // give up waiting for timeout to elapse + tokio::time::sleep(Duration::from_secs(3)).await; + vm.stop().await?; + + vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(30)) + .await?; +} + +#[phd_testcase] +async fn acpi_shutdown_interject_with_hard_reboot(ctx: &TestCtx) { + let vm = + non_acpid_vm(ctx, "acpi_shutdown_interject_with_hard_reboot").await?; + + // note: *not* running acpid, so alpine will ignore pwrbtn. + // sending a 'shutdown' request so we can be sure it was ignored when the + // instance ends up at login prompt again + vm.acpi_shutdown(60).await?; + + // give up waiting for timeout to elapse + tokio::time::sleep(Duration::from_secs(3)).await; + vm.reset().await?; + + vm.wait_to_boot().await?; +} + +#[phd_testcase] +async fn acpi_shutdown_timeout_stop(ctx: &TestCtx) { + let vm = non_acpid_vm(ctx, "acpi_shutdown_timeout_stop").await?; + + // note: *not* running acpid, so alpine should ignore pwrbtn + // and be hard-stopped in 1 second + vm.acpi_shutdown(1).await?; + + vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(5)).await?; +} + +#[phd_testcase] +async fn acpi_shutdown_timeout_reboot(ctx: &TestCtx) { + let vm = non_acpid_vm(ctx, "acpi_shutdown_timeout_reboot").await?; + + // note: *not* running acpid, so alpine should ignore pwrbtn + // and be hard-reset in 1 second + vm.acpi_reset(1).await?; + + // wait for the login prompt again + vm.wait_to_boot().await?; +} + +#[phd_testcase] +async fn acpi_shutdown_reboot_then_migrate(ctx: &TestCtx) { + let vm0 = acpid_vm(ctx, "acpi_shutdown_reboot_then_migrate_vm0").await?; + let mut vm1 = ctx + .spawn_successor_vm("acpi_shutdown_reboot_then_migrate_vm1", &vm0, None) + .await?; + + vm0.acpi_reset(30).await?; + // wait for the login prompt again + vm0.wait_to_boot().await?; + + vm1.migrate_from(&vm0, Uuid::new_v4(), MigrationTimeout::default()).await?; + + // and let's try that again on the successor + vm1.acpi_reset(30).await?; + // wait for the login prompt yet again + vm1.wait_to_boot().await?; +} diff --git a/phd-tests/tests/src/lib.rs b/phd-tests/tests/src/lib.rs index 2e061051a..1cdec013c 100644 --- a/phd-tests/tests/src/lib.rs +++ b/phd-tests/tests/src/lib.rs @@ -4,6 +4,7 @@ pub use phd_testcase; +mod acpi_shutdown; mod boot_order; mod cpuid; mod crucible; From bd5596b957d80595da9ffd8ba2965385c48d3e76 Mon Sep 17 00:00:00 2001 From: lif <> Date: Fri, 2 Oct 2026 07:46:43 +0000 Subject: [PATCH 2/6] update mock-server instance_state_put to match real server --- bin/mock-server/src/lib/lib.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/bin/mock-server/src/lib/lib.rs b/bin/mock-server/src/lib/lib.rs index 345738a43..f2aea8955 100644 --- a/bin/mock-server/src/lib/lib.rs +++ b/bin/mock-server/src/lib/lib.rs @@ -344,7 +344,7 @@ async fn instance_state_monitor( }] async fn instance_state_put( rqctx: RequestContext>, - request: TypedBody, + request: TypedBody, ) -> Result { let mut instance = rqctx.context().instance.lock().await; let instance = instance.as_mut().ok_or_else(|| { @@ -353,13 +353,14 @@ async fn instance_state_put( ) })?; let requested_state = request.into_inner(); - instance.set_target_state(&rqctx.log, requested_state).await.map_err( - |err| { + instance + .set_target_state(&rqctx.log, requested_state.state) + .await + .map_err(|err| { HttpError::for_internal_error(format!( "Failed to transition: {err}" )) - }, - )?; + })?; Ok(HttpResponseUpdatedNoContent {}) } From 596c3ebb612b26f98c80ab4fdc044d208282ce5e Mon Sep 17 00:00:00 2001 From: lif <> Date: Wed, 14 Oct 2026 07:09:02 +0000 Subject: [PATCH 3/6] pr nits --- .../src/lib/vm/state_driver.rs | 83 ++++++++++--------- lib/propolis/src/hw/chipset/i440fx.rs | 2 +- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index ea3899b01..18c23b6dd 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -880,43 +880,43 @@ impl StateDriver { fate, timeout, }) => { - // if timeout already in progress, ignore further soft-off reqs - if self.soft_off.is_none() { - let guard = self.objects.lock_exclusive().await; - let chipset = guard.chipset(); - - chipset.acpi_shutdown(); - // TODO? might watch for OSPM clearing the status bit to - // determine if guest acknowledges the button press - - // timeout to hard-stop/reset if guest takes too long - let input_queue_weak = Arc::downgrade(&self.input_queue); - // (philosophically, timeout behavior is part of what was - // externally requested...) - let quasi_ext_req = match fate { - SoftShutdownFate::Stop => ExternalRequest::stop(), - SoftShutdownFate::Reboot => ExternalRequest::reboot(), - }; - let log = self.log.clone(); - self.soft_off = Some(( - fate, - tokio::spawn(async move { - tokio::time::sleep(timeout).await; - if let Some(input_queue) = - input_queue_weak.upgrade() + if self.soft_off.is_some() { + // timeout already in progress, ignore further soft-off reqs + return HandleEventOutcome::Continue; + } + + let guard = self.objects.lock_exclusive().await; + let chipset = guard.chipset(); + + chipset.acpi_shutdown(); + // TODO? might watch for OSPM clearing the status bit to + // determine if guest acknowledges the button press + + // timeout to hard-stop/reset if guest takes too long + let input_queue_weak = Arc::downgrade(&self.input_queue); + // (philosophically, timeout behavior is part of what was + // externally requested...) + let quasi_ext_req = match fate { + SoftShutdownFate::Stop => ExternalRequest::stop(), + SoftShutdownFate::Reboot => ExternalRequest::reboot(), + }; + let log = self.log.clone(); + self.soft_off = Some(( + fate, + tokio::spawn(async move { + tokio::time::sleep(timeout).await; + if let Some(input_queue) = input_queue_weak.upgrade() { + if let Err(e) = input_queue + .queue_external_request(quasi_ext_req) { - if let Err(e) = input_queue - .queue_external_request(quasi_ext_req) - { - slog::error!( - log, - "Failed to enqueue {fate:?}: {e}" - ); - } + slog::error!( + log, + "Failed to enqueue {fate:?}: {e}" + ); } - }), - )); - } + } + }), + )); HandleEventOutcome::Continue } ExternalRequest::State(StateChangeRequest::Start) => { @@ -985,7 +985,13 @@ impl StateDriver { async fn do_reboot(&mut self) { info!(self.log, "resetting instance"); - // first abandon any timeout task that may send a redundant request + // first, abandon any timeout task that may send a redundant request. + // if the guest has, say, raced us with an independently-originating + // power signal, at worst we'd process a couple reboots in quick + // succession. (what we *don't* want is for the guest to successfully + // shut down while we're waiting for it to do so, have us reboot it + // here, and then have the timeout task hard-reboot minutes later + // once it's come back up) if let Some((_, timeout_task)) = self.soft_off.take() { timeout_task.abort(); } @@ -1004,7 +1010,10 @@ impl StateDriver { async fn do_halt(&mut self) { info!(self.log, "stopping instance"); - // first abandon any timeout task that may send a redundant request + // first, abandon any timeout task that may send a redundant request. + // (if the guest has raced us with a power signal, turning off what + // is already off is fine - but we certainly don't want someone turning + // the instance back *on* later, then our old timeout task offs it) if let Some((_, timeout_task)) = self.soft_off.take() { timeout_task.abort(); } diff --git a/lib/propolis/src/hw/chipset/i440fx.rs b/lib/propolis/src/hw/chipset/i440fx.rs index 2a1523ed6..58d89ffa0 100644 --- a/lib/propolis/src/hw/chipset/i440fx.rs +++ b/lib/propolis/src/hw/chipset/i440fx.rs @@ -103,7 +103,6 @@ struct IrqConfig { lnk_pins: [Arc; 4], - // XXX: wire up SCI notifications sci_pin: Arc, } impl IrqConfig { @@ -854,6 +853,7 @@ impl Piix3PM { pio.register(PMBASE_DEFAULT, PMBASE_LEN, piofn).unwrap(); } + // XXX: handle the logical OR of "SCI" inputs when we have many, see future hotplug work pub fn acpi_shutdown(&self) { let mut regs = self.regs.lock().unwrap(); regs.pm_status.insert(PmSts::PWRBTN_STS); From a611594f646c57c544cd9e4c4ecb7e1e9c10b766 Mon Sep 17 00:00:00 2001 From: lif <> Date: Thu, 15 Oct 2026 05:57:01 +0000 Subject: [PATCH 4/6] let the operator change their mind about an instance's post-shutdown destiny. the future is whatever you make it --- .../src/lib/vm/request_queue.rs | 32 +++++----- .../src/lib/vm/state_driver.rs | 60 ++++++++++++------- phd-tests/tests/src/acpi_shutdown.rs | 17 ++++++ 3 files changed, 71 insertions(+), 38 deletions(-) diff --git a/bin/propolis-server/src/lib/vm/request_queue.rs b/bin/propolis-server/src/lib/vm/request_queue.rs index 86ff8c47c..1390f2c08 100644 --- a/bin/propolis-server/src/lib/vm/request_queue.rs +++ b/bin/propolis-server/src/lib/vm/request_queue.rs @@ -407,7 +407,9 @@ impl ExternalRequestQueue { ExternalRequest::State(StateChangeRequest::ACPIShutdown { .. }) => { - assert!(!self.awaiting_shutdown); + // subsequent shutdown requests may be seeking to change + // the post-shutdown instance fate (stop vs. reboot), + // so don't assert !awaiting_shutdown self.awaiting_shutdown = true; } ExternalRequest::State(StateChangeRequest::Reboot) => { @@ -509,7 +511,8 @@ impl ExternalRequestQueue { // Reject ACPI shutdown requests if the instance is not in a // state where it could reasonably be expected to respond to // a power button press (that is to say, Running), and also - // ignore further shutdown requests if one is already happening. + // allow further shutdown requests if one is already happening + // to allow changing the post-shutdown action (stop/reboot). ExternalRequest::State(StateChangeRequest::ACPIShutdown { .. }) => match self.state { @@ -531,8 +534,6 @@ impl ExternalRequestQueue { QueueState::Running => { if self.awaiting_stop || self.awaiting_reboot { return Err(RequestDeniedReason::HaltPending); - } else if self.awaiting_shutdown { - return Ok(false); } } }, @@ -910,7 +911,7 @@ mod test { } #[test] - fn acpi_shutdowns_ignored_after_first() { + fn acpi_shutdowns_after_acpi_shutdown_allowed() { let mut queue = ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes); queue.notify_request_completed(CompletedRequest::Start { @@ -926,18 +927,17 @@ mod test { )) .is_ok()); - // all these further ACPI shutdown requests should be ignored - // (with intent sent to reboot, just so we can distinguish in this test - // that only the first request was enqueued) - for _ in 0..5 { - assert!(queue - .try_queue(ExternalRequest::acpi_shutdown( - SoftShutdownFate::Reboot, - Duration::from_secs(600) - )) - .is_ok()); - } + // a further ACPI shutdown request with intent sent to reboot, + // just so we can distinguish from the previous one + assert!(queue + .try_queue(ExternalRequest::acpi_shutdown( + SoftShutdownFate::Reboot, + Duration::from_secs(600) + )) + .is_ok()); + queue.pop_front().unwrap().assert_acpi_shutdown(); + queue.pop_front().unwrap().assert_acpi_reboot(); assert!(queue.is_empty()); } diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index 18c23b6dd..c1bba6b05 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -382,7 +382,10 @@ struct StateDriver { /// Contains a tuple of the action to take after the guest halts its CPUs, /// the timeout task that will forcibly halt the guest's CPUs if it is not /// aborted. - soft_off: Option<(SoftShutdownFate, tokio::task::JoinHandle<()>)>, + soft_off: Option<( + Arc>, + tokio::task::JoinHandle<()>, + )>, /// State persisted from previous attempts to migrate out of this VM. migration_src_state: crate::migrate::source::PersistentState, @@ -824,7 +827,8 @@ impl StateDriver { // asked for the guest to be *off*. if let Some((fate, _)) = &self.soft_off { self.input_queue.notify_shutdown(); - return match fate { + let fate_val = *fate.lock().await; + return match fate_val { SoftShutdownFate::Stop => { info!( self.log, @@ -880,8 +884,11 @@ impl StateDriver { fate, timeout, }) => { - if self.soft_off.is_some() { - // timeout already in progress, ignore further soft-off reqs + if let Some((fate_mut, _)) = &mut self.soft_off { + // timeout already in progress, don't start timeout task + // anew or send further PWRBTN_STS, but do change the + // post-shutdown fate + *fate_mut.lock().await = fate; return HandleEventOutcome::Continue; } @@ -889,31 +896,40 @@ impl StateDriver { let chipset = guard.chipset(); chipset.acpi_shutdown(); - // TODO? might watch for OSPM clearing the status bit to - // determine if guest acknowledges the button press + + // TODO? in future we may watch for OSPM clearing the status bit + // (per ACPI 4.8.2.2.1.1) after above chipset.acpi_shutdown call + // call to determine whether the guest has acknowledged the push + // of the power button. mayhap consider adding a tighter timeout + // task to terminate/reboot sooner if the kernel is so wedged or + // absent that it didn't even respond to pulling PWRBTN_STS high // timeout to hard-stop/reset if guest takes too long - let input_queue_weak = Arc::downgrade(&self.input_queue); - // (philosophically, timeout behavior is part of what was - // externally requested...) - let quasi_ext_req = match fate { - SoftShutdownFate::Stop => ExternalRequest::stop(), - SoftShutdownFate::Reboot => ExternalRequest::reboot(), - }; + let queue_weak = Arc::downgrade(&self.input_queue); + let fate_strong = Arc::new(tokio::sync::Mutex::new(fate)); + let fate_weak = Arc::downgrade(&fate_strong); let log = self.log.clone(); self.soft_off = Some(( - fate, + fate_strong, tokio::spawn(async move { tokio::time::sleep(timeout).await; - if let Some(input_queue) = input_queue_weak.upgrade() { - if let Err(e) = input_queue - .queue_external_request(quasi_ext_req) - { - slog::error!( - log, - "Failed to enqueue {fate:?}: {e}" - ); + let Some(queue) = queue_weak.upgrade() else { return }; + let Some(fate) = fate_weak.upgrade() else { return }; + let quasi_ext_req = match *fate.lock().await { + SoftShutdownFate::Stop => ExternalRequest::stop(), + SoftShutdownFate::Reboot => { + ExternalRequest::reboot() } + }; + // (philosophically, timeout behavior is part of + // what was externally-requested...) + if let Err(e) = + queue.queue_external_request(quasi_ext_req) + { + slog::error!( + log, + "Failed to enqueue {fate:?}: {e}" + ); } }), )); diff --git a/phd-tests/tests/src/acpi_shutdown.rs b/phd-tests/tests/src/acpi_shutdown.rs index d31eb2600..d97fb774c 100644 --- a/phd-tests/tests/src/acpi_shutdown.rs +++ b/phd-tests/tests/src/acpi_shutdown.rs @@ -101,6 +101,23 @@ async fn acpi_shutdown_interject_with_hard_reboot(ctx: &TestCtx) { vm.wait_to_boot().await?; } +#[phd_testcase] +async fn acpi_shutdown_change_of_fate(ctx: &TestCtx) { + let vm = + non_acpid_vm(ctx, "acpi_shutdown_interject_with_soft_reboot").await?; + + // note: *not* running acpid, so alpine will ignore pwrbtn. + // sending a 'shutdown' request so we can be sure we don't race with a + // very fast guest shutdown, but rather our given timeout + vm.acpi_reset(10).await?; + // this shouldn't change the timeout, but it should make the instance stop + // after the *previous* timeout + vm.acpi_shutdown(60).await?; + + vm.wait_for_state(InstanceState::Destroyed, Duration::from_secs(20)) + .await?; +} + #[phd_testcase] async fn acpi_shutdown_timeout_stop(ctx: &TestCtx) { let vm = non_acpid_vm(ctx, "acpi_shutdown_timeout_stop").await?; From c6e6b2fe83fcba8c9925c18573cd8a9883944320 Mon Sep 17 00:00:00 2001 From: lif <> Date: Sat, 17 Oct 2026 04:30:37 +0000 Subject: [PATCH 5/6] add explicit InstanceRequestedState variants for AcpiStop and AcpiReboot instead of just implying it via the presence of timeout --- bin/propolis-cli/src/main.rs | 7 ++- bin/propolis-server/src/lib/vm/active.rs | 35 ++++++----- .../src/lib/vm/request_queue.rs | 31 +++++----- .../src/lib/vm/state_driver.rs | 59 +++++++++---------- .../src/acpi_shutdown/instance.rs | 24 +++++++- .../propolis-api-types-versions/src/latest.rs | 2 +- crates/propolis-server-api/src/lib.rs | 2 +- ...json => propolis-server-7.0.0-772a7f.json} | 6 +- .../propolis-server-latest.json | 2 +- phd-tests/framework/src/test_vm/mod.rs | 4 +- 10 files changed, 100 insertions(+), 72 deletions(-) rename openapi/propolis-server/{propolis-server-7.0.0-a344ff.json => propolis-server-7.0.0-772a7f.json} (99%) diff --git a/bin/propolis-cli/src/main.rs b/bin/propolis-cli/src/main.rs index 0690a4bcf..aae5c889f 100644 --- a/bin/propolis-cli/src/main.rs +++ b/bin/propolis-cli/src/main.rs @@ -122,7 +122,7 @@ enum Command { #[clap(value_parser = parse_state)] state: InstanceStateRequested, /// The number of seconds to wait after sending ACPI PWRBTN_STS before - /// forcing stop/reset. (If omitted, stop/reset are forced immediately) + /// forcing stop/reset. (Only valid on acpi-stop and acpi-reboot states) acpi_timeout_secs: Option, }, @@ -452,8 +452,11 @@ fn parse_state(state: &str) -> anyhow::Result { "run" => Ok(InstanceStateRequested::Run), "stop" => Ok(InstanceStateRequested::Stop), "reboot" => Ok(InstanceStateRequested::Reboot), + "acpi-stop" => Ok(InstanceStateRequested::AcpiStop), + "acpi-reboot" => Ok(InstanceStateRequested::AcpiReboot), _ => Err(anyhow!( - "invalid requested state, must be one of: 'run', 'stop', 'reboot" + "invalid requested state, must be one of: \ + 'run', 'stop', 'reboot', 'acpi-stop', 'acpi-reboot'" )), } } diff --git a/bin/propolis-server/src/lib/vm/active.rs b/bin/propolis-server/src/lib/vm/active.rs index 8f0cda22c..6816afa13 100644 --- a/bin/propolis-server/src/lib/vm/active.rs +++ b/bin/propolis-server/src/lib/vm/active.rs @@ -65,21 +65,26 @@ impl ActiveVm { "state" => ?requested.state, "timeout" => ?requested.acpi_timeout_secs); - let ext_req = if let Some(secs) = requested.acpi_timeout_secs { - let timeout = Duration::from_secs(secs); - let fate = match requested.state { - InstanceStateRequested::Run => { - return Err(RequestDeniedReason::TimeoutOnRun.into()); - } - InstanceStateRequested::Stop => SoftShutdownFate::Stop, - InstanceStateRequested::Reboot => SoftShutdownFate::Reboot, - }; - ExternalRequest::acpi_shutdown(fate, timeout) - } else { - match requested.state { - InstanceStateRequested::Run => ExternalRequest::start(), - InstanceStateRequested::Stop => ExternalRequest::stop(), - InstanceStateRequested::Reboot => ExternalRequest::reboot(), + let timeout = requested.acpi_timeout_secs.map(Duration::from_secs); + let ext_req = match requested.state { + InstanceStateRequested::Run + | InstanceStateRequested::Stop + | InstanceStateRequested::Reboot + if timeout.is_some() => + { + return Err(RequestDeniedReason::TimeoutOnNonAcpiState.into()); + } + InstanceStateRequested::Run => ExternalRequest::start(), + InstanceStateRequested::Stop => ExternalRequest::stop(), + InstanceStateRequested::Reboot => ExternalRequest::reboot(), + InstanceStateRequested::AcpiStop => { + ExternalRequest::acpi_shutdown(SoftShutdownFate::Stop, timeout) + } + InstanceStateRequested::AcpiReboot => { + ExternalRequest::acpi_shutdown( + SoftShutdownFate::Reboot, + timeout, + ) } }; self.state_driver_queue diff --git a/bin/propolis-server/src/lib/vm/request_queue.rs b/bin/propolis-server/src/lib/vm/request_queue.rs index 1390f2c08..a5e33c843 100644 --- a/bin/propolis-server/src/lib/vm/request_queue.rs +++ b/bin/propolis-server/src/lib/vm/request_queue.rs @@ -66,7 +66,10 @@ pub(crate) enum StateChangeRequest { /// or reboots the instance (depending on `fate`) once the guest halts /// its CPU. If this does not occur within the `timeout`, the guest is /// ungracefully stopped/rebooted. - ACPIShutdown { fate: SoftShutdownFate, timeout: std::time::Duration }, + ACPIShutdown { + fate: SoftShutdownFate, + timeout: Option, + }, /// Halts the VM. Note that this is not a graceful shutdown and does not /// coordinate with guest software. @@ -144,7 +147,7 @@ impl ExternalRequest { pub const fn acpi_shutdown( fate: SoftShutdownFate, - timeout: std::time::Duration, + timeout: Option, ) -> Self { Self::State(StateChangeRequest::ACPIShutdown { fate, timeout }) } @@ -227,8 +230,8 @@ pub(crate) enum RequestDeniedReason { #[error("Instance failed to start or halted due to a failure")] InstanceFailed, - #[error("Cannot supply shutdown timeout to a Run request")] - TimeoutOnRun, + #[error("Cannot supply timeout with non-ACPI-shutdown state requests")] + TimeoutOnNonAcpiState, } /// A kind of request that can be popped from the queue and then completed. @@ -923,7 +926,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); @@ -932,7 +935,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Reboot, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); @@ -954,7 +957,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); // enqueue a hard stop @@ -979,7 +982,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Reboot, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); // enqueue a hard stop @@ -1004,7 +1007,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); // enqueue a hard stop @@ -1028,7 +1031,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); assert!(queue.is_empty()); @@ -1049,7 +1052,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_err()); @@ -1061,7 +1064,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); @@ -1081,7 +1084,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_err()); @@ -1092,7 +1095,7 @@ mod test { assert!(queue .try_queue(ExternalRequest::acpi_shutdown( SoftShutdownFate::Stop, - Duration::from_secs(600) + Some(Duration::from_secs(600)) )) .is_ok()); diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index c1bba6b05..ac4646b77 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -384,7 +384,7 @@ struct StateDriver { /// aborted. soft_off: Option<( Arc>, - tokio::task::JoinHandle<()>, + Option>, )>, /// State persisted from previous attempts to migrate out of this VM. @@ -884,7 +884,7 @@ impl StateDriver { fate, timeout, }) => { - if let Some((fate_mut, _)) = &mut self.soft_off { + if let Some((fate_mut, Some(_))) = &mut self.soft_off { // timeout already in progress, don't start timeout task // anew or send further PWRBTN_STS, but do change the // post-shutdown fate @@ -904,35 +904,32 @@ impl StateDriver { // task to terminate/reboot sooner if the kernel is so wedged or // absent that it didn't even respond to pulling PWRBTN_STS high - // timeout to hard-stop/reset if guest takes too long - let queue_weak = Arc::downgrade(&self.input_queue); let fate_strong = Arc::new(tokio::sync::Mutex::new(fate)); + // timeout task to hard-stop/reset if guest takes too long + let Some(timeout) = timeout else { + self.soft_off = Some((fate_strong, None)); + return HandleEventOutcome::Continue; + }; + + let queue_weak = Arc::downgrade(&self.input_queue); let fate_weak = Arc::downgrade(&fate_strong); let log = self.log.clone(); - self.soft_off = Some(( - fate_strong, - tokio::spawn(async move { - tokio::time::sleep(timeout).await; - let Some(queue) = queue_weak.upgrade() else { return }; - let Some(fate) = fate_weak.upgrade() else { return }; - let quasi_ext_req = match *fate.lock().await { - SoftShutdownFate::Stop => ExternalRequest::stop(), - SoftShutdownFate::Reboot => { - ExternalRequest::reboot() - } - }; - // (philosophically, timeout behavior is part of - // what was externally-requested...) - if let Err(e) = - queue.queue_external_request(quasi_ext_req) - { - slog::error!( - log, - "Failed to enqueue {fate:?}: {e}" - ); - } - }), - )); + let timeout_task = tokio::spawn(async move { + tokio::time::sleep(timeout).await; + let Some(queue) = queue_weak.upgrade() else { return }; + let Some(fate) = fate_weak.upgrade() else { return }; + let quasi_ext_req = match *fate.lock().await { + SoftShutdownFate::Stop => ExternalRequest::stop(), + SoftShutdownFate::Reboot => ExternalRequest::reboot(), + }; + // (philosophically, timeout behavior is part of + // what was externally-requested...) + if let Err(e) = queue.queue_external_request(quasi_ext_req) + { + slog::error!(log, "Failed to enqueue {fate:?}: {e}"); + } + }); + self.soft_off = Some((fate_strong, Some(timeout_task))); HandleEventOutcome::Continue } ExternalRequest::State(StateChangeRequest::Start) => { @@ -953,7 +950,7 @@ impl StateDriver { migration_id, websock, }) => { - if let Some((fate, _)) = &self.soft_off { + if let Some((fate, Some(_))) = &self.soft_off { slog::warn!( self.log, "Ignored MigrateAsSource while ACPI {fate:?} in progress" @@ -1008,7 +1005,7 @@ impl StateDriver { // shut down while we're waiting for it to do so, have us reboot it // here, and then have the timeout task hard-reboot minutes later // once it's come back up) - if let Some((_, timeout_task)) = self.soft_off.take() { + if let Some((_, Some(timeout_task))) = self.soft_off.take() { timeout_task.abort(); } @@ -1030,7 +1027,7 @@ impl StateDriver { // (if the guest has raced us with a power signal, turning off what // is already off is fine - but we certainly don't want someone turning // the instance back *on* later, then our old timeout task offs it) - if let Some((_, timeout_task)) = self.soft_off.take() { + if let Some((_, Some(timeout_task))) = self.soft_off.take() { timeout_task.abort(); } diff --git a/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs b/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs index ed655c9c0..92446beba 100644 --- a/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs +++ b/crates/propolis-api-types-versions/src/acpi_shutdown/instance.rs @@ -5,7 +5,26 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use crate::v1::instance::InstanceStateRequested; +use crate::v1::instance::InstanceStateRequested as InstanceStateRequestedV1; + +#[derive(Clone, Copy, Debug, Deserialize, Serialize, JsonSchema)] +pub enum InstanceStateRequested { + Run, + Stop, + Reboot, + AcpiStop, + AcpiReboot, +} + +impl From for InstanceStateRequested { + fn from(value: InstanceStateRequestedV1) -> Self { + match value { + InstanceStateRequestedV1::Run => Self::Run, + InstanceStateRequestedV1::Stop => Self::Stop, + InstanceStateRequestedV1::Reboot => Self::Reboot, + } + } +} /// Requested state change of an Instance. #[derive(Clone, Copy, Deserialize, Serialize, JsonSchema)] @@ -13,7 +32,6 @@ pub struct InstanceStateChange { /// The desired state for the Instance. pub state: InstanceStateRequested, /// The number of seconds to wait after sending ACPI `PWRBTN_STS` before - /// forcing stop/reset. (If omitted, stop/reset are forced immediately, - /// and the ACPI signal is not sent.) + /// forcing stop/reset. (Only valid for AcpiStop and AcpiReboot states.) pub acpi_timeout_secs: Option, } diff --git a/crates/propolis-api-types-versions/src/latest.rs b/crates/propolis-api-types-versions/src/latest.rs index b440742c5..08e666d2d 100644 --- a/crates/propolis-api-types-versions/src/latest.rs +++ b/crates/propolis-api-types-versions/src/latest.rs @@ -67,13 +67,13 @@ pub mod instance { pub use crate::v1::instance::InstanceState; pub use crate::v1::instance::InstanceStateMonitorRequest; pub use crate::v1::instance::InstanceStateMonitorResponse; - pub use crate::v1::instance::InstanceStateRequested; pub use crate::v1::instance::ReplacementComponent; pub use crate::v6::api::InstanceEnsureRequest; pub use crate::v6::api::InstanceInitializationMethod; pub use crate::v7::instance::InstanceStateChange; + pub use crate::v7::instance::InstanceStateRequested; } pub mod instance_spec { diff --git a/crates/propolis-server-api/src/lib.rs b/crates/propolis-server-api/src/lib.rs index 684d9df26..2e4804ca0 100644 --- a/crates/propolis-server-api/src/lib.rs +++ b/crates/propolis-server-api/src/lib.rs @@ -234,7 +234,7 @@ pub trait PropolisServerApi { Self::instance_state_put( rqctx, latest::instance::InstanceStateChange { - state: request.into_inner(), + state: request.into_inner().into(), acpi_timeout_secs: None, } .into(), diff --git a/openapi/propolis-server/propolis-server-7.0.0-a344ff.json b/openapi/propolis-server/propolis-server-7.0.0-772a7f.json similarity index 99% rename from openapi/propolis-server/propolis-server-7.0.0-a344ff.json rename to openapi/propolis-server/propolis-server-7.0.0-772a7f.json index 5718d81fe..ba19b1552 100644 --- a/openapi/propolis-server/propolis-server-7.0.0-a344ff.json +++ b/openapi/propolis-server/propolis-server-7.0.0-772a7f.json @@ -1660,7 +1660,7 @@ "properties": { "acpi_timeout_secs": { "nullable": true, - "description": "The number of seconds to wait after sending ACPI `PWRBTN_STS` before forcing stop/reset. (If omitted, stop/reset are forced immediately, and the ACPI signal is not sent.)", + "description": "The number of seconds to wait after sending ACPI `PWRBTN_STS` before forcing stop/reset. (Only valid for AcpiStop and AcpiReboot states.)", "type": "integer", "format": "uint64", "minimum": 0 @@ -1717,7 +1717,9 @@ "enum": [ "Run", "Stop", - "Reboot" + "Reboot", + "AcpiStop", + "AcpiReboot" ] }, "InstanceVCRReplace": { diff --git a/openapi/propolis-server/propolis-server-latest.json b/openapi/propolis-server/propolis-server-latest.json index b1cd71342..b842b5ebd 120000 --- a/openapi/propolis-server/propolis-server-latest.json +++ b/openapi/propolis-server/propolis-server-latest.json @@ -1 +1 @@ -propolis-server-7.0.0-a344ff.json \ No newline at end of file +propolis-server-7.0.0-772a7f.json \ No newline at end of file diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index b2cec894d..5c1fbcc97 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -579,7 +579,7 @@ impl TestVm { timeout_secs: u64, ) -> PropolisClientResult<()> { self.put_instance_state( - InstanceStateRequested::Stop, + InstanceStateRequested::AcpiStop, Some(timeout_secs), ) .await @@ -592,7 +592,7 @@ impl TestVm { timeout_secs: u64, ) -> PropolisClientResult<()> { self.put_instance_state( - InstanceStateRequested::Reboot, + InstanceStateRequested::AcpiReboot, Some(timeout_secs), ) .await From 463fb8850a1b1d0a472bc650f6edfa1da9d25714 Mon Sep 17 00:00:00 2001 From: lif <> Date: Sat, 17 Oct 2026 05:12:45 +0000 Subject: [PATCH 6/6] update mock-server to allow inspecting most-recently-sent state change request (& clippy fix in statedriver) --- bin/mock-server/src/lib/lib.rs | 53 ++++++++++++++++--- .../src/lib/vm/state_driver.rs | 9 ++-- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/bin/mock-server/src/lib/lib.rs b/bin/mock-server/src/lib/lib.rs index f2aea8955..667d1dc19 100644 --- a/bin/mock-server/src/lib/lib.rs +++ b/bin/mock-server/src/lib/lib.rs @@ -47,6 +47,7 @@ pub struct InstanceContext { serial_task: serial::SerialTask, state_watcher_rx: watch::Receiver, state_watcher_tx: watch::Sender, + last_state_change: Option, } struct MockState { @@ -92,6 +93,7 @@ impl InstanceContext { serial_task, state_watcher_rx, state_watcher_tx, + last_state_change: None, } } @@ -126,7 +128,8 @@ impl InstanceContext { } ( api::InstanceState::Running, - api::InstanceStateRequested::Reboot, + api::InstanceStateRequested::Reboot + | api::InstanceStateRequested::AcpiReboot, ) => { self.queue_states( log, @@ -138,12 +141,26 @@ impl InstanceContext { .await; Ok(()) } - (current, api::InstanceStateRequested::Reboot) => { - Err(Error::InvalidTransition { - current, - requested: api::InstanceStateRequested::Reboot, - }) + ( + api::InstanceState::Running, + api::InstanceStateRequested::AcpiStop, + ) => { + self.queue_states( + log, + &[ + api::InstanceState::Stopping, + api::InstanceState::Stopped, + ], + ) + .await; + Ok(()) } + ( + current, + api::InstanceStateRequested::Reboot + | api::InstanceStateRequested::AcpiReboot + | api::InstanceStateRequested::AcpiStop, + ) => Err(Error::InvalidTransition { current, requested: target }), (_, api::InstanceStateRequested::Run) => { self.queue_states(log, &[api::InstanceState::Running]).await; Ok(()) @@ -361,6 +378,7 @@ async fn instance_state_put( "Failed to transition: {err}" )) })?; + instance.last_state_change = Some(requested_state); Ok(HttpResponseUpdatedNoContent {}) } @@ -557,6 +575,28 @@ async fn mock_step( Ok(HttpResponseUpdatedNoContent()) } +// used for checking acpi shutdown timeout values in controlplane tests +#[endpoint { + method = GET, + path = "/mock/last_state_requested" +}] +async fn mock_last_state_requested_get( + rqctx: RequestContext>, +) -> Result, HttpError> { + let instance = rqctx.context().instance.lock().await; + let instance = instance.as_ref().ok_or_else(|| { + HttpError::for_internal_error( + "Server not initialized (no instance)".to_string(), + ) + })?; + match &instance.last_state_change { + Some(change) => Ok(HttpResponseOk(change.to_owned())), + None => Err(HttpError::for_internal_error( + "No instance_state_put yet received".to_string(), + )), + } +} + mod serial { use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; @@ -833,6 +873,7 @@ pub fn api() -> ApiDescription> { api.register(mock_mode_get).unwrap(); api.register(mock_mode_set).unwrap(); api.register(mock_step).unwrap(); + api.register(mock_last_state_requested_get).unwrap(); api } diff --git a/bin/propolis-server/src/lib/vm/state_driver.rs b/bin/propolis-server/src/lib/vm/state_driver.rs index ac4646b77..fee1b3380 100644 --- a/bin/propolis-server/src/lib/vm/state_driver.rs +++ b/bin/propolis-server/src/lib/vm/state_driver.rs @@ -102,7 +102,7 @@ use propolis_api_types::instance_spec::{ }; use propolis_api_types::migration::MigrationState; use slog::{error, info}; -use tokio::sync::Notify; +use tokio::{sync::Notify, task::JoinHandle}; use uuid::Uuid; use crate::{ @@ -360,6 +360,8 @@ impl guest_event::ChipsetEventHandler for InputQueue { } } +type ShutdownFateMutex = Arc>; + /// The context for a VM state driver task's main loop. struct StateDriver { /// The state driver's associated logger. @@ -382,10 +384,7 @@ struct StateDriver { /// Contains a tuple of the action to take after the guest halts its CPUs, /// the timeout task that will forcibly halt the guest's CPUs if it is not /// aborted. - soft_off: Option<( - Arc>, - Option>, - )>, + soft_off: Option<(ShutdownFateMutex, Option>)>, /// State persisted from previous attempts to migrate out of this VM. migration_src_state: crate::migrate::source::PersistentState,