[pve-devel] [PATCH SERIES v3] Add ability to prevent suggesting previously used VM/CT IDs

[pve-devel] [PATCH SERIES v3] Add ability to prevent suggesting previously used VM/CT IDs

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4438 bytes --]

From: "Severen Redwood" <severen.redwood@sitehost.co.nz>
To: <pve-devel@lists.proxmox.com>
Cc: "Aaron Lauterer" <a.lauterer@proxmox.com>, "Thomas Lamprecht" <t.lamprecht@proxmox.com>
Subject: [PATCH SERIES v3] Add ability to prevent suggesting previously used VM/CT IDs
Date: Tue, 05 Nov 2024 14:58:08 +1300
Message-ID: <D5DVFH6OZLBN.2KLGTH53ZHGNJ@sitehost.co.nz>

Hi everyone,

This is a small update to my previous patch series [1, 2]
adding optional support for preventing PVE from suggesting
previously used VM/CT IDs.

In particular, missing use statements have been added to the
patches for marking VM/CT IDs as used, as noticed by Aaron
[3]. Everything else remains the same as in v2.

Also note when building this patch series that the pve-cluster
patches must be applied and built first to prevent the build
of pve-manager from failing.

If anyone has any other feedback, please let me know.

Thanks,
Severen

[1]: https://lore.proxmox.com/pve-devel/mailman.177.1728018639.332.pve-devel@lists.proxmox.com/
[2]: https://lore.proxmox.com/pve-devel/mailman.624.1730255368.332.pve-devel@lists.proxmox.com/T/#t
[3]: https://lore.proxmox.com/pve-devel/mailman.624.1730255368.332.pve-devel@lists.proxmox.com/T/#m7313a65f5b26f29ff0a2b4005069a7306311ccf6


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager v3 1/2] close #4369: api: optionally only suggest unique IDs

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 6925 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Cc: a.lauterer@proxmox.com, t.lamprecht@proxmox.com, Severen Redwood <severen.redwood@sitehost.co.nz>, Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Subject: [PATCH manager v3 1/2] close #4369: api: optionally only suggest unique IDs
Date: Tue,  5 Nov 2024 15:00:49 +1300
Message-ID: <20241105020054.215734-1-severen.redwood@sitehost.co.nz>

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 data centre config, and the previously used IDs are tracked as a
list in the file `/etc/pve/used_vmids.list`.

Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
---
There are no changes to this patch since v2.

 PVE/API2/Cluster.pm | 13 +++++++--
 PVE/Makefile        |  1 +
 PVE/UsedVmidList.pm | 70 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 PVE/UsedVmidList.pm

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index c2a7a946..a3e89484 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::UsedVmidList;
 
 use PVE::API2::ACMEAccount;
 use PVE::API2::ACMEPlugin;
