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.laute...@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
