[pve-devel] [PATCH manager] get_included_guests: handle non-existing guests

[pve-devel] [PATCH manager] get_included_guests: handle non-existing guests

[Fabian Ebner]

If a guest is removed without purge, the ID will remain
in the backup configuration. Avoid using the variable $node
when it is potentially undefined. Instead, skip non-existing
guests and warn the user.

Reported here:
https://forum.proxmox.com/threads/purge-backup-does-not-remove-vm-from-datacenter-backup-list.77609/

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm                      |  5 +++++
 test/vzdump_guest_included_test.pl | 12 +++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 542228d6..6dbb6a44 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1217,6 +1217,11 @@ sub get_included_guests {
     $vmids = check_vmids(@$vmids);
 
     for my $vmid (@$vmids) {
+	if (!defined($vmlist->{ids}->{$vmid})) {
+	    debugmsg('warn', "no guest with ID '$vmid' exists in the cluster!");
+	    next;
+	}
+
 	my $node = $vmlist->{ids}->{$vmid}->{node};
 	next if (defined $job->{node} && $job->{node} ne $node);
 
diff --git a/test/vzdump_guest_included_test.pl b/test/vzdump_guest_included_test.pl
index a0f40a55..0494d40e 100755
--- a/test/vzdump_guest_included_test.pl
+++ b/test/vzdump_guest_included_test.pl
@@ -9,7 +9,7 @@ use warnings;
 
 use lib '..';
 
-use Test::More tests => 9;
+use Test::More tests => 10;
 use Test::MockModule;
 
 use PVE::VZDump;
@@ -178,6 +178,16 @@ $addtest->('Test selected VMIDs on other nodes', {
     }
 });
 
+$addtest->('Test VMID not present in vmlist', {
+    expected => {
+	node1 => [ 100 ],
+	node2 => [ 201, 212 ],
+    },
+    param => {
+	vmid => '100, 201, 212, 7654',
+    }
+});
+
 
 for my $test (@{$tests}) {
     my $testname = $test->{name};
-- 
2.20.1

Re: [pve-devel] [PATCH manager] get_included_guests: handle non-existing guests

[Thomas Lamprecht]

(FYI: forgot to hit reply-all, so resending this for the list)

On 19.10.20 12:53, Fabian Ebner wrote:
> If a guest is removed without purge, the ID will remain
> in the backup configuration. Avoid using the variable $node
> when it is potentially undefined. Instead, skip non-existing
> guests and warn the user.
>
> Reported here:
> https://forum.proxmox.com/threads/purge-backup-does-not-remove-vm-from-datacenter-backup-list.77609/

a backup job referencing to an non-existent VM must fail, that's by design.
People should either use purge, if they really want to purge a VM, or else
remove it manually from the job.

It's just important that the backup of the remaining, existing, VMs is made
nonetheless, but the job is not successful, as it was asked to backup
something that does not exists - making such errors less prominent is not
ideal.

>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/VZDump.pm                      |  5 +++++
>  test/vzdump_guest_included_test.pl | 12 +++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 542228d6..6dbb6a44 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -1217,6 +1217,11 @@ sub get_included_guests {
>      $vmids = check_vmids(@$vmids);
>  
>      for my $vmid (@$vmids) {
> +	if (!defined($vmlist->{ids}->{$vmid})) {
> +	    debugmsg('warn', "no guest with ID '$vmid' exists in the cluster!");
> +	    next;
> +	}
> +
>  	my $node = $vmlist->{ids}->{$vmid}->{node};
>  	next if (defined $job->{node} && $job->{node} ne $node);
>  

Re: [pve-devel] [PATCH manager] get_included_guests: handle non-existing guests

[Fabian Ebner]

On 10/19/20 2:38 PM, Thomas Lamprecht wrote:
> (FYI: forgot to hit reply-all, so resending this for the list)
> 
> On 19.10.20 12:53, Fabian Ebner wrote:
>> If a guest is removed without purge, the ID will remain
>> in the backup configuration. Avoid using the variable $node
>> when it is potentially undefined. Instead, skip non-existing
>> guests and warn the user.
>>
>> Reported here:
>> https://forum.proxmox.com/threads/purge-backup-does-not-remove-vm-from-datacenter-backup-list.77609/
> 
> a backup job referencing to an non-existent VM must fail, that's by design.

Well, it currently doesn't; in fact no task is being run for such guests 
(123 doesn't exists, 124 is on a different node in this example):

root@rob1 ~ # vzdump 121 123 124 --storage myfs --mode snapshot --remove 
1
Use of uninitialized value $node in hash element at 
/usr/share/perl5/PVE/VZDump.pm line 1221.
INFO: starting new backup job: vzdump 121 123 124 --storage myfs --mode 
snapshot --remove 1
INFO: skip external VMs: 124, 123
INFO: Starting Backup of VM 121 (qemu)
---8<---

The reason is that a missing guest ID is assigned to the $vmids_per_node 
hash with key equal to undef, which causes the hash to look like:
$VAR1 = {
           '' => [
                   123
                 ],
           'rob2' => [
                       124
                     ],
           'rob1' => [
                       121
                     ]
         };
and then back in the vzdump API call, it lands in the $skiplist. So 
there is no backup task run for missing guest IDs.

> People should either use purge, if they really want to purge a VM, or else
> remove it manually from the job.
> 
> It's just important that the backup of the remaining, existing, VMs is made
> nonetheless, but the job is not successful, as it was asked to backup
> something that does not exists - making such errors less prominent is not
> ideal.
> 

With the patch we at least get a warning with the real problem and avoid 
using an undefined hash key. But sure, I'll work out a v2 where there is 
a failing backup task for missing guests.

>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>   PVE/VZDump.pm                      |  5 +++++
>>   test/vzdump_guest_included_test.pl | 12 +++++++++++-
>>   2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index 542228d6..6dbb6a44 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>> @@ -1217,6 +1217,11 @@ sub get_included_guests {
>>       $vmids = check_vmids(@$vmids);
>>   
>>       for my $vmid (@$vmids) {
>> +	if (!defined($vmlist->{ids}->{$vmid})) {
>> +	    debugmsg('warn', "no guest with ID '$vmid' exists in the cluster!");
>> +	    next;
>> +	}
>> +
>>   	my $node = $vmlist->{ids}->{$vmid}->{node};
>>   	next if (defined $job->{node} && $job->{node} ne $node);
>>   
>