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

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

[Severen Redwood via pve-devel]

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

From: "Severen Redwood" <severen.redwood@sitehost.co.nz>
To: <pve-devel@lists.proxmox.com>
Cc: <a.lauterer@proxmox.com>, <t.lamprecht@proxmox.com>
Subject: [PATCH SERIES v4] Add ability to prevent suggesting previously used VM/CT IDs
Date: Fri, 08 Nov 2024 14:44:39 +1300
Message-ID: <D5GF0S3A2B5A.2GFUV82K7VL1@sitehost.co.nz>

Hi everyone,

This is another small update to my previous patch series [1]
adding optional support for preventing PVE from suggesting
previously used VM/CT IDs. Aaron had some small style nits, so
I've addressed those and also added the Reviewed-by and
Tested-by commit trailers he offered.

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.

Thanks,
Severen

[1]: https://lore.proxmox.com/pve-devel/mailman.716.1730772112.332.pve-devel@lists.proxmox.com/T/#u


[-- 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 v4 1/2] close #4369: api: optionally only suggest unique IDs

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 7086 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 v4 1/2] close #4369: api: optionally only suggest unique IDs
Date: Fri,  8 Nov 2024 14:46:15 +1300
Message-ID: <20241108014620.73352-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>
Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Changed since v3 is addressing some comments on style in `$read_id_list`.

 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..0d0a885d
--- /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

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

[Fabian Grünbichler]

> Severen Redwood via pve-devel <pve-devel@lists.proxmox.com> hat am 08.11.2024 02:46 CET geschrieben:

Hi!

sorry it took so long to get back at this series! next will be faster :)

> 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>
> Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
> Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> Changed since v3 is addressing some comments on style in `$read_id_list`.
> 
>  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');

if we initialize this with a blank hash if $want_unique is not set, then we could just have a single loop below, which might be a bit easier to read?

> +	    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..0d0a885d
> --- /dev/null
> +++ b/PVE/UsedVmidList.pm
> @@ -0,0 +1,70 @@
> +package PVE::UsedVmidList;

this doesn't work in pve-manager, since you want to use/call this module in pve-container/qemu-server and would end up having a circular dependency. it could either move to pve-cluster, or to pve-guest-common (the latter would be my preference, since it is guest-specific).

> +
> +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) {

do we want to catch malformed files here where $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);
> +    }

I think this can be written in a simpler fashion, if you just remember whether you are currently in a range or not, a single iteration over the sorted keys should work?

something like (not tested ;))

my $start = 0; # start of (potential) range
my $last = 0; # end of (potential) range
my $output = ''; # accumulated output lines
for my $curr (sort keys $used_ids->%*) {
    # first slot
    if (!$start) {
        $start = $last = $curr;
        next;
    }

    # extend range by one slot
    if ($last + 1 == $curr) {
        $last = $curr;
        next;
    }

    # write entry
    if ($start == $last) {
        $output .= "$start\n";
    } else {
        $output .= "start-$last\n";
    }

    # start new entry
    $start = last = $curr;
}

if ($start == $last) {
    $output .= "$start\n";
} else {
    $output .= "start-$last\n";
}

return $output;

alternatively, we could also not expand the ranges when parsing, not sure whether that would make things easier or more complicated though..

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


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

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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>, "Thomas Lamprecht" <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH manager v4 1/2] close #4369: api: optionally only suggest unique IDs
Date: Wed, 26 Mar 2025 16:50:40 +1300
Message-ID: <e20ee17b-8109-465e-b693-eebd37e17b46@sitehost.co.nz>

On 28/01/2025 01:16, Fabian Grünbichler wrote:
> Hi!
> 
> sorry it took so long to get back at this series! next will be faster :)

No worries! As you can see it took me a while to get back to this too... I've implemented all the suggestions around style and will post the updated patches soon.

> this doesn't work in pve-manager, since you want to use/call this module in pve-container/qemu-server and would end up having a circular dependency. it could either move to pve-cluster, or to pve-guest-common (the latter would be my preference, since it is guest-specific).

