[pve-devel] [PATCH v5 manager 1/2] vzdump: move remaining guest include logic to single method

[Thomas Lamprecht]

On 5/6/20 11:57 AM, Aaron Lauterer wrote:
> The `guest include` logic handling `all` and `exclude` parameters was in
> the `PVE::VZDump->exec_backup()` method. Moving this logic into the
> `get_included_guests` method allows us to simplify and generalize it.
> 
> This helps to make the overall logic easier to test and develop other
> features around vzdump backup jobs.
> 
> The method now returns a hash with node names as keys mapped to arrays
> of VMIDs on these nodes that are included in the vzdump job.
> 
> The VZDump API call to create a new backup is adapted to use the new
> method to create the list of local VMIDs and the skiplist.
> 
> Permission checks are kept where they are to be able to handle missing
> permissions according to the current context. The old behavior to die
> on a backup job when the user is missing the permission to a guest and
> the job is not an 'all' or 'exclude' job is kept.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---

looks good in general, nice!

some comments and nits, as you probably will have to send a next version due
to the "avoid $self in get_included_guests" change anyway, still below.

> 
> v4 -> v5:
> * move handling of `all` and `exlcude` cases from
>   `PVE::VZDump->exec_backup` to `PVE::VZDump->get_included_guests`
> * change return value to hash of node names with arrays of included
>   VMIDs to be more general
> * adapt API call to start a VZDump job accordingly. Creating the list of
>   local VMIDs and the skiplist is handled right there again as so far
>   this is the only place where that kind of separation is needed
> 
> v3 -> v4:
> * reworked the whole logic according to the discussion with
> fabian [1].
> * re-added missing early exit
> 
> [1] https://pve.proxmox.com/pipermail/pve-devel/2020-March/042451.html
> 
>  PVE/API2/VZDump.pm | 18 ++++++++---
>  PVE/VZDump.pm      | 76 +++++++++++++++++++++-------------------------
>  2 files changed, 48 insertions(+), 46 deletions(-)
> 
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index 68a3de89..77dac1b1 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -69,19 +69,26 @@ __PACKAGE__->register_method ({
>  	return 'OK' if $param->{node} && $param->{node} ne $nodename;
>  
>  	my $cmdline = PVE::VZDump::Common::command_line($param);
> -	my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
> +
> +	my $vmids_per_node = PVE::VZDump->get_included_guests($param);
> +
> +	my $vmids = $vmids_per_node->{$nodename} // [];

maybe it could make belows looping easier if you delete the local ones out?
So, for above:
my $local_vmids = delete $vmids_per_node->{$nodename} // [];

> +
> +	my $skiplist = [];
> +
> +	for my $current_node (keys %{$vmids_per_node}) {
> +	    push @{$skiplist}, @{$vmids_per_node->{$current_node}}
> +		if $current_node ne $nodename;

with the delete above you could drop the post-if here actually you should be
able to just do:

my $skiplist = [ map { @$_ } values $vmids_per_node->%* ];


(FYI: $vmids_per_node->%* is just alternate writing of %$vmids_per_node IIRC,
Wolfgang prefers it)

> +	}
>  
>  	if($param->{stop}){
>  	    PVE::VZDump::stop_running_backups();
>  	    return 'OK' if !scalar(@{$vmids});
>  	}
>  
> -	# silent exit if specified VMs run on other nodes
> +	# silent exit if specified guests run on other nodes
>  	return "OK" if !scalar(@{$vmids});
>  
> -	my @exclude = PVE::Tools::split_list(extract_param($param, 'exclude'));
> -	$param->{exclude} = PVE::VZDump::check_vmids(@exclude);
> -
>  	# exclude-path list need to be 0 separated
>  	if (defined($param->{'exclude-path'})) {
>  	    my @expaths = split(/\0/, $param->{'exclude-path'} || '');
> @@ -106,6 +113,7 @@ __PACKAGE__->register_method ({
>  		die "interrupted by signal\n";
>  	    };
>  
> +	    $param->{vmids} = $vmids;
>  	    my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
>  
>  	    eval {
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 68c08f47..cd278f4d 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -1039,29 +1039,18 @@ sub exec_backup {
>  	if scalar(@{$self->{skiplist}});
>  
>      my $tasklist = [];
> +    my $vzdump_plugins =  {};
> +    foreach my $plugin (@{$self->{plugins}}) {

maybe add: 
next if exists $vzdump_plugins->{$type};

as then we'd just overwrite the same type again.

> +	my $type = $plugin->type();
> +	$vzdump_plugins->{$type} = $plugin;
> +    }
>  
> -    if ($opts->{all}) {
> -	foreach my $plugin (@{$self->{plugins}}) {
> -	    my $vmlist = $plugin->vmlist();
> -	    foreach my $vmid (sort @$vmlist) {
> -		next if grep { $_ eq  $vmid } @{$opts->{exclude}};
> -		next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], 1);
> -	        push @$tasklist, { vmid => $vmid,  state => 'todo', plugin => $plugin, mode => $opts->{mode} };
> -	    }
> -	}
> -    } else {
> -	foreach my $vmid (sort @{$opts->{vmids}}) {
> -	    my $plugin;
> -	    foreach my $pg (@{$self->{plugins}}) {
> -		my $vmlist = $pg->vmlist();
> -		if (grep { $_ eq  $vmid } @$vmlist) {
> -		    $plugin = $pg;
> -		    last;
> -		}
> -	    }
> -	    $rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ]);
> -	    push @$tasklist, { vmid => $vmid,  state => 'todo', plugin => $plugin, mode => $opts->{mode} };
> -	}
> +    my $vmlist = PVE::Cluster::get_vmlist();
> +    foreach my $vmid (sort @{$opts->{vmids}}) {
> +	my $guest_type = $vmlist->{ids}->{$vmid}->{type};
> +	my $plugin = $vzdump_plugins->{$guest_type};
> +	next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
> +	push @$tasklist, { vmid => $vmid,  state => 'todo', plugin => $plugin, mode => $opts->{mode} };

I now it blows up lines a bit but I prefer having multiple hash elements on
separate ones most of the time.

push @$tasklist, {
    vmid => $vmid,
    ...,
};

Easier to read and change.

>      }
>  
>      # Use in-memory files for the outer hook logs to pass them to sendmail.
> @@ -1169,34 +1158,39 @@ sub get_included_guests {
>  
>      my $nodename = PVE::INotify::nodename();
>      my $vmids = [];
> +    my $vmids_per_node = {};
> +
> +    my $vmlist = PVE::Cluster::get_vmlist();
>  
> -    # convert string lists to arrays
>      if ($job->{pool}) {
>  	$vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});
> +    } elsif ($job->{vmid}) {
> +	$vmids = [ PVE::Tools::split_list($job->{vmid}) ];
>      } else {
> -	$vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
> +	# all or exclude
> +	my @exclude = PVE::Tools::split_list($job->{exclude});
> +	@exclude = @{PVE::VZDump::check_vmids(@exclude)};
> +	my $excludehash = { map { $_ => 1 } @exclude };
> +
> +	for my $id (keys %{ $vmlist->{ids} }) {

you could sort it here already, avoiding the sort after the loop.

Whole thing could also be written as:

$vmids = [ grep { !$excludehash->{$_} } sort keys $vmlist->{ids}->%* ];

but no hard feeling here.

> +	    next if $excludehash->{$id};
> +	    push @$vmids, $id;
> +	}
>      }
> +    $vmids = [ sort {$a <=> $b} @$vmids];
>  
> -    my $skiplist = [];
> -    if (!$job->{all}) {
> -	if (!$job->{node} || $job->{node} eq $nodename) {
> -	    my $vmlist = PVE::Cluster::get_vmlist();
> -	    my $localvmids = [];
> -	    foreach my $vmid (@{$vmids}) {
> -		my $d = $vmlist->{ids}->{$vmid};
> -		if ($d && ($d->{node} ne $nodename)) {
> -		    push @{$skiplist}, $vmid;
> -		} else {
> -		    push @{$localvmids}, $vmid;
> -		}
> -	    }
> -	    $vmids = $localvmids;
> -	}
> +    $vmids = PVE::VZDump::check_vmids(@$vmids);
> +
> +    foreach my $vmid (@$vmids) {
> +	my $vmid_data = $vmlist->{ids}->{$vmid};
> +	my $node = $vmid_data->{node};
> +
> +	next if (defined $job->{node} && $job->{node} ne $node);>  
> -	$job->{vmids} = PVE::VZDump::check_vmids(@{$vmids})
> +	push @{$vmids_per_node->{$node}}, $vmid;
>      }
>  
> -    return ($vmids, $skiplist);
> +    return $vmids_per_node;
>  }
>  
>  1;
>