[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