[pve-devel] [PATCH v2 manager 1/2] vzdump: make guest include logic testable

[Thomas Lamprecht]

On 5/4/20 4:08 PM, Aaron Lauterer wrote:
> As a first step to make the whole guest include logic more testable the
> part from the API endpoint has been moved to its own method with as
> little changes as possible.
> 
> Everything concerning `all` and `exclude` logic is still in the
> PVE::VZDump->exec_backup() method.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
> 
> v1 -> v2:
> * fixed return value. Array refs inside an array lead to nested
>   arrays not working with `my ($foo, $bar) = method();`
> 
> 
> As talked with thomas on[0] and off list, this patch series is meant to
> have more confidence in the ongoing changes.
> 
> My other ongoing patch series [1] will move the all the logic, even the
> one in the `exec_backup()` method into one single method.
> 
> [0] https://pve.proxmox.com/pipermail/pve-devel/2020-April/042795.html
> [1] https://pve.proxmox.com/pipermail/pve-devel/2020-April/042753.html
> 
>  PVE/API2/VZDump.pm | 36 ++++++------------------------------
>  PVE/VZDump.pm      | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index f01e4de0..68a3de89 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -69,39 +69,15 @@ __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'));
> -	}
> +	my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
>  
>  	if($param->{stop}){
>  	    PVE::VZDump::stop_running_backups();
> -	    return 'OK' if !scalar(@vmids);
> +	    return 'OK' if !scalar(@{$vmids});
>  	}
>  
> -	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);
> -	    }
> -
> -	    $param->{vmids} = PVE::VZDump::check_vmids(@vmids)
> -	}
> +	# silent exit if specified VMs 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);
> @@ -118,7 +94,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};
> @@ -167,7 +143,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..73ad9088 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);
>  
> @@ -1156,4 +1157,39 @@ sub stop_running_backups {
>      }
>  }
>  
> +sub get_included_guests {
> +    my ($self, $job) = @_;

do we  need $self here? Why not call it like: PVE::VZDump::get_included_guests($params) ?

> +
> +    my $nodename = PVE::INotify::nodename();
> +    my $vmids = [];
> +
> +    # convert string lists to arrays
> +    if ($job->{pool}) {
> +	$vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});

You use API2Tools here but do not add it as module use statement.


> +    } else {
> +	$vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
> +    }
> +
> +    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;
> +	}
> +
> +	$job->{vmids} = PVE::VZDump::check_vmids(@{$vmids})
> +    }
> +
> +    return ($vmids, $skiplist);
> +}
> +
>  1;
>