Leave the details of service management to the distro (#1074)

Various modules restart services and they all have logic to try and
detect if they are running on a system that needs 'systemctl' or
'service', and then have code to decide which order the arguments
need to be etc. On top of that, not all modules do this in the same way.

The duplication and different approaches are not ideal but this also
makes it hard to add support for a new distribution that does not use
either 'systemctl' or 'service'.

This change adds a new manage_service() method to the distro class
and updates several modules to use it.

8 files changed, 119 insertions, 113 deletions

diff --git a/cloudinit/config/cc_fan.py b/cloudinit/config/cc_fan.py
index 77984bca..91f50e22 100644
--- a/cloudinit/config/cc_fan.py
+++ b/cloudinit/config/cc_fan.py
@@ -52,35 +52,26 @@ BUILTIN_CFG = {
 }
 
 
-def stop_update_start(service, config_file, content, systemd=False):
-    if systemd:
-        cmds = {'stop': ['systemctl', 'stop', service],
-                'start': ['systemctl', 'start', service],
-                'enable': ['systemctl', 'enable', service]}
-    else:
-        cmds = {'stop': ['service', 'stop'],
-                'start': ['service', 'start']}
-
-    def run(cmd, msg):
-        try:
-            return subp.subp(cmd, capture=True)
-        except subp.ProcessExecutionError as e:
-            LOG.warning("failed: %s (%s): %s", service, cmd, e)
-            return False
-
-    stop_failed = not run(cmds['stop'], msg='stop %s' % service)
+def stop_update_start(distro, service, config_file, content):
+    try:
+        distro.manage_service('stop', service)
+        stop_failed = False
+    except subp.ProcessExecutionError as e:
+        stop_failed = True
+        LOG.warning("failed to stop %s: %s", service, e)
+
     if not content.endswith('\n'):
         content += '\n'
     util.write_file(config_file, content, omode="w")
 
-    ret = run(cmds['start'], msg='start %s' % service)
-    if ret and stop_failed:
-        LOG.warning("success: %s started", service)
-
-    if 'enable' in cmds:
-        ret = run(cmds['enable'], msg='enable %s' % service)
+    try:
+        distro.manage_service('start', service)
+        if stop_failed:
+            LOG.warning("success: %s started", service)
+    except subp.ProcessExecutionError as e:
+        LOG.warning("failed to start %s: %s", service, e)
 
-    return ret
+    distro.manage_service('enable', service)
 
 
 def handle(name, cfg, cloud, log, args):
@@ -99,7 +90,8 @@ def handle(name, cfg, cloud, log, args):
         distro.install_packages(['ubuntu-fan'])
 
     stop_update_start(
+        distro,
         service='ubuntu-fan', config_file=mycfg.get('config_path'),
-        content=mycfg.get('config'), systemd=distro.uses_systemd())
+        content=mycfg.get('config'))
 
 # vi: ts=4 expandtab
