Merge 17104e1 into 7aa4e5c

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()
 
@@ -478,7 +489,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)
@@ -515,35 +526,25 @@ 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)
-        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, _event: ConfigChangedEvent) -> None:
         """Handle the configuration change.

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=30)

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()

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):