Merge lp:~james-page/charm-helpers/refactor-service-control into lp:charm-helpers

[James Page]

Refactoring of service control code in host helper

1) Use 'service' command for all service control

Detecting upstart and init.d configuration files is overkill; this is
exactly what the 'service' command is design todo and it also deals with
saucy onwards where init.d and upstart configuration with the same
name might be installed.

'service' will always do the right thing

2) Added restart and reload helpers

reload detects an error (say the service is not running) and will fallback
to restart if so.

This is inline with the openstack charm helpers code.

[Matthew Wedgwood (mew)]

For service_reload, I don't think it's safe to assume that a service should be restarted if reload fails. I'm at a loss to find an example, but services whose "reload" commands handle invalid configuration gracefully might be brought down on a "restart."

[James Page (james-page)]

Maybe that is a little use case focussed - we used this method to either reload or (re)start haproxy for the HA openstack charms. In the instance where haproxy was not running, the restart call would start it correctly.

I'll add an option to the restart function to optionally enable this so that by default, reload won't restart.

[lp:~james-page/charm-helpers/refactor-service-control]

28. By James Page on 2013-06-20

Make restart on failed reload optional, add/update tests to cover this

[James Page (james-page)]

review: Needs Resubmitting

[Matthew Wedgwood (mew)]

+1

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2013-06-12 16:18:26 +0000
+++ charmhelpers/core/host.py	2013-06-20 21:23:22 +0000
@@ -23,16 +23,18 @@
     service('stop', service_name)
 
 
+def service_restart(service_name):
+    service('restart', service_name)
+
+
+def service_reload(service_name, restart_on_failure=False):
+    if not service('reload', service_name) and restart_on_failure:
+        service('restart', service_name)
+
+
 def service(action, service_name):
-    cmd = None
-    if os.path.exists(os.path.join('/etc/init', '%s.conf' % service_name)):
-        cmd = ['initctl', action, service_name]
-    elif os.path.exists(os.path.join('/etc/init.d', service_name)):
-        cmd = [os.path.join('/etc/init.d', service_name), action]
-    if cmd:
-        return_value = subprocess.call(cmd)
-        return return_value == 0
-    return False
+    cmd = ['service', service_name, action]
+    return subprocess.call(cmd) == 0
 
 
 def adduser(username, password=None, shell='/bin/bash', system_user=False):

=== modified file 'tests/contrib/charmsupport/test_nrpe.py'
--- tests/contrib/charmsupport/test_nrpe.py	2013-05-11 20:24:29 +0000
+++ tests/contrib/charmsupport/test_nrpe.py	2013-06-20 21:23:22 +0000
@@ -77,7 +77,7 @@
 
         self.assertEqual(None, checker.write())
 
-        self.check_call_counts(config=1, getpwnam=1, getgrnam=1, exists=2)
+        self.check_call_counts(config=1, getpwnam=1, getgrnam=1, exists=1)
 
     def test_write_restarts_service(self):
         self.patched['config'].return_value = {'nagios_context': 'test'}
@@ -86,10 +86,10 @@
 
         self.assertEqual(None, checker.write())
 
-        expected = ['initctl', 'restart', 'nagios-nrpe-server']
+        expected = ['service', 'nagios-nrpe-server', 'restart']
         self.assertEqual(expected, self.patched['call'].call_args[0][0])
         self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
-                               exists=2, call=1)
+                               exists=1, call=1)
 
     def test_update_nrpe(self):
         self.patched['config'].return_value = {'nagios_context': 'a'}
@@ -139,7 +139,7 @@
         self.patched['relation_set'].assert_called_once_with(
             relation_id="local-monitors:1", monitors=monitors)
         self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
-                               exists=4, open=2, listdir=1,
+                               exists=3, open=2, listdir=1,
                                relation_ids=1, relation_set=1)
 
 

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2013-06-12 16:18:26 +0000
+++ tests/core/test_host.py	2013-06-20 21:23:22 +0000
@@ -62,62 +62,18 @@
 
 class HelpersTest(TestCase):
     @patch('subprocess.call')
