From 190ce2cc12e1d0e5cf96bdf4a120a579bc293830 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 10:13:31 +0200 Subject: [PATCH 01/20] Reword update kernel message --- src/charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index e595d195a..2bcad51cd 100755 --- a/src/charm.py +++ b/src/charm.py @@ -478,7 +478,7 @@ def _update_kernel(self, now: bool = False) -> None: Args: now: Whether the reboot should trigger at end of event handler or now. """ - logger.info("Upgrading kernel") + logger.info("Installing a newer kernel (if available)") self._apt_install(["linux-generic"]) _, exit_code = execute_command(["ls", "/var/run/reboot-required"], check_exit=False) From f71c471c91497ca436cbd1578d41cfeaf7545f80 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 11:29:16 +0200 Subject: [PATCH 02/20] Add schedule_build_runner_image to upgrade_charm --- src/charm.py | 2 ++ tests/unit/conftest.py | 4 ++-- tests/unit/test_charm.py | 27 ++++++++++++++++++++++----- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/charm.py b/src/charm.py index 2bcad51cd..cef6dddad 100755 --- a/src/charm.py +++ b/src/charm.py @@ -537,6 +537,8 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: if not runner_manager: return + runner_manager.schedule_build_runner_image() + runner_manager.flush(FlushMode.FLUSH_BUSY_WAIT_REPO_CHECK) self._reconcile_runners( runner_manager, diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index fb5a97efd..aa8d5c055 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -8,7 +8,7 @@ import pytest -import openstack_cloud +from openstack_cloud import openstack_manager from tests.unit.mock import MockGhapiClient, MockLxdClient, MockRepoPolicyComplianceClient @@ -36,7 +36,7 @@ def disk_usage_mock(total_disk): @pytest.fixture(autouse=True) def mocks(monkeypatch, tmp_path, exec_command, lxd_exec_command, runner_binary_path): - openstack_manager_mock = unittest.mock.MagicMock(spec=openstack_cloud) + openstack_manager_mock = unittest.mock.MagicMock(spec=openstack_manager) cron_path = tmp_path / "cron.d" cron_path.mkdir() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index bde467313..dfd6dc59a 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -28,7 +28,6 @@ from firewall import FirewallEntry from github_type import GitHubRunnerStatus from runner_manager import RunnerInfo, RunnerManagerConfig - TEST_PROXY_SERVER_URL = "http://proxy.server:1234" @@ -101,18 +100,36 @@ def test_proxy_setting(harness: Harness): assert state.proxy_config.no_proxy == "127.0.0.1,localhost" -def test_install(harness: Harness, exec_command: MagicMock): +@pytest.mark.parametrize( + "hook", + [ + pytest.param( + "install", id="Install" + ), + pytest.param( + "upgrade_charm", id="Upgrade" + ), + ], +) +def test_common_install_code(hook: str, harness: Harness, exec_command: MagicMock, monkeypatch: pytest.MonkeyPatch): """ arrange: Set up charm. - act: Fire install event. - assert: Some install commands are run on the mock. + act: Fire install/upgrade event. + assert: Common install commands are run on the mock. """ - harness.charm.on.install.emit() + + monkeypatch.setattr("charm.metrics.setup_logrotate", setup_logrotate := MagicMock()) + monkeypatch.setattr("runner_manager.RunnerManager.schedule_build_runner_image", schedule_build_runner_image:=MagicMock()) + getattr(harness.charm.on, hook).emit() calls = [ call(["/usr/bin/snap", "install", "lxd", "--channel=latest/stable"]), call(["/snap/bin/lxd", "init", "--auto"]), + call(["/usr/bin/systemctl", "enable", "repo-policy-compliance"]) ] + exec_command.assert_has_calls(calls, any_order=True) + setup_logrotate.assert_called_once() + schedule_build_runner_image.assert_called_once() def test_on_config_changed_failure(harness: Harness): From 1852db317073f9bdffa5a2bbe656df62eba82651 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 11:41:13 +0200 Subject: [PATCH 03/20] Add _set_reconcile_timer call to upgrade_charm --- src/charm.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/charm.py b/src/charm.py index cef6dddad..1bfbe8b56 100755 --- a/src/charm.py +++ b/src/charm.py @@ -546,6 +546,11 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: state.runner_config.virtual_machine_resources, ) + # Ensure an update of the systemd timer for reconciliation event is written to disk. + # upgrade_charm event is not followed by a config changed event (which updates the timer) + # so the timer would otherwise not be updated. + self._set_reconcile_timer() + @catch_charm_errors def _on_config_changed(self, _event: ConfigChangedEvent) -> None: """Handle the configuration change. From e8399b30219fe1616e5e371ae69bce57d720e1a1 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 12:14:55 +0200 Subject: [PATCH 04/20] Remove download binary in install hook --- src/charm.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/charm.py b/src/charm.py index 1bfbe8b56..fb73b98e4 100755 --- a/src/charm.py +++ b/src/charm.py @@ -417,22 +417,6 @@ def _on_install(self, _event: InstallEvent) -> None: runner_manager.build_runner_image() runner_manager.schedule_build_runner_image() - self.unit.status = MaintenanceStatus("Downloading runner binary") - try: - runner_info = runner_manager.get_latest_runner_bin_url() - logger.info( - "Downloading %s from: %s", runner_info["filename"], runner_info["download_url"] - ) - self._stored.runner_bin_url = runner_info["download_url"] - runner_manager.update_runner_bin(runner_info) - # Safe guard against transient unexpected error. - except RunnerBinaryError as err: - logger.exception("Failed to update runner binary") - # Failure to download runner binary is a transient error. - # The charm automatically update runner binary on a schedule. - self.unit.status = MaintenanceStatus(f"Failed to update runner binary: {err}") - return - self.unit.status = ActiveStatus() @catch_charm_errors From 8fe96925a9ddaf287d88be206047d20a6afbafe8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 12:37:41 +0200 Subject: [PATCH 05/20] Remove outdated code --- src/charm.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/charm.py b/src/charm.py index fb73b98e4..6a329c1b0 100755 --- a/src/charm.py +++ b/src/charm.py @@ -518,8 +518,6 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: self._refresh_firewall(state) logger.info("Flushing the runners...") runner_manager = self._get_runner_manager(state) - if not runner_manager: - return runner_manager.schedule_build_runner_image() From ec2b54c01469ba3c5e5e72f65b10a267fc26ee95 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 12:45:10 +0200 Subject: [PATCH 06/20] Reword upgrading kernel msg --- src/charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index 6a329c1b0..471fbe643 100755 --- a/src/charm.py +++ b/src/charm.py @@ -462,7 +462,7 @@ def _update_kernel(self, now: bool = False) -> None: Args: now: Whether the reboot should trigger at end of event handler or now. """ - logger.info("Installing a newer kernel (if available)") + logger.info("Upgrading kernel (if available)") self._apt_install(["linux-generic"]) _, exit_code = execute_command(["ls", "/var/run/reboot-required"], check_exit=False) From c7a81dfe196a247ffd120adbda377e0b0eb388cd Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 13:42:23 +0200 Subject: [PATCH 07/20] add integration test --- tests/integration/test_charm_no_runner.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index e7ca21c02..a266b4f54 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -191,3 +191,16 @@ async def test_reconcile_runners(model: Model, app_no_runner: Application) -> No await reconcile(app=app, model=model) await wait_till_num_of_runners(unit, 0) + + +@pytest.mark.asyncio +@pytest.mark.abort_on_fail +async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_file: str) -> None: + """ + arrange: A working application with no runners. + act: Upgrade the charm. + assert: Upgrade should not fail. + """ + await app_no_runner.refresh(path=charm_file) + + await model.wait_for_idle(status=ACTIVE, idle_period=60) From 75195f67cb617d1919d129be7f196a826ce04daa Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 13:42:50 +0200 Subject: [PATCH 08/20] extract common code into method --- src/charm.py | 63 +++++++++++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 33 deletions(-) diff --git a/src/charm.py b/src/charm.py index 471fbe643..73f971688 100755 --- a/src/charm.py +++ b/src/charm.py @@ -356,6 +356,31 @@ def _block_on_openstack_config(self, state: CharmState) -> bool: return True return False + def _common_install_code(self, state: CharmState) -> bool: + """Installation code shared between install and upgrade hook + + Returns: + Whether the installation code was successful. + """ + self.unit.status = MaintenanceStatus("Installing packages") + try: + # The `_start_services`, `_install_deps` includes retry. + self._install_deps() + self._start_services(state.charm_config.token, state.proxy_config) + metrics.setup_logrotate() + except (LogrotateSetupError, SubprocessError) as err: + logger.exception(err) + if isinstance(err, LogrotateSetupError): + msg = "Failed to setup logrotate" + else: + msg = "Failed to install dependencies" + self.unit.status = BlockedStatus(msg) + return False + + self._refresh_firewall(state) + + return True + @catch_charm_errors def _on_install(self, _event: InstallEvent) -> None: """Handle the installation of charm. @@ -395,25 +420,11 @@ def _on_install(self, _event: InstallEvent) -> None: self._block_on_openstack_config(state) return - self.unit.status = MaintenanceStatus("Installing packages") - try: - # The `_start_services`, `_install_deps` includes retry. - self._install_deps() - self._start_services(state.charm_config.token, state.proxy_config) - metrics.setup_logrotate() - except (LogrotateSetupError, SubprocessError) as err: - logger.exception(err) - if isinstance(err, LogrotateSetupError): - msg = "Failed to setup logrotate" - else: - msg = "Failed to install dependencies" - self.unit.status = BlockedStatus(msg) + if not self._common_install_code(state): return - self._refresh_firewall(state) - runner_manager = self._get_runner_manager(state) - self.unit.status = MaintenanceStatus("Building runner image") + runner_manager = self._get_runner_manager(state) runner_manager.build_runner_image() runner_manager.schedule_build_runner_image() @@ -462,7 +473,7 @@ def _update_kernel(self, now: bool = False) -> None: Args: now: Whether the reboot should trigger at end of event handler or now. """ - logger.info("Upgrading kernel (if available)") + logger.info("Updating kernel (if available)") self._apt_install(["linux-generic"]) _, exit_code = execute_command(["ls", "/var/run/reboot-required"], check_exit=False) @@ -499,28 +510,14 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: state = self._setup_state() logger.info("Reinstalling dependencies...") - try: - # The `_start_services`, `_install_deps` includes retry. - self._install_deps() - self._start_services(state.charm_config.token, state.proxy_config) - metrics.setup_logrotate() - except (LogrotateSetupError, SubprocessError) as err: - logger.exception(err) - - if isinstance(err, LogrotateSetupError): - msg = "Failed to setup logrotate" - else: - msg = "Failed to install dependencies" - self.unit.status = BlockedStatus(msg) + if not self._common_install_code(state): return - state = self._setup_state() - self._refresh_firewall(state) - logger.info("Flushing the runners...") runner_manager = self._get_runner_manager(state) runner_manager.schedule_build_runner_image() + logger.info("Flushing the runners...") runner_manager.flush(FlushMode.FLUSH_BUSY_WAIT_REPO_CHECK) self._reconcile_runners( runner_manager, From 5ea0a38d24c4b5fd32b05e43f915ad31744945f0 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 13:45:05 +0200 Subject: [PATCH 09/20] extract common code into method --- src-docs/charm.py.md | 6 +++--- src/charm.py | 3 +-- tests/unit/test_charm.py | 20 +++++++++++--------- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src-docs/charm.py.md b/src-docs/charm.py.md index ffa94edac..af3aa7e98 100644 --- a/src-docs/charm.py.md +++ b/src-docs/charm.py.md @@ -13,7 +13,7 @@ Charm for creating and managing GitHub self-hosted runner instances. --- - + ## function `catch_charm_errors` @@ -39,7 +39,7 @@ Catch common errors in charm. --- - + ## function `catch_action_errors` @@ -68,7 +68,7 @@ Catch common errors in actions. ## class `GithubRunnerCharm` Charm for managing GitHub self-hosted runners. - + ### function `__init__` diff --git a/src/charm.py b/src/charm.py index 73f971688..b1b8bbf40 100755 --- a/src/charm.py +++ b/src/charm.py @@ -51,7 +51,6 @@ LogrotateSetupError, MissingRunnerBinaryError, OpenStackUnauthorizedError, - RunnerBinaryError, RunnerError, SubprocessError, TokenError, @@ -357,7 +356,7 @@ def _block_on_openstack_config(self, state: CharmState) -> bool: return False def _common_install_code(self, state: CharmState) -> bool: - """Installation code shared between install and upgrade hook + """Installation code shared between install and upgrade hook. Returns: Whether the installation code was successful. diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index dfd6dc59a..a05a8eebc 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -28,6 +28,7 @@ from firewall import FirewallEntry from github_type import GitHubRunnerStatus from runner_manager import RunnerInfo, RunnerManagerConfig + TEST_PROXY_SERVER_URL = "http://proxy.server:1234" @@ -103,15 +104,13 @@ def test_proxy_setting(harness: Harness): @pytest.mark.parametrize( "hook", [ - pytest.param( - "install", id="Install" - ), - pytest.param( - "upgrade_charm", id="Upgrade" - ), + pytest.param("install", id="Install"), + pytest.param("upgrade_charm", id="Upgrade"), ], ) -def test_common_install_code(hook: str, harness: Harness, exec_command: MagicMock, monkeypatch: pytest.MonkeyPatch): +def test_common_install_code( + hook: str, harness: Harness, exec_command: MagicMock, monkeypatch: pytest.MonkeyPatch +): """ arrange: Set up charm. act: Fire install/upgrade event. @@ -119,12 +118,15 @@ def test_common_install_code(hook: str, harness: Harness, exec_command: MagicMoc """ monkeypatch.setattr("charm.metrics.setup_logrotate", setup_logrotate := MagicMock()) - monkeypatch.setattr("runner_manager.RunnerManager.schedule_build_runner_image", schedule_build_runner_image:=MagicMock()) + monkeypatch.setattr( + "runner_manager.RunnerManager.schedule_build_runner_image", + schedule_build_runner_image := MagicMock(), + ) getattr(harness.charm.on, hook).emit() calls = [ call(["/usr/bin/snap", "install", "lxd", "--channel=latest/stable"]), call(["/snap/bin/lxd", "init", "--auto"]), - call(["/usr/bin/systemctl", "enable", "repo-policy-compliance"]) + call(["/usr/bin/systemctl", "enable", "repo-policy-compliance"]), ] exec_command.assert_has_calls(calls, any_order=True) From 0ecb8ee2e9993d909818c0861a052282f2cef5a8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 13:46:14 +0200 Subject: [PATCH 10/20] Revert "Remove download binary in install hook" This reverts commit e8399b30219fe1616e5e371ae69bce57d720e1a1. --- src/charm.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index b1b8bbf40..8768e012e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -53,7 +53,7 @@ OpenStackUnauthorizedError, RunnerError, SubprocessError, - TokenError, + TokenError, RunnerBinaryError, ) from event_timer import EventTimer, TimerStatusError from firewall import Firewall, FirewallEntry @@ -427,6 +427,22 @@ def _on_install(self, _event: InstallEvent) -> None: runner_manager.build_runner_image() runner_manager.schedule_build_runner_image() + self.unit.status = MaintenanceStatus("Downloading runner binary") + try: + runner_info = runner_manager.get_latest_runner_bin_url() + logger.info( + "Downloading %s from: %s", runner_info["filename"], runner_info["download_url"] + ) + self._stored.runner_bin_url = runner_info["download_url"] + runner_manager.update_runner_bin(runner_info) + # Safe guard against transient unexpected error. + except RunnerBinaryError as err: + logger.exception("Failed to update runner binary") + # Failure to download runner binary is a transient error. + # The charm automatically update runner binary on a schedule. + self.unit.status = MaintenanceStatus(f"Failed to update runner binary: {err}") + return + self.unit.status = ActiveStatus() @catch_charm_errors From a236f6e79614538ffedc31c9bfaeb4ec69fd27c1 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 14:31:12 +0200 Subject: [PATCH 11/20] Adjust idle period in integration test --- tests/integration/test_charm_no_runner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index a266b4f54..1f7089586 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -203,4 +203,4 @@ async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_fil """ await app_no_runner.refresh(path=charm_file) - await model.wait_for_idle(status=ACTIVE, idle_period=60) + await model.wait_for_idle(status=ACTIVE, idle_period=30) From 4761eb189fc15c0f72fa4acb02898f7579b92b40 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 14:32:02 +0200 Subject: [PATCH 12/20] lint --- src-docs/charm.py.md | 6 +++--- src/charm.py | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src-docs/charm.py.md b/src-docs/charm.py.md index af3aa7e98..ffa94edac 100644 --- a/src-docs/charm.py.md +++ b/src-docs/charm.py.md @@ -13,7 +13,7 @@ Charm for creating and managing GitHub self-hosted runner instances. --- - + ## function `catch_charm_errors` @@ -39,7 +39,7 @@ Catch common errors in charm. --- - + ## function `catch_action_errors` @@ -68,7 +68,7 @@ Catch common errors in actions. ## class `GithubRunnerCharm` Charm for managing GitHub self-hosted runners. - + ### function `__init__` diff --git a/src/charm.py b/src/charm.py index 8768e012e..a731f44e0 100755 --- a/src/charm.py +++ b/src/charm.py @@ -51,9 +51,10 @@ LogrotateSetupError, MissingRunnerBinaryError, OpenStackUnauthorizedError, + RunnerBinaryError, RunnerError, SubprocessError, - TokenError, RunnerBinaryError, + TokenError, ) from event_timer import EventTimer, TimerStatusError from firewall import Firewall, FirewallEntry From 3a62380403acf7f91b7adced4859cf733f97d62d Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 14:33:25 +0200 Subject: [PATCH 13/20] cleanup --- src/charm.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index a731f44e0..843cfa94e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -530,7 +530,6 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: return runner_manager = self._get_runner_manager(state) - runner_manager.schedule_build_runner_image() logger.info("Flushing the runners...") From 17104e121aa04612aba2ab5bd6292fa1c2de9d95 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 14:38:55 +0200 Subject: [PATCH 14/20] Reword comment --- src/charm.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/charm.py b/src/charm.py index 843cfa94e..2acba5fcb 100755 --- a/src/charm.py +++ b/src/charm.py @@ -540,9 +540,9 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: state.runner_config.virtual_machine_resources, ) - # Ensure an update of the systemd timer for reconciliation event is written to disk. - # upgrade_charm event is not followed by a config changed event (which updates the timer) - # so the timer would otherwise not be updated. + # Ensure that an update to the systemd timer for reconciliation event is written to disk. + # The upgrade_charm event is not necessarily followed by a config changed event + # (which updates the timer), so we need to explicitly update the timer here. self._set_reconcile_timer() @catch_charm_errors From c6c0c0d839c8a4522401273840be371eae608767 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 14:44:25 +0200 Subject: [PATCH 15/20] Add _block_on_openstack_config to upgrade --- src/charm.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/charm.py b/src/charm.py index 2acba5fcb..a5bde208f 100755 --- a/src/charm.py +++ b/src/charm.py @@ -525,6 +525,9 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: """ state = self._setup_state() + if self._block_on_openstack_config(state): + return + logger.info("Reinstalling dependencies...") if not self._common_install_code(state): return From 7a7c178ffc8b88b5d18a6044ae5e49bb683dd025 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 5 Apr 2024 20:55:02 +0200 Subject: [PATCH 16/20] improve integration test --- tests/integration/test_charm_no_runner.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index 1f7089586..a5a343f45 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -13,6 +13,7 @@ install_repo_policy_compliance_from_git_source, reconcile, remove_runner_bin, + run_in_unit, wait_till_num_of_runners, ) from tests.status_name import ACTIVE @@ -199,8 +200,21 @@ async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_fil """ arrange: A working application with no runners. act: Upgrade the charm. - assert: Upgrade should not fail. + assert: The upgrade_charm hook ran successfully. """ await app_no_runner.refresh(path=charm_file) - await model.wait_for_idle(status=ACTIVE, idle_period=30) + # It may take some time between the charm upgrade and the run of the upgrade charm hook, + # so set the idle_period high. + await model.wait_for_idle(status=ACTIVE, idle_period=60) + unit = app_no_runner.units[0] + unit_name_without_slash = unit.name.replace("/", "-") + ret_code, stdout = await run_in_unit( + unit=unit, command=f"cat /var/log/juju/unit-{unit_name_without_slash}.log" + ) + assert ret_code == 0, f"Failed to read the log file: {stdout}" + assert stdout, "Log file is empty." + assert "Emitting Juju event upgrade_charm." in stdout, "upgrade_charm event not yet fired." + # There may be a race condition: if the hook is executed shortly after idle, + # it may not have finished yet, so check for idle again. + await model.wait_for_idle(status=ACTIVE) From aa2f29ad72ec72d7b19b3d35fe5b3694b0903c56 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 8 Apr 2024 10:19:41 +0200 Subject: [PATCH 17/20] Don't go into BlockedStatus on error --- src/charm.py | 31 ++++++++++++++----------------- tests/unit/test_charm.py | 29 ++++++++++++++++++----------- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/src/charm.py b/src/charm.py index a5bde208f..16d3b9e3e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -356,31 +356,30 @@ def _block_on_openstack_config(self, state: CharmState) -> bool: return True return False - def _common_install_code(self, state: CharmState) -> bool: + def _common_install_code(self, state: CharmState) -> None: """Installation code shared between install and upgrade hook. - Returns: - Whether the installation code was successful. + Raises: + LogrotateSetupError: Failed to setup logrotate. + SubprocessError: Failed to install dependencies. """ self.unit.status = MaintenanceStatus("Installing packages") try: # The `_start_services`, `_install_deps` includes retry. self._install_deps() self._start_services(state.charm_config.token, state.proxy_config) + except SubprocessError: + logger.error("Failed to install dependencies") + raise + + try: metrics.setup_logrotate() - except (LogrotateSetupError, SubprocessError) as err: - logger.exception(err) - if isinstance(err, LogrotateSetupError): - msg = "Failed to setup logrotate" - else: - msg = "Failed to install dependencies" - self.unit.status = BlockedStatus(msg) - return False + except LogrotateSetupError: + logger.error("Failed to setup logrotate") + raise self._refresh_firewall(state) - return True - @catch_charm_errors def _on_install(self, _event: InstallEvent) -> None: """Handle the installation of charm. @@ -420,8 +419,7 @@ def _on_install(self, _event: InstallEvent) -> None: self._block_on_openstack_config(state) return - if not self._common_install_code(state): - return + self._common_install_code(state) self.unit.status = MaintenanceStatus("Building runner image") runner_manager = self._get_runner_manager(state) @@ -529,8 +527,7 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: return logger.info("Reinstalling dependencies...") - if not self._common_install_code(state): - return + self._common_install_code(state) runner_manager = self._get_runner_manager(state) runner_manager.schedule_build_runner_image() diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index a05a8eebc..92154bd3a 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -188,28 +188,35 @@ def test_on_flush_runners_action_success(harness: Harness, runner_binary_path: P mock_event.set_results.assert_called() -def test_on_install_failure(monkeypatch, harness): +@pytest.mark.parametrize( + "hook", + [ + pytest.param("install", id="Install"), + pytest.param("upgrade_charm", id="Upgrade"), + ], +) +def test_on_install_failure(hook: str, harness: Harness, monkeypatch: pytest.MonkeyPatch): """ arrange: Charm with mock setup_logrotate. act: 1. Mock setup_logrotate fails. - 2. Charm in block state. - assert: - 1. Mock _install_deps raises error. - 2. Charm in block state. + 2. Mock _install_deps raises error. + assert: Charm goes into error state in both cases. """ monkeypatch.setattr( "charm.metrics.setup_logrotate", setup_logrotate := unittest.mock.MagicMock() ) - setup_logrotate.side_effect = LogrotateSetupError - harness.charm.on.install.emit() - assert harness.charm.unit.status == BlockedStatus("Failed to setup logrotate") + setup_logrotate.side_effect = LogrotateSetupError("Failed to setup logrotate") + with pytest.raises(LogrotateSetupError) as exc: + getattr(harness.charm.on, hook).emit() + assert str(exc.value) == "Failed to setup logrotate" setup_logrotate.side_effect = None - GithubRunnerCharm._install_deps = raise_subprocess_error - harness.charm.on.install.emit() - assert harness.charm.unit.status == BlockedStatus("Failed to install dependencies") + monkeypatch.setattr(GithubRunnerCharm, "_install_deps", raise_subprocess_error) + with pytest.raises(SubprocessError) as exc: + getattr(harness.charm.on, hook).emit() + assert "mock stderr" in str(exc.value) def test__refresh_firewall(monkeypatch, harness: Harness, runner_binary_path: Path): From 24c6899bcabf142252d6f74192f92fea2c179be5 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 8 Apr 2024 10:46:40 +0200 Subject: [PATCH 18/20] improve integration test --- tests/integration/test_charm_no_runner.py | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index a5a343f45..a18f502bf 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -14,7 +14,7 @@ reconcile, remove_runner_bin, run_in_unit, - wait_till_num_of_runners, + wait_till_num_of_runners, wait_for, ) from tests.status_name import ACTIVE @@ -204,17 +204,15 @@ async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_fil """ await app_no_runner.refresh(path=charm_file) - # It may take some time between the charm upgrade and the run of the upgrade charm hook, - # so set the idle_period high. - await model.wait_for_idle(status=ACTIVE, idle_period=60) unit = app_no_runner.units[0] unit_name_without_slash = unit.name.replace("/", "-") - ret_code, stdout = await run_in_unit( - unit=unit, command=f"cat /var/log/juju/unit-{unit_name_without_slash}.log" - ) - assert ret_code == 0, f"Failed to read the log file: {stdout}" - assert stdout, "Log file is empty." - assert "Emitting Juju event upgrade_charm." in stdout, "upgrade_charm event not yet fired." - # There may be a race condition: if the hook is executed shortly after idle, - # it may not have finished yet, so check for idle again. + + async def is_upgrade_charm_event_emitted(): + ret_code, stdout = await run_in_unit( + unit=unit, command=f"cat /var/log/juju/unit-{unit_name_without_slash}.log" + ) + assert ret_code == 0, f"Failed to read the log file: {stdout}" + return stdout is not None and "Emitting Juju event upgrade_charm." in stdout + + await wait_for(is_upgrade_charm_event_emitted, timeout=360, check_interval=60) await model.wait_for_idle(status=ACTIVE) From 6f0f47e11e68ef5579da93011393f9fd660a4d56 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 8 Apr 2024 12:31:48 +0200 Subject: [PATCH 19/20] lint --- tests/integration/test_charm_no_runner.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index a18f502bf..6f6b79f45 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -14,7 +14,8 @@ reconcile, remove_runner_bin, run_in_unit, - wait_till_num_of_runners, wait_for, + wait_for, + wait_till_num_of_runners, ) from tests.status_name import ACTIVE @@ -208,6 +209,7 @@ async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_fil unit_name_without_slash = unit.name.replace("/", "-") async def is_upgrade_charm_event_emitted(): + """Check if the upgrade_charm event is emitted.""" ret_code, stdout = await run_in_unit( unit=unit, command=f"cat /var/log/juju/unit-{unit_name_without_slash}.log" ) From 231cca15b9c5e4cb3b9e3534800578f4b05a8861 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 8 Apr 2024 20:54:59 +0200 Subject: [PATCH 20/20] add return type to is_upgrade_charm_event_emitted --- tests/integration/helpers.py | 9 +++++---- tests/integration/test_charm_no_runner.py | 8 ++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index aca334a72..829fa6e21 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -488,7 +488,7 @@ async def dispatch_workflow( ), "Failed to create workflow" # There is a very small chance of selecting a run not created by the dispatch above. - run = await wait_for( + run: WorkflowRun | None = await wait_for( partial(_get_latest_run, workflow=workflow, start_time=start_time, branch=branch) ) assert run, f"Run not found for workflow: {workflow.name} ({workflow.id})" @@ -504,10 +504,11 @@ async def dispatch_workflow( P = ParamSpec("P") R = TypeVar("R") +S = Callable[P, R] | Callable[P, Awaitable[R]] async def wait_for( - func: Callable[P, R], + func: S, timeout: int = 300, check_interval: int = 10, ) -> R: @@ -532,7 +533,7 @@ async def wait_for( return result else: if result := func(): - return result + return cast(R, result) time.sleep(check_interval) # final check before raising TimeoutError. @@ -541,5 +542,5 @@ async def wait_for( return result else: if result := func(): - return result + return cast(R, result) raise TimeoutError() diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index 6f6b79f45..bd9598ca1 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -208,8 +208,12 @@ async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_fil unit = app_no_runner.units[0] unit_name_without_slash = unit.name.replace("/", "-") - async def is_upgrade_charm_event_emitted(): - """Check if the upgrade_charm event is emitted.""" + async def is_upgrade_charm_event_emitted() -> bool: + """Check if the upgrade_charm event is emitted. + + Returns: + bool: True if the event is emitted, False otherwise. + """ ret_code, stdout = await run_in_unit( unit=unit, command=f"cat /var/log/juju/unit-{unit_name_without_slash}.log" )