Merge 9ca0169 into 35b79da

src/charm.py

@@ -404,6 +404,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, _: InstallEvent) -> None:
         """Handle the installation of charm."""
@@ -439,25 +463,10 @@ def _on_install(self, _: 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()
 
@@ -518,7 +527,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)
@@ -550,36 +559,28 @@ def _on_upgrade_charm(self, _: UpgradeCharmEvent) -> None:
         """Handle the update of charm."""
         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,
             state.runner_config.virtual_machines,
             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, _: ConfigChangedEvent) -> None:
         """Handle the configuration change."""

tests/integration/helpers.py

@@ -511,7 +511,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})"
@@ -527,10 +527,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:
@@ -555,7 +556,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.
@@ -564,5 +565,5 @@ async def wait_for(
             return result
     else:
         if result := func():
-            return result
+            return cast(R, result)
     raise TimeoutError()

tests/integration/test_charm_no_runner.py

@@ -14,6 +14,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
@@ -192,3 +194,32 @@ 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() -> 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"
+        )
+        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)

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
 
 
@@ -44,7 +44,7 @@ def disk_usage_mock(total_disk: int):
 
 @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()

tests/unit/test_charm.py

@@ -174,18 +174,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):
@@ -242,28 +261,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):