> Severen Redwood via pve-devel <pve-devel@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 <krambr...@hrz.uni-marburg.de>
> Signed-off-by: Severen Redwood <severen.redw...@sitehost.co.nz>
> Tested-by: Aaron Lauterer <a.laute...@proxmox.com>
> Reviewed-by: Aaron Lauterer <a.laute...@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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
