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

[Aaron Lauterer <a.lauterer@proxmox.com>]

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