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 760FF1FF0E6 for ; Fri, 24 Jul 2026 14:04:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EB6E521492; Fri, 24 Jul 2026 14:04:14 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 24 Jul 2026 14:03:39 +0200 Message-Id: From: "Daniel Kral" To: "Thomas Lamprecht" , Subject: Re: applied: [PATCH-SERIES docs/ha-manager/manager v3 00/16] Negative Node Affinity Rules X-Mailer: aerc 0.21.0-177-g483b89d2ba6d-dirty References: <20260722084824.108350-1-d.kral@proxmox.com> <178484416713.2171173.16396343676880035118.b4-ty@b4> In-Reply-To: <178484416713.2171173.16396343676880035118.b4-ty@b4> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784894589086 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.219 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_LOW -0.7 Sender listed at https://www.dnswl.org/, low 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: SI3WYE4NCTVK7VKFCZCZNLYU62445MLL X-Message-ID-Hash: SI3WYE4NCTVK7VKFCZCZNLYU62445MLL X-MailFrom: d.kral@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: On Fri Jul 24, 2026 at 12:02 AM CEST, Thomas Lamprecht wrote: > Applied, thanks!=20 > > I added a few fixes on top of manager and ha-manager, would be nice if yo= u > could recheck them. The HA manager ones: > > - The "negative rule covers all cluster nodes" check compared node counts= , > which only holds while the rule's nodes are a subset of the cluster. A = rule > still naming a since-removed node inflates the count and gets dropped e= ven > though its complement is non-empty. It now takes the actual complement = via > set_difference(), like the inversion transform already does. That's a very good catch, thank you for adding the fix-ups! > > - The consistency check between negative resource affinity and node affin= ity > rules runs before the negative->positive inversion, so it read a negati= ve > rule's nodes as the allowed set and rejected feasible setups (e.g. keep= vm:1 > and vm:2 apart, both off node1, on three nodes). It now uses the comple= ment > there too; added a test for the feasible and the still-infeasible case. Here as well, I'm sorry that I've missed it since I've gone through the checks here, but unfortunately it slipped right past me.. The changes looks good to me though and there are no other global checks that need special cases for negative node affinity rules currently, because 1. in check_single_priority_node_affinity_in_resource_affinity_rules the same checks are true for negative node affinity rules as for those non-zero priority classes aren't allowed anyway 2. in check_single_node_affinity_per_positive_resource_affinity_rule it is also true as the strict/non-strict option in negative node affinity rules doesn't allow for a simple interpretation of multiple node affinity rules existing either > > Plus a trivial cleanup dropping a redundant node-set copy in the inversio= n. > > The root cause seems to be that negative rules only get inverted after al= l > checks ran, so every inter-plugin check has to special-case them. Longer= term > it'd be probably cleaner to invert right after the negative-specific chec= ks and > before the general ones, so downstream only ever sees positive rules and = this > whole class of "check saw the avoided set" issues can't happen. Needs a b= it of > check/transform phase reordering though, better done on its own, so that'= s why > I took that in as is with the smaller targeted follow-up. I have considered to interleave checks with transformations in the past for these patches [0] [1] as we need to special case positive resource affi= nity rules already as well, but I scratched the idea before sending those tree's patches and went with introducing a helper for these special cases [2]. I didn't do the interleave for those patches around [0] and [1], because I only considered arbitrary interleaving then, which is rather messy and costly for doing quick checks for the API routes. Another smaller issue was to properly map the transformed rules back to the original rules so that the users (and the web interface dialog) could identify the rules which caused any troubles. However, I think we could go forward with a fixed pipeline with a pre-check-transformation pass and a post-check-transformation pass, where it's much more straightforward to know which logic to run for the check_feasibility() and for the additional transform() and compile(). That could make those special cases for positive resource affinity rules and negative node affinity rules nicer to work with in the end. I'm prioritizing the work on making the CRM/LRM aware of non-HA resources and everything that goes with that change currently, but I can certainly send a patch series for this if there's no other takers in the mean time. [0] https://git.proxmox.com/?p=3Dpve-ha-manager.git;a=3Dcommitdiff;h=3De79f= 4b4ae436c1a26238a4585e68c53886169ba7 [1] https://git.proxmox.com/?p=3Dpve-ha-manager.git;a=3Dcommitdiff;h=3D4f09= 820fa23aa417dc15070bc2aaa50859c855f1 [2] https://git.proxmox.com/?p=3Dpve-ha-manager.git;a=3Dcommitdiff;h=3D0f63= 09d4330a9b86cf6ed57d0398277a0075076a > > Manager were mostly smaller issues/nits, see git. Thanks for those as well, will keep browser compatibility better in check for future patches. The approach with using the select listener is also much nicer than doing the selection inversion on apply from the two-way binding of the useNodePriority property.