Merge lp:~james-page/charms/trusty/rabbitmq-server/network-splits into lp:charms/trusty/rabbitmq-server

[James Page]

Add support for separate 'access-network' configuration.

[Tim Van Steenburgh (tvansteenburgh)]

Hi James,

Thanks for this. Code review looks good, but I'm unsure how to do a functional test of this, can you provide instructions?

Also, there are some charm-proof warnings to clean up:

W: config.yaml: option access-network does not have the keys: default
W: config.yaml: option key does not have the keys: default
W: config.yaml: option source does not have the keys: default

Thanks!

review: Needs Fixing

[James Page (james-page)]

Hi Tim

https://code.launchpad.net/~openstack-charmers/+junk/network-splits contains some helpers for testing; however basically you need servers with more than one configured network interface.

RE the charm proof warning; not touching those - not having a default key is intentional 'unset' being a valid state. The code works on the fact that a config-get on unset keys returns None in JSON.

review: Needs Resubmitting

[Cory Johns (johnsca)]

James,

I believe that Tim's point was that the policy was that you still need to provide a default value, even if that value is an empty string. No warnings on `charm proof` is listed on the policy: https://juju.ubuntu.com/docs/authors-charm-policy.html

Also, my understanding of the "Resubmit" status was that the review was rejected and would need to be resubmitted to be approved; is that not the case?

review: Needs Fixing

[James Page (james-page)]

charm proof is wrong - sorry I've discussed this numerous times with Marco - not providing a default value is a perfectly acceptable situation and is used extensively in my charms - Marco did at least drop this to a warning rather than an error but I'm not re-writing all of my charms to comply with a policy that I believe is wrong in this case.

review: Needs Resubmitting

[James Page (james-page)]

I've raised https://bugs.launchpad.net/charm-tools/+bug/1368056 to cover this.

[James Page (james-page)]

After some discussion with Marco on IRC, he's provided me with a way to have the key, but still leave the default unset; I've updated the charm to reflect this.

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

charm_lint_check #195 trusty-rabbitmq-server for james-page mp228152
    LINT OK: passed

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

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

charm_unit_test #224 trusty-rabbitmq-server for james-page mp228152
    UNIT FAIL: unit-test missing

UNIT Results (max last 2 lines):
INFO:root:Search string not found in makefile target commands.
ERROR:root:No make target was executed.

Full unit test output: http://paste.ubuntu.com/9542548/
Build: http://10.245.162.77:8080/job/charm_unit_test/224/

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

charm_amulet_test #244 trusty-rabbitmq-server for james-page mp228152
    AMULET FAIL: amulet-test missing

AMULET Results (max last 2 lines):
INFO:root:Search string not found in makefile target commands.
ERROR:root:No make target was executed.

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

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10759-results

review: Needs Fixing (automated testing)

[Cory Johns (johnsca)]

Test failure is due to a merge conflict, mainly in charmhelpers sync, but there are a couple in the charm itself, as well. I tried my hand at resolving, but there was a bit more than I felt comfortable with making decisions on without deeper knowledge of the charm.

review: Needs Fixing

[lp:~james-page/charms/trusty/rabbitmq-server/network-splits]

72. By James Page on 2015-01-23

Rebase on next branch

73. By James Page on 2015-01-23

Optimize calls

74. By James Page on 2015-01-23

Re-instate configuration

75. By James Page on 2015-01-23

Aligment on config

76. By James Page on 2015-01-23

Run flake8 from venv as well

Unmerged revisions

=== added file '.coveragerc'
--- .coveragerc	1970-01-01 00:00:00 +0000
+++ .coveragerc	2015-01-23 08:23:23 +0000
@@ -0,0 +1,6 @@
+[report]
+# Regexes for lines to exclude from consideration
+exclude_lines =
+    if __name__ == .__main__.:
+include=
+    hooks/rabbit*

