Re: [PATCH pve-guest-common v5 3/7] add module to track previously used guest IDs

[Fiona Ebner <f.ebner@proxmox.com>]

Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
> The /cluster/nextid API endpoint always suggests the lowest free
> guest ID, so IDs of destroyed guests get handed out again. Offering
> an opt-out requires recording every ID that has ever been in use
> somewhere both the endpoint and the guest create and destroy paths
> can reach.
> 
> Store them in /etc/pve/used-guest-ids and register it as a cfs file,
> so the list is kept in sync cluster-wide. Consecutive IDs are
> collapsed into "<start>-<end>" ranges on write to keep the file small
> on clusters that churn through many guests, and expanded back into a
> hash on read. Entries that do not parse, ranges with reversed bounds
> included, are skipped with a warning instead of failing the whole
> read.
> 
> Originally-by: Daniel Krambrock <krambrock@hrz.uni-marburg.de>
> Originally-by: Severen Redwood <severen.redwood@sitehost.co.nz>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/Makefile            |  1 +
>  src/PVE/UsedGuestIDs.pm | 93 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 src/PVE/UsedGuestIDs.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index 030e7f7..83e5ceb 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -14,6 +14,7 @@ install: PVE
>  	install -m 0644 PVE/Replication.pm ${PERL5DIR}/PVE/
>  	install -m 0644 PVE/StorageTunnel.pm ${PERL5DIR}/PVE/
>  	install -m 0644 PVE/Tunnel.pm ${PERL5DIR}/PVE/
> +	install -m 0644 PVE/UsedGuestIDs.pm ${PERL5DIR}/PVE/
>  	install -d ${PERL5DIR}/PVE/Mapping
>  	install -m 0644 PVE/Mapping/Dir.pm ${PERL5DIR}/PVE/Mapping/
>  	install -m 0644 PVE/Mapping/PCI.pm ${PERL5DIR}/PVE/Mapping/
> diff --git a/src/PVE/UsedGuestIDs.pm b/src/PVE/UsedGuestIDs.pm
> new file mode 100644
> index 0000000..31036ff
> --- /dev/null
> +++ b/src/PVE/UsedGuestIDs.pm
> @@ -0,0 +1,93 @@
> +package PVE::UsedGuestIDs;

Nit: maybe GuestID as a more general name? That would suggest moving or
adding other stuff related to guest IDs here in the future. Otherwise,
the module is quite specific. We would need to rename add_id() to
add_used_id(), read_list() -> list_used_ids() for clarity then.

> +
> +use strict;
> +use warnings;

Nit: I now this got picked up from before, but it would be nice to use
v5.36; and function signatures for new modules.

> +
> +use PVE::Cluster qw(
> +    cfs_lock_file
> +    cfs_read_file
> +    cfs_register_file
> +    cfs_write_file
> +);
> +
> +my $FILENAME = 'used-guest-ids';
> +
> +my $parse_id_list = sub {

Nit: If writing with a signature, use 'my sub', and similarly for the
other two private functions.

> +    my ($filename, $raw) = @_;
> +
> +    my $used_ids = {};
> +
> +    return $used_ids if !defined($raw);
> +
> +    for my $line (split(/\n/, $raw)) {
> +        next if $line =~ m/^\s*$/;
> +
> +        if ($line =~ m/^(\d+)$/) {
> +            $used_ids->{$1} = 1;
> +        } elsif ($line =~ m/^(\d+)-(\d+)$/) {
> +            my ($start, $end) = ($1, $2);
> +            if ($start > $end) {
> +                warn "skipping reversed range in $filename: $line\n";
> +                next;
> +            }
> +            $used_ids->{$_} = 1 for $start .. $end;

Nit: we could be smarter and avoid adding every single ID to the hash
(with a lot of (past) guests that can become a non-trivial cost), and
instead track ranges. Instead of having a public read_list() function
then, there could be a check_id_unused() function. When writing out the
file we can still merge adjacent ranges.

> +        } else {
> +            warn "skipping invalid entry in $filename: $line\n";
> +        }
> +    }
> +
> +    return $used_ids;
> +};
> +
> +my $format_entry = sub {
> +    my ($start, $last) = @_;
> +    return $start == $last ? "$start\n" : "$start-$last\n";
> +};
> +
> +my $write_id_list = sub {
> +    my ($filename, $used_ids) = @_;
> +
> +    my $output = '';
> +    my ($start, $last);
> +
> +    for my $curr (sort { $a <=> $b } keys $used_ids->%*) {
> +        if (!defined($start)) {
> +            ($start, $last) = ($curr, $curr);
> +        } elsif ($last + 1 == $curr) {
> +            $last = $curr;
> +        } else {
> +            $output .= $format_entry->($start, $last);
> +            ($start, $last) = ($curr, $curr);
> +        }
> +    }
> +
> +    $output .= $format_entry->($start, $last) if defined($start);
> +
> +    return $output;
> +};
> +
> +sub read_list {
> +    return cfs_read_file($FILENAME);
> +}
> +
> +sub add_id {
> +    my ($id) = @_;
> +
> +    cfs_lock_file(
> +        $FILENAME,
> +        10,
> +        sub {
> +            my $used_ids = cfs_read_file($FILENAME);
> +
> +            return if $used_ids->{$id};
> +
> +            $used_ids->{$id} = 1;
> +            cfs_write_file($FILENAME, $used_ids);
> +        },
> +    );
> +    die $@ if $@;
> +}
> +
> +cfs_register_file($FILENAME, $parse_id_list, $write_id_list);
> +
> +1;