From 54b377875a569686d55de628dbd0843aad28c09d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Mon, 7 Sep 2020 10:19:16 +0200 Subject: [PATCH 1/7] create a shutdown_command method in distro classes Under FreeBSD, we want to use "shutdown -p" for poweroff. Alpine linux also has some specificities. We choose to define a method that returns the shutdown command line to use, rather than a method that actually does the shutdown. This makes it easier to have the tests in test_handler_power_state do their verifications. Two tests are added for the special behaviours that are known so far. --- cloudinit/config/cc_power_state_change.py | 56 +++--------------- cloudinit/distros/__init__.py | 17 ++++++ cloudinit/distros/alpine.py | 27 +++++++++ cloudinit/distros/bsd.py | 21 +++++++ .../test_handler/test_handler_power_state.py | 58 ++++++++++++++----- 5 files changed, 117 insertions(+), 62 deletions(-) diff --git a/cloudinit/config/cc_power_state_change.py b/cloudinit/config/cc_power_state_change.py index 6fcb8a7d1aa..bd30a733b33 100644 --- a/cloudinit/config/cc_power_state_change.py +++ b/cloudinit/config/cc_power_state_change.py @@ -117,7 +117,7 @@ def check_condition(cond, log=None): def handle(_name, cfg, cloud, log, _args): try: - (args, timeout, condition) = load_power_state(cfg, cloud.distro.name) + (args, timeout, condition) = load_power_state(cfg, cloud.distro) if args is None: log.debug("no power_state provided. doing nothing") return @@ -144,19 +144,7 @@ def handle(_name, cfg, cloud, log, _args): condition, execmd, [args, devnull_fp]) -def convert_delay(delay, fmt=None, scale=None): - if not fmt: - fmt = "+%s" - if not scale: - scale = 1 - - if delay != "now": - delay = fmt % int(int(delay) * int(scale)) - - return delay - - -def load_power_state(cfg, distro_name): +def load_power_state(cfg, distro): # returns a tuple of shutdown_command, timeout # shutdown_command is None if no config found pstate = cfg.get('power_state') @@ -167,44 +155,16 @@ def load_power_state(cfg, distro_name): if not isinstance(pstate, dict): raise TypeError("power_state is not a dict.") - opt_map = {'halt': '-H', 'poweroff': '-P', 'reboot': '-r'} - + modes_ok = ['halt', 'poweroff', 'reboot'] mode = pstate.get("mode") - if mode not in opt_map: + if mode not in modes_ok: raise TypeError( "power_state[mode] required, must be one of: %s. found: '%s'." % - (','.join(opt_map.keys()), mode)) - - delay = pstate.get("delay", "now") - message = pstate.get("message") - scale = 1 - fmt = "+%s" - command = ["shutdown", opt_map[mode]] - - if distro_name == 'alpine': - # Convert integer 30 or string '30' to '1800' (seconds) as Alpine's - # halt/poweroff/reboot commands take seconds rather than minutes. - scale = 60 - # No "+" in front of delay value as not supported by Alpine's commands. - fmt = "%s" - if delay == "now": - # Alpine's commands do not understand "now". - delay = "0" - command = [mode, "-d"] - # Alpine's commands don't support a message. - message = None - - try: - delay = convert_delay(delay, fmt=fmt, scale=scale) - except ValueError as e: - raise TypeError( - "power_state[delay] must be 'now' or '+m' (minutes)." - " found '%s'." % delay - ) from e + (','.join(modes_ok), mode)) - args = command + [delay] - if message: - args.append(message) + args = distro.shutdown_command(mode=mode, + delay=pstate.get("delay", "now"), + message=pstate.get("message")) try: timeout = float(pstate.get('timeout', 30.0)) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 2537608fa74..ed9a521504c 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -749,6 +749,23 @@ def create_group(self, name, members=None): subp.subp(['usermod', '-a', '-G', name, member]) LOG.info("Added user '%s' to group '%s'", member, name) + def shutdown_command(self, mode='poweroff', delay='now', message=None): + # called from cc_power_state_change.load_power_state + opt_map = {'halt': '-H', 'poweroff': '-P', 'reboot': '-r'} + command = ["shutdown", opt_map[mode]] + try: + if delay != "now": + delay = "+%d" % int(delay) + except ValueError as e: + raise TypeError( + "power_state[delay] must be 'now' or '+m' (minutes)." + " found '%s'." % delay + ) from e + args = command + [delay] + if message: + args.append(message) + return args + def _apply_hostname_transformations_to_url(url: str, transformations: list): """ diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py index e42443fcc9f..14e9421fda1 100644 --- a/cloudinit/distros/alpine.py +++ b/cloudinit/distros/alpine.py @@ -162,4 +162,31 @@ def preferred_ntp_clients(self): return self._preferred_ntp_clients + def shutdown_command(self, mode='poweroff', delay='now', message=None): + # called from cc_power_state_change.load_power_state + # Alpine has halt/poweroff/reboot, with the following specifics: + # - we use them rather than the generic "shutdown" + # - delay is given with "-d [integer]" + # - the integer is in seconds, cannot be "now", and takes no "+" + # - no message is supported (argument ignored, here) + + command = [mode, "-d"] + + # Convert delay from minutes to seconds, as Alpine's + # halt/poweroff/reboot commands take seconds rather than minutes. + if delay == "now": + # Alpine's commands do not understand "now". + command += ['0'] + else: + # No "+" in front of delay value (not supported) + try: + command += ['%d' % int(int(delay) * 60)] + except ValueError as e: + raise TypeError( + "power_state[delay] must be 'now' or '+m' (minutes)." + " found '%s'." % delay + ) from e + + return command + # vi: ts=4 expandtab diff --git a/cloudinit/distros/bsd.py b/cloudinit/distros/bsd.py index 2ed7a7d5881..daccc17b8c9 100644 --- a/cloudinit/distros/bsd.py +++ b/cloudinit/distros/bsd.py @@ -127,3 +127,24 @@ def apply_locale(self, locale, out_fn=None): def apply_network_config_names(self, netconfig): LOG.debug('Cannot rename network interface.') + + def shutdown_command(self, mode='poweroff', delay='now', message=None): + # called from cc_power_state_change.load_power_state + + # direct copy from the generic shutdown() method, except that + # poweroff is with -p, not -P. + + opt_map = {'halt': '-H', 'poweroff': '-p', 'reboot': '-r'} + command = ["shutdown", opt_map[mode]] + try: + if delay != "now": + delay = "+%d" % int(delay) + except ValueError as e: + raise TypeError( + "power_state[delay] must be 'now' or '+m' (minutes)." + " found '%s'." % delay + ) from e + args = command + [delay] + if message: + args.append(message) + return args diff --git a/tests/unittests/test_handler/test_handler_power_state.py b/tests/unittests/test_handler/test_handler_power_state.py index 93b24fdc643..7dec00404d1 100644 --- a/tests/unittests/test_handler/test_handler_power_state.py +++ b/tests/unittests/test_handler/test_handler_power_state.py @@ -4,72 +4,102 @@ from cloudinit.config import cc_power_state_change as psc +from cloudinit import distros +from cloudinit import helpers + from cloudinit.tests import helpers as t_help from cloudinit.tests.helpers import mock class TestLoadPowerState(t_help.TestCase): + def setUp(self): + super(TestLoadPowerState, self).setUp() + cls = distros.fetch('ubuntu') + paths = helpers.Paths({}) + self.dist = cls('ubuntu', {}, paths) + def test_no_config(self): # completely empty config should mean do nothing - (cmd, _timeout, _condition) = psc.load_power_state({}, 'ubuntu') + (cmd, _timeout, _condition) = psc.load_power_state({}, self.dist) self.assertIsNone(cmd) def test_irrelevant_config(self): # no power_state field in config should return None for cmd (cmd, _timeout, _condition) = psc.load_power_state({'foo': 'bar'}, - 'ubuntu') + self.dist) self.assertIsNone(cmd) def test_invalid_mode(self): + cfg = {'power_state': {'mode': 'gibberish'}} - self.assertRaises(TypeError, psc.load_power_state, cfg, 'ubuntu') + self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist) cfg = {'power_state': {'mode': ''}} - self.assertRaises(TypeError, psc.load_power_state, cfg, 'ubuntu') + self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist) def test_empty_mode(self): cfg = {'power_state': {'message': 'goodbye'}} - self.assertRaises(TypeError, psc.load_power_state, cfg, 'ubuntu') + self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist) def test_valid_modes(self): cfg = {'power_state': {}} for mode in ('halt', 'poweroff', 'reboot'): cfg['power_state']['mode'] = mode - check_lps_ret(psc.load_power_state(cfg, 'ubuntu'), mode=mode) + check_lps_ret(psc.load_power_state(cfg, self.dist), mode=mode) def test_invalid_delay(self): cfg = {'power_state': {'mode': 'poweroff', 'delay': 'goodbye'}} - self.assertRaises(TypeError, psc.load_power_state, cfg, 'ubuntu') + self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist) def test_valid_delay(self): cfg = {'power_state': {'mode': 'poweroff', 'delay': ''}} for delay in ("now", "+1", "+30"): cfg['power_state']['delay'] = delay - check_lps_ret(psc.load_power_state(cfg, 'ubuntu')) + check_lps_ret(psc.load_power_state(cfg, self.dist)) def test_message_present(self): cfg = {'power_state': {'mode': 'poweroff', 'message': 'GOODBYE'}} - ret = psc.load_power_state(cfg, 'ubuntu') - check_lps_ret(psc.load_power_state(cfg, 'ubuntu')) + ret = psc.load_power_state(cfg, self.dist) + check_lps_ret(psc.load_power_state(cfg, self.dist)) self.assertIn(cfg['power_state']['message'], ret[0]) def test_no_message(self): # if message is not present, then no argument should be passed for it cfg = {'power_state': {'mode': 'poweroff'}} - (cmd, _timeout, _condition) = psc.load_power_state(cfg, 'ubuntu') + (cmd, _timeout, _condition) = psc.load_power_state(cfg, self.dist) self.assertNotIn("", cmd) - check_lps_ret(psc.load_power_state(cfg, 'ubuntu')) + check_lps_ret(psc.load_power_state(cfg, self.dist)) self.assertTrue(len(cmd) == 3) def test_condition_null_raises(self): cfg = {'power_state': {'mode': 'poweroff', 'condition': None}} - self.assertRaises(TypeError, psc.load_power_state, cfg, 'ubuntu') + self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist) def test_condition_default_is_true(self): cfg = {'power_state': {'mode': 'poweroff'}} - _cmd, _timeout, cond = psc.load_power_state(cfg, 'ubuntu') + _cmd, _timeout, cond = psc.load_power_state(cfg, self.dist) self.assertEqual(cond, True) + def test_freebsd_poweroff_uses_lowercase_p(self): + cls = distros.fetch('freebsd') + paths = helpers.Paths({}) + freebsd = cls('freebsd', {}, paths) + cfg = {'power_state': {'mode': 'poweroff'}} + ret = psc.load_power_state(cfg, freebsd) + assert '-p' in ret[0] + + def test_alpine_delay(self): + # alpine takes delay in seconds. + cls = distros.fetch('alpine') + paths = helpers.Paths({}) + alpine = cls('alpine', {}, paths) + cfg = {'power_state': {'mode': 'poweroff', 'delay': ''}} + for delay, value in {'now': 0, "+1": 60, "+30": 1800}.items(): + cfg['power_state']['delay'] = delay + ret = psc.load_power_state(cfg, alpine) + assert ret[0][1] == '-d' + assert ret[0][2] == str(value) + class TestCheckCondition(t_help.TestCase): def cmd_with_exit(self, rc): From 2c5d0ef46891b001ccd3ac3de9db6e6707deaef7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Mon, 7 Sep 2020 10:23:09 +0200 Subject: [PATCH 2/7] add myself to tools/.github-cla-signers --- tools/.github-cla-signers | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/.github-cla-signers b/tools/.github-cla-signers index c67db436933..8fa61d85180 100644 --- a/tools/.github-cla-signers +++ b/tools/.github-cla-signers @@ -6,6 +6,7 @@ candlerb dermotbradley dhensby eandersson +emmanuelthome izzyleung johnsonshi landon912 From e940e4d6b27444f80f34b546fcaa12c0a02875a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Thu, 10 Sep 2020 22:33:00 +0200 Subject: [PATCH 3/7] assert -> self.assert* in unittests for load_power_state Co-authored-by: Daniel Watkins --- tests/unittests/test_handler/test_handler_power_state.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unittests/test_handler/test_handler_power_state.py b/tests/unittests/test_handler/test_handler_power_state.py index 7dec00404d1..4ac494249ac 100644 --- a/tests/unittests/test_handler/test_handler_power_state.py +++ b/tests/unittests/test_handler/test_handler_power_state.py @@ -86,7 +86,7 @@ def test_freebsd_poweroff_uses_lowercase_p(self): freebsd = cls('freebsd', {}, paths) cfg = {'power_state': {'mode': 'poweroff'}} ret = psc.load_power_state(cfg, freebsd) - assert '-p' in ret[0] + self.assertIn('-p', ret[0]) def test_alpine_delay(self): # alpine takes delay in seconds. @@ -94,11 +94,11 @@ def test_alpine_delay(self): paths = helpers.Paths({}) alpine = cls('alpine', {}, paths) cfg = {'power_state': {'mode': 'poweroff', 'delay': ''}} - for delay, value in {'now': 0, "+1": 60, "+30": 1800}.items(): + for delay, value in (('now', 0), ("+1", 60), ("+30", 1800)): cfg['power_state']['delay'] = delay ret = psc.load_power_state(cfg, alpine) - assert ret[0][1] == '-d' - assert ret[0][2] == str(value) + self.assertEqual('-d', ret[0][1]) + self.assertEqual(str(value), ret[0][2]) class TestCheckCondition(t_help.TestCase): From ae8ee254081f70ae2b21400ecca1bb380229eb93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Thu, 10 Sep 2020 22:43:50 +0200 Subject: [PATCH 4/7] create class object Distro.shutdown_options_map This follows from the review suggestions. Indeed, we can avoid the cumbersome duplication of the Distro.shutdown_command in the BSD command by mere variation of a shutdown_options_map class object. --- cloudinit/distros/__init__.py | 6 ++++-- cloudinit/distros/alpine.py | 3 +-- cloudinit/distros/bsd.py | 25 ++++--------------------- 3 files changed, 9 insertions(+), 25 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index ed9a521504c..c2e8d9ff420 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -73,6 +73,9 @@ class Distro(metaclass=abc.ABCMeta): renderer_configs = {} _preferred_ntp_clients = None networking_cls = LinuxNetworking + # This is used by self.shutdown_command(), and can be overridden in + # subclasses + shutdown_options_map = {'halt': '-H', 'poweroff': '-P', 'reboot': '-r'} def __init__(self, name, cfg, paths): self._paths = paths @@ -751,8 +754,7 @@ def create_group(self, name, members=None): def shutdown_command(self, mode='poweroff', delay='now', message=None): # called from cc_power_state_change.load_power_state - opt_map = {'halt': '-H', 'poweroff': '-P', 'reboot': '-r'} - command = ["shutdown", opt_map[mode]] + command = ["shutdown", self.shutdown_options_map[mode]] try: if delay != "now": delay = "+%d" % int(delay) diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py index 14e9421fda1..40be076f786 100644 --- a/cloudinit/distros/alpine.py +++ b/cloudinit/distros/alpine.py @@ -178,9 +178,8 @@ def shutdown_command(self, mode='poweroff', delay='now', message=None): # Alpine's commands do not understand "now". command += ['0'] else: - # No "+" in front of delay value (not supported) try: - command += ['%d' % int(int(delay) * 60)] + command.append(str(int(delay) * 60)) except ValueError as e: raise TypeError( "power_state[delay] must be 'now' or '+m' (minutes)." diff --git a/cloudinit/distros/bsd.py b/cloudinit/distros/bsd.py index daccc17b8c9..f717a667681 100644 --- a/cloudinit/distros/bsd.py +++ b/cloudinit/distros/bsd.py @@ -17,6 +17,10 @@ class BSD(distros.Distro): hostname_conf_fn = '/etc/rc.conf' rc_conf_fn = "/etc/rc.conf" + # This differs from the parent Distro class, which has -P for + # poweroff. + shutdown_options_map = {'halt': '-H', 'poweroff': '-p', 'reboot': '-r'} + # Set in BSD distro subclasses group_add_cmd_prefix = [] pkg_cmd_install_prefix = [] @@ -127,24 +131,3 @@ def apply_locale(self, locale, out_fn=None): def apply_network_config_names(self, netconfig): LOG.debug('Cannot rename network interface.') - - def shutdown_command(self, mode='poweroff', delay='now', message=None): - # called from cc_power_state_change.load_power_state - - # direct copy from the generic shutdown() method, except that - # poweroff is with -p, not -P. - - opt_map = {'halt': '-H', 'poweroff': '-p', 'reboot': '-r'} - command = ["shutdown", opt_map[mode]] - try: - if delay != "now": - delay = "+%d" % int(delay) - except ValueError as e: - raise TypeError( - "power_state[delay] must be 'now' or '+m' (minutes)." - " found '%s'." % delay - ) from e - args = command + [delay] - if message: - args.append(message) - return args From aeca5e299bb81cff0b2463b15dd871d737e86955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Fri, 11 Sep 2020 16:03:04 +0200 Subject: [PATCH 5/7] load_power_state: modes_ok becomes distro.shutdown_options_map Co-authored-by: Daniel Watkins --- cloudinit/config/cc_power_state_change.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/config/cc_power_state_change.py b/cloudinit/config/cc_power_state_change.py index bd30a733b33..b0cfafcd29c 100644 --- a/cloudinit/config/cc_power_state_change.py +++ b/cloudinit/config/cc_power_state_change.py @@ -157,7 +157,7 @@ def load_power_state(cfg, distro): modes_ok = ['halt', 'poweroff', 'reboot'] mode = pstate.get("mode") - if mode not in modes_ok: + if mode not in distro.shutdown_options_map: raise TypeError( "power_state[mode] required, must be one of: %s. found: '%s'." % (','.join(modes_ok), mode)) From d80a16e9655acbf0a6d837e8167194e0c704e1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Fri, 11 Sep 2020 16:04:22 +0200 Subject: [PATCH 6/7] shutdown_command only eats keyword arguments Co-authored-by: Daniel Watkins --- cloudinit/distros/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index c2e8d9ff420..980e2d4d4c4 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -752,7 +752,7 @@ def create_group(self, name, members=None): subp.subp(['usermod', '-a', '-G', name, member]) LOG.info("Added user '%s' to group '%s'", member, name) - def shutdown_command(self, mode='poweroff', delay='now', message=None): + def shutdown_command(self, *, mode, delay, message): # called from cc_power_state_change.load_power_state command = ["shutdown", self.shutdown_options_map[mode]] try: From 7f38e56d98f57a27803ba19a440df4da3c377b48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Thom=C3=A9?= Date: Fri, 11 Sep 2020 16:05:08 +0200 Subject: [PATCH 7/7] shutdown_command: defend against "delay" being a tuple Co-authored-by: Daniel Watkins --- cloudinit/distros/__init__.py | 2 +- cloudinit/distros/alpine.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py index 980e2d4d4c4..7e3f8876be6 100755 --- a/cloudinit/distros/__init__.py +++ b/cloudinit/distros/__init__.py @@ -761,7 +761,7 @@ def shutdown_command(self, *, mode, delay, message): except ValueError as e: raise TypeError( "power_state[delay] must be 'now' or '+m' (minutes)." - " found '%s'." % delay + " found '%s'." % (delay,) ) from e args = command + [delay] if message: diff --git a/cloudinit/distros/alpine.py b/cloudinit/distros/alpine.py index 40be076f786..e92ff3fb8b4 100644 --- a/cloudinit/distros/alpine.py +++ b/cloudinit/distros/alpine.py @@ -183,7 +183,7 @@ def shutdown_command(self, mode='poweroff', delay='now', message=None): except ValueError as e: raise TypeError( "power_state[delay] must be 'now' or '+m' (minutes)." - " found '%s'." % delay + " found '%s'." % (delay,) ) from e return command