From 843a6dd741e01a322d9c126a5520ad5038dfad9f Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Wed, 11 Oct 2023 14:36:08 -0400 Subject: [PATCH 1/3] Retry run if image missing and handle fixup --- supervisor/docker/addon.py | 11 +-- supervisor/docker/audio.py | 13 +--- supervisor/docker/cli.py | 13 +--- supervisor/docker/dns.py | 13 +--- supervisor/docker/homeassistant.py | 13 +--- supervisor/docker/interface.py | 25 +++++++ supervisor/docker/multicast.py | 13 +--- supervisor/docker/observer.py | 13 +--- .../resolution/fixups/addon_execute_repair.py | 57 +++++++++++++++ tests/docker/test_interface.py | 21 ++++++ .../fixup/test_addon_execute_repair.py | 73 +++++++++++++++++++ 11 files changed, 183 insertions(+), 82 deletions(-) create mode 100644 supervisor/resolution/fixups/addon_execute_repair.py create mode 100644 tests/resolution/fixup/test_addon_execute_repair.py diff --git a/supervisor/docker/addon.py b/supervisor/docker/addon.py index 05da47c44cc..009fa549b84 100644 --- a/supervisor/docker/addon.py +++ b/supervisor/docker/addon.py @@ -501,24 +501,16 @@ def mounts(self) -> list[Mount]: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - # Security check if not self.addon.protected: _LOGGER.warning("%s running with disabled protected mode!", self.addon.name) - # Cleanup - await self.stop() - # Don't set a hostname if no separate UTS namespace is used hostname = None if self.uts_mode else self.addon.hostname # Create & Run container try: - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( tag=str(self.addon.version), name=self.name, hostname=hostname, @@ -549,7 +541,6 @@ async def run(self) -> None: ) raise - self._meta = docker_container.attrs _LOGGER.info( "Starting Docker add-on %s with version %s", self.image, self.version ) diff --git a/supervisor/docker/audio.py b/supervisor/docker/audio.py index eeb34249d39..003f8f6b0bb 100644 --- a/supervisor/docker/audio.py +++ b/supervisor/docker/audio.py @@ -92,16 +92,7 @@ def cpu_rt_runtime(self) -> int | None: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - - # Cleanup - await self.stop() - - # Create & Run container - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( tag=str(self.sys_plugins.audio.version), init=False, ipv4=self.sys_docker.network.audio, @@ -118,8 +109,6 @@ async def run(self) -> None: }, mounts=self.mounts, ) - - self._meta = docker_container.attrs _LOGGER.info( "Starting Audio %s with version %s - %s", self.image, diff --git a/supervisor/docker/cli.py b/supervisor/docker/cli.py index 7f0fe9a7a36..ca0eec0865d 100644 --- a/supervisor/docker/cli.py +++ b/supervisor/docker/cli.py @@ -33,16 +33,7 @@ def name(self) -> str: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - - # Cleanup - await self.stop() - - # Create & Run container - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( entrypoint=["/init"], tag=str(self.sys_plugins.cli.version), init=False, @@ -60,8 +51,6 @@ async def run(self) -> None: ENV_TOKEN: self.sys_plugins.cli.supervisor_token, }, ) - - self._meta = docker_container.attrs _LOGGER.info( "Starting CLI %s with version %s - %s", self.image, diff --git a/supervisor/docker/dns.py b/supervisor/docker/dns.py index 56fd07082a4..b357951ca03 100644 --- a/supervisor/docker/dns.py +++ b/supervisor/docker/dns.py @@ -35,16 +35,7 @@ def name(self) -> str: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - - # Cleanup - await self.stop() - - # Create & Run container - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( tag=str(self.sys_plugins.dns.version), init=False, dns=False, @@ -65,8 +56,6 @@ async def run(self) -> None: ], oom_score_adj=-300, ) - - self._meta = docker_container.attrs _LOGGER.info( "Starting DNS %s with version %s - %s", self.image, diff --git a/supervisor/docker/homeassistant.py b/supervisor/docker/homeassistant.py index c96f6c7600b..ea5507ce592 100644 --- a/supervisor/docker/homeassistant.py +++ b/supervisor/docker/homeassistant.py @@ -152,16 +152,7 @@ def mounts(self) -> list[Mount]: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - - # Cleanup - await self.stop() - - # Create & Run container - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( tag=(self.sys_homeassistant.version), name=self.name, hostname=self.name, @@ -186,8 +177,6 @@ async def run(self) -> None: tmpfs={"/tmp": ""}, oom_score_adj=-300, ) - - self._meta = docker_container.attrs _LOGGER.info( "Starting Home Assistant %s with version %s", self.image, self.version ) diff --git a/supervisor/docker/interface.py b/supervisor/docker/interface.py index af63b6d3580..d385a233f65 100644 --- a/supervisor/docker/interface.py +++ b/supervisor/docker/interface.py @@ -377,6 +377,31 @@ async def run(self) -> None: """Run Docker image.""" raise NotImplementedError() + async def _run(self, **kwargs) -> None: + """Run Docker image with retry inf necessary.""" + if await self.is_running(): + return + + # Cleanup + await self.stop() + + # Create & Run container + try: + docker_container = await self.sys_run_in_executor( + self.sys_docker.run, self.image, **kwargs + ) + except DockerNotFound as err: + # If image is missing, capture the exception as this shouldn't happen + # Try to keep things working for user by pulling image and retrying once + capture_exception(err) + await self.install(self.version) + docker_container = await self.sys_run_in_executor( + self.sys_docker.run, self.image, **kwargs + ) + + # Store metadata + self._meta = docker_container.attrs + @Job( name="docker_interface_stop", limit=JobExecutionLimit.GROUP_ONCE, diff --git a/supervisor/docker/multicast.py b/supervisor/docker/multicast.py index 4c8947f885e..1439cd40c28 100644 --- a/supervisor/docker/multicast.py +++ b/supervisor/docker/multicast.py @@ -38,16 +38,7 @@ def capabilities(self) -> list[Capabilities]: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - - # Cleanup - await self.stop() - - # Create & Run container - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( tag=str(self.sys_plugins.multicast.version), init=False, name=self.name, @@ -59,8 +50,6 @@ async def run(self) -> None: extra_hosts={"supervisor": self.sys_docker.network.supervisor}, environment={ENV_TIME: self.sys_timezone}, ) - - self._meta = docker_container.attrs _LOGGER.info( "Starting Multicast %s with version %s - Host", self.image, self.version ) diff --git a/supervisor/docker/observer.py b/supervisor/docker/observer.py index 1f0eeaa3cd8..67ec4d02959 100644 --- a/supervisor/docker/observer.py +++ b/supervisor/docker/observer.py @@ -35,16 +35,7 @@ def name(self) -> str: ) async def run(self) -> None: """Run Docker image.""" - if await self.is_running(): - return - - # Cleanup - await self.stop() - - # Create & Run container - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, - self.image, + await self._run( tag=str(self.sys_plugins.observer.version), init=False, ipv4=self.sys_docker.network.observer, @@ -63,8 +54,6 @@ async def run(self) -> None: ports={"80/tcp": 4357}, oom_score_adj=-300, ) - - self._meta = docker_container.attrs _LOGGER.info( "Starting Observer %s with version %s - %s", self.image, diff --git a/supervisor/resolution/fixups/addon_execute_repair.py b/supervisor/resolution/fixups/addon_execute_repair.py new file mode 100644 index 00000000000..e497a98831c --- /dev/null +++ b/supervisor/resolution/fixups/addon_execute_repair.py @@ -0,0 +1,57 @@ +"""Helper to fix missing image for addon.""" + +import logging + +from ...coresys import CoreSys +from ..const import ContextType, IssueType, SuggestionType +from .base import FixupBase + +_LOGGER: logging.Logger = logging.getLogger(__name__) + + +def setup(coresys: CoreSys) -> FixupBase: + """Check setup function.""" + return FixupAddonExecuteRepair(coresys) + + +class FixupAddonExecuteRepair(FixupBase): + """Storage class for fixup.""" + + async def process_fixup(self, reference: str | None = None) -> None: + """Pull the addons image.""" + addon = self.sys_addons.get(reference, local_only=True) + if not addon: + _LOGGER.info( + "Cannot repair addon %s as it is not installed, dismissing suggestion", + reference, + ) + return + + if await addon.instance.exists(): + _LOGGER.info( + "Addon %s does not need repair, dismissing suggestion", reference + ) + return + + _LOGGER.info("Installing image for addon %s") + await addon.instance.install(addon.version) + + @property + def suggestion(self) -> SuggestionType: + """Return a SuggestionType enum.""" + return SuggestionType.EXECUTE_REPAIR + + @property + def context(self) -> ContextType: + """Return a ContextType enum.""" + return ContextType.ADDON + + @property + def issues(self) -> list[IssueType]: + """Return a IssueType enum list.""" + return [IssueType.MISSING_IMAGE] + + @property + def auto(self) -> bool: + """Return if a fixup can be apply as auto fix.""" + return True diff --git a/tests/docker/test_interface.py b/tests/docker/test_interface.py index 4132564af26..6fc649f0f4a 100644 --- a/tests/docker/test_interface.py +++ b/tests/docker/test_interface.py @@ -10,6 +10,7 @@ import pytest from requests import RequestException +from supervisor.addons import Addon from supervisor.const import BusEvent, CpuArch from supervisor.coresys import CoreSys from supervisor.docker.const import ContainerState @@ -223,3 +224,23 @@ async def test_image_pull_fail( ) capture_exception.assert_called_once_with(err) + + +async def test_run_missing_image( + coresys: CoreSys, + install_addon_ssh: Addon, + container: MagicMock, + capture_exception: Mock, + path_extern, +): + """Test run retries after a pull when the image is missing.""" + coresys.docker.containers.create.side_effect = [NotFound("missing"), MagicMock()] + container.status = "stopped" + install_addon_ssh.data["image"] = "test_image" + + with patch.object(DockerInterface, "install") as install: + await install_addon_ssh.instance.run() + install.assert_called_once_with(AwesomeVersion("9.2.1"), None, False, None) + + coresys.docker.containers.create.call_count == 2 + capture_exception.assert_called_once() diff --git a/tests/resolution/fixup/test_addon_execute_repair.py b/tests/resolution/fixup/test_addon_execute_repair.py new file mode 100644 index 00000000000..ea3ae5a0b5f --- /dev/null +++ b/tests/resolution/fixup/test_addon_execute_repair.py @@ -0,0 +1,73 @@ +"""Test fixup core execute repair.""" + +from unittest.mock import MagicMock, patch + +from docker.errors import NotFound + +from supervisor.addons.addon import Addon +from supervisor.coresys import CoreSys +from supervisor.docker.addon import DockerAddon +from supervisor.docker.interface import DockerInterface +from supervisor.docker.manager import DockerAPI +from supervisor.resolution.const import ContextType, IssueType, SuggestionType +from supervisor.resolution.fixups.addon_execute_repair import FixupAddonExecuteRepair + + +async def test_fixup(docker: DockerAPI, coresys: CoreSys, install_addon_ssh: Addon): + """Test fixup rebuilds addon's container.""" + docker.images.get.side_effect = NotFound("missing") + install_addon_ssh.data["image"] = "test_image" + + addon_execute_repair = FixupAddonExecuteRepair(coresys) + assert addon_execute_repair.auto is True + + coresys.resolution.create_issue( + IssueType.MISSING_IMAGE, + ContextType.ADDON, + reference="local_ssh", + suggestions=[SuggestionType.EXECUTE_REPAIR], + ) + with patch.object(DockerInterface, "install") as install: + await addon_execute_repair() + install.assert_called_once() + + assert not coresys.resolution.issues + assert not coresys.resolution.suggestions + + +async def test_fixup_no_addon(coresys: CoreSys): + """Test fixup dismisses if addon is missing.""" + addon_execute_repair = FixupAddonExecuteRepair(coresys) + assert addon_execute_repair.auto is True + + coresys.resolution.create_issue( + IssueType.MISSING_IMAGE, + ContextType.ADDON, + reference="local_ssh", + suggestions=[SuggestionType.EXECUTE_REPAIR], + ) + + with patch.object(DockerAddon, "install") as install: + await addon_execute_repair() + install.assert_not_called() + + +async def test_fixup_image_exists( + docker: DockerAPI, coresys: CoreSys, install_addon_ssh: Addon +): + """Test fixup dismisses if image exists.""" + docker.images.get.return_value = MagicMock() + + addon_execute_repair = FixupAddonExecuteRepair(coresys) + assert addon_execute_repair.auto is True + + coresys.resolution.create_issue( + IssueType.MISSING_IMAGE, + ContextType.ADDON, + reference="local_ssh", + suggestions=[SuggestionType.EXECUTE_REPAIR], + ) + + with patch.object(DockerAddon, "install") as install: + await addon_execute_repair() + install.assert_not_called() From f2ec21c3b9bea7b2287b19df86b9c6c2ec7ae99b Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Thu, 12 Oct 2023 11:07:46 -0400 Subject: [PATCH 2/3] Fix lint and run error test --- tests/docker/test_addon.py | 4 +--- tests/docker/test_interface.py | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/docker/test_addon.py b/tests/docker/test_addon.py index bd284f49db8..62ac7393bf9 100644 --- a/tests/docker/test_addon.py +++ b/tests/docker/test_addon.py @@ -184,7 +184,6 @@ def test_not_journald_addon( async def test_addon_run_docker_error( coresys: CoreSys, addonsdata_system: dict[str, Data], - capture_exception: Mock, os_environ, ): """Test docker error when addon is run.""" @@ -196,14 +195,13 @@ async def test_addon_run_docker_error( with patch.object(DockerAddon, "stop"), patch.object( AddonOptions, "validate", new=PropertyMock(return_value=lambda _: None) - ), pytest.raises(DockerNotFound): + ), patch.object(DockerAddon, "install"), pytest.raises(DockerNotFound): await docker_addon.run() assert ( Issue(IssueType.MISSING_IMAGE, ContextType.ADDON, reference="test_addon") in coresys.resolution.issues ) - capture_exception.assert_not_called() async def test_addon_run_add_host_error( diff --git a/tests/docker/test_interface.py b/tests/docker/test_interface.py index 6fc649f0f4a..13b256ff542 100644 --- a/tests/docker/test_interface.py +++ b/tests/docker/test_interface.py @@ -242,5 +242,5 @@ async def test_run_missing_image( await install_addon_ssh.instance.run() install.assert_called_once_with(AwesomeVersion("9.2.1"), None, False, None) - coresys.docker.containers.create.call_count == 2 + assert coresys.docker.containers.create.call_count == 2 capture_exception.assert_called_once() From a66e207377155abeee0f9f299c2e19c485211545 Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Mon, 16 Oct 2023 16:20:36 -0400 Subject: [PATCH 3/3] Remove retry and just capture exception --- supervisor/docker/interface.py | 6 +----- tests/docker/test_addon.py | 23 +++++++---------------- tests/docker/test_interface.py | 13 ++++++++----- 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/supervisor/docker/interface.py b/supervisor/docker/interface.py index d385a233f65..aba8bb77384 100644 --- a/supervisor/docker/interface.py +++ b/supervisor/docker/interface.py @@ -392,12 +392,8 @@ async def _run(self, **kwargs) -> None: ) except DockerNotFound as err: # If image is missing, capture the exception as this shouldn't happen - # Try to keep things working for user by pulling image and retrying once capture_exception(err) - await self.install(self.version) - docker_container = await self.sys_run_in_executor( - self.sys_docker.run, self.image, **kwargs - ) + raise # Store metadata self._meta = docker_container.attrs diff --git a/tests/docker/test_addon.py b/tests/docker/test_addon.py index 62ac7393bf9..652d2e9f1f7 100644 --- a/tests/docker/test_addon.py +++ b/tests/docker/test_addon.py @@ -39,13 +39,6 @@ def fixture_addonsdata_user() -> dict[str, Data]: yield mock -@pytest.fixture(name="os_environ") -def fixture_os_environ(): - """Mock os.environ.""" - with patch("supervisor.config.os.environ") as mock: - yield mock - - def get_docker_addon( coresys: CoreSys, addonsdata_system: dict[str, Data], config_file: str ): @@ -60,7 +53,7 @@ def get_docker_addon( def test_base_volumes_included( - coresys: CoreSys, addonsdata_system: dict[str, Data], os_environ + coresys: CoreSys, addonsdata_system: dict[str, Data], path_extern ): """Dev and data volumes always included.""" docker_addon = get_docker_addon( @@ -86,7 +79,7 @@ def test_base_volumes_included( def test_addon_map_folder_defaults( - coresys: CoreSys, addonsdata_system: dict[str, Data], os_environ + coresys: CoreSys, addonsdata_system: dict[str, Data], path_extern ): """Validate defaults for mapped folders in addons.""" docker_addon = get_docker_addon( @@ -143,7 +136,7 @@ def test_addon_map_folder_defaults( def test_journald_addon( - coresys: CoreSys, addonsdata_system: dict[str, Data], os_environ + coresys: CoreSys, addonsdata_system: dict[str, Data], path_extern ): """Validate volume for journald option.""" docker_addon = get_docker_addon( @@ -171,7 +164,7 @@ def test_journald_addon( def test_not_journald_addon( - coresys: CoreSys, addonsdata_system: dict[str, Data], os_environ + coresys: CoreSys, addonsdata_system: dict[str, Data], path_extern ): """Validate journald option defaults off.""" docker_addon = get_docker_addon( @@ -182,9 +175,7 @@ def test_not_journald_addon( async def test_addon_run_docker_error( - coresys: CoreSys, - addonsdata_system: dict[str, Data], - os_environ, + coresys: CoreSys, addonsdata_system: dict[str, Data], path_extern ): """Test docker error when addon is run.""" await coresys.dbus.timedate.connect(coresys.dbus.bus) @@ -195,7 +186,7 @@ async def test_addon_run_docker_error( with patch.object(DockerAddon, "stop"), patch.object( AddonOptions, "validate", new=PropertyMock(return_value=lambda _: None) - ), patch.object(DockerAddon, "install"), pytest.raises(DockerNotFound): + ), pytest.raises(DockerNotFound): await docker_addon.run() assert ( @@ -208,7 +199,7 @@ async def test_addon_run_add_host_error( coresys: CoreSys, addonsdata_system: dict[str, Data], capture_exception: Mock, - os_environ, + path_extern, ): """Test error adding host when addon is run.""" await coresys.dbus.timedate.connect(coresys.dbus.bus) diff --git a/tests/docker/test_interface.py b/tests/docker/test_interface.py index 13b256ff542..a9222de5457 100644 --- a/tests/docker/test_interface.py +++ b/tests/docker/test_interface.py @@ -16,7 +16,12 @@ from supervisor.docker.const import ContainerState from supervisor.docker.interface import DockerInterface from supervisor.docker.monitor import DockerContainerStateEvent -from supervisor.exceptions import DockerAPIError, DockerError, DockerRequestError +from supervisor.exceptions import ( + DockerAPIError, + DockerError, + DockerNotFound, + DockerRequestError, +) @pytest.fixture(autouse=True) @@ -233,14 +238,12 @@ async def test_run_missing_image( capture_exception: Mock, path_extern, ): - """Test run retries after a pull when the image is missing.""" + """Test run captures the exception when image is missing.""" coresys.docker.containers.create.side_effect = [NotFound("missing"), MagicMock()] container.status = "stopped" install_addon_ssh.data["image"] = "test_image" - with patch.object(DockerInterface, "install") as install: + with pytest.raises(DockerNotFound): await install_addon_ssh.instance.run() - install.assert_called_once_with(AwesomeVersion("9.2.1"), None, False, None) - assert coresys.docker.containers.create.call_count == 2 capture_exception.assert_called_once()