Repository navigation
Conversation
f3aa4de to
2453efc
Compare
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 <lgfa29@gmail.com>
2453efc to
0b3e6b4
Compare
iximeow
left a comment
There was a problem hiding this comment.
a smattering of thoughts, this is easily half me trying to put the state bits of propolis-server back into my head (sorry!)
thanks for the work on this, and alpine-sans-apicd ignoring the button press makes for a very useful test case..
| self.put_instance_state( | ||
| InstanceStateRequested::Stop, | ||
| Some(timeout_secs), | ||
| ) |
There was a problem hiding this comment.
fwiw a thought on my mind is if we might want to spell the ACPI operation as an InstanceStateRequested::AcpiStop (and below, InstanceStateRequested::AcpiReboot) where the optional timeout says "if it didn't X in Yms, then X it ourselves".. I suppose in reality the difference isn't all that important. but it comes to mind in part because "what's InstanceStateRequested::Run with a "30s timeout" mean?"
other than the absurd Run state request, I mostly feel like "get the OS to seen SCI pulse with no later obligation for the VMM" would be interesting for testing. but in our bold systemd future we've given up on userland getting to notice power buttons itself..
| // 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 |
There was a problem hiding this comment.
stray comment maybe? or do you mean re. all the remaining lines in this arm?
There was a problem hiding this comment.
in some respect it's probably slightly conceptually misplaced for this comment as-written to be in StateDriver, ultimately being in reference to how "OSPM responds to the event by clearing the PWRBTN_STS bit"[ACPI 4.8.2.2.1.1]. my thought is that if the OS doesn't do this, we can assume the kernel (if any) is completely wedged and not bother with the longer timeout, and this was the place i figured we would put such a tighter timeout task to check if the bit was cleared.
| // first abandon any timeout task that may send a redundant request | ||
| if let Some((_, timeout_task)) = self.soft_off.take() { | ||
| timeout_task.abort(); | ||
| } | ||
|
|
There was a problem hiding this comment.
something I had to think about a bit: a guest-initiated reboot could race with the timeout and mean that there's already a pending ExternalRequest::reboot() that we'll process pretty much as soon as we're out of this function. that's ok, because in the reset case it's no different from someone poking the reset button a bunch. but the comment probably could note that we may have already lost the race with a redundant op enqueued? (it really seems like we don't want to go remove it from the queue, since we'd need to distinguish it from a bona fide API request and that would need more different plumbing..)
in the halt case you're.. turning off the thing that's off, so abstractly that's OK. but in reality as soon as we've processed the first stop we're out of event_loop anyway, so the second would never be seen.
| // state. | ||
| ExternalRequest::State(StateChangeRequest::Start) => { | ||
| if self.awaiting_stop { | ||
| if self.awaiting_stop || self.awaiting_shutdown { |
There was a problem hiding this comment.
I'm having a hard time puzzling through the bools we're accumulating here.. we could be awaiting_shutdown because of a soft-off+reboot, which means denying a start because of HaltPending is both true and .. not? concretely, I'm wondering things like, "what is awaiting_shutdown if we had an ACPI restart but then preempted that with a force stop?" - I think we'd end up in awaiting_shutdown = true after stopping the VM?
so overall I'm both wondering if this should still operate in terms of "awaiting stop" and "awaiting reboot", since we're kind of processing ACPIShutdown as a stand-in for the eventual fate, right?
There was a problem hiding this comment.
"what is awaiting_shutdown if we had an ACPI restart but then preempted that with a force stop?" - I think we'd end up in awaiting_shutdown = true after stopping the VM?
if i'm understanding your scenario correctly, i believe we'd end up with awaiting_shutdown = false per this part of try_queue
propolis/bin/propolis-server/src/lib/vm/request_queue.rs
Lines 418 to 422 in bd5596b
meanwhile i think if we treated an ACPIShutdown as an awaiting_stop/awaiting_reboot according to its fate, we may end up ignoring such a force-stop request 'for idempotency'
propolis/bin/propolis-server/src/lib/vm/request_queue.rs
Lines 443 to 447 in bd5596b
which we could of course change, but at some point we do need to distinguish between the two if we're to allow force-stops/reboots through while we're awaiting an unforceful shutdown (and the way it's currently wired, to allow the timeout's request to be processed as well)
with all that said i do agree that the number of anticipatory booleans does begin to make me nervous!
| } else if self.awaiting_shutdown { | ||
| return Ok(false); |
There was a problem hiding this comment.
my read is that at this point we'd ignore a soft-power-off if the VM had seen a previous soft-reboot. that seems fine (especially in consideration that we don't want to ever-extend the deadline), but I'm thinking through this from the console PoV: say I've got an instance with a 10-minute default timer on shutdown/reboot events. if I've tried to reboot my instance and it seems stuck, then I wait 10 seconds and get bored and irritated and try to stop it.
then if that's gotten to Propolis, we'll ignore the shutdown request and say "ok" to the API. if the timer elapses and you did not force-stop it (maybe you went to get water instead), you come back to find that the shutdown was forgotten and your instance might be back at a login prompt. that seems unfortunate!
if this is handled in Nexus in ways I haven't seen yet, maybe I'm overthinking it a bit.
There was a problem hiding this comment.
the opinion the RFD seems to have is that once you've made a request, no changing your mind on timeout, and right now i suppose i'm extending that to 'no changing your mind on fate either' - but i certainly don't like the operator experience in the aforementioned hypothetical, so perhaps we should consider letting its fate be changed.. if you've sent a soft-reboot and a timeout began, the OS was already given a signal, so changing your mind on what you want it to do (make it a soft-off) after either the guest sends the power event after completing its shutdown or the existing timeout runs out certainly doesn't strike me as an unreasonable thing to allow
Changes the instance state put API to support the propolis-server behaviors specified by RFD 622.
(migration-from-base phd-test failure: propolis-client is the new v7, so phd-runner can't talk to the v6 propolis-server artifact)