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

[Aaron Lauterer]

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 will return the VMIDs for the whole cluster. If a VMID is
present on the local node and thus part of the local backup job is still
determined in the VZDump::exec_backup method.

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.

The creation of the skiplist is moved to the VZDump::exec_backup method,
close to the place where it is used.

Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
---
changes: rebased

 PVE/API2/VZDump.pm | 36 +++-------------------
 PVE/VZDump.pm      | 74 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index f01e4de0..bc380099 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -69,43 +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'));
-	}
+
+	$param->{vmids} = PVE::VZDump->get_included_guests($param, $nodename);
+	my @vmids = @{$param->{vmids}};
 
 	if($param->{stop}){
 	    PVE::VZDump::stop_running_backups();
 	    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)
-	}
-
-	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'} || '');
@@ -130,7 +102,7 @@ __PACKAGE__->register_method ({
 		die "interrupted by signal\n";
 	    };
 
-	    my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
+	    my $vzdump = PVE::VZDump->new($cmdline, $param);
 
 	    eval {
 		$vzdump->getlock($upid); # only one process allowed
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index f3274196..9368d439 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);
 
@@ -379,7 +380,7 @@ sub sendmail {
 };
 
 sub new {
-    my ($class, $cmdline, $opts, $skiplist) = @_;
+    my ($class, $cmdline, $opts) = @_;
 
     mkpath $logdir;
 
@@ -418,8 +419,7 @@ sub new {
     $opts->{dumpdir} =~ s|/+$|| if ($opts->{dumpdir});
     $opts->{tmpdir} =~ s|/+$|| if ($opts->{tmpdir});
 
-    $skiplist = [] if !$skiplist;
-    my $self = bless { cmdline => $cmdline, opts => $opts, skiplist => $skiplist };
+    my $self = bless { cmdline => $cmdline, opts => $opts };
 
     my $findexcl = $self->{findexcl} = [];
     if ($defaults->{'exclude-path'}) {
@@ -1026,33 +1026,33 @@ sub exec_backup {
 
     my $opts = $self->{opts};
 
+    my $nodename = PVE::INotify::nodename();
+    my $skiplist = [];
+
+    if ($nodename && (!$opts->{node} ||$opts->{node} eq $nodename)) {
+	my $vmlist = PVE::Cluster::get_vmlist();
+	foreach my $vmid (@{$opts->{vmids}}) {
+	    my $d = $vmlist->{ids}->{$vmid};
+	    if ($d && ($d->{node} ne $nodename)) {
+		push @$skiplist, $vmid;
+	    }
+	}
+    }
+
     debugmsg ('info', "starting new backup job: $self->{cmdline}", undef, 1);
-    debugmsg ('info', "skip external VMs: " . join(', ', @{$self->{skiplist}}))
-	if scalar(@{$self->{skiplist}});
+    debugmsg ('info', "skip external VMs: " . join(', ', @{$skiplist}))
+	if scalar(@{$skiplist});
 
     my $tasklist = [];
 
-    if ($opts->{all}) {
+    foreach my $vmid (sort @{$opts->{vmids}}) {
 	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;
-		}
+	    if (grep { $_ eq  $vmid } @$vmlist) {
+		next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
+		push @$tasklist, { vmid => $vmid,  state => 'todo', plugin => $plugin, mode => $opts->{mode} };
+		last;
 	    }
-	    $rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ]);
-	    push @$tasklist, { vmid => $vmid,  state => 'todo', plugin => $plugin, mode => $opts->{mode} };
 	}
     }
 
@@ -1156,4 +1156,32 @@ sub stop_running_backups {
     }
 }
 
+sub get_included_guests {
+    my ($self, $job, $nodename) = @_;
+
+    my @vmids;
+    my $vmlist = PVE::Cluster::get_vmlist();
+
+    if ($job->{pool}) {
+	@vmids = @{PVE::API2Tools::get_resource_pool_guest_members($job->{pool})};
+    } elsif ($job->{vmid}) {
+	# selected VMIDs
+	@vmids = PVE::Tools::split_list(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;
+
+    return PVE::VZDump::check_vmids(@vmids);
+}
+
 1;
-- 
2.20.1