Re: [PATCH many v4 00/39] notifications: add nested match expressions

["Michael Köppl" <m.koeppl@proxmox.com>]

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öppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>

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 series
> 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 the new

[snip]