=== modified file 'hooks/rabbit_utils.py'
--- hooks/rabbit_utils.py	2015-01-19 19:02:16 +0000
+++ hooks/rabbit_utils.py	2015-01-23 08:23:23 +0000
@@ -37,18 +37,39 @@
     peer_retrieve
 )
 
+from collections import OrderedDict
+
 PACKAGES = ['rabbitmq-server', 'python-amqplib']
 
 RABBITMQ_CTL = '/usr/sbin/rabbitmqctl'
 COOKIE_PATH = '/var/lib/rabbitmq/.erlang.cookie'
 ENV_CONF = '/etc/rabbitmq/rabbitmq-env.conf'
 RABBITMQ_CONF = '/etc/rabbitmq/rabbitmq.config'
+ENABLED_PLUGINS = '/etc/rabbitmq/enabled_plugins'
 RABBIT_USER = 'rabbitmq'
 LIB_PATH = '/var/lib/rabbitmq/'
 HOSTS_FILE = '/etc/hosts'
 
 _named_passwd = '/var/lib/charm/{}/{}.passwd'
 
+# hook_contexts are used as a convenient mechanism to render templates
+# logically, consider building a hook_context for template rendering so
+# the charm doesn't concern itself with template specifics etc.
+CONFIG_FILES = OrderedDict([
+    (RABBITMQ_CONF, {
+        'hook_contexts': None,
+        'services': ['rabbitmq-server']
+    }),
+    (ENV_CONF, {
+        'hook_contexts': None,
+        'services': ['rabbitmq-server']
+    }),
+    (ENABLED_PLUGINS, {
+        'hook_contexts': None,
+        'services': ['rabbitmq-server']
+    }),
+])
+
 
 class RabbitmqError(Exception):
     pass
@@ -532,3 +553,20 @@
     if lsb_release()['DISTRIB_CODENAME'].lower() < "trusty":
         raise Exception("IPv6 is not supported in the charms for Ubuntu "
                         "versions less than Trusty 14.04")
+
+
+def restart_map():
+    '''Determine the correct resource map to be passed to
+    charmhelpers.core.restart_on_change() based on the services configured.
+
+    :returns: dict: A dictionary mapping config file to lists of services
+                    that should be restarted when file changes.
+    '''
+    _map = []
+    for f, ctxt in CONFIG_FILES.iteritems():
+        svcs = []
+        for svc in ctxt['services']:
+            svcs.append(svc)
+        if svcs:
+            _map.append((f, svcs))
+    return OrderedDict(_map)

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2015-01-12 21:16:00 +0000
+++ hooks/rabbitmq_server_relations.py	2015-01-23 08:23:23 +0000
@@ -31,11 +31,15 @@
 from charmhelpers.fetch import (
     add_source,
     apt_update,
-    apt_install)
+    apt_install,
+)
 
 from charmhelpers.core.hookenv import (
-    open_port, close_port,
-    log, ERROR,
+    open_port,
+    close_port,
+    log,
+    ERROR,
+    INFO,
     relation_get,
     relation_set,
     relation_ids,
@@ -50,7 +54,11 @@
     UnregisteredHookError
 )
 from charmhelpers.core.host import (
-    rsync, service_stop, service_restart, cmp_pkgrevno
+    cmp_pkgrevno,
+    restart_on_change,
+    rsync,
+    service_stop,
+    service_restart,
 )
 from charmhelpers.contrib.charmsupport.nrpe import NRPE
 from charmhelpers.contrib.ssl.service import ServiceCA
@@ -63,6 +71,8 @@
     peer_retrieve_by_prefix,
 )
 
+from charmhelpers.contrib.network.ip import get_address_in_network
+
 hooks = Hooks()
 
 SERVICE_NAME = os.getenv('JUJU_UNIT_NAME').split('/')[0]
@@ -150,7 +160,16 @@
     if config('prefer-ipv6'):
         relation_settings['private-address'] = get_ipv6_addr()[0]
     else:
