[pve-devel] [PATCH manager] close #4369: make VMID suggestion strategy configurable

[pve-devel] [PATCH manager] close #4369: make VMID suggestion strategy configurable

[Daniel Krambrock via pve-devel]

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

From: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
To: pve-devel@lists.proxmox.com
Subject: [PATCH manager] close #4369: make VMID suggestion strategy configurable
Date: Wed, 22 May 2024 14:05:50 +0200
Message-ID: <20240522120553.49114-2-krambrock@hrz.uni-marburg.de>

VMID suggestion strategy is set in datacenter config

Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
---
 PVE/API2/Cluster.pm           | 16 +++++++++--
 PVE/Makefile                  |  1 +
 PVE/UsedVmidList.pm           | 53 +++++++++++++++++++++++++++++++++++
 www/manager6/dc/OptionView.js | 14 +++++++++
 4 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 04387ab4..be79ddf0 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,23 @@ __PACKAGE__->register_method({
 
 	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
 	my $next_id = $dc_conf->{'next-id'} // {};
+	my $strategy = $dc_conf->{'next-id-strategy'} // "next-free";
 
 	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 ($strategy eq "next-free") {
+	    for (my $i = $lower; $i < $upper; $i++) {
+	        return $i if !defined($idlist->{$i});
+	    }
+	} elsif ($strategy eq "max+1") {
+	    return %$idlist ? (List::Util::max(keys %$idlist) + 1) : $lower
+	} elsif ($strategy eq "list") {
+	    for (my $i = $lower; $i < $upper; $i++) {
+	        return $i if (!defined($idlist->{$i}) and !PVE::UsedVmidList::is_on_vmid_list($i)) ;
+	    }
+	} else {
+	    die "unable to get any free VMID with strategy $strategy\n";
 	}
 
 	die "unable to get any free VMID in range [$lower, $upper]\n";
diff --git a/PVE/Makefile b/PVE/Makefile
index 660de4d0..fe627296 100644
--- a/PVE/Makefile
+++ b/PVE/Makefile
@@ -14,6 +14,7 @@ PERLSOURCE = 			\
 	Jobs.pm			\
 	NodeConfig.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..a60c75d6
--- /dev/null
+++ b/PVE/UsedVmidList.pm
@@ -0,0 +1,53 @@
+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", sort @data);
+};
+
+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');
+
+	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;
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index b200fd12..613a2183 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -339,6 +339,20 @@ Ext.define('PVE.dc.OptionView', {
 		submitValue: true,
 	    }],
 	});
+	me.add_combobox_row('next-id-strategy', gettext('Next Free VMID Strategy'), {
+	    renderer: v => {
+		if (v === '__default__') {return Proxmox.Utils.defaultText;}
+	        return v;
+	    },
+	    comboItems: [
+		['__default__', Proxmox.Utils.defaultText + ' (next-free)'],
+		['next-free', 'next-free'],
+		['max+1', 'max+1'],
+		['list', 'list'],
+	    ],
+	    defaultValue: '__default__',
+	    deleteEmpty: true,
+	});
 	me.rows['tag-style'] = {
 	    required: true,
 	    renderer: (value) => {
-- 
2.39.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] close #4369: make VMID suggestion strategy configurable

[Daniel Krambrock via pve-devel]

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

From: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
To: pve-devel@lists.proxmox.com
Subject: [PATCH cluster] close #4369: make VMID suggestion strategy configurable
Date: Wed, 22 May 2024 14:05:51 +0200
Message-ID: <20240522120553.49114-3-krambrock@hrz.uni-marburg.de>

Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
---
 src/PVE/Cluster.pm          | 1 +
 src/PVE/DataCenterConfig.pm | 9 +++++++++
 2 files changed, 10 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/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index abd0bbf..1394623 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -337,6 +337,15 @@ my $datacenter_schema = {
 	    format => $next_id_format,
 	    description => "Control the range for the free VMID auto-selection pool.",
 	},
+	'next-id-strategy' => {
+	    optional => 1,
+	    type => 'string',
+            enum => ['next-free', 'max+1', 'list'],
+            default => 'next-free',
+            description => "VMID allocation strategie. 'next-free' returns the lowerst free ID. "
+                ."'max+1' returns the biggerst used ID + 1. "
+                ."'list' returns the lowerst free ID that is not on the list of previously used IDs.",
+	},
 	migration => {
 	    optional => 1,
 	    type => 'string', format => $migration_format,
-- 
2.39.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] close #4369: make VMID suggestion strategy configurable

[Daniel Krambrock via pve-devel]

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

From: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
To: pve-devel@lists.proxmox.com
Subject: [PATCH container] close #4369: make VMID suggestion strategy configurable
Date: Wed, 22 May 2024 14:05:52 +0200
Message-ID: <20240522120553.49114-4-krambrock@hrz.uni-marburg.de>

on container deletion the VMID is added to the list of previously used
VMIDs

Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
---
 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 138288c..f71f005 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -799,6 +799,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.39.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] close #4369: make VMID suggestion strategy configurable