@@ -866,12 +867,20 @@ __PACKAGE__->register_method({
 
 	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
 	my $next_id = $dc_conf->{'next-id'} // {};
+	my $want_unique = $dc_conf->{'unique-next-id'} // 0;
 
 	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 ($want_unique) {
+	    my $used_ids = PVE::Cluster::cfs_read_file('used_vmids.list');
+	    for (my $i = $lower; $i < $upper; $i++) {
+	        return $i if !defined($idlist->{$i}) and !defined($used_ids->{$i});
+	    }
+	} 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";
diff --git a/PVE/Makefile b/PVE/Makefile
index efcb250d..29775e78 100644
--- a/PVE/Makefile
+++ b/PVE/Makefile
@@ -15,6 +15,7 @@ PERLSOURCE = 			\
 	NodeConfig.pm		\
 	PullMetric.pm		\
 	Report.pm		\
+	UsedVmidList.pm		\
 	VZDump.pm
 
 all: pvecfg.pm $(SUBDIRS)
diff --git a/PVE/UsedVmidList.pm b/PVE/UsedVmidList.pm
new file mode 100644
index 00000000..b88a8681
--- /dev/null
+++ b/PVE/UsedVmidList.pm
@@ -0,0 +1,70 @@
+package PVE::UsedVmidList;
+
+use strict;
+use warnings;
+
+use PVE::Cluster;
+
+my $read_id_list = sub {
+    my ($filename, $raw) = @_;
+
+    return {} if !defined($raw);
+
+    my %used_ids;
+    my @lines = split(/\n/, $raw);
+    foreach my $line (@lines) {
+	if ($line =~ m/^(\d+)$/) {
+	    $used_ids{$1} = 1;
+	} elsif ($line =~ m/^(\d+)-(\d+)$/) {
+	    foreach my $id ($1..$2) {
+		$used_ids{$id} = 1;
+	    }
+	} else {
+	    warn "Skipping invalid entry in used_vmids.list: $line\n";
+	}
+    }
+
+    return \%used_ids;
+};
+
+my $write_id_list = sub {
+    my ($filename, $used_ids) = @_;
+    my @used_ids = sort {$a <=> $b} keys(%$used_ids);
+
+    my @lines;
+    my $len = scalar(@used_ids);
+    for (my $i = 0; $i < $len; $i++) {
+	my $line = "$used_ids[$i]";
+
+	my $j = $i;
+	while ($j + 1 < $len and $used_ids[$j] + 1 == $used_ids[$j + 1]) {
+	    $j++;
+	}
+
+	# If we find a range of consecutive IDs, write $ids[$i]-$ids[$j] to
+	# denote the range so that we avoid storing each individual integer.
+	if ($i != $j) {
+	    $line .= "-$used_ids[$j]";
+	}
+
+	$i = $j;
+	push(@lines, $line);
+    }
+
+    return join("\n", @lines) . "\n";
+};
+
+PVE::Cluster::cfs_register_file('used_vmids.list', $read_id_list, $write_id_list);
+
+sub add_vmid {
+    my ($vmid) = @_;
+
+    PVE::Cluster::cfs_lock_file('used_vmids.list', 10, sub {
+	my $used_ids = PVE::Cluster::cfs_read_file('used_vmids.list');
+
+	$used_ids->{$vmid} = 1;
+	PVE::Cluster::cfs_write_file('used_vmids.list', $used_ids);
+    });
+}
+
+1;
-- 
2.47.0



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4035 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Cc: a.lauterer@proxmox.com, t.lamprecht@proxmox.com, Severen Redwood <severen.redwood@sitehost.co.nz>, Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Subject: [PATCH manager v3 2/2] close #4369: ui: add datacenter option for unique VM/CT IDs
Date: Tue,  5 Nov 2024 15:00:50 +1300
Message-ID: <20241105020054.215734-2-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>
---
There are no changes to this patch since v2.

 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 b200fd12..feace344 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -339,6 +339,10 @@ Ext.define('PVE.dc.OptionView', {
 		submitValue: true,
 	    }],
 	});
+	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.0



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH container v3] api: record CT ID as used after a container is destroyed

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4204 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Cc: a.lauterer@proxmox.com, t.lamprecht@proxmox.com, Severen Redwood <severen.redwood@sitehost.co.nz>, Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Subject: [PATCH container v3] api: record CT ID as used after a container is destroyed
Date: Tue,  5 Nov 2024 15:00:51 +1300
Message-ID: <20241105020054.215734-3-severen.redwood@sitehost.co.nz>

After a container is destroyed, record that its ID has been used via the
`PVE::UsedVmidList` module so that the `/cluster/nextids` endpoint can
later optionally avoid suggesting previously used IDs.

Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
---
Changed since v2 is the addition of the use statement for `PVE::UsedVmidList`.

 src/PVE/API2/LXC.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 213e518..cc47c5c 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -28,6 +28,7 @@ use PVE::API2::LXC::Config;
 use PVE::API2::LXC::Status;
 use PVE::API2::LXC::Snapshot;
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::UsedVmidList qw(add_vmid);
 use base qw(PVE::RESTHandler);
 
 BEGIN {
@@ -794,7 +795,9 @@ __PACKAGE__->register_method({
 		}
 	    }
 
-	    # only now remove the zombie config, else we can have reuse race
+	    # only now mark the CT ID as previously used and remove the zombie
+	    # config, else we can have reuse race
+	    PVE::UsedVmidList::add_vmid($vmid);
 	    PVE::LXC::Config->destroy_config($vmid);
 	};
 
-- 
2.47.0



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH qemu-server v3] api: record VM ID as used after a virtual machine is destroyed

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4188 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Cc: a.lauterer@proxmox.com, t.lamprecht@proxmox.com, Severen Redwood <severen.redwood@sitehost.co.nz>, Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Subject: [PATCH qemu-server v3] api: record VM ID as used after a virtual machine is destroyed
Date: Tue,  5 Nov 2024 15:00:52 +1300
Message-ID: <20241105020054.215734-4-severen.redwood@sitehost.co.nz>

