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 F20591FF0AA for ; Tue, 22 Sep 2026 16:22:37 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 633212159C; Tue, 22 Sep 2026 16:22:34 +0200 (CEST) Message-ID: <0ee919d3-7ff6-4905-ae82-c348f8445037@proxmox.com> Date: Tue, 22 Sep 2026 16:22:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH pve-guest-common v5 3/7] add module to track previously used guest IDs To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , pve-devel@lists.proxmox.com References: <20260921155410.938337-1-m.koeppl@proxmox.com> <20260921155410.938337-4-m.koeppl@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260921155410.938337-4-m.koeppl@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790086944638 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.384 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: E5FDOAB5QYLRQYTISHBHVROHNMNSKCS3 X-Message-ID-Hash: E5FDOAB5QYLRQYTISHBHVROHNMNSKCS3 X-MailFrom: f.ebner@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: Am 21.09.26 um 5:54 PM schrieb Michael Köppl: > The /cluster/nextid API endpoint always suggests the lowest free > guest ID, so IDs of destroyed guests get handed out again. Offering > an opt-out requires recording every ID that has ever been in use > somewhere both the endpoint and the guest create and destroy paths > can reach. > > Store them in /etc/pve/used-guest-ids and register it as a cfs file, > so the list is kept in sync cluster-wide. Consecutive IDs are > collapsed into "-" ranges on write to keep the file small > on clusters that churn through many guests, and expanded back into a > hash on read. Entries that do not parse, ranges with reversed bounds > included, are skipped with a warning instead of failing the whole > read. > > Originally-by: Daniel Krambrock > Originally-by: Severen Redwood > Signed-off-by: Michael Köppl > --- > src/Makefile | 1 + > src/PVE/UsedGuestIDs.pm | 93 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 94 insertions(+) > create mode 100644 src/PVE/UsedGuestIDs.pm > > diff --git a/src/Makefile b/src/Makefile > index 030e7f7..83e5ceb 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -14,6 +14,7 @@ install: PVE > install -m 0644 PVE/Replication.pm ${PERL5DIR}/PVE/ > install -m 0644 PVE/StorageTunnel.pm ${PERL5DIR}/PVE/ > install -m 0644 PVE/Tunnel.pm ${PERL5DIR}/PVE/ > + install -m 0644 PVE/UsedGuestIDs.pm ${PERL5DIR}/PVE/ > install -d ${PERL5DIR}/PVE/Mapping > install -m 0644 PVE/Mapping/Dir.pm ${PERL5DIR}/PVE/Mapping/ > install -m 0644 PVE/Mapping/PCI.pm ${PERL5DIR}/PVE/Mapping/ > diff --git a/src/PVE/UsedGuestIDs.pm b/src/PVE/UsedGuestIDs.pm > new file mode 100644 > index 0000000..31036ff > --- /dev/null > +++ b/src/PVE/UsedGuestIDs.pm > @@ -0,0 +1,93 @@ > +package PVE::UsedGuestIDs; Nit: maybe GuestID as a more general name? That would suggest moving or adding other stuff related to guest IDs here in the future. Otherwise, the module is quite specific. We would need to rename add_id() to add_used_id(), read_list() -> list_used_ids() for clarity then. > + > +use strict; > +use warnings; Nit: I now this got picked up from before, but it would be nice to use v5.36; and function signatures for new modules. > + > +use PVE::Cluster qw( > + cfs_lock_file > + cfs_read_file > + cfs_register_file > + cfs_write_file > +); > + > +my $FILENAME = 'used-guest-ids'; > + > +my $parse_id_list = sub { Nit: If writing with a signature, use 'my sub', and similarly for the other two private functions. > + my ($filename, $raw) = @_; > + > + my $used_ids = {}; > + > + return $used_ids if !defined($raw); > + > + for my $line (split(/\n/, $raw)) { > + next if $line =~ m/^\s*$/; > + > + if ($line =~ m/^(\d+)$/) { > + $used_ids->{$1} = 1; > + } elsif ($line =~ m/^(\d+)-(\d+)$/) { > + my ($start, $end) = ($1, $2); > + if ($start > $end) { > + warn "skipping reversed range in $filename: $line\n"; > + next; > + } > + $used_ids->{$_} = 1 for $start .. $end; Nit: we could be smarter and avoid adding every single ID to the hash (with a lot of (past) guests that can become a non-trivial cost), and instead track ranges. Instead of having a public read_list() function then, there could be a check_id_unused() function. When writing out the file we can still merge adjacent ranges. > + } else { > + warn "skipping invalid entry in $filename: $line\n"; > + } > + } > + > + return $used_ids; > +}; > + > +my $format_entry = sub { > + my ($start, $last) = @_; > + return $start == $last ? "$start\n" : "$start-$last\n"; > +}; > + > +my $write_id_list = sub { > + my ($filename, $used_ids) = @_; > + > + my $output = ''; > + my ($start, $last); > + > + for my $curr (sort { $a <=> $b } keys $used_ids->%*) { > + if (!defined($start)) { > + ($start, $last) = ($curr, $curr); > + } elsif ($last + 1 == $curr) { > + $last = $curr; > + } else { > + $output .= $format_entry->($start, $last); > + ($start, $last) = ($curr, $curr); > + } > + } > + > + $output .= $format_entry->($start, $last) if defined($start); > + > + return $output; > +}; > + > +sub read_list { > + return cfs_read_file($FILENAME); > +} > + > +sub add_id { > + my ($id) = @_; > + > + cfs_lock_file( > + $FILENAME, > + 10, > + sub { > + my $used_ids = cfs_read_file($FILENAME); > + > + return if $used_ids->{$id}; > + > + $used_ids->{$id} = 1; > + cfs_write_file($FILENAME, $used_ids); > + }, > + ); > + die $@ if $@; > +} > + > +cfs_register_file($FILENAME, $parse_id_list, $write_id_list); > + > +1;