From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 8A8161FF0AA for ; Tue, 22 Sep 2026 17:02:39 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 6A8A92144E; Tue, 22 Sep 2026 17:02:36 +0200 (CEST) Message-ID: Date: Tue, 22 Sep 2026 17:02:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH many v5 0/7] add option to prevent suggesting previously used VMIDs To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , pve-devel@lists.proxmox.com References: <20260921155410.938337-1-m.koeppl@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260921155410.938337-1-m.koeppl@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790089345121 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.527 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: XDMYJAJAX6UYKTZNOKPAFFLP3BIUGS64 X-Message-ID-Hash: XDMYJAJAX6UYKTZNOKPAFFLP3BIUGS64 X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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.