[PATCH many v6 00/18] add option to prevent suggesting previously used VMIDs

[PATCH many v6 00/18] 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 is rebased on the latest master branches and incorporates
the feedback from v5 [1].

The last patch for cluster, guest-common, qemu-server and container and
the last two for manager add the optional enforcement of the next-id
settings. The rest of the series works without them. Since IDs are
recorded before a guest exists and stay recorded if its creation fails,
retrying a failed creation with the same ID is rejected when both
'enforce' and 'unique' are set.

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.

Dependencies:
- guest-common needs a versioned build and runtime dependency on
  libpve-cluster-perl with virtual-guest/used-guest-ids registered,
  since loading PVE::AbstractConfig now registers the file.
- qemu-server and container need a versioned dependency on
  libpve-guest-common-perl with register_used_id() and its 'existing'
  option.
- guest-common with enforcement should break older qemu-server and
  container, which do not exempt destroying existing guests from it.
- manager needs versioned dependencies on libpve-cluster-perl (next-id
  'unique' and 'enforce') and libpve-guest-common-perl
  (get_next_unused_id() and check_enforced_id()).

Feedback regarding the "forced" merging of ranges to keep the file size
in check would be much appreciated. The merging could continue until
the file size is below the lower threshold to avoid the warning being
printed too soon again, but I wanted to avoid unnecessarily marking
guest IDs as used.

Tests were also added for the pve-guest-common GuestID module
specifically since the logic behind it is complex enough to warrant
some unit testts.

Changes since v5 (thanks @Fiona for the extensive feedback):
- Moved the file to virtual-guest/used-guest-ids.
- Keep used IDs as ranges in memory as well. get_next_unused_id()
  returns the lowest ID that is neither in use nor was used before.
- Renamed the module to PVE::GuestID, use signatures, and renamed
  add_id() to register_used_id().
- Record IDs in create_and_lock_config(), which also covers qm
  importovf, and when remote migration with --delete removes a guest.
- Failing to record an ID only warns while 'unique' is off.
- Close the smallest gaps once the file would exceed 768 KiB, and warn
  above 512 KiB. Are these thresholds fine?
- 'unique' is now a sub-property of next-id.
- New optional 'enforce' sub-property of next-id.
- Added tests for for the GuestID module.

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/
[1] https://lore.proxmox.com/pve-devel/20260921155410.938337-1-m.koeppl@proxmox.com/


cluster:

Michael Köppl (2):
  datacenter config: add unique subproperty to next-id
  datacenter config: next-id: add enforce subproperty

Severen Redwood (1):
  cluster files: add virtual-guest/used-guest-ids

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


guest-common:

Michael Köppl (6):
  add module to track previously used guest IDs
  tests: add tests for used guest ID tracking
  abstract config: register used guest ID when creating config
  guest id: keep used ID list below the pmxcfs file size limit
  tests: add tests for used-guest-ids max file size handling
  guest id: optionally enforce the next-id range and uniqueness

 debian/control              |   1 +
 src/Makefile                |   1 +
 src/PVE/AbstractConfig.pm   |   5 +-
 src/PVE/GuestID.pm          | 251 +++++++++++++++++++++++++++++
 src/tests/Makefile          |   7 +-
 src/tests/guest-id-tests.pl | 303 ++++++++++++++++++++++++++++++++++++
 6 files changed, 565 insertions(+), 3 deletions(-)
 create mode 100644 src/PVE/GuestID.pm
 create mode 100755 src/tests/guest-id-tests.pl


qemu-server:

Michael Köppl (2):
  api: record VM ID as used on destruction and remote migration
  api, remote migrate: exempt existing VMs from next-id enforcement

 src/PVE/API2/Qemu.pm   | 9 +++++++++
 src/PVE/QemuMigrate.pm | 4 ++++
 2 files changed, 13 insertions(+)


container:

Michael Köppl (2):
  api: record CT ID as used on destruction and remote migration
  api, migrate: exempt existing CTs from next-id enforcement

 src/PVE/API2/LXC.pm    | 5 +++++
 src/PVE/LXC/Migrate.pm | 4 ++++
 2 files changed, 9 insertions(+)


manager:

Michael Köppl (4):
  fix #4369: api: optionally only suggest unique IDs
  ui: dc options: rename VMID to guest ID
  api: nextid: reject IDs forbidden by next-id enforcement
  ui: dc options: add option to enforce next free guest ID settings

Severen Redwood (1):
  fix #4369: ui: dc options: add option for unique VM/CT IDs

 PVE/API2/Cluster.pm           | 19 +++++++++++++++----
 www/manager6/dc/OptionView.js | 18 +++++++++++++++++-
 2 files changed, 32 insertions(+), 5 deletions(-)


Summary over all repositories:
  15 files changed, 636 insertions(+), 8 deletions(-)

--
Generated by murpp 0.12.1

[PATCH cluster v6 01/18] cluster files: add virtual-guest/used-guest-ids

[Michael Köppl]

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

