[PATCH many v5 0/7] add option to prevent suggesting previously used VMIDs

[PATCH many v5 0/7] add option to prevent suggesting previously used VMIDs

[Michael Köppl]

This is based on the original series [0] by Severen Redwood and Daniel
Krambrock. It it rebased on the latest master branches and incorporates
the feedback from v4.

I kept the trailers from the original series where I made only minimal
changes, but added markers (MK: ...) in those cases. For bigger
changes, I changed the `Co-authored-by` and `Signed-off-by` trailers to
`Signed-off-by`. I removed Aaron's T-b and R-b trailers since it's been
a long time and the trailers might be misleading considering the series
has changed in some of its implementation details overall.

Changes since v4:
- File in pmxcfs has been renamed to used-guest-ids to make it a bit
  more general (both VMs and CTs) while still making clear which IDs
  are meant.
- Moved the `UsedVmidList` module from pve-manager to pve-guest-common
  and renamed it `UsedGuestIDs`. Also renamed some of the functions
  there for more consistent naming.
- Implemented `write_id_list` more closely aligned with Fabian's
  feedback.
- Also record used VMIDs when creating, cloning, destroying guests as
  well as incoming remote migrations. (Thanks, @Fiona, for the
  off-list input and feedback!)
- Moved the `add_id` calls for recording used IDs to before anything
  is persisted during any of the above actions.
- Rephrased some commit messages.

[0] https://lore.proxmox.com/pve-devel/mailman.62.1731030290.372.pve-devel@lists.proxmox.com/

pve-cluster:

Severen Redwood (2):
  cluster files: add used-guest-ids
  datacenter config: add unique-next-id to schema

 src/PVE/Cluster.pm          | 1 +
 src/PVE/DataCenterConfig.pm | 6 ++++++
 src/pmxcfs/status.c         | 1 +
 3 files changed, 8 insertions(+)


pve-guest-common:

Michael Köppl (1):
  add module to track previously used guest IDs

 src/Makefile            |  1 +
 src/PVE/UsedGuestIDs.pm | 93 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100644 src/PVE/UsedGuestIDs.pm


qemu-server:

Michael Köppl (1):
  api: record VM ID as used on creation, cloning, and destruction

 src/PVE/API2/Qemu.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)


pve-container:

Michael Köppl (1):
  api: record CT ID as used on creation, cloning, and destruction

 src/PVE/API2/LXC.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)


pve-manager:

Michael Köppl (1):
  close #4369: api: optionally only suggest unique IDs

Severen Redwood (1):
  close #4369: ui: add datacenter option for unique VM/CT IDs

 PVE/API2/Cluster.pm           | 9 ++++++++-
 www/manager6/dc/OptionView.js | 4 ++++
 2 files changed, 12 insertions(+), 1 deletion(-)


Summary over all repositories:
  9 files changed, 150 insertions(+), 1 deletions(-)

-- 
Generated by murpp 0.12.1

[PATCH pve-cluster v5 1/7] cluster files: add used-guest-ids

[Michael Köppl]

From: Severen Redwood <severen.redwood@sitehost.co.nz>

Add `/etc/pve/used-guest-ids` to the list of cluster files, which will
be used for recording previously used VM/CT IDs. This is required so
that it can optionally be ensured that such IDs are not suggested.

Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
[MK: change name of file to used-guest-ids
     adapt commit message]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/Cluster.pm  | 1 +
 src/pmxcfs/status.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
