[wolson,r=jamespage,t=osci] Update restart logic around config-change hook to avoid extra restarts.

.coveragerc

@@ -0,0 +1,6 @@
+[report]
+# Regexes for lines to exclude from consideration
+exclude_lines =
+    if __name__ == .__main__.:
+include=
+    hooks/rabbit*

hooks/rabbit_utils.py

@@ -37,18 +37,39 @@ from charmhelpers.contrib.peerstorage import (
     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 @@ def assert_charm_supports_ipv6():
     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)

hooks/rabbitmq_server_relations.py

@@ -31,11 +31,15 @@ from charmhelpers.contrib.openstack.utils import save_script_rc
 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 @@ from charmhelpers.core.hookenv import (
     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
@@ -200,9 +208,10 @@ def cluster_joined(relation_id=None):
         # 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 +221,6 @@ def cluster_joined(relation_id=None):
         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 +233,7 @@ def cluster_joined(relation_id=None):
 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 +247,13 @@ def cluster_changed():
     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 +268,24 @@ def cluster_changed():
             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 \
@@ -595,12 +613,8 @@ def configure_rabbit_ssl():
     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 +636,7 @@ def config_changed():
     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 +653,15 @@ def config_changed():
         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()
 
 
 def pre_install_hooks():