[pve-devel] [PATCH v3 qemu-server] vzdump: move include logic for volumes to method

[Aaron Lauterer]

Thx for the info :)

On 3/24/20 10:16 AM, Fabian Ebner wrote:
> On 17.03.20 15:34, Fabian Grünbichler wrote:
>> On March 16, 2020 4:44 pm, Aaron Lauterer wrote:
>>> Move the logic which volumes are included in the backup job to its own
>>> method and adapt the VZDump code accordingly. This makes it possible to
>>> develop other features around backup jobs.
>>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>>> ---
>>>
>>> v2 -> v3: rebased
>>>
>>> v1 -> v2:
>>> * implemented the suggestions from Fabian [0]
>>> * incorporated changes that happened in the meantime, most notably the
>>>    check for efidisk without omvf bios set
>>>
>>> I decided to keep the check for IOThreaded VMs where it is because it
>>> will only trigger if the backup is run with an older qemu version. By
>>> the time this patch series gets applied and shipped I think it unlikely
>>> that this will actually be triggered anymore.
>>>
>>> There also seems to be a problem with the way the IFs are nested in that
>>> section. I sent in separate small patch to fix it [1]. I wasn't sure if
>>> I should have implemented that patch here as well. Depending on which
>>> patch gets applied first, the other will need some rebasing.
>>>
>>> [0] https://pve.proxmox.com/pipermail/pve-devel/2020-January/041360.html
>>> [1] 
>>> https://pve.proxmox.com/pipermail/pve-devel/2020-February/041968.html
>>>
>>>   PVE/QemuConfig.pm        | 31 +++++++++++++++++++++++++++++++
>>>   PVE/VZDump/QemuServer.pm | 38 +++++++++++++++++++-------------------
>>>   2 files changed, 50 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>>> index 1ba728a..f5b737c 100644
>>> --- a/PVE/QemuConfig.pm
>>> +++ b/PVE/QemuConfig.pm
>>> @@ -130,6 +130,37 @@ sub get_replicatable_volumes {
>>>       return $volhash;
>>>   }
>>> +sub get_backup_volumes {
>>> +    my ($class, $conf) = @_;
>>> +
>>> +    my $ret_volumes = [];
>>> +
>>> +    my $test_volume = sub {
>>> +    my ($key, $drive) = @_;
>>> +
>>> +    return if PVE::QemuServer::drive_is_cdrom($drive);
>>> +
>>> +    my $volume = {};
>>> +    my $included = $drive->{backup} // 1;;
>>> +    my $reason = defined($drive->{backup}) ? 'disabled' : 'enabled';
>>> +
>>> +    if ($key =~ m/^efidisk/ && (!defined($conf->{bios}) || 
>>> $conf->{bios} ne 'ovmf')) {
>>> +        $included = 0;
>>> +        $reason = "efidisk with non omvf bios";
>>> +    }
>>> +    $volume->{key} = $key;
>>> +    $volume->{included} = $included;
>>> +    $volume->{reason} = $reason;
>>> +    $volume->{data} = $drive;
>>> +
>>> +    push @$ret_volumes, $volume;
>>> +    };
>>> +
>>> +    PVE::QemuServer::foreach_drive($conf, $test_volume);
>>
>> nit: this is in PVE::QemuServer::Drive now, and we don't want to add new
>> dependencies from QemuConfig to QemuServer.
>>
> 
> Once it's available, the new abstract foreach_volume (will be part of 
> QemuConfig.pm via AbstractConfig.pm) can also be used here. I'll send 
> the next version of the series[0] later this week and hopefully it's in 
> good enough shape to be applied.
> 
> [0]: https://pve.proxmox.com/pipermail/pve-devel/2020-March/042324.html
> 
> 
>>> +
>>> +    return $ret_volumes;
>>> +}
>>> +
>>>   sub __snapshot_save_vmstate {
>>>       my ($class, $vmid, $conf, $snapname, $storecfg, $statestorage, 
>>> $suspend) = @_;
>>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>>> index 7695ad6..38471de 100644
>>> --- a/PVE/VZDump/QemuServer.pm
>>> +++ b/PVE/VZDump/QemuServer.pm
>>> @@ -69,37 +69,37 @@ sub prepare {
>>>       my $vollist = [];
>>>       my $drivehash = {};
>>> -    PVE::QemuServer::foreach_drive($conf, sub {
>>> -    my ($ds, $drive) = @_;
>>> +    my $backup_included = PVE::QemuConfig->get_backup_volumes($conf);
>>> -    return if PVE::QemuServer::drive_is_cdrom($drive);
>>> +    foreach my $volume(@{$backup_included}) {
>>> +    my $volid = $volume->{data}->{file};
>>> +    my $name = $volume->{key};
>>> -    my $volid = $drive->{file};
>>> -
>>> -    if (defined($drive->{backup}) && !$drive->{backup}) {
>>> -        $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
>>> -        return;
>>> -    } elsif ($self->{vm_was_running} && $drive->{iothread}) {
>>> +    if (!$volume->{included}) {
>>> +        if ($volume->{reason} eq 'efidisk with non omvf bios') {
>>> +        $self->loginfo("excluding '$name' (efidisks can only be 
>>> backed up when BIOS is set to 'ovmf')");
>>> +        next;
>>> +        }
>>> +        $self->loginfo("exclude disk '$name' '$volid' (backup=no)");
>>> +        next;
>>> +    } elsif ($self->{vm_was_running} && $volume->{data}->{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 " .
>>> +        die "disk '$name' '$volid' (iothread=on) can't use backup 
>>> feature with running QEMU " .
>>>               "version < 4.0.1! Either set backup=no for this drive 
>>> or upgrade QEMU and restart VM\n";
>>>           }
>>> -    } elsif ($ds =~ m/^efidisk/ && (!defined($conf->{bios}) || 
>>> $conf->{bios} ne 'ovmf')) {
>>> -        $self->loginfo("excluding '$ds' (efidisks can only be backed 
>>> up when BIOS is set to 'ovmf')");
>>> -        return;
>>>       } else {
>>> -        my $log = "include disk '$ds' '$volid'";
>>> -       if (defined $drive->{size}) {
>>> -        my $readable_size = 
>>> PVE::JSONSchema::format_size($drive->{size});
>>> +        my $log = "include disk '$name' '$volid'";
>>> +        if (defined $volume->{data}->{size}) {
>>> +        my $readable_size = 
>>> PVE::JSONSchema::format_size($volume->{data}->{size});
>>>           $log .= " $readable_size";
>>> -       }
>>> +        }
>>>           $self->loginfo($log);
>>>       }
>>>       my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 
>>> 1);
>>>       push @$vollist, $volid if $storeid;
>>> -    $drivehash->{$ds} = $drive;
>>> -    });
>>> +    $drivehash->{$name} = $volume->{data};
>>> +    }
>>>       PVE::Storage::activate_volumes($self->{storecfg}, $vollist);
>>> -- 
>>> 2.20.1
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel at pve.proxmox.com
>>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>