Merge lp:~james-page/charm-helpers/systemd-support into lp:charm-helpers

[James Page]

[lp:~james-page/charm-helpers/systemd-support]

516. By James Page on 2016-01-06

Drop else for System ValueError raise.

517. By James Page on 2016-01-06

Tweak

[Adam Collard (adam-collard)]

First pass, several cleanups needed

review: Needs Fixing

[lp:~james-page/charm-helpers/systemd-support]

518. By James Page on 2016-01-06

Feedback from review

519. By James Page on 2016-01-06

optimize is systemd

[Liam Young (gnuoy)]

approve

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2015-11-27 20:57:05 +0000
+++ charmhelpers/core/host.py	2016-01-06 14:04:36 +0000
@@ -72,7 +72,9 @@
         stopped = service_stop(service_name)
     upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
     sysv_file = os.path.join(initd_dir, service_name)
-    if os.path.exists(upstart_file):
+    if init_is_systemd():
+        service('disable', service_name)
+    elif os.path.exists(upstart_file):
         override_path = os.path.join(
             init_dir, '{}.override'.format(service_name))
         with open(override_path, 'w') as fh:
@@ -80,9 +82,9 @@
     elif os.path.exists(sysv_file):
         subprocess.check_call(["update-rc.d", service_name, "disable"])
     else:
-        # XXX: Support SystemD too
         raise ValueError(
-            "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
+            "Unable to detect {0} as SystemD, Upstart {1} or"
+            " SysV {2}".format(
                 service_name, upstart_file, sysv_file))
     return stopped
 
@@ -94,7 +96,9 @@
     Reenable starting again at boot. Start the service"""
     upstart_file = os.path.join(init_dir, "{}.conf".format(service_name))
     sysv_file = os.path.join(initd_dir, service_name)
-    if os.path.exists(upstart_file):
+    if init_is_systemd():
+        service('enable', service_name)
+    elif os.path.exists(upstart_file):
         override_path = os.path.join(
             init_dir, '{}.override'.format(service_name))
         if os.path.exists(override_path):
@@ -102,9 +106,9 @@
     elif os.path.exists(sysv_file):
         subprocess.check_call(["update-rc.d", service_name, "enable"])
     else:
-        # XXX: Support SystemD too
         raise ValueError(
-            "Unable to detect {0} as either Upstart {1} or SysV {2}".format(
+            "Unable to detect {0} as SystemD, Upstart {1} or"
+            " SysV {2}".format(
                 service_name, upstart_file, sysv_file))
 
     started = service_running(service_name)
@@ -115,23 +119,29 @@
 
 def service(action, service_name):
     """Control a system service"""
-    cmd = ['service', service_name, action]
+    if init_is_systemd():
+        cmd = ['systemctl', action, service_name]
+    else:
+        cmd = ['service', service_name, action]
     return subprocess.call(cmd) == 0
 
 
-def service_running(service):
+def service_running(service_name):
     """Determine whether a system service is running"""
-    try:
-        output = subprocess.check_output(
-            ['service', service, 'status'],
-            stderr=subprocess.STDOUT).decode('UTF-8')
-    except subprocess.CalledProcessError:
-        return False
+    if init_is_systemd():
+        return service('is-active', service_name)
     else:
-        if ("start/running" in output or "is running" in output):
-            return True
-        else:
+        try:
+            output = subprocess.check_output(
+                ['service', service_name, 'status'],
+                stderr=subprocess.STDOUT).decode('UTF-8')
+        except subprocess.CalledProcessError:
             return False
+        else:
+            if ("start/running" in output or "is running" in output):
+                return True
+            else:
+                return False
 
 
 def service_available(service_name):
@@ -146,6 +156,13 @@
         return True
 
 
+SYSTEMD_SYSTEM = '/run/systemd/system'
+
+
+def init_is_systemd():
+    return os.path.isdir(SYSTEMD_SYSTEM)
+
+
 def adduser(username, password=None, shell='/bin/bash', system_user=False,
             primary_group=None, secondary_groups=None):
     """

=== modified file 'tests/contrib/charmsupport/test_nrpe.py'
--- tests/contrib/charmsupport/test_nrpe.py	2015-10-29 23:52:26 +0000
+++ tests/contrib/charmsupport/test_nrpe.py	2016-01-06 14:04:36 +0000
@@ -25,6 +25,7 @@
         'relation_ids': {'object': nrpe},
         'relation_set': {'object': nrpe},
         'relations_of_type': {'object': nrpe},
+        'service': {'object': nrpe},
     }
 
     def setUp(self):
@@ -108,10 +109,9 @@
 
         self.assertEqual(None, checker.write())
 
-        expected = ['service', 'nagios-nrpe-server', 'restart']
-        self.assertEqual(expected, self.patched['call'].call_args[0][0])
+        self.patched['service'].assert_called_with('restart', 'nagios-nrpe-server')
         self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
