[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 at 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) => {