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

[Fabian Ebner]

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
>