Add `/etc/pve/virtual-guest/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, move it to virtual-guest/
     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..9479baa 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -89,6 +89,7 @@ my $observed = {
     'sdn/.running-config' => 1,
     'virtual-guest/cpu-models.conf' => 1,
     'virtual-guest/profiles.cfg' => 1,
+    'virtual-guest/used-guest-ids' => 1,
     'mapping/directory.cfg' => 1,
     'mapping/pci.cfg' => 1,
     'mapping/usb.cfg' => 1,
diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index 12a6c46..d04ad22 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -116,6 +116,7 @@ static memdb_change_t memdb_change_array[] = {
     {.path = "sdn/.running-config"},
     {.path = "virtual-guest/cpu-models.conf"},
     {.path = "virtual-guest/profiles.cfg"},
+    {.path = "virtual-guest/used-guest-ids"},
     {.path = "firewall/cluster.fw"},
     {.path = "mapping/directory.cfg"},
     {.path = "mapping/pci.cfg"},
-- 
2.47.3

[PATCH cluster v6 02/18] datacenter config: add unique subproperty to next-id

[Michael Köppl]

Add 'unique' as a subproperty to next-id config schema. It controls
whether only unique (i.e. neither currently nor previously in use)
guest IDs should be suggested by the `/cluster/nextid` API endpoint.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/DataCenterConfig.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index 004122e..4a58efe 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -217,6 +217,13 @@ my $next_id_format = {
         default => 1000 * 1000, # lower than the maximum on purpose
         optional => 1,
     },
+    unique => {
+        type => 'boolean',
+        description => "Only suggest guest IDs that are neither currently in use"
+            . " nor have previously been used.",
+        default => 0,
+        optional => 1,
+    },
 };
 
 my $u2f_format = {
-- 
2.47.3

[PATCH cluster v6 03/18] datacenter config: next-id: add enforce subproperty

[Michael Köppl]

Add 'enforce' as a subproperty to the next-id config schema. If enabled,
guest IDs outside of the configured range are rejected and, if 'unique'
is enabled as well, previously used IDs are also rejected, even if they
are not currently in use. The boundaries of the range count only when
explicitly configured, not for defaults.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/DataCenterConfig.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index 4a58efe..5f4960c 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -224,6 +224,14 @@ my $next_id_format = {
         default => 0,
         optional => 1,
     },
+    enforce => {
+        type => 'boolean',
+        description => "Reject IDs for new guests outside of the configured"
+            . " 'lower' and 'upper' boundaries and, with 'unique', IDs that"
+            . " were used before.",
+        default => 0,
+        optional => 1,
+    },
 };
 
 my $u2f_format = {
-- 
2.47.3

[PATCH guest-common v6 04/18] 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/virtual-guest/used-guest-ids and register it as
a cfs file, so the list is kept in sync cluster-wide. IDs are stored as
"<start>-<end>" ranges and are not expanded per ID since cfs_read_file()
hands out a deep copy of the parsed data on every call, which gets
expensive on clusters that churn through many guests. Adjacent ranges
are merged on write to keep the file small. Entries that do not parse,
ranges with reversed bounds included, are skipped with a warning instead
of failing the whole read.

get_next_unused_id() returns the lowest ID at or above a given one that
is neither in use nor recorded as having been used previously.

Recording failures are non-critical and will only warn while 'unique' is
off.

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/GuestID.pm | 132 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)
 create mode 100644 src/PVE/GuestID.pm

diff --git a/src/Makefile b/src/Makefile
index 030e7f7..ceba738 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/GuestID.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/GuestID.pm b/src/PVE/GuestID.pm
new file mode 100644
index 0000000..862d92b
--- /dev/null
+++ b/src/PVE/GuestID.pm
@@ -0,0 +1,132 @@
+package PVE::GuestID;
+
+use v5.36;
+
+use PVE::Cluster qw(
+    cfs_lock_file
+    cfs_read_file
+    cfs_register_file
+    cfs_write_file
+);
+
+my $FILENAME = 'virtual-guest/used-guest-ids';
+
+my sub parse_id_list($filename, $raw) {
+    my $ranges = [];
+
+    return $ranges if !defined($raw);
+
+    for my $line (split(/\n/, $raw)) {
+        next if $line =~ m/^\s*$/;
+
+        if ($line =~ m/^(\d+)$/) {
+            push $ranges->@*, [$1, $1];
+        } elsif ($line =~ m/^(\d+)-(\d+)$/) {
+            my ($start, $end) = ($1, $2);
+            if ($start > $end) {
+                warn "skipping reversed range in $filename: $line\n";
+                next;
+            }
+            push $ranges->@*, [$start, $end];
+        } else {
+            warn "skipping invalid entry in $filename: $line\n";
+        }
+    }
+
+    # lookup and insertion rely on the ranges being ordered by start,
+    # merging adjacent and overlapping ones is done when writing the
+    # id list.
+    return [sort { $a->[0] <=> $b->[0] } $ranges->@*];
+}
+
+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;
+}
+
+my sub format_entry($start, $last) {
+    return $start == $last ? "$start\n" : "$start-$last\n";
+}
+
+my sub write_id_list($filename, $ranges) {
+    my $output = '';
+    my ($start, $last);
+
+    for my $range ($ranges->@*) {
+        my ($curr_start, $curr_end) = $range->@*;
+
+        if (!defined($start)) {
+            ($start, $last) = ($curr_start, $curr_end);
+        } elsif ($curr_start <= $last + 1) {
+            $last = $curr_end if $curr_end > $last;
+        } else {
+            $output .= format_entry($start, $last);
+            ($start, $last) = ($curr_start, $curr_end);
+        }
+    }
+
+    $output .= format_entry($start, $last) if defined($start);
+
+    return $output;
+}
+
+# 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);
+    }
+
+    return $id;
+}
+
+my sub insert_id($ranges, $id) {
+    my $i = 0;
+
+    while ($i < @$ranges && $ranges->[$i]->[1] < $id) {
+        $i++;
+    }
+
+    return 0 if $i < @$ranges && $ranges->[$i]->[0] <= $id;
+
+    splice(@$ranges, $i, 0, [$id, $id]);
+
+    return 1;
+}
+
+sub register_used_id($id) {
+    cfs_lock_file(
+        $FILENAME,
+        10,
+        sub {
+            my $ranges = cfs_read_file($FILENAME);
+
+            return if !insert_id($ranges, $id);
+
+            cfs_write_file($FILENAME, $ranges);
+        },
+    );
+
+    if (my $err = $@) {
+        my $dc_conf = cfs_read_file('datacenter.cfg');
+
+        my $emsg = "unable to record guest ID $id as used";
+        die "$emsg - $err" if $dc_conf->{'next-id'}->{unique};
+        warn "$emsg - $err";
+    }
+}
+
+cfs_register_file($FILENAME, \&parse_id_list, \&write_id_list);
+
+1;
-- 
2.47.3

[PATCH guest-common v6 05/18] tests: add tests for used guest ID tracking

[Michael Köppl]

The used guest ID range handling has a few cases that are easy to get
subtly wrong. Adjacent and contained ranges have to be merged on write,
the next unused ID has to be found across adjacent ranges and past
existing guests that were possibly never recorded in the file, etc.

Add tests to cover the parser, writer, get_next_unused_id() and
register_used_id().

Test::MockModule is used here and added as a build dependency because
that is what most other PVE repositories use.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 debian/control              |   1 +
 src/tests/Makefile          |   7 +-
 src/tests/guest-id-tests.pl | 201 ++++++++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+), 2 deletions(-)
 create mode 100755 src/tests/guest-id-tests.pl

diff --git a/debian/control b/debian/control
index 21f0db2..539e3f0 100644
--- a/debian/control
+++ b/debian/control
@@ -6,6 +6,7 @@ Build-Depends: debhelper-compat (= 13),
                libpve-cluster-perl,
                libpve-common-perl,
                libpve-storage-perl (>= 8.3.4),
+               libtest-mockmodule-perl,
                pve-cluster,
 Standards-Version: 4.6.2
 Homepage: https://www.proxmox.com
diff --git a/src/tests/Makefile b/src/tests/Makefile
index 2fec185..f10564b 100644
--- a/src/tests/Makefile
+++ b/src/tests/Makefile
@@ -1,8 +1,11 @@
 
 all:
 
-.PHONY: check abstract-config
-check: abstract-config
+.PHONY: check abstract-config guest-id
+check: abstract-config guest-id
 
 abstract-config: abstract-config-tests.pl
 	perl -I.. ./abstract-config-tests.pl
+
+guest-id: guest-id-tests.pl
+	perl -I.. ./guest-id-tests.pl
diff --git a/src/tests/guest-id-tests.pl b/src/tests/guest-id-tests.pl
new file mode 100755
index 0000000..3be492a
--- /dev/null
+++ b/src/tests/guest-id-tests.pl
@@ -0,0 +1,201 @@
+#!/usr/bin/perl
+
+use v5.36;
+
+use lib qw(..);
+
+use Test::MockModule;
+use Test::More;
+
+my $FILE = '/etc/pve/virtual-guest/used-guest-ids';
+
+my ($parse, $write);
+my $raw; # contents of the stubbed used-guest-ids file
+my $writes = 0;
+my $write_error;
+my $vmlist;
+my $dc_conf = {};
+
+my $pve_cluster_module = Test::MockModule->new('PVE::Cluster');
+$pve_cluster_module->mock(
+    cfs_register_file => sub($filename, $parser, $writer) {
+        ($parse, $write) = ($parser, $writer);
+    },
+    cfs_read_file => sub($filename) {
+        return $dc_conf if $filename eq 'datacenter.cfg';
+        return $parse->("/etc/pve/$filename", $raw);
+    },
+    cfs_write_file => sub($filename, $data) {
+        die $write_error if defined($write_error);
+        $writes++;
+        $raw = $write->("/etc/pve/$filename", $data);
+    },
+    cfs_lock_file => sub($filename, $timeout, $code) {
+        return eval { $code->() };
+    },
+    get_vmlist => sub() { return $vmlist },
+);
+
+require PVE::GuestID;
+
+sub guests(@ids) {
+    return { ids => { map { $_ => { node => 'localhost', type => 'qemu' } } @ids } };
+}
+
+subtest 'parse' => sub {
+    my $tests = [
+        ['canonical file', "100-250\n300\n900-1200\n", [[100, 250], [300, 300], [900, 1200]]],
+        [
+            'unordered file is sorted but not merged',
+            "300-400\n100-200\n201-250\n150-160\n",
+            [[100, 200], [150, 160], [201, 250], [300, 400]],
+        ],
+        ['missing file', undef, []],
+        ['empty file', '', []],
+        ['blank lines only', "\n  \n", []],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $input, $expected) = $test->@*;
+        is_deeply($parse->($FILE, $input), $expected, $name);
+    }
+
+    my @warnings;
+    local $SIG{__WARN__} = sub { push @warnings, $_[0] };
+
+    is_deeply(
+        $parse->($FILE, "100\nfoo\n50-10\n200\n"),
+        [[100, 100], [200, 200]],
+        'invalid entry and reversed range are skipped',
+    );
+    is_deeply(
+        \@warnings,
+        [
+            "skipping invalid entry in $FILE: foo\n",
+            "skipping reversed range in $FILE: 50-10\n",
+        ],
+        'skipped entries are warned about',
+    );
+};
+
+subtest 'write' => sub {
+    my $tests = [
+        [
+            'canonical file is written unchanged',
+            "100-250\n300\n900-1200\n",
+            "100-250\n300\n900-1200\n",
+        ],
+        [
+            'overlapping and adjacent ranges are merged',
+            "300-400\n100-200\n201-250\n150-160\n",
+            "100-250\n300-400\n",
+        ],
+        ['adjacent single IDs are merged', "100\n101\n102\n", "100-102\n"],
+        ['contained range keeps its container', "100-500\n200-300\n", "100-500\n"],
+        ['nothing recorded', '', ''],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $input, $expected) = $test->@*;
+        is($write->($FILE, $parse->($FILE, $input)), $expected, $name);
+    }
+};
+
+subtest 'get_next_unused_id' => sub {
+    my $tests = [
+        # name, file contents, existing guests, input ID, expected ID
+        ['below all ranges', "100-200\n300-400\n", [], 50, 50],
+        ['start of a range', "100-200\n300-400\n", [], 100, 201],
+        ['inside a range', "100-200\n300-400\n", [], 150, 201],
+        ['in a gap', "100-200\n300-400\n", [], 250, 250],
+        ['above all ranges', "100-200\n300-400\n", [], 500, 500],
+        ['across adjacent ranges', "100-200\n201-300\n", [], 150, 301],
+        ['existing guests are skipped', "100-105\n", [106, 107], 100, 108],
+        ['used range after an existing guest', "100-105\n107-110\n", [106], 100, 111],
+        ['existing guests not recorded in the file', '', [100, 101], 100, 102],
+        ['no vmlist', "100-105\n", undef, 100, 106],
+        ['nothing recorded, no guests', undef, [], 100, 100],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $file, $existing, $id, $expected) = $test->@*;
+
+        $raw = $file;
+        $vmlist = defined($existing) ? guests($existing->@*) : undef;
+
+        is(PVE::GuestID::get_next_unused_id($id), $expected, $name);
+    }
+};
+
+subtest 'register_used_id' => sub {
+    $raw = undef;
+
+    my $steps = [
+        ['first ID', 100, "100\n"],
+        ['extends a range at its end', 101, "100-101\n"],
+        ['starts a new range after a gap', 103, "100-101\n103\n"],
+        ['bridges two ranges', 102, "100-103\n"],
+        ['extends a range at its start', 99, "99-103\n"],
+        ['starts a new range above all others', 200, "99-103\n200\n"],
+    ];
+
+    for my $step ($steps->@*) {
+        my ($name, $id, $expected) = $step->@*;
+        PVE::GuestID::register_used_id($id);
+        is($raw, $expected, $name);
+    }
+
+    $writes = 0;
+    PVE::GuestID::register_used_id(101);
+    PVE::GuestID::register_used_id(200);
+    is($writes, 0, 'already recorded IDs are not written again');
+    is($raw, "99-103\n200\n", 'file unchanged by already recorded IDs');
+
+    $raw = "300-400\n100-200\n201-250\n150-160\n";
+    PVE::GuestID::register_used_id(275);
+    is($raw, "100-250\n275\n300-400\n", 'unordered file is normalized on write');
+};
+
+subtest 'register_used_id errors' => sub {
+    my $tests = [
+        # name, datacenter.cfg, whether the error reaches the caller
+        ['unique IDs requested', { 'next-id' => { unique => 1 } }, 1],
+        ['unique IDs not requested', { 'next-id' => { lower => 100 } }, 0],
+        ['no next-id settings', {}, 0],
+    ];
+
+    for my $test ($tests->@*) {
+        my ($name, $conf, $fatal) = $test->@*;
+
+        $dc_conf = $conf;
+        $raw = "100\n";
+        $write_error = "write failed\n";
+
+        my @warnings;
+        local $SIG{__WARN__} = sub { push @warnings, $_[0] };
+
+        eval { PVE::GuestID::register_used_id(500) };
+
+        if ($fatal) {
+            is(
+                $@,
+                "unable to record guest ID 500 as used - write failed\n",
+                "$name: error is passed on to the caller",
+            );
+            is_deeply(\@warnings, [], "$name: no warning");
+        } else {
+            is($@, '', "$name: error is not passed on");
+            is_deeply(
+                \@warnings,
+                ["unable to record guest ID 500 as used - write failed\n"],
+                "$name: error is warned about",
+            );
+        }
+        is($raw, "100\n", "$name: file unchanged");
+    }
+
+    $write_error = undef;
+    $dc_conf = {};
+};
+
+done_testing();
-- 
2.47.3

[PATCH guest-common v6 06/18] abstract config: register used guest ID when creating config

[Michael Köppl]

This ensures that upon creation of a guest, the chosen guest ID is
always recorded such that optionally enabling unique IDs later on can
ensure that no ID that has previously been used can be chosen again.
Moving it into create_and_lock_config avoids repeating the
register_used_id call at every site where a guest config is created,
including importovf and incoming remote migrations.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/AbstractConfig.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 7bcae19..229e4bd 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -10,6 +10,7 @@ use PVE::Storage;
 use PVE::Tools qw(lock_file lock_file_full);
 
 use PVE::GuestHelpers qw(typesafe_ne);
+use PVE::GuestID;
 use PVE::ReplicationConfig;
 use PVE::Replication;
 
@@ -261,6 +262,8 @@ sub create_and_lock_config {
         sub {
             PVE::Cluster::check_vmid_unused($vmid, $allow_existing);
 
+            PVE::GuestID::register_used_id($vmid);
+
             my $conf = eval { $class->load_config($vmid) } || {};
             $class->check_lock($conf);
             $conf->{lock} = $lock // 'create';
-- 
2.47.3

[PATCH guest-common v6 07/18] guest id: keep used ID list below the pmxcfs file size limit

[Michael Köppl]

The used-guest-ids file can grow to more than 1 MiB if many
non-adjacent ID ranges or individual guest IDs are stored. While
unlikely to happen with regular operation, exceeding this limit on a
file in pmxcfs will cause every operation to fail, regardless of
whether the feature is turned on or not. To avoid this, add 2
thresholds: the first at 512 KiB, at which users are warned that their
ranges might be aggressively merged in the future, and the second at
768 KiB, at which this aggressive merging happens.

Once the second threshold is exceeded, the smallest gaps between ranges
will be merged until the file size is below the threshold again. This
will cause previously unused IDs to be recorded as used, resulting in
fewer available IDs overall.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
Hope I didn't overdo it with the comments here, but I felt that some
explanatory comments were warranted. I also considered aggressively
merging ranges until the file sizes is at $SIZE_WARN again. That would
mean that even more guest IDs would be marked as used even though not
strictly necessary. Feedback here would be much appreciated. I think it
can be argued that repeated warnings if the file size if between
$SIZE_WARN and $SIZE_MAX are annoying, but I'm not sure if the "lost"
guest IDs would be worth it.

 src/PVE/GuestID.pm | 104 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 93 insertions(+), 11 deletions(-)

diff --git a/src/PVE/GuestID.pm b/src/PVE/GuestID.pm
index 862d92b..4ab6f2d 100644
--- a/src/PVE/GuestID.pm
+++ b/src/PVE/GuestID.pm
@@ -11,6 +11,10 @@ use PVE::Cluster qw(

 my $FILENAME = 'virtual-guest/used-guest-ids';

+# keep well below the 1 MiB file size limit of pmxcfs, writes fail beyond it
+my $SIZE_WARN = 512 * 1024;
+my $SIZE_MAX = 768 * 1024;
+
 my sub parse_id_list($filename, $raw) {
     my $ranges = [];

@@ -54,24 +58,102 @@ my sub format_entry($start, $last) {
     return $start == $last ? "$start\n" : "$start-$last\n";
 }

-my sub write_id_list($filename, $ranges) {
-    my $output = '';
-    my ($start, $last);
+my sub merge_ranges($ranges) {
+    my $merged = [];

     for my $range ($ranges->@*) {
-        my ($curr_start, $curr_end) = $range->@*;
+        my ($start, $end) = $range->@*;
+        my $last = $merged->[-1];

-        if (!defined($start)) {
-            ($start, $last) = ($curr_start, $curr_end);
-        } elsif ($curr_start <= $last + 1) {
-            $last = $curr_end if $curr_end > $last;
+        if ($last && $start <= $last->[1] + 1) {
+            $last->[1] = $end if $end > $last->[1];
         } else {
-            $output .= format_entry($start, $last);
-            ($start, $last) = ($curr_start, $curr_end);
+            push $merged->@*, [$start, $end];
         }
     }

-    $output .= format_entry($start, $last) if defined($start);
+    return $merged;
+}
+
+my sub format_ranges($ranges) {
+    return join('', map { format_entry($_->@*) } $ranges->@*);
+}
+
+# Closes the smallest gaps between ranges in $merged until the
+# formatted list of $size bytes fits into $SIZE_MAX. The unused IDs in
+# a closed gap count as used from then on. Returns the resulting ranges
+# and the number of unused IDs that got marked as used.
+my sub close_smallest_gaps($merged, $size) {
+    my @gap_sizes;
+    for my $i (1 .. $#$merged) {
+        my $prev_end = $merged->[$i - 1]->[1];
+        my $curr_start = $merged->[$i]->[0];
+
+        $gap_sizes[$i] = $curr_start - $prev_end - 1;
+    }
+
+    my @gap_order = sort { $gap_sizes[$a] <=> $gap_sizes[$b] || $a <=> $b } 1 .. $#$merged;
+
+    # Record start and end of each group. Initially, with e.g. 6 ranges,
+    # this would be:
+    #   start: (0, 1, 2, 3, 4, 5)
+    #   end:   (0, 1, 2, 3, 4, 5)
+    # After joining the ranges at indices 1-3 and at 4 5, this would be:
+    #   start: (0, 1, 1, 1, 4, 4)
+    #   end:   (0, 3, 2, 3, 5, 5)
+    my @group_start = (0 .. $#$merged);
+    my @group_end = (0 .. $#$merged);
+
+    my $closed_id_count = 0;
+
+    for my $i (@gap_order) {
+        last if $size <= $SIZE_MAX;
+
+        my ($first_index, $last_index) = ($group_start[$i - 1], $group_end[$i]);
+        my ($new_range_start, $new_range_end) =
+            ($merged->[$first_index]->[0], $merged->[$last_index]->[1]);
+        # used IDs directly before and after gap $i
+        my ($before_gap, $after_gap) = ($merged->[$i - 1]->[1], $merged->[$i]->[0]);
+
+        # the lines of both groups are replaced by a single line
+        # spanning them
+        $size +=
+            length(format_entry($new_range_start, $new_range_end)) -
+            length(format_entry($new_range_start, $before_gap)) -
+            length(format_entry($after_gap, $new_range_end));
+
+        $group_end[$first_index] = $last_index;
+        $group_start[$last_index] = $first_index;
+        $closed_id_count += $gap_sizes[$i];
+    }
+
+    my $closed = [];
+    for (my $i = 0; $i <= $#$merged; $i = $group_end[$i] + 1) {
+        push $closed->@*, [$merged->[$i]->[0], $merged->[$group_end[$i]]->[1]];
+    }
+
+    return ($closed, $closed_id_count);
+}
+
+my sub write_id_list($filename, $ranges) {
+    my $merged = merge_ranges($ranges);
+    my $output = format_ranges($merged);
+
+    if (length($output) > $SIZE_MAX) {
+        my ($closed, $closed_ids) = close_smallest_gaps($merged, length($output));
+        $output = format_ranges($closed);
+        warn "$filename would exceed "
+            . ($SIZE_MAX / 1024) . " KiB"
+            . ", recorded $closed_ids unused guest IDs in the smallest"
+            . " gaps as used\n";
+    } elsif (length($output) > $SIZE_WARN) {
+        warn "$filename is "
+            . int(length($output) / 1024) . " KiB"
+            . ", the smallest gaps between used guest IDs will be"
+            . " closed beyond "
+            . ($SIZE_MAX / 1024)
+            . " KiB\n";
+    }

     return $output;
 }
--
2.47.3

[PATCH guest-common v6 08/18] tests: add tests for used-guest-ids max file size handling

[Michael Köppl]

The test cases cover whether a file exceeding the thresholds is
correctly detected and also cover multiple scenarios for merging ranges
with gaps between them until file size is below the threshold again.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/tests/guest-id-tests.pl | 102 ++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/src/tests/guest-id-tests.pl b/src/tests/guest-id-tests.pl
index 3be492a..6482f64 100755
--- a/src/tests/guest-id-tests.pl
+++ b/src/tests/guest-id-tests.pl
@@ -4,11 +4,16 @@ use v5.36;
 
 use lib qw(..);
 
+use List::Util qw(all min sum0);
 use Test::MockModule;
 use Test::More;
 
 my $FILE = '/etc/pve/virtual-guest/used-guest-ids';
 
+# must match the thresholds in PVE::GuestID
+my $SIZE_WARN = 512 * 1024;
+my $SIZE_MAX = 768 * 1024;
+
 my ($parse, $write);
 my $raw; # contents of the stubbed used-guest-ids file
 my $writes = 0;
@@ -42,6 +47,38 @@ sub guests(@ids) {
     return { ids => { map { $_ => { node => 'localhost', type => 'qemu' } } @ids } };
 }
 
+# single IDs from 100 upwards, with the gaps between them cycling through @gaps
+sub single_ids($count, @gaps) {
+    my @lines;
+    my $id = 100;
+    for my $i (0 .. $count - 1) {
+        push @lines, "$id\n";
+        $id += 1 + $gaps[$i % @gaps];
+    }
+    return join('', @lines);
+}
+
+sub gap_sizes($ranges) {
+    my @gap_sizes;
+    for my $i (1 .. $#$ranges) {
+        push @gap_sizes, $ranges->[$i]->[0] - $ranges->[$i - 1]->[1] - 1;
+    }
+    return @gap_sizes;
+}
+
+# whether every range in $inner lies within a range in $outer, both sorted
+sub covers($outer, $inner) {
+    my $i = 0;
+    for my $range ($inner->@*) {
+        while ($i < @$outer && $outer->[$i]->[1] < $range->[0]) {
+            $i++;
+        }
+        return 0 if $i >= @$outer;
+        return 0 if $outer->[$i]->[0] > $range->[0] || $outer->[$i]->[1] < $range->[1];
+    }
+    return 1;
+}
+
 subtest 'parse' => sub {
     my $tests = [
         ['canonical file', "100-250\n300\n900-1200\n", [[100, 250], [300, 300], [900, 1200]]],
@@ -101,6 +138,71 @@ subtest 'write' => sub {
     }
 };
 
+subtest 'write size limit' => sub {
+    my sub write_with_warnings($input) {
+        my @warnings;
+        local $SIG{__WARN__} = sub { push @warnings, $_[0] };
+        my $output = $write->($FILE, $parse->($FILE, $input));
+        return ($output, \@warnings);
+    }
+
+    my $small = single_ids(50_000, 1);
+    ok(length($small) < $SIZE_WARN, 'small list is below the warning threshold');
+    my ($output, $warnings) = write_with_warnings($small);
+    is($output, $small, 'below the warning threshold: written unchanged');
+    is_deeply($warnings, [], 'below the warning threshold: no warning');
+
+    my $medium = single_ids(90_000, 1);
+    ok(length($medium) > $SIZE_WARN && length($medium) < $SIZE_MAX,
+        'medium list is in between');
+    ($output, $warnings) = write_with_warnings($medium);
+    is($output, $medium, 'above the warning threshold: written unchanged');
+    is(scalar($warnings->@*), 1, 'above the warning threshold: one warning');
+    like(
+        $warnings->[0],
+        qr/^\Q$FILE\E is \d+ KiB, .* closed beyond 768 KiB\n\z/,
+        'above the warning threshold: size is warned about',
+    );
+
+    # above the hard limit, all gaps are equal, so they get closed from
+    # the lowest ID upwards
+    my $large = single_ids(150_000, 1);
+    ok(length($large) > $SIZE_MAX, 'large list is above the limit');
+    my $input = $parse->($FILE, $large);
+    ($output, $warnings) = write_with_warnings($large);
+    my $closed = $parse->($FILE, $output);
+    ok(length($output) <= $SIZE_MAX, 'above the limit: output fits');
+    ok(covers($closed, $input), 'above the limit: every used ID is still recorded');
+    is(scalar($warnings->@*), 1, 'above the limit: one warning');
+    my ($count) =
+        ($warnings->[0] // '') =~ m/would exceed 768 KiB, recorded (\d+) unused guest IDs? /;
+    is(
+        $count,
+        sum0(gap_sizes($input)) - sum0(gap_sizes($closed)),
+        'above the limit: number of IDs marked as used is reported',
+    );
+    is($closed->[0]->[0], 100, 'equal gaps: the lowest ID starts the joined range');
+    ok(
+        (all { $_->[0] == $_->[1] } $closed->@[1 .. $#$closed]),
+        'equal gaps: only the lowest ones are closed',
+    );
+
+    # above the hard limit, mixed gaps
+    my $mixed = single_ids(200_000, 1, 2, 5, 50);
+    $input = $parse->($FILE, $mixed);
+    ($output, $warnings) = write_with_warnings($mixed);
+    $closed = $parse->($FILE, $output);
+    my @remaining = gap_sizes($closed);
+    ok(length($output) <= $SIZE_MAX, 'mixed gaps: output fits');
+    ok(covers($closed, $input), 'mixed gaps: every used ID is still recorded');
+    is(min(@remaining), 5, 'mixed gaps: all gaps of 1 and 2 are closed first');
+    is(
+        scalar(grep { $_ == 50 } @remaining),
+        scalar(grep { $_ == 50 } gap_sizes($input)),
+        'mixed gaps: the largest gaps are kept',
+    );
+};
+
 subtest 'get_next_unused_id' => sub {
     my $tests = [
         # name, file contents, existing guests, input ID, expected ID
-- 
2.47.3

[PATCH guest-common v6 09/18] guest id: optionally enforce the next-id range and uniqueness

[Michael Köppl]

If the 'enforce' subproperty of next-id is enabled, ensure that only
guest IDs from the configured range and, if unique is enabled as well,
only previously unused IDs can be used. By setting existing => 1, the
enforcement of these criteria is disabled for actions on existing
guests, such as destroy, remote migration with --delete, and restore
over an existing guest.

IDs are recorded before a guest exists and stay recorded if its
creation fails later. With both 'enforce' and 'unique' set, retrying a
failed creation with the same ID is therefore rejected.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/AbstractConfig.pm |  4 ++--
 src/PVE/GuestID.pm        | 49 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 229e4bd..2f20596 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -260,9 +260,9 @@ sub create_and_lock_config {
         $vmid,
         5,
         sub {
-            PVE::Cluster::check_vmid_unused($vmid, $allow_existing);
+            my $is_new = PVE::Cluster::check_vmid_unused($vmid, $allow_existing);
 
-            PVE::GuestID::register_used_id($vmid);
+            PVE::GuestID::register_used_id($vmid, { existing => !$is_new });
 
             my $conf = eval { $class->load_config($vmid) } || {};
             $class->check_lock($conf);
diff --git a/src/PVE/GuestID.pm b/src/PVE/GuestID.pm
index 4ab6f2d..02abf24 100644
--- a/src/PVE/GuestID.pm
+++ b/src/PVE/GuestID.pm
@@ -187,26 +187,63 @@ my sub insert_id($ranges, $id) {
     return 1;
 }
 
-sub register_used_id($id) {
-    cfs_lock_file(
+my sub check_range($next_id, $id) {
+    my $lower = $next_id->{lower};
+    my $upper = $next_id->{upper};
+
+    die "guest ID $id is below the lower boundary $lower of the next-id range\n"
+        if defined($lower) && $id < $lower;
+    die "guest ID $id is not below the upper boundary $upper of the next-id range\n"
+        if defined($upper) && $id >= $upper;
+}
+
+# Dies if the next-id datacenter option enforces its range or uniqueness
+# and a new guest must not use $id. Existing guests are not considered.
+sub check_enforced_id($id) {
+    my $next_id = cfs_read_file('datacenter.cfg')->{'next-id'} // {};
+    return if !$next_id->{enforce};
+
+    check_range($next_id, $id);
+
+    if ($next_id->{unique}) {
+        my $ranges = cfs_read_file($FILENAME);
+        die "guest ID $id was used before\n" if next_unused($ranges, $id) != $id;
+    }
+}
+
+# Records $id as used. If the next-id datacenter option enforces its
+# range or uniqueness, IDs a new guest must not use are rejected, unless
+# $opts->{existing} marks $id as belonging to an existing guest.
+sub register_used_id($id, $opts = {}) {
+    my $next_id = cfs_read_file('datacenter.cfg')->{'next-id'} // {};
+    # never reject an existing guest
+    my $enforce = $next_id->{enforce} && !$opts->{existing};
+
+    check_range($next_id, $id) if $enforce;
+
+    my $recorded = cfs_lock_file(
         $FILENAME,
         10,
         sub {
             my $ranges = cfs_read_file($FILENAME);
 
-            return if !insert_id($ranges, $id);
+            return 0 if !insert_id($ranges, $id);
 
             cfs_write_file($FILENAME, $ranges);
+            return 1;
         },
     );
 
     if (my $err = $@) {
-        my $dc_conf = cfs_read_file('datacenter.cfg');
-
         my $emsg = "unable to record guest ID $id as used";
-        die "$emsg - $err" if $dc_conf->{'next-id'}->{unique};
+        die "$emsg - $err" if $next_id->{unique};
         warn "$emsg - $err";
+
+        return;
     }
+
+    die "guest ID $id was used before\n"
+        if $enforce && $next_id->{unique} && !$recorded;
 }
 
 cfs_register_file($FILENAME, \&parse_id_list, \&write_id_list);
-- 
2.47.3

[PATCH qemu-server v6 10/18] api: record VM ID as used on destruction and remote migration

[Michael Köppl]

Record the ID of a VM as used when it is destroyed, cloned or removed
after a remote migration with --delete, so that the `/cluster/nextid`
endpoint can later optionally avoid suggesting previously used IDs.

For creation, restore and incoming remote migrations the ID is
recorded by create_and_lock_config(). VM clones write the new config
directly instead, so they record it explicitly. Recording on
destruction as well covers guests that were created before IDs were
tracked. The same holds for a remote migration with --delete, which
removes the source guest without going through the destroy API. This
call explicitly only warns on failure, since the guest has already
moved.

With 'unique' set in the next-id datacenter option, a failure to
record aborts create, clone and destroy before anything is changed.
Otherwise it only warns.

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   | 9 +++++++++
 src/PVE/QemuMigrate.pm | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 922c6599..9cb36e2b 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -25,6 +25,7 @@ use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
 use PVE::ReplicationConfig;
 use PVE::GuestHelpers qw(assert_tag_permissions);
+use PVE::GuestID;
 use PVE::GuestImport;
 use PVE::QemuConfig;
 use PVE::QemuServer;
@@ -2858,6 +2859,10 @@ __PACKAGE__->register_method({
 
         $early_checks->();
 
+        # record before destroying anything, so a failure here leaves the VM intact
+        eval { PVE::GuestID::register_used_id($vmid) };
+        die "unable to destroy VM $vmid - $@" if $@;
+
         my $realcmd = sub {
             my $upid = shift;
 
@@ -4621,6 +4626,10 @@ __PACKAGE__->register_method({
                 $newconf->{description} = $param->{description};
             }
 
+            # record before the config exists, so a failure here leaves nothing behind
+            eval { PVE::GuestID::register_used_id($newid) };
+            die "unable to clone VM $vmid - $@" 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");
diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..3f2d9b91 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -13,6 +13,7 @@ use PVE::Cluster;
 use PVE::Format qw(render_bytes);
 use PVE::Firewall::Helpers;
 use PVE::GuestHelpers qw(safe_boolean_ne safe_string_ne);
+use PVE::GuestID;
 use PVE::INotify;
 use PVE::JSONSchema;
 use PVE::RPCEnvironment;
@@ -1904,6 +1905,9 @@ sub phase3_cleanup {
     }
 
     if ($self->{opts}->{remote} && $self->{opts}->{delete}) {
+        eval { PVE::GuestID::register_used_id($vmid) };
+        warn $@ if $@;
+
         eval { PVE::QemuServer::destroy_vm($self->{storecfg}, $vmid, 1, undef, 0) };
         warn "Failed to remove source VM - $@\n" if $@;
     }
-- 
2.47.3

[PATCH qemu-server v6 11/18] api, remote migrate: exempt existing VMs from next-id enforcement

[Michael Köppl]

With 'enforce' set in the next-id datacenter option,
register_used_id() rejects IDs outside the configured range and, with
'unique', IDs that were used before. However, this behavior is only
meant for IDs claimed by new guests.

Destroying a VM and removing the source VM after a remote migration with
--delete both record the ID of a VM that already exists. That ID was
recorded when the VM was created and it may lie outside the configured
range if the range was changed later. Thus, exempt these cases from
enforcement.

Cloning creates a new VM and stays subject to enforcement.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/API2/Qemu.pm   | 2 +-
 src/PVE/QemuMigrate.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 9cb36e2b..210a0861 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -2860,7 +2860,7 @@ __PACKAGE__->register_method({
         $early_checks->();
 
         # record before destroying anything, so a failure here leaves the VM intact
-        eval { PVE::GuestID::register_used_id($vmid) };
+        eval { PVE::GuestID::register_used_id($vmid, { existing => 1 }) };
         die "unable to destroy VM $vmid - $@" if $@;
 
         my $realcmd = sub {
diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 3f2d9b91..680b5960 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1905,7 +1905,7 @@ sub phase3_cleanup {
     }
 
     if ($self->{opts}->{remote} && $self->{opts}->{delete}) {
-        eval { PVE::GuestID::register_used_id($vmid) };
+        eval { PVE::GuestID::register_used_id($vmid, { existing => 1 }) };
         warn $@ if $@;
 
         eval { PVE::QemuServer::destroy_vm($self->{storecfg}, $vmid, 1, undef, 0) };
-- 
2.47.3

[PATCH container v6 12/18] api: record CT ID as used on destruction and remote migration

[Michael Köppl]

Record the ID of a CT as used when it is destroyed or removed after a
remote migration with --delete, so that the `/cluster/nextid` endpoint
can later optionally avoid suggesting previously used IDs.

For creation, restore and incoming remote migrations the ID is
recorded by create_and_lock_config(). Recording on destruction as well
covers guests that were created before IDs were tracked. The same holds
for a remote migration with --delete, which removes the source guest
without going through the destroy API; that call only warns on failure,
since the guest has already moved.

With 'unique' set in the next-id datacenter option, a failure to
record aborts create, clone and destroy before anything is changed.
Otherwise it only warns.

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    | 5 +++++
 src/PVE/LXC/Migrate.pm | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 5f94d5a1..fb4262c9 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -14,6 +14,7 @@ use PVE::DataCenterConfig;
 use PVE::Exception qw(raise raise_param_exc raise_perm_exc);
 use PVE::Firewall;
 use PVE::GuestHelpers;
+use PVE::GuestID;
 use PVE::INotify;
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::RESTHandler;
@@ -882,6 +883,10 @@ __PACKAGE__->register_method({
 
         $early_checks->($conf);
 
+        # record before destroying anything, so a failure here leaves the CT intact
+        eval { PVE::GuestID::register_used_id($vmid) };
+        die "unable to destroy CT $vmid - $@" 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
 
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index d243d901..a215dede 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -7,6 +7,7 @@ use File::Basename;
 use File::Copy; # fixme: remove
 
 use PVE::Cluster;
+use PVE::GuestID;
 use PVE::INotify;
 use PVE::JSONSchema;
 use PVE::Replication;
@@ -541,6 +542,9 @@ sub final_cleanup {
             PVE::Tunnel::write_tunnel($self->{tunnel}, 60, 'start');
         }
         if ($self->{opts}->{delete}) {
+            eval { PVE::GuestID::register_used_id($vmid) };
+            warn $@ if $@;
+
             PVE::LXC::destroy_lxc_container(
                 PVE::Storage::config(),
                 $vmid,
-- 
2.47.3

[PATCH container v6 13/18] api, migrate: exempt existing CTs from next-id enforcement

[Michael Köppl]

With 'enforce' set in the next-id datacenter option,
register_used_id() rejects IDs outside the configured range and, with
'unique', IDs that were used before. That is only meant for IDs claimed
by new guests.

Destroying a CT and removing the source CT after a remote migration with
--delete both record the ID of a CT that already exists. That ID was
recorded when the CT was created and it may lie outside the configured
range if the range was changed later. Thus, exempt these cases from
enforcement.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/PVE/API2/LXC.pm    | 2 +-
 src/PVE/LXC/Migrate.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index fb4262c9..179eb3c4 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -884,7 +884,7 @@ __PACKAGE__->register_method({
         $early_checks->($conf);
 
         # record before destroying anything, so a failure here leaves the CT intact
-        eval { PVE::GuestID::register_used_id($vmid) };
+        eval { PVE::GuestID::register_used_id($vmid, { existing => 1 }) };
         die "unable to destroy CT $vmid - $@" if $@;
 
         my $running_error_msg = "unable to destroy CT $vmid - container is running\n";
diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index a215dede..5db867aa 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -542,7 +542,7 @@ sub final_cleanup {
             PVE::Tunnel::write_tunnel($self->{tunnel}, 60, 'start');
         }
         if ($self->{opts}->{delete}) {
-            eval { PVE::GuestID::register_used_id($vmid) };
+            eval { PVE::GuestID::register_used_id($vmid, { existing => 1 }) };
             warn $@ if $@;
 
             PVE::LXC::destroy_lxc_container(
-- 
2.47.3

[PATCH manager v6 14/18] fix #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 with unique=1 in the next-id
datacenter option and the previously used IDs are tracked as a list in
the file `/etc/pve/virtual-guest/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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 4e5efbfd9..e5017f6c4 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -11,6 +11,7 @@ use PVE::DataCenterConfig;
 use PVE::Exception qw(raise_param_exc);
 use PVE::Firewall;
 use PVE::GuestHelpers;
