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 DD5721FF0C0 for ; Wed, 21 Oct 2026 16:40:39 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id F352E215FA; Wed, 21 Oct 2026 16:40:31 +0200 (CEST) Content-Type: text/plain; charset=UTF-8 Date: Wed, 21 Oct 2026 16:40:19 +0200 Message-Id: From: =?utf-8?q?Michael_K=C3=B6ppl?= To: "Lukas Wagner" , , Subject: Re: [PATCH many v4 00/39] notifications: add nested match expressions Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.22.0 References: <20261012131413.314088-1-l.wagner@proxmox.com> In-Reply-To: <20261012131413.314088-1-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792593619810 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.234 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: 234FKI5ZDI23K63D73YQ4EGPCUHC7EBM X-Message-ID-Hash: 234FKI5ZDI23K63D73YQ4EGPCUHC7EBM X-MailFrom: m.koeppl@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: Thanks for your hard work on this! Tested this version of the series as well. Upgrade path: - Create multiple legacy matchers to check that the conversion works as expected: - all mode with match field and match severity - any mode with inverted match - matching multiple fields with exact or regex - "at least one does not match" with calendar and severity - no clauses but inverted (always matching) - calendar match with "mon8-12" (no space). - The above were all converted as expected. The only noteworthy conversion is the one with the "mon8-12" calendar match. The UI when does not display any selected days and saving is blocked because "Calendar rules must match at least one day." Partial upgrade, 1 node not upgraded, - Attempted to edit an existing legacy matcher from an upgraded node. Was rejected as expected. - Attempted to edit a target with and without the default-matcher present in the config. Noticed that when the legacy default-matcher is not written to the config yet, editing the notification target is not possible, failing with "At least one cluster node does not support matcher expressions". Probably because the implementation wants to write the new default-matcher with the expression to the config and performing the version check before. If the legacy default-matcher is already present, editing the target works. - On any not-yet-upgraded node, editing targets and matchers continues to work. Fully upgraded cluster, UI and API tests: - Through the API/pvesh, adding expressions with both exact matches and regex is possible, resulting in an expression containing only whichever came first in the expression being stored. Noted in on the patch that implements this behavior. Not a blocker IMO, but wanted to point it out nonetheless. - Through the API/pvesh, field names that would previously have been rejected by the legacy matchers are now accepted (e.g. spaces, umlauts, etc.). Added a comment to the individual patch, but could also be a follow-up IMO. - Played around with a match expression with variations of nested not(not(...)) and compared the resulting tree in the UI to logically match. - Edited the tree in the UI so see if children of expression nodes were moved as expected, if inversions are applied correctly, and if nodes cannot be dropped to the root level. - Changed "All sub-rules match" in the UI tree to "Match Severity". This makes all the children disappear without any confirmation dialog. The entire editing process can of course be aborted to avoid this, but considering the deletion of the "All sub-rules match" requires confirmation, I think this case would also benefit from such a confirmation. Can easily be a follow-up, though. This is also true when changing the root node. - Triggered various notifications, checking that the expected matchers matched. Other than what's noted above on the individual scenarios, the new match expressions as well as upgrading from the legacy ones worked as expected and IMO the UI is really intuitive and solid regarding what it allows. Really nice work! With my comments addressed, consider this: Reviewed-by: Michael K=C3=B6ppl Tested-by: Michael K=C3=B6ppl On Mon Oct 12, 2026 at 3:13 PM CEST, Lukas Wagner wrote: > NOTE: The last patch for pve-manager needs to be updated depending on > which version of pve-manager includes these series! (see patch note) > > NOTE: I have a couple of other patches planned for the notification stack= (**); > so I guess it would make sense wait a bit before applying this -- just to= avoid > too many version bumps and too much maintainer churn. I have a patch seri= es > already in the works that builds on top of this one, so I would prefer to > perform any non-trivial changes to this feature as follow-ups; rebasing t= he new [snip]