Re: [pve-devel] [PATCH ha-manager 00/18] HA rules fixes + cleanup + performance improvements

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

Tested this series by:
- Creating rules with resources that were set to ignored, checked if the
  resources are not shown as dependent resources when migrating
- Also tested what happens when the resource state was recently changed.
  Noticed that when changing from 'started' to 'ignored', it can take a
  few seconds before the UI reflects that (e.g. when using the migrate
  dialog). This isn't unexpected, but wanted to note it nonetheless.
- Checking that conflicts between positive resource affinity rules and
  node affinity rules are now detected correctly that weren't detected
  before
- Manually created various combinations of node affinity and resource
  affinity rules and checked that they are applied
- Also re-ran some group-to-rules migrations, just to be sure

I did not notice any problems apart from the minor delay in the UI noted
above. The rules are more robust now and the changes solve some problems
that I had encountered before.

I also had a closer look at the code and left some comments on the
individual patches, but nothing major. I had a particularly close look
at the compiled rules to be certain that they produce the expected
structure.

With my comments on the individual patches addressed, please consider
this series

Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Michael Köppl <m.koeppl@proxmox.com>


On Thu Aug 21, 2025 at 4:35 PM CEST, Daniel Kral wrote:
> I put the patches in decreasing priority:
>
> PATCH 1    fix output of get_resource_info() about dependent resources
> PATCH 2    fix retranslating ha rules on nodelist changes
> PATCH 3-5  fix wrong assumption about positive resource affinity checks
> PATCH 6-18 ha cleanup and performance improvements
>
>
> Ad PATCH 1, 2, 3-5:
>
> The first few patches fix some wrong assumptions and bugs.
>
>
> Ad PATCH 3-5:
>
> During the initial HA rules implementation there were quite a few
> changes done to how rules were checked and translated, which moved the
> merging of positive resource affinity rules to the end of the pipeline.
>
> Not taking notice of this clearly enough, this causes two checks to have
> wrong assumptions about positive positive resource affinity rules: they
> assume that the resource sets of positive resource affinity rules are
> already disjoint from each other, even though that is only done at a
> later stage.
>
> As it would be rather cumbersome to interleave checks/pruning infeasible
> rules and rule transforms [0] (i.e. move the merging transform inbetween
> the previous resource affinity checks and the inter-consistency check; I
> tried that but it looked rather ugly), the best method IMO was to use
> the already existing helper to find these disjoint sets. This also
> provides these checks with the correct positive resource affinity rule
> ids to blame instead of building any extra logic for handling that if
> checks/transforms would've been interleaved.
>
>
> Ad PATCH 6-18:
>
> These patches are dependent on PATCH 2 so I put these changes in one
> patch series rather than referencing them from each other.
>
> After the fixes, the patch series contains a few patches to clean up
> obvious code smells or things I wanted to differently but didn't have
> the time to do when it was time for the release. The major thing here is
> that the HA rules are compiled into a more compact representation now,
> which are used by the scheduler and other users to be more efficient.
> Otherwise, larger rule sets could slow down the HA Manager
> significantly, because it's harder to traverse the rules hash (e.g. find
> a ha resource's rule(s)) instead of directly accessing it in a more
> compact hash (i.e. $affinity->{$sid}) for example.
>
> Some of these changes will also make implementing some feature
> enhancements such as migration blockers for node affinity rules and
> negative node affinity rules easier.
>
>
> Daniel Kral (18):
>   config: do not add ignored resources to dependent resources
>   manager: retranslate rules if nodes are added or removed
>   rules: factor out disjoint rules' resource set helper
>   rules: resource affinity: inter-consistency check with merged positive
>     rules
>   rules: add merged positive resource affinity info in global checks
>   rules: make rules sorting optional in foreach_rule helper
>   rename rule's canonicalize stage to transform stage
>   rules: make plugins register transformers instead of plugin_transform
>   rules: node affinity: decouple get_node_affinity helper from Usage
>     class
>   compile ha rules to a more compact representation
>   test: rules: use to_json instead of Data::Dumper for config output
>   test: rules: add compiled config output to rules config test cases
>   rules: node affinity: define node priority outside hash access
>   move minimum version check helper to ha tools
>   manager: move group migration cooldown variable into helper
>   api: status: sync active service counting with lrm's helper
>   manager: group migration: sync active service counting with lrm's
>     helper
>   factor out counting of active services into helper
>
>  debian/pve-ha-manager.install                 |   1 +
>  src/PVE/API2/HA/Rules.pm                      |   1 +
>  src/PVE/API2/HA/Status.pm                     |  17 +-
>  src/PVE/HA/Config.pm                          |  18 +-
>  src/PVE/HA/LRM.pm                             |  20 +-
>  src/PVE/HA/Manager.pm                         |  86 ++--
>  src/PVE/HA/NodeStatus.pm                      |  14 +
>  src/PVE/HA/Rules.pm                           | 170 ++++++--
>  src/PVE/HA/Rules/Helpers.pm                   |  61 +++
>  src/PVE/HA/Rules/Makefile                     |   2 +-
>  src/PVE/HA/Rules/NodeAffinity.pm              |  90 ++--
>  src/PVE/HA/Rules/ResourceAffinity.pm          | 200 ++++-----
>  src/PVE/HA/Tools.pm                           |  52 +++
>  ...efaults-for-node-affinity-rules.cfg.expect | 145 ++++---
>  ...lts-for-resource-affinity-rules.cfg.expect |  87 ++--
>  ...onsistent-node-resource-affinity-rules.cfg |  19 +
>  ...nt-node-resource-affinity-rules.cfg.expect | 185 ++++++---
>  .../inconsistent-resource-affinity-rules.cfg  |  15 +
>  ...sistent-resource-affinity-rules.cfg.expect |  21 +-
>  ...egative-resource-affinity-rules.cfg.expect |  73 ++--
>  ...fective-resource-affinity-rules.cfg.expect |  18 +-
>  ...egative-resource-affinity-rules.cfg.expect | 342 +++++++++------
>  ...ositive-resource-affinity-rules.cfg.expect | 391 +++++++++++++-----
>  ...egative-resource-affinity-rules.cfg.expect | 186 +++++----
>  ...ositive-resource-affinity-rules.cfg.expect | 264 +++++++++---
>  ...ty-with-resource-affinity-rules.cfg.expect | 114 +++--
>  ...rce-refs-in-node-affinity-rules.cfg.expect | 200 ++++++---
>  src/test/test_failover1.pl                    |  10 +-
>  src/test/test_rules_config.pl                 |  14 +-
>  29 files changed, 1838 insertions(+), 978 deletions(-)
>  create mode 100644 src/PVE/HA/Rules/Helpers.pm



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel