Re: [PATCH guest-common v6 09/18] guest id: optionally enforce the next-id range and uniqueness

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

Am 24.09.26 um 6:16 PM schrieb Michael Köppl:
> If the 'enforce' subproperty of next-id is enabled, ensure that only
> guest IDs from the configured range and, if unique is enabled as well,
> only previously unused IDs can be used. By setting existing => 1, the
> enforcement of these criteria is disabled for actions on existing
> guests, such as destroy, remote migration with --delete, and restore
> over an existing guest.
> 
> IDs are recorded before a guest exists and stay recorded if its
> creation fails later. With both 'enforce' and 'unique' set, retrying a
> failed creation with the same ID is therefore rejected.
> 
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
>  src/PVE/AbstractConfig.pm |  4 ++--
>  src/PVE/GuestID.pm        | 49 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
> index 229e4bd..2f20596 100644
> --- a/src/PVE/AbstractConfig.pm
> +++ b/src/PVE/AbstractConfig.pm
> @@ -260,9 +260,9 @@ sub create_and_lock_config {
>          $vmid,
>          5,
>          sub {
> -            PVE::Cluster::check_vmid_unused($vmid, $allow_existing);
> +            my $is_new = PVE::Cluster::check_vmid_unused($vmid, $allow_existing);
>  
> -            PVE::GuestID::register_used_id($vmid);
> +            PVE::GuestID::register_used_id($vmid, { existing => !$is_new });
>  
>              my $conf = eval { $class->load_config($vmid) } || {};
>              $class->check_lock($conf);
> diff --git a/src/PVE/GuestID.pm b/src/PVE/GuestID.pm
> index 4ab6f2d..02abf24 100644
> --- a/src/PVE/GuestID.pm
> +++ b/src/PVE/GuestID.pm
> @@ -187,26 +187,63 @@ my sub insert_id($ranges, $id) {
>      return 1;
>  }
>  
> -sub register_used_id($id) {
> -    cfs_lock_file(
> +my sub check_range($next_id, $id) {

assert_ rather than check_ since it dies

I'd also go for something more descriptive like assert_id_in_next_id_range

> +    my $lower = $next_id->{lower};
> +    my $upper = $next_id->{upper};
> +
> +    die "guest ID $id is below the lower boundary $lower of the next-id range\n"
> +        if defined($lower) && $id < $lower;
> +    die "guest ID $id is not below the upper boundary $upper of the next-id range\n"
> +        if defined($upper) && $id >= $upper;
> +}
> +
> +# Dies if the next-id datacenter option enforces its range or uniqueness
> +# and a new guest must not use $id. Existing guests are not considered.
> +sub check_enforced_id($id) {

Similarly here, maybe assert_id_satisfies_next_id_settings?

Nit: adding this helper could be its own commit.

> +    my $next_id = cfs_read_file('datacenter.cfg')->{'next-id'} // {};

As already noted in patch 4/18, I feel like we should abort if we can't
read the file, since it might mean violating enforce+unique otherwise.

Nit: maybe $next_id_opts to avoid potential ambiguity?

> +    return if !$next_id->{enforce};
> +
> +    check_range($next_id, $id);
> +
> +    if ($next_id->{unique}) {
> +        my $ranges = cfs_read_file($FILENAME);
> +        die "guest ID $id was used before\n" if next_unused($ranges, $id) != $id;
> +    }
> +}
> +
> +# Records $id as used. If the next-id datacenter option enforces its
> +# range or uniqueness, IDs a new guest must not use are rejected, unless
> +# $opts->{existing} marks $id as belonging to an existing guest.
> +sub register_used_id($id, $opts = {}) {
> +    my $next_id = cfs_read_file('datacenter.cfg')->{'next-id'} // {};

Same as above with respect to dying.

> +    # never reject an existing guest
> +    my $enforce = $next_id->{enforce} && !$opts->{existing};
> +
> +    check_range($next_id, $id) if $enforce;
> +
> +    my $recorded = cfs_lock_file(
>          $FILENAME,
>          10,
>          sub {
>              my $ranges = cfs_read_file($FILENAME);
>  
> -            return if !insert_id($ranges, $id);
> +            return 0 if !insert_id($ranges, $id);
>  
>              cfs_write_file($FILENAME, $ranges);
> +            return 1;
>          },
>      );
>  
>      if (my $err = $@) {
> -        my $dc_conf = cfs_read_file('datacenter.cfg');
> -
>          my $emsg = "unable to record guest ID $id as used";
> -        die "$emsg - $err" if $dc_conf->{'next-id'}->{unique};
> +        die "$emsg - $err" if $next_id->{unique};
>          warn "$emsg - $err";
> +
> +        return;
>      }
> +
> +    die "guest ID $id was used before\n"
> +        if $enforce && $next_id->{unique} && !$recorded;
>  }
>  
>  cfs_register_file($FILENAME, \&parse_id_list, \&write_id_list);