[pve-devel] [PATCH qemu-server] vzdump: fix log output for disks with iothread

[Aaron Lauterer]

On 3/4/20 3:31 PM, Thomas Lamprecht wrote:
> On 2/27/20 12:01 PM, Stefan Reiter wrote:
>> On 2/27/20 11:48 AM, Fabian Grünbichler wrote:
>>> On February 26, 2020 11:53 am, Aaron Lauterer wrote:
>>>> If IO-Thread is activated and a new enough Qemu version installed the
>>>> program still ran into the elsif clause and never in the else clause.
>>>> Thus the "include disk ..." log output was missing for these disks.
>>>>
>>>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>>>> ---
>>>>    PVE/VZDump/QemuServer.pm | 9 +++++----
>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>>>> index 7695ad6..e36a259 100644
>>>> --- a/PVE/VZDump/QemuServer.pm
>>>> +++ b/PVE/VZDump/QemuServer.pm
>>>> @@ -79,11 +79,12 @@ sub prepare {
>>>>        if (defined($drive->{backup}) && !$drive->{backup}) {
>>>>            $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
>>>>            return;
>>>> -    } elsif ($self->{vm_was_running} && $drive->{iothread}) {
>>>> -        if (!PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4, 0, 1)) {
>>>> -        die "disk '$ds' '$volid' (iothread=on) can't use backup feature with running QEMU " .
>>>> +    } elsif ($self->{vm_was_running}
>>>> +         && $drive->{iothread}
>>>> +         && !PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4, 0, 1))
>>> not new, I know, but:
>>>
>>> two of those three are not related to the drive. one of those two is
>>> actually a qmp call. instead of having a condition spanning three lines,
>>> we could refactor it like so:
>>>
>>> my $iothread_allowed = -1; // to avoid repeated QMP calls
>>>
>>> foreach_drive(..) {
>>>
>>> my $iothread = $self->{vm_was_running} && $drive->{iothread}; // do we even care?
>>> if ($iothread && $iothread_allowed == -1) {
>>>       $iothread_allowed = PVE::QemuServer::Machine::runs_at_least_qemu_version(...); // cache on first use
>>> }
>>>
>>> if ( ... ) {
>>>
>>> } elsif ($iothread && !$iothread_allowed) { // simple condition ;)
>>>     die ...
>>> }
>>>
>>> alternatively, the die could also move into the else branch, assuming we
>>> don't have efidisks with iothreads?
>>>
>>> also, isn't this actually wrong? in stop mode, we'd get the new qemu
>>> version when it counts, even if the VM is running the old version right
>>> now?
>>>
>>
>> Doesn't a "stop" mode backup imply the VM being shut down and restarted before backup? I.e. the backup always will run on the currently installed QEMU version?
>>
>> If that's the case, this is correct, since we only care about which QEMU version we send the QMP command to.
>>
> 
> what's the outcome/state of this?
> 

I think the state right now AFAIU is to decide if the current behavior 
is okay in regards to do the check in the current running qemu version, 
which could be the one with the iothread bug, while the actual backup 
(in stop mode?) could happen in a recently installed qemu version that 
would work with iothread.

Or if we want to handle that in some way. Ad hoc I wouldn't know how though.