[pve-devel] [PATCH ha-manager 04/18] rules: resource affinity: inter-consistency check with merged positive rules

[Daniel Kral]

The resource affinity rule set is checked whether it contains pairs of
positive and negative resource affinity rules, which specify two or more
HA resources in them, and prunes them as those are infeasible.

This check has the assumption that each positive resource affinity
rule's resource set is disjoint from each other, but this is only done
in the later transformation stage when positive resource affinity with
overlapping HA resources in them are merged to one rule.

For example, the following inconsistent rules are not pruned:

- positive resource affinity rule between vm:101 and vm:102
- positive resource affinity rule between vm:102 and vm:103
- negative resource affinity rule between vm:101 and vm:103

Therefore build the same disjoint positive resource affinity resource
sets as the merge_connected_positive_resource_affinity_rules(...)
subroutine, so that the inconsistency check has the necessary
information in advance.

Fixes: 367cdbfa ("rules: introduce resource affinity rule plugin")
Reported-by: Hannes Dürr <h.duerr at proxmox.com>
Signed-off-by: Daniel Kral <d.kral at proxmox.com>
---
 src/PVE/HA/Rules/ResourceAffinity.pm          | 75 ++++---------------
 .../inconsistent-resource-affinity-rules.cfg  | 15 ++++
 ...sistent-resource-affinity-rules.cfg.expect |  5 +-
 3 files changed, 34 insertions(+), 61 deletions(-)