-                               exists=1, call=1)
+                               exists=1, service=1)
 
     def test_update_nrpe(self):
         self.patched['config'].return_value = {'nagios_context': 'a',

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2015-11-27 20:57:05 +0000
+++ tests/core/test_host.py	2016-01-06 14:04:36 +0000
@@ -62,8 +62,25 @@
 
 
 class HelpersTest(TestCase):
+
+    @patch('os.path')
+    def test_init_is_systemd_upstart(self, path):
+        """Upstart based init is correctly detected"""
+        path.isdir.return_value = False
+        self.assertFalse(host.init_is_systemd())
+        path.isdir.assert_called_with('/run/systemd/system')
+
+    @patch('os.path')
+    def test_init_is_systemd_system(self, path):
+        """Systemd based init is correctly detected"""
+        path.isdir.return_value = True
+        self.assertTrue(host.init_is_systemd())
+        path.isdir.assert_called_with('/run/systemd/system')
+
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.call')
-    def test_runs_service_action(self, mock_call):
+    def test_runs_service_action(self, mock_call, systemd):
+        systemd.return_value = False
         mock_call.return_value = 0
         action = 'some-action'
         service_name = 'foo-service'
@@ -73,8 +90,24 @@
         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):
+    @patch.object(host, 'init_is_systemd')
+    @patch('subprocess.call')
+    def test_runs_systemctl_action(self, mock_call, systemd):
+        """Ensure that service calls under systemd call 'systemctl'."""
+        systemd.return_value = True
+        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(['systemctl', action, service_name])
+
+    @patch.object(host, 'init_is_systemd')
+    @patch('subprocess.call')
+    def test_returns_false_when_service_fails(self, mock_call, systemd):
+        systemd.return_value = False
         mock_call.return_value = 1
         action = 'some-action'
         service_name = 'foo-service'
@@ -108,13 +141,43 @@
 
         service.assert_called_with('restart', service_name)
 
+    @patch.object(host, 'service_running')
+    @patch.object(host, 'init_is_systemd')
+    @patch.object(host, 'service')
+    def test_pauses_a_running_systemd_unit(self, service, systemd,
+                                           service_running):
+        """Pause on a running systemd unit will be stopped and disabled."""
+        service_name = 'foo-service'
+        service_running.return_value = True
+        systemd.return_value = True
+        self.assertTrue(host.service_pause(service_name))
+        service.assert_has_calls([
+            call('stop', service_name),
+            call('disable', service_name)])
+
+    @patch.object(host, 'service_running')
+    @patch.object(host, 'init_is_systemd')
+    @patch.object(host, 'service')
+    def test_resumes_a_stopped_systemd_unit(self, service, systemd,
+                                            service_running):
+        """Resume on a stopped systemd unit will be started and enabled."""
+        service_name = 'foo-service'
+        service_running.return_value = False
+        systemd.return_value = True
+        self.assertTrue(host.service_resume(service_name))
+        service.assert_has_calls([
+            call('enable', service_name),
+            call('start', service_name)])
+
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch.object(host, 'service')
-    def test_pauses_a_running_upstart_service(self, service, check_output):
+    def test_pauses_a_running_upstart_service(self, service, check_output, systemd):
         """Pause on a running service will call service stop."""
         check_output.return_value = b'foo-service start/running, process 123'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_pauses_an_upstart_service")
         conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
         # Just needs to exist
@@ -130,13 +193,15 @@
             override_contents = fh.read()
         self.assertEqual("manual\n", override_contents)
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch.object(host, 'service')
-    def test_pauses_a_stopped_upstart_service(self, service, check_output):
+    def test_pauses_a_stopped_upstart_service(self, service, check_output, systemd):
         """Pause on a stopped service will not call service stop."""
         check_output.return_value = b'foo-service stop/waiting'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_pauses_an_upstart_service")
         conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
         # Just needs to exist
@@ -154,15 +219,17 @@
             override_contents = fh.read()
         self.assertEqual("manual\n", override_contents)
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch('subprocess.check_call')
     @patch.object(host, 'service')
     def test_pauses_a_running_sysv_service(self, service, check_call,
-                                           check_output):
+                                           check_output, systemd):
         """Pause calls service stop on a running sysv service."""
         check_output.return_value = b'foo-service start/running, process 123'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_pauses_a_sysv_service")
         sysv_path = os.path.join(tempdir, service_name)
         # Just needs to exist
