[pve-devel] [PATCH v2 qemu-server 0/2] Fix ACPI-suspended VMs resuming after migration

[pve-devel] [PATCH v2 qemu-server 0/2] Fix ACPI-suspended VMs resuming after migration

[Filip Schauer]

Shutdown and reboot commands do infact time out instead of failing
immediately on suspended VMs. This is fixed by extending vm_is_paused
with the "suspended" runstate. It also make sense to check for the
"prelaunch" runstate, as the VM is not running in that state. To
accommodate the changes, the function is renamed to vm_is_frozen.

Filip Schauer (2):
  Add missing checks for paused runstates
  Rename vm_is_paused to vm_is_frozen

 PVE/API2/Qemu.pm         |  8 ++++----
 PVE/QemuMigrate.pm       |  8 ++++----
 PVE/QemuServer.pm        |  8 ++++++--
 PVE/VZDump/QemuServer.pm | 14 +++++++-------
 4 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH v2 qemu-server 1/2] Add missing checks for paused runstates

[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>
---
 PVE/QemuServer.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index bf1de17..954fed7 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8596,7 +8596,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" or
+	$qmpstatus->{status} eq "suspended" or
+	$qmpstatus->{status} eq "prelaunch"
+    );
 }
 
 sub check_volume_storage_type {
-- 
2.39.2

[pve-devel] [PATCH v2 qemu-server 2/2] Rename vm_is_paused to vm_is_frozen

[Filip Schauer]

Rename vm_is_paused to vm_is_frozen to avoid confusion with the "paused"
runstate.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 PVE/API2/Qemu.pm         |  8 ++++----
 PVE/QemuMigrate.pm       |  8 ++++----
 PVE/QemuServer.pm        |  2 +-
 PVE/VZDump/QemuServer.pm | 14 +++++++-------
 4 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9606e72..35060e1 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3086,12 +3086,12 @@ __PACKAGE__->register_method({
 	#
 	# checking the qmp status here to get feedback to the gui/cli/api
 	# and the status query should not take too long
-	if (PVE::QemuServer::vm_is_paused($vmid)) {
+	if (PVE::QemuServer::vm_is_frozen($vmid)) {
 	    if ($param->{forceStop}) {
-		warn "VM is paused - stop instead of shutdown\n";
+		warn "VM is frozen - stop instead of shutdown\n";
 		$shutdown = 0;
 	    } else {
-		die "VM is paused - cannot shutdown\n";
+		die "VM is frozen - cannot shutdown\n";
 	    }
 	}
 
@@ -3162,7 +3162,7 @@ __PACKAGE__->register_method({
 	my $node = extract_param($param, 'node');
 	my $vmid = extract_param($param, 'vmid');
 
-	die "VM is paused - cannot shutdown\n" if PVE::QemuServer::vm_is_paused($vmid);
+	die "VM is frozen - cannot shutdown\n" if PVE::QemuServer::vm_is_frozen($vmid);
 
 	die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
 
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 506911e..5bf273a 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -223,7 +223,7 @@ sub prepare {
 	    }
 	}
 
-	$self->{vm_was_paused} = 1 if PVE::QemuServer::vm_is_paused($vmid);
+	$self->{vm_was_frozen} = 1 if PVE::QemuServer::vm_is_frozen($vmid);
     }
 
     my ($loc_res, $mapped_res, $missing_mappings_by_node) = PVE::QemuServer::check_local_resources($conf, 1);
@@ -1375,7 +1375,7 @@ sub phase2_cleanup {
     # Can end up in POSTMIGRATE state if failure occurred after convergence. Try going back to
     # original state. Unfortunately, direct transition from POSTMIGRATE to PAUSED is not possible.
     if ($vm_status && $vm_status eq 'postmigrate') {
-	if (!$self->{vm_was_paused}) {
+	if (!$self->{vm_was_frozen}) {
 	    eval { mon_cmd($vmid, 'cont'); };
 	    $self->log('err', "resuming VM failed: $@") if $@;
 	} else {
@@ -1488,7 +1488,7 @@ sub phase3_cleanup {
 	# deletes local FDB entries if learning is disabled, they'll be re-added on target on resume
 	PVE::QemuServer::del_nets_bridge_fdb($conf, $vmid);
 
-	if (!$self->{vm_was_paused}) {
+	if (!$self->{vm_was_frozen}) {
 	    # config moved and nbd server stopped - now we can resume vm on target
 	    if ($tunnel && $tunnel->{version} && $tunnel->{version} >= 1) {
 		my $cmd = $tunnel->{version} == 1 ? "resume $vmid" : "resume";
@@ -1519,7 +1519,7 @@ sub phase3_cleanup {
 	    && PVE::QemuServer::parse_guest_agent($conf)->{fstrim_cloned_disks}
 	    && $self->{running}
 	) {
-	    if (!$self->{vm_was_paused}) {
+	    if (!$self->{vm_was_frozen}) {
 		$self->log('info', "issuing guest fstrim");
 		if ($self->{opts}->{remote}) {
 		    PVE::Tunnel::write_tunnel($self->{tunnel}, 600, 'fstrim');
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 954fed7..60b3541 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8589,7 +8589,7 @@ sub complete_migration_storage {
     return $res;
 }
 
-sub vm_is_paused {
+sub vm_is_frozen {
     my ($vmid) = @_;
     my $qmpstatus = eval {
 	PVE::QemuConfig::assert_config_exists_on_node($vmid);
diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
index b38d7e7..8ec322d 100644
--- a/PVE/VZDump/QemuServer.pm
+++ b/PVE/VZDump/QemuServer.pm
@@ -64,11 +64,11 @@ sub prepare {
 	if defined($conf->{name});
 
     $self->{vm_was_running} = 1;
-    $self->{vm_was_paused} = 0;
+    $self->{vm_was_frozen} = 0;
     if (!PVE::QemuServer::check_running($vmid)) {
 	$self->{vm_was_running} = 0;
-    } elsif (PVE::QemuServer::vm_is_paused($vmid)) {
-	$self->{vm_was_paused} = 1;
+    } elsif (PVE::QemuServer::vm_is_frozen($vmid)) {
+	$self->{vm_was_frozen} = 1;
     }
 
     $task->{hostname} = $conf->{name};
@@ -198,7 +198,7 @@ sub start_vm {
 sub suspend_vm {
     my ($self, $task, $vmid) = @_;
 
-    return if $self->{vm_was_paused};
+    return if $self->{vm_was_frozen};
 
     $self->cmd ("qm suspend $vmid --skiplock");
 }
@@ -206,7 +206,7 @@ sub suspend_vm {
 sub resume_vm {
     my ($self, $task, $vmid) = @_;
 
-    return if $self->{vm_was_paused};
+    return if $self->{vm_was_frozen};
 
     $self->cmd ("qm resume $vmid --skiplock");
 }
@@ -888,7 +888,7 @@ sub _get_task_devlist {
 
 sub qga_fs_freeze {
     my ($self, $task, $vmid) = @_;
-    return if !$self->{vmlist}->{$vmid}->{agent} || $task->{mode} eq 'stop' || !$self->{vm_was_running} || $self->{vm_was_paused};
+    return if !$self->{vmlist}->{$vmid}->{agent} || $task->{mode} eq 'stop' || !$self->{vm_was_running} || $self->{vm_was_frozen};
 
     if (!PVE::QemuServer::qga_check_running($vmid, 1)) {
 	$self->loginfo("skipping guest-agent 'fs-freeze', agent configured but not running?");
@@ -944,7 +944,7 @@ sub enforce_vm_running_for_backup {
 sub resume_vm_after_job_start {
     my ($self, $task, $vmid) = @_;
 
-    return if !$self->{vm_was_running} || $self->{vm_was_paused};
+    return if !$self->{vm_was_running} || $self->{vm_was_frozen};
 
     if (my $stoptime = $task->{vmstoptime}) {
 	my $delay = time() - $task->{vmstoptime};
-- 
2.39.2

Re: [pve-devel] [PATCH v2 qemu-server 1/2] Add missing checks for paused runstates

[Fiona Ebner]

Am 25.08.23 um 14:18 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>
> ---
>  PVE/QemuServer.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index bf1de17..954fed7 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -8596,7 +8596,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" or
> +	$qmpstatus->{status} eq "suspended" or

Style nit: mixing '&&' and 'or' is not too nice. For boolean expressions
like here, '&&' and '||' should always be used. For assertions like
$variable or die "error"
usually 'or' is used in our code base.

Once that's fixed:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> +	$qmpstatus->{status} eq "prelaunch"
> +    );
>  }
>  
>  sub check_volume_storage_type {

Re: [pve-devel] [PATCH v2 qemu-server 2/2] Rename vm_is_paused to vm_is_frozen

[Fiona Ebner]

Am 25.08.23 um 14:18 schrieb Filip Schauer:
> Rename vm_is_paused to vm_is_frozen to avoid confusion with the "paused"
> runstate.
> 

I know I suggested renaming the function, but with 'frozen' there is
potential for new confusion with fsfreeze, especially with the
user-facing warnings/errors. It's also that 'paused' sounds nicer to
users here IMHO.

Maybe we can call it vcpus_(are_)paused and keep the user-facing
messages the same? Or we can just keep the current name. Right now,
there is no case where we need to distinguish between having runstate
'paused' and the return value of vm_is_paused().

Re: [pve-devel] [PATCH v2 qemu-server 0/2] Fix ACPI-suspended VMs resuming after migration

[Filip Schauer]

v3 patch available:
https://lists.proxmox.com/pipermail/pve-devel/2023-October/059387.html

On 25/08/2023 14:18, Filip Schauer wrote:
> Shutdown and reboot commands do infact time out instead of failing
> immediately on suspended VMs. This is fixed by extending vm_is_paused
> with the "suspended" runstate. It also make sense to check for the
> "prelaunch" runstate, as the VM is not running in that state. To
> accommodate the changes, the function is renamed to vm_is_frozen.
>
> Filip Schauer (2):
>    Add missing checks for paused runstates
>    Rename vm_is_paused to vm_is_frozen
>
>   PVE/API2/Qemu.pm         |  8 ++++----
>   PVE/QemuMigrate.pm       |  8 ++++----
>   PVE/QemuServer.pm        |  8 ++++++--
>   PVE/VZDump/QemuServer.pm | 14 +++++++-------
>   4 files changed, 21 insertions(+), 17 deletions(-)
>