After a virtual machine is destroyed, record that its ID has been used
via the `PVE::UsedVmidList` module so that the `/cluster/nextids`
endpoint can later optionally avoid suggesting previously used IDs.

Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
---
Changed since v2 is the addition of the use statement for `PVE::UsedVmidList`.

 PVE/API2/Qemu.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 848001b6..8759c1e6 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -52,6 +52,7 @@ use PVE::Replication;
 use PVE::ReplicationState;
 use PVE::StorageTunnel;
 use PVE::RESTEnvironment qw(log_warn);
+use PVE::UsedVmidList qw(add_vmid);
 
 BEGIN {
     if (!$ENV{PVE_GENERATING_DOCS}) {
@@ -2335,7 +2336,9 @@ __PACKAGE__->register_method({
 		    }
 		}
 
-		# only now remove the zombie config, else we can have reuse race
+		# only now mark the VM ID as previously used and remove the
+		# zombie config, else we can have reuse race
+		PVE::UsedVmidList::add_vmid($vmid);
 		PVE::QemuConfig->destroy_config($vmid);
 	    });
 	};
-- 
2.47.0



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH cluster v3 5/6] cluster files: add used_vmids.list

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4190 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Cc: a.lauterer@proxmox.com, t.lamprecht@proxmox.com, Severen Redwood <severen.redwood@sitehost.co.nz>, Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Subject: [PATCH cluster v3 5/6] cluster files: add used_vmids.list
Date: Tue,  5 Nov 2024 15:00:53 +1300
Message-ID: <20241105020054.215734-5-severen.redwood@sitehost.co.nz>

Add `/etc/pve/used_vmids.list` to the list of cluster files, which will
be used for recording previously used VM/CT IDs. This is required so
that we can optionally ensure that such IDs are not 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>
---
There are no changes to this patch since v2.

 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 f899dbe..059c7af 100644
--- a/src/PVE/Cluster.pm
+++ b/src/PVE/Cluster.pm
@@ -84,6 +84,7 @@ my $observed = {
     'virtual-guest/profiles.cfg' => 1,
     'mapping/pci.cfg' => 1,
     'mapping/usb.cfg' => 1,
+    'used_vmids.list' => 1,
 };
 
 sub prepare_observed_file_basedirs {
diff --git a/src/pmxcfs/status.c b/src/pmxcfs/status.c
index dc44464..7f60145 100644
--- a/src/pmxcfs/status.c
+++ b/src/pmxcfs/status.c
@@ -114,6 +114,7 @@ static memdb_change_t memdb_change_array[] = {
 	{ .path = "firewall/cluster.fw" },
 	{ .path = "mapping/pci.cfg" },
 	{ .path = "mapping/usb.cfg" },
+	{ .path = "used_vmids.list" },
 };
 
 static GMutex mutex;
-- 
2.47.0



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH cluster v3 6/6] datacenter config: add unique-next-id to schema

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4057 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Cc: a.lauterer@proxmox.com, t.lamprecht@proxmox.com, Severen Redwood <severen.redwood@sitehost.co.nz>, Daniel Krambrock <krambrock@hrz.uni-marburg.de>
Subject: [PATCH cluster v3 6/6] datacenter config: add unique-next-id to schema
Date: Tue,  5 Nov 2024 15:00:54 +1300
Message-ID: <20241105020054.215734-6-severen.redwood@sitehost.co.nz>

Add the `unique-next-id` property to the datacentre 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>
---
There are no changes to this patch since v2.

 src/PVE/DataCenterConfig.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index abd0bbf..d1d4533 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -337,6 +337,11 @@ 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', format => $migration_format,
-- 
2.47.0



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH container v3] api: record CT ID as used after a container is destroyed

[Aaron Lauterer]

beside one small nit that could be cleaned up in a follow-up,

consider this:

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>


On  2024-11-05  03:00, Severen Redwood wrote:
> After a container is destroyed, record that its ID has been used via the
> `PVE::UsedVmidList` module so that the `/cluster/nextids` endpoint can
> later optionally avoid suggesting previously used IDs.
> 
> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> ---
> Changed since v2 is the addition of the use statement for `PVE::UsedVmidList`.
> 
>   src/PVE/API2/LXC.pm | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 213e518..cc47c5c 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -28,6 +28,7 @@ use PVE::API2::LXC::Config;
>   use PVE::API2::LXC::Status;
>   use PVE::API2::LXC::Snapshot;
>   use PVE::JSONSchema qw(get_standard_option);
> +use PVE::UsedVmidList qw(add_vmid);

adding the `qw(add_vmid)` here is not strictly necessary, AFAIU. We call 
the function with the full PVE::UsedVmidList:: prefix anyway.

