From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; Thu, 22 Oct 2020 12:30:30 +0200 (CEST) From: Fabian Ebner 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 --- 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