fix: key error on charm state upgrade (#295)

* fix: key error on charm state upgrade * test: update docstring & use class method * use logger.info * lint * test: integration test * test: add charm origin for refresh * rename charm upgrade for no_runner test * override default deploy settings * inject lxd profile to local charm * inject lxd & wait for update event * juju 2.9 testable syntax (revision not supported) * 2.9 compatible download --------- Co-authored-by: Christopher Bartz <[email protected]>
canonical · Jun 12, 2024 · 82067e5 · 82067e5
1 parent 1a4e388
commit 82067e5
Show file tree

Hide file tree

Showing 8 changed files with 245 additions and 70 deletions.
diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml
@@ -15,7 +15,7 @@ jobs:
       pre-run-script: scripts/pre-integration-test.sh
       provider: lxd
       test-tox-env: integration-juju2.9
-      modules: '["test_charm_base_image", "test_charm_fork_repo", "test_charm_no_runner", "test_charm_scheduled_events", "test_charm_one_runner", "test_charm_metrics_success", "test_charm_metrics_failure", "test_self_hosted_runner", "test_charm_with_proxy", "test_charm_with_juju_storage", "test_debug_ssh"]'
+      modules: '["test_charm_base_image", "test_charm_fork_repo", "test_charm_no_runner", "test_charm_scheduled_events", "test_charm_one_runner", "test_charm_metrics_success", "test_charm_metrics_failure", "test_self_hosted_runner", "test_charm_with_proxy", "test_charm_with_juju_storage", "test_debug_ssh", "test_charm_upgrade"]'
   integration-tests:
     name: Integration test with juju 3.1
     uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main
@@ -25,7 +25,7 @@ jobs:
       pre-run-script: scripts/pre-integration-test.sh
       provider: lxd
       test-tox-env: integration-juju3.1
-      modules: '["test_charm_base_image", "test_charm_fork_repo", "test_charm_no_runner", "test_charm_scheduled_events", "test_charm_one_runner", "test_charm_metrics_success", "test_charm_metrics_failure", "test_self_hosted_runner", "test_charm_with_proxy", "test_charm_with_juju_storage", "test_debug_ssh"]'
+      modules: '["test_charm_base_image", "test_charm_fork_repo", "test_charm_no_runner", "test_charm_scheduled_events", "test_charm_one_runner", "test_charm_metrics_success", "test_charm_metrics_failure", "test_self_hosted_runner", "test_charm_with_proxy", "test_charm_with_juju_storage", "test_debug_ssh", "test_charm_upgrade"]'
   # openstack tests use microstack, whose setup is kind of special
   # - due to the huge resource requirements, we use self-hosted runners for these tests
   # - microstack requires juju 3.2 and microk8s 1.26

diff --git a/src-docs/charm_state.py.md b/src-docs/charm_state.py.md
@@ -225,7 +225,7 @@ The charm state.
 
 ---
 
-<a href="../src/charm_state.py#L868"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm_state.py#L878"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>classmethod</kbd> `from_charm`
 

diff --git a/src/charm_state.py b/src/charm_state.py
@@ -846,24 +846,34 @@ def _check_immutable_config_change(
         prev_state = json.loads(json_data)
         logger.info("Previous charm state: %s", prev_state)
 
-        if prev_state["runner_config"]["runner_storage"] != runner_storage:
-            logger.error(
-                "Storage option changed from %s to %s, blocking the charm",
-                prev_state["runner_config"]["runner_storage"],
-                runner_storage,
-            )
-            raise ImmutableConfigChangedError(
-                msg="runner-storage config cannot be changed after deployment, redeploy if needed"
-            )
-        if prev_state["runner_config"]["base_image"] != base_image.value:
-            logger.error(
-                "Base image option changed from %s to %s, blocking the charm",
-                prev_state["runner_config"]["base_image"],
-                runner_storage,
-            )
-            raise ImmutableConfigChangedError(
-                msg="base-image config cannot be changed after deployment, redeploy if needed"
-            )
+        try:
+            if prev_state["runner_config"]["runner_storage"] != runner_storage:
+                logger.error(
+                    "Storage option changed from %s to %s, blocking the charm",
+                    prev_state["runner_config"]["runner_storage"],
+                    runner_storage,
+                )
+                raise ImmutableConfigChangedError(
+                    msg=(
+                        "runner-storage config cannot be changed after deployment, "
+                        "redeploy if needed"
+                    )
+                )
+        except KeyError as exc:
+            logger.info("Key %s not found, this will be updated to current config.", exc.args[0])
+
+        try:
+            if prev_state["runner_config"]["base_image"] != base_image.value:
+                logger.error(
+                    "Base image option changed from %s to %s, blocking the charm",
+                    prev_state["runner_config"]["base_image"],
+                    runner_storage,
+                )
+                raise ImmutableConfigChangedError(
+                    msg="base-image config cannot be changed after deployment, redeploy if needed"
+                )
+        except KeyError as exc:
+            logger.info("Key %s not found, this will be updated to current config.", exc.args[0])
 
     @classmethod
     def from_charm(cls, charm: CharmBase) -> "CharmState":

diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py
@@ -5,7 +5,6 @@
 import logging
 import random
 import secrets
-import zipfile
 from pathlib import Path
 from time import sleep
 from typing import Any, AsyncIterator, Iterator, Optional
@@ -33,6 +32,7 @@
 from tests.integration.helpers import (
     deploy_github_runner_charm,
     ensure_charm_has_runner,
+    inject_lxd_profile,
     reconcile,
     wait_for,
 )
@@ -61,39 +61,13 @@ def charm_file(
     """Path to the built charm."""
     charm = pytestconfig.getoption("--charm-file")
     assert charm, "Please specify the --charm-file command line option"
+    charm_path_str = f"./{charm}"
 
     if openstack_clouds_yaml:
-        return f"./{charm}"
-
-    lxd_profile_str = """config:
-    security.nesting: true
-    security.privileged: true
-    raw.lxc: |
-        lxc.apparmor.profile=unconfined
-        lxc.mount.auto=proc:rw sys:rw cgroup:rw
-        lxc.cgroup.devices.allow=a
-        lxc.cap.drop=
-devices:
-    kmsg:
-        path: /dev/kmsg
-        source: /dev/kmsg
-        type: unix-char
-"""
-    if loop_device:
-        lxd_profile_str += f"""    loop-control:
-        path: /dev/loop-control
-        type: unix-char
-    loop14:
-        path: {loop_device}
-        type: unix-block
-"""
-
-    with zipfile.ZipFile(charm, mode="a") as charm_file:
-        charm_file.writestr(
-            "lxd-profile.yaml",
-            lxd_profile_str,
-        )
-    return f"./{charm}"
+        return charm_path_str
+
+    inject_lxd_profile(charm_file=Path(charm_path_str), loop_device=loop_device)
+    return charm_path_str
 
 
 @pytest.fixture(scope="module")

diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py
@@ -6,9 +6,11 @@
 import inspect
 import json
 import logging
+import pathlib
 import subprocess
 import time
 import typing
+import zipfile
 from asyncio import sleep
 from datetime import datetime, timezone
 from functools import partial
@@ -374,6 +376,7 @@ async def deploy_github_runner_charm(
     reconcile_interval: int,
     constraints: dict | None = None,
     config: dict | None = None,
+    deploy_kwargs: dict | None = None,
     wait_idle: bool = True,
 ) -> Application:
     """Deploy github-runner charm.
@@ -392,6 +395,7 @@ async def deploy_github_runner_charm(
         constraints: The custom machine constraints to use. See DEFAULT_RUNNER_CONSTRAINTS
             otherwise.
         config: Additional custom config to use.
+        deploy_kwargs: Additional model deploy arguments.
         wait_idle: wait for model to become idle.
 
     Returns:
@@ -432,6 +436,7 @@ async def deploy_github_runner_charm(
         config=default_config,
         constraints=constraints or DEFAULT_RUNNER_CONSTRAINTS,
         storage=storage,
+        **(deploy_kwargs or {}),
     )
 
     if wait_idle:
@@ -606,3 +611,58 @@ async def wait_for(
         if result := func():
             return cast(R, result)
     raise TimeoutError()
+
+
+def inject_lxd_profile(charm_file: pathlib.Path, loop_device: str | None) -> None:
+    """Injects LXD profile to charm file.
+
+    Args:
+        charm_file: Path to charm file to deploy.
+        loop_device: Loop device used to mount runner image.
+    """
+    lxd_profile_str = """config:
+    security.nesting: true
+    security.privileged: true
+    raw.lxc: |
+        lxc.apparmor.profile=unconfined
+        lxc.mount.auto=proc:rw sys:rw cgroup:rw
+        lxc.cgroup.devices.allow=a
+        lxc.cap.drop=
+devices:
+    kmsg:
+        path: /dev/kmsg
+        source: /dev/kmsg
+        type: unix-char
+"""
+    if loop_device:
+        lxd_profile_str += f"""    loop-control:
+        path: /dev/loop-control
+        type: unix-char
+    loop14:
+        path: {loop_device}
+        type: unix-block
+"""
+
+    with zipfile.ZipFile(charm_file, mode="a") as file:
+        file.writestr(
+            "lxd-profile.yaml",
+            lxd_profile_str,
+        )
+
+
+async def is_upgrade_charm_event_emitted(unit: Unit) -> bool:
+    """Check if the upgrade_charm event is emitted.
+
+    This is to ensure false positives from only waiting for ACTIVE status.
+
+    Args:
+        unit: The unit to check for upgrade charm event.
+
+    Returns:
+        bool: True if the event is emitted, False otherwise.
+    """
+    unit_name_without_slash = unit.name.replace("/", "-")
+    juju_unit_log_file = f"/var/log/juju/unit-{unit_name_without_slash}.log"
+    ret_code, stdout, stderr = await run_in_unit(unit=unit, command=f"cat {juju_unit_log_file}")
+    assert ret_code == 0, f"Failed to read the log file: {stderr}"
+    return stdout is not None and "Emitting Juju event upgrade_charm." in stdout
diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py
@@ -2,6 +2,7 @@
 # See LICENSE file for licensing details.
 
 """Integration tests for github-runner charm with no runner."""
+import functools
 import json
 from datetime import datetime, timezone
 
@@ -14,6 +15,7 @@
     check_runner_binary_exists,
     get_repo_policy_compliance_pip_info,
     install_repo_policy_compliance_from_git_source,
+    is_upgrade_charm_event_emitted,
     reconcile,
     remove_runner_bin,
     run_in_unit,
@@ -200,7 +202,9 @@ async def test_reconcile_runners(model: Model, app_no_runner: Application) -> No
 
 @pytest.mark.asyncio
 @pytest.mark.abort_on_fail
-async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_file: str) -> None:
+async def test_charm_no_runner_upgrade(
+    model: Model, app_no_runner: Application, charm_file: str
+) -> None:
     """
     arrange: A working application with no runners.
     act: Upgrade the charm.
@@ -211,22 +215,9 @@ async def test_charm_upgrade(model: Model, app_no_runner: Application, charm_fil
     await app_no_runner.refresh(path=charm_file)
 
     unit = app_no_runner.units[0]
-    unit_name_without_slash = unit.name.replace("/", "-")
-    juju_unit_log_file = f"/var/log/juju/unit-{unit_name_without_slash}.log"
-
-    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, stderr = await run_in_unit(
-            unit=unit, command=f"cat {juju_unit_log_file}"
-        )
-        assert ret_code == 0, f"Failed to read the log file: {stderr}"
-        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 wait_for(
+        functools.partial(is_upgrade_charm_event_emitted, unit), timeout=360, check_interval=60
+    )
     await model.wait_for_idle(status=ACTIVE)
 
     ret_code, stdout, stderr = await run_in_unit(

diff --git a/tests/integration/test_charm_upgrade.py b/tests/integration/test_charm_upgrade.py
@@ -0,0 +1,106 @@
+#  Copyright 2024 Canonical Ltd.
+#  See LICENSE file for licensing details.
+
+"""Integration tests for charm upgrades."""
+
+import functools
+import pathlib
+
+import pytest
+from juju.client import client
+from juju.model import Model
+from pytest_operator.plugin import OpsTest
+
+from charm_state import VIRTUAL_MACHINES_CONFIG_NAME
+from tests.integration.helpers import (
+    deploy_github_runner_charm,
+    inject_lxd_profile,
+    is_upgrade_charm_event_emitted,
+    wait_for,
+)
+
+
+@pytest.mark.asyncio
+async def test_charm_upgrade(
+    model: Model,
+    ops_test: OpsTest,
+    charm_file: str,
+    loop_device: str | None,
+    app_name: str,
+    path: str,
+    token: str,
+    http_proxy: str,
+    https_proxy: str,
+    no_proxy: str,
+    tmp_path: pathlib.Path,
+):
+    """
+    arrange: given latest stable version of the charm (current 161).
+    act: charm upgrade is called.
+    assert: the charm is upgraded successfully.
+    """
+    latest_stable_path = tmp_path / "github-runner.charm"
+    latest_stable_revision = 161  # update this value every release to stable.
+    # download the charm and inject lxd profile for testing
+    retcode, stdout, stderr = await ops_test.juju(
+        "download",
+        "github-runner",
+        # do not specify revision
+        # --revision cannot be specified together with --arch, --base, --channel
+        "--channel",
+        "latest/stable",
+        "--series",
+        "jammy",
+        "--filepath",
+        str(latest_stable_path),
+        "--no-progress",
+    )
+    assert retcode == 0, f"failed to download charm, {stdout} {stderr}"
+    inject_lxd_profile(pathlib.Path(latest_stable_path), loop_device=loop_device)
+
+    # deploy latest stable version of the charm
+    application = await deploy_github_runner_charm(
+        model=model,
+        charm_file=str(latest_stable_path),
+        app_name=app_name,
+        path=path,
+        token=token,
+        runner_storage="juju-storage",
+        http_proxy=http_proxy,
+        https_proxy=https_proxy,
+        no_proxy=no_proxy,
+        reconcile_interval=5,
+        # override default virtual_machines=0 config.
+        config={VIRTUAL_MACHINES_CONFIG_NAME: 1},
+    )
+    await model.wait_for_idle(
+        apps=[application.name],
+        raise_on_error=False,
+        wait_for_active=True,
+        timeout=180 * 60,
+        check_freq=30,
+    )
+    origin = client.CharmOrigin(
+        source="charm-hub",
+        track="22.04",
+        risk="latest/stable",
+        branch="deadbeef",
+        hash_="hash",
+        id_="id",
+        revision=latest_stable_revision,
+        base=client.Base("22.04", "ubuntu"),
+    )
+
+    # upgrade the charm with current local charm
+    await application.local_refresh(path=charm_file, charm_origin=origin)
+    unit = application.units[0]
+    await wait_for(
+        functools.partial(is_upgrade_charm_event_emitted, unit), timeout=360, check_interval=60
+    )
+    await model.wait_for_idle(
+        apps=[application.name],
+        raise_on_error=False,
+        wait_for_active=True,
+        timeout=180 * 60,
+        check_freq=30,
+    )