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

[Aaron Lauterer]

On 6/17/20 3:50 PM, Thomas Lamprecht wrote:
> 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).

sounds good and sorry for that style slip, must have pressed undo one or 
two times too many at some point without noticing it ;)
> 
>> 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},
> };
>