>   use base qw(PVE::RESTHandler);
>   
>   BEGIN {
> @@ -794,7 +795,9 @@ __PACKAGE__->register_method({
>   		}
>   	    }
>   
> -	    # only now remove the zombie config, else we can have reuse race
> +	    # only now mark the CT ID as previously used and remove the zombie
> +	    # config, else we can have reuse race
> +	    PVE::UsedVmidList::add_vmid($vmid);
>   	    PVE::LXC::Config->destroy_config($vmid);
>   	};
>   



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH qemu-server v3] api: record VM ID as used after a virtual machine is destroyed

[Aaron Lauterer]

beside one small nit that could be cleaned up in a follow-up,

consider this:

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2024-11-05  03:00, Severen Redwood wrote:
> After a virtual machine is destroyed, record that its ID has been used
> via the `PVE::UsedVmidList` module so that the `/cluster/nextids`
> endpoint can later optionally avoid suggesting previously used IDs.
> 
> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> ---
> Changed since v2 is the addition of the use statement for `PVE::UsedVmidList`.
> 
>   PVE/API2/Qemu.pm | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 848001b6..8759c1e6 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -52,6 +52,7 @@ use PVE::Replication;
>   use PVE::ReplicationState;
>   use PVE::StorageTunnel;
>   use PVE::RESTEnvironment qw(log_warn);
> +use PVE::UsedVmidList qw(add_vmid);

same here as in the container, we could probably do without the qw(add_vmid)
>   
>   BEGIN {
>       if (!$ENV{PVE_GENERATING_DOCS}) {
> @@ -2335,7 +2336,9 @@ __PACKAGE__->register_method({
>   		    }
>   		}
>   
> -		# only now remove the zombie config, else we can have reuse race
> +		# only now mark the VM ID as previously used and remove the
> +		# zombie config, else we can have reuse race
> +		PVE::UsedVmidList::add_vmid($vmid);
>   		PVE::QemuConfig->destroy_config($vmid);
>   	    });
>   	};



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager v3 1/2] close #4369: api: optionally only suggest unique IDs

[Aaron Lauterer]

besides some smaller things that could be cleaned up in a follow-up,

