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 <[email protected]> Tested-by: Michael Köppl <[email protected]> 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 [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
