[pve-devel] [RFC/PATCH v2 manager 8/8] don't group by node in get_included_guests

[Fabian Ebner <f.ebner@proxmox.com>]

Seems to simplify the handling for two of the callers and not complicate it for
the remaining one.

Including the type as well avoids the need to use the vmlist again in the
included_volumes API call.

In get_included_volumes, returning {} in the else branch was made explicit.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

This is mostly a matter of taste and completely optional to the goal of the
series.

I'm not completely happy with using the '' key for the orphaned IDs and this
representation feels closer to what the callers actually need. Although, one
can argue about the caller in the vzdump API call.

And just some thought for the future: if we'd pass along the full hash to
exec_backup instead of just the IDs, the skipping of external IDs could be done
more locally.

@Aaron: Does getting rid of the grouping complicate some of the things that are
still planned with get_included_guests?

 PVE/API2/Backup.pm                 |  23 ++---
 PVE/API2/BackupInfo.pm             |   2 +-
 PVE/API2/VZDump.pm                 |  23 +++--
 PVE/VZDump.pm                      |  15 ++-
 test/vzdump_guest_included_test.pl | 152 ++++++++++++++++++++++++++---
 5 files changed, 169 insertions(+), 46 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 3b343636..63d9c261 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -422,22 +422,13 @@ __PACKAGE__->register_method({
 	}
 	raise_param_exc({ id => "No such job '$param->{id}'" }) if !$job;
 
-	my $vmlist = PVE::Cluster::get_vmlist();
-
-	my @job_vmids;
-
 	my $included_guests = PVE::VZDump::get_included_guests($job);
 
-	for my $node (keys %{$included_guests}) {
-	    my $node_vmids = $included_guests->{$node};
-	    push(@job_vmids, @{$node_vmids});
-	}
-
 	# remove VMIDs to which the user has no permission to not leak infos
 	# like the guest name
 	my @allowed_vmids = grep {
-		$rpcenv->check($user, "/vms/$_", [ 'VM.Audit' ], 1);
-	} @job_vmids;
+	    $rpcenv->check($user, "/vms/$_", [ 'VM.Audit' ], 1);
+	} keys %{$included_guests};
 
 	my $result = {
 	    children => [],
@@ -447,11 +438,11 @@ __PACKAGE__->register_method({
 
 	    my $children = [];
 
-	    # It's possible that a job has VMIDs configured that are not in
-	    # vmlist. This could be because a guest was removed but not purged.
+	    # When removing a guest without purge, the backup configuration
+	    # is not updated. Such orphaned IDs don't have a node.
 	    # Since there is no more data available we can only deliver the VMID
 	    # and no volumes.
-	    if (!defined $vmlist->{ids}->{$vmid}) {
+	    if (!defined($included_guests->{$vmid}->{node})) {
 		push(@{$result->{children}}, {
 		    id => int($vmid),
 		    type => 'unknown',
@@ -460,8 +451,8 @@ __PACKAGE__->register_method({
 		next;
 	    }
 
-	    my $type = $vmlist->{ids}->{$vmid}->{type};
-	    my $node = $vmlist->{ids}->{$vmid}->{node};
+	    my $type = $included_guests->{$vmid}->{type};
+	    my $node = $included_guests->{$vmid}->{node};
 
 	    my $conf;
 	    my $volumes;
diff --git a/PVE/API2/BackupInfo.pm b/PVE/API2/BackupInfo.pm
index 4c461e59..aa22ad34 100644
--- a/PVE/API2/BackupInfo.pm
+++ b/PVE/API2/BackupInfo.pm
@@ -25,7 +25,7 @@ sub get_included_vmids {
 
     for my $job (@$all_jobs) {
 	my $job_included_guests = PVE::VZDump::get_included_guests($job);
-	push @all_vmids, ( map { @{$_} } values %{$job_included_guests} );
+	push @all_vmids, keys %{$job_included_guests};
     }
 
     return { map { $_ => 1 } @all_vmids };
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 806ac7fd..f8aa46b7 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -70,15 +70,20 @@ __PACKAGE__->register_method ({
 
 	my $cmdline = PVE::VZDump::Common::command_line($param);
 
-	my $vmids_per_node = PVE::VZDump::get_included_guests($param);
-
-	my $local_vmids = delete $vmids_per_node->{$nodename} // [];
-
-	# include IDs for deleted guests, and visibly fail later
-	my $orphaned_vmids = delete $vmids_per_node->{''} // [];
-	push @{$local_vmids}, @{$orphaned_vmids};
-
-	my $skiplist = [ map { @$_ } values $vmids_per_node->%* ];
+	my $included_guests = PVE::VZDump::get_included_guests($param);
+
+	my $local_vmids = [];
+	my $skiplist = [];
+
+	foreach my $vmid (keys %{$included_guests}) {
+	    my $node = $included_guests->{$vmid}->{node};
+	    # include IDs for deleted guests here, and visibly fail later
+	    if (!defined($node) || $node eq $nodename) {
+		push @{$local_vmids}, $vmid;
+	    } else {
+		push @{$skiplist}, $vmid;
+	    }
+	}
 
 	if($param->{stop}){
 	    PVE::VZDump::stop_running_backups();
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 2f31c534..f6399cf7 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1201,7 +1201,7 @@ sub get_included_guests {
     my ($job) = @_;
 
     my $vmids = [];
-    my $vmids_per_node = {};
+    my $vmid_hash = {};
 
     my $vmlist = PVE::Cluster::get_vmlist();
 
@@ -1219,22 +1219,27 @@ sub get_included_guests {
 	    push @$vmids, $id;
 	}
     } else {
-	return $vmids_per_node;
+	return {};
     }
     $vmids = check_vmids(@$vmids);
 
     for my $vmid (@$vmids) {
 	if (defined($vmlist->{ids}->{$vmid})) {
 	    my $node = $vmlist->{ids}->{$vmid}->{node};
+	    my $type = $vmlist->{ids}->{$vmid}->{type};
+
 	    next if (defined $job->{node} && $job->{node} ne $node);
 
-	    push @{$vmids_per_node->{$node}}, $vmid;
+	    $vmid_hash->{$vmid} = {
+		node => $node,
+		type => $type,
+	    };
 	} else {
-	    push @{$vmids_per_node->{''}}, $vmid;
+	    $vmid_hash->{$vmid} = {};
 	}
     }
 
-    return $vmids_per_node;
+    return $vmid_hash;
 }
 
 1;
diff --git a/test/vzdump_guest_included_test.pl b/test/vzdump_guest_included_test.pl
index 157644de..6ebe009a 100755
--- a/test/vzdump_guest_included_test.pl
+++ b/test/vzdump_guest_included_test.pl
@@ -85,8 +85,38 @@ my $addtest = sub {
 
 $addtest->('Test all guests', {
     expected => {
-	node1 => [ 100, 101, 112, 113 ],
-	node2 => [ 200, 201, 212, 213 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	112 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	113 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	213 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	all => 1,
@@ -95,7 +125,22 @@ $addtest->('Test all guests', {
 
 $addtest->('Test all guests with node limit', {
     expected => {
-	node2 => [ 200, 201, 212, 213 ],
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	213 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	all => 1,
@@ -105,8 +150,30 @@ $addtest->('Test all guests with node limit', {
 
 $addtest->('Test exclude', {
     expected => {
-	node1 =>[ 101, 112, 113 ],
-	node2 => [ 201, 212,  213 ],
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	112 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	113 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	213 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	all => 1,
@@ -116,7 +183,18 @@ $addtest->('Test exclude', {
 
 $addtest->('Test exclude with node limit', {
     expected => {
-	node1 =>[ 101, 112, 113 ],
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	112 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
+	113 => {
+	    'type' => 'lxc',
+	    'node' => 'node1',
+	},
     },
     param => {
 	all => 1,
@@ -127,8 +205,22 @@ $addtest->('Test exclude with node limit', {
 
 $addtest->('Test pool members', {
     expected => {
-	node1 =>[ 100, 101 ],
-	node2 => [ 200, 201 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	101 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
     },
     param => {
 	pool => 'testpool',
@@ -137,7 +229,14 @@ $addtest->('Test pool members', {
 
 $addtest->('Test pool members with node limit', {
     expected => {
-	node2 => [ 200, 201 ],
+	200 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
     },
     param => {
 	pool => 'testpool',
@@ -147,8 +246,18 @@ $addtest->('Test pool members with node limit', {
 
 $addtest->('Test selected VMIDs', {
     expected => {
-	node1 =>[ 100 ],
-	node2 => [ 201, 212 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
     },
     param => {
 	vmid => '100, 201, 212',
@@ -157,7 +266,10 @@ $addtest->('Test selected VMIDs', {
 
 $addtest->('Test selected VMIDs with node limit', {
     expected => {
-	node1 =>[ 100 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
     },
     param => {
 	vmid => '100, 201, 212',
@@ -176,9 +288,19 @@ $addtest->('Test selected VMIDs on other nodes', {
 
 $addtest->('Test VMID not present in vmlist', {
     expected => {
-	node1 => [ 100 ],
-	node2 => [ 201, 212 ],
-	'' => [ 7654 ],
+	100 => {
+	    'type' => 'qemu',
+	    'node' => 'node1',
+	},
+	201 => {
+	    'type' => 'qemu',
+	    'node' => 'node2',
+	},
+	212 => {
+	    'type' => 'lxc',
+	    'node' => 'node2',
+	},
+	7654 => {},
    },
     param => {
 	vmid => '100, 201, 212, 7654',
-- 
2.20.1