Re: [PATCH many v5 0/7] add option to prevent suggesting previously used VMIDs

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

Am 22.09.26 um 5:02 PM schrieb Fiona Ebner:
> Am 21.09.26 um 5:54 PM schrieb Michael Köppl:
>> This is based on the original series [0] by Severen Redwood and Daniel
>> Krambrock. It it rebased on the latest master branches and incorporates
>> the feedback from v4.
>>
>> I kept the trailers from the original series where I made only minimal
>> changes, but added markers (MK: ...) in those cases. For bigger
>> changes, I changed the `Co-authored-by` and `Signed-off-by` trailers to
>> `Signed-off-by`. I removed Aaron's T-b and R-b trailers since it's been
>> a long time and the trailers might be misleading considering the series
>> has changed in some of its implementation details overall.
>>
>> Changes since v4:
>> - File in pmxcfs has been renamed to used-guest-ids to make it a bit
>>   more general (both VMs and CTs) while still making clear which IDs
>>   are meant.
>> - Moved the `UsedVmidList` module from pve-manager to pve-guest-common
>>   and renamed it `UsedGuestIDs`. Also renamed some of the functions
>>   there for more consistent naming.
>> - Implemented `write_id_list` more closely aligned with Fabian's
>>   feedback.
>> - Also record used VMIDs when creating, cloning, destroying guests as
>>   well as incoming remote migrations. (Thanks, @Fiona, for the
>>   off-list input and feedback!)
>> - Moved the `add_id` calls for recording used IDs to before anything
>>   is persisted during any of the above actions.
>> - Rephrased some commit messages.
>>
>> [0] https://lore.proxmox.com/pve-devel/mailman.62.1731030290.372.pve-devel@lists.proxmox.com/
> 
> Looks good in general, some nits and some high-level comments in the
> individual patches. I do feel like the suggested change in 3/7 for
> tracking ranges instead of constructing a hash with potentially millions
> of entries might be worth it, especially since it happens for each guest
> that is created and only ever gets more expensive otherwise.
> 
> LLM automated review also has some comments and they look valid to me,
> so I'm mentioning them here:
> 
> 1. It would be good to record the needed package dependencies in the
> cover letter (and optionally individual patches), to ease a maintainers
> job when applying.
> 
> 2. Recording every second ID can lead to a file larger than the 1 MiB
> pmxcfs limit and then every operation will fail regardless of whether
> the feature is turned on or not.
> 
> 3. Remote migration uses destroy_{lxc_container,vm} directly if the
> 'delete' parameter is set.
> 
> For the second point, an example is:
> [I] root@pve9a1 ~# expr $(seq 100 2 1000000 | wc --bytes) / 1024 / 1024
> 3
> This is unlikely to happen with regular operation of course, because we
> always suggest the lowest free one, but it can't be ruled out. Not
> entirely sure what to do about it, but it would be good if we can avoid
> the issue. Maybe start dropping gaps and merging anyways if we get close
> to the limit? At the very least, we might consider making the failure
> non-critical if the feature is turned off.

Similarly for exhaustion (which is even less likely to happen in
practice, but still): if the feature is turned off, it should not lead
to failure.

Another way other than auto-closing gaps (if going for that, we can of
course choose the smallest one each time) would be to enforce picking an
adjacent ID if we are getting close to the limit. But I somehow feel
like auto-closing gaps would be preferable from a user perspective. What
do you think? In both cases, we could start warning that this will
happen at some earlier size threshold.

> While looking through the last point, I also noticed that 'qm importovf'
> is still a place where we create a new config, so should be recording
> the new ID. Maybe we even want to use make sure all places use the
> create_and_lock_config() method and do it at the beginning of that? That
> might be the cleanest and most future-proof.