feat: Job label validation (#382)

* pass over supported_labels * fix docstrings for integration tests * pin correct commit
canonical · Sep 27, 2024 · 24feff6 · 24feff6
1 parent 3d813a9
commit 24feff6
Show file tree

Hide file tree

Showing 5 changed files with 142 additions and 30 deletions.
diff --git a/requirements.txt b/requirements.txt
@@ -12,4 +12,4 @@ cosl ==0.0.15
 # juju 3.1.2.0 depends on pyyaml<=6.0 and >=5.1.2
 PyYAML ==6.0.*
 pyOpenSSL==24.2.1
-github_runner_manager @ git+https://github.com/canonical/github-runner-manager.git@2866c28b62e966a179931201e786c548573e6e7c
+github_runner_manager @ git+https://github.com/canonical/github-runner-manager.git@30fcc502eba82c4c4216f66f61380cfcbed517b1
diff --git a/src-docs/charm.md b/src-docs/charm.md
@@ -18,10 +18,11 @@ Charm for creating and managing GitHub self-hosted runner instances.
 - **RECONCILIATION_INTERVAL_TIMEOUT_FACTOR**
 - **RECONCILE_RUNNERS_EVENT**
 - **REACTIVE_MQ_DB_NAME**
+- **GITHUB_SELF_HOSTED_ARCH_LABELS**
 
 ---
 
-<a href="../src/charm.py#L120"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm.py#L122"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>function</kbd> `catch_charm_errors`
 
@@ -47,7 +48,7 @@ Catch common errors in charm.
 
 ---
 
-<a href="../src/charm.py#L161"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm.py#L163"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>function</kbd> `catch_action_errors`
 
@@ -73,7 +74,7 @@ Catch common errors in actions.
 
 ---
 
-<a href="../src/charm.py#L113"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm.py#L115"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `ReconcileRunnersEvent`
 Event representing a periodic check to ensure runners are ok. 
@@ -84,7 +85,7 @@ Event representing a periodic check to ensure runners are ok.
 
 ---
 
-<a href="../src/charm.py#L199"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm.py#L201"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `GithubRunnerCharm`
 Charm for managing GitHub self-hosted runners. 
@@ -101,7 +102,7 @@ Charm for managing GitHub self-hosted runners.
  - <b>`ram_pool_path`</b>:  The path to memdisk storage. 
  - <b>`kernel_module_path`</b>:  The path to kernel modules. 
 
-<a href="../src/charm.py#L222"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/charm.py#L224"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>method</kbd> `__init__`
 

diff --git a/src/charm.py b/src/charm.py
@@ -107,6 +107,8 @@
 REACTIVE_MQ_DB_NAME = "github-runner-webhook-router"
 
 
+GITHUB_SELF_HOSTED_ARCH_LABELS = {"x64", "arm64"}
+
 logger = logging.getLogger(__name__)
 
 
@@ -1281,18 +1283,39 @@ def _get_runner_scaler(
         )
         reactive_runner_config = None
         if reactive_config := state.reactive_config:
+            # The charm is not able to determine which architecture the runner is running on,
+            # so we add all architectures to the supported labels.
+            supported_labels = set(self._create_labels(state)) | GITHUB_SELF_HOSTED_ARCH_LABELS
             reactive_runner_config = ReactiveRunnerConfig(
                 queue=ReactiveQueueConfig(
                     mongodb_uri=reactive_config.mq_uri, queue_name=self.app.name
                 ),
                 runner_manager=runner_manager_config,
                 cloud_runner_manager=openstack_runner_manager_config,
                 github_token=token,
+                supported_labels=supported_labels,
             )
         return RunnerScaler(
             runner_manager=runner_manager, reactive_runner_config=reactive_runner_config
         )
 
+    @staticmethod
+    def _create_labels(state: CharmState) -> list[str]:
+        """Create Labels instance.
+
+        Args:
+            state: Charm state used to create the labels.
+
+        Returns:
+            An instance of Labels.
+        """
+        image_labels = []
+        image = state.runner_config.openstack_image
+        if image and image.id and image.tags:
+            image_labels = image.tags
+
+        return list(state.charm_config.labels) + image_labels
+
     def _create_openstack_runner_manager_config(
         self, path: GitHubPath, state: CharmState
     ) -> OpenStackRunnerManagerConfig:
@@ -1316,19 +1339,15 @@ def _create_openstack_runner_manager_config(
             cloud=clouds[0],
         )
         server_config = None
-        image_labels = []
         image = state.runner_config.openstack_image
         if image and image.id:
             server_config = OpenStackServerConfig(
                 image=image.id,
                 flavor=state.runner_config.openstack_flavor,
                 network=state.runner_config.openstack_network,
             )
-            if image.tags:
-                image_labels += image.tags
-        runner_config = GitHubRunnerConfig(
-            github_path=path, labels=(*state.charm_config.labels, *image_labels)
-        )
+        labels = self._create_labels(state)
+        runner_config = GitHubRunnerConfig(github_path=path, labels=labels)
         service_config = SupportServiceConfig(
             proxy_config=state.proxy_config,
             dockerhub_mirror=state.charm_config.dockerhub_mirror,

diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py
@@ -761,18 +761,18 @@ async def mongodb_fixture(model: Model, existing_app: str | None) -> Application
 @pytest_asyncio.fixture(scope="module", name="app_for_reactive")
 async def app_for_reactive_fixture(
     model: Model,
-    basic_app: Application,
+    app_openstack_runner: Application,
     mongodb: Application,
     existing_app: Optional[str],
 ) -> Application:
     """Application for testing reactive."""
     if not existing_app:
-        await model.relate(f"{basic_app.name}:mongodb", f"{mongodb.name}:database")
+        await model.relate(f"{app_openstack_runner.name}:mongodb", f"{mongodb.name}:database")
 
-    await basic_app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"})
-    await model.wait_for_idle(apps=[basic_app.name, mongodb.name], status=ACTIVE)
+    await app_openstack_runner.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"})
+    await model.wait_for_idle(apps=[app_openstack_runner.name, mongodb.name], status=ACTIVE)
 
-    return basic_app
+    return app_openstack_runner
 
 
 @pytest_asyncio.fixture(scope="module", name="basic_app")

diff --git a/tests/integration/test_reactive.py b/tests/integration/test_reactive.py
@@ -1,9 +1,9 @@
 #  Copyright 2024 Canonical Ltd.
 #  See LICENSE file for licensing details.
+
+"""Testing reactive mode. This is only supported for the OpenStack cloud."""
 import json
-import random
 import re
-import secrets
 
 import pytest
 from github import Branch, Repository
@@ -21,9 +21,21 @@
     wait_for_completion,
 )
 
+pytestmark = pytest.mark.openstack
+
+
+@pytest.fixture(name="setup_queue", autouse=True)
+async def setup_queue_fixture(
+    ops_test: OpsTest,
+    app_for_reactive: Application,
+):
+    mongodb_uri = await _get_mongodb_uri(ops_test, app_for_reactive)
+
+    _clear_queue(mongodb_uri, app_for_reactive.name)
+    _assert_queue_is_empty(mongodb_uri, app_for_reactive.name)
+
 
-@pytest.mark.openstack
-async def test_reactive_mode_consumes_jobs(
+async def test_reactive_mode_spawns_runner(
     ops_test: OpsTest,
     app_for_reactive: Application,
     github_repository: Repository,
@@ -32,13 +44,9 @@ async def test_reactive_mode_consumes_jobs(
     """
     arrange: A charm integrated with mongodb and a message is added to the queue.
     act: Call reconcile.
-    assert: The message is consumed and the job details are logged.
+    assert: The message is consumed and a runner is spawned.
     """
-    unit = app_for_reactive.units[0]
-    mongodb_uri = await _get_mongodb_uri_from_integration_data(ops_test, unit)
-    if not mongodb_uri:
-        mongodb_uri = await _get_mongodb_uri_from_secrets(ops_test, unit.model)
-    assert mongodb_uri, "mongodb uri not found in integration data or secret"
+    mongodb_uri = await _get_mongodb_uri(ops_test, app_for_reactive)
 
     run = await dispatch_workflow(
         app=app_for_reactive,
@@ -53,10 +61,12 @@ async def test_reactive_mode_consumes_jobs(
     job = jobs[0]
     job_url = job.url
     job = JobDetails(
-        labels=[secrets.token_hex(16) for _ in range(random.randint(1, 4))], url=job_url
+        labels={app_for_reactive.name, "x64"},  # The architecture label should be ignored in the
+        # label validation in the reactive consumer.
+        url=job_url,
     )
     _add_to_queue(
-        json.dumps(job.dict() | {"ignored_noise": "foobar"}),
+        json.dumps(json.loads(job.json()) | {"ignored_noise": "foobar"}),
         mongodb_uri,
         app_for_reactive.name,
     )
@@ -70,6 +80,65 @@ async def test_reactive_mode_consumes_jobs(
     await reconcile(app_for_reactive, app_for_reactive.model)
 
 
+async def test_reactive_mode_does_not_consume_jobs_with_unsupported_labels(
+    ops_test: OpsTest,
+    app_for_reactive: Application,
+    github_repository: Repository,
+    test_github_branch: Branch,
+):
+    """
+    arrange: A charm integrated with mongodb and an unsupported label is added to the queue.
+    act: Call reconcile.
+    assert: No runner is spawned and the message is requeued.
+    """
+    mongodb_uri = await _get_mongodb_uri(ops_test, app_for_reactive)
+
+    run = await dispatch_workflow(
+        app=app_for_reactive,
+        branch=test_github_branch,
+        github_repository=github_repository,
+        conclusion="success",  # this is ignored currently if wait=False kwarg is used
+        workflow_id_or_name=DISPATCH_TEST_WORKFLOW_FILENAME,
+        wait=False,
+    )
+    jobs = list(run.jobs())
+    assert len(jobs) == 1, "Expected 1 job to be created"
+    job = jobs[0]
+    job_url = job.url
+    job = JobDetails(labels={"not supported label"}, url=job_url)
+    _add_to_queue(
+        job.json(),
+        mongodb_uri,
+        app_for_reactive.name,
+    )
+
+    await reconcile(app_for_reactive, app_for_reactive.model)
+
+    run.update()
+    assert run.status == "queued"
+    run.cancel()
+
+    _assert_queue_has_size(mongodb_uri, app_for_reactive.name, 1)
+
+
+async def _get_mongodb_uri(ops_test: OpsTest, app_for_reactive: Application) -> str:
+    """Get the mongodb uri.
+
+    Args:
+        ops_test: The ops_test plugin.
+        app_for_reactive: The juju application containing the unit.
+
+    Returns:
+        The mongodb uri.
+
+    """
+    mongodb_uri = await _get_mongodb_uri_from_integration_data(ops_test, app_for_reactive.units[0])
+    if not mongodb_uri:
+        mongodb_uri = await _get_mongodb_uri_from_secrets(ops_test, app_for_reactive.model)
+    assert mongodb_uri, "mongodb uri not found in integration data or secret"
+    return mongodb_uri
+
+
 async def _get_mongodb_uri_from_integration_data(ops_test: OpsTest, unit: Unit) -> str | None:
     """Get the mongodb uri from the relation data.
 
@@ -136,13 +205,36 @@ def _add_to_queue(msg: str, mongodb_uri: str, queue_name: str) -> None:
             simple_queue.put(msg, retry=True)
 
 
+def _clear_queue(mongodb_uri: str, queue_name: str) -> None:
+    """Clear the queue.
+
+    Args:
+        mongodb_uri: The mongodb uri.
+        queue_name: The name of the queue to clear.
+    """
+    with Connection(mongodb_uri) as conn:
+        with conn.SimpleQueue(queue_name) as simple_queue:
+            simple_queue.clear()
+
+
 def _assert_queue_is_empty(mongodb_uri: str, queue_name: str):
     """Assert that the queue is empty.
 
     Args:
         mongodb_uri: The mongodb uri.
         queue_name: The name of the queue to check.
     """
+    _assert_queue_has_size(mongodb_uri, queue_name, 0)
+
+
+def _assert_queue_has_size(mongodb_uri: str, queue_name: str, size: int):
+    """Assert that the queue is empty.
+
+    Args:
+        mongodb_uri: The mongodb uri.
+        queue_name: The name of the queue to check.
+        size: The expected size of the queue.
+    """
     with Connection(mongodb_uri) as conn:
         with conn.SimpleQueue(queue_name) as simple_queue:
-            assert simple_queue.qsize() == 0
+            assert simple_queue.qsize() == size