[pve-devel] [PATCH ha-manager 5/6] CRM: refactor check if state transition to active is ok

[Thomas Lamprecht]

Mainly addresses a problem where we read the manager status without
catching any possible exceptions.

As this was done only to check if our node has active fencing jobs,
which tells us that it makes no sense to even try to acquire the
manager lock - as we're fenced soon anyway.
Besides this check we always checked if we're quorate and if there
are services configured, so move
both checks in the new 'can_get_active' method, which replaces the
check_pending_fencing and the has_services method.

Move the quorum check in front and catch a possible error from the
following manager status read.
As a side effect the state transition code gets a bit shorter without
hiding the check intention.

Signed-off-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
---
 src/PVE/HA/CRM.pm   | 37 +++++++++++++++++++++++++------------
 src/PVE/HA/Tools.pm | 15 ---------------
 2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/src/PVE/HA/CRM.pm b/src/PVE/HA/CRM.pm
index 3dac3ac..2d70570 100644
--- a/src/PVE/HA/CRM.pm
+++ b/src/PVE/HA/CRM.pm
@@ -111,14 +111,32 @@ sub get_protected_ha_manager_lock {
     return 0;
 }
 
-sub check_pending_fencing {
-    my ($manager_status, $node) = @_;
+# checks quorum, for no active pending fence jobs and if services are configured
+sub can_get_active {
+    my ($self, $allow_no_service) = @_;
 
+    my $haenv = $self->{haenv};
+
+    return 0 if !$haenv->quorate();
+
+    my $manager_status = eval { $haenv->read_manager_status() };
+    if (my $err = $@) {
+	$haenv->log('err', "could not read manager status: $err");
+	return 0;
+    }
     my $ss = $manager_status->{service_status};
+    return 0 if PVE::HA::Tools::count_fenced_services($ss, $haenv->nodename());
 
-    return 1 if PVE::HA::Tools::count_fenced_services($ss, $node);
+    if (!$allow_no_service) {
+	my $conf = eval { $haenv->read_service_config() };
+	if (my $err = $@) {
+	    $haenv->log('err', "could not read service config: $err");
+	    return undef;
+	}
+	return 0 if !scalar(%{$conf});
+    }
 
-    return 0;
+    return 1;
 }
 
 sub do_one_iteration {
@@ -129,15 +147,11 @@ sub do_one_iteration {
     my $status = $self->get_local_status();
     my $state = $status->{state};
 
-    my $manager_status = $haenv->read_manager_status();
-    my $pending_fencing = check_pending_fencing($manager_status, $haenv->nodename());
-    
     # do state changes first 
 
     if ($state eq 'wait_for_quorum') {
 
-	if (!$pending_fencing && $haenv->quorate() &&
-	    PVE::HA::Tools::has_services($haenv)) {
+	if ($self->can_get_active()) {
 	    if ($self->get_protected_ha_manager_lock()) {
 		$self->set_local_status({ state => 'master' });
 	    } else {
@@ -147,8 +161,7 @@ sub do_one_iteration {
 
     } elsif ($state eq 'slave') {
 
-	if (!$pending_fencing && $haenv->quorate() &&
-	    PVE::HA::Tools::has_services($haenv)) {
+	if ($self->can_get_active()) {
 	    if ($self->get_protected_ha_manager_lock()) {
 		$self->set_local_status({ state => 'master' });
 	    }
@@ -158,7 +171,7 @@ sub do_one_iteration {
 
     } elsif ($state eq 'lost_manager_lock') {
 
-	if (!$pending_fencing && $haenv->quorate()) {
+	if ($self->can_get_active(1)) {
 	    if ($self->get_protected_ha_manager_lock()) {
 		$self->set_local_status({ state => 'master' });
 	    }
diff --git a/src/PVE/HA/Tools.pm b/src/PVE/HA/Tools.pm
index 0f49bc5..88f775e 100644
--- a/src/PVE/HA/Tools.pm
+++ b/src/PVE/HA/Tools.pm
@@ -148,21 +148,6 @@ sub write_json_to_file {
     PVE::Tools::file_set_contents($filename, $raw);
 }
 
-sub has_services {
-    my ($haenv, $node) = @_;
-
-    my $conf = $haenv->read_service_config();
-
-    # if no node defined any service count is fine
-    return scalar(%{$conf}) if !defined($node);
-
-    foreach my $d (values %$conf) {
-	return 1 if $d->{node} eq $node;
-    }
-
-    return undef;
-}
-
 sub count_fenced_services {
     my ($ss, $node) = @_;
 
-- 
2.11.0