[pve-devel] [PATCH v4 manager 1/1] backup: move logic to include guests into method

[Aaron Lauterer]

On 4/6/20 1:23 PM, Thomas Lamprecht wrote:
> On 4/3/20 4:11 PM, Aaron Lauterer wrote:
>> This extracts the logic which guests are to be included in a backup job
>> into its own method 'get_included_guests'. This makes it possible to
>> develop other features around backup jobs.
>>
>> Logic which was spread out accross the API2/VZDump.pm file and the
>> VZDump.pm file is refactored into the new method, most notably the
>> logic that dealt with excluded guests.
>>
>> The new method returns either just the included VMIDs on the local node
>> or an array with with the local VMIDs and the VMIDs present on other
>> nodes in the cluster. The latter is used for 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>
>> ---
>>
>> v3 -> v4:
>> * reworked the whole logic according to the discussion with
>> fabian [0].
>> * re-added missing early exit
>>
> 
> this seems to be a perfect fit for adding some light testing before doing
> the change.

I'll look into it.

> 
>> [0] https://pve.proxmox.com/pipermail/pve-devel/2020-March/042451.html
>>
>>   PVE/API2/VZDump.pm | 43 ++++++--------------------
>>   PVE/VZDump.pm      | 76 ++++++++++++++++++++++++++++++++--------------
>>   2 files changed, 63 insertions(+), 56 deletions(-)
>>
>> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
>> index f01e4de0..88f53b82 100644
>> --- a/PVE/API2/VZDump.pm
>> +++ b/PVE/API2/VZDump.pm
>> @@ -69,43 +69,17 @@ __PACKAGE__->register_method ({
>>   	return 'OK' if $param->{node} && $param->{node} ne $nodename;
>>   
>>   	my $cmdline = PVE::VZDump::Common::command_line($param);
>> -	my @vmids;
>> -	# convert string lists to arrays
>> -	if ($param->{pool}) {
>> -	    @vmids = @{PVE::API2Tools::get_resource_pool_guest_members($param->{pool})};
>> -	} else {
>> -	    @vmids = PVE::Tools::split_list(extract_param($param, 'vmid'));
>> -	}
>>   
>> -	if($param->{stop}){
>> -	    PVE::VZDump::stop_running_backups();
>> -	    return 'OK' if !scalar(@vmids);
>> -	}
>> +	my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
>>   
>> -	my $skiplist = [];
>> -	if (!$param->{all}) {
>> -	    if (!$param->{node} || $param->{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;
>> -		# silent exit if specified VMs run on other nodes
>> -		return "OK" if !scalar(@vmids);
>> -	    }
>> +	# silent exit if specified guests run on other nodes
>> +	return "OK" if !scalar(@$vmids);
>>   
>> -	    $param->{vmids} = PVE::VZDump::check_vmids(@vmids)
>> +	if($param->{stop}){
>> +	    PVE::VZDump::stop_running_backups();
>> +	    return 'OK' if !scalar(@$vmids);
> 
> How can I even get to above, if we return 'OK' if @$vmids is empty before
> this if already?

thx for catching this

> 
> 
>>   	}
>>   
>> -	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'} || '');
>> @@ -118,7 +92,7 @@ __PACKAGE__->register_method ({
>>   	}
>>   
>>   	die "you can only backup a single VM with option --stdout\n"
>> -	    if $param->{stdout} && scalar(@vmids) != 1;
>> +	    if $param->{stdout} && scalar(@$vmids) != 1;
>>   
>>   	$rpcenv->check($user, "/storage/$param->{storage}", [ 'Datastore.AllocateSpace' ])
>>   	    if $param->{storage};
>> @@ -130,6 +104,7 @@ __PACKAGE__->register_method ({
>>   		die "interrupted by signal\n";
>>   	    };
>>   
>> +	    $param->{vmids} = $vmids;
>>   	    my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
>>   
>>   	    eval {
>> @@ -167,7 +142,7 @@ __PACKAGE__->register_method ({
>>   	}
>>   
>>   	my $taskid;
>> -	$taskid = $vmids[0] if scalar(@vmids) == 1;
>> +	$taskid = $$vmids[0] if scalar(@$vmids) == 1;
>>   
>>   	return $rpcenv->fork_worker('vzdump', $taskid, $user, $worker);
>>      }});
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index f3274196..3bcfd422 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>> @@ -21,6 +21,7 @@ use PVE::RPCEnvironment;
>>   use PVE::Storage;
>>   use PVE::VZDump::Common;
>>   use PVE::VZDump::Plugin;
>> +use PVE::Tools qw(extract_param);
>>   
>>   my @posix_filesystems = qw(ext3 ext4 nfs nfs4 reiserfs xfs);
>>   
>> @@ -1026,34 +1027,23 @@ sub exec_backup {
>>   
>>       my $opts = $self->{opts};
>>   
>> +    my $nodename = PVE::INotify::nodename();
>> +
>>       debugmsg ('info', "starting new backup job: $self->{cmdline}", undef, 1);
>>       debugmsg ('info', "skip external VMs: " . join(', ', @{$self->{skiplist}}))
>>   	if scalar(@{$self->{skiplist}});
>>   
>>       my $tasklist = [];
>> +    my $vzdump_plugins =  {};
>> +    foreach my $p (@{$self->{plugins}}) {
> 
> I do not want such one character variables, even for short stuff
> 
> for my $plugin (@{$self->{plugins}}) {

Ok
> 
>> +	$vzdump_plugins->{$p->type()} = $p;
> 
> my $type = $plugin->type();
> vzdump_plugins->{$type} = $plugin;
> 

ok

> 
> Also, this (+ related changes below) could well be it's own patch, it has nothing
> directly to do with pulling out the "which guest to include" logic out.
> 
> 

okay, will put this in a separate patch
>> +    }
>>   
>> -    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 $plugin = $vzdump_plugins->{$vmlist->{ids}->{$vmid}->{type}};
> 
> NAK, accessing other hashes with a hash key access is considered bad style, avoid this.
> 
same as above right?

>> +	next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
>> +	push @$tasklist, { vmid => $vmid,  state => 'todo', plugin => $plugin, mode => $opts->{mode} };
>>       }
>>   
>>       # Use in-memory files for the outer hook logs to pass them to sendmail.
>> @@ -1156,4 +1146,46 @@ sub stop_running_backups {
>>       }
>>   }
>>   
>> +sub get_included_guests {
>> +    my ($self, $job) = @_;
>> +
>> +    my $nodename = PVE::INotify::nodename();
>> +    my @vmids;
> 
> why not a array reference here? Would make most calls below a bit easier,
> and cheaper (less copying).

okay, will do that
> 
>> +    my $local_vmids = [];
>> +    my $foreign_vmids = [];
> 
> We never use "foreign" to describe that something is located on another
> cluster node, use $cluster_vmids
> Actually, we could just use a $hash with the nodes as key, as this seems a bit
> a specific distinction to do in such a general method, e.g.:
> 
> {
>      node1 => [100, 200],
>      node1 => [3900, ...],
>      ...
> 
>      node1 => [...],
> }
> 

also a possible approach I agree. But then I would like to add more 
wrapper methods. For now I definitely need one that returns all VMIDs 
and one that separates between local and cluster ones. That would then 
help the wantarray situation below.

> 
> Also, I like to avoid wantarray, at least if easily possible. If you'd need a
> method for just the local vmids included in a job I'd add an additional method
> which calls the more general one and filters plus explicitly tells the reader
> the behaviour in it's name, e.g. "get_included_local_guests"
> 
>> +
>> +    my $vmlist = PVE::Cluster::get_vmlist();
>> +
>> +    if ($job->{pool}) {
>> +	@vmids = @{PVE::API2Tools::get_resource_pool_guest_members($job->{pool})};
>> +    } elsif ($job->{vmid}) {
>> +	# selected VMIDs
> 
> useless comment? What does it explain for a why or how question?
added it so one knows where the vmids in this step come from as it is 
not as obvious, same goes for the comment in the next else clause

> 
>> +	@vmids = PVE::Tools::split_list(extract_param($job, 'vmid'));
> 
> do we depend on the side effect that the $job hash ref has vmid deleted?
> If so, a comment at the top of the method regarding this and exclude below
> would be nice. Further,
> 

AFAICT it's not really needed, the first elsif option should work as 
well. Do I understand correctly that `extract_param()` should be avoided?

> } elsif (my $vmids = $job->{vmid}) {
>      $vmids = [ PVE::Tools::split_list($vmid) ];
> 
> or
> 
> } elsif (my $vmids = delete $job->{vmid}) {
>      ...
> 
> or if you really want to use extract_param
> 
> } elsif (my $vmids = $extract_param($job, 'vmid')) {
>      ...
> 
> 
>> +    } else {
>> +	# all or exclude
>> +	my @exclude = PVE::Tools::split_list(extract_param($job, 'exclude'));
>> +	@exclude = @{PVE::VZDump::check_vmids(@exclude)};
>> +	my $excludehash = { map { $_ => 1 } @exclude };
>> +
>> +	foreach my $id (keys %{ $vmlist->{ids} }) {
>> +	    next if $excludehash->{$id};
>> +	    push @vmids, $id
>> +		if !$job->{node} || $vmlist->{ids}->{$id}->{node} eq $job->{node};
>> +	}
>> +    }
>> +    @vmids = sort {$a <=> $b} @vmids;
>> +
>> +    @vmids = @{PVE::VZDump::check_vmids(@vmids)};
>> +
>> +    foreach my $vmid (@vmids) {
>> +	my $d = $vmlist->{ids}->{$vmid};
>> +	if ($d && ($d->{node} ne $nodename)) {
>> +	    push @$foreign_vmids, $vmid;
>> +	} else {
>> +	    push @$local_vmids, $vmid;
>> +	}
>> +    }
>> +    return wantarray ? ($local_vmids, $foreign_vmids) : $local_vmids;
>> +}
>> +
>>   1;
>>
>