Merge lp:~thedac/charms/trusty/rabbitmq-server/le-ignore-min-cluster into lp:~openstack-charmers-archive/charms/trusty/rabbitmq-server/next

[David Ames]

Ignore min-cluster-size when juju has leadership election LP:1500204

This MP also addresses two major hurdles to rabbitmq clustering.

1) When more than one node has run stop_app at the same time nodes cannot join the cluster.

The ultimate solution to this is using charmhelpers.coordinator. However, this will require a significant design change. In the meantime, inserting a random wait period before attempting join the cluster is effective in resolving the race.

2) When using juju leadership election, if the elected leader is the third or greater node (i.e. rabbitmq/2 +), the leader would never be joined to the cluster.

Changing the clustering algorithm for each non-leader to join_cluster with the leader resolves this. Also by clustering with the leader rather than non-leader nodes we avoid split-brain clusters.

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

charm_unit_test #10526 rabbitmq-server-next for thedac mp273474
    UNIT OK: passed

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

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

charm_lint_check #11332 rabbitmq-server-next for thedac mp273474
    LINT OK: passed

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

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

charm_amulet_test #7125 rabbitmq-server-next for thedac mp273474
    AMULET OK: passed

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

[Ryan Beisner (1chb1n)]

Loooookin good....

 :)

[Chris Glass (tribaal)]

This looks great!

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

charm_lint_check #11427 rabbitmq-server-next for thedac mp273474
    LINT OK: passed

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

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

charm_unit_test #10618 rabbitmq-server-next for thedac mp273474
    UNIT OK: passed

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

[lp:~thedac/charms/trusty/rabbitmq-server/le-ignore-min-cluster]

122. By David Ames on 2015-10-06

Return an empty list if a leader has not yet been elected

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

charm_lint_check #11428 rabbitmq-server-next for thedac mp273474
    LINT OK: passed

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

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

charm_unit_test #10619 rabbitmq-server-next for thedac mp273474
    UNIT OK: passed

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

[Billy Olsen (billy-olsen)]

Per our discussion on IRC, please add unit tests.

review: Needs Fixing

[lp:~thedac/charms/trusty/rabbitmq-server/le-ignore-min-cluster]

123. By David Ames on 2015-10-06

Fix comments

124. By David Ames on 2015-10-06

Unit tests

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

charm_lint_check #11429 rabbitmq-server-next for thedac mp273474
    LINT OK: passed

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

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

charm_unit_test #10620 rabbitmq-server-next for thedac mp273474
    UNIT OK: passed

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

[Billy Olsen (billy-olsen)]

LGTM, just waiting on the amulet result

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

charm_amulet_test #7171 rabbitmq-server-next for thedac mp273474
    AMULET OK: passed

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

[Billy Olsen (billy-olsen)]

Approved

review: Approve

[Chris Glass (tribaal)]

I realize this is already merged, but I'll do my comments anyway.

It would be good to add an amulet test to the branch asserting that a cluster forms with only i.e. 2 units (one less than min-cluster-size) if leader election is available.

Two inline comments.

=== modified file 'hooks/rabbit_utils.py'
--- hooks/rabbit_utils.py	2015-10-01 09:29:50 +0000
+++ hooks/rabbit_utils.py	2015-10-06 22:49:58 +0000
@@ -4,6 +4,8 @@
 import subprocess
 import glob
 import tempfile
+import random
+import time
 
 from rabbitmq_context import (
     RabbitMQSSLContext,
@@ -19,7 +21,6 @@
 from charmhelpers.core.hookenv import (
     config,
     relation_ids,
-    relation_get,
     related_units,
     log, ERROR,
     INFO,
@@ -301,54 +302,37 @@
         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
-    active_nodes = running_nodes()
     num_tries = 0
-    for node in available_nodes:
-        log('Clustering with remote rabbit host (%s).' % node)
-        if node in active_nodes:
+    for node in leader_node():
+        if node in running_nodes():
             log('Host already clustered with %s.' % node)
             return False
-
+        log('Clustering with remote rabbit host (%s).' % node)
+        # 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)
         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'):
@@ -619,8 +603,34 @@
 
 
 @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:

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-10-01 09:41:38 +0000
+++ hooks/rabbitmq_server_relations.py	2015-10-06 22:49:58 +0000
@@ -66,6 +66,7 @@
     is_leader,
     charm_dir,
     status_set,
+    unit_private_ip,
 )
 from charmhelpers.core.host import (
     cmp_pkgrevno,
@@ -74,7 +75,6 @@
     service_stop,
     service_restart,
     write_file,
-    mkdir,
 )
 from charmhelpers.contrib.charmsupport import nrpe
 
@@ -260,16 +260,21 @@
     """
     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()
+            return True
+        except NotImplementedError:
+            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
 
@@ -318,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')
@@ -343,10 +349,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 'unit_tests/test_rabbit_utils.py'
--- unit_tests/test_rabbit_utils.py	2015-10-01 09:27:20 +0000
+++ unit_tests/test_rabbit_utils.py	2015-10-06 22:49:58 +0000
@@ -157,3 +157,74 @@
             RABBITMQCTL_CLUSTERSTATUS_SOLO
         self.assertEqual(rabbit_utils.running_nodes(),
                          ['rabbit@juju-devel3-machine-14'])
+
+    @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