-        relation_settings['hostname'] = unit_get('private-address')
+        # NOTE(jamespage)
+        # override private-address settings if access-network is
+        # configured and an appropriate network interface is configured.
+        relation_settings['hostname'] = \
+            get_address_in_network(config('access-network'),
+                                   unit_get('private-address'))
+        relation_settings['private-address'] = \
+            get_address_in_network(config('access-network'),
+                                   unit_get('private-address'))
+
 
     configure_client_ssl(relation_settings)
 
@@ -167,6 +186,7 @@
     # set if need HA queues or not
     if cmp_pkgrevno('rabbitmq-server', '3.0.1') < 0:
         relation_settings['ha_queues'] = True
+
     peer_store_and_set(relation_id=relation_id,
                        relation_settings=relation_settings)
 
@@ -200,9 +220,10 @@
         # then use the current hostname
         nodename = socket.gethostname()
 
-    if nodename:
+    if nodename and rabbit.get_node_name() != nodename:
         log('forcing nodename=%s' % nodename)
-        # need to stop it under current nodename
+        # would like to have used the restart_on_change decorator, but
+        # need to stop it under current nodename prior to updating env
         service_stop('rabbitmq-server')
         rabbit.update_rmq_env_conf(hostname='rabbit@%s' % nodename,
                                    ipv6=config('prefer-ipv6'))
@@ -212,7 +233,6 @@
         log('cluster_joined: Relation greater.')
         return
 
-    rabbit.COOKIE_PATH = '/var/lib/rabbitmq/.erlang.cookie'
     if not os.path.isfile(rabbit.COOKIE_PATH):
         log('erlang cookie missing from %s' % rabbit.COOKIE_PATH,
             level=ERROR)
@@ -225,7 +245,7 @@
 def cluster_changed():
     rdata = relation_get()
     if 'cookie' not in rdata:
-        log('cluster_joined: cookie not yet set.')
+        log('cluster_joined: cookie not yet set.', level=INFO)
         return
 
     if config('prefer-ipv6') and rdata.get('hostname'):
@@ -239,21 +259,13 @@
     whitelist = [a for a in rdata.keys() if a not in blacklist]
     peer_echo(includes=whitelist)
 
-    # sync cookie
-    cookie = peer_retrieve('cookie')
-    if open(rabbit.COOKIE_PATH, 'r').read().strip() == cookie:
-        log('Cookie already synchronized with peer.')
-    else:
-        log('Synchronizing erlang cookie from peer.')
-        rabbit.service('stop')
-        with open(rabbit.COOKIE_PATH, 'wb') as out:
-            out.write(cookie)
-        rabbit.service('start')
+    # sync the cookie with peers if necessary
+    update_cookie()
 
     if is_relation_made('ha') and \
             config('ha-vip-only') is False:
         log('hacluster relation is present, skipping native '
-            'rabbitmq cluster config.')
+            'rabbitmq cluster config.', level=INFO)
         return
 
     # cluster with node
@@ -268,6 +280,24 @@
             amqp_changed(relation_id=rid, remote_unit=unit)
 
 
+def update_cookie():
+    # sync cookie
+    cookie = peer_retrieve('cookie')
+    cookie_local = None
+    with open(rabbit.COOKIE_PATH, 'r') as f:
+        cookie_local = f.read().strip()
+
+    if cookie_local == cookie:
+        log('Cookie already synchronized with peer.')
+        return
+
+    log('Synchronizing erlang cookie from peer.', level=INFO)
+    service_stop('rabbitmq-server')
+    with open(rabbit.COOKIE_PATH, 'wb') as out:
+        out.write(cookie)
+    service_restart('rabbitmq-server')
+
+
 @hooks.hook('cluster-relation-departed')
 def cluster_departed():
     if is_relation_made('ha') and \
@@ -407,13 +437,13 @@
         rbd_size = config('rbd-size')
         sizemb = int(rbd_size.split('G')[0]) * 1024
         blk_device = '/dev/rbd/%s/%s' % (POOL_NAME, rbd_img)
