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

[Fabian Ebner <f.ebner@proxmox.com>]

On 21.01.21 10:26, Dominik Csapak wrote:
> 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?
> 

Yes, it should be enough if there is something like a 'backup-init' lock 
in the config during the initial phase. The vm_resume function 
explicitly checks for the 'backup' lock to decide if it can proceed.

> 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 we had the above locking, I think it should work for snapshot mode 
already. Only suspend mode has the resume call at the very end in the 
cleanup phase of the backup. And maybe that call isn't actually needed ?
* for containers 'cleanup->{resume} is set to 0' again, so it doesn't matter
* for VMs, it's resumed within archive() via resume_vm_after_job_start() 
already before that cleanup phase
Or should suspend mode force the VM to be suspended during the whole backup?

Last, but not least (from the docs):

suspend mode
This mode is provided for compatibility reason, and suspends the VM 
before calling the snapshot mode. Since suspending the VM results in a 
longer downtime and does not necessarily improve the data consistency, 
the use of the snapshot mode is recommended instead.

Are there any plans to remove suspend mode for VMs for PVE 7.0?

>>
>>>       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};
>>>
> 
>