[Daniel Krambrock via pve-devel]

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

From: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
To: pve-devel@lists.proxmox.com
Subject: [PATCH qemu] close #4369: make VMID suggestion strategy configurable
Date: Wed, 22 May 2024 14:05:53 +0200
Message-ID: <20240522120553.49114-5-krambrock@hrz.uni-marburg.de>

On guest deletion the VMID is added to the list of previously used
VMIDs

Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
---
 PVE/API2/Qemu.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 2a1d4d7..c24b317 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -2313,6 +2313,7 @@ __PACKAGE__->register_method({
 	    });
 	};
 
+	PVE::UsedVmidList::add_vmid($vmid);
 	return $rpcenv->fork_worker('qmdestroy', $vmid, $authuser, $realcmd);
     }});
 
-- 
2.39.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] (no subject)

[Shannon Sterz]

On Wed May 22, 2024 at 2:05 PM CEST, Daniel Krambrock wrote:
> This series of patches let the user choose a VMID suggestion strategy
> to avoid the recycled VMID problem.

Hi Daniel!

Thanks for your contribution! First of some top-level feedback:

- Please familiarize yourself with the Developer Documentation [1], Perl
  [2], and JavaScript [3] style guides before contributing.
- Most importantly, if you haven't already, please contact
  `office@proxmox.com` about signing the Harmony CLA [4].
- All your patches provide the same git short message. Please provide
  more specific context for each patch and not just the the overall goal
  of your series.

> Default is 'next-free', the previous strategy where the suggested VMID
> is the lowest not used ID in range. This reuses VMIDs.
> Added options are:
> - 'max-1': selects the highest existing VMID and adds 1

I'm guessing you typo-ed the `max-1` here, as the other patches seem to
correctly use `max+1`. Please note that such a strategy is still prone
to re-use of IDs if one where to remove the VM with the highest ID.

> - 'list': returns the lowest free VMID within the specified
>   range that is not on a list of previously used VMIDs
> On guest deletion the VMID is added to '/etc/pve/used_vmids.list'

Not sure if tracking the used VM/CT IDs in a separate file is the most
elegant solution here. Especially as this is a somewhat niche usecase.

Anyway, I'll provide some more detailed feedback in-line.

Kind regards, Shannon

PS: Please use "reply-all" when replying to me, so the full discussion
stays on the mailing list. Thanks!




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

Re: [pve-devel] [PATCH manager] close #4369: make VMID suggestion strategy configurable

[Shannon Sterz]

This is a fairly large patch. It could easily be split into a backend
and front-end portion. That would make it easier to review, in my
opinion.