-        # rbd_pool_rep_count = config('ceph-osd-replication-count')
+        ceph.create_pool(service=SERVICE_NAME, name=POOL_NAME,
+                         replicas=int(config('ceph-osd-replication-count')))
         ceph.ensure_ceph_storage(service=SERVICE_NAME, pool=POOL_NAME,
                                  rbd_img=rbd_img, sizemb=sizemb,
                                  fstype='ext4', mount_point=RABBIT_DIR,
                                  blk_device=blk_device,
-                                 system_services=['rabbitmq-server'])  # ,
-        # rbd_pool_replicas=rbd_pool_rep_count)
+                                 system_services=['rabbitmq-server'])
         subprocess.check_call(['chown', '-R', '%s:%s' %
                                (RABBIT_USER, RABBIT_GROUP), RABBIT_DIR])
     else:
@@ -595,12 +625,8 @@
     open_port(ssl_port)
 
 
-def restart_rabbit_update_nrpe():
-    service_restart('rabbitmq-server')
-    update_nrpe_checks()
-
-
 @hooks.hook('config-changed')
+@restart_on_change(rabbit.restart_map())
 def config_changed():
     if config('prefer-ipv6'):
         rabbit.assert_charm_supports_ipv6()
@@ -622,9 +648,7 @@
     chmod(RABBIT_DIR, 0o775)
 
     if config('prefer-ipv6'):
-        service_stop('rabbitmq-server')
         rabbit.update_rmq_env_conf(ipv6=config('prefer-ipv6'))
-        service_restart('rabbitmq-server')
 
     if config('management_plugin') is True:
         rabbit.enable_plugin(MAN_PLUGIN)
@@ -641,15 +665,22 @@
         ha_is_active_active = config("ha-vip-only")
 
         if ha_is_active_active:
-            restart_rabbit_update_nrpe()
+            update_nrpe_checks()
         else:
             if is_elected_leader('res_rabbitmq_vip'):
-                restart_rabbit_update_nrpe()
+                update_nrpe_checks()
             else:
                 log("hacluster relation is present but this node is not active"
                     " skipping update nrpe checks")
     else:
-        restart_rabbit_update_nrpe()
+        update_nrpe_checks()
+
+    # NOTE(jamespage)
+    # trigger amqp_changed to pickup and changes to network
+    # configuration via the access-network config option.
+    for rid in relation_ids('amqp'):
+        for unit in related_units(rid):
+            amqp_changed(relation_id=rid, remote_unit=unit)
 
 
 def pre_install_hooks():

=== modified file 'unit_tests/test_rabbitmq_server_relations.py'
--- unit_tests/test_rabbitmq_server_relations.py	2014-11-27 10:52:50 +0000
+++ unit_tests/test_rabbitmq_server_relations.py	2015-01-23 08:23:23 +0000
@@ -69,14 +69,16 @@
         self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0'}}
         rabbitmq_server_relations.amqp_changed(None, None)
         mock_peer_store_and_set.assert_called_with(
-            relation_settings={'hostname': host_addr,
+            relation_settings={'private-address': '10.1.2.3',
+                               'hostname': host_addr,
                                'ha_queues': True},
             relation_id=None)
 
         self.fake_repo = {'rabbitmq-server': {'pkg_vers': '3.0.2'}}
         rabbitmq_server_relations.amqp_changed(None, None)
         mock_peer_store_and_set.assert_called_with(
-            relation_settings={'hostname': host_addr},
+            relation_settings={'private-address': '10.1.2.3',
+                               'hostname': host_addr},
             relation_id=None)
 
     @patch('rabbitmq_server_relations.peer_store_and_set')
@@ -108,6 +110,8 @@
         mock_config.side_effect = config
         ipv6_addr = "2001:db8:1:0:f816:3eff:fed6:c140"
         mock_get_ipv6_addr.return_value = [ipv6_addr]
+        host_addr = "10.1.2.3"
+        unit_get.return_value = host_addr
         is_elected_leader.return_value = True
         relation_get.return_value = {}
         is_clustered.return_value = False