[pve-devel] [PATCH qemu-server] fix: also add resource mappings that do not exist to missing mappings

[pve-devel] [PATCH qemu-server] fix: also add resource mappings that do not exist to missing mappings

[Markus Frank]

Currently you get a "Can't use an undefined value..." error because all
get_node_mapping functions return undefined if there is no resource
mapping entry for the id.

Fixes: a52eb3c4e ("check local resources: extend for mapped resources")
Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
This patch supersedes "fix: print error message that the resource
mapping entry does not exist"

 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index ccdceedc..2790942c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2512,7 +2512,7 @@ sub check_local_resources {
 	    } elsif ($type eq 'dir') {
 		$entry = PVE::Mapping::Dir::get_node_mapping($dir_map, $id, $node);
 	    }
-	    if (!scalar($entry->@*)) {
+	    if (!$entry || !scalar($entry->@*)) {
 		push @{$missing_mappings_by_node->{$node}}, $key;
 	    }
 	}
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix: also add resource mappings that do not exist to missing mappings

[Fiona Ebner]

Am 10.04.25 um 12:59 schrieb Markus Frank:
> Currently you get a "Can't use an undefined value..." error because all
> get_node_mapping functions return undefined if there is no resource
> mapping entry for the id.
> 
> Fixes: a52eb3c4e ("check local resources: extend for mapped resources")
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> This patch supersedes "fix: print error message that the resource
> mapping entry does not exist"
> 
>  PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index ccdceedc..2790942c 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2512,7 +2512,7 @@ sub check_local_resources {
>  	    } elsif ($type eq 'dir') {
>  		$entry = PVE::Mapping::Dir::get_node_mapping($dir_map, $id, $node);
>  	    }
> -	    if (!scalar($entry->@*)) {
> +	    if (!$entry || !scalar($entry->@*)) {
>  		push @{$missing_mappings_by_node->{$node}}, $key;
>  	    }
>  	}

Looking at this more closely, I think v1 is better actually. Sorry, I
had misunderstood the exact situation when the issue happens.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix: also add resource mappings that do not exist to missing mappings

[Markus Frank via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4701 bytes --]

From: Markus Frank <m.frank@proxmox.com>
To: Fiona Ebner <f.ebner@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server] fix: also add resource mappings that do not exist to missing mappings
Date: Thu, 10 Apr 2025 14:14:43 +0200
Message-ID: <546be58f-8c82-435d-89fa-79cc0c278991@proxmox.com>



On  2025-04-10 13:18, Fiona Ebner wrote:
> Am 10.04.25 um 12:59 schrieb Markus Frank:
>> Currently you get a "Can't use an undefined value..." error because all
>> get_node_mapping functions return undefined if there is no resource
>> mapping entry for the id.
>>
>> Fixes: a52eb3c4e ("check local resources: extend for mapped resources")
>> Signed-off-by: Markus Frank <m.frank@proxmox.com>
>> ---
>> This patch supersedes "fix: print error message that the resource
>> mapping entry does not exist"
>>
>>   PVE/QemuServer.pm | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index ccdceedc..2790942c 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -2512,7 +2512,7 @@ sub check_local_resources {
>>   	    } elsif ($type eq 'dir') {
>>   		$entry = PVE::Mapping::Dir::get_node_mapping($dir_map, $id, $node);
>>   	    }
>> -	    if (!scalar($entry->@*)) {
>> +	    if (!$entry || !scalar($entry->@*)) {
>>   		push @{$missing_mappings_by_node->{$node}}, $key;
>>   	    }
>>   	}
> 
> Looking at this more closely, I think v1 is better actually. Sorry, I
> had misunderstood the exact situation when the issue happens.

I think this patch is actually better (especially for the WebUI).

When I use die, the error is printed in an error window, but the migration window is still open and I can start the migration (which immediately stops and shows the same error again).

With this patch, in the migration window, the key is printed as a mapped resource that is not available on the selected target, which is actually correct (because it is not defined for any node). And I cannot start the migration process.




[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server] fix: also add resource mappings that do not exist to missing mappings

[Fiona Ebner]

Am 10.04.25 um 14:14 schrieb Markus Frank:
> On  2025-04-10 13:18, Fiona Ebner wrote:
>> Am 10.04.25 um 12:59 schrieb Markus Frank:
>>> Currently you get a "Can't use an undefined value..." error because all
>>> get_node_mapping functions return undefined if there is no resource
>>> mapping entry for the id.
>>>
>>> Fixes: a52eb3c4e ("check local resources: extend for mapped resources")
>>> Signed-off-by: Markus Frank <m.frank@proxmox.com>
>>> ---
>>> This patch supersedes "fix: print error message that the resource
>>> mapping entry does not exist"
>>>
>>>   PVE/QemuServer.pm | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index ccdceedc..2790942c 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -2512,7 +2512,7 @@ sub check_local_resources {
>>>           } elsif ($type eq 'dir') {
>>>           $entry = PVE::Mapping::Dir::get_node_mapping($dir_map, $id,
>>> $node);
>>>           }
>>> -        if (!scalar($entry->@*)) {
>>> +        if (!$entry || !scalar($entry->@*)) {
>>>           push @{$missing_mappings_by_node->{$node}}, $key;
>>>           }
>>>       }
>>
>> Looking at this more closely, I think v1 is better actually. Sorry, I
>> had misunderstood the exact situation when the issue happens.
> 
> I think this patch is actually better (especially for the WebUI).
> 
> When I use die, the error is printed in an error window, but the
> migration window is still open and I can start the migration (which
> immediately stops and shows the same error again).

This is a bit orthogonal IMHO. The question is, should we even show the
migration window if the precondition API call fails? We cannot show the
correct information then and migration will also just fail (except if
there is a bug where migration would not catch the issue, but the
precondition check does, but that would need to be fixed).

> With this patch, in the migration window, the key is printed as a mapped
> resource that is not available on the selected target, which is actually
> correct (because it is not defined for any node). And I cannot start the
> migration process.

Well, the key would also be printed in the other case if you adapt the
error message to print it there :P

I do think the distinction between "not defined at all" and "missing for
a specific node" is worth having. The "not defined at all" case is a
fundamental configuration issue, while the other can be perfectly valid
(just preventing ability to migrate). You cannot start the VM either in
the "not defined at all" case, so IMHO it does warrant a nice error pop-up.

Just my opinion :)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel