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

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

Am 01.10.26 um 2:04 PM schrieb Fiona Ebner:
> 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.

Sorry, ignore this one please. It does already die in that case.

> 
> 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.

And this is also a wrong comment I forgot to delete before sending, but
the name could be made $next_id_opts

> 
>> +    # 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);
> 
> 
> 
> 
>