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

[Fabian Grünbichler]

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.

> +
> +    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
> 
>