From f09b27910e4a9a756e2480724e57122bedbd4f06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cerm=C3=A1k?= Date: Wed, 3 Sep 2025 16:07:15 +0200 Subject: [PATCH 1/2] Write cidfiles of Docker containers and mount them individually to /run/cid There is no standard way to get the container ID in the container itself, which can be needed for instance for #6006. The usual pattern is to use the --cidfile argument of Docker CLI and mount the generated file to the container. However, this is feature of Docker CLI and we can't use it when creating the containers via API. To get container ID to implement native logging in e.g. Core as well, we need the help of the Supervisor. This change implements similar feature fully in Supervisor's DockerAPI class that orchestrates lifetime of all containers managed by Supervisor. The files are created in the SUPERVISOR_DATA directory, as it needs to be persisted between reboots, just as the instances of Docker containers are. Supervisor's cidfile must be created when starting the Supervisor itself, for that see home-assistant/operating-system#4276. --- supervisor/bootstrap.py | 4 + supervisor/config.py | 6 ++ supervisor/docker/manager.py | 22 ++++ tests/conftest.py | 1 + tests/docker/test_addon.py | 3 + tests/docker/test_manager.py | 169 +++++++++++++++++++++++++++++- tests/plugins/test_plugin_base.py | 3 + 7 files changed, 207 insertions(+), 1 deletion(-) diff --git a/supervisor/bootstrap.py b/supervisor/bootstrap.py index a77587bfb62..60154259cc7 100644 --- a/supervisor/bootstrap.py +++ b/supervisor/bootstrap.py @@ -226,6 +226,10 @@ def initialize_system(coresys: CoreSys) -> None: ) config.path_addon_configs.mkdir() + if not config.path_cidfiles.is_dir(): + _LOGGER.debug("Creating Docker cidfiles folder at '%s'", config.path_cidfiles) + config.path_cidfiles.mkdir() + def warning_handler(message, category, filename, lineno, file=None, line=None): """Warning handler which logs warnings using the logging module.""" diff --git a/supervisor/config.py b/supervisor/config.py index 83c9e9380dc..620a555a390 100644 --- a/supervisor/config.py +++ b/supervisor/config.py @@ -54,6 +54,7 @@ EMERGENCY_DATA = PurePath("emergency") ADDON_CONFIGS = PurePath("addon_configs") CORE_BACKUP_DATA = PurePath("core/backup") +CIDFILES = PurePath("cidfiles") DEFAULT_BOOT_TIME = datetime.fromtimestamp(0, UTC).isoformat() @@ -399,6 +400,11 @@ def path_extern_media(self) -> PurePath: """Return root media data folder external for Docker.""" return PurePath(self.path_extern_supervisor, MEDIA_DATA) + @property + def path_cidfiles(self) -> Path: + """Return CID files folder.""" + return self.path_supervisor / CIDFILES + @property def addons_repositories(self) -> list[str]: """Return list of custom Add-on repositories.""" diff --git a/supervisor/docker/manager.py b/supervisor/docker/manager.py index e67bbc9348c..83e47c83469 100644 --- a/supervisor/docker/manager.py +++ b/supervisor/docker/manager.py @@ -321,11 +321,29 @@ def run( if not network_mode: kwargs["network"] = None + # Setup cidfile and bind mount it + cidfile_path = None + if name: + cidfile_path = self.coresys.config.path_cidfiles / f"{name}.cid" + + # Remove the file if it exists e.g. as a leftover from unclean shutdown + if cidfile_path.is_file(): + with suppress(OSError): + cidfile_path.unlink(missing_ok=True) + + # Bind mount to /run/cid in container + if "volumes" not in kwargs: + kwargs["volumes"] = {} + kwargs["volumes"]["/run/cid"] = {"bind": str(cidfile_path), "mode": "ro"} + # Create container try: container = self.containers.create( f"{image}:{tag}", use_config_proxy=False, **kwargs ) + if cidfile_path: + with cidfile_path.open("w", encoding="ascii") as cidfile: + cidfile.write(str(container.id)) except docker_errors.NotFound as err: raise DockerNotFound( f"Image {image}:{tag} does not exist for {name}", _LOGGER.error @@ -563,6 +581,10 @@ def stop_container( _LOGGER.info("Cleaning %s application", name) docker_container.remove(force=True, v=True) + cidfile_path = self.coresys.config.path_cidfiles / f"{name}.cid" + with suppress(OSError): + cidfile_path.unlink(missing_ok=True) + def start_container(self, name: str) -> None: """Start Docker container.""" try: diff --git a/tests/conftest.py b/tests/conftest.py index f14fbf09e38..1e3807c0bc0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -488,6 +488,7 @@ async def tmp_supervisor_data(coresys: CoreSys, tmp_path: Path) -> Path: coresys.config.path_addon_configs.mkdir(parents=True) coresys.config.path_ssl.mkdir() coresys.config.path_core_backup.mkdir(parents=True) + coresys.config.path_cidfiles.mkdir() yield tmp_path diff --git a/tests/docker/test_addon.py b/tests/docker/test_addon.py index 1b710e2a73f..37700bf9348 100644 --- a/tests/docker/test_addon.py +++ b/tests/docker/test_addon.py @@ -347,6 +347,7 @@ async def test_addon_run_add_host_error( addonsdata_system: dict[str, Data], capture_exception: Mock, path_extern, + tmp_supervisor_data: Path, ): """Test error adding host when addon is run.""" await coresys.dbus.timedate.connect(coresys.dbus.bus) @@ -433,6 +434,7 @@ async def test_addon_new_device( dev_path: str, cgroup: str, is_os: bool, + tmp_supervisor_data: Path, ): """Test new device that is listed in static devices.""" coresys.hardware.disk.get_disk_free_space = lambda x: 5000 @@ -463,6 +465,7 @@ async def test_addon_new_device_no_haos( install_addon_ssh: Addon, docker: DockerAPI, dev_path: str, + tmp_supervisor_data: Path, ): """Test new device that is listed in static devices on non HAOS system with CGroup V2.""" coresys.hardware.disk.get_disk_free_space = lambda x: 5000 diff --git a/tests/docker/test_manager.py b/tests/docker/test_manager.py index cd6e2b4a44c..59e99a37f9f 100644 --- a/tests/docker/test_manager.py +++ b/tests/docker/test_manager.py @@ -1,11 +1,13 @@ """Test Docker manager.""" -from unittest.mock import MagicMock +import asyncio +from unittest.mock import MagicMock, patch from docker.errors import DockerException import pytest from requests import RequestException +from supervisor.config import CIDFILES from supervisor.docker.manager import CommandReturn, DockerAPI from supervisor.exceptions import DockerError @@ -134,3 +136,168 @@ async def test_run_command_custom_stdout_stderr(docker: DockerAPI): # Verify the result assert result.exit_code == 0 assert result.output == b"output" + + +async def test_run_container_with_cidfile(docker: DockerAPI, tmp_supervisor_data): + """Test container creation with cidfile and bind mount.""" + # Mock container + mock_container = MagicMock() + mock_container.id = "test_container_id_12345" + + container_name = "test_container" + cidfile_path = tmp_supervisor_data / CIDFILES / f"{container_name}.cid" + + docker.docker.containers.run.return_value = mock_container + + # Mock container creation + with patch.object( + docker.containers, "create", return_value=mock_container + ) as create_mock: + # Execute run with a container name + loop = asyncio.get_event_loop() + result = await loop.run_in_executor( + None, + lambda kwrgs: docker.run(**kwrgs), + {"image": "test_image", "tag": "latest", "name": container_name}, + ) + + # Check the container creation parameters + create_mock.assert_called_once() + kwargs = create_mock.call_args[1] + + assert "volumes" in kwargs + assert "/run/cid" in kwargs["volumes"] + assert kwargs["volumes"]["/run/cid"]["bind"] == str(cidfile_path) + assert kwargs["volumes"]["/run/cid"]["mode"] == "ro" + + # Verify container start was called + mock_container.start.assert_called_once() + + # Verify cidfile was written with container ID + assert cidfile_path.exists() + assert cidfile_path.read_text() == mock_container.id + + assert result == mock_container + + +async def test_run_container_with_leftover_cidfile( + docker: DockerAPI, tmp_supervisor_data +): + """Test container creation removes leftover cidfile before creating new one.""" + # Mock container + mock_container = MagicMock() + mock_container.id = "test_container_id_new" + + container_name = "test_container" + cidfile_path = tmp_supervisor_data / "cidfiles" / f"{container_name}.cid" + + # Create a leftover cidfile + cidfile_path.touch() + + # Mock container creation + with patch.object( + docker.containers, "create", return_value=mock_container + ) as create_mock: + # Execute run with a container name + loop = asyncio.get_event_loop() + result = await loop.run_in_executor( + None, + lambda kwrgs: docker.run(**kwrgs), + {"image": "test_image", "tag": "latest", "name": container_name}, + ) + + # Verify container was created + create_mock.assert_called_once() + + # Verify new cidfile was written with container ID + assert cidfile_path.exists() + assert cidfile_path.read_text() == mock_container.id + + assert result == mock_container + + +async def test_stop_container_with_cidfile_cleanup( + docker: DockerAPI, tmp_supervisor_data +): + """Test container stop with cidfile cleanup.""" + # Mock container + mock_container = MagicMock() + mock_container.status = "running" + + container_name = "test_container" + cidfile_path = tmp_supervisor_data / "cidfiles" / f"{container_name}.cid" + + # Create a cidfile + cidfile_path.touch() + + # Mock the containers.get method and cidfile cleanup + with ( + patch.object(docker.containers, "get", return_value=mock_container), + ): + # Call stop_container with remove_container=True + loop = asyncio.get_event_loop() + await loop.run_in_executor( + None, + lambda kwrgs: docker.stop_container(**kwrgs), + {"timeout": 10, "remove_container": True, "name": container_name}, + ) + + # Verify container operations + mock_container.stop.assert_called_once_with(timeout=10) + mock_container.remove.assert_called_once_with(force=True, v=True) + + assert not cidfile_path.exists() + + +async def test_stop_container_without_removal_no_cidfile_cleanup(docker: DockerAPI): + """Test container stop without removal doesn't clean up cidfile.""" + # Mock container + mock_container = MagicMock() + mock_container.status = "running" + + container_name = "test_container" + + # Mock the containers.get method and cidfile cleanup + with ( + patch.object(docker.containers, "get", return_value=mock_container), + patch("pathlib.Path.unlink") as mock_unlink, + ): + # Call stop_container with remove_container=False + docker.stop_container(container_name, timeout=10, remove_container=False) + + # Verify container operations + mock_container.stop.assert_called_once_with(timeout=10) + mock_container.remove.assert_not_called() + + # Verify cidfile cleanup was NOT called + mock_unlink.assert_not_called() + + +async def test_cidfile_cleanup_handles_oserror(docker: DockerAPI, tmp_supervisor_data): + """Test that cidfile cleanup handles OSError gracefully.""" + # Mock container + mock_container = MagicMock() + mock_container.status = "running" + + container_name = "test_container" + cidfile_path = tmp_supervisor_data / "cidfiles" / f"{container_name}.cid" + + # Create a cidfile + cidfile_path.touch() + + # Mock the containers.get method and cidfile cleanup to raise OSError + with ( + patch.object(docker.containers, "get", return_value=mock_container), + patch( + "pathlib.Path.unlink", side_effect=OSError("File not found") + ) as mock_unlink, + ): + # Call stop_container - should not raise exception + docker.stop_container(container_name, timeout=10, remove_container=True) + + # Verify container operations completed + mock_container.stop.assert_called_once_with(timeout=10) + mock_container.remove.assert_called_once_with(force=True, v=True) + + # Verify cidfile cleanup was attempted + mock_unlink.assert_called_once_with(missing_ok=True) diff --git a/tests/plugins/test_plugin_base.py b/tests/plugins/test_plugin_base.py index ae0df926537..c47144a5b57 100644 --- a/tests/plugins/test_plugin_base.py +++ b/tests/plugins/test_plugin_base.py @@ -1,6 +1,7 @@ """Test base plugin functionality.""" import asyncio +from pathlib import Path from unittest.mock import ANY, MagicMock, Mock, PropertyMock, patch from awesomeversion import AwesomeVersion @@ -165,6 +166,8 @@ async def test_plugin_watchdog_max_failed_attempts( error: PluginError, container: MagicMock, caplog: pytest.LogCaptureFixture, + tmp_supervisor_data: Path, + path_extern, ) -> None: """Test plugin watchdog gives up after max failed attempts.""" with patch.object(type(plugin.instance), "attach"): From 04c56282a2de81ffc11e198aa8a6a570efc30c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20=C4=8Cerm=C3=A1k?= Date: Fri, 5 Sep 2025 11:16:00 +0200 Subject: [PATCH 2/2] Address review comments, fix mounting of the cidfile --- supervisor/bootstrap.py | 6 +++--- supervisor/config.py | 11 ++++++++--- supervisor/docker/manager.py | 13 ++++++++++--- tests/conftest.py | 2 +- tests/docker/test_manager.py | 29 +++++++++++++++++------------ 5 files changed, 39 insertions(+), 22 deletions(-) diff --git a/supervisor/bootstrap.py b/supervisor/bootstrap.py index 60154259cc7..c1f46ec260f 100644 --- a/supervisor/bootstrap.py +++ b/supervisor/bootstrap.py @@ -226,9 +226,9 @@ def initialize_system(coresys: CoreSys) -> None: ) config.path_addon_configs.mkdir() - if not config.path_cidfiles.is_dir(): - _LOGGER.debug("Creating Docker cidfiles folder at '%s'", config.path_cidfiles) - config.path_cidfiles.mkdir() + if not config.path_cid_files.is_dir(): + _LOGGER.debug("Creating Docker cidfiles folder at '%s'", config.path_cid_files) + config.path_cid_files.mkdir() def warning_handler(message, category, filename, lineno, file=None, line=None): diff --git a/supervisor/config.py b/supervisor/config.py index 620a555a390..03abb91fbc4 100644 --- a/supervisor/config.py +++ b/supervisor/config.py @@ -54,7 +54,7 @@ EMERGENCY_DATA = PurePath("emergency") ADDON_CONFIGS = PurePath("addon_configs") CORE_BACKUP_DATA = PurePath("core/backup") -CIDFILES = PurePath("cidfiles") +CID_FILES = PurePath("cid_files") DEFAULT_BOOT_TIME = datetime.fromtimestamp(0, UTC).isoformat() @@ -401,9 +401,14 @@ def path_extern_media(self) -> PurePath: return PurePath(self.path_extern_supervisor, MEDIA_DATA) @property - def path_cidfiles(self) -> Path: + def path_cid_files(self) -> Path: """Return CID files folder.""" - return self.path_supervisor / CIDFILES + return self.path_supervisor / CID_FILES + + @property + def path_extern_cid_files(self) -> PurePath: + """Return CID files folder.""" + return PurePath(self.path_extern_supervisor, CID_FILES) @property def addons_repositories(self) -> list[str]: diff --git a/supervisor/docker/manager.py b/supervisor/docker/manager.py index 83e47c83469..b44fc386f1e 100644 --- a/supervisor/docker/manager.py +++ b/supervisor/docker/manager.py @@ -324,17 +324,24 @@ def run( # Setup cidfile and bind mount it cidfile_path = None if name: - cidfile_path = self.coresys.config.path_cidfiles / f"{name}.cid" + cidfile_path = self.coresys.config.path_cid_files / f"{name}.cid" # Remove the file if it exists e.g. as a leftover from unclean shutdown if cidfile_path.is_file(): with suppress(OSError): cidfile_path.unlink(missing_ok=True) + extern_cidfile_path = ( + self.coresys.config.path_extern_cid_files / f"{name}.cid" + ) + # Bind mount to /run/cid in container if "volumes" not in kwargs: kwargs["volumes"] = {} - kwargs["volumes"]["/run/cid"] = {"bind": str(cidfile_path), "mode": "ro"} + kwargs["volumes"][str(extern_cidfile_path)] = { + "bind": "/run/cid", + "mode": "ro", + } # Create container try: @@ -581,7 +588,7 @@ def stop_container( _LOGGER.info("Cleaning %s application", name) docker_container.remove(force=True, v=True) - cidfile_path = self.coresys.config.path_cidfiles / f"{name}.cid" + cidfile_path = self.coresys.config.path_cid_files / f"{name}.cid" with suppress(OSError): cidfile_path.unlink(missing_ok=True) diff --git a/tests/conftest.py b/tests/conftest.py index 1e3807c0bc0..0bab5ea4e7c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -488,7 +488,7 @@ async def tmp_supervisor_data(coresys: CoreSys, tmp_path: Path) -> Path: coresys.config.path_addon_configs.mkdir(parents=True) coresys.config.path_ssl.mkdir() coresys.config.path_core_backup.mkdir(parents=True) - coresys.config.path_cidfiles.mkdir() + coresys.config.path_cid_files.mkdir() yield tmp_path diff --git a/tests/docker/test_manager.py b/tests/docker/test_manager.py index 59e99a37f9f..70c4371d217 100644 --- a/tests/docker/test_manager.py +++ b/tests/docker/test_manager.py @@ -7,7 +7,7 @@ import pytest from requests import RequestException -from supervisor.config import CIDFILES +from supervisor.coresys import CoreSys from supervisor.docker.manager import CommandReturn, DockerAPI from supervisor.exceptions import DockerError @@ -138,14 +138,17 @@ async def test_run_command_custom_stdout_stderr(docker: DockerAPI): assert result.output == b"output" -async def test_run_container_with_cidfile(docker: DockerAPI, tmp_supervisor_data): +async def test_run_container_with_cidfile( + coresys: CoreSys, docker: DockerAPI, path_extern, tmp_supervisor_data +): """Test container creation with cidfile and bind mount.""" # Mock container mock_container = MagicMock() mock_container.id = "test_container_id_12345" container_name = "test_container" - cidfile_path = tmp_supervisor_data / CIDFILES / f"{container_name}.cid" + cidfile_path = coresys.config.path_cid_files / f"{container_name}.cid" + extern_cidfile_path = coresys.config.path_extern_cid_files / f"{container_name}.cid" docker.docker.containers.run.return_value = mock_container @@ -166,9 +169,9 @@ async def test_run_container_with_cidfile(docker: DockerAPI, tmp_supervisor_data kwargs = create_mock.call_args[1] assert "volumes" in kwargs - assert "/run/cid" in kwargs["volumes"] - assert kwargs["volumes"]["/run/cid"]["bind"] == str(cidfile_path) - assert kwargs["volumes"]["/run/cid"]["mode"] == "ro" + assert str(extern_cidfile_path) in kwargs["volumes"] + assert kwargs["volumes"][str(extern_cidfile_path)]["bind"] == "/run/cid" + assert kwargs["volumes"][str(extern_cidfile_path)]["mode"] == "ro" # Verify container start was called mock_container.start.assert_called_once() @@ -181,7 +184,7 @@ async def test_run_container_with_cidfile(docker: DockerAPI, tmp_supervisor_data async def test_run_container_with_leftover_cidfile( - docker: DockerAPI, tmp_supervisor_data + coresys: CoreSys, docker: DockerAPI, path_extern, tmp_supervisor_data ): """Test container creation removes leftover cidfile before creating new one.""" # Mock container @@ -189,7 +192,7 @@ async def test_run_container_with_leftover_cidfile( mock_container.id = "test_container_id_new" container_name = "test_container" - cidfile_path = tmp_supervisor_data / "cidfiles" / f"{container_name}.cid" + cidfile_path = coresys.config.path_cid_files / f"{container_name}.cid" # Create a leftover cidfile cidfile_path.touch() @@ -217,7 +220,7 @@ async def test_run_container_with_leftover_cidfile( async def test_stop_container_with_cidfile_cleanup( - docker: DockerAPI, tmp_supervisor_data + coresys: CoreSys, docker: DockerAPI, path_extern, tmp_supervisor_data ): """Test container stop with cidfile cleanup.""" # Mock container @@ -225,7 +228,7 @@ async def test_stop_container_with_cidfile_cleanup( mock_container.status = "running" container_name = "test_container" - cidfile_path = tmp_supervisor_data / "cidfiles" / f"{container_name}.cid" + cidfile_path = coresys.config.path_cid_files / f"{container_name}.cid" # Create a cidfile cidfile_path.touch() @@ -273,14 +276,16 @@ async def test_stop_container_without_removal_no_cidfile_cleanup(docker: DockerA mock_unlink.assert_not_called() -async def test_cidfile_cleanup_handles_oserror(docker: DockerAPI, tmp_supervisor_data): +async def test_cidfile_cleanup_handles_oserror( + coresys: CoreSys, docker: DockerAPI, path_extern, tmp_supervisor_data +): """Test that cidfile cleanup handles OSError gracefully.""" # Mock container mock_container = MagicMock() mock_container.status = "running" container_name = "test_container" - cidfile_path = tmp_supervisor_data / "cidfiles" / f"{container_name}.cid" + cidfile_path = coresys.config.path_cid_files / f"{container_name}.cid" # Create a cidfile cidfile_path.touch()