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

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

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);
>>   
>