Merge lp:~tribaal/charms/trusty/rabbitmq-server/backport-lp1500204 into lp:charms/trusty/rabbitmq-server

[Chris Glass]

This branch is a backport to -stable of the patch/diff fixing LP:1500204 here: https://code.launchpad.net/~thedac/charms/trusty/rabbitmq-server/le-ignore-min-cluster/+merge/273474

This basically makes the charm ignore the min-cluster-size option in case juju leader-election is available. If it is, the nodes are told to cluster with the leader.

[Adam Collard (adam-collard)]

review: Needs Information

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #10804 rabbitmq-server for tribaal mp273958
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/10804/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #11615 rabbitmq-server for tribaal mp273958
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/11615/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7265 rabbitmq-server for tribaal mp273958
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/7265/

[Chris Glass (tribaal)]

Putting this back in WIP until we fix the comments in -next first. Will re-backport to this branch.

[Chris Glass (tribaal)]

As discussed with members of the openstack-charmers team, we'll backport the -next version of the code, and not try to address some of the comments here (although they are valid).

Comments will be taken care of when we hopefully get rid of both min-cluster-size and max-cluster-retry altogether.

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #10960 rabbitmq-server for tribaal mp273958
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/10960/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #11782 rabbitmq-server for tribaal mp273958
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/11782/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7299 rabbitmq-server for tribaal mp273958
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/12774024/
Build: http://10.245.162.77:8080/job/charm_amulet_test/7299/

[Ryan Beisner (1chb1n)]

FYI, the stable rmq charm still possesses the old tests (pre-refactor).

[lp:~tribaal/charms/trusty/rabbitmq-server/backport-lp1500204]

110. By Chris Glass on 2015-10-13

Empty commit to try and kick OSCI again.

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #10961 rabbitmq-server for tribaal mp273958
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/10961/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #11784 rabbitmq-server for tribaal mp273958
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/11784/

[Ryan Beisner (1chb1n)]

No need to re-kick. rmq stable has known-problematic tests.

For a validated backport, the next amulet tests should also be ported.

[Liam Young (gnuoy)]

Approved

review: Approve

=== modified file 'hooks/rabbit_utils.py'
--- hooks/rabbit_utils.py	2015-09-23 19:36:14 +0000
+++ hooks/rabbit_utils.py	2015-10-13 17:03:15 +0000
@@ -6,6 +6,8 @@
 import subprocess
 import glob
 import tempfile
