Re: [PATCH guest-common v7 05/24] add module to track previously used guest IDs

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

Am 05.10.26 um 4:50 PM schrieb Michael Köppl:
> +my sub next_unused($ranges, $id) {
> +    my $next = $id;
> +    for my $range ($ranges->@*) {
> +        my ($start, $end) = $range->@*;
> +        next if $end < $next;
> +        last if $next < $start;
> +        $next = $end + 1;
> +    }
> +    return $next;
> +}
> +

---snip 8<---

> +}
> +
> +# Returns the lowest ID at or above $id that is neither taken by an
> +# existing guest nor recorded as used before.
> +sub get_next_unused_id($id) {
> +    my $ranges = cfs_read_file($FILENAME);
> +    my $vmlist = PVE::Cluster::get_vmlist() // {};
> +    my $existing = $vmlist->{ids} // {};
> +
> +    $id = next_unused($ranges, $id);
> +    while (defined($existing->{$id})) {
> +        $id = next_unused($ranges, $id + 1);

This one was found by our LLM auto-review:

"Consider checking the scaling of get_next_unused_id() and retaining a
range cursor: each existing guest whose ID was never recorded causes
next_unused() to restart traversal of the historical ranges. This makes
a request O(historical ranges × consecutive existing guests).

For example, 100,000 isolated recorded IDs below lower=500000 fit below
the file-size cap. If 10,000 pre-tracking guests occupy IDs
500000–509999, a single /cluster/nextid request performs roughly one
billion range iterations, potentially tying up its API worker."

I think we can just pass along $existing and check within the
next_unused() function. Or even inline next_unused() if we switch
assert_id_satisfies_next_id_settings() to use get_next_unused_id(),
maybe with a param to ignore the existing guests, since that is already
done on the call site, i.e. the nextid endpoint. But also wouldn't be
expensive to consider them.