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

[Thomas Lamprecht]

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.

> [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?


>  	}
>  
> -	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}}) {

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

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


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.


> +    }
>  
> -    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.

> +	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).

> +    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, 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?

> +	@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, 

} 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;
>