diff --git a/cloudinit/config/cc_ntp.py b/cloudinit/config/cc_ntp.py
index f4468c9d..c3aee798 100644
--- a/cloudinit/config/cc_ntp.py
+++ b/cloudinit/config/cc_ntp.py
@@ -473,21 +473,6 @@ def write_ntp_config_template(distro_name, service_name=None, servers=None,
         util.del_file(template_fn)
 
 
-def reload_ntp(service, systemd=False):
-    """Restart or reload an ntp system service.
-
-    @param service: A string specifying the name of the service to be affected.
-    @param systemd: A boolean indicating if the distro uses systemd, defaults
-    to False.
-    @returns: A tuple of stdout, stderr results from executing the action.
-    """
-    if systemd:
-        cmd = ['systemctl', 'reload-or-restart', service]
-    else:
-        cmd = ['service', service, 'restart']
-    subp.subp(cmd, capture=True)
-
-
 def supplemental_schema_validation(ntp_config):
     """Validate user-provided ntp:config option values.
 
@@ -596,10 +581,11 @@ def handle(name, cfg, cloud, log, _args):
                        packages=ntp_client_config['packages'],
                        check_exe=ntp_client_config['check_exe'])
     try:
-        reload_ntp(ntp_client_config['service_name'],
-                   systemd=cloud.distro.uses_systemd())
+        cloud.distro.manage_service('reload',
+                                    ntp_client_config.get('service_name'))
     except subp.ProcessExecutionError as e:
         LOG.exception("Failed to reload/start ntp service: %s", e)
         raise
 
+
 # vi: ts=4 expandtab
diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py
index 2a2bc931..dd2bbd00 100644
--- a/cloudinit/config/cc_rsyslog.py
+++ b/cloudinit/config/cc_rsyslog.py
@@ -207,16 +207,11 @@ HOST_PORT_RE = re.compile(
     r'([:](?P<port>[0-9]+))?$')
 
 
-def reload_syslog(command=DEF_RELOAD, systemd=False):
-    service = 'rsyslog'
+def reload_syslog(distro, command=DEF_RELOAD):
     if command == DEF_RELOAD:
-        if systemd:
-            cmd = ['systemctl', 'reload-or-try-restart', service]
-        else:
-            cmd = ['service', service, 'restart']
-    else:
-        cmd = command
-    subp.subp(cmd, capture=True)
+        service = distro.get_option('rsyslog_svcname', 'rsyslog')
+        return distro.manage_service('try-reload', service)
+    return subp.subp(command, capture=True)
 
 
 def load_config(cfg):
@@ -429,9 +424,7 @@ def handle(name, cfg, cloud, log, _args):
         return
 
     try:
-        restarted = reload_syslog(
-            command=mycfg[KEYNAME_RELOAD],
-            systemd=cloud.distro.uses_systemd()),
+        restarted = reload_syslog(cloud.distro, command=mycfg[KEYNAME_RELOAD])
     except subp.ProcessExecutionError as e:
         restarted = False
         log.warning("Failed to reload syslog", e)
diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index 803a3aa9..3843aaf7 100755
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -94,18 +94,15 @@ PW_SET = (''.join([x for x in ascii_letters + digits
                    if x not in 'loLOI01']))
 
 
-def handle_ssh_pwauth(pw_auth, service_cmd=None, service_name="ssh"):
+def handle_ssh_pwauth(pw_auth, distro):
     """Apply sshd PasswordAuthentication changes.
 
     @param pw_auth: config setting from 'pw_auth'.
                     Best given as True, False, or "unchanged".
-    @param service_cmd: The service command list (['service'])
-    @param service_name: The name of the sshd service for the system.
+    @param distro: an instance of the distro class for the target distribution
 
     @return: None"""
     cfg_name = "PasswordAuthentication"
-    if service_cmd is None:
-        service_cmd = ["service"]
 
     if util.is_true(pw_auth):
         cfg_val = 'yes'
@@ -124,11 +121,7 @@ def handle_ssh_pwauth(pw_auth, service_cmd=None, service_name="ssh"):
         LOG.debug("No need to restart SSH service, %s not updated.", cfg_name)
         return
 
-    if 'systemctl' in service_cmd:
-        cmd = list(service_cmd) + ["restart", service_name]
-    else:
-        cmd = list(service_cmd) + [service_name, "restart"]
-    subp.subp(cmd)
+    distro.manage_service('restart', distro.get_option('ssh_svcname', 'ssh'))
     LOG.debug("Restarted the SSH daemon.")
 
 
@@ -229,9 +222,7 @@ def handle(_name, cfg, cloud, log, args):
             if expired_users:
                 log.debug("Expired passwords for: %s users", expired_users)
 
-    handle_ssh_pwauth(
-        cfg.get('ssh_pwauth'), service_cmd=cloud.distro.init_cmd,
-        service_name=cloud.distro.get_option('ssh_svcname', 'ssh'))
+    handle_ssh_pwauth(cfg.get('ssh_pwauth'), cloud.distro)
 
     if len(errors):
         log.debug("%s errors occured, re-raising the last one", len(errors))
diff --git a/cloudinit/config/tests/test_set_passwords.py b/cloudinit/config/tests/test_set_passwords.py
index bbe2ee8f..79118a12 100644
--- a/cloudinit/config/tests/test_set_passwords.py
+++ b/cloudinit/config/tests/test_set_passwords.py
@@ -14,57 +14,52 @@ class TestHandleSshPwauth(CiTestCase):
 
     with_logs = True
 
-    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch("cloudinit.distros.subp.subp")
     def test_unknown_value_logs_warning(self, m_subp):
-        setpass.handle_ssh_pwauth("floo")
+        cloud = self.tmp_cloud(distro='ubuntu')
+        setpass.handle_ssh_pwauth("floo", cloud.distro)
         self.assertIn("Unrecognized value: ssh_pwauth=floo",
                       self.logs.getvalue())
         m_subp.assert_not_called()
 
     @mock.patch(MODPATH + "update_ssh_config", return_value=True)
-    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch("cloudinit.distros.subp.subp")
     def test_systemctl_as_service_cmd(self, m_subp, m_update_ssh_config):
         """If systemctl in service cmd: systemctl restart name."""
-        setpass.handle_ssh_pwauth(
-            True, service_cmd=["systemctl"], service_name="myssh")
-        self.assertEqual(mock.call(["systemctl", "restart", "myssh"]),
-                         m_subp.call_args)
-
-    @mock.patch(MODPATH + "update_ssh_config", return_value=True)
-    @mock.patch(MODPATH + "subp.subp")
-    def test_service_as_service_cmd(self, m_subp, m_update_ssh_config):
-        """If systemctl in service cmd: systemctl restart name."""
-        setpass.handle_ssh_pwauth(
-            True, service_cmd=["service"], service_name="myssh")
-        self.assertEqual(mock.call(["service", "myssh", "restart"]),
-                         m_subp.call_args)
+        cloud = self.tmp_cloud(distro='ubuntu')
+        cloud.distro.init_cmd = ['systemctl']
+        setpass.handle_ssh_pwauth(True, cloud.distro)
+        m_subp.assert_called_with(
+            ["systemctl", "restart", "ssh"], capture=True)
 
     @mock.patch(MODPATH + "update_ssh_config", return_value=False)
-    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch("cloudinit.distros.subp.subp")
     def test_not_restarted_if_not_updated(self, m_subp, m_update_ssh_config):
         """If config is not updated, then no system restart should be done."""
-        setpass.handle_ssh_pwauth(True)
+        cloud = self.tmp_cloud(distro='ubuntu')
+        setpass.handle_ssh_pwauth(True, cloud.distro)
         m_subp.assert_not_called()
         self.assertIn("No need to restart SSH", self.logs.getvalue())
 
     @mock.patch(MODPATH + "update_ssh_config", return_value=True)
-    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch("cloudinit.distros.subp.subp")
     def test_unchanged_does_nothing(self, m_subp, m_update_ssh_config):
         """If 'unchanged', then no updates to config and no restart."""
-        setpass.handle_ssh_pwauth(
-            "unchanged", service_cmd=["systemctl"], service_name="myssh")
+        cloud = self.tmp_cloud(distro='ubuntu')
+        setpass.handle_ssh_pwauth("unchanged", cloud.distro)
         m_update_ssh_config.assert_not_called()
         m_subp.assert_not_called()
 
-    @mock.patch(MODPATH + "subp.subp")
+    @mock.patch("cloudinit.distros.subp.subp")
     def test_valid_change_values(self, m_subp):
         """If value is a valid changen value, then update should be called."""
+        cloud = self.tmp_cloud(distro='ubuntu')
         upname = MODPATH + "update_ssh_config"
         optname = "PasswordAuthentication"
         for value in util.FALSE_STRINGS + util.TRUE_STRINGS:
             optval = "yes" if value in util.TRUE_STRINGS else "no"
             with mock.patch(upname, return_value=False) as m_update:
-                setpass.handle_ssh_pwauth(value)
+                setpass.handle_ssh_pwauth(value, cloud.distro)
                 m_update.assert_called_with({optname: optval})
         m_subp.assert_not_called()
 
diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index 426a2cf4..fe0015c6 100755
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -798,6 +798,34 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
             args.append(message)
         return args
 
+    def manage_service(self, action, service):
+        """
+        Perform the requested action on a service. This handles the common
+        'systemctl' and 'service' cases and may be overridden in subclasses
+        as necessary.
+        May raise ProcessExecutionError
+        """
+        init_cmd = self.init_cmd
+        if self.uses_systemd() or 'systemctl' in init_cmd:
+            init_cmd = ['systemctl']
+            cmds = {'stop': ['stop', service],
+                    'start': ['start', service],
+                    'enable': ['enable', service],
+                    'restart': ['restart', service],
+                    'reload': ['reload-or-restart', service],
+                    'try-reload': ['reload-or-try-restart', service],
+                    }
+        else:
+            cmds = {'stop': [service, 'stop'],
+                    'start': [service, 'start'],
+                    'enable': [service, 'start'],
+                    'restart': [service, 'restart'],
+                    'reload': [service, 'restart'],
+                    'try-reload': [service, 'restart'],
+                    }
+        cmd = list(init_cmd) + list(cmds[action])
+        return subp.subp(cmd, capture=True)
+
 
 def _apply_hostname_transformations_to_url(url: str, transformations: list):
     """
diff --git a/tests/unittests/test_distros/test_manage_service.py b/tests/unittests/test_distros/test_manage_service.py
new file mode 100644
index 00000000..47e7cfb0
--- /dev/null
+++ b/tests/unittests/test_distros/test_manage_service.py
@@ -0,0 +1,38 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+from cloudinit.tests.helpers import (CiTestCase, mock)
+from tests.unittests.util import TestingDistro
+
+
+class TestManageService(CiTestCase):
+
+    with_logs = True
+
+    def setUp(self):
+        super(TestManageService, self).setUp()
+        self.dist = TestingDistro()
+
+    @mock.patch.object(TestingDistro, 'uses_systemd', return_value=False)
+    @mock.patch("cloudinit.distros.subp.subp")
+    def test_manage_service_systemctl_initcmd(self, m_subp, m_sysd):
+        self.dist.init_cmd = ['systemctl']
+        self.dist.manage_service('start', 'myssh')
+        m_subp.assert_called_with(['systemctl', 'start', 'myssh'],
+                                  capture=True)
+
+    @mock.patch.object(TestingDistro, 'uses_systemd', return_value=False)
+    @mock.patch("cloudinit.distros.subp.subp")
+    def test_manage_service_service_initcmd(self, m_subp, m_sysd):
+        self.dist.init_cmd = ['service']
+        self.dist.manage_service('start', 'myssh')
+        m_subp.assert_called_with(['service', 'myssh', 'start'], capture=True)
+
+    @mock.patch.object(TestingDistro, 'uses_systemd', return_value=True)
+    @mock.patch("cloudinit.distros.subp.subp")
+    def test_manage_service_systemctl(self, m_subp, m_sysd):
+        self.dist.init_cmd = ['ignore']
+        self.dist.manage_service('start', 'myssh')
+        m_subp.assert_called_with(['systemctl', 'start', 'myssh'],
+                                  capture=True)
+
+# vi: ts=4 sw=4 expandtab
diff --git a/tests/unittests/test_handler/test_handler_ntp.py b/tests/unittests/test_handler/test_handler_ntp.py
index 5dfa564f..b9e2ba57 100644
--- a/tests/unittests/test_handler/test_handler_ntp.py
+++ b/tests/unittests/test_handler/test_handler_ntp.py
@@ -107,22 +107,6 @@ class TestNtp(FilesystemMockingTestCase):
                                   check_exe='timesyncd')
         install_func.assert_called_once_with([])
 
