Re: [pve-devel] [PATCH manager 1/4] API2Tools: rrd: remove O(n^2) lookup for keys

[Dominik Csapak <d.csapak@proxmox.com>]

On 9/5/25 3:49 PM, Aaron Lauterer wrote:
> 
> 
> On  2025-09-05  14:06, Dominik Csapak wrote:
>> the idea was that we get any of the 'new' versions on lookup, but that
>> lead to iterating through possibly all keys. Since that was called for
>> each resource in e.g. /cluster/resources api call, the runtime was
>> O(n^2) for the number of resources.
>>
>> To avoid that, simply look up the currently only valid key here which
>> makes this lookup much cheaper.
>>
>> In my test setup with ~10000 guests, it reduces the time for a call
>> to /cluster/resources from ~22s to ~400ms
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> NOTE: this is only a workaround and Aaron is working to making this
>> whole part unnecessary, but for now i think this is a good stop-gap
>>
>>   PVE/API2Tools.pm | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/API2Tools.pm b/PVE/API2Tools.pm
>> index abe73fa2..863f5f55 100644
>> --- a/PVE/API2Tools.pm
>> +++ b/PVE/API2Tools.pm
>> @@ -52,10 +52,9 @@ sub get_rrd_key {
>>           return "pve2.3-${type}/${id}";
>>       }
>> -    # if no old key has been found, we expect on in the newer format: 
>> pve-{type}-{version}/{id}
>> -    # We accept all new versions, as the expectation is that they are 
>> only allowed to add new colums as non-breaking change
>> -    for my $k (keys %$rrd) {
>> -        return $k if $k =~ m/^pve-\Q${type}\E-\d\d?.\d\/\Q${id}\E$/;
>> +    my $key = "pve-${type}-9.0/${id}";
>> +    if (defined($rrd->{$key})) {
>> +        return $key;
> 
> Have you tested this in a mixed PVE8 + PVE9 cluster? This might break it 
> and we might have to keep the dynamic checking around.
> 
> I don't have too much time right now. But I think this would break a 
> mixed version situation, e.g. during a cluster upgrade, where we receive 
> both keys, pve2-... and pve-...-9.0 ones.
> 
>>       }
>>   }
> 

but isn't that why the checks above (outside the context) are there?:

---
  # check for old formats: pve2-{type}/{id}. For VMs and CTs the version 
number is different than for nodes and storages
  if ($type ne "vm" && exists $rrd->{"pve2-${type}/${id}"}) {
      return "pve2-${type}/${id}";
  } elsif ($type eq "vm" && exists $rrd->{"pve2.3-${type}/${id}"}) {
      return "pve2.3-${type}/${id}";
  }
---

?


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