Re: [pve-devel] [PATCH qemu-server v3] fix #2788: do not resume vms after backup if they were paused before

[Dominik Csapak <d.csapak@proxmox.com>]

On 1/20/21 3:15 PM, Fabian Ebner wrote:
> One minor thing inline, and that likely needs a fix elsewhere if we even 
> want to bother. Other than that:
> 
> Tested-by: Fabian Ebner <f.ebner@proxmox.com>
> 
> On 20.01.21 13:32, Dominik Csapak wrote:
>> by checking if the vm is paused at the beginning and skipping the resume
>> now we also skip the qga freeze/thaw (which cannot work if the vm is
>> paused)
>>
>> moved the 'vm_is_paused' sub from the api to PVE/QemuServer.pm so it is
>> available everywhere we need it.
>>
>> since a suspend backup would pause the vm anyway, we can skip that step
>> also
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes to v2:
>> * actually include code changes (git add is not optional), sry for the 
>> noise
>>
>>   PVE/API2/Qemu.pm         | 14 ++------------
>>   PVE/QemuServer.pm        | 10 ++++++++++
>>   PVE/VZDump/QemuServer.pm | 11 +++++++++--
>>   3 files changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index e2d2d67..623c998 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -2402,16 +2402,6 @@ __PACKAGE__->register_method({
>>       return $rpcenv->fork_worker('qmreset', $vmid, $authuser, $realcmd);
>>       }});
>> -my sub vm_is_paused {
>> -    my ($vmid) = @_;
>> -    my $qmpstatus = eval {
>> -    PVE::QemuConfig::assert_config_exists_on_node($vmid);
>> -    mon_cmd($vmid, "query-status");
>> -    };
>> -    warn "$@\n" if $@;
>> -    return $qmpstatus && $qmpstatus->{status} eq "paused";
>> -}
>> -
>>   __PACKAGE__->register_method({
>>       name => 'vm_shutdown',
>>       path => '{vmid}/status/shutdown',
>> @@ -2480,7 +2470,7 @@ __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 (vm_is_paused($vmid)) {
>> +    if (PVE::QemuServer::vm_is_paused($vmid)) {
>>           if ($param->{forceStop}) {
>>           warn "VM is paused - stop instead of shutdown\n";
>>           $shutdown = 0;
>> @@ -2556,7 +2546,7 @@ __PACKAGE__->register_method({
>>       my $node = extract_param($param, 'node');
>>       my $vmid = extract_param($param, 'vmid');
>> -    die "VM is paused - cannot shutdown\n" if vm_is_paused($vmid);
>> +    die "VM is paused - cannot shutdown\n" if 
>> PVE::QemuServer::vm_is_paused($vmid);
>>       die "VM $vmid not running\n" if 
>> !PVE::QemuServer::check_running($vmid);
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 54278e5..d7ee05f 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -7382,4 +7382,14 @@ sub complete_migration_storage {
>>       return $res;
>>   }
>> +sub vm_is_paused {
>> +    my ($vmid) = @_;
>> +    my $qmpstatus = eval {
>> +    PVE::QemuConfig::assert_config_exists_on_node($vmid);
>> +    mon_cmd($vmid, "query-status");
>> +    };
>> +    warn "$@\n" if $@;
>> +    return $qmpstatus && $qmpstatus->{status} eq "paused";
>> +}
>> +
>>   1;
>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>> index b322701..b5e74d3 100644
>> --- a/PVE/VZDump/QemuServer.pm
>> +++ b/PVE/VZDump/QemuServer.pm
>> @@ -62,8 +62,11 @@ sub prepare {
>>       if defined($conf->{name});
>>       $self->{vm_was_running} = 1;
>> +    $self->{vm_was_paused} = 0;
>>       if (!PVE::QemuServer::check_running($vmid)) {
>>       $self->{vm_was_running} = 0;
>> +    } elsif (PVE::QemuServer::vm_is_paused($vmid)) {
>> +    $self->{vm_was_paused} = 1;
>>       }
>>       $task->{hostname} = $conf->{name};
>> @@ -176,12 +179,16 @@ sub start_vm {
>>   sub suspend_vm {
>>       my ($self, $task, $vmid) = @_;
>> +    return if $self->{vm_was_paused};
>> +
>>       $self->cmd ("qm suspend $vmid --skiplock");
>>   }
>>   sub resume_vm {
>>       my ($self, $task, $vmid) = @_;
>> +    return if $self->{vm_was_paused};
>> +
>>       $self->cmd ("qm resume $vmid --skiplock");
>>   }
>> @@ -794,7 +801,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};
>> +    return if !$self->{vmlist}->{$vmid}->{agent} || $task->{mode} eq 
>> 'stop' || !$self->{vm_was_running} || $self->{vm_was_paused};
> 
> It's possible to suspend and resume VMs during backup. E.g. somebody 
> else can resume the VM (even without using --skiplock) right after the 
> backup task called suspend_vm. Relying on vm_was_paused here leads to 
> skipping the fs-freeze for such an (admittedly unlikely) edge case.

i mean i get why it is possible to resume / suspend during backup,
but really this should not be possible until the backup started

so maybe we have to have some kind of intermediate locking?

alternatively we can replace that check here
with an actual qmp status check (that is also racy though...)

also if i suspend between the paused check and the resume part
it will resume it again..

> 
>>       if (!PVE::QemuServer::qga_check_running($vmid, 1)) {
>>       $self->loginfo("skipping guest-agent 'fs-freeze', agent 
>> configured but not running?");
>> @@ -872,7 +879,7 @@ sub register_qmeventd_handle {
>>   sub resume_vm_after_job_start {
>>       my ($self, $task, $vmid) = @_;
>> -    return if !$self->{vm_was_running};
>> +    return if !$self->{vm_was_running} || $self->{vm_was_paused};
>>       if (my $stoptime = $task->{vmstoptime}) {
>>       my $delay = time() - $task->{vmstoptime};
>>