-    @mock.patch("cloudinit.config.cc_ntp.subp")
-    def test_reload_ntp_defaults(self, mock_subp):
-        """Test service is restarted/reloaded (defaults)"""
-        service = 'ntp_service_name'
-        cmd = ['service', service, 'restart']
-        cc_ntp.reload_ntp(service)
-        mock_subp.subp.assert_called_with(cmd, capture=True)
-
-    @mock.patch("cloudinit.config.cc_ntp.subp")
-    def test_reload_ntp_systemd(self, mock_subp):
-        """Test service is restarted/reloaded (systemd)"""
-        service = 'ntp_service_name'
-        cc_ntp.reload_ntp(service, systemd=True)
-        cmd = ['systemctl', 'reload-or-restart', service]
-        mock_subp.subp.assert_called_with(cmd, capture=True)
-
     def test_ntp_rename_ntp_conf(self):
         """When NTP_CONF exists, rename_ntp moves it."""
         ntpconf = self.tmp_path("ntp.conf", self.new_root)
@@ -483,10 +467,11 @@ class TestNtp(FilesystemMockingTestCase):
             cc_ntp.handle('notimportant', cfg, mycloud, None, None)
             self.assertEqual(0, m_select.call_count)
 
+    @mock.patch("cloudinit.distros.subp")
     @mock.patch("cloudinit.config.cc_ntp.subp")
     @mock.patch('cloudinit.config.cc_ntp.select_ntp_client')
     @mock.patch("cloudinit.distros.Distro.uses_systemd")
