[pve-devel] [PATCH container 2/2] backup: refactor mp included in backup to use from other modules

[Fabian Grünbichler]

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.lauterer at 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 at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>