From 8e539e045371769a63f3b33fc53eed568d644695 Mon Sep 17 00:00:00 2001

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Date: Tue, 31 May 2022 09:33:08 +0200

Subject: [PATCH] Leave the details of service management to the distro (#1074)

RH-Author: Emanuele Giuseppe Esposito <eesposit@redhat.com>

RH-MergeRequest: 73: Leave the details of service management to the distro (#1074)

RH-Commit: [1/1] d33e0976d8811a6cb2755eb08fd3a4c811be53f5 (eesposit/cloud-init)

RH-Bugzilla: 2091935

RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>

RH-Acked-by: Mohamed Gamal Morsy <mmorsy@redhat.com>

commit 8c89009e75c7cf6c2f87635b82656f07f58095e1

Author: Andy Fiddaman <omnios@citrus-it.co.uk>

Date:   Wed Oct 20 20:58:27 2021 +0000

    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.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

---

 cloudinit/config/cc_fan.py                    | 42 ++++++++-----------

 cloudinit/config/cc_ntp.py                    | 20 ++-------

 cloudinit/config/cc_rsyslog.py                | 17 +++-----

 cloudinit/config/cc_set_passwords.py          | 17 ++------

 cloudinit/config/tests/test_set_passwords.py  | 41 ++++++++----------

 cloudinit/distros/__init__.py                 | 28 +++++++++++++

 .../test_distros/test_manage_service.py       | 38 +++++++++++++++++

 .../test_handler/test_handler_ntp.py          | 29 +++----------

 8 files changed, 119 insertions(+), 113 deletions(-)

 create mode 100644 tests/unittests/test_distros/test_manage_service.py

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 e183993f..4f358c8a 100644

--- a/cloudinit/config/cc_ntp.py

+++ b/cloudinit/config/cc_ntp.py

@@ -459,21 +459,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.

@@ -583,10 +568,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 433de751..9efbf61f 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 220bd11f..a8c57cb8 100755

--- a/cloudinit/distros/__init__.py

+++ b/cloudinit/distros/__init__.py

@@ -784,6 +784,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 6b9c8377..c059e2e6 100644

--- a/tests/unittests/test_handler/test_handler_ntp.py

+++ b/tests/unittests/test_handler/test_handler_ntp.py

@@ -112,22 +112,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)

@@ -488,10 +472,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:

@@ -509,7 +494,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.

@@ -525,7 +510,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))

@@ -673,9 +658,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')

@@ -702,11 +686,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

-- 

2.31.1