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

[Aaron Lauterer]

On 6/5/20 8:43 PM, Thomas Lamprecht wrote:
> On 5/6/20 11:57 AM, 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>
>> ---
>> v4->v5:
>> * use new foreach_volume call
>> * change $ret_volumes to $return_volumes
>> * use dedicated variables in PVE::VZDump::QemuServer where hash is used
>>    more than once
>> * renamed $backup_included and other variables to (IMHO) better names
>>
>> v3->v4: changed function call for `foreach_drive` to QemuServer::Drive
>>
>> This can be changed to `foreach_volume` once the patches by Fabian Ebner
>> are through.
> 
> is that now in?

yes, see v4->v5. Or do you mean something else?

> 
> Looks OK, one not to important style comment below.
> 
>>   PVE/QemuConfig.pm        | 31 ++++++++++++++++++++++++++++
>>   PVE/VZDump/QemuServer.pm | 44 +++++++++++++++++++++-------------------
>>   2 files changed, 54 insertions(+), 21 deletions(-)
>>
>> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>> index 240fc06..dac913f 100644
>> --- a/PVE/QemuConfig.pm
>> +++ b/PVE/QemuConfig.pm
>> @@ -165,6 +165,37 @@ sub get_replicatable_volumes {
>>       return $volhash;
>>   }
>>   
>> +sub get_backup_volumes {
>> +    my ($class, $conf) = @_;
>> +
>> +    my $return_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 @$return_volumes, $volume;
> 
> Not sure if I did already comment this on an earlier revision, but personally I'd prefer
> something like:
> 
> push @$return_volumes, {
>      key => $key,
>      included => $included,
>      ...
> };
> 
> (or at least save the whole info to my $volume directly on declaration and then push)

There was some comment in an earlier revision at this point but I think 
about something different.

I do see your point, will be changed in the next revision.
> 
>> +    };
>> +
>> +    PVE::QemuConfig->foreach_volume($conf, $test_volume);
>> +
>> +    return $return_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 f122539..480979c 100644
>> --- a/PVE/VZDump/QemuServer.pm
>> +++ b/PVE/VZDump/QemuServer.pm
>> @@ -69,37 +69,39 @@ sub prepare {
>>   
>>       my $vollist = [];
>>       my $drivehash = {};
>> -    PVE::QemuConfig->foreach_volume($conf, sub {
>> -	my ($ds, $drive) = @_;
>> -
>> -	return if PVE::QemuServer::drive_is_cdrom($drive);
>> -
>> -	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}) {
>> +    my $backup_volumes = PVE::QemuConfig->get_backup_volumes($conf);
>> +
>> +    foreach my $volume (@{$backup_volumes}) {
>> +	my $name = $volume->{key};
>> +	my $data = $volume->{data};
>> +	my $volid = $data->{file};
>> +	my $size = $data->{size};
>> +
>> +	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} && $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 $size) {
>> +		my $readable_size = PVE::JSONSchema::format_size($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);
>>   
>>
>