Merge lp:~james-page/charm-helpers/inteli-restart into lp:charm-helpers

[James Page]

Inteligent restarts of services based on configuration file changes

Example:

@inteli_restart({'/etc/ceph/ceph.conf': [ 'ceph-mon', 'ceph-osd' ]})
def ceph_client_changed():
   ...

In the above example, ceph-mon and ceph-osd would be restarted if the configuration file /etc/ceph/ceph.conf is changed by the hook being executed.

This was missed in the initial sync from openstack-charm-helpers as it landed quite late in to the quantum-gateway charm.

[Adam Gandelman (gandelman-a)]

Not sure where, but any chance this can live somewhere out of hahelpers? I'd like to be able to use this without carrying the rest of the hahelpers. Would it make sense to add this to charmhelpers.core.host?

[Matthew Wedgwood (mew)]

This sounds like an excellent addition to core.host. Could we also give it a more meaningful name, like 'restart_on_change'?

[James Page (james-page)]

Fine with me - I'll refactor

[lp:~james-page/charm-helpers/inteli-restart]

25. By James Page on 2013-06-11

Initial integration into core.host with first set of tests

26. By James Page on 2013-06-11

Revert changes to utils.py

[James Page (james-page)]

OK _ how does that look?

I can expand the test cases a bit as well to ensure that service restarts are only called once even if multiple files change for example

[Adam Gandelman (gandelman-a)]

Looks good, couple of small things:

- White space in tests, comments in restart_on_change() are missing indents.
- Definitely no blocker but test_file_hash_exist could mock out the hashlib stuff so the test isn't testing actual md5 calculation, just that the hexdigest() is being returned correctly to caller.

[lp:~james-page/charm-helpers/inteli-restart]

27. By James Page on 2013-06-11

Sortout indent on comment

28. By James Page on 2013-06-11

Tidy up lint

[James Page (james-page)]

Tidied whitespace and other lint errors/warnings as well

[lp:~james-page/charm-helpers/inteli-restart]

29. By James Page on 2013-06-11

Patchout hashlib, add multiservice test

30. By James Page on 2013-06-11

Check restart calls a different way

[James Page (james-page)]

review: Needs Resubmitting

[Adam Gandelman (gandelman-a)]

LGTM.

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2013-06-11 14:09:19 +0000
+++ charmhelpers/core/host.py	2013-06-11 21:49:25 +0000
@@ -9,6 +9,7 @@
 import pwd
 import grp
 import subprocess
+import hashlib
 
 from hookenv import log, execution_environment
 
@@ -188,3 +189,45 @@
         system_mounts = [m[1::-1] for m in [l.strip().split()
                                             for l in f.readlines()]]
     return system_mounts
+
+
+def file_hash(path):
+    ''' Generate a md5 hash of the contents of 'path' or None if not found '''
+    if os.path.exists(path):
+        h = hashlib.md5()
+        with open(path, 'r') as source:
+            h.update(source.read())  # IGNORE:E1101 - it does have update
+        return h.hexdigest()
+    else:
+        return None
+
+
+def restart_on_change(restart_map):
+    ''' Restart services based on configuration files changing
+
+    This function is used a decorator, for example
+
+        @restart_on_change({
+            '/etc/ceph/ceph.conf': [ 'cinder-api', 'cinder-volume' ]
+            })
+        def ceph_client_changed():
+            ...
+
+    In this example, the cinder-api and cinder-volume services
+    would be restarted if /etc/ceph/ceph.conf is changed by the
+    ceph_client_changed function.
+    '''
+    def wrap(f):
+        def wrapped_f(*args):
+            checksums = {}
+            for path in restart_map:
+                checksums[path] = file_hash(path)
+            f(*args)
+            restarts = []
+            for path in restart_map:
+                if checksums[path] != file_hash(path):
+                    restarts += restart_map[path]
+            for service_name in list(set(restarts)):
+                service('restart', service_name)
+        return wrapped_f
+    return wrap

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2013-06-10 15:59:37 +0000
+++ tests/core/test_host.py	2013-06-11 21:49:25 +0000
@@ -561,3 +561,103 @@
                 ['/dev/pts', 'devpts']
             ])
             mock_open.assert_called_with('/proc/mounts')
+
+    _hash_files = {
+        '/etc/exists.conf': 'lots of nice ceph configuration',
+        '/etc/missing.conf': None
+    }
+
+    @patch('hashlib.md5')
+    @patch('os.path.exists')
+    def test_file_hash_exists(self, exists, md5):
+        filename = '/etc/exists.conf'
+        exists.side_effect = [True]
+        m = md5()
+        m.hexdigest.return_value = self._hash_files[filename]
+        with patch_open() as (mock_open, mock_file):
+            mock_file.read.return_value = self._hash_files[filename]
+            result = host.file_hash(filename)
+            self.assertEqual(result, self._hash_files[filename])
+
+    @patch('os.path.exists')
+    def test_file_hash_missing(self, exists):
+        filename = '/etc/missing.conf'
+        exists.side_effect = [False]
+        with patch_open() as (mock_open, mock_file):
+            mock_file.read.return_value = self._hash_files[filename]
+            result = host.file_hash(filename)
+            self.assertEqual(result, None)
+
+    @patch.object(host, 'service')
+    @patch('os.path.exists')
+    def test_restart_no_changes(self, exists, service):
+        file_name = '/etc/missing.conf'
+        restart_map = {
+            file_name: ['test-service']
+            }
+        exists.side_effect = [False, False]
+
+        @host.restart_on_change(restart_map)
+        def make_no_changes():
+            pass
+
+        make_no_changes()
+
+        assert not service.called
+
+        exists.assert_has_calls([
+            call(file_name),
+        ])
+
+    @patch.object(host, 'service')
+    @patch('os.path.exists')
+    def test_restart_on_change(self, exists, service):
+        file_name = '/etc/missing.conf'
+        restart_map = {
+            file_name: ['test-service']
+            }
+        exists.side_effect = [False, True]
+
+        @host.restart_on_change(restart_map)
+        def make_some_changes(mock_file):
+            mock_file.read.return_value = "newstuff"
+
+        with patch_open() as (mock_open, mock_file):
+            make_some_changes(mock_file)
+
+        for service_name in restart_map[file_name]:
+            service.assert_called_with('restart', service_name)
+
+        exists.assert_has_calls([
+            call(file_name),
+        ])
+
+    @patch.object(host, 'service')
+    @patch('os.path.exists')
+    def test_multiservice_restart_on_change(self, exists, service):
+        file_name_one = '/etc/missing.conf'
+        file_name_two = '/etc/exists.conf'
+        restart_map = {
+            file_name_one: ['test-service'],
+            file_name_two: ['test-service', 'test-service2']
+            }
+        exists.side_effect = [False, True, True, True]
+
+        @host.restart_on_change(restart_map)
+        def make_some_changes():
+            pass
+
+        with patch_open() as (mock_open, mock_file):
+            mock_file.read.side_effect = ['exists', 'missing', 'exists2']
+            make_some_changes()
+
+        # Restart should only happen once per service
+        service.assert_has_calls([
+                            call('restart', 'test-service2'),
+                            call('restart', 'test-service')
+                            ])
+
+        exists.assert_has_calls([
+            call(file_name_one),
+            call(file_name_two)
+        ])