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

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

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 | 18 ++++++++---
 PVE/VZDump.pm      | 76 +++++++++++++++++++++-------------------------
 2 files changed, 48 insertions(+), 46 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 68a3de89..77dac1b1 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -69,19 +69,26 @@ __PACKAGE__->register_method ({
 	return 'OK' if $param->{node} && $param->{node} ne $nodename;
 
 	my $cmdline = PVE::VZDump::Common::command_line($param);
-	my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
+
+	my $vmids_per_node = PVE::VZDump->get_included_guests($param);
+
+	my $vmids = $vmids_per_node->{$nodename} // [];
+
+	my $skiplist = [];
+
+	for my $current_node (keys %{$vmids_per_node}) {
+	    push @{$skiplist}, @{$vmids_per_node->{$current_node}}
+		if $current_node ne $nodename;
+	}
 
 	if($param->{stop}){
 	    PVE::VZDump::stop_running_backups();
 	    return 'OK' if !scalar(@{$vmids});
 	}
 
-	# silent exit if specified VMs run on other nodes
+	# silent exit if specified guests 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);
-
 	# exclude-path list need to be 0 separated
 	if (defined($param->{'exclude-path'})) {
 	    my @expaths = split(/\0/, $param->{'exclude-path'} || '');
@@ -106,6 +113,7 @@ __PACKAGE__->register_method ({
 		die "interrupted by signal\n";
 	    };
 
+	    $param->{vmids} = $vmids;
 	    my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
 
 	    eval {
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 68c08f47..cd278f4d 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1039,29 +1039,18 @@ sub exec_backup {
 	if scalar(@{$self->{skiplist}});
 
     my $tasklist = [];
+    my $vzdump_plugins =  {};
+    foreach my $plugin (@{$self->{plugins}}) {
+	my $type = $plugin->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} };
     }
 
     # Use in-memory files for the outer hook logs to pass them to sendmail.
@@ -1169,34 +1158,39 @@ sub get_included_guests {
 
     my $nodename = PVE::INotify::nodename();
     my $vmids = [];
+    my $vmids_per_node = {};
+
+    my $vmlist = PVE::Cluster::get_vmlist();
 
-    # convert string lists to arrays
     if ($job->{pool}) {
 	$vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});
+    } elsif ($job->{vmid}) {
+	$vmids = [ PVE::Tools::split_list($job->{vmid}) ];
     } else {
-	$vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
+	# all or exclude
+	my @exclude = PVE::Tools::split_list($job->{exclude});
+	@exclude = @{PVE::VZDump::check_vmids(@exclude)};
+	my $excludehash = { map { $_ => 1 } @exclude };
+
+	for my $id (keys %{ $vmlist->{ids} }) {
+	    next if $excludehash->{$id};
+	    push @$vmids, $id;
+	}
     }
+    $vmids = [ sort {$a <=> $b} @$vmids];
 
-    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;
-	}
+    $vmids = PVE::VZDump::check_vmids(@$vmids);
+
+    foreach my $vmid (@$vmids) {
+	my $vmid_data = $vmlist->{ids}->{$vmid};
+	my $node = $vmid_data->{node};
+
+	next if (defined $job->{node} && $job->{node} ne $node);
 
-	$job->{vmids} = PVE::VZDump::check_vmids(@{$vmids})
+	push @{$vmids_per_node->{$node}}, $vmid;
     }
 
-    return ($vmids, $skiplist);
+    return $vmids_per_node;
 }
 
 1;
-- 
2.20.1