From ae507b8e16771a4b98faef40a873f103b57385eb Mon Sep 17 00:00:00 2001 From: Andy Fiddaman Date: Fri, 10 Sep 2021 13:49:12 +0000 Subject: [PATCH] Leave the details of service management to the distro. 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. --- cloudinit/config/cc_fan.py | 27 ++---- cloudinit/config/cc_ntp.py | 24 +---- cloudinit/config/cc_rsyslog.py | 15 ++-- cloudinit/config/cc_set_passwords.py | 17 +--- cloudinit/config/tests/test_set_passwords.py | 41 ++++----- cloudinit/distros/__init__.py | 26 ++++++ .../test_distros/test_manage_service.py | 87 +++++++++++++++++++ .../test_handler/test_handler_ntp.py | 29 ++----- 8 files changed, 155 insertions(+), 111 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 77984bca0e6..6e3ac80b104 100644 --- a/cloudinit/config/cc_fan.py +++ b/cloudinit/config/cc_fan.py @@ -52,33 +52,17 @@ } -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): + stop_failed = not distro.manage_service('stop', service) if not content.endswith('\n'): content += '\n' util.write_file(config_file, content, omode="w") - ret = run(cmds['start'], msg='start %s' % service) + ret = distro.manage_service('start', service) if ret and stop_failed: LOG.warning("success: %s started", service) - if 'enable' in cmds: - ret = run(cmds['enable'], msg='enable %s' % service) + ret = distro.manage_service('enable', service) return ret @@ -99,7 +83,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 e2231cbbf48..4f7f18ca6c5 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. @@ -595,11 +580,8 @@ def handle(name, cfg, cloud, log, _args): install_ntp_client(cloud.distro.install_packages, 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()) - except subp.ProcessExecutionError as e: - LOG.exception("Failed to reload/start ntp service: %s", e) - raise + cloud.distro.manage_service('reload', + ntp_client_config.get('service_name')) + # vi: ts=4 expandtab diff --git a/cloudinit/config/cc_rsyslog.py b/cloudinit/config/cc_rsyslog.py index 2a2bc931e72..c0805b25369 100644 --- a/cloudinit/config/cc_rsyslog.py +++ b/cloudinit/config/cc_rsyslog.py @@ -207,16 +207,13 @@ r'([:](?P[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'] + service = distro.get_option('rsyslog_svcname', 'rsyslog') + return distro.manage_service('try-reload', service) else: cmd = command - subp.subp(cmd, capture=True) + return subp.subp(cmd, capture=True) def load_config(cfg): @@ -429,9 +426,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 433de751fa1..9efbf61f6c0 100755 --- a/cloudinit/config/cc_set_passwords.py +++ b/cloudinit/config/cc_set_passwords.py @@ -94,18 +94,15 @@ 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 bbe2ee8faad..79118a12398 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 a0526948e1f..fff21c3792c 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -795,6 +795,32 @@ def shutdown_command(self, *, mode, delay, message): args.append(message) return args + def manage_service(self, action, service): + 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]) + try: + return subp.subp(cmd, capture=True) + except subp.ProcessExecutionError as e: + LOG.warning("failed: %s (%s): %s", service, cmd, e) + return False + 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 00000000000..67033077205 --- /dev/null +++ b/tests/unittests/test_distros/test_manage_service.py @@ -0,0 +1,87 @@ +# This file is part of cloud-init. See LICENSE file for license information. + +from cloudinit import distros +from cloudinit.tests.helpers import (CiTestCase, mock) + + +class MyBaseDistro(distros.Distro): + # MyBaseDistro is here to test base Distro class implementations + + def __init__(self, name="basedistro", cfg=None, paths=None): + if not cfg: + cfg = {} + if not paths: + paths = {} + super(MyBaseDistro, self).__init__(name, cfg, paths) + + def install_packages(self, pkglist): + raise NotImplementedError() + + def _write_network(self, settings): + raise NotImplementedError() + + def package_command(self, command, args=None, pkgs=None): + raise NotImplementedError() + + def update_package_sources(self): + raise NotImplementedError() + + def apply_locale(self, locale, out_fn=None): + raise NotImplementedError() + + def set_timezone(self, tz): + raise NotImplementedError() + + def _read_hostname(self, filename, default=None): + raise NotImplementedError() + + def _write_hostname(self, hostname, filename): + raise NotImplementedError() + + def _read_system_hostname(self): + raise NotImplementedError() + + +class TestManageService(CiTestCase): + + with_logs = True + + def setUp(self): + super(TestManageService, self).setUp() + self.dist = MyBaseDistro() + + @mock.patch("cloudinit.distros.uses_systemd") + @mock.patch("cloudinit.distros.subp.subp") + def test_manage_service_systemctl_initcmd(self, m_subp, m_sysd): + self.dist.init_cmd = ['systemctl'] + m_sysd.return_value = False + self.dist.manage_service('start', 'myssh') + m_subp.assert_called_with(['systemctl', 'start', 'myssh'], + capture=True) + + @mock.patch("cloudinit.distros.uses_systemd") + @mock.patch("cloudinit.distros.subp.subp") + def test_manage_service_service_initcmd(self, m_subp, m_sysd): + self.dist.init_cmd = ['service'] + m_sysd.return_value = False + self.dist.manage_service('start', 'myssh') + m_subp.assert_called_with(['service', 'myssh', 'start'], capture=True) + + @mock.patch("cloudinit.distros.uses_systemd") + @mock.patch("cloudinit.distros.subp.subp") + def test_manage_service_service(self, m_subp, m_sysd): + self.dist.init_cmd = ['service'] + m_sysd.return_value = False + self.dist.manage_service('start', 'myssh') + m_subp.assert_called_with(['service', 'myssh', 'start'], capture=True) + + @mock.patch("cloudinit.distros.uses_systemd") + @mock.patch("cloudinit.distros.subp.subp") + def test_manage_service_systemctl(self, m_subp, m_sysd): + self.dist.init_cmd = ['ignore'] + m_sysd.return_value = True + 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 6b9c8377bf0..c059e2e615a 100644 --- a/tests/unittests/test_handler/test_handler_ntp.py +++ b/tests/unittests/test_handler/test_handler_ntp.py @@ -112,22 +112,6 @@ def test_ntp_install_no_op_with_empty_pkg_list(self, mock_subp): 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 @@ def test_ntp_handler_enabled_false(self, m_select): 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 @@ def test_ntp_the_whole_package(self, m_sysd, m_select, m_subp): 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 @@ def test_ntp_the_whole_package(self, m_sysd, m_select, m_subp): 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 @@ def test_ntp_user_config_overrides_system_cfg(self, m_which): 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 @@ def test_ntp_user_provided_config_with_template(self, m_install, m_reload): 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