Thanks for catching this! I've moved it to a separate commit for pve-guest-common.

> alternatively, we could also not expand the ranges when parsing, not sure whether that would make things easier or more complicated though..

I tried that at one point, but found it greatly complicated the code for minimal gain considering that using a hash map should be fast enough in practice. For example, 100,000 recorded IDs are handled with no perceivable difference in performance compared to an empty ID list.



[-- 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 v4 2/2] close #4369: ui: add datacenter option for unique VM/CT IDs

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4179 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 v4 2/2] close #4369: ui: add datacenter option for unique VM/CT IDs
Date: Fri,  8 Nov 2024 14:46:16 +1300
Message-ID: <20241108014620.73352-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>
Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
There are no changes to this patch since v3.

 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 cluster v4 1/2] cluster files: add used_vmids.list

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4313 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 v4 1/2] cluster files: add used_vmids.list
Date: Fri,  8 Nov 2024 14:46:17 +1300
Message-ID: <20241108014620.73352-3-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>
Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
There are no changes to this patch since v3.

 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

Re: [pve-devel] [PATCH cluster v4 1/2] cluster files: add used_vmids.list

[Fabian Grünbichler]

> Severen Redwood via pve-devel <pve-devel@lists.proxmox.com> hat am 08.11.2024 02:46 CET geschrieben:

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

this is encroaching bikeshed-territory, but we don't currently use ".list" as an extension in /etc/pve (and I automatically have to think of APT repository files when seeing it ;)). I think a plain "used_vmids" or "used-vmids" would also work?

maybe somebody else has an opinion or suggestion?

> 
> Co-authored-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
> Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> There are no changes to this patch since v3.
> 
>  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


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

Re: [pve-devel] [PATCH cluster v4 1/2] cluster files: add used_vmids.list

[Thomas Lamprecht]

Am 27.01.25 um 13:18 schrieb Fabian Grünbichler:
>> Severen Redwood via pve-devel <pve-devel@lists.proxmox.com> hat am 08.11.2024 02:46 CET geschrieben:
> 
>> 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.
> 
> this is encroaching bikeshed-territory, but we don't currently use ".list" as an extension in /etc/pve (and I automatically have to think of APT repository files when seeing it ;)). I think a plain "used_vmids" or "used-vmids" would also work?
> 
> maybe somebody else has an opinion or suggestion?

I agree, and I would prefer kebab-case in any way, i.e.: used-vmids

Besides that it mostly depends on if we want to re-use a common file for other
such specific use cases around VMIDs like per-{user,group,pool} reservations
(or allow-lists) of VMID ranges.

One certainly does not need to implement that for landing this series, but
might be worth use a slightly more structured format and generalized file
name to allow easier extension in that direction without new files and
only needing to look at a single file when allocating VMID.
But this can also be adapted on applying, it should mostly affect parsing
and serialization, and maybe those even not _that_ much.


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

Re: [pve-devel] [PATCH cluster v4 1/2] cluster files: add used_vmids.list

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: "Thomas Lamprecht" <t.lamprecht@proxmox.com>, "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Subject: Re: [pve-devel] [PATCH cluster v4 1/2] cluster files: add used_vmids.list
Date: Wed, 26 Mar 2025 16:51:23 +1300
Message-ID: <fd441572-6c7c-4b62-bdef-0d738ac699fb@sitehost.co.nz>

On 29/01/2025 03:57, Thomas Lamprecht wrote:
> Am 27.01.25 um 13:18 schrieb Fabian Grünbichler:
>>> Severen Redwood via pve-devel <pve-devel@lists.proxmox.com> hat am 08.11.2024 02:46 CET geschrieben:
>>
>>> 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.
>>
>> this is encroaching bikeshed-territory, but we don't currently use ".list" as an extension in /etc/pve (and I automatically have to think of APT repository files when seeing it ;)). I think a plain "used_vmids" or "used-vmids" would also work?
>>
>> maybe somebody else has an opinion or suggestion?
> 
> I agree, and I would prefer kebab-case in any way, i.e.: used-vmids

