From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id C0EB21FF0AB for ; Wed, 23 Sep 2026 10:14:07 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 9EF9C2156F; Wed, 23 Sep 2026 10:14:04 +0200 (CEST) Message-ID: <7cabe74c-bd98-49cd-b671-9899dcab281f@proxmox.com> Date: Wed, 23 Sep 2026 10:13:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH many v5 0/7] add option to prevent suggesting previously used VMIDs From: Fiona Ebner 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 In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790151239977 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.520 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: LJ2G2SG4WN2Z2L4IANLGX6QLQQUECZOB X-Message-ID-Hash: LJ2G2SG4WN2Z2L4IANLGX6QLQQUECZOB 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 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.