index 034b78c..aa6e93d 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -92,6 +92,7 @@ my $observed = {
     'mapping/directory.cfg' => 1,
     'mapping/pci.cfg' => 1,
     'mapping/usb.cfg' => 1,
+    'used-guest-ids' => 1,
 };
 
 sub prepare_observed_file_basedirs {
diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index 12a6c46..1b80c85 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -120,6 +120,7 @@ static memdb_change_t memdb_change_array[] = {
     {.path = "mapping/directory.cfg"},
     {.path = "mapping/pci.cfg"},
     {.path = "mapping/usb.cfg"},
+    {.path = "used-guest-ids"},
 };
 
 static GMutex mutex;
-- 
2.47.3

[PATCH pve-cluster v5 2/7] datacenter config: add unique-next-id to schema

[Michael Köppl]

From: Severen Redwood <severen.redwood@sitehost.co.nz>

Add the `unique-next-id` property to the datacenter config schema to
track whether only unique (ie. neither currently nor previously in use)
VM/CT IDs should be suggested by the `/cluster/nextid` API endpoint.

Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
[MK: use datacenter in commit message instead of datacentre]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/DataCenterConfig.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index 004122e..54f9c10 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -415,6 +415,12 @@ my $datacenter_schema = {
             format => $next_id_format,
             description => "Control the range for the free VMID auto-selection pool.",
         },
+        'unique-next-id' => {
+            optional => 1,
+            type => 'boolean',
+            description =>
+                "Only suggest VMIDs that are neither currently in use nor have previously been used.",
+        },
         migration => {
             optional => 1,
             type => 'string',
-- 
2.47.3

[PATCH pve-guest-common v5 3/7] add module to track previously used guest IDs

[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 "<start>-<end>" 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 <krambrock@hrz.uni-marburg.de>
Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 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;
+
+use strict;
+use warnings;
+
+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 {
+    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;
+        } 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;
-- 
2.47.3

[PATCH qemu-server v5 4/7] api: record VM ID as used on creation, cloning, and destruction

[Michael Köppl]

Record an ID as used when a VM is created, cloned, destroyed, or brought
in by a remote migration, so that the `/cluster/nextid` endpoint can
later optionally avoid suggesting previously used IDs.

Recording is done on creation as well such that the ID stays recorded
even when a user removes the config by hand, which bypasses the destroy
API.

Each call happens before the first persistent change and a failure to
record aborts the operation. That way a VM is never created or destroyed
without its ID being recorded and a failure leaves nothing half-done
behind.

Originally-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 71247eec..c2571f0c 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -68,6 +68,7 @@ use PVE::ReplicationState;
 use PVE::StorageTunnel;
 use PVE::RESTEnvironment qw(log_warn);
 use PVE::Ticket;
+use PVE::UsedGuestIDs;
 
 BEGIN {
     if (!$ENV{PVE_GENERATING_DOCS}) {
@@ -1371,6 +1372,10 @@ __PACKAGE__->register_method({
 
         my $emsg = $is_restore ? "unable to restore VM $vmid -" : "unable to create VM $vmid -";
 
+        # record before the config exists, so a failure here leaves nothing behind
+        eval { PVE::UsedGuestIDs::add_id($vmid) };
+        die "$emsg could not record VMID as used - $@" if $@;
+
         eval { PVE::QemuConfig->create_and_lock_config($vmid, $force) };
         die "$emsg $@" if $@;
 
@@ -2847,6 +2852,10 @@ __PACKAGE__->register_method({
 
         $early_checks->();
 
+        # record before destroying anything, so a failure here leaves the VM intact
+        eval { PVE::UsedGuestIDs::add_id($vmid) };
+        die "unable to destroy VM $vmid - could not record VMID as used - $@" if $@;
+
         my $realcmd = sub {
             my $upid = shift;
 
@@ -4610,6 +4619,10 @@ __PACKAGE__->register_method({
                 $newconf->{description} = $param->{description};
             }
 
+            # record before the config exists, so a failure here leaves nothing behind
+            eval { PVE::UsedGuestIDs::add_id($newid) };
+            die "unable to clone VM $vmid - could not record VMID $newid as used - $@" if $@;
+
             # create empty/temp config - this fails if VM already exists on other node
             # FIXME use PVE::QemuConfig->create_and_lock_config and adapt code
             PVE::Tools::file_set_contents($conffile, "# qmclone temporary file\nlock: clone\n");
@@ -6708,6 +6721,11 @@ __PACKAGE__->register_method({
 
         PVE::Cluster::check_cfs_quorum();
 
+        # remote migration brings in a VMID this cluster has not seen,
+        # so record it here
+        eval { PVE::UsedGuestIDs::add_id($vmid) };
+        die "unable to create VM $vmid - could not record VMID as used - $@" if $@;
+
         my $lock = 'create';
         eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, $lock); };
 
-- 
2.47.3

[PATCH pve-container v5 5/7] api: record CT ID as used on creation, cloning, and destruction

[Michael Köppl]

Record an ID as used when a container is created, cloned, destroyed, or
brought in by a remote migration, so that the `/cluster/nextid` endpoint
can later optionally avoid suggesting previously used IDs.

Recording is done on creation as well such that the ID stays recorded
even when a user removes the config by hand, which bypasses the destroy
API.

Each call happens before the first persistent change and a failure to
record aborts the operation. That way a CT is never created or destroyed
without its ID being recorded and a failure leaves nothing half-done
behind.

Originally-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/API2/LXC.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 5f94d5a1..73c06fbd 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -26,6 +26,7 @@ use PVE::SafeSyslog;
 use PVE::Storage;
 use PVE::Ticket;
 use PVE::Tools qw(extract_param run_command);
+use PVE::UsedGuestIDs;
 use PVE::VZDump::Plugin;
 
 use PVE::LXC;
@@ -422,6 +423,10 @@ __PACKAGE__->register_method({
 
         my $emsg = $restore ? "unable to restore CT $vmid -" : "unable to create CT $vmid -";
 
+        # record before the config exists, so a failure here leaves nothing behind
+        eval { PVE::UsedGuestIDs::add_id($vmid) };
+        die "$emsg could not record VMID as used - $@" if $@;
+
         eval { PVE::LXC::Config->create_and_lock_config($vmid, $force) };
         die "$emsg $@" if $@;
 
@@ -882,6 +887,10 @@ __PACKAGE__->register_method({
 
         $early_checks->($conf);
 
+        # record before destroying anything, so a failure here leaves the CT intact
+        eval { PVE::UsedGuestIDs::add_id($vmid) };
+        die "unable to destroy CT $vmid - could not record VMID as used - $@" if $@;
+
         my $running_error_msg = "unable to destroy CT $vmid - container is running\n";
         die $running_error_msg if !$param->{force} && PVE::LXC::check_running($vmid); # check early
 
@@ -2038,6 +2047,10 @@ __PACKAGE__->register_method({
             );
         };
 
+        # record before the config exists, so a failure here leaves nothing behind
+        eval { PVE::UsedGuestIDs::add_id($newid) };
+        die "unable to clone CT $vmid - could not record VMID $newid as used - $@" if $@;
+
         my $src_conf = PVE::LXC::Config->set_lock($vmid, 'disk');
 
         eval { PVE::LXC::Config->create_and_lock_config($newid, 0); };
@@ -3148,6 +3161,11 @@ __PACKAGE__->register_method({
 
         PVE::Cluster::check_cfs_quorum();
 
+        # remote migration brings in a VMID this cluster has not seen,
+        # so record it here
+        eval { PVE::UsedGuestIDs::add_id($vmid) };
+        die "unable to create CT $vmid - could not record VMID as used - $@" if $@;
+
         my $socket_addr = "/run/pve/ct-$vmid.mtunnel";
 
         my $lock = 'create';
-- 
2.47.3

[PATCH pve-manager v5 6/7] close #4369: api: optionally only suggest unique IDs

[Michael Köppl]

At the moment, the `/cluster/nextid` API endpoint will return the lowest
available VM/CT ID, which means that it will suggest re-using VM IDs.
This can be undesirable, so add an optional check to ensure that it
chooses an ID which is not and has never been in use.

This optional behaviour is enabled when `unique-next-id: 1` in
the datacenter config, and the previously used IDs are tracked as a
list in the file `/etc/pve/used-guest-ids`.

Originally-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/API2/Cluster.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 4e5efbfd9..8178562a5 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -20,6 +20,7 @@ use PVE::RPCEnvironment;
 use PVE::SafeSyslog;
 use PVE::Storage;
 use PVE::Tools qw(extract_param);
+use PVE::UsedGuestIDs;
 
 use PVE::API2::ACMEAccount;
 use PVE::API2::ACMEPlugin;
@@ -1041,12 +1042,18 @@ __PACKAGE__->register_method({
 
         my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
         my $next_id = $dc_conf->{'next-id'} // {};
+        my $used_ids;
+        if ($dc_conf->{'unique-next-id'}) {
+            $used_ids = PVE::UsedGuestIDs::read_list();
+        } else {
+            $used_ids = {};
+        }
 
         my $lower = $next_id->{lower} // 100;
         my $upper = $next_id->{upper} // (1000 * 1000); # note, lower than the schema-maximum
 
         for (my $i = $lower; $i < $upper; $i++) {
-            return $i if !defined($idlist->{$i});
+            return $i if !defined($idlist->{$i}) and !defined($used_ids->{$i});
         }
 
         die "unable to get any free VMID in range [$lower, $upper]\n";
-- 
2.47.3

[PATCH pve-manager v5 7/7] close #4369: ui: add datacenter option for unique VM/CT IDs

[Michael Köppl]

From: Severen Redwood <severen.redwood@sitehost.co.nz>

Add a 'suggest unique VMIDs' row to the datacenter options page that
allows choosing whether the `/cluster/nextid` API endpoint (and thereby
any UI elements that suggest IDs) should avoid suggesting previously
used IDs. This option defaults to off to ensure that this change in
behaviour is opt in.

Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
[MK: rebase on latest master]
Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/dc/OptionView.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index dc12aa7e1..490df0028 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -384,6 +384,10 @@ Ext.define('PVE.dc.OptionView', {
                 },
             ],
         });
+        me.add_boolean_row('unique-next-id', gettext('Suggest unique VMIDs'), {
+            defaultValue: 0,
+            deleteDefaultValue: true,
+        });
         me.rows['tag-style'] = {
             required: true,
             renderer: (value) => {
-- 
2.47.3

Re: [PATCH pve-cluster v5 1/7] cluster files: add used-guest-ids

[Fiona Ebner]

Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
> From: Severen Redwood <severen.redwood@sitehost.co.nz>
> 
> Add `/etc/pve/used-guest-ids` to the list of cluster files, which will
> be used for recording previously used VM/CT IDs. This is required so
> that it can optionally be ensured that such IDs are not suggested.
> 
> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> [MK: change name of file to used-guest-ids
>      adapt commit message]
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/Cluster.pm  | 1 +
>  src/pmxcfs/status.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/src/PVE/Cluster.pm b/src/PVE/Cluster.pm
> index 034b78c..aa6e93d 100644
> --- a/src/PVE/Cluster.pm
> +++ b/src/PVE/Cluster.pm
> @@ -92,6 +92,7 @@ my $observed = {
>      'mapping/directory.cfg' => 1,
>      'mapping/pci.cfg' => 1,
>      'mapping/usb.cfg' => 1,
> +    'used-guest-ids' => 1,

I'm wondering if we should avoid adding it to the top-level? There is
the virtual-guest directory already, where it would fit.

The name is slightly too much towards virtual machines IMHO, but it
already exists and we do also have the not-just-for-VMs (but
not-yet-in-use) profiles.cfg there:
https://lore.proxmox.com/pve-devel/20231117114548.3208470-2-d.csapak@proxmox.com/

>  };
>  
>  sub prepare_observed_file_basedirs {
> diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
> index 12a6c46..1b80c85 100644
> --- a/src/pmxcfs/status.c
> +++ b/src/pmxcfs/status.c
> @@ -120,6 +120,7 @@ static memdb_change_t memdb_change_array[] = {
>      {.path = "mapping/directory.cfg"},
>      {.path = "mapping/pci.cfg"},
>      {.path = "mapping/usb.cfg"},
> +    {.path = "used-guest-ids"},
>  };
>  
>  static GMutex mutex;

Re: [PATCH pve-guest-common v5 3/7] add module to track previously used guest IDs

[Fiona Ebner]

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 "<start>-<end>" 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 <krambrock@hrz.uni-marburg.de>
> Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  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;

Re: [PATCH pve-cluster v5 2/7] datacenter config: add unique-next-id to schema

[Fiona Ebner]

Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
> From: Severen Redwood <severen.redwood@sitehost.co.nz>
> 
> Add the `unique-next-id` property to the datacenter config schema to
> track whether only unique (ie. neither currently nor previously in use)
> VM/CT IDs should be suggested by the `/cluster/nextid` API endpoint.
> 
> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> [MK: use datacenter in commit message instead of datacentre]
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/DataCenterConfig.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
> index 004122e..54f9c10 100644
> --- a/src/PVE/DataCenterConfig.pm
> +++ b/src/PVE/DataCenterConfig.pm
> @@ -415,6 +415,12 @@ my $datacenter_schema = {
>              format => $next_id_format,
>              description => "Control the range for the free VMID auto-selection pool.",

Should we add this as a sub-property of the next-id property string
instead rather than a separate new option?

>          },
> +        'unique-next-id' => {
> +            optional => 1,
> +            type => 'boolean',
> +            description =>
> +                "Only suggest VMIDs that are neither currently in use nor have previously been used.",

Style nit: line too long
Nit: guest ID instead of VMID

> +        },
>          migration => {
>              optional => 1,
>              type => 'string',

Re: [PATCH pve-manager v5 6/7] close #4369: api: optionally only suggest unique IDs

[Fiona Ebner]

Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
> At the moment, the `/cluster/nextid` API endpoint will return the lowest
> available VM/CT ID, which means that it will suggest re-using VM IDs.
> This can be undesirable, so add an optional check to ensure that it
> chooses an ID which is not and has never been in use.
> 
> This optional behaviour is enabled when `unique-next-id: 1` in
> the datacenter config, and the previously used IDs are tracked as a
> list in the file `/etc/pve/used-guest-ids`.
> 
> Originally-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  PVE/API2/Cluster.pm | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
> index 4e5efbfd9..8178562a5 100644
> --- a/PVE/API2/Cluster.pm
> +++ b/PVE/API2/Cluster.pm
> @@ -20,6 +20,7 @@ use PVE::RPCEnvironment;
>  use PVE::SafeSyslog;
>  use PVE::Storage;
>  use PVE::Tools qw(extract_param);
> +use PVE::UsedGuestIDs;
>  
>  use PVE::API2::ACMEAccount;
>  use PVE::API2::ACMEPlugin;
> @@ -1041,12 +1042,18 @@ __PACKAGE__->register_method({
>  
>          my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>          my $next_id = $dc_conf->{'next-id'} // {};
> +        my $used_ids;
> +        if ($dc_conf->{'unique-next-id'}) {
> +            $used_ids = PVE::UsedGuestIDs::read_list();
> +        } else {
> +            $used_ids = {};
> +        }
>  
>          my $lower = $next_id->{lower} // 100;
>          my $upper = $next_id->{upper} // (1000 * 1000); # note, lower than the schema-maximum
>  
>          for (my $i = $lower; $i < $upper; $i++) {
> -            return $i if !defined($idlist->{$i});
> +            return $i if !defined($idlist->{$i}) and !defined($used_ids->{$i});

Style nit: use '&&' instead of 'and' for logical expressions

>          }
>  
>          die "unable to get any free VMID in range [$lower, $upper]\n";

Re: [PATCH qemu-server v5 4/7] api: record VM ID as used on creation, cloning, and destruction

[Fiona Ebner]

Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
> @@ -6708,6 +6721,11 @@ __PACKAGE__->register_method({
>  
>          PVE::Cluster::check_cfs_quorum();
>  
> +        # remote migration brings in a VMID this cluster has not seen,
> +        # so record it here

Style nit: comment would fit on one line
(same in the next patch)

> +        eval { PVE::UsedGuestIDs::add_id($vmid) };
> +        die "unable to create VM $vmid - could not record VMID as used - $@" if $@;
> +
>          my $lock = 'create';
>          eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, $lock); };
>  

Re: [PATCH pve-cluster v5 2/7] datacenter config: add unique-next-id to schema

[Michael Köppl]

On Tue Sep 22, 2026 at 4:22 PM CEST, Fiona Ebner wrote:
> Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
>> From: Severen Redwood <severen.redwood@sitehost.co.nz>
>>
>> Add the `unique-next-id` property to the datacenter config schema to
>> track whether only unique (ie. neither currently nor previously in use)
>> VM/CT IDs should be suggested by the `/cluster/nextid` API endpoint.
>>
>> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
>> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
>> [MK: use datacenter in commit message instead of datacentre]
>> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
>> ---
>>  src/PVE/DataCenterConfig.pm | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
>> index 004122e..54f9c10 100644
>> --- a/src/PVE/DataCenterConfig.pm
>> +++ b/src/PVE/DataCenterConfig.pm
>> @@ -415,6 +415,12 @@ my $datacenter_schema = {
>>              format => $next_id_format,
>>              description => "Control the range for the free VMID auto-selection pool.",
>
> Should we add this as a sub-property of the next-id property string
> instead rather than a separate new option?

Thanks for having a look! Yeah, I think this would also work better in
the web UI if we also change the option there to "Next Free VMID" and
simply add a "unique" checkbox. Just my 2 cents, though.

>
>>          },
>> +        'unique-next-id' => {
>> +            optional => 1,
>> +            type => 'boolean',
>> +            description =>
>> +                "Only suggest VMIDs that are neither currently in use nor have previously been used.",
>
> Style nit: line too long
> Nit: guest ID instead of VMID

I mostly kept this as VMID because in the description for next-id we
also use VMID and I wanted to avoid using different nomenclature for
what essentially affects the same kind of ID. If we make it a
sub-property, should we use "Next Free Guest ID" in the web UI as well
as "guest ID" in the description here? If we keep them as separate
properties, it would IMO make sense to also adapt the next-id
description to make it clear that this refers to the same thing.

>
>> +        },
>>          migration => {
>>              optional => 1,
>>              type => 'string',

Re: [PATCH many v5 0/7] add option to prevent suggesting previously used VMIDs

[Fiona Ebner]

Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
> This is based on the original series [0] by Severen Redwood and Daniel
> Krambrock. It it rebased on the latest master branches and incorporates
> the feedback from v4.
> 
> I kept the trailers from the original series where I made only minimal
> changes, but added markers (MK: ...) in those cases. For bigger
> changes, I changed the `Co-authored-by` and `Signed-off-by` trailers to
> `Signed-off-by`. I removed Aaron's T-b and R-b trailers since it's been
> a long time and the trailers might be misleading considering the series
> has changed in some of its implementation details overall.
> 
> Changes since v4:
> - File in pmxcfs has been renamed to used-guest-ids to make it a bit
>   more general (both VMs and CTs) while still making clear which IDs
>   are meant.
> - Moved the `UsedVmidList` module from pve-manager to pve-guest-common
>   and renamed it `UsedGuestIDs`. Also renamed some of the functions
>   there for more consistent naming.
> - Implemented `write_id_list` more closely aligned with Fabian's
>   feedback.
> - Also record used VMIDs when creating, cloning, destroying guests as
>   well as incoming remote migrations. (Thanks, @Fiona, for the
>   off-list input and feedback!)
> - Moved the `add_id` calls for recording used IDs to before anything
>   is persisted during any of the above actions.
> - Rephrased some commit messages.
> 
> [0] https://lore.proxmox.com/pve-devel/mailman.62.1731030290.372.pve-devel@lists.proxmox.com/

Looks good in general, some nits and some high-level comments in the
individual patches. I do feel like the suggested change in 3/7 for
tracking ranges instead of constructing a hash with potentially millions
of entries might be worth it, especially since it happens for each guest
that is created and only ever gets more expensive otherwise.

LLM automated review also has some comments and they look valid to me,
so I'm mentioning them here:

1. It would be good to record the needed package dependencies in the
cover letter (and optionally individual patches), to ease a maintainers
job when applying.

2. Recording every second ID can lead to a file larger than the 1 MiB
pmxcfs limit and then every operation will fail regardless of whether
the feature is turned on or not.

3. Remote migration uses destroy_{lxc_container,vm} directly if the
'delete' parameter is set.

For the second point, an example is:
[I] root@pve9a1 ~# expr $(seq 100 2 1000000 | wc --bytes) / 1024 / 1024
3
This is unlikely to happen with regular operation of course, because we
always suggest the lowest free one, but it can't be ruled out. Not
entirely sure what to do about it, but it would be good if we can avoid
the issue. Maybe start dropping gaps and merging anyways if we get close
to the limit? At the very least, we might consider making the failure
non-critical if the feature is turned off.

While looking through the last point, I also noticed that 'qm importovf'
is still a place where we create a new config, so should be recording
the new ID. Maybe we even want to use make sure all places use the
create_and_lock_config() method and do it at the beginning of that? That
might be the cleanest and most future-proof.

Re: [PATCH pve-guest-common v5 3/7] add module to track previously used guest IDs

[Michael Köppl]

On Tue Sep 22, 2026 at 4:22 PM CEST, Fiona Ebner wrote:

[snip]

>> 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.

I think I'd also prefer PVE::GuestID here to avoid possibly having
another module later on or having to refactor this one. Will adapt for
v6!

>
>> +
>> +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.

will adapt for v6, thanks!

>
>> +
>> +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.

will also be updated for v6, thanks!

>
>> +    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.

Thanks for pointing this out! I think the range approach is a good idea!
Should we have a public function that returns the end of a range for a
given guest ID instead of the boolean check_id_unused()? We could then
use it to skip ranges when searching for a free guest ID at
/cluster/nextid instead of invoking cfs_read_file() for each check if a
candidate ID is available. Something like this:

my $i = $lower;
while ($i < $upper) {
    if (defined($idlist->{$i})) { $i++; next; }
    if ($check_unique) {
        if (defined(my $end =
		  PVE::UsedGuestIDs::get_used_range_end($i))) {
            $i = $end + 1;
            next;
        }
    }
    return $i;
}

>
>> +        } 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";
>> +};

[snip]