No problem, I've changed it to `/etc/pve/used-ids` now. Slightly more generic than `used-vmids` since there are also container IDs.

> Besides that it mostly depends on if we want to re-use a common file for other
> such specific use cases around VMIDs like per-{user,group,pool} reservations
> (or allow-lists) of VMID ranges.
> 
> One certainly does not need to implement that for landing this series, but
> might be worth use a slightly more structured format and generalized file
> name to allow easier extension in that direction without new files and
> only needing to look at a single file when allocating VMID.
> But this can also be adapted on applying, it should mostly affect parsing
> and serialization, and maybe those even not _that_ much.
I've not done this just yet since I'm not sure what sort of format you would prefer. Possibly some kind of section headers to separate out the different parts of the file? But maybe it's best to leave that for when the patches are applied.


[-- 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 cluster v4 1/2] cluster files: add used_vmids.list

[Thomas Lamprecht]

Am 26.03.25 um 04:51 schrieb Severen Redwood:
> No problem, I've changed it to `/etc/pve/used-ids` now. Slightly more generic than `used-vmids` since there are also container IDs.

Hmm, I see where you come from, but that's IMO making it a bit overly
generic, there are many IDs in a Proxmox VE system.

So let's go for something a bit more specific while still ensuring it's
not suggesting that this is purely for VMs, like e.g. `used-guest-ids`
as we normally use virtual guest in places where we talk about both, CTs
and VMs.

> I've not done this just yet since I'm not sure what sort of format you would prefer. Possibly some kind of section headers to separate out the different parts of the file? But maybe it's best to leave that for when the patches are applied.

Hmm, rethinking this now it might be not worth the uncertain future planning.
Let's focus on guest IDs and keep this file purely for that feature.
If needed, we can do a bigger change on a major release, while that's not trivial,
its manageable enough with some cut-offs, alternatively using additional files
can be fine too.


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

Re: [pve-devel] [PATCH cluster v4 1/2] cluster files: add used_vmids.list

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>, Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH cluster v4 1/2] cluster files: add used_vmids.list
Date: Fri, 28 Mar 2025 10:02:23 +1300
Message-ID: <441cec36-60f0-45ab-841f-e85d2949a063@sitehost.co.nz>

On 26/03/2025 20:22, Thomas Lamprecht wrote:
> Am 26.03.25 um 04:51 schrieb Severen Redwood:
>> No problem, I've changed it to `/etc/pve/used-ids` now. Slightly more generic than `used-vmids` since there are also container IDs.
> 
> Hmm, I see where you come from, but that's IMO making it a bit overly
> generic, there are many IDs in a Proxmox VE system.
> 
> So let's go for something a bit more specific while still ensuring it's
> not suggesting that this is purely for VMs, like e.g. `used-guest-ids`
> as we normally use virtual guest in places where we talk about both, CTs
> and VMs.

This makes sense to me. I will post the updated patches today with that filename :)




[-- 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 v4 2/2] datacenter config: add unique-next-id to schema

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4180 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 v4 2/2] datacenter config: add unique-next-id to schema
Date: Fri,  8 Nov 2024 14:46:18 +1300
Message-ID: <20241108014620.73352-4-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>
Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
There are no changes to this patch since v3.

 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

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

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4309 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 v4] api: record VM ID as used after a virtual machine is destroyed
Date: Fri,  8 Nov 2024 14:46:19 +1300
Message-ID: <20241108014620.73352-5-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>
Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Changed since v3 is removing an unnecessary `qw(add_vmid)` after
`use 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..22bc58d6 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;
 
 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 container v4] api: record CT ID as used after a container is destroyed

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4325 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 v4] api: record CT ID as used after a container is destroyed
Date: Fri,  8 Nov 2024 14:46:20 +1300
Message-ID: <20241108014620.73352-6-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>
Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Changed since v3 is removing an unnecessary `qw(add_vmid)` after
`use 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..e35b26d 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;
 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

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

