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

[Thomas Lamprecht]

Am 6/17/20 um 2:13 PM schrieb Aaron Lauterer:
> 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>
> ---
> 
> v5 -> v6:
> * incorporate suggested changes
> * did not move sorting in PVE::VZDump::get_included_guests into the `if`
>   clause because that would sort them only for `all` and/or `exclude`
>   jobs but the VMIDs can be provided in random order for other job types
>   like manually selected VMIDs as well.
> 
> 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 | 19 ++++++-----
>  PVE/VZDump.pm      | 80 ++++++++++++++++++++++------------------------
>  2 files changed, 50 insertions(+), 49 deletions(-)

applied, thanks! made a few small followups, most importan pulling the sorting
inside check_vmid, as that generates a new array anyway so needs to iterate
over all, and a style fix (see below).

> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index bdbf641e..2f477606 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -1044,29 +1044,23 @@ sub exec_backup {
>  	if scalar(@{$self->{skiplist}});
>  
>      my $tasklist = [];
> +    my $vzdump_plugins =  {};
> +    foreach my $plugin (@{$self->{plugins}}) {
> +	my $type = $plugin->type();
> +	next if exists $vzdump_plugins->{$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} };


For hashes:
* always a trailing comma, allows to make insertions without touching any existing line at the end
* closing brace on new line:
* optionally and not always wanted, sort keys alphabetically


push @$tasklist, {
    vmid => $vmid,
    state => 'todo',
    plugin => $plugin,
    mode => $opts->{mode},
};