@@ -175,15 +242,17 @@
         service.assert_called_with('stop', service_name)
         check_call.assert_called_with(["update-rc.d", service_name, "disable"])
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch('subprocess.check_call')
     @patch.object(host, 'service')
     def test_pauses_a_stopped_sysv_service(self, service, check_call,
-                                           check_output):
+                                           check_output, systemd):
         """Pause does not call service stop on a stopped sysv service."""
         check_output.return_value = b'foo-service stop/waiting'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_pauses_a_sysv_service")
         sysv_path = os.path.join(tempdir, service_name)
         # Just needs to exist
@@ -198,10 +267,12 @@
             AssertionError, service.assert_called_with, 'stop', service_name)
         check_call.assert_called_with(["update-rc.d", service_name, "disable"])
 
+    @patch.object(host, 'init_is_systemd')
     @patch.object(host, 'service')
-    def test_pause_with_unknown_service(self, service):
+    def test_pause_with_unknown_service(self, service, systemd):
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_pauses_with_unknown_service")
         self.addCleanup(rmtree, tempdir)
         exception = self.assertRaises(
@@ -211,13 +282,15 @@
             "Unable to detect {0}".format(service_name), str(exception))
         self.assertIn(tempdir, str(exception))
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch.object(host, 'service')
-    def test_resumes_a_running_upstart_service(self, service, check_output):
+    def test_resumes_a_running_upstart_service(self, service, check_output, systemd):
         """When the service is already running, service start isn't called."""
         check_output.return_value = b'foo-service start/running, process 111'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_resumes_an_upstart_service")
         conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
         with open(conf_path, "w") as fh:
@@ -232,13 +305,15 @@
             tempdir, "{}.override".format(service_name))
         self.assertFalse(os.path.exists(override_path))
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch.object(host, 'service')
-    def test_resumes_a_stopped_upstart_service(self, service, check_output):
+    def test_resumes_a_stopped_upstart_service(self, service, check_output, systemd):
         """When the service is stopped, service start is called."""
         check_output.return_value = b'foo-service stop/waiting'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_resumes_an_upstart_service")
         conf_path = os.path.join(tempdir, "{}.conf".format(service_name))
         with open(conf_path, "w") as fh:
@@ -251,14 +326,16 @@
             tempdir, "{}.override".format(service_name))
         self.assertFalse(os.path.exists(override_path))
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch('subprocess.check_call')
     @patch.object(host, 'service')
-    def test_resumes_a_sysv_service(self, service, check_call, check_output):
+    def test_resumes_a_sysv_service(self, service, check_call, check_output, systemd):
         """When process is in a stop/waiting state, service start is called."""
         check_output.return_value = b'foo-service stop/waiting'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_resumes_a_sysv_service")
         sysv_path = os.path.join(tempdir, service_name)
         # Just needs to exist
@@ -271,15 +348,17 @@
         service.assert_called_with('start', service_name)
         check_call.assert_called_with(["update-rc.d", service_name, "enable"])
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
     @patch('subprocess.check_call')
     @patch.object(host, 'service')
     def test_resume_a_running_sysv_service(self, service, check_call,
-                                           check_output):
+                                           check_output, systemd):
         """When process is already running, service start isn't called."""
         check_output.return_value = b'foo-service start/running, process 123'
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_resumes_a_sysv_service")
         sysv_path = os.path.join(tempdir, service_name)
         # Just needs to exist
@@ -294,10 +373,12 @@
             AssertionError, service.assert_called_with, 'start', service_name)
         check_call.assert_called_with(["update-rc.d", service_name, "enable"])
 
+    @patch.object(host, 'init_is_systemd')
     @patch.object(host, 'service')
-    def test_resume_with_unknown_service(self, service):
+    def test_resume_with_unknown_service(self, service, systemd):
         service_name = 'foo-service'
         service.side_effect = [True]
+        systemd.return_value = False
         tempdir = mkdtemp(prefix="test_resumes_with_unknown_service")
         self.addCleanup(rmtree, tempdir)
         exception = self.assertRaises(
@@ -379,18 +460,24 @@
             call('restart', service_name)
         ])
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
-    def test_service_running_on_stopped_service(self, check_output):
+    def test_service_running_on_stopped_service(self, check_output, systemd):
+        systemd.return_value = False
         check_output.return_value = b'foo stop/waiting'
         self.assertFalse(host.service_running('foo'))
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
-    def test_service_running_on_running_service(self, check_output):
+    def test_service_running_on_running_service(self, check_output, systemd):
+        systemd.return_value = False
         check_output.return_value = b'foo start/running, process 23871'
         self.assertTrue(host.service_running('foo'))
 
+    @patch.object(host, 'init_is_systemd')
     @patch('subprocess.check_output')
-    def test_service_running_on_unknown_service(self, check_output):
+    def test_service_running_on_unknown_service(self, check_output, systemd):
+        systemd.return_value = False
         exc = subprocess.CalledProcessError(1, ['status'])
         check_output.side_effect = exc
         self.assertFalse(host.service_running('foo'))