diff --git a/README.md b/README.md index bcd0302e..488d973c 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,8 @@ It is currently under active development and has not yet had a formal release. * The following sysctl settings: * `fs.aio-max-nr=1048576` * `kernel.pid_max=4194304` + * `kernel.keys.maxkeys=20000` and `kernel.keys.maxbytes=2000000`, so that + nested containers do not exhaust the rootless user's keyring quota * If using SELinux in enforcing mode: * `setsebool -P container_manage_cgroup=true` * `setsebool -P container_use_devices=true` @@ -126,6 +128,10 @@ It's possible to skip the automatic suite-scheduling behavior: export TEUTHOLOGY_SUITE=none ``` +These variables, like `SSH_AUTH_SOCK`, are read when the teuthology container +is created. Unless it is still running, `create` and `start` replace it, so +the environment of the current shell applies. + ### Using testnodes from an existing lab If you need to use "real" testnodes and have access to a lab, there are a few additonal steps to take. We will use the Sepia lab as an example below: @@ -192,4 +198,4 @@ ceph-devstack -v start ```bash ceph-devstack wait teuthology podman logs -f teuthology -``` \ No newline at end of file +``` diff --git a/ceph_devstack/ceph_devstack.pp b/ceph_devstack/ceph_devstack.pp index 04433ece..16b6045d 100644 Binary files a/ceph_devstack/ceph_devstack.pp and b/ceph_devstack/ceph_devstack.pp differ diff --git a/ceph_devstack/ceph_devstack.te b/ceph_devstack/ceph_devstack.te index f661669b..7439fd2a 100644 --- a/ceph_devstack/ceph_devstack.te +++ b/ceph_devstack/ceph_devstack.te @@ -22,7 +22,7 @@ require { class dir mounton; type proc_kcore_t; - class file mounton; + class file { getattr mounton open read }; type device_t; class filesystem remount; @@ -78,8 +78,10 @@ require { type tun_tap_device_t; - class sock_file write; + class sock_file { getattr write }; class unix_stream_socket connectto; + + type udev_var_run_t; } #============= container_init_t ============== @@ -125,3 +127,7 @@ allow container_init_t tun_tap_device_t:chr_file mounton; allow container_t user_tmp_t:sock_file write; allow container_t unconfined_t:unix_stream_socket connectto; + +allow container_init_t udev_var_run_t:dir { ioctl mounton read }; +allow container_init_t udev_var_run_t:file { getattr open read }; +allow container_init_t udev_var_run_t:sock_file getattr; diff --git a/ceph_devstack/exec.py b/ceph_devstack/exec.py index 9cc27def..1bddb991 100644 --- a/ceph_devstack/exec.py +++ b/ceph_devstack/exec.py @@ -8,7 +8,7 @@ import signal import subprocess -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Tuple from ceph_devstack import logger, VERBOSE @@ -70,6 +70,25 @@ def signal_children(self, signal: signal.Signals, recursive=True): with contextlib.suppress(ProcessLookupError): os.killpg(pid, signal) + async def collect_output(self) -> Tuple[str, str]: + stdout = b"" + stderr = b"" + if self.stdout is not None: + stdout = await self.stdout.read() + if self.stderr is not None: + stderr = await self.stderr.read() + return stdout.decode(), stderr.decode() + + async def log_failure(self, cmd: List[str]) -> Tuple[str, str]: + stdout, stderr = await self.collect_output() + returncode = self.returncode if self.returncode is not None else -1 + logger.error(f"Command failed ({returncode}): {' '.join(cmd)}") + for line in stderr.rstrip("\n").splitlines(): + logger.error(line) + for line in stdout.rstrip("\n").splitlines(): + logger.error(line) + return stdout, stderr + class Command: def __init__( diff --git a/ceph_devstack/host.py b/ceph_devstack/host.py index 3be43902..8c744acc 100644 --- a/ceph_devstack/host.py +++ b/ceph_devstack/host.py @@ -2,6 +2,7 @@ import logging import os import pathlib +import shlex import socket import sys import yaml @@ -149,7 +150,8 @@ class RemoteHost(Host): def _remote_args(self, args: List[str], stream_output: bool) -> List[str]: remote = list(self.base_args) remote.append("--") - remote.extend(args) + # Quote each arg so compound arguments like "bash -c ''" stay as one shell word + remote.extend(shlex.quote(arg) for arg in args) return remote def cmd( diff --git a/ceph_devstack/requirements.py b/ceph_devstack/requirements.py index 1291dbea..5fbaf8c7 100644 --- a/ceph_devstack/requirements.py +++ b/ceph_devstack/requirements.py @@ -296,5 +296,10 @@ async def check_requirements(): # sysctl settings for OSD result = result and await SysctlValue("fs.aio-max-nr", 2097152).evaluate() result = result and await SysctlValue("kernel.pid_max", 4194304).evaluate() + # Every nested container gets a session keyring charged to the rootless + # user; the defaults (200 keys, 20000 bytes) run out with a few + # cephadm clusters ("crun: join keyctl ...: Disk quota exceeded"). + result = result and await SysctlValue("kernel.keys.maxkeys", 20000).evaluate() + result = result and await SysctlValue("kernel.keys.maxbytes", 2000000).evaluate() return result diff --git a/ceph_devstack/resources/__init__.py b/ceph_devstack/resources/__init__.py index 0a3c2a10..b9607191 100644 --- a/ceph_devstack/resources/__init__.py +++ b/ceph_devstack/resources/__init__.py @@ -65,9 +65,13 @@ async def cmd( ) returncode = await proc.wait() if check and returncode != 0: - # out = await proc.stderr.read() - # logger.error(out.decode()) - raise CalledProcessError(cmd=args, returncode=returncode) + stdout, stderr = await proc.log_failure(args) + raise CalledProcessError( + returncode, + args, + output=stdout or None, + stderr=stderr or None, + ) return proc def format_cmd(self, args: List): diff --git a/ceph_devstack/resources/ceph/__init__.py b/ceph_devstack/resources/ceph/__init__.py index 17cc5641..5864c1b6 100644 --- a/ceph_devstack/resources/ceph/__init__.py +++ b/ceph_devstack/resources/ceph/__init__.py @@ -196,7 +196,10 @@ async def remove(self): await SSHKeyPair().remove() async def watch(self): - logger.info("Watching containers; will replace any that are stopped") + logger.info( + "Watching containers; will replace any that are removed and, " + "except for teuthology, restart any that are stopped" + ) containers = [] for spec in self.service_specs.values(): if not spec["count"] > 0: @@ -213,7 +216,9 @@ async def watch(self): ) await container.create() await container.start() - elif not await container.is_running(): + elif not await container.is_running() and not isinstance( + container, Teuthology + ): logger.info(f"Container {container.name} stopped; restarting") await container.start() diff --git a/ceph_devstack/resources/ceph/containers.py b/ceph_devstack/resources/ceph/containers.py index 934afa37..1d3807c7 100644 --- a/ceph_devstack/resources/ceph/containers.py +++ b/ceph_devstack/resources/ceph/containers.py @@ -4,7 +4,7 @@ from pathlib import Path from typing import List -from ceph_devstack import config, DEFAULT_CONFIG_PATH +from ceph_devstack import config, DEFAULT_CONFIG_PATH, logger from ceph_devstack.host import host from ceph_devstack.resources.container import Container @@ -210,6 +210,11 @@ def create_cmd(self): ",".join(self.capabilities), "--security-opt", "unmask=/sys/dev/block", + # With this container's /proc paths masked, crun inside it fails + # to mask them for nested containers (e.g. "mount `tmpfs` to + # `proc/acpi`: Permission denied", seen with podman 5.8/crun 1.29). + "--security-opt", + "unmask=/proc/*", "-v", "/sys/dev/block:/sys/dev/block", "-v", @@ -218,6 +223,8 @@ def create_cmd(self): "/dev/fuse:/dev/fuse", "-v", "/dev/disk:/dev/disk", + "-v", + "/run/udev:/run/udev:ro", *self._get_available_dmi_mounts(), *self.additional_volumes, "--device", @@ -376,7 +383,7 @@ def create_cmd(self): f"{ansible_inv}/secrets:/etc/ansible/secrets", ] ssh_auth_socket = os.environ.get("SSH_AUTH_SOCK") - if ssh_auth_socket and Path(ssh_auth_socket).exists(): + if ssh_auth_socket and host.path_exists(ssh_auth_socket): cmd += [ "-v", f"{ssh_auth_socket}:{ssh_auth_socket}", @@ -424,4 +431,11 @@ def archive_dir(self) -> Path: async def create(self): self.archive_dir.expanduser().resolve().mkdir(parents=True, exist_ok=True) + # The container keeps the environment it was created with: the + # TEUTHOLOGY_* settings and an SSH agent socket that may be gone by + # its next start. A stopped one holds nothing the archive does not, + # so replace it with one built from the current environment. + if await self.exists() and not await self.is_running(): + logger.info(f"{self.name}: recreating from the current environment") + await self.remove() await super().create() diff --git a/ceph_devstack/resources/ceph/requirements.py b/ceph_devstack/resources/ceph/requirements.py index 00d79678..7a19d3e9 100644 --- a/ceph_devstack/resources/ceph/requirements.py +++ b/ceph_devstack/resources/ceph/requirements.py @@ -1,3 +1,8 @@ +import hashlib +import re + +from subprocess import CalledProcessError + from ceph_devstack import logger, PROJECT_ROOT from ceph_devstack.requirements import Requirement, FixableRequirement @@ -43,43 +48,20 @@ async def suggest(self): class SELinuxModule(FixableRequirement): + MODULE_NAME = "ceph_devstack" + SHIPPED_PP = PROJECT_ROOT / f"{MODULE_NAME}.pp" + def __init__(self): fix_cmd = self.fix_cmd_prebuilt if self.host.type == "remote": fix_cmd = ["podman", "machine", "ssh", "--"] + fix_cmd self.fix_cmd = fix_cmd - fix_cmd_build = [ - "(sudo", - "dnf", - "install", - "-y", - "policycoreutils-devel", - "selinux-policy-devel", - "&&", - "cd", - str(PROJECT_ROOT), - "&&", - "make", - "-f", - "/usr/share/selinux/devel/Makefile", - "ceph_devstack.pp", - "&&", - "sudo", - "semodule", - "-i", - "ceph_devstack.pp)", - ] - fix_cmd_prebuilt = [ - "sudo", - "semodule", - "-i", - str(PROJECT_ROOT / "ceph_devstack.pp"), - ] + fix_cmd_prebuilt = ["sudo", "semodule", "-i", str(SHIPPED_PP)] suggest_msg = ( "SELinux is in Enforcing mode. To run nested rootless podman " "containers, it is necessary to install ceph-devstack's SELinux " - "module" + "module (module may be missing or outdated)" ) async def check(self): @@ -87,4 +69,50 @@ async def check(self): assert proc.stdout is not None await proc.wait() out = (await proc.stdout.read()).decode() - return "ceph_devstack" in out.split("\n") + if self.MODULE_NAME not in out.split("\n"): + return False + if not self.SHIPPED_PP.exists(): + logger.warning( + f"Shipped SELinux module not found at {self.SHIPPED_PP}; " + "skipping hash check" + ) + return True + installed_hash = await self._installed_module_hash() + if installed_hash is None: + logger.warning( + "Could not extract the installed SELinux module; " + "falling back to presence-only check" + ) + return True + shipped_hash = hashlib.sha256(self.SHIPPED_PP.read_bytes()).hexdigest() + if installed_hash != shipped_hash: + logger.warning( + f"SELinux module hash mismatch: shipped={shipped_hash}, " + f"installed={installed_hash}" + ) + return False + return True + + async def _installed_module_hash(self) -> str | None: + """sha256 of the installed module, or None if it can't be determined.""" + # semodule -E writes a byte-identical .pp into the cwd (and chatters + # on stdout, hence the redirect); always clean up the temp dir. + script = ( + "d=$(mktemp -d) || exit 1; " + "trap 'sudo rm -rf \"$d\"' EXIT; " + 'cd "$d" && ' + f"sudo semodule -E {self.MODULE_NAME} >/dev/null 2>&1 && " + f"sha256sum {self.MODULE_NAME}.pp" + ) + try: + proc = await self.host.arun(["bash", "-c", script]) + assert proc.stdout is not None + if await proc.wait() != 0: + return None + out = (await proc.stdout.read()).decode().split() + except (OSError, CalledProcessError): + return None + # Output is "hash filename"; anything else is unparseable. + if out and re.fullmatch(r"[0-9a-f]{64}", out[0]): + return out[0] + return None diff --git a/tests/resources/ceph/test_cephdevstack_core.py b/tests/resources/ceph/test_cephdevstack_core.py index d30a1625..fa22590e 100644 --- a/tests/resources/ceph/test_cephdevstack_core.py +++ b/tests/resources/ceph/test_cephdevstack_core.py @@ -287,6 +287,30 @@ async def test_stop_calls_stop_on_all_containers(self): mock_paddles.stop.assert_called_once() +class _StopWatching(Exception): + pass + + +class TestCephDevStackWatch: + async def test_restarts_stopped_containers_except_teuthology(self): + devstack = CephDevStack() + teuth = AsyncMock(spec=Teuthology) + paddles = AsyncMock(spec=Paddles) + for obj, name in ((teuth, "teuthology"), (paddles, "paddles")): + obj.name = name + obj.exists.return_value = True + obj.is_running.return_value = False + paddles.start.side_effect = _StopWatching + devstack.service_specs = { + "teuthology": {"count": 1, "objects": [teuth]}, + "paddles": {"count": 1, "objects": [paddles]}, + } + with pytest.raises(_StopWatching): + await devstack.watch() + teuth.start.assert_not_awaited() + paddles.start.assert_awaited_once() + + class TestCephDevStackWait: async def test_wait_returns_process_id(self): devstack = CephDevStack() diff --git a/tests/resources/ceph/test_requirements_ceph.py b/tests/resources/ceph/test_requirements_ceph.py index cc4b58a7..3e93261f 100644 --- a/tests/resources/ceph/test_requirements_ceph.py +++ b/tests/resources/ceph/test_requirements_ceph.py @@ -1,3 +1,5 @@ +import hashlib +import pytest from unittest.mock import AsyncMock, MagicMock, patch from ceph_devstack import config @@ -126,38 +128,62 @@ async def side_effect_arun(args): assert "chgrp" in " ".join(self.req.fix_cmd) -class TestSELinuxModule: - def setup_method(self): - self.req = SELinuxModule() +SHIPPED = b"shipped module" +SHIPPED_HASH = hashlib.sha256(SHIPPED).hexdigest().encode() +LISTED = b"ceph_devstack\nother_module\n" - async def test_selinux_module_check_true(self): - mock_proc = AsyncMock() - mock_proc.stdout = AsyncMock() - mock_proc.stdout.read = AsyncMock(return_value=b"ceph_devstack\nother_module\n") - mock_proc.wait = AsyncMock(return_value=0) - with patch.object(self.req.host, "arun", return_value=mock_proc): - result = await self.req.check() - assert result is True - async def test_selinux_module_check_false(self): - mock_proc = AsyncMock() - mock_proc.stdout = AsyncMock() - mock_proc.stdout.read = AsyncMock( - return_value=b"other_module\nanother_module\n" - ) - mock_proc.wait = AsyncMock(return_value=0) - with patch.object(self.req.host, "arun", return_value=mock_proc): - result = await self.req.check() - assert result is False +def _proc(returncode: int, stdout: bytes) -> AsyncMock: + proc = AsyncMock() + proc.stdout.read = AsyncMock(return_value=stdout) + proc.wait = AsyncMock(return_value=returncode) + return proc - async def test_selinux_module_check_empty_output(self): - mock_proc = AsyncMock() - mock_proc.stdout = AsyncMock() - mock_proc.stdout.read = AsyncMock(return_value=b"") - mock_proc.wait = AsyncMock(return_value=0) - with patch.object(self.req.host, "arun", return_value=mock_proc): - result = await self.req.check() - assert result is False + +class TestSELinuxModule: + @pytest.fixture + def req(self, tmp_path): + shipped_pp = tmp_path / "ceph_devstack.pp" + shipped_pp.write_bytes(SHIPPED) + with patch.object(SELinuxModule, "SHIPPED_PP", shipped_pp): + yield SELinuxModule() + + @pytest.mark.parametrize( + "listed, extract_rc, extract_out, expected", + [ + pytest.param(b"other_module\n", 0, b"", False, id="not-listed"), + pytest.param( + LISTED, 0, SHIPPED_HASH + b" ceph_devstack.pp\n", True, id="current" + ), + pytest.param( + LISTED, 0, b"0" * 64 + b" ceph_devstack.pp\n", False, id="stale" + ), + # Any failure to read the installed module must not read as + # "stale", otherwise doctor --fix would reinstall forever. + pytest.param(LISTED, 1, b"", True, id="extract-failed"), + pytest.param( + LISTED, + 0, + b"Extracting at highest existing priority '400'.\n", + True, + id="unparseable", + ), + ], + ) + async def test_check(self, req, listed, extract_rc, extract_out, expected): + procs = [_proc(0, listed), _proc(extract_rc, extract_out)] + with patch.object(req.host, "arun", side_effect=procs): + assert await req.check() is expected + + async def test_check_falls_back_when_extraction_cannot_run(self, req): + procs = [_proc(0, LISTED), OSError("bash not found")] + with patch.object(req.host, "arun", side_effect=procs): + assert await req.check() is True + + async def test_check_without_shipped_module_is_presence_only(self, req): + req.SHIPPED_PP.unlink() + with patch.object(req.host, "arun", side_effect=[_proc(0, LISTED)]): + assert await req.check() is True class TestSELinuxModuleFixCmd: diff --git a/tests/resources/ceph/test_testnode.py b/tests/resources/ceph/test_testnode.py index 69271fb1..6971afc5 100644 --- a/tests/resources/ceph/test_testnode.py +++ b/tests/resources/ceph/test_testnode.py @@ -26,6 +26,16 @@ def test_testnode_create_cmd_includes_related_devices(self, cls): assert "--device=/dev/loop6" in create_cmd assert "--device=/dev/loop7" in create_cmd + def test_testnode_create_cmd_mounts_run_udev_read_only(self, cls): + create_cmd = cls("testnode_1").create_cmd + index = create_cmd.index("/run/udev:/run/udev:ro") + assert create_cmd[index - 1] == "-v" + + def test_testnode_create_cmd_unmasks_proc_for_nested_containers(self, cls): + create_cmd = cls("testnode_1").create_cmd + index = create_cmd.index("unmask=/proc/*") + assert create_cmd[index - 1] == "--security-opt" + def test_testnode_devices_is_based_on_loop_device_count_config(self, cls): config.load(Path(__file__).parent.joinpath("fixtures", "testnode-config.toml")) testnode = cls("testnode_1") diff --git a/tests/resources/ceph/test_teuthology.py b/tests/resources/ceph/test_teuthology.py new file mode 100644 index 00000000..69674e6c --- /dev/null +++ b/tests/resources/ceph/test_teuthology.py @@ -0,0 +1,62 @@ +from unittest.mock import AsyncMock, patch + +import pytest + +from ceph_devstack import config +from ceph_devstack.resources.ceph.containers import Teuthology + + +class TestTeuthologyCreate: + @pytest.fixture + def teuth(self, tmp_path): + config["data_dir"] = str(tmp_path) + teuth = Teuthology() + teuth.remove = AsyncMock() + return teuth + + @pytest.fixture + def podman_create(self): + with patch( + "ceph_devstack.resources.container.Container.create", new=AsyncMock() + ) as podman_create: + yield podman_create + + @pytest.mark.parametrize( + "exists,running,replaced", + [ + (False, False, False), + (True, True, False), + (True, False, True), + ], + ids=["missing", "running", "stopped"], + ) + async def test_replaces_only_a_stopped_container( + self, teuth, podman_create, exists, running, replaced + ): + teuth.exists = AsyncMock(return_value=exists) + teuth.is_running = AsyncMock(return_value=running) + await teuth.create() + assert teuth.remove.await_count == int(replaced) + podman_create.assert_awaited_once() + assert teuth.archive_dir.is_dir() + + +class TestTeuthologySSHAuthSock: + SOCK = "/tmp/ssh-XXXX/agent.123" + + def create_cmd(self, monkeypatch, sock_exists: bool) -> list[str]: + monkeypatch.setenv("SSH_AUTH_SOCK", self.SOCK) + with patch( + "ceph_devstack.resources.ceph.containers.host.path_exists", + side_effect=lambda path: sock_exists and str(path) == self.SOCK, + ): + return Teuthology().create_cmd + + def test_mounts_socket_present_on_container_host(self, monkeypatch): + cmd = self.create_cmd(monkeypatch, sock_exists=True) + assert f"{self.SOCK}:{self.SOCK}" in cmd + assert f"SSH_AUTH_SOCK={self.SOCK}" in cmd + + def test_skips_socket_missing_on_container_host(self, monkeypatch): + cmd = self.create_cmd(monkeypatch, sock_exists=False) + assert not any("SSH_AUTH_SOCK" in arg or self.SOCK in arg for arg in cmd) diff --git a/tests/resources/test_podmanresource.py b/tests/resources/test_podmanresource.py index 3ee8b6e4..2d181cdd 100644 --- a/tests/resources/test_podmanresource.py +++ b/tests/resources/test_podmanresource.py @@ -1,10 +1,11 @@ +import logging import pytest from pathlib import Path from subprocess import CalledProcessError -from unittest.mock import patch - +from unittest.mock import AsyncMock, patch +from ceph_devstack.exec import Subprocess from ceph_devstack.resources import PodmanResource @@ -54,7 +55,25 @@ async def test_cmd(self, cls): print(m_arun.await_args_list) m_arun.assert_awaited_once_with(["0"], cwd=Path("."), stream_output=False) - async def test_cmd_failed(self, cls): - obj = cls() - with pytest.raises(CalledProcessError): - await obj.cmd(["false"], check=True) + async def test_cmd_failed(self, cls, caplog): + class FakeProc: + returncode = 1 + collect_output = Subprocess.collect_output + log_failure = Subprocess.log_failure + + def __init__(self): + self.stdout = AsyncMock() + self.stdout.read = AsyncMock(return_value=b"") + self.stderr = AsyncMock() + self.stderr.read = AsyncMock(return_value=b"podman-failure\n") + + async def wait(self): + return 1 + + with patch("ceph_devstack.host.host.arun", return_value=FakeProc()) as m_arun: + obj = cls() + with caplog.at_level(logging.ERROR), pytest.raises(CalledProcessError): + await obj.cmd(["podman", "fail"], check=True) + m_arun.assert_awaited_once() + assert "Command failed (1)" in caplog.text + assert "podman-failure" in caplog.text diff --git a/tests/test_host.py b/tests/test_host.py index 997dd3c6..6588576e 100644 --- a/tests/test_host.py +++ b/tests/test_host.py @@ -1,6 +1,6 @@ import pytest -from ceph_devstack.host import LocalHost +from ceph_devstack.host import LocalHost, RemoteHost class TestLocalHost: @@ -11,3 +11,22 @@ def host(self): def test_path_exists_expands_tilde(self, host): assert host.path_exists("~") is True assert host.path_exists("~/nonexistent_dir_12345") is False + + +SSH = ["podman", "machine", "ssh", "--"] + + +class TestRemoteHost: + @pytest.mark.parametrize( + "args,expected", + [ + (["podman", "ps"], ["podman", "ps"]), + ( + ["sudo", "losetup", "/dev/loop0"], + SSH + ["sudo", "losetup", "/dev/loop0"], + ), + (["bash", "-c", "echo hi; ls"], SSH + ["bash", "-c", "'echo hi; ls'"]), + ], + ) + def test_cmd_runs_non_podman_commands_in_machine(self, args, expected): + assert RemoteHost().cmd(args).args == expected diff --git a/tests/test_requirements_core.py b/tests/test_requirements_core.py index 0d1a325a..63937706 100644 --- a/tests/test_requirements_core.py +++ b/tests/test_requirements_core.py @@ -414,6 +414,9 @@ async def test_check_requirements_returns_true_when_all_pass(self): MockSysctl.return_value = AsyncMock(evaluate=AsyncMock(return_value=True)) result = await requirements.check_requirements() assert result is True + checked = [c.args for c in MockSysctl.call_args_list] + assert ("kernel.keys.maxkeys", 20000) in checked + assert ("kernel.keys.maxbytes", 2000000) in checked async def test_check_requirements_returns_false_on_runtime_failure(self): with (