Am 6/17/20 um 2:13 PM schrieb Aaron Lauterer:
> 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.laute...@proxmox.com>
> ---
>
> v5 -> v6: create hash with volume data directly in push call instead of
> adding a new variable beforehand
looks OK in general, some issue with logging the reason below - sorry for
noticing that only now.
>
> 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
>
> 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..02cddeb 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 $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";
> + }
> +
> + push @$return_volumes, {
> + key => $key,
> + included => $included,
> + reason => $reason,
> + data => $drive,
> + };
> + };
> +
> + 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 91df455..9eeb1b9 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};
data is so general, maybe volcfg or even volume_cfg or more even™ volume_config
(for both, hash key and variable name)
> + 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)");
I really do not like checking the reason string manually and transforming it
here
if we have it under full control already on generation.
Rather just do
$self->loginfo("exclude disk '$name' '$volid' ($volume->{reason})");
If you think the reason isn't explaining enough add to it when setting it, but
IMO
the efi one works OK as is, and the "nobackup" set just could be "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) {
as $size is only used here you could avoid it above and do:
if (defined(my $size = $vol_cfg->{size})) {
just a suggestion.
> + 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);
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
