Merge lp:~verterok/charms/trusty/tanuki-spec-manager/sane_db-relation-changed_restarts into lp:~tanuki/charms/trusty/tanuki-spec-manager/trunk

[Guillermo Gonzalez]

Fix handling of db-relation-changed hook and avoid pointless restarts.

"reuse" some knowledge from landscape charm on how handle db-relation-changed hook and avoid pointless restarts.

This is to fix the case of adding a new service/unit relation to postgresql and getting a db-relation-changed triggered on all already related services, with a change on allowed-units field which shouldn't trigger a restart on all other services.

[Celso Providelo (cprov)]

Thanks, Guillo.

I am happy to try it on staging and if it works fine propagate the Restart(ManagerCallback) to other charms, since it's very portable.

review: Approve

=== modified file 'hooks/actions.py'
--- hooks/actions.py	2015-10-29 21:27:35 +0000
+++ hooks/actions.py	2015-11-03 23:08:19 +0000
@@ -5,13 +5,15 @@
 import subprocess
 import yaml
 
+from hashlib import md5
+
 from charmhelpers import fetch
 from charmhelpers.core import (
     hookenv,
     host,
 )
 from charmhelpers.core.services import helpers
-from charmhelpers.core.services.base import service_restart
+from charmhelpers.core.services.base import service_restart, ManagerCallback
 from charmhelpers.core.templating import render
 from charmhelpers.payload import (
     archive,
@@ -36,31 +38,70 @@
 ETC_DIR = os.path.join(BASE_DIR, 'etc')
 
 
-def restart(service_name):
-    """Restart the service, only if the config changed"""
-    current_config_hash = get_config_file_hash()
-    if current_config_hash != config['current_config_hash']:
-        service_restart(service_name)
-
-
-def get_config_file_hash():
-    from hashlib import md5
-    config_file = os.path.join(SERVICE_DIR, 'service.conf')
-    if not os.path.exists(config_file):
-        return None
-    with open(config_file, 'r') as fd:
-        h = md5(fd.read())
-        return h.hexdigest()
-
-
-def refresh_config_hash(service_name):
-    current_config_hash = get_config_file_hash()
-    config['current_config_hash'] = current_config_hash
-    config.save()
-
-
-def log_start(service_name):
-    hookenv.log('spec-manager starting')
+class Restart(ManagerCallback):
+    """Restart the service, only if the config or relevant bits of the
+    db-relation changed
+    """
+
+    no_restart_db_keys = ('allowed-units',)
+
+    def _get_required_data(self, manager, service_name, key):
+        """Get the service manager required_data entry matching the given key"""
+        service = manager.get_service(service_name)
+        for data in service["required_data"]:
+            if key in data:
+                return data[key]
+
+    def _update_persisted_data(self, key, value):
+        """Persist the given 'value' for the given 'key' and return the old value.
+
+        @param key: The key to update.
+        @param value: The value to persist.
+        @return: The old value of the key, or None if it's a new value.
+        """
+        old = config.get(key, None)
+        config[key] = value
+        config.save()
+        return old
+
+    def _needs_restart_db_changed(self, db_new, db_old):
+        if db_old is None:
+            return True
+        new = db_new.copy()
+        old = db_old.copy()
+        for key in self.no_restart_db_keys:
+            new.pop(key)
+            old.pop(key, None)
+        return new != old
+
+    def get_config_file_hash(self):
+        config_file = os.path.join(SERVICE_DIR, 'service.conf')
+        if not os.path.exists(config_file):
+            return None
+        with open(config_file, 'r') as fd:
+            h = md5(fd.read())
+            return h.hexdigest()
+
+    def __call__(self, manager, service_name, event_name):
+
+        if hookenv.hook_name() == 'db-relation-changed':
+            db_new = self._get_required_data(manager, service_name, 'db')
+            # for some unknown reason we can get a list or dict
+            if isinstance(db_new, list):
+                db_new = db_new[0]
+            db_old = self._update_persisted_data('db', db_new)
+            if self._needs_restart_db_changed(db_new, db_old):
+                hookenv.log('Restarting service, db config changed')
+                service_restart(service_name)
+                return
+        elif hookenv.hook_name() == 'config-changed':
+            config_new = self.get_config_file_hash()
+            config_old = self._update_persisted_data('config_hash', config_new)
+            if config_new != config_old:
+                hookenv.log('Restarting service, config file changed')
+                service_restart(service_name)
+                return
+        hookenv.log('Not restarting service, config and db-relation unchanged')
 
 
 def ensure_directories(service_name):
@@ -323,7 +364,7 @@
                               username=parsed.username,
                               password=parsed.password,
                               use_tls=use_tls,
-                            vhost=vhost)
+                              vhost=vhost)
             checks.append(amqp_check)
         return checks
 
 
=== modified file 'hooks/services.py'
--- hooks/services.py	2015-10-29 21:27:35 +0000
+++ hooks/services.py	2015-11-03 23:08:19 +0000
@@ -18,7 +18,6 @@
             'required_data': [config,
                               actions.PostgresqlRelation(required=True)],
             'data_ready': [
-                actions.refresh_config_hash,
                 actions.ensure_directories,
                 actions.get_cloud_service_from_tarball,
                 actions.install_python_packages,
@@ -30,11 +29,10 @@
                 actions.write_db_config,
                 actions.setup_store_poller,
                 actions.publish_conn_check_relation_data,
-                actions.log_start,
                 actions.nrpe_external_master_relation,
             ],
             'start': [
-                actions.restart
+                actions.Restart()
             ],
         },
     ])