On Wed May 22, 2024 at 2:05 PM CEST, Daniel Krambrock wrote:
> VMID suggestion strategy is set in datacenter config
>
> Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> ---
>  PVE/API2/Cluster.pm           | 16 +++++++++--
>  PVE/Makefile                  |  1 +
>  PVE/UsedVmidList.pm           | 53 +++++++++++++++++++++++++++++++++++
>  www/manager6/dc/OptionView.js | 14 +++++++++
>  4 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 04387ab4..be79ddf0 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,23 @@ __PACKAGE__->register_method({
>
>  	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>  	my $next_id = $dc_conf->{'next-id'} // {};
> +	my $strategy = $dc_conf->{'next-id-strategy'} // "next-free";
>
>  	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 ($strategy eq "next-free") {
> +	    for (my $i = $lower; $i < $upper; $i++) {
> +	        return $i if !defined($idlist->{$i});
> +	    }
> +	} elsif ($strategy eq "max+1") {
> +	    return %$idlist ? (List::Util::max(keys %$idlist) + 1) : $lower
> +	} elsif ($strategy eq "list") {
> +	    for (my $i = $lower; $i < $upper; $i++) {
> +	        return $i if (!defined($idlist->{$i}) and !PVE::UsedVmidList::is_on_vmid_list($i)) ;

Nit: Unnecessary space before semicolon.

> +	    }
> +	} else {
> +	    die "unable to get any free VMID with strategy $strategy\n";
>  	}
>
>  	die "unable to get any free VMID in range [$lower, $upper]\n";
> diff --git a/PVE/Makefile b/PVE/Makefile
> index 660de4d0..fe627296 100644
> --- a/PVE/Makefile
> +++ b/PVE/Makefile
> @@ -14,6 +14,7 @@ PERLSOURCE = 			\
>  	Jobs.pm			\
>  	NodeConfig.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..a60c75d6
> --- /dev/null
> +++ b/PVE/UsedVmidList.pm
> @@ -0,0 +1,53 @@
> +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", sort @data);
> +};
> +
> +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');
> +
> +	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;
> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
> index b200fd12..613a2183 100644
> --- a/www/manager6/dc/OptionView.js
> +++ b/www/manager6/dc/OptionView.js
> @@ -339,6 +339,20 @@ Ext.define('PVE.dc.OptionView', {
>  		submitValue: true,
>  	    }],
>  	});
> +	me.add_combobox_row('next-id-strategy', gettext('Next Free VMID Strategy'), {
> +	    renderer: v => {
> +		if (v === '__default__') {return Proxmox.Utils.defaultText;}
> +	        return v;

Nit: The indetation here seems off, both lines should be indented with
two tabs.

Also, please note that our JS style guide says you should avoid single
line if statements in new code.

> +	    },
> +	    comboItems: [
> +		['__default__', Proxmox.Utils.defaultText + ' (next-free)'],
> +		['next-free', 'next-free'],
> +		['max+1', 'max+1'],
> +		['list', 'list'],
> +	    ],
> +	    defaultValue: '__default__',
> +	    deleteEmpty: true,
> +	});
>  	me.rows['tag-style'] = {
>  	    required: true,
>  	    renderer: (value) => {



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

Re: [pve-devel] [PATCH cluster] close #4369: make VMID suggestion strategy configurable

[Shannon Sterz]

On Wed May 22, 2024 at 2:05 PM CEST, Daniel Krambrock wrote:
> Signed-off-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> ---
>  src/PVE/Cluster.pm          | 1 +
>  src/PVE/DataCenterConfig.pm | 9 +++++++++
>  2 files changed, 10 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/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
> index abd0bbf..1394623 100644
> --- a/src/PVE/DataCenterConfig.pm
> +++ b/src/PVE/DataCenterConfig.pm
> @@ -337,6 +337,15 @@ my $datacenter_schema = {
>  	    format => $next_id_format,
>  	    description => "Control the range for the free VMID auto-selection pool.",
>  	},
> +	'next-id-strategy' => {
> +	    optional => 1,
> +	    type => 'string',
> +            enum => ['next-free', 'max+1', 'list'],
> +            default => 'next-free',
> +            description => "VMID allocation strategie. 'next-free' returns the lowerst free ID. "
> +                ."'max+1' returns the biggerst used ID + 1. "
> +                ."'list' returns the lowerst free ID that is not on the list of previously used IDs.",

Nit: Some spell-checking

- strategy, not strategie
- lowest, not lowerst
- biggest, not biggerst

> +	},
>  	migration => {
>  	    optional => 1,
>  	    type => 'string', format => $migration_format,



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