[pve-devel] applied: [RFC PATCH qemu-server] use 'system_wakeup' to resume suspended vms

[Thomas Lamprecht]

On 6/13/18 11:17 AM, Dominik Csapak wrote:
> when a vm is suspended (e.g. autosuspend on windows)
> we detect that it is not running, display the resume button,
> but 'cont' does not wakeup the system from suspend
> 
> with this we can wake up suspended vms
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/QemuServer.pm | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 00e1669..277bc58 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5170,6 +5170,13 @@ sub vm_resume {
>  
>      PVE::QemuConfig->lock_config($vmid, sub {
>  
> +	my $res = vm_mon_cmd($vmid, 'query-status');
> +	my $resume_cmd = 'cont';
> +
> +	if ($res->{status} && $res->{status} eq 'suspended') {
> +	    $resume_cmd = 'system_wakeup';
> +	}
> +
>  	if (!$nocheck) {
>  
>  	    my $conf = PVE::QemuConfig->load_config($vmid);
> @@ -5177,10 +5184,10 @@ sub vm_resume {
>  	    PVE::QemuConfig->check_lock($conf)
>  		if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
>  
> -	    vm_mon_cmd($vmid, "cont");
> +	    vm_mon_cmd($vmid, $resume_cmd);
>  
>  	} else {
> -	    vm_mon_cmd_nocheck($vmid, "cont");
> +	    vm_mon_cmd_nocheck($vmid, $resume_cmd);
>  	}
>      });
>  }
> 

applied, but I wonder if we could remove the $nocheck from this
method...

nocheck was introduced in 289e0b8564dce494481cd3a5d534b801835f85b6 as
a band-aid, but it only accomplishes skipping the check_running call
in the vm_qmp_command (which vm_mon_cmd uses) and avoiding the
load_conf for some race cases with migration where the config wasn't
moved yet at this point.

But as neither cont nor system_wakup will work on an non-running VM
we could use only vm_mon_cmd once at the end of the locked scope?

Further nocheck may be removed, the load_conf could be wrapped in
skiplock, which the migration case uses anyway.
The qm skiplock param would need to be kept as dummy param for now,
to keep incoming migration backward compatibility... 

What do you think? I.e., something like this followup:

---8<---
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index c15c71f..ab1f73d 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -2349,7 +2349,7 @@ __PACKAGE__->register_method({
            vmid => get_standard_option('pve-vmid',
                                        { completion => \&PVE::QemuServer::complete_vmid_running }),
            skiplock => get_standard_option('skiplock'),
-           nocheck => { type => 'boolean', optional => 1 },
+           nocheck => { type => 'boolean', optional => 1, description => 'deprecated' },

        },
     },
@@ -2371,16 +2371,14 @@ __PACKAGE__->register_method({
        raise_param_exc({ skiplock => "Only root may use this option." })
            if $skiplock && $authuser ne 'root at pam';

-       my $nocheck = extract_param($param, 'nocheck');
-
-       die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid, $nocheck);
+       die "VM $vmid not running\n";

        my $realcmd = sub {
            my $upid = shift;

            syslog('info', "resume VM $vmid: $upid\n");

-           PVE::QemuServer::vm_resume($vmid, $skiplock, $nocheck);
+           PVE::QemuServer::vm_resume($vmid, $skiplock);

            return;
        };
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index c017a59..83a50a2 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -297,7 +297,7 @@ __PACKAGE__->register_method ({
            } elsif ($line =~ /^resume (\d+)$/) {
                my $vmid = $1;
                if (PVE::QemuServer::check_running($vmid, 1)) {
-                   eval { PVE::QemuServer::vm_resume($vmid, 1, 1); };
+                   eval { PVE::QemuServer::vm_resume($vmid, 1) };
                    if ($@) {
                        $tunnel_write->("ERR: resume failed - $@");
                    } else {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 277bc58..a93625a 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5166,29 +5166,22 @@ sub vm_suspend {
 }

 sub vm_resume {
-    my ($vmid, $skiplock, $nocheck) = @_;
+    my ($vmid, $skiplock) = @_;

     PVE::QemuConfig->lock_config($vmid, sub {

        my $res = vm_mon_cmd($vmid, 'query-status');
        my $resume_cmd = 'cont';
-
        if ($res->{status} && $res->{status} eq 'suspended') {
            $resume_cmd = 'system_wakeup';
        }

-       if (!$nocheck) {
-
+       if (!$skiplock) {
            my $conf = PVE::QemuConfig->load_config($vmid);
-
-           PVE::QemuConfig->check_lock($conf)
-               if !($skiplock || PVE::QemuConfig->has_lock($conf, 'backup'));
-
-           vm_mon_cmd($vmid, $resume_cmd);
-
-       } else {
-           vm_mon_cmd_nocheck($vmid, $resume_cmd);
+           PVE::QemuConfig->check_lock($conf) if !PVE::QemuConfig->has_lock($conf, 'backup');
        }
+
+       vm_mon_cmd($vmid, $resume_cmd);
     });
 }

@@ -6299,7 +6292,7 @@ sub qemu_drive_mirror_monitor {
                        eval { PVE::QemuServer::vm_mon_cmd($vmid, "guest-fsfreeze-thaw"); };
                    } else {
                        print "resume vm\n";
-                       eval {  PVE::QemuServer::vm_resume($vmid, 1, 1); };
+                       eval { vm_resume($vmid, 1) };
                    }

                    last;