From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 3BBF01FF09B for ; Mon, 12 Oct 2026 15:22:32 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9255621586; Mon, 12 Oct 2026 15:22:29 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 12 Oct 2026 15:16:58 +0200 Message-Id: To: "Fiona Ebner" , =?utf-8?q?Michael_K=C3=B6ppl?= , Subject: Re: [PATCH guest-common v7 05/24] add module to track previously used guest IDs From: =?utf-8?q?Michael_K=C3=B6ppl?= X-Mailer: aerc 0.22.0 References: <20261005144805.825538-1-m.koeppl@proxmox.com> <20261005144805.825538-6-m.koeppl@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791811018675 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.146 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: A3RCHLHHV5JLGCGVOVHK2GLXBAGJFTYW X-Message-ID-Hash: A3RCHLHHV5JLGCGVOVHK2GLXBAGJFTYW X-MailFrom: m.koeppl@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri Oct 9, 2026 at 6:22 PM CEST, Fiona Ebner wrote: [snip] >> +# 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 =3D cfs_read_file($FILENAME); >> + my $vmlist =3D PVE::Cluster::get_vmlist() // {}; >> + my $existing =3D $vmlist->{ids} // {}; >> + >> + $id =3D next_unused($ranges, $id); >> + while (defined($existing->{$id})) { >> + $id =3D 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 =C3=97 consecutive existing guests). > > For example, 100,000 isolated recorded IDs below lower=3D500000 fit below > the file-size cap. If 10,000 pre-tracking guests occupy IDs > 500000=E2=80=93509999, 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. This is a good catch, thanks for pointing it out. I'd go with inlining next_unused such that the current position in the sorted ranges is kept across iterations instead of starting over for every existing guest. I'd skip the param to ignore existing guests, though, since, as you said, the nextid endpoint is already checking for that and the lookup itself is cheap.