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

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

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.

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.