[PATCH ha-manager v2 4/7] manager: apply negative resource affinity before get_node_affinity

[Daniel Kral <d.kral@proxmox.com>]

When selecting possible HA resource node placements, get_node_affinity()
in select_service_node() and get_resource_migration_candidates() is
responsible to determine the highest node priority class with at least
one available node and the allowed set from an input node set, which
must only contain available nodes to the specific HA resource.

A necessary condition for a node to be considered as available to an HA
resource is whether the node is in 'online' state (i.e., not 'unknown',
'maintenance', 'fence', nor 'gone').

However, negative resource affinity rules introduced a new condition,
where nodes become unavailable to HA resources: another HA resource,
with which the HA resource must not share the node with, is already
running on the node.

Currently, the negative resource affinity rules are applied on the
highest node priority class selected by get_node_affinity(). This can
result in an empty set if all the remaining nodes in the highest node
priority class cannot be selected because of negative resource affinity
constraints.

Therefore, apply the negative resource affinity rules on the available
node set already, so get_node_affinity() will only select node priority
classes, which meet both conditions mentioned above.

It should be noted that the HA stack currently does not allow an HA
resource to be referenced in both resource affinity rules and explicit
multi-priority node affinity rules. However, non-strict node affinity
rules implicitly create multi-priority node affinity rules, because
there the member nodes are in the highest node priority class, while the
non-member nodes are in the least node priority class.

As seen in "test-resource-affinity-with-node-affinity-strict-negative3",
this makes the HA Manager move HA resources away from their preferred
node, if an HA resource, that it must not share the node with, starts
there. This is valid with the semantics of non-strict node affinity
rules. Nonetheless, handling negative affinity rules in these scenarios
should be improved in the future, e.g., by moving the starting HA
resource to another available node in request_start already.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
changes since v1:
- move this patch up first
- instead of looping through the node priority classes and effectively
  letting apply_negative_resource_affinity() decide the node priority
  class itself, remove them in $online_nodes already beforehand; this
  results in the same behavior and is easier to read and think about
- adapt comment accordingly

 src/PVE/HA/Manager.pm                         | 20 ++++++++++++----
 .../README                                    |  5 ++--
 .../log.expect                                | 24 ++++++++++++++-----
 .../README                                    |  7 +++---
 .../log.expect                                | 16 +++++++------
 5 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index 753da1a8..c38b1ddd 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -206,10 +206,15 @@ sub get_resource_migration_candidates {
         my $current_leader_node = $ss->{$leader_sid}->{node};
         my $online_nodes = { map { $_ => 1 } $online_node_usage->list_nodes() };
 
-        my (undef, $target_nodes) = get_node_affinity($node_affinity, $leader_sid, $online_nodes);
         my ($together, $separate) =
             get_resource_affinity($resource_affinity, $leader_sid, $ss, $online_nodes);
-        apply_negative_resource_affinity($separate, $target_nodes);
+
+        # Remove nodes with active HA resources that must not run on the same node as $leader_sid
+        # here already, so get_node_affinity() acknowledges these nodes as unavailable to
+        # $leader_sid when determining the highest available node priority class.
+        apply_negative_resource_affinity($separate, $online_nodes);
+
+        my (undef, $target_nodes) = get_node_affinity($node_affinity, $leader_sid, $online_nodes);
 
         delete $target_nodes->{$current_leader_node};
 
@@ -352,12 +357,18 @@ sub select_service_node {
         $compiled_rules->@{qw(node-affinity resource-affinity)};
 
     my $online_nodes = { map { $_ => 1 } $online_node_usage->list_nodes() };
+
+    my ($together, $separate) = get_resource_affinity($resource_affinity, $sid, $ss, $online_nodes);
+
+    # Remove nodes with active HA resources that must not run on the same node as $sid
+    # here already, so get_node_affinity() acknowledges these nodes as unavailable to
+    # $sid when determining the highest available node priority class.
+    apply_negative_resource_affinity($separate, $online_nodes);
+
     my ($allowed_nodes, $pri_nodes) = get_node_affinity($node_affinity, $sid, $online_nodes);
 
     return undef if !%$pri_nodes;
 
-    my ($together, $separate) = get_resource_affinity($resource_affinity, $sid, $ss, $online_nodes);
-
     # stay on current node if possible (avoids random migrations)
     if (
         $node_preference eq 'none'
@@ -377,7 +388,6 @@ sub select_service_node {
         }
     }
 
-    apply_negative_resource_affinity($separate, $pri_nodes);
     apply_positive_resource_affinity($together, $pri_nodes);
 
     return $maintenance_fallback
diff --git a/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/README b/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/README
index c6a11cec..e1fc0d04 100644
--- a/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/README
+++ b/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/README
@@ -4,5 +4,6 @@
 - in a non-strict node affinity rule to node2 and node3 (equal priority), and
 - in a strict negative resource affinity rule with each other.
 
-Tests whether the HA resource on node3 will stay there, even though node3 is
-put in maintenance mode, because it cannot find any replacement node.
+Tests whether the HA resource on node3 will correctly move to a replacement
+node, which is different from the node of the other HA resource (node2), and
+moves back to its previous maintenance node as soon as it's available again.
diff --git a/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/log.expect b/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/log.expect
index 5c7aa99c..1fc25206 100644
--- a/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/log.expect
+++ b/src/test/test-resource-affinity-with-node-affinity-maintenance-strict-negative1/log.expect
@@ -30,13 +30,25 @@ info     25    node3/lrm: service status vm:102 started
 info    120      cmdlist: execute crm node3 enable-node-maintenance
 info    125    node3/lrm: status change active => maintenance
 info    140    node1/crm: node 'node3': state changed from 'online' => 'maintenance'
-warn    140    node1/crm: service 'vm:102': cannot find a replacement node while its current node is in maintenance
-warn    160    node1/crm: service 'vm:102': cannot find a replacement node while its current node is in maintenance
-warn    180    node1/crm: service 'vm:102': cannot find a replacement node while its current node is in maintenance
-warn    200    node1/crm: service 'vm:102': cannot find a replacement node while its current node is in maintenance
+info    140    node1/crm: migrate service 'vm:102' to node 'node1' (running)
+info    140    node1/crm: service 'vm:102': state changed from 'started' to 'migrate'  (node = node3, target = node1)
+info    141    node1/lrm: got lock 'ha_agent_node1_lock'
+info    141    node1/lrm: status change wait_for_agent_lock => active
+info    145    node3/lrm: service vm:102 - start migrate to node 'node1'
+info    145    node3/lrm: service vm:102 - end migrate to node 'node1'
+info    160    node1/crm: service 'vm:102': state changed from 'migrate' to 'started'  (node = node1)
+info    161    node1/lrm: starting service vm:102
+info    161    node1/lrm: service status vm:102 started
 info    220      cmdlist: execute crm node3 disable-node-maintenance
-warn    220    node1/crm: service 'vm:102': cannot find a replacement node while its current node is in maintenance
+info    225    node3/lrm: got lock 'ha_agent_node3_lock'
 info    225    node3/lrm: status change maintenance => active
 info    240    node1/crm: node 'node3': state changed from 'maintenance' => 'online'
-info    240    node1/crm: service 'vm:102': clearing stale maintenance node 'node3' setting (is current node)
+info    240    node1/crm: moving service 'vm:102' back to 'node3', node came back from maintenance.
+info    240    node1/crm: migrate service 'vm:102' to node 'node3' (running)
+info    240    node1/crm: service 'vm:102': state changed from 'started' to 'migrate'  (node = node1, target = node3)
+info    241    node1/lrm: service vm:102 - start migrate to node 'node3'
+info    241    node1/lrm: service vm:102 - end migrate to node 'node3'
+info    260    node1/crm: service 'vm:102': state changed from 'migrate' to 'started'  (node = node3)
+info    265    node3/lrm: starting service vm:102
+info    265    node3/lrm: service status vm:102 started
 info    820     hardware: exit simulation - done
diff --git a/src/test/test-resource-affinity-with-node-affinity-strict-negative3/README b/src/test/test-resource-affinity-with-node-affinity-strict-negative3/README
index 062fc665..581b0e9a 100644
--- a/src/test/test-resource-affinity-with-node-affinity-strict-negative3/README
+++ b/src/test/test-resource-affinity-with-node-affinity-strict-negative3/README
@@ -1,7 +1,8 @@
 Test whether a strict negative resource affinity rule among three resources,
-where two resources are restricted each to nodes they are not yet on, can be
-exchanged to the nodes described by their node affinity rules, if one of the
-resources is stopped.
+where all resources are restricted each to nodes they are not yet on, can be
+exchanged to the nodes described by their node affinity rules or fallback to
+another valid configuration within the semantics of non-strict node affinity
+rules, if one of the resources is stopped.
 
 The test scenario is:
 - vm:101, vm:102, and vm:103 should be on node2, node3 or node1 respectively
diff --git a/src/test/test-resource-affinity-with-node-affinity-strict-negative3/log.expect b/src/test/test-resource-affinity-with-node-affinity-strict-negative3/log.expect
index 1ed34c36..66974583 100644
--- a/src/test/test-resource-affinity-with-node-affinity-strict-negative3/log.expect
+++ b/src/test/test-resource-affinity-with-node-affinity-strict-negative3/log.expect
@@ -57,11 +57,13 @@ info    285    node3/lrm: service status vm:102 started
 info    320      cmdlist: execute service vm:101 started
 info    320    node1/crm: service 'vm:101': state changed from 'stopped' to 'request_start'  (node = node1)
 info    320    node1/crm: service 'vm:101': state changed from 'request_start' to 'started'  (node = node1)
-info    320    node1/crm: migrate service 'vm:101' to node 'node2' (running)
-info    320    node1/crm: service 'vm:101': state changed from 'started' to 'migrate'  (node = node1, target = node2)
-info    321    node1/lrm: service vm:101 - start migrate to node 'node2'
-info    321    node1/lrm: service vm:101 - end migrate to node 'node2'
-info    340    node1/crm: service 'vm:101': state changed from 'migrate' to 'started'  (node = node2)
-info    343    node2/lrm: starting service vm:101
-info    343    node2/lrm: service status vm:101 started
+info    320    node1/crm: migrate service 'vm:103' to node 'node2' (running)
+info    320    node1/crm: service 'vm:103': state changed from 'started' to 'migrate'  (node = node1, target = node2)
+info    321    node1/lrm: starting service vm:101
+info    321    node1/lrm: service status vm:101 started
+info    321    node1/lrm: service vm:103 - start migrate to node 'node2'
+info    321    node1/lrm: service vm:103 - end migrate to node 'node2'
+info    340    node1/crm: service 'vm:103': state changed from 'migrate' to 'started'  (node = node2)
+info    343    node2/lrm: starting service vm:103
+info    343    node2/lrm: service status vm:103 started
 info    920     hardware: exit simulation - done
-- 
2.47.3