[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 at 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 at hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redwood at sitehost.co.nz>
> Tested-by: Aaron Lauterer <a.lauterer at proxmox.com>
> Reviewed-by: Aaron Lauterer <a.lauterer at 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