Merge lp:~hopem/charm-helpers/retry-crm-resource-check-if-not-running into lp:charm-helpers

[Edward Hope-Morley]

[Jorge Niedbalski (niedbalski)]

Nevertheless that corosync resource synchronization should be a pretty quick process, this workaround seems like a good addition for cases in which this is not happening quickly.

- Do you mind to move the nice retry_on_exception method to charmhelpers.core.decorators ?
- I found a minor python 3 issue trying to decode a string. (commented)

Other than that LGTM.

review: Needs Fixing

[Jorge Niedbalski (niedbalski)]

Thanks Edward, I made a small change for fixing the tests , checking if the current status is already a valid text_type, if not it cast to a text_type and encodes it on utf-8.

review: Approve

=== modified file 'charmhelpers/contrib/hahelpers/cluster.py'
--- charmhelpers/contrib/hahelpers/cluster.py	2014-11-26 08:22:54 +0000
+++ charmhelpers/contrib/hahelpers/cluster.py	2014-12-17 17:49:23 +0000
@@ -13,6 +13,7 @@
 
 import subprocess
 import os
+
 from socket import gethostname as get_unit_hostname
 
 import six
@@ -28,12 +29,19 @@
     WARNING,
     unit_get,
 )
+from charmhelpers.core.decorators import (
+    retry_on_exception,
+)
 
 
 class HAIncompleteConfig(Exception):
     pass
 
 
+class CRMResourceNotFound(Exception):
+    pass
+
+
 def is_elected_leader(resource):
     """
     Returns True if the charm executing this is the elected cluster leader.
@@ -68,24 +76,31 @@
     return False
 
 
-def is_crm_leader(resource):
+@retry_on_exception(5, base_delay=2, exc_type=CRMResourceNotFound)
+def is_crm_leader(resource, retry=False):
     """
     Returns True if the charm calling this is the elected corosync leader,
     as returned by calling the external "crm" command.
+
+    We allow this operation to be retried to avoid the possibility of getting a
+    false negative. See LP #1396246 for more info.
     """
-    cmd = [
-        "crm", "resource",
-        "show", resource
-    ]
+    cmd = ['crm', 'resource', 'show', resource]
     try:
-        status = subprocess.check_output(cmd).decode('UTF-8')
+        status = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+        if not six.PY3:
+            status = status.decode("utf-8")
+
     except subprocess.CalledProcessError:
-        return False
-    else:
-        if get_unit_hostname() in status:
-            return True
-        else:
-            return False
+        status = None
+
+    if status and get_unit_hostname() in status:
+        return True
+
+    if status and "resource %s is NOT running" % (resource) in status:
+        raise CRMResourceNotFound("CRM resource %s not found" % (resource))
+
+    return False
 
 
 def is_leader(resource):

=== added file 'charmhelpers/core/decorators.py'
--- charmhelpers/core/decorators.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/core/decorators.py	2014-12-17 17:49:23 +0000
@@ -0,0 +1,41 @@
+#
+# Copyright 2014 Canonical Ltd.
+#
+# Authors:
+#  Edward Hope-Morley <[e-mail]>
+#
+
+import time
+
+from charmhelpers.core.hookenv import (
+    log,
+    INFO,
+)
+
+
+def retry_on_exception(num_retries, base_delay=0, exc_type=Exception):
+    """If the decorated function raises exception exc_type, allow num_retries
+    retry attempts before raise the exception.
+    """
+    def _retry_on_exception_inner_1(f):
+        def _retry_on_exception_inner_2(*args, **kwargs):
+            retries = num_retries
+            multiplier = 1
+            while True:
+                try:
+                    return f(*args, **kwargs)
+                except exc_type:
+                    if not retries:
+                        raise
+
+                delay = base_delay * multiplier
+                multiplier += 1
+                log("Retrying '%s' %d more times (delay=%s)" %
+                    (f.__name__, retries, delay), level=INFO)
+                retries -= 1
+                if delay:
+                    time.sleep(delay)
+
+        return _retry_on_exception_inner_2
+
+    return _retry_on_exception_inner_1

=== modified file 'tests/contrib/hahelpers/test_cluster_utils.py'
--- tests/contrib/hahelpers/test_cluster_utils.py	2014-11-26 08:22:54 +0000
+++ tests/contrib/hahelpers/test_cluster_utils.py	2014-12-17 17:49:23 +0000
@@ -1,5 +1,5 @@
 
-from mock import patch, MagicMock
+from mock import patch, MagicMock, call
 
 from subprocess import CalledProcessError
 from testtools import TestCase
@@ -48,19 +48,36 @@
         check_output.return_value = crm
         self.assertTrue(cluster_utils.is_crm_leader('vip'))
 
+    @patch('charmhelpers.core.decorators.time')
     @patch('subprocess.check_output')
-    def test_is_not_leader(self, check_output):
+    def test_is_not_leader(self, check_output, mock_time):
         '''It determines its unit is not leader'''
         self.get_unit_hostname.return_value = 'node1'
         crm = b'resource vip is running on: node2'
         check_output.return_value = crm
         self.assertFalse(cluster_utils.is_crm_leader('some_resource'))
-
-    @patch('subprocess.check_output')
-    def test_is_crm_leader_no_cluster(self, check_output):
+        self.assertFalse(mock_time.called)
+
+    @patch('charmhelpers.core.decorators.log')
+    @patch('charmhelpers.core.decorators.time')
+    @patch('subprocess.check_output')
+    def test_is_not_leader_resource_not_exists(self, check_output, mock_time,
+                                               mock_log):
+        '''It determines its unit is not leader'''
+        self.get_unit_hostname.return_value = 'node1'
+        check_output.return_value = "resource vip is NOT running"
+        self.assertRaises(cluster_utils.CRMResourceNotFound,
+                          cluster_utils.is_crm_leader, 'vip')
+        mock_time.assert_has_calls([call.sleep(2), call.sleep(4),
+                                    call.sleep(6)])
+
+    @patch('charmhelpers.core.decorators.time')
+    @patch('subprocess.check_output')
+    def test_is_crm_leader_no_cluster(self, check_output, mock_time):
         '''It is not leader if there is no cluster up'''
         check_output.side_effect = CalledProcessError(1, 'crm')
         self.assertFalse(cluster_utils.is_crm_leader('vip'))
+        self.assertFalse(mock_time.called)
 
     def test_peer_units(self):
         '''It lists all peer units for cluster relation'''