-    def test_ntp_the_whole_package(self, m_sysd, m_select, m_subp):
+    def test_ntp_the_whole_package(self, m_sysd, m_select, m_subp, m_dsubp):
         """Test enabled config renders template, and restarts service """
         cfg = {'ntp': {'enabled': True}}
         for distro in cc_ntp.distros:
@@ -504,7 +489,7 @@ class TestNtp(FilesystemMockingTestCase):
 
             if distro == 'alpine':
                 uses_systemd = False
-                expected_service_call = ['service', service_name, 'restart']
+                expected_service_call = ['rc-service', service_name, 'restart']
                 # _mock_ntp_client_config call above did not specify a client
                 # value and so it defaults to "ntp" which on Alpine Linux only
                 # supports servers and not pools.
@@ -520,7 +505,7 @@ class TestNtp(FilesystemMockingTestCase):
                 m_util.is_false.return_value = util.is_false(
                     cfg['ntp']['enabled'])
                 cc_ntp.handle('notimportant', cfg, mycloud, None, None)
-                m_subp.subp.assert_called_with(
+                m_dsubp.subp.assert_called_with(
                     expected_service_call, capture=True)
 
             self.assertEqual(expected_content, util.load_file(confpath))
@@ -668,9 +653,8 @@ class TestNtp(FilesystemMockingTestCase):
             self.assertEqual(sorted(expected_cfg), sorted(result))
             m_which.assert_has_calls([])
 
-    @mock.patch('cloudinit.config.cc_ntp.reload_ntp')
     @mock.patch('cloudinit.config.cc_ntp.install_ntp_client')
-    def test_ntp_user_provided_config_with_template(self, m_install, m_reload):
+    def test_ntp_user_provided_config_with_template(self, m_install):
         custom = r'\n#MyCustomTemplate'
         user_template = NTP_TEMPLATE + custom
         confpath = os.path.join(self.new_root, 'etc/myntp/myntp.conf')
@@ -697,11 +681,10 @@ class TestNtp(FilesystemMockingTestCase):
                 util.load_file(confpath))
 
     @mock.patch('cloudinit.config.cc_ntp.supplemental_schema_validation')
-    @mock.patch('cloudinit.config.cc_ntp.reload_ntp')
     @mock.patch('cloudinit.config.cc_ntp.install_ntp_client')
     @mock.patch('cloudinit.config.cc_ntp.select_ntp_client')
     def test_ntp_user_provided_config_template_only(self, m_select, m_install,
-                                                    m_reload, m_schema):
+                                                    m_schema):
         """Test custom template for default client"""
         custom = r'\n#MyCustomTemplate'
         user_template = NTP_TEMPLATE + custom