[pve-devel] [PATCH qemu-server] backup: refactor disk included in backup to use from other modules

[Fabian Grünbichler]

On January 16, 2020 2:00 pm, Aaron Lauterer wrote:
> This refactoring allows us to use the code that decides which disks will
> be included in a backup from multiple places and provide a reason for
> the decision.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
>  PVE/QemuConfig.pm        | 30 ++++++++++++++++++++++++++++++
>  PVE/VZDump/QemuServer.pm | 28 ++++++++++++++--------------
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 1ba728a..bcf4180 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -130,6 +130,36 @@ sub get_replicatable_volumes {
>      return $volhash;
>  }
>  
> +sub get_volumes_backup_status {
> +    my ($class, $conf) = @_;
> +
> +    my $volhash = {};
> +
> +    my $test = sub {
> +	my ($volid, $attr) = @_;

same as for pve-container applies here

> +
> +	return if !$volid;
> +	return if PVE::QemuServer::drive_is_cdrom($attr);
> +
> +	my $status = 1;
> +	my $reason = 'default include';

same as for pve-container, but switched around - we don't know whether 
this is explicitly enabled, or enabled-by-default.

my $included = $attr->{backup} // 1;
my $reason = defined($attr->{backup}) ? 'config' : 'default';

> +
> +	if (exists($attr->{backup}) && !$attr->{backup}) {
> +	    $status = 0;
> +	    $reason = 'manual';
> +	}
> +
> +	$volhash->{$volid}->{included} = $status;
> +	$volhash->{$volid}->{reason} = $reason;
> +	$volhash->{$volid}->{volume} = $attr->{file};
> +	$volhash->{$volid}->{data} = $attr;

last two are also redundant

> +    };
> +
> +    PVE::QemuServer::foreach_drive($conf, $test);
> +
> +    return $volhash;
> +}
> +
>  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 3d9c61a..0336ade 100644
> --- a/PVE/VZDump/QemuServer.pm
> +++ b/PVE/VZDump/QemuServer.pm
> @@ -69,34 +69,34 @@ sub prepare {
>  
>      my $vollist = [];
>      my $drivehash = {};
> -    PVE::QemuServer::foreach_drive($conf, sub {
> -	my ($ds, $drive) = @_;
> +    my $backup_status = PVE::QemuConfig->get_volumes_backup_status($conf);
>  
> -	return if PVE::QemuServer::drive_is_cdrom($drive);
> +    foreach my $name (sort keys %{$backup_status}) {

this is again a different order then foreach_drive. I'd suggest either 
iterating using PVE::QemuServer::valid_drive_names, or switching the 
return value of your helper to return a list.

> +        my $disk = $backup_status->{$name};
>  
> -	my $volid = $drive->{file};
> +        my $volid = $disk->{data}->{file};
>  
> -	if (defined($drive->{backup}) && !$drive->{backup}) {
> -	    $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
> +	if (exists $disk->{included} && !$disk->{included}) {

if (!$disk->{included}) {

is enough - we ensure that value is either 1 or 0 in our helper

> +	    $self->loginfo("exclude disk '$name' '$volid' (backup=no)");
>  	    return;

this is wrong, we are no longer in a sub, but in a foreach (-> next;)

> -	} elsif ($self->{vm_was_running} && $drive->{iothread}) {
> +	} elsif ($self->{vm_was_running} && $disk->{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";

would it make sense to move this to the helper? otherwise the overview 
says a disk will be included, but the actual backup dies because it 
cannot be included?

>  	    }
>  	} 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 $disk->{data}->{size}) {
> +		my $readable_size = PVE::JSONSchema::format_size($disk->{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} = $disk->{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
> 
>