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

[Aaron Lauterer]

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

v2 -> v3:
* remove $self from get_included_guests
* added `use` statement for API2Tools
* fixed a regression introduced in the previous revision that would
  skip the backup job completely if the --all flag was set

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
(new revision is on the way)

 PVE/API2/VZDump.pm | 36 ++++++------------------------------
 PVE/VZDump.pm      | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index f01e4de0..8ba20d17 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}) && !$param->{all};
 
 	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 c4ed60b7..55280784 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -21,6 +21,8 @@ use PVE::RPCEnvironment;
 use PVE::Storage;
 use PVE::VZDump::Common;
 use PVE::VZDump::Plugin;
+use PVE::Tools qw(extract_param);
+use PVE::API2Tools;
 
 my @posix_filesystems = qw(ext3 ext4 nfs nfs4 reiserfs xfs);
 
@@ -1165,4 +1167,39 @@ sub stop_running_backups {
     }
 }
 
+sub get_included_guests {
+    my ($job) = @_;
+
+    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});
+    } 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;
-- 
2.20.1