+use PVE::GuestID;
 use PVE::HA::Config;
 use PVE::HA::Env::PVE2;
 use PVE::INotify;
@@ -1045,8 +1046,13 @@ __PACKAGE__->register_method({
         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});
+        if ($next_id->{unique}) {
+            my $id = PVE::GuestID::get_next_unused_id($lower);
+            return $id if $id < $upper;
+        } else {
+            for (my $i = $lower; $i < $upper; $i++) {
+                return $i if !defined($idlist->{$i});
+            }
         }
 
         die "unable to get any free VMID in range [$lower, $upper]\n";
-- 
2.47.3

[PATCH manager v6 15/18] ui: dc options: rename VMID to guest ID

[Michael Köppl]

To make clear that this is for both VMs and CTs, use the more general
term "guest ID".

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/dc/OptionView.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index dc12aa7e1..b4291afd3 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -360,7 +360,7 @@ Ext.define('PVE.dc.OptionView', {
             minValue: 1,
             maxValue: 64, // arbitrary but generous limit as limits are good
         });
-        me.add_inputpanel_row('next-id', gettext('Next Free VMID Range'), {
+        me.add_inputpanel_row('next-id', gettext('Next Free Guest ID Range'), {
             renderer: PVE.Utils.render_as_property_string,
             url: '/api2/extjs/cluster/options',
             items: [
-- 
2.47.3

[PATCH manager v6 16/18] fix #4369: ui: dc options: add option for unique VM/CT IDs

[Michael Köppl]

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

Add an additional checkbox to the "Next Free Guest ID" option 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.

Also rename the top-level option to "Next Free Guest ID" since it's no
longer just the range.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/dc/OptionView.js | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index b4291afd3..97f54cda1 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -360,7 +360,7 @@ Ext.define('PVE.dc.OptionView', {
             minValue: 1,
             maxValue: 64, // arbitrary but generous limit as limits are good
         });
-        me.add_inputpanel_row('next-id', gettext('Next Free Guest ID Range'), {
+        me.add_inputpanel_row('next-id', gettext('Next Free Guest ID'), {
             renderer: PVE.Utils.render_as_property_string,
             url: '/api2/extjs/cluster/options',
             items: [
@@ -382,6 +382,13 @@ Ext.define('PVE.dc.OptionView', {
                     maxValue: 1000 * 1000 * 1000 - 1,
                     submitValue: true,
                 },
+                {
+                    xtype: 'proxmoxcheckbox',
+                    name: 'unique',
+                    fieldLabel: gettext('Unique'),
+                    boxLabel: gettext('Only suggest guest IDs that were never used'),
+                    value: 0,
+                },
             ],
         });
         me.rows['tag-style'] = {
-- 
2.47.3

[PATCH manager v6 17/18] api: nextid: reject IDs forbidden by next-id enforcement

[Michael Köppl]

With 'enforce' set in the next-id datacenter option, new guests may
only use IDs within the configured range and, with 'unique', IDs that
were not used before. Creating a guest with any other ID fails anyway,
but the GUI validates its guest ID field through this endpoint, so
reject such IDs here as well to report the problem before a task is
started.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 PVE/API2/Cluster.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index e5017f6c4..995917f0a 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -1036,8 +1036,13 @@ __PACKAGE__->register_method({
         my $idlist = $vmlist->{ids} || {};
 
         if (my $vmid = $param->{vmid}) {
-            return $vmid if !defined($idlist->{$vmid});
-            raise_param_exc({ vmid => "VM $vmid already exists" });
+            raise_param_exc({ vmid => "VM $vmid already exists" })
+                if defined($idlist->{$vmid});
+
+            eval { PVE::GuestID::check_enforced_id($vmid) };
+            raise_param_exc({ vmid => $@ }) if $@;
+
+            return $vmid;
         }
 
         my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
-- 
2.47.3

[PATCH manager v6 18/18] ui: dc options: add option to enforce next free guest ID settings

[Michael Köppl]

Add an 'Enforce' checkbox to the "Next Free Guest ID" option. With it
set, the range and, if 'unique' is set as well, never having been used
are required for the IDs of new guests, instead of only being used to
suggest IDs.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/dc/OptionView.js | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index 97f54cda1..72680b2d6 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -389,6 +389,15 @@ Ext.define('PVE.dc.OptionView', {
                     boxLabel: gettext('Only suggest guest IDs that were never used'),
                     value: 0,
                 },
+                {
+                    xtype: 'proxmoxcheckbox',
+                    name: 'enforce',
+                    fieldLabel: gettext('Enforce'),
+                    boxLabel: gettext(
+                        'Reject guest IDs outside the range or, with unique, guest IDs that were used before',
+                    ),
+                    value: 0,
+                },
             ],
         });
         me.rows['tag-style'] = {
-- 
2.47.3