diff --git a/src/PVE/HA/Rules/ResourceAffinity.pm b/src/PVE/HA/Rules/ResourceAffinity.pm
index edfa4b3d..e9b368a4 100644
--- a/src/PVE/HA/Rules/ResourceAffinity.pm
+++ b/src/PVE/HA/Rules/ResourceAffinity.pm
@@ -206,14 +206,18 @@ sub check_inter_resource_affinity_rules_consistency {
 
     my @conflicts = ();
 
-    while (my ($positiveid, $positive) = each %$positive_rules) {
-        my $positive_resources = $positive->{resources};
+    my @disjoint_positive_rules =
+        PVE::HA::Rules::Helpers::find_disjoint_rules_resource_sets($positive_rules);
+
+    for my $entry (@disjoint_positive_rules) {
+        my $positive_resources = $entry->{resources};
 
         while (my ($negativeid, $negative) = each %$negative_rules) {
             my $common_resources = set_intersect($positive_resources, $negative->{resources});
             next if %$common_resources < 2;
 
-            push @conflicts, [$positiveid, $negativeid];
+            push @conflicts, ['negative', $negativeid];
+            push @conflicts, ['positive', $_] for $entry->{ruleids}->@*;
         }
     }
 
@@ -234,12 +238,15 @@ __PACKAGE__->register_check(
         my ($conflicts, $errors) = @_;
 
         for my $conflict (@$conflicts) {
-            my ($positiveid, $negativeid) = @$conflict;
+            my ($type, $ruleid) = @$conflict;
 
-            push $errors->{$positiveid}->{resources}->@*,
-                "rule shares two or more resources with a negative resource affinity rule";
-            push $errors->{$negativeid}->{resources}->@*,
-                "rule shares two or more resources with a positive resource affinity rule";
+            if ($type eq 'positive') {
+                push $errors->{$ruleid}->{resources}->@*,
+                    "rule shares two or more resources with a negative resource affinity rule";
+            } elsif ($type eq 'negative') {
+                push $errors->{$ruleid}->{resources}->@*,
+                    "rule shares two or more resources with a positive resource affinity rule";
+            }
         }
     },
 );
@@ -248,58 +255,6 @@ __PACKAGE__->register_check(
 
 =cut
 
-my $sort_by_lowest_resource_id = sub {
-    my ($rules) = @_;
-
-    my $lowest_rule_resource_id = {};
-    for my $ruleid (keys %$rules) {
-        my @rule_resources = sort keys $rules->{$ruleid}->{resources}->%*;
-        $lowest_rule_resource_id->{$ruleid} = $rule_resources[0];
-    }
-
-    # sort rules such that rules with the lowest numbered resource come first
-    my @sorted_ruleids = sort {
-        $lowest_rule_resource_id->{$a} cmp $lowest_rule_resource_id->{$b}
-    } sort keys %$rules;
-
-    return @sorted_ruleids;
-};
-
-# returns a list of hashes, which contain disjoint resource affinity rules, i.e.,
-# put resource affinity constraints on disjoint sets of resources
-my $find_disjoint_resource_affinity_rules = sub {
-    my ($rules) = @_;
-
-    my @disjoint_rules = ();
-
-    # order needed so that it is easier to check whether there is an overlap
-    my @sorted_ruleids = $sort_by_lowest_resource_id->($rules);
-
-    for my $ruleid (@sorted_ruleids) {
-        my $rule = $rules->{$ruleid};
-
-        my $found = 0;
-        for my $entry (@disjoint_rules) {
-            next if sets_are_disjoint($rule->{resources}, $entry->{resources});
-
-            $found = 1;
-            push @{ $entry->{ruleids} }, $ruleid;
-            $entry->{resources}->{$_} = 1 for keys $rule->{resources}->%*;
-
-            last;
-        }
-        if (!$found) {
-            push @disjoint_rules,
-                {
-                    ruleids => [$ruleid],
-                    resources => { $rule->{resources}->%* },
-                };
-        }
-    }
-
-    return @disjoint_rules;
-};
-
 =head3 merge_connected_positive_resource_affinity_rules($rules, $positive_rules)
 
 Modifies C<$rules> to contain only disjoint positive resource affinity rules
diff --git a/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg b/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg
index a620e293..6bfc2dad 100644
--- a/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg
+++ b/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg
@@ -1,3 +1,4 @@
+# Case 1: Remove positive and negative resource affinity rules, which share two or more ha resources.
 resource-affinity: keep-apart1
 	resources vm:102,vm:103
 	affinity negative
@@ -9,3 +10,17 @@ resource-affinity: keep-apart2
 resource-affinity: stick-together1
 	resources vm:101,vm:102,vm:103,vm:104,vm:106
 	affinity positive
+
+# Case 2: Remove positive and negative resource affinity rules, which share two or more HA resources with the positive
+#         resource affinity being split in two.
+resource-affinity: split-stick-together1
+	resources vm:201,vm:202
+	affinity positive
+
+resource-affinity: split-stick-together2
+	resources vm:202,vm:203
+	affinity positive
+
+resource-affinity: split-keep-apart1
+	resources vm:201,vm:203
+	affinity negative
diff --git a/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg.expect b/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg.expect
index d4a2d7b2..8f2338d9 100644
--- a/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg.expect
+++ b/src/test/rules_cfgs/inconsistent-resource-affinity-rules.cfg.expect
@@ -1,11 +1,14 @@
 --- Log ---
 Drop rule 'keep-apart1', because rule shares two or more resources with a positive resource affinity rule.
 Drop rule 'keep-apart2', because rule shares two or more resources with a positive resource affinity rule.
+Drop rule 'split-keep-apart1', because rule shares two or more resources with a positive resource affinity rule.
+Drop rule 'split-stick-together1', because rule shares two or more resources with a negative resource affinity rule.
+Drop rule 'split-stick-together2', because rule shares two or more resources with a negative resource affinity rule.
 Drop rule 'stick-together1', because rule shares two or more resources with a negative resource affinity rule.
 Drop rule 'stick-together1', because rule shares two or more resources with a negative resource affinity rule.
 --- Config ---
 $VAR1 = {
-          'digest' => '50875b320034d8ac7dded185e590f5f87c4e2bb6',
+          'digest' => '80cdc11a1d5bf2d1d500665af1210cd59aabede6',
           'ids' => {},
           'order' => {}
         };
-- 
2.47.2