On January 16, 2020 2:00 pm, Aaron Lauterer wrote:
> This refactor will allow us to use the same code that decides which
> mountpoint will be backed up in multiple places and provide a reason for
> the decision.
>
> Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
> ---
> src/PVE/LXC/Config.pm | 38 ++++++++++++++++++++++++++++++++++++++
> src/PVE/VZDump/LXC.pm | 42 +++++++++++++++++++++++++-----------------
> 2 files changed, 63 insertions(+), 17 deletions(-)
>
> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
> index 760ec23..1dcfc40 100644
> --- a/src/PVE/LXC/Config.pm
> +++ b/src/PVE/LXC/Config.pm
> @@ -1558,4 +1558,42 @@ sub get_replicatable_volumes {
> return $volhash;
> }
>
> +sub get_volumes_backup_status {
> + my ($class, $conf) = @_;
> +
> + my $volhash = {};
> +
> + my $test_volid = sub {
this does not test a volid
> + my ($volid, $attr) = @_;
and these are not the parameters that this function will be called with.
my ($key, $mountpoint)
or
my ($key, $volume)
> + return if !$volid;
this should not be possible (foreach_mountpoint iterates over keys)
> +
> + my $status = $class->mountpoint_backup_enabled($volid, $attr);
$enabled ?
> + my $reason = 'not able to determine reason';
'unknown' (likely already translated, way easier if not), but the
if/elsif is exhaustive since mountpoint_backup_enabled always returns
1/0, so it could be converted to
if ($enabled) {
...
} else {
...
}
> +
> + if ($status == 0) {
> + if ($attr->{type} ne 'volume') {
> + $reason = 'not a volume';
"$attr->{type} mountpoint"
> + } else {
> + $reason = 'default exclude';
or explicit? we don't know..
> + }
> + } elsif ($status == 1) {
> + if ($volid eq 'rootfs') {
> + $reason = 'rootfs';
> + } elsif (exists($attr->{backup}) && $attr->{backup}) {
the exists is redundant. in fact, the whole condition is, since if
$enabled == 1, and it's not a rootfs, it can only be because of the
mountpoint having the property set ;)
> + $reason = 'manual';
> + }
> + }
I wonder whether it would not make more sense to push this into
mountpoint_backup_enabled, and optionally let that return the reason as
well? otherwise we have to places where we encode this logic..
-
> +
> + $volhash->{$volid}->{included} = $status;
> + $volhash->{$volid}->{reason} = $reason;
> + $volhash->{$volid}->{volume} = $attr->{volume};
> + $volhash->{$volid}->{data} = $attr;
the last two are redundant
> + };
> +
> + PVE::LXC::Config->foreach_mountpoint($conf, $test_volid);
> +
> + return $volhash;
if we instead push to a list here, we'd have the proper order, and could
just iterate in the VZDump code below.. not sure what implications that
would have for the rest of the code though ;)
> +}
> +
> 1;
> diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
> index 0260184..df60d08 100644
> --- a/src/PVE/VZDump/LXC.pm
> +++ b/src/PVE/VZDump/LXC.pm
> @@ -118,24 +118,32 @@ sub prepare {
> $task->{rootgid} = $rootgid;
>
> my $volids = $task->{volids} = [];
> - PVE::LXC::Config->foreach_mountpoint($conf, sub {
> - my ($name, $data) = @_;
> - my $volid = $data->{volume};
> - my $mount = $data->{mp};
> - my $type = $data->{type};
> -
> - return if !$volid || !$mount;
> -
> - if (!PVE::LXC::Config->mountpoint_backup_enabled($name, $data)) {
> - push @$exclude_dirs, $mount;
> - $self->loginfo("excluding $type mount point $name ('$mount') from
> backup");
> - return;
> - }
>
> - push @$disks, $data;
> - push @$volids, $volid
> - if $type eq 'volume';
> - });
> + my $backup_status = PVE::LXC::Config->get_volumes_backup_status($conf);
same naming comment applies here
> +
> + # mp order is important. rootfs needs to be processed before mpX
> + my @mp_keys = sort keys %{$backup_status};
> + unshift(@mp_keys, pop @mp_keys);
> +
> + foreach my $name (@mp_keys) {
please use the proper iterator then ;) that way we don't need to hunt
bugs if we ever introduce a new mountpoint class starting with something
after 'r'.
> + my $disk = $backup_status->{$name};
> +
> + my $volid = $disk->{data}->{volume};
> + my $mount = $disk->{data}->{mp};
> + my $type = $disk->{data}->{type};
> +
> + return if !$volid || !$mount;
> +
> + if (exists $disk->{included} && !$disk->{included}) {
> + push @$exclude_dirs, $mount;
> + $self->loginfo("excluding $type mount point $name ('$mount')
> from backup");
> + next;
> + }
> +
> + push @$disks, $disk->{data};
> + push @$volids, $volid
> + if $disk->{included};
> + }
it's usually a bad sign when the new code after extracting parts into a
helper is longer than the old code, but much of this is redundant (not
tested!):
my $mountpoint_info = PVE::LXC::Config->get_backup_info($conf);
foreach my $mp (PVE::LXC::Config->mountpoint_names()) {
next if !$mountpoint_info->{$mp};
my $info = $mountpoint_info->{$mp};
my $data = $info->{data};
next if !$data->{volume} || !$data->{mp};
if (!$info->{included}) {
push @$exclude_dirs, $data->{mp};
$self->loginfo("excluding $data->{type} mount point $mp ('$data->{mp}')
from backup");
} else {
push @$disks, $data;
push @$volids, $data->{volume};
}
}
>
> if ($mode eq 'snapshot') {
> if (!PVE::LXC::Config->has_feature('snapshot', $conf, $storage_cfg,
> undef, undef, 1)) {
> --
> 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
