[PATCH manager 1/2] fix #7589: rules: allow setting false 'disable' parameter

[PATCH manager 1/2] fix #7589: rules: allow setting false 'disable' parameter

[Dominik Rusovac]

Allow setting 'disabled' parameter of rule to false via API.

Rules used to be excluded if respective 'disable' parameter did not
exist, regardless of its value. Keep a rule's 'disable' parameter and
properly check on its value.

Either of the options to disable a rule, removing 'disable' as well as
setting it to false, should work now.

Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
---

Notes:
    to rule out potential regressions, I modified all tests relating to
    rule_cfgs such that any enabled rule had the 'disabled' parameter
    explicitly set to 0, and verified that all of the tests passed

 src/PVE/API2/HA/Rules.pm  | 4 ----
 src/PVE/CLI/ha_manager.pm | 2 +-
 src/PVE/HA/Rules.pm       | 2 +-
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/HA/Rules.pm b/src/PVE/API2/HA/Rules.pm
index 0b44d51..5f0ce04 100644
--- a/src/PVE/API2/HA/Rules.pm
+++ b/src/PVE/API2/HA/Rules.pm
@@ -273,8 +273,6 @@ __PACKAGE__->register_method({
         my $type = extract_param($param, 'type');
         my $ruleid = extract_param($param, 'rule');
 
-        delete $param->{disable} if !$param->{disable};
-
         my $plugin = PVE::HA::Rules->lookup($type);
 
         my $opts = $plugin->check_config($ruleid, $param, 1, 1);
@@ -325,8 +323,6 @@ __PACKAGE__->register_method({
         my $digest = extract_param($param, 'digest');
         my $delete = extract_param($param, 'delete');
 
-        delete $param->{disable} if !$param->{disable};
-
         if ($delete) {
             $delete = [PVE::Tools::split_list($delete)];
         }
diff --git a/src/PVE/CLI/ha_manager.pm b/src/PVE/CLI/ha_manager.pm
index 6625de6..cc4361f 100644
--- a/src/PVE/CLI/ha_manager.pm
+++ b/src/PVE/CLI/ha_manager.pm
@@ -259,7 +259,7 @@ our $cmddef = {
                     'enabled', 'state', 'rule', 'type', 'resources', 'comment',
                 ];
                 for my $rule (@$data) {
-                    $rule->{enabled} = int(!exists($rule->{disable}));
+                    $rule->{enabled} = int(!$rule->{disable});
                     $rule->{state} = $rule->{errors} ? 'ignored (conflicts)' : 'in use';
                 }
                 PVE::CLIFormatter::print_api_result($data, $schema, $props_to_print, $options);
diff --git a/src/PVE/HA/Rules.pm b/src/PVE/HA/Rules.pm
index 3a14eeb..90625af 100644
--- a/src/PVE/HA/Rules.pm
+++ b/src/PVE/HA/Rules.pm
@@ -563,7 +563,7 @@ sub foreach_rule {
         next if !$rule; # skip invalid rules
         next if defined($sid) && !defined($rule->{resources}->{$sid});
         next if defined($opts{type}) && $rule->{type} ne $opts{type};
-        next if $opts{exclude_disabled_rules} && exists($rule->{disable});
+        next if $opts{exclude_disabled_rules} && $rule->{disable};
 
         $func->($rule, $ruleid);
     }
-- 
2.47.3

[PATCH manager 2/2] test: rules_cfg: declare explicitly not disabled rules

[Dominik Rusovac]

Adopt tests 'rules_cfgs/inconsistent*' and explicitly set 'disabled' to
false for any enabled rule to check that rules are still compiled
correctly.

Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
---
 ...-disabled-node-resource-affinity-rules.cfg |  88 ++++++++++++
 ...ed-node-resource-affinity-rules.cfg.expect | 133 ++++++++++++++++++
 ...y-not-disabled-resource-affinity-rules.cfg |  32 +++++
 ...isabled-resource-affinity-rules.cfg.expect |  22 +++
 4 files changed, 275 insertions(+)
 create mode 100644 src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg
 create mode 100644 src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg.expect
 create mode 100644 src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg
 create mode 100644 src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg.expect

diff --git a/src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg b/src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg
new file mode 100644
index 0000000..e5a158a
--- /dev/null
+++ b/src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg
@@ -0,0 +1,88 @@
+# Case 1: Do not remove a positive resource affinity rule, where there is exactly one node to keep them together.
+node-affinity: vm101-vm102-must-be-on-node1
+	resources vm:101,vm:102
+	nodes node1
+	strict 1
+    disable 0
+
+resource-affinity: vm101-vm102-must-be-kept-together
+	resources vm:101,vm:102
+	affinity positive
+    disable 0
+
+# Case 2: Do not remove a negative resource affinity rule, where there are exactly enough nodes available to keep them apart.
+node-affinity: vm201-must-be-on-node1
+	resources vm:201
+	nodes node1
+	strict 1
+    disable 0
+
+node-affinity: vm202-must-be-on-node2
+	resources vm:202
+	nodes node2
+	strict 1
+    disable 0
+
+resource-affinity: vm201-vm202-must-be-kept-separate
+	resources vm:201,vm:202
+	affinity negative
+    disable 0
+
+# Case 3: Remove positive resource affinity rules, where two resources are restricted to different nodes.
+node-affinity: vm301-must-be-on-node1
+	resources vm:301
+	nodes node1
+	strict 1
+    disable 0
+
+node-affinity: vm301-must-be-on-node2
+	resources vm:302
+	nodes node2
+	strict 1
+    disable 0
+
+resource-affinity: vm301-vm302-must-be-kept-together
+	resources vm:301,vm:302
+	affinity positive
+    disable 0
+
+# Case 4: Remove negative resource affinity rules, where two resources are restricted to less nodes than needed to keep them apart.
+node-affinity: vm401-must-be-on-node1
+	resources vm:401
+	nodes node1
+	strict 1
+    disable 0
+
+node-affinity: vm402-must-be-on-node1
+	resources vm:402
+	nodes node1
+	strict 1
+    disable 0
+
+resource-affinity: vm401-vm402-must-be-kept-separate
+	resources vm:401,vm:402
+	affinity negative
+    disable 0
+
+# Case 5: Remove positive resource affinity rules, which have overlapping HA resources, and are restricted with two different node affinity rules.
+node-affinity: vm501-must-be-on-node1
+	resources vm:501
+	nodes node1
+	strict 1
+    disable 0
+
+node-affinity: vm503-must-be-on-node2
+	resources vm:503
+	nodes node2
+	strict 1
+    disable 0
+
+resource-affinity: vm501-vm502-must-be-kept-together
+	resources vm:501,vm:502
+	affinity positive
+    disable 0
+
+resource-affinity: vm502-vm503-must-be-kept-together
+	resources vm:502,vm:503
+	affinity positive
+    disable 0
diff --git a/src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg.expect b/src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg.expect
new file mode 100644
index 0000000..5e32e06
--- /dev/null
+++ b/src/test/rules_cfgs/explicitly-not-disabled-node-resource-affinity-rules.cfg.expect
@@ -0,0 +1,133 @@
+--- Log ---
+Drop rule 'vm301-must-be-on-node1', because at least one resource is in a positive resource affinity rule and there are other resources in at least one other node affinity rule already.
+Drop rule 'vm301-must-be-on-node2', because at least one resource is in a positive resource affinity rule and there are other resources in at least one other node affinity rule already.
+Drop rule 'vm301-vm302-must-be-kept-together', because resources are in multiple node affinity rules.
+Drop rule 'vm401-must-be-on-node1', because at least one resource is in a negative resource affinity rule and this rule would restrict these to less nodes than available to the resources.
+Drop rule 'vm401-vm402-must-be-kept-separate', because two or more resources are restricted to less nodes than available to the resources.
+Drop rule 'vm402-must-be-on-node1', because at least one resource is in a negative resource affinity rule and this rule would restrict these to less nodes than available to the resources.
+Drop rule 'vm501-must-be-on-node1', because at least one resource is in a positive resource affinity rule and there are other resources in at least one other node affinity rule already.
+Drop rule 'vm501-vm502-must-be-kept-together', because resources are in multiple node affinity rules.
+Drop rule 'vm502-vm503-must-be-kept-together', because resources are in multiple node affinity rules.
+Drop rule 'vm503-must-be-on-node2', because at least one resource is in a positive resource affinity rule and there are other resources in at least one other node affinity rule already.
+--- Config ---
+{
+   "digest" : "0c16b9fb9c5a0f7824f1bfbd13b1bd96010d7550",
+   "ids" : {
+      "vm101-vm102-must-be-kept-together" : {
+         "affinity" : "positive",
+         "disable" : 0,
+         "resources" : {
+            "vm:101" : 1,
+            "vm:102" : 1
+         },
+         "type" : "resource-affinity"
+      },
+      "vm101-vm102-must-be-on-node1" : {
+         "disable" : 0,
+         "nodes" : {
+            "node1" : {
+               "priority" : 0
+            }
+         },
+         "resources" : {
+            "vm:101" : 1,
+            "vm:102" : 1
+         },
+         "strict" : 1,
+         "type" : "node-affinity"
+      },
+      "vm201-must-be-on-node1" : {
+         "disable" : 0,
+         "nodes" : {
+            "node1" : {
+               "priority" : 0
+            }
+         },
+         "resources" : {
+            "vm:201" : 1
+         },
+         "strict" : 1,
+         "type" : "node-affinity"
+      },
+      "vm201-vm202-must-be-kept-separate" : {
+         "affinity" : "negative",
+         "disable" : 0,
+         "resources" : {
+            "vm:201" : 1,
+            "vm:202" : 1
+         },
+         "type" : "resource-affinity"
+      },
+      "vm202-must-be-on-node2" : {
+         "disable" : 0,
+         "nodes" : {
+            "node2" : {
+               "priority" : 0
+            }
+         },
+         "resources" : {
+            "vm:202" : 1
+         },
+         "strict" : 1,
+         "type" : "node-affinity"
+      }
+   },
+   "order" : {
+      "vm101-vm102-must-be-kept-together" : 2,
+      "vm101-vm102-must-be-on-node1" : 1,
+      "vm201-must-be-on-node1" : 3,
+      "vm201-vm202-must-be-kept-separate" : 5,
+      "vm202-must-be-on-node2" : 4
+   }
+}
+--- Compiled Config ---
+{
+   "node-affinity" : {
+      "vm:101" : {
+         "nodes" : {
+            "node1" : {
+               "priority" : 0
+            }
+         }
+      },
+      "vm:102" : {
+         "nodes" : {
+            "node1" : {
+               "priority" : 0
+            }
+         }
+      },
+      "vm:201" : {
+         "nodes" : {
+            "node1" : {
+               "priority" : 0
+            }
+         }
+      },
+      "vm:202" : {
+         "nodes" : {
+            "node2" : {
+               "priority" : 0
+            }
+         }
+      }
+   },
+   "resource-affinity" : {
+      "negative" : {
+         "vm:201" : {
+            "vm:202" : 1
+         },
+         "vm:202" : {
+            "vm:201" : 1
+         }
+      },
+      "positive" : {
+         "vm:101" : {
+            "vm:102" : 1
+         },
+         "vm:102" : {
+            "vm:101" : 1
+         }
+      }
+   }
+}
diff --git a/src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg b/src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg
new file mode 100644
index 0000000..88feb0b
--- /dev/null
+++ b/src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg
@@ -0,0 +1,32 @@
+# 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
+    disable 0
+
+resource-affinity: keep-apart2
+	resources vm:102,vm:104,vm:106
+	affinity negative
+    disable 0
+
+resource-affinity: stick-together1
+	resources vm:101,vm:102,vm:103,vm:104,vm:106
+	affinity positive
+    disable 0
+
+# 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
+    disable 0
+
+resource-affinity: split-stick-together2
+	resources vm:202,vm:203
+	affinity positive
+    disable 0
+
+resource-affinity: split-keep-apart1
+	resources vm:201,vm:203
+	affinity negative
+    disable 0
diff --git a/src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg.expect b/src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg.expect
new file mode 100644
index 0000000..019159c
--- /dev/null
+++ b/src/test/rules_cfgs/explicitly-not-disabled-resource-affinity-rules.cfg.expect
@@ -0,0 +1,22 @@
+--- 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 ---
+{
+   "digest" : "d4de6b15d65b32e975e51766e23a4a25a950ff2e",
+   "ids" : {},
+   "order" : {}
+}
+--- Compiled Config ---
+{
+   "node-affinity" : {},
+   "resource-affinity" : {
+      "negative" : {},
+      "positive" : {}
+   }
+}
-- 
2.47.3