[Fabian Grünbichler]

> Severen Redwood via pve-devel <pve-devel@lists.proxmox.com> hat am 08.11.2024 02:46 CET geschrieben:
> 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>
> Tested-by: Aaron Lauterer <a.lauterer@proxmox.com>
> Reviewed-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> Changed since v3 is removing an unnecessary `qw(add_vmid)` after
> `use 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..e35b26d 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;
>  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);

if we only add it when the VM is removed via the API, we might miss those where an admin just does "rm /etc/pve/../XXX.conf" (which might happen if something is blocking regular removal)? wouldn't it be safer to add them when creating the VM? or potentially in both cases to increase the likelihood of a VM being registered?

alternatively, a call to nextid already has both lists and could warn if there is a mismatch or even fix it up? i.e., if we don't only add on removal, but upon creation, then the intersection of currently used VMIDs and previously used VMIDs should be identical to the currently used VMIDs. if not, then any missing ones can be added/registered?

all of this obviously applies to containers and VMs..

>  	    PVE::LXC::Config->destroy_config($vmid);
>  	};
>  
> -- 
> 2.47.0


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

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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH container v4] api: record CT ID as used after a container is destroyed
Date: Wed, 26 Mar 2025 16:51:13 +1300
Message-ID: <b9175549-036c-4b9f-bdcd-3b585f9e26ac@sitehost.co.nz>

On 28/01/2025 01:16, Fabian Grünbichler wrote:
> if we only add it when the VM is removed via the API, we might miss those where an admin just does "rm /etc/pve/../XXX.conf" (which might happen if something is blocking regular removal)? wouldn't it be safer to add them when creating the VM? or potentially in both cases to increase the likelihood of a VM being registered?
> 
> alternatively, a call to nextid already has both lists and could warn if there is a mismatch or even fix it up? i.e., if we don't only add on removal, but upon creation, then the intersection of currently used VMIDs and previously used VMIDs should be identical to the currently used VMIDs. if not, then any missing ones can be added/registered?
> 
> all of this obviously applies to containers and VMs..

This is a good point. I've implemented your first suggestion for a start and tried to position the calls to `add_id` where I think they would make sense (namely just after the config file is created for creating/cloning and just after it is deleted for destroying), but of course I don't have the most intimate knowledge of PVE. Please let me know what you think once I post the updated patch series :)



[-- 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 SERIES v4] Add ability to prevent suggesting previously used VM/CT IDs

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 3697 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: Re: [pve-devel] [PATCH SERIES v4] Add ability to prevent suggesting previously used VM/CT IDs
Date: Wed, 4 Dec 2024 10:43:59 +1300
Message-ID: <d39f8d05-d5a6-48c9-996a-dca751f62ff3@sitehost.co.nz>

Hi everyone,

Just bumping this patch series since it's been a few weeks :)

Is there anything blocking these patches from being accepted?
I'm happy to address any feedback if so.

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

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

[Severen Redwood via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 4236 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: Re: [pve-devel] [PATCH SERIES v4] Add ability to prevent suggesting previously used VM/CT IDs
Date: Mon, 20 Jan 2025 11:01:07 +1300
Message-ID: <da02576a-d8ac-4e40-b28f-7e1833fbae0e@sitehost.co.nz>

Hi everyone,

Hope you've all had a great break over the holidays.

I don't want to be a pain, but just bumping this again as it's nearing
a couple months since there was movement on this patch series. I received
a Reviewed-by trailer from Aaron, so it seems that these patches are
something you are willing to eventually accept? As always, I'm happy to
address any further feedback.

Thanks,
Severen

On 04/12/2024 10:43, Severen Redwood wrote:
> Hi everyone,
> 
> Just bumping this patch series since it's been a few weeks :)
> 
> Is there anything blocking these patches from being accepted?
> I'm happy to address any feedback if so.
> 
> 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