From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
	by lore.proxmox.com (Postfix) with ESMTPS id 9266D1FF186
	for <inbox@lore.proxmox.com>; Tue,  5 Nov 2024 16:53:44 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id 2AF5B33242;
	Tue,  5 Nov 2024 16:53:53 +0100 (CET)
Message-ID: <e9c1096f-baaf-48db-bf5e-e7207c69e70c@proxmox.com>
Date: Tue, 5 Nov 2024 16:53:48 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Severen Redwood <severen.redwood@sitehost.co.nz>,
 pve-devel@lists.proxmox.com
References: <mailman.712.1730771900.332.pve-devel@lists.proxmox.com>
 <20241105020054.215734-1-severen.redwood@sitehost.co.nz>
From: Aaron Lauterer <a.lauterer@proxmox.com>
In-Reply-To: <20241105020054.215734-1-severen.redwood@sitehost.co.nz>
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.037 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH manager v3 1/2] close #4369: api: optionally
 only suggest unique IDs
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: t.lamprecht@proxmox.com
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.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