[pve-devel] [PATCH v3 qemu-server] Fix ACPI-suspended VMs resuming after migration

[pve-devel] [PATCH v3 qemu-server] Fix ACPI-suspended VMs resuming after migration

[Filip Schauer]

Add checks for "suspended" and "prelaunch" runstates when checking
whether a VM is paused.

This fixes the following issues:
* ACPI-suspended VMs automatically resuming after migration
* Shutdown and reboot commands timing out instead of failing
  immediately on suspended VMs

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
Changes since v2:
* Make && and || operators consistent
* Revert rename vm_is_paused to vm_is_frozen

We could have renamed vm_is_paused to vcpus_are_paused, but I do not
consider that necessary.

 PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 1b1ccf4..2895675 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8503,7 +8503,11 @@ sub vm_is_paused {
 	mon_cmd($vmid, "query-status");
     };
     warn "$@\n" if $@;
-    return $qmpstatus && $qmpstatus->{status} eq "paused";
+    return $qmpstatus && (
+	$qmpstatus->{status} eq "paused" ||
+	$qmpstatus->{status} eq "suspended" ||
+	$qmpstatus->{status} eq "prelaunch"
+    );
 }
 
 sub check_volume_storage_type {
-- 
2.39.2

[pve-devel] applied: [PATCH v3 qemu-server] Fix ACPI-suspended VMs resuming after migration

[Fiona Ebner]

Am 09.10.23 um 15:25 schrieb Filip Schauer:
> Add checks for "suspended" and "prelaunch" runstates when checking
> whether a VM is paused.
> 
> This fixes the following issues:
> * ACPI-suspended VMs automatically resuming after migration
> * Shutdown and reboot commands timing out instead of failing
>   immediately on suspended VMs
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---

applied, thanks! It was not enough to fix the issue with backup however,
because qmeventd didn't play nice, so I sent a follow-up:
https://lists.proxmox.com/pipermail/pve-devel/2023-October/059394.html

Re: [pve-devel] [PATCH v3 qemu-server] Fix ACPI-suspended VMs resuming after migration

[Thomas Lamprecht]

Am 09/10/2023 um 15:25 schrieb Filip Schauer:
> Add checks for "suspended" and "prelaunch" runstates when checking
> whether a VM is paused.
> 
> This fixes the following issues:
> * ACPI-suspended VMs automatically resuming after migration
> * Shutdown and reboot commands timing out instead of failing
>   immediately on suspended VMs
> 

I checked the call-sites and what I'm wondering is, can the VM from those
new states get waked up without QMP intervention, say a ACPI-suspension
be triggered by some (virtual) RTC or via network (like wake-on-lan),
as then, we should add a big notice comment on this method to ensure
new users of it are informed about that possibility.

Also, with that change we might have added a race for suspend-mode backups,
at least if VMs really can wake up without a QMP command (which I find likely).
I.e., between the time we checked and set vm_was_paused until we actually
suspend, because if the VM would wake up in between we might get inconsistent
stuff and skip things like fs-freeze.

While we recommend stop and snapshot modes over suspend mode, the latter is
still exposed via UI and API and should work OK enough.

Note that ACPI S3 suspend and our vm_suspend are very different things, our
vm_suspend is doing a "stop" monitor command, resulting in all vCPUs being
stopped, while S3 is a (virtual) firmware/machine feature – I mean, I guess
there's a reason that those things are reported as different states..
It doesn't help that our vm_suspend method doesn't actually do a (ACPI S3
like) suspension, but a (vCPU) "stop".

The "prelaunch" state, OTOH., seems pretty much like "paused", with the
difference that the VM vCPUs never ran in the former, this seems fine to
handle the same. But for "suspended" I'm not sure if we'd like to detect
that as paused only if conflating both is OK for a call-site, so maybe
with an opt-in parameter. Alternatively we could return the status in
the "true" case so that call-sites can decide what to do for any such
special handling without an extra parameter.

Re: [pve-devel] [PATCH v3 qemu-server] Fix ACPI-suspended VMs resuming after migration

[Fiona Ebner]

Am 10.10.23 um 13:45 schrieb Thomas Lamprecht:
> Am 09/10/2023 um 15:25 schrieb Filip Schauer:
>> Add checks for "suspended" and "prelaunch" runstates when checking
>> whether a VM is paused.
>>
>> This fixes the following issues:
>> * ACPI-suspended VMs automatically resuming after migration
>> * Shutdown and reboot commands timing out instead of failing
>>   immediately on suspended VMs
>>
> 
> I checked the call-sites and what I'm wondering is, can the VM from those
> new states get waked up without QMP intervention, say a ACPI-suspension
> be triggered by some (virtual) RTC or via network (like wake-on-lan),
> as then, we should add a big notice comment on this method to ensure
> new users of it are informed about that possibility.

Sorry! I did not consider this. Yes, it can get woken up without QMP.
Even just with keyboard presses in case of my Debian test VM. So our
config locks are useless here :/

It'll also be an issue for migration:
1. migration is started and remembers that "vm_was_paused"
2. migration continues
3. during migration VM wakes up
4. migration finishes, but doesn't resume guest on the target, because
"vm_was_paused"

We could either:

1. tolerate the old behavior (VM might get resumed on target if it was
suspended) - if we declare migration an ACPI-wake-up event ;)
2. tolerate the new behavior (VM might wake up and not be resumed on
target later) - seems worse then old behavior when it happens
3. disallow migration when in 'suspended' runstate
4. add new check just before moving guest to target - but not sure how
we'd do that without races again...

