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

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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Subject: [PATCH manager 1/2] close #4369: api: optionally only suggest unique IDs
Date: Fri, 27 Sep 2024 01:52:28 +1200
Message-ID: <20240926135516.117065-2-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>
---
 PVE/API2/Cluster.pm | 12 ++++++++--
 PVE/Makefile        |  1 +
 PVE/UsedVmidList.pm | 55 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100644 PVE/UsedVmidList.pm

diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 04387ab4..304b5595 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;
@@ -813,12 +814,19 @@ __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) {
+	    for (my $i = $lower; $i < $upper; $i++) {
+	        return $i if !defined($idlist->{$i}) and !PVE::UsedVmidList::is_on_vmid_list($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..c1751729
--- /dev/null
+++ b/PVE/UsedVmidList.pm
@@ -0,0 +1,55 @@
+package PVE::UsedVmidList;
+
+use strict;
+use warnings;
+
+use PVE::Cluster;
+
+my $parse_vmid_list = sub {
+    my ($filename, $raw) = @_;
+
+    return [] if !defined($raw);
+
+    my @parsed;
+    my @lines = split(/\n/, $raw);
+    foreach my $line (@lines) {
+	next if $line =~ m/^\s*$/;
+
+	if ($line =~ m/^(\d+)$/) {
+	    push(@parsed, $1);
+	} else {
+	    warn "Skipping invalid used_vmids.list entry: $line\n";
+	}
+    }
+
+    return \@parsed;
+};
+
+my $write_vmid_list = sub {
+    my ($filename, @data) = @_;
+
+    return join("\n", @data) . "\n";
+};
+
+PVE::Cluster::cfs_register_file('used_vmids.list', $parse_vmid_list, $write_vmid_list);
+
+sub add_vmid {
+    my ($vmid) = @_;
+
+    PVE::Cluster::cfs_lock_file('used_vmids.list', 10, sub {
+	my $vmid_list = PVE::Cluster::cfs_read_file('used_vmids.list');
+
+	if (!is_on_vmid_list($vmid)) {
+	    push(@$vmid_list, $vmid);
+	    PVE::Cluster::cfs_write_file('used_vmids.list', join("\n", @$vmid_list));
+	}
+    });
+}
+
+sub is_on_vmid_list {
+    my ($vmid) = @_;
+    my $vmid_list = PVE::Cluster::cfs_read_file('used_vmids.list');
+    return scalar(grep { $_ == $vmid } @$vmid_list);
+}
+
+1;
-- 
2.46.2



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

[Dietmar Maurer]

The format of the used_vmids.list is simply, but can lead to
a very large file over time (we want to avoid large files on /etc/pev/).

>    PVE::Cluster::cfs_write_file('used_vmids.list', join("\n", @$vmid_list));

A future version could compress that list, by using integer ranges,
for example:

---used_vmids.list---
100-10005
10007-20000
---------------------

I guess this would be simple to implement...


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

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

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH manager 1/2] close #4369: api: optionally only suggest unique IDs
Date: Sun, 29 Sep 2024 13:47:31 +0000
Message-ID: <bd449ad74b19fc183d83d2d67b02bfc4afe486e1.camel@groupe-cyllene.com>

Hi,

I'm very interested by this patch series too.

My 2 cents:

Couldn't we simply move the deleted vm config file
to a trash/tombstone directory ?

/etc/pve/.deleted/<vmid>.conf ?


(I could be great to be able to mass delete vms in // without having a
big lock on a file)


I'm not sure about read performance with a lof of delete vms ?


Regards,

Alexandre


-------- Message initial --------
De: Dietmar Maurer <dietmar@proxmox.com>
Répondre à: Proxmox VE development discussion <pve-
devel@lists.proxmox.com>
À: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Objet: Re: [pve-devel] [PATCH manager 1/2] close #4369: api: optionally
only suggest unique IDs
Date: 26/09/2024 17:34:20

The format of the used_vmids.list is simply, but can lead to
a very large file over time (we want to avoid large files on
/etc/pev/).

>    PVE::Cluster::cfs_write_file('used_vmids.list', join("\n",
> @$vmid_list));

A future version could compress that list, by using integer ranges,
for example:

---used_vmids.list---
100-10005
10007-20000
---------------------

I guess this would be simple to implement...


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://antiphishing.vadesecure.com/v4?f=S084UmJ0TUJ5SGx4M2JIcAIhGX6X6z
EreV6yzjRX6e4emC-ysDzTtLPBgM9ZbyIXhW74ybnKUou-
3S_OXWnGoQ&i=QlRib2gzNE4zbnc2V0tXWXbojbkfbDhF08DP0Pbwl7A&k=txWj&r=OHc5N
2VwYkhyTWJ3YXNsN2-
dQK1UWYRKaljQfJwVQ6Ar59VbBlnwTPhyqMnvPxOp&s=ceea9f4110a5b090c1585cc8c70
3e460dc77ba643b2d94451299a18d4b7dc18d&u=https%3A%2F%2Flists.proxmox.com
%2Fcgi-bin%2Fmailman%2Flistinfo%2Fpve-devel



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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,  Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
Subject: Re: [pve-devel] [PATCH manager 1/2] close #4369: api: optionally only suggest unique IDs
Date: Mon, 30 Sep 2024 14:58:51 +1300
Message-ID: <ce6ov3znxweopubjnsw35m5aiboohi2r5yza76brljruxyhbmx@wbnvnhtmvq4v>

On Sun, Sep 29, 2024 at 01:47:31PM GMT, DERUMIER, Alexandre via pve-devel wrote:
> Couldn't we simply move the deleted vm config file
> to a trash/tombstone directory ?
> 
> /etc/pve/.deleted/<vmid>.conf ?
> 
> 
> (I could be great to be able to mass delete vms in // without having a
> big lock on a file)

This is an interesting idea, though I think creating an empty file is
probably better than moving the config as that would only store
information that will never be needed again. Needing to acquire a lock
on the list when deleting multiple VMs may be a bottleneck, but I'm not
entirely convinced it would actually be a problem in practice. How many
VMs do you expect to be deleting all at once? And how often?

This approach would also use more storage as you now have the overhead
of FS metadata for every single ID you have marked as used.

Dietmar, what do you think is the best option here? I'm personally
leaning towards using the list with your run-length encoding suggestion,
but I'm open to implementing Alexandre's idea if you think it's worth
it.


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

[Dietmar Maurer]

> This approach would also use more storage as you now have the overhead
> of FS metadata for every single ID you have marked as used.
> 
> Dietmar, what do you think is the best option here? I'm personally
> leaning towards using the list with your run-length encoding suggestion,
> but I'm open to implementing Alexandre's idea if you think it's worth
> it.

The pmxcfs filesystem has limits, and I do not really want to waste space for such things. I would prefer the run-length encoded list.

@Alexandre: Why do you want to keep a backup of old config files?


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

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

[DERUMIER, Alexandre via pve-devel]

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

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "dietmar@proxmox.com" <dietmar@proxmox.com>, "severen.redwood@sitehost.co.nz" <severen.redwood@sitehost.co.nz>
Subject: Re: [pve-devel] [PATCH manager 1/2] close #4369: api: optionally only suggest unique IDs
Date: Mon, 30 Sep 2024 08:06:34 +0000
Message-ID: <e9570961e343c6a605e626bc53107c8c4be58fdf.camel@groupe-cyllene.com>

>>The pmxcfs filesystem has limits, and I do not really want to waste
>>space for such things. I would prefer the run-length encoded list.

>>@Alexandre: Why do you want to keep a backup of old config files?

Don't need content of old config. ( I have backup anyway).

I was more thinking about something "atomic" delete, like a simple
vmid.conf move could be done, where we can do a lot of delete in //
without need to lock a global variable.

To be honest, I don't known enough the internal of pxmcfs to known what
is the best method.

My main problems currently is limitation of // for vm create/delete
when we create vms with automation scripts.




Talking about unique ID, any plan (longterm) to implement (optionnal)
uuid one day ? (I known that it's a lot of work, but it could be
usefull to be able to share storage between differents cluster, and be
able to move vms between them without need to rewrite volume datas with
different id)



-------- Message initial --------
De: Dietmar Maurer <dietmar@proxmox.com>
À: Severen Redwood <severen.redwood@sitehost.co.nz>, Proxmox VE
development discussion <pve-devel@lists.proxmox.com>, Alexandre
Derumier <alexandre.derumier@groupe-cyllene.com>
Objet: Re: [pve-devel] [PATCH manager 1/2] close #4369: api: optionally
only suggest unique IDs
Date: 30/09/2024 09:50:25

> This approach would also use more storage as you now have the
> overhead
> of FS metadata for every single ID you have marked as used.
> 
> Dietmar, what do you think is the best option here? I'm personally
> leaning towards using the list with your run-length encoding
> suggestion,
> but I'm open to implementing Alexandre's idea if you think it's worth
> it.

The pmxcfs filesystem has limits, and I do not really want to waste
space for such things. I would prefer the run-length encoded list.

@Alexandre: Why do you want to keep a backup of old config files?



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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Subject: [PATCH manager 2/2] close #4369: ui: add datacenter option for unique VM/CT IDs
Date: Fri, 27 Sep 2024 01:52:29 +1200
Message-ID: <20240926135516.117065-3-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>
---
 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.46.2



[-- 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] api: record CT ID as used after a container is destroyed

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Subject: [PATCH container] api: record CT ID as used after a container is destroyed
Date: Fri, 27 Sep 2024 01:52:30 +1200
Message-ID: <20240926135516.117065-4-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>
---
 src/PVE/API2/LXC.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 918e719..c4cc427 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -800,6 +800,7 @@ __PACKAGE__->register_method({
 
 	my $realcmd = sub { PVE::LXC::Config->lock_config($vmid, $code); };
 
+	PVE::UsedVmidList::add_vmid($vmid);
 	return $rpcenv->fork_worker('vzdestroy', $vmid, $authuser, $realcmd);
     }});
 
-- 
2.46.2



[-- 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] api: record VM ID as used after a virtual machine is destroyed

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Subject: [PATCH qemu-server] api: record VM ID as used after a virtual machine is destroyed
Date: Fri, 27 Sep 2024 01:52:31 +1200
Message-ID: <20240926135516.117065-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>
---
 PVE/API2/Qemu.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index d25a79fe..67a6191f 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -2340,6 +2340,7 @@ __PACKAGE__->register_method({
 	    });
 	};
 
