diff --git a/src/charm.py b/src/charm.py index e595d195a..16d3b9e3e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -356,6 +356,30 @@ def _block_on_openstack_config(self, state: CharmState) -> bool: return True return False + def _common_install_code(self, state: CharmState) -> None: + """Installation code shared between install and upgrade hook. + + 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: + logger.error("Failed to setup logrotate") + raise + + self._refresh_firewall(state) + @catch_charm_errors def _on_install(self, _event: InstallEvent) -> None: """Handle the installation of charm. @@ -395,25 +419,10 @@ 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) - return - - self._refresh_firewall(state) - runner_manager = self._get_runner_manager(state) + self._common_install_code(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() @@ -478,7 +487,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("Updating kernel (if available)") self._apt_install(["linux-generic"]) _, exit_code = execute_command(["ls", "/var/run/reboot-required"], check_exit=False) @@ -514,29 +523,16 @@ 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 self._block_on_openstack_config(state): return - state = self._setup_state() - self._refresh_firewall(state) - logger.info("Flushing the runners...") + logger.info("Reinstalling dependencies...") + self._common_install_code(state) + runner_manager = self._get_runner_manager(state) - if not runner_manager: - return + 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, @@ -544,6 +540,11 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: state.runner_config.virtual_machine_resources, ) + # 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 def _on_config_changed(self, _event: ConfigChangedEvent) -> None: """Handle the configuration change. diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index e7ca21c02..6f6b79f45 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -13,6 +13,8 @@ install_repo_policy_compliance_from_git_source, reconcile, remove_runner_bin, + run_in_unit, + wait_for, wait_till_num_of_runners, ) from tests.status_name import ACTIVE @@ -191,3 +193,28 @@ 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: The upgrade_charm hook ran successfully. + """ + await app_no_runner.refresh(path=charm_file) + + 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.""" + 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) 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..92154bd3a 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -101,18 +101,37 @@ 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): @@ -169,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):