consider this:

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2024-11-05  03:00, Severen Redwood wrote:
> 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 data centre config, and the previously used IDs are tracked as a
> list in the file `/etc/pve/used_vmids.list`.
> 
> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> ---
> There are no changes to this patch since v2.
> 
>   PVE/API2/Cluster.pm | 13 +++++++--
>   PVE/Makefile        |  1 +
>   PVE/UsedVmidList.pm | 70 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 82 insertions(+), 2 deletions(-)
>   create mode 100644 PVE/UsedVmidList.pm
> 
> diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
> index c2a7a946..a3e89484 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::UsedVmidList;
>   
>   use PVE::API2::ACMEAccount;
>   use PVE::API2::ACMEPlugin;
> @@ -866,12 +867,20 @@ __PACKAGE__->register_method({
>   
>   	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>   	my $next_id = $dc_conf->{'next-id'} // {};
> +	my $want_unique = $dc_conf->{'unique-next-id'} // 0;
>   
>   	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 ($want_unique) {
> +	    my $used_ids = PVE::Cluster::cfs_read_file('used_vmids.list');
> +	    for (my $i = $lower; $i < $upper; $i++) {
> +	        return $i if !defined($idlist->{$i}) and !defined($used_ids->{$i});
> +	    }
> +	} 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";
> diff --git a/PVE/Makefile b/PVE/Makefile
> index efcb250d..29775e78 100644
> --- a/PVE/Makefile
> +++ b/PVE/Makefile
> @@ -15,6 +15,7 @@ PERLSOURCE = 			\
>   	NodeConfig.pm		\
>   	PullMetric.pm		\
>   	Report.pm		\
> +	UsedVmidList.pm		\
>   	VZDump.pm
>   
>   all: pvecfg.pm $(SUBDIRS)
> diff --git a/PVE/UsedVmidList.pm b/PVE/UsedVmidList.pm
> new file mode 100644
> index 00000000..b88a8681
> --- /dev/null
> +++ b/PVE/UsedVmidList.pm
> @@ -0,0 +1,70 @@
> +package PVE::UsedVmidList;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Cluster;
> +
> +my $read_id_list = sub {
> +    my ($filename, $raw) = @_;
> +
> +    return {} if !defined($raw);
> +

The following block is valid perl, though a bit unusual for our 
codebase. used_ids could be defined as $used_ids = {};

> +    my %used_ids;
> +    my @lines = split(/\n/, $raw);
> +    foreach my $line (@lines) {
> +	if ($line =~ m/^(\d+)$/) {
> +	    $used_ids{$1} = 1;

Then the above line would be $used_ids->{$1}

> +	} elsif ($line =~ m/^(\d+)-(\d+)$/) {
> +	    foreach my $id ($1..$2) {
> +		$used_ids{$id} = 1;

same here

> +	    }
> +	} else {
> +	    warn "Skipping invalid entry in used_vmids.list: $line\n";
> +	}
> +    }
> +
> +    return \%used_ids;

and we don't need to manually send a reference on return but can do

return $used_ids;

> +};
> +
> +my $write_id_list = sub {
> +    my ($filename, $used_ids) = @_;
> +    my @used_ids = sort {$a <=> $b} keys(%$used_ids);
> +
> +    my @lines;
> +    my $len = scalar(@used_ids);
> +    for (my $i = 0; $i < $len; $i++) {
> +	my $line = "$used_ids[$i]";
> +
> +	my $j = $i;
> +	while ($j + 1 < $len and $used_ids[$j] + 1 == $used_ids[$j + 1]) {
> +	    $j++;
> +	}
> +
> +	# If we find a range of consecutive IDs, write $ids[$i]-$ids[$j] to
> +	# denote the range so that we avoid storing each individual integer.
> +	if ($i != $j) {
> +	    $line .= "-$used_ids[$j]";
> +	}
> +
> +	$i = $j;
> +	push(@lines, $line);
> +    }
> +
> +    return join("\n", @lines) . "\n";
> +};
> +
> +PVE::Cluster::cfs_register_file('used_vmids.list', $read_id_list, $write_id_list);
> +
> +sub add_vmid {
> +    my ($vmid) = @_;
> +
> +    PVE::Cluster::cfs_lock_file('used_vmids.list', 10, sub {
> +	my $used_ids = PVE::Cluster::cfs_read_file('used_vmids.list');
> +
> +	$used_ids->{$vmid} = 1;
> +	PVE::Cluster::cfs_write_file('used_vmids.list', $used_ids);
> +    });
> +}
> +
> +1;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH SERIES v3] Add ability to prevent suggesting previously used VM/CT IDs

[Aaron Lauterer]

gave this series another spin in my test cluster and so far it seems to 
work as described.
I did a very quick test if we see some noticeable "lag" when destroying 
a guest by filling the `used_vmids.list` with IDs from 100..30000 and a 
stepsize of 2, so close to 15k lines. But I didn't notice that it would 
take longer.

A few small style nits are addressed, in the specific patches. But 
nothing severe that would justify a v4 right away.

Consider this series:

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2024-11-05  02:58, Severen Redwood wrote:
> Hi everyone,
> 
> This is a small update to my previous patch series [1, 2]
> adding optional support for preventing PVE from suggesting
> previously used VM/CT IDs.
> 
> In particular, missing use statements have been added to the
> patches for marking VM/CT IDs as used, as noticed by Aaron
> [3]. Everything else remains the same as in v2.
> 
> Also note when building this patch series that the pve-cluster
> patches must be applied and built first to prevent the build
> of pve-manager from failing.
> 
> If anyone has any other feedback, please let me know.
> 
> Thanks,
> Severen
> 
> [1]: https://lore.proxmox.com/pve-devel/mailman.177.1728018639.332.pve-devel@lists.proxmox.com/
> [2]: https://lore.proxmox.com/pve-devel/mailman.624.1730255368.332.pve-devel@lists.proxmox.com/T/#t
> [3]: https://lore.proxmox.com/pve-devel/mailman.624.1730255368.332.pve-devel@lists.proxmox.com/T/#m7313a65f5b26f29ff0a2b4005069a7306311ccf6
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH SERIES v3] Add ability to prevent suggesting previously used VM/CT IDs

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 3681 bytes --]

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: Aaron Lauterer <a.lauterer@proxmox.com>, pve-devel@lists.proxmox.com
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [PATCH SERIES v3] Add ability to prevent suggesting previously used VM/CT IDs
Date: Fri, 8 Nov 2024 13:44:39 +1300
Message-ID: <0d5cd0df-09c9-451d-a8c3-e0a777dd783b@sitehost.co.nz>

Hi Aaron,

Thanks for the review and testing! I'll post v4 today to
address your comments and add those commit trailers.

Thanks,
Severen


[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel