[pve-devel] [PATCH ha-manager v4 2/4] Manager: record tried node on relocation policy

[Thomas Lamprecht]

Instead of counting up an integer on each failed start trial, record
the already tried nodes. We can then use the size of the tried nodes
record array as 'try count' and achieve so the same behaviour as with
the 'relocate_trial' hash earlier.

Log the tried nodes after the service started or if it could not be
started at all, so an admin can follow the behaviour and investigate
the reason of the failure on a specific node.

This prepares us also for a more intelligent recovery node selection,
as we can skip already tried nodes from the current recovery cycle.

Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
---

changes since v3:
* add record_service_failed_on_node which should help understanding what happens
  in the code, as else the access to the references witouth writing them explicit
  back may be confusing.
* save the tried and failed nodes in the service status so we need no extra logic
  to delete it if a service gets removed from HA.


 src/PVE/HA/Manager.pm                      | 45 +++++++++++++++++++++---------
 src/test/test-resource-failure2/log.expect |  1 +
 src/test/test-resource-failure5/log.expect |  2 +-
 3 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index 2dfe492..3f28094 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -209,6 +209,7 @@ my $change_service_state = sub {
 
     my $old_state = $sd->{state};
     my $old_node = $sd->{node};
+    my $old_failed_nodes = $sd->{failed_nodes};
 
     die "no state change" if $old_state eq $new_state; # just to be sure
 
@@ -218,6 +219,7 @@ my $change_service_state = sub {
 
     $sd->{state} = $new_state;
     $sd->{node} = $old_node;
+    $sd->{failed_nodes} = $old_failed_nodes;
 
     my $text_state = '';
     foreach my $k (sort keys %params) {
@@ -364,14 +366,10 @@ sub manage {
     foreach my $sid (keys %$ss) {
 	next if $sc->{$sid};
 	$haenv->log('info', "removing stale service '$sid' (no config)");
+	# remove all service related state information
 	delete $ss->{$sid};
     }
 
-    # remove stale relocation try entries
-    foreach my $sid (keys %{$ms->{relocate_trial}}) {
-	delete $ms->{relocate_trial}->{$sid} if !$ss->{$sid};
-    }
-
     $self->update_crm_commands();
 
     for (;;) {
@@ -568,6 +566,16 @@ sub next_state_stopped {
     $haenv->log('err', "service '$sid' - unknown state '$cd->{state}' in service configuration");
 }
 
+sub record_service_failed_on_node {
+	my ($self, $sid, $node) = @_;
+
+	if(!defined($self->{ss}->{$sid}->{failed_nodes})) {
+	    $self->{ss}->{$sid}->{failed_nodes} = [];
+	}
+
+	push @{$self->{ss}->{$sid}->{failed_nodes}}, $node;
+}
+
 sub next_state_started {
     my ($self, $sid, $cd, $sd, $lrm_res) = @_;
 
@@ -608,36 +616,43 @@ sub next_state_started {
 	} else {
 
 	    my $try_next = 0;
+
 	    if ($lrm_res) {
+
 		my $ec = $lrm_res->{exit_code};
 		if ($ec == SUCCESS) {
 
-		    $master_status->{relocate_trial}->{$sid} = 0;
+		    if (defined($sd->{failed_nodes})) {
+			$haenv->log('info', "relocation policy successful for '$sid'," .
+				    " failed nodes: " . join(', ', @{$sd->{failed_nodes}}) );
+		    }
+
+		    delete $sd->{failed_nodes};
 
 		} elsif ($ec == ERROR) {
 		    # apply our relocate policy if we got ERROR from the LRM
+		    $self->record_service_failed_on_node($sid, $sd->{node});
 
-		    my $try = $master_status->{relocate_trial}->{$sid} || 0;
-
-		    if ($try < $cd->{max_relocate}) {
+		    if (scalar(@{$sd->{failed_nodes}}) <= $cd->{max_relocate}) {
 
-			$try++;
 			# tell select_service_node to relocate if possible
 			$try_next = 1;
 
 			$haenv->log('warning', "starting service $sid on node".
 				   " '$sd->{node}' failed, relocating service.");
-			$master_status->{relocate_trial}->{$sid} = $try;
 
 		    } else {
 
-			$haenv->log('err', "recovery policy for service".
-				   " $sid failed, entering error state!");
+			$haenv->log('err', "recovery policy for service $sid " .
+			            "failed, entering error state. Failed nodes: ".
+			            join(', ', @{$sd->{failed_nodes}}));
 			&$change_service_state($self, $sid, 'error');
 			return;
 
 		    }
 		} else {
+		    $self->record_service_failed_on_node($sid, $sd->{node});
+
 		    $haenv->log('err', "service '$sid' got unrecoverable error" .
 				" (exit code $ec))");
 		    # we have no save way out (yet) for other errors
@@ -673,8 +688,12 @@ sub next_state_error {
     my ($self, $sid, $cd, $sd, $lrm_res) = @_;
 
     my $ns = $self->{ns};
+    my $ms = $self->{ms};
 
     if ($cd->{state} eq 'disabled') {
+	# clean up on error recovery
+	delete $sd->{failed_nodes};
+
 	&$change_service_state($self, $sid, 'stopped');
 	return;
     }
diff --git a/src/test/test-resource-failure2/log.expect b/src/test/test-resource-failure2/log.expect
index 604ad95..6e64cc5 100644
--- a/src/test/test-resource-failure2/log.expect
+++ b/src/test/test-resource-failure2/log.expect
@@ -41,4 +41,5 @@ info    201    node1/lrm: got lock 'ha_agent_node1_lock'
 info    201    node1/lrm: status change wait_for_agent_lock => active
 info    201    node1/lrm: starting service fa:130
 info    201    node1/lrm: service status fa:130 started
+info    220    node1/crm: relocation policy successful for 'fa:130', failed nodes: node2
 info    720     hardware: exit simulation - done
diff --git a/src/test/test-resource-failure5/log.expect b/src/test/test-resource-failure5/log.expect
index eb87f9f..807a237 100644
--- a/src/test/test-resource-failure5/log.expect
+++ b/src/test/test-resource-failure5/log.expect
@@ -28,7 +28,7 @@ warn    123    node2/lrm: restart policy: retry number 1 for service 'fa:130'
 info    143    node2/lrm: starting service fa:130
 warn    143    node2/lrm: unable to start service fa:130
 err     143    node2/lrm: unable to start service fa:130 on local node after 1 retries
-err     160    node1/crm: recovery policy for service fa:130 failed, entering error state!
+err     160    node1/crm: recovery policy for service fa:130 failed, entering error state. Failed nodes: node2
 info    160    node1/crm: service 'fa:130': state changed from 'started' to 'error'
 err     163    node2/lrm: service fa:130 is in an error state and needs manual intervention. Look up 'ERROR RECOVERY' in the documentation.
 info    220      cmdlist: execute service fa:130 disabled
-- 
2.1.4