> Also, with that change we might have added a race for suspend-mode backups,
> at least if VMs really can wake up without a QMP command (which I find likely).
> I.e., between the time we checked and set vm_was_paused until we actually
> suspend, because if the VM would wake up in between we might get inconsistent
> stuff and skip things like fs-freeze.

That race is already there without the patch. QEMU does not transition
from 'suspended' state to 'paused' state when QMP 'stop' is issued, i.e.
what our 'qm suspend' or vm_suspend() actually does. So it doesn't
matter if we call that during backup or not when the VM is already in
'suspended' state.

The window for the race is a bit larger though:
now: VM wakes up between check if paused and 'backup' QMP
before: VM wakes up after fsfreeze was skipped because guest agent was
detected as not running

We could either:

1. (ironically) disallow 'suspend' mode backup when in 'suspended' runstate.
2. resume and pause the VM upon 'suspend' mode backup, but is also
surprising
3. make the race window smaller again by doing the qga_check_running()
and fs-freeze if true, if VM was in 'suspended' runstate.

I don't see how to avoid the possibility of a wake-up during an
inconvenient time except with one of the first two options.

> While we recommend stop and snapshot modes over suspend mode, the latter is
> still exposed via UI and API and should work OK enough.
> 
> Note that ACPI S3 suspend and our vm_suspend are very different things, our
> vm_suspend is doing a "stop" monitor command, resulting in all vCPUs being
> stopped, while S3 is a (virtual) firmware/machine feature – I mean, I guess
> there's a reason that those things are reported as different states..
> It doesn't help that our vm_suspend method doesn't actually do a (ACPI S3
> like) suspension, but a (vCPU) "stop".
> 
> The "prelaunch" state, OTOH., seems pretty much like "paused", with the
> difference that the VM vCPUs never ran in the former, this seems fine to
> handle the same. But for "suspended" I'm not sure if we'd like to detect
> that as paused only if conflating both is OK for a call-site, so maybe
> with an opt-in parameter. Alternatively we could return the status in
> the "true" case so that call-sites can decide what to do for any such
> special handling without an extra parameter.

Yes, I like the opt-in approach.

Re: [pve-devel] [PATCH v3 qemu-server] Fix ACPI-suspended VMs resuming after migration

[Fiona Ebner]

Am 10.10.23 um 16:19 schrieb Fiona Ebner:
>> Also, with that change we might have added a race for suspend-mode backups,
>> at least if VMs really can wake up without a QMP command (which I find likely).
>> I.e., between the time we checked and set vm_was_paused until we actually
>> suspend, because if the VM would wake up in between we might get inconsistent
>> stuff and skip things like fs-freeze.
> 
> That race is already there without the patch. QEMU does not transition
> from 'suspended' state to 'paused' state when QMP 'stop' is issued, i.e.
> what our 'qm suspend' or vm_suspend() actually does. So it doesn't
> matter if we call that during backup or not when the VM is already in
> 'suspended' state.
> 
> The window for the race is a bit larger though:
> now: VM wakes up between check if paused and 'backup' QMP
> before: VM wakes up after fsfreeze was skipped because guest agent was
> detected as not running
> 

What's written above actually applies to 'snapshot' mode backup, but not
to 'suspend' mode backup. 'suspend' mode backup is and was broken in
this regard already:

> root@pve8a1 ~ # vzdump 100 --storage pbs --mode suspend 
> INFO: starting new backup job: vzdump 100 --mode suspend --storage pbs
> INFO: Starting Backup of VM 100 (qemu)
> INFO: Backup started at 2023-10-11 09:25:53
> INFO: status = running
> INFO: backup mode: suspend
> INFO: ionice priority: 7
> INFO: VM Name: Copy-of-VM-apache
> INFO: include disk 'scsi0' 'rbd:vm-100-disk-0' 4618348134
> INFO: suspending guest
> INFO: snapshots found (not included into backup)
> INFO: creating Proxmox Backup Server archive 'vm/100/2023-10-11T07:25:53Z'
> INFO: skipping guest-agent 'fs-freeze', agent configured but not running?
> INFO: started backup task '923c95cf-6bb6-4476-825e-390c147a4e76'
> INFO: resuming VM again after 3 seconds
> INFO: scsi0: dirty-bitmap status: OK (8.0 MiB of 4.3 GiB dirty)
> INFO: using fast incremental mode (dirty-bitmap), 8.0 MiB dirty of 4.3 GiB total
> INFO: 100% (8.0 MiB of 8.0 MiB) in 1s, read: 8.0 MiB/s, write: 8.0 MiB/s
> INFO: backup was done incrementally, reused 4.29 GiB (99%)
> INFO: transferred 8.00 MiB in 1 seconds (8.0 MiB/s)
> INFO: adding notes to backup
> INFO: resume vm
> INFO: Finished Backup of VM 100 (00:00:06)
> INFO: Backup finished at 2023-10-11 09:25:59
> INFO: Backup job finished successfully

We never got the fsfreeze in suspend mode, because we do it after the
QMP 'stop' when the vCPUs are paused, so the guest agent won't be
running. The backup just ends up with whatever state the filesystem was
in when QMP 'stop' is issued.

Time to finally deprecate 'suspend' mode backup for VMs for good? It
really has just disadvantages compared to 'snapshot' mode and our docs
state "This mode is provided for compatibility reason" for a long time now.

> We could either:
> 
> 1. (ironically) disallow 'suspend' mode backup when in 'suspended' runstate.
> 2. resume and pause the VM upon 'suspend' mode backup, but is also
> surprising
> 3. make the race window smaller again by doing the qga_check_running()
> and fs-freeze if true, if VM was in 'suspended' runstate.
> 
> I don't see how to avoid the possibility of a wake-up during an
> inconvenient time except with one of the first two options.
>