From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 85DEE61C2E
 for <pve-devel@lists.proxmox.com>; Thu, 22 Oct 2020 12:31:03 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 4750D1F07A
 for <pve-devel@lists.proxmox.com>; Thu, 22 Oct 2020 12:30:33 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS id 5509B1EFD9
 for <pve-devel@lists.proxmox.com>; Thu, 22 Oct 2020 12:30:30 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 22E0145ABA
 for <pve-devel@lists.proxmox.com>; Thu, 22 Oct 2020 12:30:30 +0200 (CEST)
From: Fabian Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Thu, 22 Oct 2020 12:30:17 +0200
Message-Id: <20201022103017.19715-9-f.ebner@proxmox.com>
X-Mailer: git-send-email 2.20.1
In-Reply-To: <20201022103017.19715-1-f.ebner@proxmox.com>
References: <20201022103017.19715-1-f.ebner@proxmox.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.407 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [backup.pm, backupinfo.pm, vzdump.pm]
 URIBL_SBL 0.644 Contains an URL's NS IP listed in the Spamhaus SBL blocklist
 [backup.pm]
 URIBL_SBL_A 0.1 Contains URL's A record listed in the Spamhaus SBL blocklist
 [backup.pm]
Subject: [pve-devel] [RFC/PATCH v2 manager 8/8] don't group by node in
 get_included_guests
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 22 Oct 2020 10:31:03 -0000

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