+import random
+import time
 
 from charmhelpers.contrib.openstack.utils import (
     get_hostname,
@@ -13,12 +15,10 @@
 
 from charmhelpers.core.hookenv import (
     config,
-    relation_ids,
-    relation_get,
-    related_units,
     log, ERROR,
     INFO,
-    service_name
+    service_name,
+    cached,
 )
 
 from charmhelpers.core.host import (
@@ -242,68 +242,42 @@
         cluster_cmd = 'join_cluster'
     else:
         cluster_cmd = 'cluster'
-    out = subprocess.check_output([RABBITMQ_CTL, 'cluster_status'])
-    log('cluster status is %s' % str(out))
-
-    # check if node is already clustered
-    total_nodes = 1
-    running_nodes = []
-    m = re.search("\{running_nodes,\[(.*?)\]\}", out.strip(), re.DOTALL)
-    if m is not None:
-        running_nodes = m.group(1).split(',')
-        running_nodes = [x.replace("'", '') for x in running_nodes]
-        total_nodes = len(running_nodes)
-
-    if total_nodes > 1:
+
+    if clustered():
         log('Node is already clustered, skipping')
         return False
 
-    # check all peers and try to cluster with them
-    available_nodes = []
-    for r_id in relation_ids('cluster'):
-        for unit in related_units(r_id):
-            if config('prefer-ipv6'):
-                address = relation_get('hostname',
-                                       rid=r_id, unit=unit)
-            else:
-                address = relation_get('private-address',
-                                       rid=r_id, unit=unit)
-            if address is not None:
-                node = get_hostname(address, fqdn=False)
-                if node:
-                    available_nodes.append(node)
-                else:
-                    log('Cannot resolve hostname for {} '
-                        'using DNS servers'.format(address), level='WARNING')
-
-    if len(available_nodes) == 0:
+    # Check the leader and try to cluster with it
+    if len(leader_node()) == 0:
         log('No nodes available to cluster with')
         return False
 
-    # iterate over all the nodes, join to the first available
     num_tries = 0
-    for node in available_nodes:
-        log('Clustering with remote rabbit host (%s).' % node)
-        if node in running_nodes:
+    for node in leader_node():
+        if node in running_nodes():
             log('Host already clustered with %s.' % node)
             return False
-
+        # NOTE: The primary problem rabbitmq has clustering is when
+        # more than one node attempts to cluster at the same time.
+        # The asynchronous nature of hook firing nearly guarantees
+        # this. Using random time wait is a hack until we can
+        # implement charmhelpers.coordinator.
+        time.sleep(random.random()*100)
+        log('Clustering with remote rabbit host (%s).' % node)
         try:
             cmd = [RABBITMQ_CTL, 'stop_app']
             subprocess.check_call(cmd)
-            cmd = [RABBITMQ_CTL, cluster_cmd, 'rabbit@%s' % node]
-            try:
-                subprocess.check_output(cmd, stderr=subprocess.STDOUT)
-            except subprocess.CalledProcessError as e:
-                if not e.returncode == 2 or \
-                        "{ok,already_member}" not in e.output:
-                    raise e
+            cmd = [RABBITMQ_CTL, cluster_cmd, node]
+            subprocess.check_output(cmd, stderr=subprocess.STDOUT)
             cmd = [RABBITMQ_CTL, 'start_app']
             subprocess.check_call(cmd)
             log('Host clustered with %s.' % node)
             return True
-        except:
-            log('Failed to cluster with %s.' % node)
+        except subprocess.CalledProcessError as e:
+            log('Failed to cluster with %s. Exception: %s'
+                % (node, e))
+            cmd = [RABBITMQ_CTL, 'start_app']
+            subprocess.check_call(cmd)
         # continue to the next node
         num_tries += 1
         if num_tries > config('max-cluster-tries'):
@@ -591,3 +565,51 @@
     for v in restart_map().values():
         _services = _services + v
     return list(set(_services))
+
+
+def running_nodes():
+    ''' Determine the current set of running rabbitmq-units in the cluster '''
+    out = subprocess.check_output([RABBITMQ_CTL, 'cluster_status'])
+
+    running_nodes = []
+    m = re.search("\{running_nodes,\[(.*?)\]\}", out.strip(), re.DOTALL)
+    if m is not None:
+        running_nodes = m.group(1).split(',')
+        running_nodes = [x.replace("'", '').strip() for x in running_nodes]
+
+    return running_nodes
+
+
+@cached
+def leader_node():
+    ''' Provide the leader node for clustering '''
+    # Each rabbitmq node should join_cluster with the leader
+    # to avoid split-brain clusters.
+    leader_node_ip = peer_retrieve('leader_node_ip')
+    if leader_node_ip:
+        return ["rabbit@" + get_node_hostname(leader_node_ip)]
+    else:
+        return []
+
+
+def get_node_hostname(address):
+    ''' Resolve IP address to hostname for nodes '''
+    node = get_hostname(address, fqdn=False)
+    if node:
+        return node
+    else:
+        log('Cannot resolve hostname for {} using DNS servers'.format(address),
+            level='WARNING')
+        return None
+
+
+@cached
+def clustered():
+    ''' Determine whether local rabbitmq-server is clustered '''
+    # NOTE: A rabbitmq node can only join a cluster once.
+    # Simply checking for more than one running node tells us
+    # if this unit is in a cluster.
+    if len(running_nodes()) > 1:
+        return True
+    else:
+        return False

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-09-23 19:36:14 +0000
+++ hooks/rabbitmq_server_relations.py	2015-10-13 17:03:15 +0000
@@ -63,6 +63,7 @@
     UnregisteredHookError,
     is_leader,
     charm_dir,
+    unit_private_ip,
 )
 from charmhelpers.core.host import (
     cmp_pkgrevno,
@@ -257,16 +258,23 @@
     """
     min_size = config('min-cluster-size')
     if min_size:
-        size = 0
-        for rid in relation_ids('cluster'):
-            size = len(related_units(rid))
+        # Ignore min-cluster-size if juju has leadership election
+        try:
+            is_leader()
+            log("Ignoring min-cluster-size in favour of Juju leader election")
+            return True
+        except NotImplementedError:
+            log("Leader election is not available, using min-cluster-size")
+            size = 0
+            for rid in relation_ids('cluster'):
+                size = len(related_units(rid))
 
-        # Include this unit
-        size += 1
-        if min_size > size:
-            log("Insufficient number of peer units to form cluster "
-                "(expected=%s, got=%s)" % (min_size, size), level=INFO)
-            return False
+            # Include this unit
+            size += 1
+            if min_size > size:
+                log("Insufficient number of peer units to form cluster "
+                    "(expected=%s, got=%s)" % (min_size, size), level=INFO)
+                return False
 
     return True
 
@@ -315,6 +323,7 @@
         log('Leader peer_storing cookie', level=INFO)
         cookie = open(rabbit.COOKIE_PATH, 'r').read().strip()
         peer_store('cookie', cookie)
+        peer_store('leader_node_ip', unit_private_ip())
 
 
 @hooks.hook('cluster-relation-changed')
@@ -341,10 +350,8 @@
             rabbit.update_hosts_file({private_address: hostname})
 
     if not is_sufficient_peers():
-        # Stop rabbit until leader has finished configuring
-        log('Not enough peers, stopping until leader is configured',
+        log('Not enough peers, waiting until leader is configured',
             level=INFO)
-        service_stop('rabbitmq-server')
         return
 
     # sync the cookie with peers if necessary

=== modified file 'metadata.yaml'
--- metadata.yaml	2013-11-15 19:15:16 +0000
+++ metadata.yaml	2015-10-13 17:03:15 +0000
@@ -5,7 +5,7 @@
  RabbitMQ is an implementation of AMQP, the emerging standard for high
  performance enterprise messaging. The RabbitMQ server is a robust and
  scalable implementation of an AMQP broker.
-categories: ["misc"]
+tags: ["misc"]
 provides:
   amqp:
     interface: rabbitmq

=== modified file 'unit_tests/test_rabbit_utils.py'
--- unit_tests/test_rabbit_utils.py	2015-04-13 15:42:57 +0000
+++ unit_tests/test_rabbit_utils.py	2015-10-13 17:03:15 +0000
@@ -69,3 +69,74 @@
         self.assertEqual(lines[0], "#somedata\n")
         self.assertEqual(lines[1], "%s %s\n" % (map.items()[0]))
         self.assertEqual(lines[4], "%s %s\n" % (map.items()[3]))
+
+    @mock.patch('rabbit_utils.get_hostname')
+    def test_get_node_hostname(self, mock_get_hostname):
+        mock_get_hostname.return_value = 'juju-devel3-machine-13'
+        self.assertEqual(rabbit_utils.get_node_hostname('192.168.20.50'),
+                         'juju-devel3-machine-13')
+        mock_get_hostname.assert_called_with('192.168.20.50', fqdn=False)
+
+    @mock.patch('rabbit_utils.get_node_hostname')
+    @mock.patch('rabbit_utils.peer_retrieve')
+    def test_leader_node(self, mock_peer_retrieve, mock_get_node_hostname):
+        mock_peer_retrieve.return_value = '192.168.20.50'
+        mock_get_node_hostname.return_value = 'juju-devel3-machine-15'
+        self.assertEqual(rabbit_utils.leader_node(),
+                         ['rabbit@juju-devel3-machine-15'])
+
+    @mock.patch('rabbit_utils.subprocess.check_call')
+    @mock.patch('rabbit_utils.subprocess.check_output')
+    @mock.patch('rabbit_utils.time')
+    @mock.patch('rabbit_utils.running_nodes')
+    @mock.patch('rabbit_utils.leader_node')
+    @mock.patch('rabbit_utils.clustered')
+    @mock.patch('rabbit_utils.cmp_pkgrevno')
+    def test_cluster_with_not_clustered(self, mock_cmp_pkgrevno,
+                                        mock_clustered, mock_leader_node,
+                                        mock_running_nodes, mock_time,
+                                        mock_check_output, mock_check_call):
+        mock_cmp_pkgrevno.return_value = True
+        mock_clustered.return_value = False
+        mock_leader_node.return_value = ['rabbit@juju-devel7-machine-11']
+        mock_running_nodes.return_value = ['rabbit@juju-devel5-machine-19']
+        rabbit_utils.cluster_with()
+        mock_check_output.assert_called_with([rabbit_utils.RABBITMQ_CTL,
+                                              'join_cluster',
+                                              'rabbit@juju-devel7-machine-11'],
+                                             stderr=-2)
+
+    @mock.patch('rabbit_utils.subprocess.check_call')
+    @mock.patch('rabbit_utils.subprocess.check_output')
+    @mock.patch('rabbit_utils.time')
+    @mock.patch('rabbit_utils.running_nodes')
+    @mock.patch('rabbit_utils.leader_node')
+    @mock.patch('rabbit_utils.clustered')
+    @mock.patch('rabbit_utils.cmp_pkgrevno')
+    def test_cluster_with_clustered(self, mock_cmp_pkgrevno, mock_clustered,
+                                    mock_leader_node, mock_running_nodes,
+                                    mock_time, mock_check_output,
+                                    mock_check_call):
+        mock_clustered.return_value = True
+        mock_leader_node.return_value = ['rabbit@juju-devel7-machine-11']
+        mock_running_nodes.return_value = ['rabbit@juju-devel5-machine-19',
+                                           'rabbit@juju-devel7-machine-11']
+        rabbit_utils.cluster_with()
+        assert not mock_check_output.called
+
+    @mock.patch('rabbit_utils.subprocess.check_call')
+    @mock.patch('rabbit_utils.subprocess.check_output')
+    @mock.patch('rabbit_utils.time')
+    @mock.patch('rabbit_utils.running_nodes')
+    @mock.patch('rabbit_utils.leader_node')
+    @mock.patch('rabbit_utils.clustered')
+    @mock.patch('rabbit_utils.cmp_pkgrevno')
+    def test_cluster_with_no_leader(self, mock_cmp_pkgrevno, mock_clustered,
+                                    mock_leader_node, mock_running_nodes,
+                                    mock_time, mock_check_output,
+                                    mock_check_call):
+        mock_clustered.return_value = False
+        mock_leader_node.return_value = []
+        mock_running_nodes.return_value = ['rabbit@juju-devel5-machine-19']
+        rabbit_utils.cluster_with()
+        assert not mock_check_output.called