+	PVE::UsedVmidList::add_vmid($vmid);
 	return $rpcenv->fork_worker('qmdestroy', $vmid, $authuser, $realcmd);
     }});
 
-- 
2.46.2



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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Subject: [PATCH cluster 1/2] cluster files: add used_vmids.list
Date: Fri, 27 Sep 2024 01:52:32 +1200
Message-ID: <20240926135516.117065-6-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>
---
 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.46.2



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

[Severen Redwood via pve-devel]

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

From: Severen Redwood <severen.redwood@sitehost.co.nz>
To: pve-devel@lists.proxmox.com
Subject: [PATCH cluster 2/2] datacenter config: add unique-next-id to schema
Date: Fri, 27 Sep 2024 01:52:33 +1200
Message-ID: <20240926135516.117065-7-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>
---
 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.46.2



[-- 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] api: record CT ID as used after a container is destroyed

[Thomas Lamprecht]

Am 26/09/2024 um 15:52 schrieb Severen Redwood:
> 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>
> ---
>  src/PVE/API2/LXC.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 918e719..c4cc427 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -800,6 +800,7 @@ __PACKAGE__->register_method({
>  
>  	my $realcmd = sub { PVE::LXC::Config->lock_config($vmid, $code); };
>  
> +	PVE::UsedVmidList::add_vmid($vmid);

at this point the CT is not yet destroyed, only the worker that tries to destroy
it is started, destruction can still fail.

It'd be better to place it inside the $realcmd, or even the $code, ideally
before the actual unlink of the vmid.conf file, as after that happens the
ID gets free again from the POV of the pmxcfs and thus pve-cluster's VMID
list that is used by the next-id call.

>  	return $rpcenv->fork_worker('vzdestroy', $vmid, $authuser, $realcmd);
>      }});
>  



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

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