-    @patch('os.path.exists')
-    def test_runs_service_action(self, exists, mock_call):
-        init_exists = True
-        init_d_exists = False
-        exists.side_effect = [init_exists, init_d_exists]
-        mock_call.return_value = 0
-        action = 'some-action'
-        service_name = 'foo-service'
-
-        result = host.service(action, service_name)
-
-        self.assertTrue(result)
-        mock_call.assert_called_with(['initctl', action, service_name])
-        exists.assert_has_calls([
-            call(os.path.join('/etc/init', '%s.conf' % service_name)),
-        ])
-
-    @patch('subprocess.call')
-    @patch('os.path.exists')
-    def test_runs_service_action_on_init_d(self, exists, mock_call):
-        init_exists = False
-        init_d_exists = True
-        exists.side_effect = [init_exists, init_d_exists]
-        mock_call.return_value = 0
-        action = 'some-action'
-        service_name = 'foo-service'
-
-        result = host.service(action, service_name)
-
-        self.assertTrue(result)
-        mock_call.assert_called_with([os.path.join('/etc/init.d',
-                                                   service_name), action])
-        exists.assert_has_calls([
-            call(os.path.join('/etc/init.d', service_name)),
-        ])
-
-    @patch('subprocess.call')
-    @patch('os.path.exists')
-    def test_doesnt_run_service_if_it_doesn_exist(self, exists, mock_call):
-        init_exists = False
-        init_d_exists = False
-        exists.side_effect = [init_exists, init_d_exists]
-        action = 'some-action'
-        service_name = 'foo-service'
-
-        result = host.service(action, service_name)
-
-        self.assertFalse(result)
-        self.assertFalse(mock_call.called)
-
-    @patch('subprocess.call')
-    @patch('os.path.exists')
-    def test_returns_false_when_service_fails(self, exists, mock_call):
-        init_exists = False
-        init_d_exists = True
-        exists.side_effect = [init_exists, init_d_exists]
+    def test_runs_service_action(self, mock_call):
+        mock_call.return_value = 0
+        action = 'some-action'
+        service_name = 'foo-service'
+
+        result = host.service(action, service_name)
+
+        self.assertTrue(result)
+        mock_call.assert_called_with(['service', service_name, action])
+
+    @patch('subprocess.call')
+    def test_returns_false_when_service_fails(self, mock_call):
         mock_call.return_value = 1
         action = 'some-action'
         service_name = 'foo-service'
@@ -125,8 +81,7 @@
         result = host.service(action, service_name)
 
         self.assertFalse(result)
-        mock_call.assert_called_with([os.path.join('/etc/init.d',
-                                                   service_name), action])
+        mock_call.assert_called_with(['service', service_name, action])
 
     @patch.object(host, 'service')
     def test_starts_a_service(self, service):
@@ -142,6 +97,40 @@
 
         service.assert_called_with('stop', service_name)
 
+    @patch.object(host, 'service')
+    def test_restarts_a_service(self, service):
+        service_name = 'foo-service'
+        host.service_restart(service_name)
+
+        service.assert_called_with('restart', service_name)
+
+    @patch.object(host, 'service')
+    def test_reloads_a_service(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [True]
+        host.service_reload(service_name)
+
+        service.assert_called_with('reload', service_name)
+
+    @patch.object(host, 'service')
+    def test_failed_reload_restarts_a_service(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False, True]
+        host.service_reload(service_name, restart_on_failure=True)
+
+        service.assert_has_calls([
+                            call('reload', service_name),
+                            call('restart', service_name)
+                            ])
+
+    @patch.object(host, 'service')
+    def test_failed_reload_without_restart(self, service):
+        service_name = 'foo-service'
+        service.side_effect = [False]
+        host.service_reload(service_name)
+
+        service.assert_called_with('reload', service_name)
+
     @patch('pwd.getpwnam')
     @patch('subprocess.check_call')
     @patch.object(host, 'log')