Skip to content

Commit

Permalink
fix(cmd): Do not hardcode reboot command (#5208)
Browse files Browse the repository at this point in the history 
Attempting a `cloud-init clean --reboot` fails on alpine because this
command is hard-coded. This code already has an Init object available,
which has a Distro attribute. Stamp out the duplicate hard-coded
implementation and re-use distro reboot code to acquire cross-distro
compatibility.

Error:
Could not reboot this system using "['shutdown', '-r', 'now']": Unexpected error while running comma  nd.
Command: ['shutdown', '-r', 'now']
Exit code: -
Reason: [Errno 2] No such file or directory: b'shutdown'
Stdout: -
Stderr: -
  • Loading branch information
@holmanb
holmanb committed May 1, 2024
1 parent 44ad8ac commit efd37c0
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 86 deletions.
11 changes: 7 additions & 4 deletions cloudinit/cmd/clean.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,10 @@ def get_parser(parser=None):
return parser


def remove_artifacts(remove_logs, remove_seed=False, remove_config=None):
def remove_artifacts(init, remove_logs, remove_seed=False, remove_config=None):
"""Helper which removes artifacts dir and optionally log files.
@param: init: Init object to use
@param: remove_logs: Boolean. Set True to delete the cloud_dir path. False
preserves them.
@param: remove_seed: Boolean. Set True to also delete seed subdir in
Expand All @@ -117,7 +118,6 @@ def remove_artifacts(remove_logs, remove_seed=False, remove_config=None):
Can be any of: all, network, ssh_config.
@returns: 0 on success, 1 otherwise.
"""
init = Init(ds_deps=[])
init.read_cfg()
if remove_logs:
for log_file in get_config_logfiles(init.cfg):
Expand Down Expand Up @@ -158,8 +158,9 @@ def remove_artifacts(remove_logs, remove_seed=False, remove_config=None):

def handle_clean_args(name, args):
"""Handle calls to 'cloud-init clean' as a subcommand."""
init = Init(ds_deps=[])
exit_code = remove_artifacts(
args.remove_logs, args.remove_seed, args.remove_config
init, args.remove_logs, args.remove_seed, args.remove_config
)
if args.machine_id:
if uses_systemd():
Expand All @@ -169,7 +170,9 @@ def handle_clean_args(name, args):
# Non-systemd like FreeBSD regen machine-id when file is absent
del_file(ETC_MACHINE_ID)
if exit_code == 0 and args.reboot:
cmd = ["shutdown", "-r", "now"]
cmd = init.distro.shutdown_command(
mode="reboot", delay="now", message=None
)
try:
subp(cmd, capture=False)
except ProcessExecutionError as e:
Expand Down
5 changes: 3 additions & 2 deletions cloudinit/distros/__init__.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1134,9 +1134,10 @@ 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, delay, message):
@classmethod
def shutdown_command(cls, *, mode, delay, message):
# called from cc_power_state_change.load_power_state
command = ["shutdown", self.shutdown_options_map[mode]]
command = ["shutdown", cls.shutdown_options_map[mode]]
try:
if delay != "now":
delay = "+%d" % int(delay)
Expand Down
128 changes: 48 additions & 80 deletions tests/unittests/cmd/test_clean.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import cloudinit.settings
from cloudinit.cmd import clean
from cloudinit.distros import Distro
from cloudinit.stages import Init
from cloudinit.util import ensure_dir, sym_link, write_file
from tests.unittests.helpers import mock, wrap_and_call

Expand All @@ -30,21 +32,14 @@ def clean_paths(tmpdir):

@pytest.fixture(scope="function")
def init_class(clean_paths):
class FakeInit:
cfg = {
"def_log_file": clean_paths.log,
"output": {"all": f"|tee -a {clean_paths.output_log}"},
}
# Ensure cloud_dir has a trailing slash, to match real behaviour
paths = MyPaths(cloud_dir=f"{clean_paths.cloud_dir}/")

def __init__(self, ds_deps):
pass

def read_cfg(self):
pass

return FakeInit
init = mock.Mock(spec=Init)
init.paths = MyPaths(cloud_dir=f"{clean_paths.cloud_dir}/")
init.distro.shutdown_command = Distro.shutdown_command
init.cfg = {
"def_log_file": clean_paths.log,
"output": {"all": f"|tee -a {clean_paths.output_log}"},
}
return init


class TestClean:
Expand All @@ -56,10 +51,8 @@ def test_remove_artifacts_removes_logs(self, clean_paths, init_class):
assert (
os.path.exists(clean_paths.cloud_dir) is False
), "Unexpected cloud_dir"
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init=init_class,
remove_logs=True,
)
assert (
Expand Down Expand Up @@ -93,10 +86,8 @@ def test_remove_net_conf(self, clean_paths, init_class):
"cloudinit.cmd.clean.GEN_NET_CONFIG_FILES",
[f.strpath for f in TEST_GEN_NET_CONFIG_FILES],
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
remove_config=["network"],
)
Expand Down Expand Up @@ -130,10 +121,8 @@ def test_remove_ssh_conf(self, clean_paths, init_class):
"cloudinit.cmd.clean.GEN_SSH_CONFIG_FILES",
TEST_GEN_SSH_CONFIG_FILES,
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
remove_config=["ssh_config"],
)
Expand Down Expand Up @@ -170,10 +159,8 @@ def test_remove_all_conf(self, clean_paths, init_class):
"cloudinit.cmd.clean.GEN_SSH_CONFIG_FILES",
TEST_GEN_SSH_CONFIG_FILES,
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
remove_config=["all"],
)
Expand All @@ -200,10 +187,8 @@ def test_keep_net_conf(self, clean_paths, init_class):
"cloudinit.cmd.clean.GEN_NET_CONFIG_FILES",
TEST_GEN_NET_CONFIG_FILES,
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
remove_config=[],
)
Expand All @@ -225,12 +210,8 @@ def test_remove_artifacts_runparts_clean_d(self, clean_paths, init_class):
with mock.patch.object(
cloudinit.settings, "CLEAN_RUNPARTS_DIR", clean_paths.clean_dir
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{
"Init": {"side_effect": init_class},
},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
)
assert (
Expand All @@ -243,10 +224,8 @@ def test_remove_artifacts_preserves_logs(self, clean_paths, init_class):
clean_paths.log.write("cloud-init-log")
clean_paths.output_log.write("cloud-init-output-log")

retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
)
assert 0 == retcode
Expand All @@ -269,10 +248,8 @@ def test_remove_artifacts_removes_unlinks_symlinks(
with mock.patch.object(
cloudinit.settings, "CLEAN_RUNPARTS_DIR", clean_paths.clean_dir
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
)
assert 0 == retcode
Expand All @@ -295,10 +272,8 @@ def test_remove_artifacts_removes_artifacts_skipping_seed(
with mock.patch.object(
cloudinit.settings, "CLEAN_RUNPARTS_DIR", clean_paths.clean_dir
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
)
assert 0 == retcode
Expand All @@ -323,10 +298,8 @@ def test_remove_artifacts_removes_artifacts_removes_seed(
with mock.patch.object(
cloudinit.settings, "CLEAN_RUNPARTS_DIR", clean_paths.clean_dir
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{"Init": {"side_effect": init_class}},
clean.remove_artifacts,
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
remove_seed=True,
)
Expand All @@ -346,15 +319,13 @@ def test_remove_artifacts_returns_one_on_errors(
ensure_dir(clean_paths.cloud_dir)
ensure_dir(clean_paths.cloud_dir.join("dir1"))

retcode = wrap_and_call(
"cloudinit.cmd.clean",
{
"del_dir": {"side_effect": OSError("oops")},
"Init": {"side_effect": init_class},
},
clean.remove_artifacts,
remove_logs=False,
)
with mock.patch(
"cloudinit.cmd.clean.del_dir", side_effect=OSError("oops")
):
retcode = clean.remove_artifacts(
init_class,
remove_logs=False,
)
assert 1 == retcode
_out, err = capsys.readouterr()
assert (
Expand All @@ -368,7 +339,7 @@ def test_handle_clean_args_reboots(self, init_class):
called_cmds = []

def fake_subp(cmd, capture):
called_cmds.append((cmd, capture))
called_cmds.append(cmd)
return "", ""

myargs = namedtuple(
Expand All @@ -385,14 +356,14 @@ def fake_subp(cmd, capture):
"cloudinit.cmd.clean",
{
"subp": {"side_effect": fake_subp},
"Init": {"side_effect": init_class},
"Init": {"return_value": init_class},
},
clean.handle_clean_args,
name="does not matter",
args=cmdargs,
)
assert 0 == retcode
assert [(["shutdown", "-r", "now"], False)] == called_cmds
assert [["shutdown", "-r", "now"]] == called_cmds

@pytest.mark.parametrize(
"machine_id,systemd_val",
Expand Down Expand Up @@ -428,16 +399,13 @@ def test_handle_clean_args_removed_machine_id(
with mock.patch.object(
cloudinit.cmd.clean, "ETC_MACHINE_ID", machine_id_path.strpath
):
retcode = wrap_and_call(
"cloudinit.cmd.clean",
{
"Init": {"side_effect": init_class},
},
clean.handle_clean_args,
name="does not matter",
args=cmdargs,
)
assert 0 == retcode
with mock.patch(
"cloudinit.cmd.clean.Init", return_value=init_class
):
assert 0 == clean.handle_clean_args(
name="does not matter",
args=cmdargs,
)
if systemd_val:
if machine_id:
assert "uninitialized\n" == machine_id_path.read()
Expand All @@ -453,7 +421,7 @@ def test_status_main(self, clean_paths, init_class):
wrap_and_call(
"cloudinit.cmd.clean",
{
"Init": {"side_effect": init_class},
"Init": {"return_value": init_class},
"sys.argv": {"new": ["clean", "--logs"]},
},
clean.main,
Expand Down

0 comments on commit efd37c0

Please sign in to comment.