[Severen Redwood via pve-devel]

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

From: "Severen Redwood" <severen.redwood@sitehost.co.nz>
To: "Thomas Lamprecht" <t.lamprecht@proxmox.com>, <pve-devel@lists.proxmox.com>
Subject: Re: [PATCH container] api: record CT ID as used after a container is destroyed
Date: Mon, 30 Sep 2024 13:24:36 +1300
Message-ID: <D4J6W8WNZVVO.I4L8Y84TQEFT@sitehost.co.nz>

On Fri Sep 27, 2024 at 5:48 AM NZST, Thomas Lamprecht wrote:
> at this point the CT is not yet destroyed, only the worker that tries to destroy
> it is started, destruction can still fail.
>
> It'd be better to place it inside the $realcmd, or even the $code, ideally
> before the actual unlink of the vmid.conf file, as after that happens the
> ID gets free again from the POV of the pmxcfs and thus pve-cluster's VMID
> list that is used by the next-id call.

Thanks for pointing that out! Upon review, that is definitely the wrong
place to add the CT ID to the list. I'll move the call to
`PVE::UsedVmidList::add_vmid` to sit just before the call to
`PVE::LXC::Config->destroy_config` in the `$code` subroutine instead,
which I think is in the vein of your suggestion.


[-- 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 qemu-server] api: record VM ID as used after a virtual machine is destroyed

[Thomas Lamprecht]

Am 26/09/2024 um 15:52 schrieb Severen Redwood:
> 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>
> ---
>  PVE/API2/Qemu.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d25a79fe..67a6191f 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -2340,6 +2340,7 @@ __PACKAGE__->register_method({
>  	    });
>  	};
>  
> +	PVE::UsedVmidList::add_vmid($vmid);

same here, you write "after a virtual machine is destroyed", but this is quite
a bit before that as the worker might need quite a bit of time to finish, and
that can even fail.

While it's not really causing a issue with recording the VMID as reserved,
that should be evaluated explicitly and mentioned in the commit message.

>  	return $rpcenv->fork_worker('qmdestroy', $vmid, $authuser, $realcmd);
>      }});
>  



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