From 03a17181f7cc70d31f2d06349c7f93e792e02d87 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 11 Sep 2024 11:04:54 +0200 Subject: [PATCH 01/18] add public interface --- src-docs/reactive.consumer.md | 32 +++++++++++++++---- src-docs/reactive.runner.md | 25 +++++++++++++-- .../reactive/consumer.py | 20 ++++++++++-- src/github_runner_manager/reactive/runner.py | 26 ++++++++++++++- 4 files changed, 90 insertions(+), 13 deletions(-) diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index 99ce9ed..cc46433 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -8,12 +8,12 @@ Module responsible for consuming jobs from the message queue. --- - + ## function `consume` ```python -consume(mongodb_uri: str, queue_name: str) → None +consume(queue_config: QueueConfig, runner_manager: RunnerManager) → None ``` Consume a job from the message queue. @@ -24,8 +24,8 @@ Log the job details and acknowledge the message. If the job details are invalid, **Args:** - - `mongodb_uri`: The URI of the MongoDB database. - - `queue_name`: The name of the queue. + - `queue_config`: The configuration for the message queue. + - `runner_manager`: The runner manager used to create the runner. @@ -36,7 +36,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` @@ -57,7 +57,7 @@ The signal handler exits the process. --- - + ## class `JobDetails` A class to translate the payload. @@ -75,7 +75,25 @@ A class to translate the payload. --- - + + +## class `QueueConfig` +The configuration for the message queue. + + + +**Attributes:** + + - `mongodb_uri`: The URI of the MongoDB database. + - `queue_name`: The name of the queue. + + + + + +--- + + ## class `JobError` Raised when a job error occurs. diff --git a/src-docs/reactive.runner.md b/src-docs/reactive.runner.md index 83fd8b5..4a2dbd1 100644 --- a/src-docs/reactive.runner.md +++ b/src-docs/reactive.runner.md @@ -12,7 +12,7 @@ Script to spawn a reactive runner process. --- - + ## function `setup_root_logging` @@ -25,7 +25,7 @@ Set up logging for the reactive runner process. --- - + ## function `main` @@ -42,3 +42,24 @@ Spawn a process that consumes a message from the queue to create a runner. - `ValueError`: If the required environment variables are not set +--- + + + +## class `ReactiveRunnerConfig` +The configuration for the reactive runner to spawn. + + + +**Attributes:** + + - `queue`: The queue configuration. + - `runner_manager`: The runner manager configuration. + - `runner`: The GitHub runner configuration. + - `openstack_cloud`: The OpenStack cloud configuration. + - `openstack_server`: The OpenStack server configuration. + + + + + diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index f868fed..0a8fd56 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -13,6 +13,9 @@ from kombu import Connection from kombu.simple import SimpleQueue from pydantic import BaseModel, HttpUrl, ValidationError +from pydantic.networks import MongoDsn + +from github_runner_manager.manager.runner_manager import RunnerManager logger = logging.getLogger(__name__) @@ -28,20 +31,31 @@ class JobDetails(BaseModel): labels: list[str] run_url: HttpUrl +class QueueConfig(BaseModel): + """The configuration for the message queue. + + Attributes: + mongodb_uri: The URI of the MongoDB database. + queue_name: The name of the queue. + """ + + mongodb_uri: MongoDsn + queue_name: str + class JobError(Exception): """Raised when a job error occurs.""" -def consume(mongodb_uri: str, queue_name: str) -> None: +def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: """Consume a job from the message queue. Log the job details and acknowledge the message. If the job details are invalid, reject the message and raise an error. Args: - mongodb_uri: The URI of the MongoDB database. - queue_name: The name of the queue. + queue_config: The configuration for the message queue. + runner_manager: The runner manager used to create the runner. Raises: JobError: If the job details are invalid. diff --git a/src/github_runner_manager/reactive/runner.py b/src/github_runner_manager/reactive/runner.py index 415ce6d..7b85b5a 100644 --- a/src/github_runner_manager/reactive/runner.py +++ b/src/github_runner_manager/reactive/runner.py @@ -7,7 +7,13 @@ import os import sys -from github_runner_manager.reactive.consumer import consume +from pydantic import BaseModel + +from github_runner_manager.manager.cloud_runner_manager import GitHubRunnerConfig +from github_runner_manager.manager.runner_manager import RunnerManagerConfig +from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackCloudConfig, \ + OpenStackServerConfig +from github_runner_manager.reactive.consumer import consume, QueueConfig from github_runner_manager.reactive.runner_manager import MQ_URI_ENV_VAR, QUEUE_NAME_ENV_VAR @@ -21,6 +27,24 @@ def setup_root_logging() -> None: ) +class ReactiveRunnerConfig(BaseModel): + """The configuration for the reactive runner to spawn. + + Attributes: + queue: The queue configuration. + runner_manager: The runner manager configuration. + runner: The GitHub runner configuration. + openstack_cloud: The OpenStack cloud configuration. + openstack_server: The OpenStack server configuration. + """ + + queue: QueueConfig + runner_manager: RunnerManagerConfig + runner: GitHubRunnerConfig + openstack_cloud: OpenStackCloudConfig + openstack_server: OpenStackServerConfig | None = None + + def main() -> None: """Spawn a process that consumes a message from the queue to create a runner. From b2ee88bd9625ada6fd152ee2f0ce0ca80bc68098 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 12 Sep 2024 07:52:45 +0200 Subject: [PATCH 02/18] WIP checkin --- src/github_runner_manager/reactive/consumer.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index 0a8fd56..ea67f16 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -60,12 +60,13 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: Raises: JobError: If the job details are invalid. """ - with Connection(mongodb_uri) as conn: - with closing(SimpleQueue(conn, queue_name)) as simple_queue: + with Connection(queue_config.mongodb_uri) as conn: + with closing(SimpleQueue(conn, queue_config.queue_name)) as simple_queue: with signal_handler(signal.SIGTERM): msg = simple_queue.get(block=True) try: job_details = cast(JobDetails, JobDetails.parse_raw(msg.payload)) + runner_manager.create_runners(1) except ValidationError as exc: msg.reject(requeue=True) raise JobError(f"Invalid job details: {msg.payload}") from exc @@ -76,6 +77,9 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: ) msg.ack() +def _check_job(run_url: HttpUrl) -> None: + + @contextlib.contextmanager def signal_handler(signal_code: signal.Signals) -> Generator[None, None, None]: From 62092fa1105179ae7115babbe4ec139c081fcbbd Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 13 Sep 2024 10:14:19 +0200 Subject: [PATCH 03/18] change reactive.runner_manager.reconcile signature --- .../manager/runner_scaler.py | 11 ++++----- .../reactive/consumer.py | 2 -- src/github_runner_manager/reactive/runner.py | 24 ------------------- .../reactive/runner_manager.py | 9 +++---- src/github_runner_manager/types_/__init__.py | 24 +++++++++++++++++++ 5 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/github_runner_manager/manager/runner_scaler.py b/src/github_runner_manager/manager/runner_scaler.py index df92b3f..5cbec8c 100644 --- a/src/github_runner_manager/manager/runner_scaler.py +++ b/src/github_runner_manager/manager/runner_scaler.py @@ -20,7 +20,7 @@ RunnerManager, ) from github_runner_manager.metrics import events as metric_events -from github_runner_manager.types_ import ReactiveConfig +from github_runner_manager.types_ import ReactiveRunnerConfig logger = logging.getLogger(__name__) @@ -49,7 +49,7 @@ class RunnerInfo: class RunnerScaler: """Manage the reconcile of runners.""" - def __init__(self, runner_manager: RunnerManager, reactive_config: ReactiveConfig | None): + def __init__(self, runner_manager: RunnerManager, reactive_config: ReactiveRunnerConfig | None): """Construct the object. Args: @@ -126,7 +126,7 @@ def reconcile(self, quantity: int) -> int: if self._reactive_config is not None: logger.info("Reactive configuration detected, going into experimental reactive mode.") - return self._reconcile_reactive(quantity, self._reactive_config.mq_uri) + return self._reconcile_reactive(quantity) metric_stats = {} start_timestamp = time.time() @@ -249,7 +249,7 @@ def _issue_reconciliation_metric( except IssueMetricEventError: logger.exception("Failed to issue Reconciliation metric") - def _reconcile_reactive(self, quantity: int, mq_uri: MongoDsn) -> int: + def _reconcile_reactive(self, quantity: int) -> int: """Reconcile runners reactively. Args: @@ -263,6 +263,5 @@ def _reconcile_reactive(self, quantity: int, mq_uri: MongoDsn) -> int: logger.info("Reactive mode is experimental and not yet fully implemented.") return reactive_runner_manager.reconcile( quantity=quantity, - mq_uri=mq_uri, - queue_name=self._manager.manager_name, + reactive_config=self._reactive_config, ) diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index ea67f16..5b6a17c 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -77,8 +77,6 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: ) msg.ack() -def _check_job(run_url: HttpUrl) -> None: - @contextlib.contextmanager diff --git a/src/github_runner_manager/reactive/runner.py b/src/github_runner_manager/reactive/runner.py index 7b85b5a..4d40151 100644 --- a/src/github_runner_manager/reactive/runner.py +++ b/src/github_runner_manager/reactive/runner.py @@ -7,12 +7,6 @@ import os import sys -from pydantic import BaseModel - -from github_runner_manager.manager.cloud_runner_manager import GitHubRunnerConfig -from github_runner_manager.manager.runner_manager import RunnerManagerConfig -from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackCloudConfig, \ - OpenStackServerConfig from github_runner_manager.reactive.consumer import consume, QueueConfig from github_runner_manager.reactive.runner_manager import MQ_URI_ENV_VAR, QUEUE_NAME_ENV_VAR @@ -27,24 +21,6 @@ def setup_root_logging() -> None: ) -class ReactiveRunnerConfig(BaseModel): - """The configuration for the reactive runner to spawn. - - Attributes: - queue: The queue configuration. - runner_manager: The runner manager configuration. - runner: The GitHub runner configuration. - openstack_cloud: The OpenStack cloud configuration. - openstack_server: The OpenStack server configuration. - """ - - queue: QueueConfig - runner_manager: RunnerManagerConfig - runner: GitHubRunnerConfig - openstack_cloud: OpenStackCloudConfig - openstack_server: OpenStackServerConfig | None = None - - def main() -> None: """Spawn a process that consumes a message from the queue to create a runner. diff --git a/src/github_runner_manager/reactive/runner_manager.py b/src/github_runner_manager/reactive/runner_manager.py index fd5fddc..97a245f 100644 --- a/src/github_runner_manager/reactive/runner_manager.py +++ b/src/github_runner_manager/reactive/runner_manager.py @@ -11,6 +11,7 @@ import subprocess # nosec from pathlib import Path +from github_runner_manager.types_ import ReactiveRunnerConfig from github_runner_manager.utilities import secure_run_subprocess logger = logging.getLogger(__name__) @@ -37,19 +38,19 @@ class ReactiveRunnerError(Exception): """Raised when a reactive runner error occurs.""" -def reconcile(quantity: int, mq_uri: str, queue_name: str) -> int: +def reconcile(quantity: int, reactive_config: ReactiveRunnerConfig) -> int: """Spawn a runner reactively. Args: quantity: The number of runners to spawn. - mq_uri: The message queue URI. - queue_name: The name of the queue. + reactive_config: The reactive runner configuration. Raises a ReactiveRunnerError if the runner fails to spawn. Returns: The number of reactive runner processes spawned. """ + queue_config = reactive_config.queue pids = _get_pids() current_quantity = len(pids) logger.info("Current quantity of reactive runner processes: %s", current_quantity) @@ -58,7 +59,7 @@ def reconcile(quantity: int, mq_uri: str, queue_name: str) -> int: logger.info("Will spawn %d new reactive runner process(es)", delta) _setup_logging_for_processes() for _ in range(delta): - _spawn_runner(mq_uri=mq_uri, queue_name=queue_name) + _spawn_runner(mq_uri=queue_config.mongodb_uri, queue_name=queue_config.queue_name) elif delta < 0: logger.info("Will kill %d process(es).", -delta) for pid in pids[:-delta]: diff --git a/src/github_runner_manager/types_/__init__.py b/src/github_runner_manager/types_/__init__.py index cd712da..431f383 100644 --- a/src/github_runner_manager/types_/__init__.py +++ b/src/github_runner_manager/types_/__init__.py @@ -6,6 +6,12 @@ from pydantic import AnyHttpUrl, BaseModel, Field, IPvAnyAddress, MongoDsn, validator +from github_runner_manager.manager.cloud_runner_manager import GitHubRunnerConfig +from github_runner_manager.manager.runner_manager import RunnerManagerConfig +from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackCloudConfig, \ + OpenStackServerConfig +from github_runner_manager.reactive.consumer import QueueConfig + class ReactiveConfig(BaseModel): """Represents the configuration for reactive scheduling. @@ -106,3 +112,21 @@ class RepoPolicyComplianceConfig(BaseModel): token: str url: AnyHttpUrl + + +class ReactiveRunnerConfig(BaseModel): + """The configuration for the reactive runner to spawn. + + Attributes: + queue: The queue configuration. + runner_manager: The runner manager configuration. + runner: The GitHub runner configuration. + openstack_cloud: The OpenStack cloud configuration. + openstack_server: The OpenStack server configuration. + """ + + queue: QueueConfig + runner_manager: RunnerManagerConfig + runner: GitHubRunnerConfig + openstack_cloud: OpenStackCloudConfig + openstack_server: OpenStackServerConfig | None = None From 6f6a3b2a2ba7de1781dbf1b365d4807fa664a51c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 13 Sep 2024 10:33:41 +0200 Subject: [PATCH 04/18] move reactive types into own module --- src-docs/manager.runner_scaler.md | 14 +- src-docs/reactive.consumer.md | 24 +--- src-docs/reactive.runner.md | 25 +--- src-docs/reactive.runner_manager.md | 9 +- src-docs/reactive.types_.md | 50 ++++++++ src-docs/types_.general.md | 121 ------------------ src-docs/types_.md | 23 +--- .../manager/runner_scaler.py | 3 +- .../reactive/consumer.py | 13 +- src/github_runner_manager/reactive/runner.py | 2 +- .../reactive/runner_manager.py | 4 +- src/github_runner_manager/reactive/types_.py | 38 ++++++ src/github_runner_manager/types_/__init__.py | 34 ----- 13 files changed, 112 insertions(+), 248 deletions(-) create mode 100644 src-docs/reactive.types_.md delete mode 100644 src-docs/types_.general.md create mode 100644 src/github_runner_manager/reactive/types_.py diff --git a/src-docs/manager.runner_scaler.md b/src-docs/manager.runner_scaler.md index e5abac0..6ef3527 100644 --- a/src-docs/manager.runner_scaler.md +++ b/src-docs/manager.runner_scaler.md @@ -9,7 +9,7 @@ Module for scaling the runners amount. --- - + ## class `RunnerInfo` Information on the runners. @@ -50,17 +50,17 @@ __init__( --- - + ## class `RunnerScaler` Manage the reconcile of runners. - + ### method `__init__` ```python -__init__(runner_manager: RunnerManager, reactive_config: ReactiveConfig | None) +__init__(runner_manager: RunnerManager, reactive_config: RunnerConfig | None) ``` Construct the object. @@ -77,7 +77,7 @@ Construct the object. --- - + ### method `flush` @@ -100,7 +100,7 @@ Flush the runners. --- - + ### method `get_runner_info` @@ -117,7 +117,7 @@ Get information on the runners. --- - + ### method `reconcile` diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index cc46433..2d78544 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -8,7 +8,7 @@ Module responsible for consuming jobs from the message queue. --- - + ## function `consume` @@ -36,7 +36,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` @@ -75,25 +75,7 @@ A class to translate the payload. --- - - -## class `QueueConfig` -The configuration for the message queue. - - - -**Attributes:** - - - `mongodb_uri`: The URI of the MongoDB database. - - `queue_name`: The name of the queue. - - - - - ---- - - + ## class `JobError` Raised when a job error occurs. diff --git a/src-docs/reactive.runner.md b/src-docs/reactive.runner.md index 4a2dbd1..83fd8b5 100644 --- a/src-docs/reactive.runner.md +++ b/src-docs/reactive.runner.md @@ -12,7 +12,7 @@ Script to spawn a reactive runner process. --- - + ## function `setup_root_logging` @@ -25,7 +25,7 @@ Set up logging for the reactive runner process. --- - + ## function `main` @@ -42,24 +42,3 @@ Spawn a process that consumes a message from the queue to create a runner. - `ValueError`: If the required environment variables are not set ---- - - - -## class `ReactiveRunnerConfig` -The configuration for the reactive runner to spawn. - - - -**Attributes:** - - - `queue`: The queue configuration. - - `runner_manager`: The runner manager configuration. - - `runner`: The GitHub runner configuration. - - `openstack_cloud`: The OpenStack cloud configuration. - - `openstack_server`: The OpenStack server configuration. - - - - - diff --git a/src-docs/reactive.runner_manager.md b/src-docs/reactive.runner_manager.md index 774c368..299e5dc 100644 --- a/src-docs/reactive.runner_manager.md +++ b/src-docs/reactive.runner_manager.md @@ -18,12 +18,12 @@ Module for managing reactive runners. --- - + ## function `reconcile` ```python -reconcile(quantity: int, mq_uri: str, queue_name: str) → int +reconcile(quantity: int, reactive_config: RunnerConfig) → int ``` Spawn a runner reactively. @@ -33,8 +33,7 @@ Spawn a runner reactively. **Args:** - `quantity`: The number of runners to spawn. - - `mq_uri`: The message queue URI. - - `queue_name`: The name of the queue. + - `reactive_config`: The reactive runner configuration. Raises a ReactiveRunnerError if the runner fails to spawn. @@ -46,7 +45,7 @@ Raises a ReactiveRunnerError if the runner fails to spawn. --- - + ## class `ReactiveRunnerError` Raised when a reactive runner error occurs. diff --git a/src-docs/reactive.types_.md b/src-docs/reactive.types_.md new file mode 100644 index 0000000..0c39bea --- /dev/null +++ b/src-docs/reactive.types_.md @@ -0,0 +1,50 @@ + + + + +# module `reactive.types_` + + + + + + +--- + + + +## class `QueueConfig` +The configuration for the message queue. + + + +**Attributes:** + + - `mongodb_uri`: The URI of the MongoDB database. + - `queue_name`: The name of the queue. + + + + + +--- + + + +## class `RunnerConfig` +The configuration for the reactive runner to spawn. + + + +**Attributes:** + + - `queue`: The queue configuration. + - `runner_manager`: The runner manager configuration. + - `runner`: The GitHub runner configuration. + - `openstack_cloud`: The OpenStack cloud configuration. + - `openstack_server`: The OpenStack server configuration. + + + + + diff --git a/src-docs/types_.general.md b/src-docs/types_.general.md deleted file mode 100644 index 3a0a036..0000000 --- a/src-docs/types_.general.md +++ /dev/null @@ -1,121 +0,0 @@ - - - - -# module `types_.general` -Module containing a collection of unrelated types. - - - ---- - - - -## class `ReactiveConfig` -Represents the configuration for reactive scheduling. - - - -**Attributes:** - - - `mq_uri`: The URI of the MQ to use to spawn runners reactively. - - - - - ---- - - - -## class `ProxyConfig` -Proxy configuration. - - - -**Attributes:** - - - `aproxy_address`: The address of aproxy snap instance if use_aproxy is enabled. - - `http`: HTTP proxy address. - - `https`: HTTPS proxy address. - - `no_proxy`: Comma-separated list of hosts that should not be proxied. - - `use_aproxy`: Whether aproxy should be used for the runners. - - ---- - -#### property aproxy_address - -Return the aproxy address. - - - ---- - - - -### classmethod `check_use_aproxy` - -```python -check_use_aproxy(use_aproxy: bool, values: dict) → bool -``` - -Validate the proxy configuration. - - - -**Args:** - - - `use_aproxy`: Value of use_aproxy variable. - - `values`: Values in the pydantic model. - - - -**Raises:** - - - `ValueError`: if use_aproxy was set but no http/https was passed. - - - -**Returns:** - Validated use_aproxy value. - - ---- - - - -## class `SSHDebugConnection` -SSH connection information for debug workflow. - - - -**Attributes:** - - - `host`: The SSH relay server host IP address inside the VPN. - - `port`: The SSH relay server port. - - `rsa_fingerprint`: The host SSH server public RSA key fingerprint. - - `ed25519_fingerprint`: The host SSH server public ed25519 key fingerprint. - - - - - ---- - - - -## class `RepoPolicyComplianceConfig` -Configuration for the repo policy compliance service. - - - -**Attributes:** - - - `token`: Token for the repo policy compliance service. - - `url`: URL of the repo policy compliance service. - - - - - diff --git a/src-docs/types_.md b/src-docs/types_.md index b423cd2..8f73fad 100644 --- a/src-docs/types_.md +++ b/src-docs/types_.md @@ -11,23 +11,6 @@ Package containing modules with type definitions. -## class `ReactiveConfig` -Represents the configuration for reactive scheduling. - - - -**Attributes:** - - - `mq_uri`: The URI of the MQ to use to spawn runners reactively. - - - - - ---- - - - ## class `ProxyConfig` Proxy configuration. @@ -52,7 +35,7 @@ Return the aproxy address. --- - + ### classmethod `check_use_aproxy` @@ -83,7 +66,7 @@ Validate the proxy configuration. --- - + ## class `SSHDebugConnection` SSH connection information for debug workflow. @@ -103,7 +86,7 @@ SSH connection information for debug workflow. --- - + ## class `RepoPolicyComplianceConfig` Configuration for the repo policy compliance service. diff --git a/src/github_runner_manager/manager/runner_scaler.py b/src/github_runner_manager/manager/runner_scaler.py index 5cbec8c..40f956d 100644 --- a/src/github_runner_manager/manager/runner_scaler.py +++ b/src/github_runner_manager/manager/runner_scaler.py @@ -7,7 +7,6 @@ import time from dataclasses import dataclass -from pydantic import MongoDsn import github_runner_manager.reactive.runner_manager as reactive_runner_manager from github_runner_manager.errors import IssueMetricEventError, MissingServerConfigError @@ -20,7 +19,7 @@ RunnerManager, ) from github_runner_manager.metrics import events as metric_events -from github_runner_manager.types_ import ReactiveRunnerConfig +from github_runner_manager.reactive.types_ import RunnerConfig as ReactiveRunnerConfig logger = logging.getLogger(__name__) diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index 5b6a17c..f5d7378 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -13,9 +13,9 @@ from kombu import Connection from kombu.simple import SimpleQueue from pydantic import BaseModel, HttpUrl, ValidationError -from pydantic.networks import MongoDsn from github_runner_manager.manager.runner_manager import RunnerManager +from github_runner_manager.reactive.types_ import QueueConfig logger = logging.getLogger(__name__) @@ -31,17 +31,6 @@ class JobDetails(BaseModel): labels: list[str] run_url: HttpUrl -class QueueConfig(BaseModel): - """The configuration for the message queue. - - Attributes: - mongodb_uri: The URI of the MongoDB database. - queue_name: The name of the queue. - """ - - mongodb_uri: MongoDsn - queue_name: str - class JobError(Exception): """Raised when a job error occurs.""" diff --git a/src/github_runner_manager/reactive/runner.py b/src/github_runner_manager/reactive/runner.py index 4d40151..415ce6d 100644 --- a/src/github_runner_manager/reactive/runner.py +++ b/src/github_runner_manager/reactive/runner.py @@ -7,7 +7,7 @@ import os import sys -from github_runner_manager.reactive.consumer import consume, QueueConfig +from github_runner_manager.reactive.consumer import consume from github_runner_manager.reactive.runner_manager import MQ_URI_ENV_VAR, QUEUE_NAME_ENV_VAR diff --git a/src/github_runner_manager/reactive/runner_manager.py b/src/github_runner_manager/reactive/runner_manager.py index 97a245f..b83ab8d 100644 --- a/src/github_runner_manager/reactive/runner_manager.py +++ b/src/github_runner_manager/reactive/runner_manager.py @@ -11,7 +11,7 @@ import subprocess # nosec from pathlib import Path -from github_runner_manager.types_ import ReactiveRunnerConfig +from github_runner_manager.reactive.types_ import RunnerConfig from github_runner_manager.utilities import secure_run_subprocess logger = logging.getLogger(__name__) @@ -38,7 +38,7 @@ class ReactiveRunnerError(Exception): """Raised when a reactive runner error occurs.""" -def reconcile(quantity: int, reactive_config: ReactiveRunnerConfig) -> int: +def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: """Spawn a runner reactively. Args: diff --git a/src/github_runner_manager/reactive/types_.py b/src/github_runner_manager/reactive/types_.py new file mode 100644 index 0000000..b5cea28 --- /dev/null +++ b/src/github_runner_manager/reactive/types_.py @@ -0,0 +1,38 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +from pydantic import BaseModel, MongoDsn + +from github_runner_manager.manager.cloud_runner_manager import GitHubRunnerConfig +from github_runner_manager.manager.runner_manager import RunnerManagerConfig +from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackCloudConfig, \ + OpenStackServerConfig + + +class QueueConfig(BaseModel): + """The configuration for the message queue. + + Attributes: + mongodb_uri: The URI of the MongoDB database. + queue_name: The name of the queue. + """ + + mongodb_uri: MongoDsn + queue_name: str + + +class RunnerConfig(BaseModel): + """The configuration for the reactive runner to spawn. + + Attributes: + queue: The queue configuration. + runner_manager: The runner manager configuration. + runner: The GitHub runner configuration. + openstack_cloud: The OpenStack cloud configuration. + openstack_server: The OpenStack server configuration. + """ + + queue: QueueConfig + runner_manager: RunnerManagerConfig + runner: GitHubRunnerConfig + openstack_cloud: OpenStackCloudConfig + openstack_server: OpenStackServerConfig | None = None diff --git a/src/github_runner_manager/types_/__init__.py b/src/github_runner_manager/types_/__init__.py index 431f383..300495c 100644 --- a/src/github_runner_manager/types_/__init__.py +++ b/src/github_runner_manager/types_/__init__.py @@ -6,22 +6,6 @@ from pydantic import AnyHttpUrl, BaseModel, Field, IPvAnyAddress, MongoDsn, validator -from github_runner_manager.manager.cloud_runner_manager import GitHubRunnerConfig -from github_runner_manager.manager.runner_manager import RunnerManagerConfig -from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackCloudConfig, \ - OpenStackServerConfig -from github_runner_manager.reactive.consumer import QueueConfig - - -class ReactiveConfig(BaseModel): - """Represents the configuration for reactive scheduling. - - Attributes: - mq_uri: The URI of the MQ to use to spawn runners reactively. - """ - - mq_uri: MongoDsn - class ProxyConfig(BaseModel): """Proxy configuration. @@ -112,21 +96,3 @@ class RepoPolicyComplianceConfig(BaseModel): token: str url: AnyHttpUrl - - -class ReactiveRunnerConfig(BaseModel): - """The configuration for the reactive runner to spawn. - - Attributes: - queue: The queue configuration. - runner_manager: The runner manager configuration. - runner: The GitHub runner configuration. - openstack_cloud: The OpenStack cloud configuration. - openstack_server: The OpenStack server configuration. - """ - - queue: QueueConfig - runner_manager: RunnerManagerConfig - runner: GitHubRunnerConfig - openstack_cloud: OpenStackCloudConfig - openstack_server: OpenStackServerConfig | None = None From fd91b026005ae0c974b528ba21089f79414b3968 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 13 Sep 2024 11:53:11 +0200 Subject: [PATCH 05/18] refactor config and use it in script --- src-docs/manager.runner_manager.md | 14 ++--- src-docs/manager.runner_scaler.md | 19 +++--- ...penstack_cloud.openstack_runner_manager.md | 63 ++++++++++++++----- src-docs/reactive.consumer.md | 2 +- src-docs/reactive.runner.md | 7 +-- src-docs/reactive.runner_manager.md | 7 +-- src-docs/reactive.types_.md | 12 ++-- .../manager/runner_manager.py | 6 +- .../manager/runner_scaler.py | 10 +-- .../openstack_runner_manager.py | 51 ++++++++------- .../reactive/consumer.py | 3 +- src/github_runner_manager/reactive/runner.py | 26 ++++---- .../reactive/runner_manager.py | 14 ++--- src/github_runner_manager/reactive/types_.py | 17 +++-- src/github_runner_manager/types_/__init__.py | 2 +- 15 files changed, 141 insertions(+), 112 deletions(-) diff --git a/src-docs/manager.runner_manager.md b/src-docs/manager.runner_manager.md index 652f3e9..92997c8 100644 --- a/src-docs/manager.runner_manager.md +++ b/src-docs/manager.runner_manager.md @@ -77,6 +77,7 @@ Configuration for the runner manager. **Attributes:** + - `name`: A name to identify this manager. - `token`: GitHub personal access token to query GitHub API. - `path`: Path to GitHub repository or organization to registry the runners. @@ -85,7 +86,7 @@ Configuration for the runner manager. ### method `__init__` ```python -__init__(token: str, path: GitHubOrg | GitHubRepo) → None +__init__(name: str, token: str, path: GitHubOrg | GitHubRepo) → None ``` @@ -98,7 +99,7 @@ __init__(token: str, path: GitHubOrg | GitHubRepo) → None --- - + ## class `RunnerManager` Manage the runners. @@ -110,16 +111,12 @@ Manage the runners. - `manager_name`: A name to identify this manager. - `name_prefix`: The name prefix of the runners. - + ### method `__init__` ```python -__init__( - manager_name: str, - cloud_runner_manager: CloudRunnerManager, - config: RunnerManagerConfig -) +__init__(cloud_runner_manager: CloudRunnerManager, config: RunnerManagerConfig) ``` Construct the object. @@ -128,7 +125,6 @@ Construct the object. **Args:** - - `manager_name`: A name to identify this manager. - `cloud_runner_manager`: For managing the cloud instance of the runner. - `config`: Configuration of this class. diff --git a/src-docs/manager.runner_scaler.md b/src-docs/manager.runner_scaler.md index 6ef3527..c567451 100644 --- a/src-docs/manager.runner_scaler.md +++ b/src-docs/manager.runner_scaler.md @@ -9,7 +9,7 @@ Module for scaling the runners amount. --- - + ## class `RunnerInfo` Information on the runners. @@ -50,17 +50,20 @@ __init__( --- - + ## class `RunnerScaler` Manage the reconcile of runners. - + ### method `__init__` ```python -__init__(runner_manager: RunnerManager, reactive_config: RunnerConfig | None) +__init__( + runner_manager: RunnerManager, + reactive_runner_config: RunnerConfig | None +) ``` Construct the object. @@ -70,14 +73,14 @@ Construct the object. **Args:** - `runner_manager`: The RunnerManager to perform runner reconcile. - - `reactive_config`: Reactive runner configuration. + - `reactive_runner_config`: Reactive runner configuration. --- - + ### method `flush` @@ -100,7 +103,7 @@ Flush the runners. --- - + ### method `get_runner_info` @@ -117,7 +120,7 @@ Get information on the runners. --- - + ### method `reconcile` diff --git a/src-docs/openstack_cloud.openstack_runner_manager.md b/src-docs/openstack_cloud.openstack_runner_manager.md index be23251..ee45ee0 100644 --- a/src-docs/openstack_cloud.openstack_runner_manager.md +++ b/src-docs/openstack_cloud.openstack_runner_manager.md @@ -78,18 +78,23 @@ __init__(image: str, flavor: str, network: str) → None --- - + -## class `OpenStackRunnerManager` -Manage self-hosted runner on OpenStack cloud. +## class `OpenStackRunnerManagerConfig` +Configuration for OpenStack runner manager. **Attributes:** - - `name_prefix`: The name prefix of the runners created. + - `manager_name`: The name of the manager. + - `prefix`: The prefix of the runner names. + - `cloud_config`: The configuration for OpenStack cloud. + - `server_config`: The configuration for OpenStack server. + - `runner_config`: The configuration for the GitHub runner. + - `service_config`: The configuration for supporting services. - + ### method `__init__` @@ -104,18 +109,42 @@ __init__( ) → None ``` + + + + + + + + +--- + + + +## class `OpenStackRunnerManager` +Manage self-hosted runner on OpenStack cloud. + + + +**Attributes:** + + - `name_prefix`: The name prefix of the runners created. + + + +### method `__init__` + +```python +__init__(config: OpenStackRunnerManagerConfig) → None +``` + Construct the object. **Args:** - - `manager_name`: A name to identify this manager. - - `prefix`: The prefix to runner name. - - `cloud_config`: The configuration for OpenStack authorisation. - - `server_config`: The configuration for creating OpenStack server. Unable to create runner if None. - - `runner_config`: The configuration for the runner. - - `service_config`: The configuration of supporting services of the runners. + - `config`: The configuration for the openstack runner manager. --- @@ -133,7 +162,7 @@ The prefix of runner names. --- - + ### method `cleanup` @@ -156,7 +185,7 @@ Cleanup runner and resource on the cloud. --- - + ### method `create_runner` @@ -186,7 +215,7 @@ Create a self-hosted runner. --- - + ### method `delete_runner` @@ -210,7 +239,7 @@ Delete self-hosted runners. --- - + ### method `flush_runners` @@ -233,7 +262,7 @@ Remove idle and/or busy runners. --- - + ### method `get_runner` @@ -256,7 +285,7 @@ Get a self-hosted runner by instance id. --- - + ### method `get_runners` diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index 2d78544..d7fe26f 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -36,7 +36,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` diff --git a/src-docs/reactive.runner.md b/src-docs/reactive.runner.md index 83fd8b5..a9afd69 100644 --- a/src-docs/reactive.runner.md +++ b/src-docs/reactive.runner.md @@ -7,12 +7,11 @@ Script to spawn a reactive runner process. **Global Variables** --------------- -- **MQ_URI_ENV_VAR** -- **QUEUE_NAME_ENV_VAR** +- **RUNNER_CONFIG_ENV_VAR** --- - + ## function `setup_root_logging` @@ -25,7 +24,7 @@ Set up logging for the reactive runner process. --- - + ## function `main` diff --git a/src-docs/reactive.runner_manager.md b/src-docs/reactive.runner_manager.md index 299e5dc..ffb8b00 100644 --- a/src-docs/reactive.runner_manager.md +++ b/src-docs/reactive.runner_manager.md @@ -7,18 +7,17 @@ Module for managing reactive runners. **Global Variables** --------------- -- **MQ_URI_ENV_VAR** -- **QUEUE_NAME_ENV_VAR** - **PYTHON_BIN** - **REACTIVE_RUNNER_SCRIPT_MODULE** - **REACTIVE_RUNNER_CMD_LINE_PREFIX** - **PID_CMD_COLUMN_WIDTH** - **PIDS_COMMAND_LINE** - **UBUNTU_USER** +- **RUNNER_CONFIG_ENV_VAR** --- - + ## function `reconcile` @@ -45,7 +44,7 @@ Raises a ReactiveRunnerError if the runner fails to spawn. --- - + ## class `ReactiveRunnerError` Raised when a reactive runner error occurs. diff --git a/src-docs/reactive.types_.md b/src-docs/reactive.types_.md index 0c39bea..039de3a 100644 --- a/src-docs/reactive.types_.md +++ b/src-docs/reactive.types_.md @@ -3,15 +3,13 @@ # module `reactive.types_` - - - +Module containing reactive scheduling related types. --- - + ## class `QueueConfig` The configuration for the message queue. @@ -29,7 +27,7 @@ The configuration for the message queue. --- - + ## class `RunnerConfig` The configuration for the reactive runner to spawn. @@ -40,9 +38,7 @@ The configuration for the reactive runner to spawn. - `queue`: The queue configuration. - `runner_manager`: The runner manager configuration. - - `runner`: The GitHub runner configuration. - - `openstack_cloud`: The OpenStack cloud configuration. - - `openstack_server`: The OpenStack server configuration. + - `cloud_runner_manager`: The OpenStack runner manager configuration. diff --git a/src/github_runner_manager/manager/runner_manager.py b/src/github_runner_manager/manager/runner_manager.py index f8ef0ae..76f6b34 100644 --- a/src/github_runner_manager/manager/runner_manager.py +++ b/src/github_runner_manager/manager/runner_manager.py @@ -83,10 +83,12 @@ class RunnerManagerConfig: """Configuration for the runner manager. Attributes: + name: A name to identify this manager. token: GitHub personal access token to query GitHub API. path: Path to GitHub repository or organization to registry the runners. """ + name: str token: str path: GitHubPath @@ -101,18 +103,16 @@ class RunnerManager: def __init__( self, - manager_name: str, cloud_runner_manager: CloudRunnerManager, config: RunnerManagerConfig, ): """Construct the object. Args: - manager_name: A name to identify this manager. cloud_runner_manager: For managing the cloud instance of the runner. config: Configuration of this class. """ - self.manager_name = manager_name + self.manager_name = config.name self._config = config self._cloud = cloud_runner_manager self.name_prefix = self._cloud.name_prefix diff --git a/src/github_runner_manager/manager/runner_scaler.py b/src/github_runner_manager/manager/runner_scaler.py index 40f956d..35ffdf9 100644 --- a/src/github_runner_manager/manager/runner_scaler.py +++ b/src/github_runner_manager/manager/runner_scaler.py @@ -7,7 +7,6 @@ import time from dataclasses import dataclass - import github_runner_manager.reactive.runner_manager as reactive_runner_manager from github_runner_manager.errors import IssueMetricEventError, MissingServerConfigError from github_runner_manager.manager.cloud_runner_manager import HealthState @@ -48,15 +47,17 @@ class RunnerInfo: class RunnerScaler: """Manage the reconcile of runners.""" - def __init__(self, runner_manager: RunnerManager, reactive_config: ReactiveRunnerConfig | None): + def __init__( + self, runner_manager: RunnerManager, reactive_runner_config: ReactiveRunnerConfig | None + ): """Construct the object. Args: runner_manager: The RunnerManager to perform runner reconcile. - reactive_config: Reactive runner configuration. + reactive_runner_config: Reactive runner configuration. """ self._manager = runner_manager - self._reactive_config = reactive_config + self._reactive_config = reactive_runner_config def get_runner_info(self) -> RunnerInfo: """Get information on the runners. @@ -253,7 +254,6 @@ def _reconcile_reactive(self, quantity: int) -> int: Args: quantity: Number of intended runners. - mq_uri: The URI of the MQ to use to spawn runners reactively. Returns: The difference between intended runners and actual runners. In reactive mode diff --git a/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index 23fdeb1..52ca177 100644 --- a/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -98,6 +98,27 @@ class OpenStackServerConfig: network: str +@dataclass +class OpenStackRunnerManagerConfig: + """Configuration for OpenStack runner manager. + + Attributes: + manager_name: The name of the manager. + prefix: The prefix of the runner names. + cloud_config: The configuration for OpenStack cloud. + server_config: The configuration for OpenStack server. + runner_config: The configuration for the GitHub runner. + service_config: The configuration for supporting services. + """ + + manager_name: str + prefix: str + cloud_config: OpenStackCloudConfig + server_config: OpenStackServerConfig | None + runner_config: GitHubRunnerConfig + service_config: SupportServiceConfig + + @dataclass class _RunnerHealth: """Runners with health state. @@ -118,33 +139,21 @@ class OpenStackRunnerManager(CloudRunnerManager): name_prefix: The name prefix of the runners created. """ - # Ignore "Too many arguments", as the class requires a lot of configurations. def __init__( # pylint: disable=R0913 self, - manager_name: str, - prefix: str, - cloud_config: OpenStackCloudConfig, - server_config: OpenStackServerConfig | None, - runner_config: GitHubRunnerConfig, - service_config: SupportServiceConfig, + config: OpenStackRunnerManagerConfig, ) -> None: """Construct the object. Args: - manager_name: A name to identify this manager. - prefix: The prefix to runner name. - cloud_config: The configuration for OpenStack authorisation. - server_config: The configuration for creating OpenStack server. Unable to create - runner if None. - runner_config: The configuration for the runner. - service_config: The configuration of supporting services of the runners. + config: The configuration for the openstack runner manager. """ - self._manager_name = manager_name - self._prefix = prefix - self._cloud_config = cloud_config - self._server_config = server_config - self._runner_config = runner_config - self._service_config = service_config + self._manager_name = config.manager_name + self._prefix = config.prefix + self._cloud_config = config.cloud_config + self._server_config = config.server_config + self._runner_config = config.runner_config + self._service_config = config.service_config self._openstack_cloud = OpenstackCloud( clouds_config=self._cloud_config.clouds_config, cloud=self._cloud_config.cloud, @@ -152,7 +161,7 @@ def __init__( # pylint: disable=R0913 ) # Setting the env var to this process and any child process spawned. - proxies = service_config.proxy_config + proxies = self._service_config.proxy_config if proxies and (no_proxy := proxies.no_proxy): set_env_var("NO_PROXY", no_proxy) if proxies and (http_proxy := proxies.http): diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index f5d7378..7723fa9 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -55,7 +55,7 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: msg = simple_queue.get(block=True) try: job_details = cast(JobDetails, JobDetails.parse_raw(msg.payload)) - runner_manager.create_runners(1) + #runner_manager.create_runners(1) except ValidationError as exc: msg.reject(requeue=True) raise JobError(f"Invalid job details: {msg.payload}") from exc @@ -67,7 +67,6 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: msg.ack() - @contextlib.contextmanager def signal_handler(signal_code: signal.Signals) -> Generator[None, None, None]: """Set a signal handler and after the context, restore the default handler. diff --git a/src/github_runner_manager/reactive/runner.py b/src/github_runner_manager/reactive/runner.py index 415ce6d..5042fde 100644 --- a/src/github_runner_manager/reactive/runner.py +++ b/src/github_runner_manager/reactive/runner.py @@ -7,8 +7,11 @@ import os import sys +from github_runner_manager.manager.runner_manager import RunnerManager +from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackRunnerManager from github_runner_manager.reactive.consumer import consume -from github_runner_manager.reactive.runner_manager import MQ_URI_ENV_VAR, QUEUE_NAME_ENV_VAR +from github_runner_manager.reactive.runner_manager import RUNNER_CONFIG_ENV_VAR +from github_runner_manager.reactive.types_ import RunnerConfig def setup_root_logging() -> None: @@ -27,23 +30,24 @@ def main() -> None: Raises: ValueError: If the required environment variables are not set """ - mq_uri = os.environ.get(MQ_URI_ENV_VAR) - queue_name = os.environ.get(QUEUE_NAME_ENV_VAR) + runner_config_str = os.environ.get(RUNNER_CONFIG_ENV_VAR) - if not mq_uri: + if not runner_config_str: raise ValueError( - f"Missing {MQ_URI_ENV_VAR} environment variable. " + f"Missing {RUNNER_CONFIG_ENV_VAR} environment variable. " "Please set it to the message queue URI." ) - if not queue_name: - raise ValueError( - f"Missing {QUEUE_NAME_ENV_VAR} environment variable. " - "Please set it to the name of the queue." - ) + runner_config = RunnerConfig.parse_raw(runner_config_str) setup_root_logging() - consume(mq_uri, queue_name) + queue_config = runner_config.queue + openstack_runner_manager = OpenStackRunnerManager(config=runner_config.cloud_runner_manager) + runner_manager = RunnerManager( + cloud_runner_manager=openstack_runner_manager, + config=runner_config.runner_manager, + ) + consume(queue_config=queue_config, runner_manager=runner_manager) if __name__ == "__main__": diff --git a/src/github_runner_manager/reactive/runner_manager.py b/src/github_runner_manager/reactive/runner_manager.py index b83ab8d..5ac6538 100644 --- a/src/github_runner_manager/reactive/runner_manager.py +++ b/src/github_runner_manager/reactive/runner_manager.py @@ -16,8 +16,6 @@ logger = logging.getLogger(__name__) -MQ_URI_ENV_VAR = "MQ_URI" -QUEUE_NAME_ENV_VAR = "QUEUE_NAME" REACTIVE_RUNNER_LOG_DIR = Path("/var/log/reactive_runner") PYTHON_BIN = "/usr/bin/python3" @@ -32,6 +30,7 @@ "--sort=-start_time", ] UBUNTU_USER = "ubuntu" +RUNNER_CONFIG_ENV_VAR = "RUNNER_CONFIG" class ReactiveRunnerError(Exception): @@ -50,7 +49,6 @@ def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: Returns: The number of reactive runner processes spawned. """ - queue_config = reactive_config.queue pids = _get_pids() current_quantity = len(pids) logger.info("Current quantity of reactive runner processes: %s", current_quantity) @@ -59,7 +57,7 @@ def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: logger.info("Will spawn %d new reactive runner process(es)", delta) _setup_logging_for_processes() for _ in range(delta): - _spawn_runner(mq_uri=queue_config.mongodb_uri, queue_name=queue_config.queue_name) + _spawn_runner(reactive_config) elif delta < 0: logger.info("Will kill %d process(es).", -delta) for pid in pids[:-delta]: @@ -106,17 +104,15 @@ def _setup_logging_for_processes() -> None: shutil.chown(REACTIVE_RUNNER_LOG_DIR, user=UBUNTU_USER, group=UBUNTU_USER) -def _spawn_runner(mq_uri: str, queue_name: str) -> None: +def _spawn_runner(runner_config: RunnerConfig) -> None: """Spawn a runner. Args: - mq_uri: The message queue URI. - queue_name: The name of the queue. + runner_config: The runner configuration to pass to the spawned runner process. """ env = { "PYTHONPATH": os.environ["PYTHONPATH"], - MQ_URI_ENV_VAR: mq_uri, - QUEUE_NAME_ENV_VAR: queue_name, + RUNNER_CONFIG_ENV_VAR: runner_config.json(), } # We do not want to wait for the process to finish, so we do not use with statement. # We trust the command. diff --git a/src/github_runner_manager/reactive/types_.py b/src/github_runner_manager/reactive/types_.py index b5cea28..aac315f 100644 --- a/src/github_runner_manager/reactive/types_.py +++ b/src/github_runner_manager/reactive/types_.py @@ -1,11 +1,14 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. + +"""Module containing reactive scheduling related types.""" + from pydantic import BaseModel, MongoDsn -from github_runner_manager.manager.cloud_runner_manager import GitHubRunnerConfig from github_runner_manager.manager.runner_manager import RunnerManagerConfig -from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackCloudConfig, \ - OpenStackServerConfig +from github_runner_manager.openstack_cloud.openstack_runner_manager import ( + OpenStackRunnerManagerConfig, +) class QueueConfig(BaseModel): @@ -26,13 +29,9 @@ class RunnerConfig(BaseModel): Attributes: queue: The queue configuration. runner_manager: The runner manager configuration. - runner: The GitHub runner configuration. - openstack_cloud: The OpenStack cloud configuration. - openstack_server: The OpenStack server configuration. + cloud_runner_manager: The OpenStack runner manager configuration. """ queue: QueueConfig runner_manager: RunnerManagerConfig - runner: GitHubRunnerConfig - openstack_cloud: OpenStackCloudConfig - openstack_server: OpenStackServerConfig | None = None + cloud_runner_manager: OpenStackRunnerManagerConfig diff --git a/src/github_runner_manager/types_/__init__.py b/src/github_runner_manager/types_/__init__.py index 300495c..70127e1 100644 --- a/src/github_runner_manager/types_/__init__.py +++ b/src/github_runner_manager/types_/__init__.py @@ -4,7 +4,7 @@ """Package containing modules with type definitions.""" from typing import Optional -from pydantic import AnyHttpUrl, BaseModel, Field, IPvAnyAddress, MongoDsn, validator +from pydantic import AnyHttpUrl, BaseModel, Field, IPvAnyAddress, validator class ProxyConfig(BaseModel): From 6e0875d8287a7f13b2a187dcd144a9046cfa7981 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 13 Sep 2024 14:36:37 +0200 Subject: [PATCH 06/18] WIP checkin unit tests --- .../reactive/runner_manager.py | 2 +- tests/unit/reactive/test_consumer.py | 25 +++++++++----- tests/unit/reactive/test_runner_manager.py | 33 ++++++++++++------- tests/unit/test_openstack_runner_manager.py | 5 +-- tests/unit/test_runner_scaler.py | 4 +-- 5 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/github_runner_manager/reactive/runner_manager.py b/src/github_runner_manager/reactive/runner_manager.py index 5ac6538..ea895e0 100644 --- a/src/github_runner_manager/reactive/runner_manager.py +++ b/src/github_runner_manager/reactive/runner_manager.py @@ -36,7 +36,7 @@ class ReactiveRunnerError(Exception): """Raised when a reactive runner error occurs.""" - +# TODO: rename reactive_config argument to runner_config def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: """Spawn a runner reactively. diff --git a/tests/unit/reactive/test_consumer.py b/tests/unit/reactive/test_consumer.py index c036b92..a39630a 100644 --- a/tests/unit/reactive/test_consumer.py +++ b/tests/unit/reactive/test_consumer.py @@ -3,32 +3,41 @@ import secrets from contextlib import closing +from unittest.mock import MagicMock import pytest from kombu import Connection, Message from github_runner_manager.reactive import consumer from github_runner_manager.reactive.consumer import JobError +from github_runner_manager.reactive.types_ import QueueConfig IN_MEMORY_URI = "memory://" FAKE_RUN_URL = "https://api.github.com/repos/fakeusergh-runner-test/actions/runs/8200803099" -def test_consume(caplog: pytest.LogCaptureFixture): +@pytest.fixture(name="queue_config") +def queue_config_fixture() -> QueueConfig: + """Return a QueueConfig object.""" + queue_name = secrets.token_hex(16) + + # we use construct to avoid pydantic validation as IN_MEMORY_URI is not a valid URL + return QueueConfig.construct(mongodb_uri=IN_MEMORY_URI, queue_name=queue_name) + +def test_consume(caplog: pytest.LogCaptureFixture, queue_config: QueueConfig): """ arrange: A job placed in the message queue. act: Call consume assert: The job is logged. """ - queue_name = secrets.token_hex(16) job_details = consumer.JobDetails( labels=[secrets.token_hex(16), secrets.token_hex(16)], run_url=FAKE_RUN_URL, ) - _put_in_queue(job_details.json(), queue_name) + _put_in_queue(job_details.json(), queue_config.queue_name) - # we use construct to avoid pydantic validation as IN_MEMORY_URI is not a valid URL - consumer.consume(IN_MEMORY_URI, queue_name) + + consumer.consume(queue_config=queue_config, runner_manager=MagicMock()) assert str(job_details.labels) in caplog.text assert job_details.run_url in caplog.text @@ -45,17 +54,17 @@ def test_consume(caplog: pytest.LogCaptureFixture): pytest.param("no json at all", id="invalid json"), ], ) -def test_job_details_validation_error(job_str: str): +def test_job_details_validation_error(job_str: str, queue_config: QueueConfig): """ arrange: A job placed in the message queue with invalid details. act: Call consume assert: A JobError is raised and the message is requeued. """ - queue_name = secrets.token_hex(16) + queue_name = queue_config.queue_name _put_in_queue(job_str, queue_name) with pytest.raises(JobError) as exc_info: - consumer.consume(IN_MEMORY_URI, queue_name) + consumer.consume(queue_config=queue_config, runner_manager=MagicMock()) assert "Invalid job details" in str(exc_info.value) # Ensure message has been requeued by reconsuming it diff --git a/tests/unit/reactive/test_runner_manager.py b/tests/unit/reactive/test_runner_manager.py index b6de746..04adc41 100644 --- a/tests/unit/reactive/test_runner_manager.py +++ b/tests/unit/reactive/test_runner_manager.py @@ -16,6 +16,7 @@ ReactiveRunnerError, reconcile, ) +from github_runner_manager.reactive.types_ import RunnerConfig, QueueConfig from github_runner_manager.utilities import secure_run_subprocess EXAMPLE_MQ_URI = "http://example.com" @@ -63,8 +64,18 @@ def subprocess_popen_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: return subprocess_popen_mock +@pytest.fixture(name="runner_config") +def runner_config_fixture() -> RunnerConfig: + """Return a RunnerConfig object.""" + queue_name = secrets.token_hex(16) + + # we use construct to avoid pydantic validation as IN_MEMORY_URI is not a valid URL + queue_config = QueueConfig.construct(mongodb_uri=EXAMPLE_MQ_URI, queue_name=queue_name) + return RunnerConfig.construct(queue=queue_config) + def test_reconcile_spawns_runners( - secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, log_dir: Path + secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, log_dir: Path, + runner_config: RunnerConfig ): """ arrange: Mock that two reactive runner processes are active. @@ -74,7 +85,7 @@ def test_reconcile_spawns_runners( queue_name = secrets.token_hex(16) _arrange_reactive_processes(secure_run_subprocess_mock, count=2) - delta = reconcile(5, mq_uri=EXAMPLE_MQ_URI, queue_name=queue_name) + delta = reconcile(5, reactive_config=runner_config) assert delta == 3 assert subprocess_popen_mock.call_count == 3 @@ -82,17 +93,17 @@ def test_reconcile_spawns_runners( def test_reconcile_does_not_spawn_runners( - secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock + secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, + runner_config: RunnerConfig ): """ arrange: Mock that two reactive runner processes are active. act: Call reconcile with a quantity of 2. assert: No runners are spawned. """ - queue_name = secrets.token_hex(16) _arrange_reactive_processes(secure_run_subprocess_mock, count=2) - delta = reconcile(2, mq_uri=EXAMPLE_MQ_URI, queue_name=queue_name) + delta = reconcile(2, reactive_config=runner_config) assert delta == 0 assert subprocess_popen_mock.call_count == 0 @@ -102,15 +113,15 @@ def test_reconcile_kills_processes_for_too_many_processes( secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, os_kill_mock: MagicMock, + runner_config: RunnerConfig ): """ arrange: Mock that 3 reactive runner processes are active. act: Call reconcile with a quantity of 1. assert: 2 processes are killed. """ - queue_name = secrets.token_hex(16) _arrange_reactive_processes(secure_run_subprocess_mock, count=3) - delta = reconcile(1, mq_uri=EXAMPLE_MQ_URI, queue_name=queue_name) + delta = reconcile(1, reactive_config=runner_config) assert delta == -2 assert subprocess_popen_mock.call_count == 0 @@ -121,16 +132,16 @@ def test_reconcile_ignore_process_not_found_on_kill( secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, os_kill_mock: MagicMock, + runner_config: RunnerConfig ): """ arrange: Mock 3 reactive processes and os.kill to fail once with a ProcessLookupError. act: Call reconcile with a quantity of 1. assert: The returned delta is still -2. """ - queue_name = secrets.token_hex(16) _arrange_reactive_processes(secure_run_subprocess_mock, count=3) os_kill_mock.side_effect = [None, ProcessLookupError] - delta = reconcile(1, mq_uri=EXAMPLE_MQ_URI, queue_name=queue_name) + delta = reconcile(1, reactive_config=runner_config) assert delta == -2 assert subprocess_popen_mock.call_count == 0 @@ -138,7 +149,7 @@ def test_reconcile_ignore_process_not_found_on_kill( def test_reconcile_raises_reactive_runner_error_on_ps_failure( - secure_run_subprocess_mock: MagicMock, + secure_run_subprocess_mock: MagicMock, runner_config: RunnerConfig ): """ arrange: Mock that the ps command fails. @@ -154,7 +165,7 @@ def test_reconcile_raises_reactive_runner_error_on_ps_failure( ) with pytest.raises(ReactiveRunnerError) as err: - reconcile(1, mq_uri=EXAMPLE_MQ_URI, queue_name=queue_name) + reconcile(1, reactive_config=runner_config) assert "Failed to get list of processes" in str(err.value) diff --git a/tests/unit/test_openstack_runner_manager.py b/tests/unit/test_openstack_runner_manager.py index 9550dc1..02b8cf3 100644 --- a/tests/unit/test_openstack_runner_manager.py +++ b/tests/unit/test_openstack_runner_manager.py @@ -16,7 +16,7 @@ @pytest.fixture(scope="function", name="mock_openstack_runner_manager") def mock_openstack_runner_manager_fixture(): """The mocked OpenStackRunnerManager instance.""" - return openstack_runner_manager.OpenStackRunnerManager( + config = openstack_runner_manager.OpenStackRunnerManagerConfig( manager_name="mock-manager", prefix="mock-manager", cloud_config=openstack_runner_manager.OpenStackCloudConfig( @@ -47,8 +47,9 @@ def mock_openstack_runner_manager_fixture(): dockerhub_mirror=None, ssh_debug_connections=None, repo_policy_compliance=None, - ), + ) ) + return openstack_runner_manager.OpenStackRunnerManager(config=config) @pytest.mark.parametrize( diff --git a/tests/unit/test_runner_scaler.py b/tests/unit/test_runner_scaler.py index 1ebaffc..d480ce2 100644 --- a/tests/unit/test_runner_scaler.py +++ b/tests/unit/test_runner_scaler.py @@ -83,8 +83,8 @@ def runner_manager_fixture( "github_runner_manager.manager.runner_manager.runner_metrics.issue_events", MagicMock() ) - config = RunnerManagerConfig("mock_token", github_path) - runner_manager = RunnerManager("mock_runners", mock_cloud, config) + config = RunnerManagerConfig("mock_runners", "mock_token", github_path) + runner_manager = RunnerManager(mock_cloud, config) runner_manager._github = mock_github return runner_manager From 5e4218b9053f982b55ff7c25d89e2a97073d060e Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 16 Sep 2024 14:35:29 +0200 Subject: [PATCH 07/18] implement spawning a runner --- src-docs/github_client.md | 51 ++++++++++--- src-docs/reactive.consumer.md | 33 ++++++-- src-docs/reactive.runner.md | 4 +- src-docs/reactive.types_.md | 1 + src/github_runner_manager/github_client.py | 32 +++++++- src/github_runner_manager/metrics/github.py | 2 +- .../reactive/consumer.py | 76 ++++++++++++++++++- src/github_runner_manager/reactive/runner.py | 4 +- src/github_runner_manager/reactive/types_.py | 2 + tests/unit/metrics/test_github.py | 4 +- tests/unit/reactive/test_consumer.py | 75 ++++++++++++++++-- tests/unit/reactive/test_runner_manager.py | 20 ++--- tests/unit/test_github_client.py | 10 +-- tests/unit/test_openstack_runner_manager.py | 2 +- 14 files changed, 265 insertions(+), 51 deletions(-) diff --git a/src-docs/github_client.md b/src-docs/github_client.md index b452914..2b135de 100644 --- a/src-docs/github_client.md +++ b/src-docs/github_client.md @@ -10,7 +10,7 @@ Migrate to PyGithub in the future. PyGithub is still lacking some API such as re --- - + ## function `catch_http_errors` @@ -36,12 +36,12 @@ Catch HTTP errors and raise custom exceptions. --- - + ## class `GithubClient` GitHub API client. - + ### method `__init__` @@ -62,7 +62,7 @@ Instantiate the GiHub API client. --- - + ### method `delete_runner` @@ -81,19 +81,48 @@ Delete the self-hosted runner from GitHub. --- - + -### method `get_job_info` +### method `get` ```python -get_job_info( +get(url: HttpUrl) → dict | list | str | int | float | bool | None +``` + +Make a GET call to the GitHub API. + + + +**Args:** + + - `url`: The URL to call. + + + +**Raises:** + + - `ValueError`: If the URL is not a GitHub API URL. + + + +**Returns:** + The JSON response from the API. + +--- + + + +### method `get_job_stats` + +```python +get_job_stats( path: GitHubRepo, workflow_run_id: str, runner_name: str ) → JobStats ``` -Get information about a job for a specific workflow run. +Get information about a job for a specific workflow run identified by the runner name. @@ -117,7 +146,7 @@ Get information about a job for a specific workflow run. --- - + ### method `get_runner_github_info` @@ -140,7 +169,7 @@ Get runner information on GitHub under a repo or org. --- - + ### method `get_runner_registration_token` @@ -163,7 +192,7 @@ Get token from GitHub used for registering runners. --- - + ### method `get_runner_remove_token` diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index d7fe26f..3978e4f 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -8,12 +8,16 @@ Module responsible for consuming jobs from the message queue. --- - + ## function `consume` ```python -consume(queue_config: QueueConfig, runner_manager: RunnerManager) → None +consume( + queue_config: QueueConfig, + runner_manager: RunnerManager, + github_client: GithubClient +) → None ``` Consume a job from the message queue. @@ -26,6 +30,7 @@ Log the job details and acknowledge the message. If the job details are invalid, - `queue_config`: The configuration for the message queue. - `runner_manager`: The runner manager used to create the runner. + - `github_client`: The GitHub client to use to check the job status. @@ -36,7 +41,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` @@ -57,7 +62,25 @@ The signal handler exits the process. --- - + + +## class `JobPickedUpStates` +The states of a job that indicate it has been picked up. + + + +**Attributes:** + + - `COMPLETED`: The job has completed. + - `IN_PROGRESS`: The job is in progress. + + + + + +--- + + ## class `JobDetails` A class to translate the payload. @@ -75,7 +98,7 @@ A class to translate the payload. --- - + ## class `JobError` Raised when a job error occurs. diff --git a/src-docs/reactive.runner.md b/src-docs/reactive.runner.md index a9afd69..266c19d 100644 --- a/src-docs/reactive.runner.md +++ b/src-docs/reactive.runner.md @@ -11,7 +11,7 @@ Script to spawn a reactive runner process. --- - + ## function `setup_root_logging` @@ -24,7 +24,7 @@ Set up logging for the reactive runner process. --- - + ## function `main` diff --git a/src-docs/reactive.types_.md b/src-docs/reactive.types_.md index 039de3a..18b7ad8 100644 --- a/src-docs/reactive.types_.md +++ b/src-docs/reactive.types_.md @@ -39,6 +39,7 @@ The configuration for the reactive runner to spawn. - `queue`: The queue configuration. - `runner_manager`: The runner manager configuration. - `cloud_runner_manager`: The OpenStack runner manager configuration. + - `github_token`: str diff --git a/src/github_runner_manager/github_client.py b/src/github_runner_manager/github_client.py index a888724..4c77212 100644 --- a/src/github_runner_manager/github_client.py +++ b/src/github_runner_manager/github_client.py @@ -7,13 +7,16 @@ remove token for runner. """ import functools +import json import logging +import urllib.request from datetime import datetime from typing import Callable, ParamSpec, TypeVar from urllib.error import HTTPError from ghapi.all import GhApi, pages from ghapi.page import paged +from pydantic import HttpUrl from typing_extensions import assert_never from github_runner_manager.errors import GithubApiError, JobNotFoundError, TokenError @@ -34,6 +37,8 @@ # Return type of the function decorated with retry ReturnT = TypeVar("ReturnT") +Json = dict | list | str | int | float | bool | None + def catch_http_errors(func: Callable[ParamT, ReturnT]) -> Callable[ParamT, ReturnT]: """Catch HTTP errors and raise custom exceptions. @@ -200,8 +205,8 @@ def delete_runner(self, path: GitHubPath, runner_id: int) -> None: runner_id=runner_id, ) - def get_job_info(self, path: GitHubRepo, workflow_run_id: str, runner_name: str) -> JobStats: - """Get information about a job for a specific workflow run. + def get_job_stats(self, path: GitHubRepo, workflow_run_id: str, runner_name: str) -> JobStats: + """Get information about a job for a specific workflow run identified by the runner name. Args: path: GitHub repository path in the format '/'. @@ -258,3 +263,26 @@ def get_job_info(self, path: GitHubRepo, workflow_run_id: str, runner_name: str) ) from exc raise JobNotFoundError(f"Could not find job for runner {runner_name}.") + + @catch_http_errors + def get(self, url: HttpUrl) -> Json: + """Make a GET call to the GitHub API. + + Args: + url: The URL to call. + + Raises: + ValueError: If the URL is not a GitHub API URL. + + Returns: + The JSON response from the API. + """ + if not url.startswith("https://api.github.com"): + raise ValueError("Only GitHub API URLs are allowed.") + # use urllib to make an authenticated requests using the github token + request = urllib.request.Request(url, headers={"Authorization": f"token {self._token}"}) + request.add_header("Accept", "application/vnd.github+json") + request.add_header("X-GitHub-Api-Version", "2022-11-28") + # we check that the url is a valid github api url + with urllib.request.urlopen(request) as response: # nosec + return json.loads(response.read()) diff --git a/src/github_runner_manager/metrics/github.py b/src/github_runner_manager/metrics/github.py index 19b4b56..0c4c8c5 100644 --- a/src/github_runner_manager/metrics/github.py +++ b/src/github_runner_manager/metrics/github.py @@ -34,7 +34,7 @@ def job( owner, repo = pre_job_metrics.repository.split("/", maxsplit=1) try: - job_info = github_client.get_job_info( + job_info = github_client.get_job_stats( path=GitHubRepo(owner=owner, repo=repo), workflow_run_id=pre_job_metrics.workflow_run_id, runner_name=runner_name, diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index 7723fa9..295ca6c 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -7,19 +7,34 @@ import signal import sys from contextlib import closing +from enum import Enum +from time import sleep from types import FrameType from typing import Generator, cast -from kombu import Connection +from kombu import Connection, Message from kombu.simple import SimpleQueue from pydantic import BaseModel, HttpUrl, ValidationError +from github_runner_manager.github_client import GithubClient from github_runner_manager.manager.runner_manager import RunnerManager from github_runner_manager.reactive.types_ import QueueConfig logger = logging.getLogger(__name__) +class JobPickedUpStates(str, Enum): + """The states of a job that indicate it has been picked up. + + Attributes: + COMPLETED: The job has completed. + IN_PROGRESS: The job is in progress. + """ + + COMPLETED = "completed" + IN_PROGRESS = "in_progress" + + class JobDetails(BaseModel): """A class to translate the payload. @@ -36,7 +51,9 @@ class JobError(Exception): """Raised when a job error occurs.""" -def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: +def consume( + queue_config: QueueConfig, runner_manager: RunnerManager, github_client: GithubClient +) -> None: """Consume a job from the message queue. Log the job details and acknowledge the message. @@ -45,6 +62,7 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: Args: queue_config: The configuration for the message queue. runner_manager: The runner manager used to create the runner. + github_client: The GitHub client to use to check the job status. Raises: JobError: If the job details are invalid. @@ -55,7 +73,6 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: msg = simple_queue.get(block=True) try: job_details = cast(JobDetails, JobDetails.parse_raw(msg.payload)) - #runner_manager.create_runners(1) except ValidationError as exc: msg.reject(requeue=True) raise JobError(f"Invalid job details: {msg.payload}") from exc @@ -64,7 +81,58 @@ def consume(queue_config: QueueConfig, runner_manager: RunnerManager) -> None: job_details.labels, job_details.run_url, ) - msg.ack() + _spawn_runner( + runner_manager=runner_manager, + job_url=job_details.run_url, + msg=msg, + github_client=github_client, + ) + + +def _spawn_runner( + runner_manager: RunnerManager, job_url: HttpUrl, msg: Message, github_client: GithubClient +) -> None: + """Spawn a runner. + + A runner is only spawned if the job has not yet been picked up by a runner. + After spawning a runner, it is checked if the job has been picked up. + + If the job has been picked up, the message is acknowledged. + If the job has not been picked up after 5 minutes, the message is rejected and requeued. + + Args: + runner_manager: The runner manager to use. + job_url: The URL of the job. + msg: The message to acknowledge or reject. + github_client: The GitHub client to use to check the job status. + """ + if _check_job_been_picked_up(job_url=job_url, github_client=github_client): + msg.ack() + return + runner_manager.create_runners(1) + for _ in range(10): + if _check_job_been_picked_up(job_url=job_url, github_client=github_client): + msg.ack() + break + sleep(30) + else: + msg.reject(requeue=True) + + +def _check_job_been_picked_up(job_url: HttpUrl, github_client: GithubClient) -> bool: + """Check if the job has already been picked up. + + Args: + job_url: The URL of the job. + github_client: The GitHub client to use to check the job status. + + Returns: + True if the job has been picked up, False otherwise. + """ + # See response format: + # https://docs.github.com/en/rest/actions/workflow-jobs?apiVersion=2022-11-28#get-a-job-for-a-workflow-run + response = github_client.get(job_url) + return response["status"] in [*JobPickedUpStates] @contextlib.contextmanager diff --git a/src/github_runner_manager/reactive/runner.py b/src/github_runner_manager/reactive/runner.py index 5042fde..9358f28 100644 --- a/src/github_runner_manager/reactive/runner.py +++ b/src/github_runner_manager/reactive/runner.py @@ -7,6 +7,7 @@ import os import sys +from github_runner_manager.github_client import GithubClient from github_runner_manager.manager.runner_manager import RunnerManager from github_runner_manager.openstack_cloud.openstack_runner_manager import OpenStackRunnerManager from github_runner_manager.reactive.consumer import consume @@ -47,7 +48,8 @@ def main() -> None: cloud_runner_manager=openstack_runner_manager, config=runner_config.runner_manager, ) - consume(queue_config=queue_config, runner_manager=runner_manager) + github_client = GithubClient(token=runner_config.github_token) + consume(queue_config=queue_config, runner_manager=runner_manager, github_client=github_client) if __name__ == "__main__": diff --git a/src/github_runner_manager/reactive/types_.py b/src/github_runner_manager/reactive/types_.py index aac315f..47cb18b 100644 --- a/src/github_runner_manager/reactive/types_.py +++ b/src/github_runner_manager/reactive/types_.py @@ -30,8 +30,10 @@ class RunnerConfig(BaseModel): queue: The queue configuration. runner_manager: The runner manager configuration. cloud_runner_manager: The OpenStack runner manager configuration. + github_token: str """ queue: QueueConfig runner_manager: RunnerManagerConfig cloud_runner_manager: OpenStackRunnerManagerConfig + github_token: str diff --git a/tests/unit/metrics/test_github.py b/tests/unit/metrics/test_github.py index 7d13495..16ff74c 100644 --- a/tests/unit/metrics/test_github.py +++ b/tests/unit/metrics/test_github.py @@ -38,7 +38,7 @@ def test_job(pre_job_metrics: PreJobMetrics): runner_name = secrets.token_hex(16) created_at = datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc) started_at = created_at + timedelta(seconds=3600) - github_client.get_job_info.return_value = JobStats( + github_client.get_job_stats.return_value = JobStats( created_at=created_at, started_at=started_at, runner_name=runner_name, @@ -62,7 +62,7 @@ def test_job_job_not_found(pre_job_metrics: PreJobMetrics): """ github_client = MagicMock(spec=GithubClient) runner_name = secrets.token_hex(16) - github_client.get_job_info.side_effect = JobNotFoundError("Job not found") + github_client.get_job_stats.side_effect = JobNotFoundError("Job not found") with pytest.raises(GithubMetricsError): github_metrics.job( diff --git a/tests/unit/reactive/test_consumer.py b/tests/unit/reactive/test_consumer.py index a39630a..e7612e3 100644 --- a/tests/unit/reactive/test_consumer.py +++ b/tests/unit/reactive/test_consumer.py @@ -3,6 +3,7 @@ import secrets from contextlib import closing +from queue import Empty from unittest.mock import MagicMock import pytest @@ -24,11 +25,18 @@ def queue_config_fixture() -> QueueConfig: # we use construct to avoid pydantic validation as IN_MEMORY_URI is not a valid URL return QueueConfig.construct(mongodb_uri=IN_MEMORY_URI, queue_name=queue_name) -def test_consume(caplog: pytest.LogCaptureFixture, queue_config: QueueConfig): + +@pytest.fixture(name="mock_sleep", autouse=True) +def mock_sleep_fixture(monkeypatch: pytest.MonkeyPatch) -> None: + """Mock the sleep function.""" + monkeypatch.setattr(consumer, "sleep", lambda _: None) + + +def test_consume(monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig): """ - arrange: A job placed in the message queue. - act: Call consume - assert: The job is logged. + arrange: A job placed in the message queue which has not yet been picked up. + act: Call consume. + assert: A runner is created and the message is acknowledged. """ job_details = consumer.JobDetails( labels=[secrets.token_hex(16), secrets.token_hex(16)], @@ -36,10 +44,53 @@ def test_consume(caplog: pytest.LogCaptureFixture, queue_config: QueueConfig): ) _put_in_queue(job_details.json(), queue_config.queue_name) + runner_manager_mock = MagicMock(spec=consumer.RunnerManager) + github_client_mock = MagicMock(spec=consumer.GithubClient) + github_client_mock.get.side_effect = [ + {"status": "queued"}, + {"status": consumer.JobPickedUpStates.IN_PROGRESS}, + ] + + consumer.consume( + queue_config=queue_config, + runner_manager=runner_manager_mock, + github_client=github_client_mock, + ) + + runner_manager_mock.create_runners.assert_called_once_with(1) - consumer.consume(queue_config=queue_config, runner_manager=MagicMock()) - assert str(job_details.labels) in caplog.text - assert job_details.run_url in caplog.text + # Ensure message has been acknowledged by assuming an Empty exception is raised + with pytest.raises(Empty): + _consume_from_queue(queue_config.queue_name) + + +def test_consume_reject_if_job_gets_not_picked_up( + monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig +): + """ + arrange: A job placed in the message queue which will not get picked up. + act: Call consume. + assert: The message is requeued. + """ + job_details = consumer.JobDetails( + labels=[secrets.token_hex(16), secrets.token_hex(16)], + run_url=FAKE_RUN_URL, + ) + _put_in_queue(job_details.json(), queue_config.queue_name) + + runner_manager_mock = MagicMock(spec=consumer.RunnerManager) + github_client_mock = MagicMock(spec=consumer.GithubClient) + github_client_mock.get.return_value = {"status": "queued"} + + consumer.consume( + queue_config=queue_config, + runner_manager=runner_manager_mock, + github_client=github_client_mock, + ) + + # Ensure message has been requeued by reconsuming it + msg = _consume_from_queue(queue_config.queue_name) + assert msg.payload == job_details.json() @pytest.mark.parametrize( @@ -63,8 +114,16 @@ def test_job_details_validation_error(job_str: str, queue_config: QueueConfig): queue_name = queue_config.queue_name _put_in_queue(job_str, queue_name) + runner_manager_mock = MagicMock(spec=consumer.RunnerManager) + github_client_mock = MagicMock(spec=consumer.GithubClient) + github_client_mock.get.return_value = {"status": consumer.JobPickedUpStates.IN_PROGRESS} + with pytest.raises(JobError) as exc_info: - consumer.consume(queue_config=queue_config, runner_manager=MagicMock()) + consumer.consume( + queue_config=queue_config, + runner_manager=runner_manager_mock, + github_client=github_client_mock, + ) assert "Invalid job details" in str(exc_info.value) # Ensure message has been requeued by reconsuming it diff --git a/tests/unit/reactive/test_runner_manager.py b/tests/unit/reactive/test_runner_manager.py index 04adc41..8d88c95 100644 --- a/tests/unit/reactive/test_runner_manager.py +++ b/tests/unit/reactive/test_runner_manager.py @@ -16,7 +16,7 @@ ReactiveRunnerError, reconcile, ) -from github_runner_manager.reactive.types_ import RunnerConfig, QueueConfig +from github_runner_manager.reactive.types_ import QueueConfig, RunnerConfig from github_runner_manager.utilities import secure_run_subprocess EXAMPLE_MQ_URI = "http://example.com" @@ -73,16 +73,18 @@ def runner_config_fixture() -> RunnerConfig: queue_config = QueueConfig.construct(mongodb_uri=EXAMPLE_MQ_URI, queue_name=queue_name) return RunnerConfig.construct(queue=queue_config) + def test_reconcile_spawns_runners( - secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, log_dir: Path, - runner_config: RunnerConfig + secure_run_subprocess_mock: MagicMock, + subprocess_popen_mock: MagicMock, + log_dir: Path, + runner_config: RunnerConfig, ): """ arrange: Mock that two reactive runner processes are active. act: Call reconcile with a quantity of 5. assert: Three runners are spawned. Log file is setup. """ - queue_name = secrets.token_hex(16) _arrange_reactive_processes(secure_run_subprocess_mock, count=2) delta = reconcile(5, reactive_config=runner_config) @@ -93,8 +95,9 @@ def test_reconcile_spawns_runners( def test_reconcile_does_not_spawn_runners( - secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, - runner_config: RunnerConfig + secure_run_subprocess_mock: MagicMock, + subprocess_popen_mock: MagicMock, + runner_config: RunnerConfig, ): """ arrange: Mock that two reactive runner processes are active. @@ -113,7 +116,7 @@ def test_reconcile_kills_processes_for_too_many_processes( secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, os_kill_mock: MagicMock, - runner_config: RunnerConfig + runner_config: RunnerConfig, ): """ arrange: Mock that 3 reactive runner processes are active. @@ -132,7 +135,7 @@ def test_reconcile_ignore_process_not_found_on_kill( secure_run_subprocess_mock: MagicMock, subprocess_popen_mock: MagicMock, os_kill_mock: MagicMock, - runner_config: RunnerConfig + runner_config: RunnerConfig, ): """ arrange: Mock 3 reactive processes and os.kill to fail once with a ProcessLookupError. @@ -156,7 +159,6 @@ def test_reconcile_raises_reactive_runner_error_on_ps_failure( act: Call reconcile with a quantity of 1. assert: A ReactiveRunnerError is raised. """ - queue_name = secrets.token_hex(16) secure_run_subprocess_mock.return_value = CompletedProcess( args=PIDS_COMMAND_LINE, returncode=1, diff --git a/tests/unit/test_github_client.py b/tests/unit/test_github_client.py index 2cea912..7e52aa8 100644 --- a/tests/unit/test_github_client.py +++ b/tests/unit/test_github_client.py @@ -95,7 +95,7 @@ def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawDat assert: The correct JobStats object is returned. """ github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_info( + job_stats = github_client.get_job_stats( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, @@ -128,7 +128,7 @@ def test_get_job_info_no_conclusion(github_client: GithubClient, job_stats_raw: ] } github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_info( + job_stats = github_client.get_job_stats( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, @@ -156,7 +156,7 @@ def test_github_api_pagination_multiple_pages( ) github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_info( + job_stats = github_client.get_job_stats( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, @@ -186,7 +186,7 @@ def test_github_api_pagination_job_not_found( github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) with pytest.raises(JobNotFoundError): - github_client.get_job_info( + github_client.get_job_stats( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, @@ -200,7 +200,7 @@ def test_github_api_http_error(github_client: GithubClient, job_stats_raw: JobSt github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) with pytest.raises(JobNotFoundError): - github_client.get_job_info( + github_client.get_job_stats( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, diff --git a/tests/unit/test_openstack_runner_manager.py b/tests/unit/test_openstack_runner_manager.py index 02b8cf3..a5809e7 100644 --- a/tests/unit/test_openstack_runner_manager.py +++ b/tests/unit/test_openstack_runner_manager.py @@ -47,7 +47,7 @@ def mock_openstack_runner_manager_fixture(): dockerhub_mirror=None, ssh_debug_connections=None, repo_policy_compliance=None, - ) + ), ) return openstack_runner_manager.OpenStackRunnerManager(config=config) From b879f4a339058129f02f86df6d27a1425d8237c2 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 16 Sep 2024 14:44:20 +0200 Subject: [PATCH 08/18] lint --- src-docs/reactive.runner_manager.md | 2 +- src/github_runner_manager/reactive/runner_manager.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src-docs/reactive.runner_manager.md b/src-docs/reactive.runner_manager.md index ffb8b00..4c5f696 100644 --- a/src-docs/reactive.runner_manager.md +++ b/src-docs/reactive.runner_manager.md @@ -17,7 +17,7 @@ Module for managing reactive runners. --- - + ## function `reconcile` diff --git a/src/github_runner_manager/reactive/runner_manager.py b/src/github_runner_manager/reactive/runner_manager.py index ea895e0..f214cef 100644 --- a/src/github_runner_manager/reactive/runner_manager.py +++ b/src/github_runner_manager/reactive/runner_manager.py @@ -36,6 +36,7 @@ class ReactiveRunnerError(Exception): """Raised when a reactive runner error occurs.""" + # TODO: rename reactive_config argument to runner_config def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: """Spawn a runner reactively. From 821bfb482e9ded7013dbb3b21fe8ed9c971675c8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 16 Sep 2024 15:14:51 +0200 Subject: [PATCH 09/18] Add unit tests for github_client --- pyproject.toml | 2 +- src-docs/errors.md | 17 +++++++-- src-docs/github_client.md | 20 +++++------ src/github_runner_manager/errors.py | 4 +++ src/github_runner_manager/github_client.py | 13 ++++--- tests/unit/test_github_client.py | 40 ++++++++++++++++++++-- 6 files changed, 76 insertions(+), 20 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index da93d53..7f89083 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -46,7 +46,7 @@ omit = [ ] [tool.coverage.report] -fail_under = 79 +fail_under = 80 show_missing = true diff --git a/src-docs/errors.md b/src-docs/errors.md index c15e359..1256246 100644 --- a/src-docs/errors.md +++ b/src-docs/errors.md @@ -198,6 +198,17 @@ Represents an error when the job could not be found on GitHub. +## class `WrongUrlError` +Represents an error when the URL is invalid. + + + + + +--- + + + ## class `OpenStackError` Base class for OpenStack errors. @@ -207,7 +218,7 @@ Base class for OpenStack errors. --- - + ## class `OpenStackInvalidConfigError` Represents an invalid OpenStack configuration. @@ -218,7 +229,7 @@ Represents an invalid OpenStack configuration. --- - + ## class `SSHError` Represents an error while interacting with SSH. @@ -229,7 +240,7 @@ Represents an error while interacting with SSH. --- - + ## class `KeyfileError` Represents missing keyfile for SSH. diff --git a/src-docs/github_client.md b/src-docs/github_client.md index 2b135de..764867f 100644 --- a/src-docs/github_client.md +++ b/src-docs/github_client.md @@ -10,7 +10,7 @@ Migrate to PyGithub in the future. PyGithub is still lacking some API such as re --- - + ## function `catch_http_errors` @@ -36,12 +36,12 @@ Catch HTTP errors and raise custom exceptions. --- - + ## class `GithubClient` GitHub API client. - + ### method `__init__` @@ -62,7 +62,7 @@ Instantiate the GiHub API client. --- - + ### method `delete_runner` @@ -81,7 +81,7 @@ Delete the self-hosted runner from GitHub. --- - + ### method `get` @@ -101,7 +101,7 @@ Make a GET call to the GitHub API. **Raises:** - - `ValueError`: If the URL is not a GitHub API URL. + - `WrongUrlError`: If the URL is not a GitHub API URL. @@ -110,7 +110,7 @@ Make a GET call to the GitHub API. --- - + ### method `get_job_stats` @@ -146,7 +146,7 @@ Get information about a job for a specific workflow run identified by the runner --- - + ### method `get_runner_github_info` @@ -169,7 +169,7 @@ Get runner information on GitHub under a repo or org. --- - + ### method `get_runner_registration_token` @@ -192,7 +192,7 @@ Get token from GitHub used for registering runners. --- - + ### method `get_runner_remove_token` diff --git a/src/github_runner_manager/errors.py b/src/github_runner_manager/errors.py index a91555b..abbc45e 100644 --- a/src/github_runner_manager/errors.py +++ b/src/github_runner_manager/errors.py @@ -73,6 +73,10 @@ class JobNotFoundError(GithubClientError): """Represents an error when the job could not be found on GitHub.""" +class WrongUrlError(GithubClientError): + """Represents an error when the URL is invalid.""" + + class OpenStackError(Exception): """Base class for OpenStack errors.""" diff --git a/src/github_runner_manager/github_client.py b/src/github_runner_manager/github_client.py index 4c77212..bc258b5 100644 --- a/src/github_runner_manager/github_client.py +++ b/src/github_runner_manager/github_client.py @@ -19,7 +19,12 @@ from pydantic import HttpUrl from typing_extensions import assert_never -from github_runner_manager.errors import GithubApiError, JobNotFoundError, TokenError +from github_runner_manager.errors import ( + GithubApiError, + JobNotFoundError, + TokenError, + WrongUrlError, +) from github_runner_manager.types_.github import ( GitHubOrg, GitHubPath, @@ -272,17 +277,17 @@ def get(self, url: HttpUrl) -> Json: url: The URL to call. Raises: - ValueError: If the URL is not a GitHub API URL. + WrongUrlError: If the URL is not a GitHub API URL. Returns: The JSON response from the API. """ if not url.startswith("https://api.github.com"): - raise ValueError("Only GitHub API URLs are allowed.") + raise WrongUrlError("Only GitHub API URLs are allowed.") # use urllib to make an authenticated requests using the github token request = urllib.request.Request(url, headers={"Authorization": f"token {self._token}"}) request.add_header("Accept", "application/vnd.github+json") request.add_header("X-GitHub-Api-Version", "2022-11-28") - # we check that the url is a valid github api url + # We have checked that the url is a valid github api url, so we can safely call the API. with urllib.request.urlopen(request) as response: # nosec return json.loads(response.read()) diff --git a/tests/unit/test_github_client.py b/tests/unit/test_github_client.py index 7e52aa8..4556aa0 100644 --- a/tests/unit/test_github_client.py +++ b/tests/unit/test_github_client.py @@ -1,6 +1,7 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. import http +import json import random import secrets from collections import namedtuple @@ -10,7 +11,7 @@ import pytest -from github_runner_manager.errors import JobNotFoundError +from github_runner_manager.errors import JobNotFoundError, WrongUrlError from github_runner_manager.github_client import GithubClient from github_runner_manager.types_.github import GitHubRepo, JobConclusion, JobStats @@ -19,6 +20,8 @@ ["created_at", "started_at", "runner_name", "conclusion", "id"], ) +TEST_URLLIB_RESPONSE_JSON = {"test": "test"} + @pytest.fixture(name="job_stats_raw") def job_stats_fixture() -> JobStatsRawData: @@ -33,8 +36,18 @@ def job_stats_fixture() -> JobStatsRawData: ) +@pytest.fixture(name="urllib_urlopen_mock") +def urllib_open_mock_fixture(monkeypatch: pytest.MonkeyPatch) -> MagicMock: + """Mock the urllib.request.urlopen function.""" + urllib_open_mock = MagicMock() + monkeypatch.setattr("urllib.request.urlopen", urllib_open_mock) + return urllib_open_mock + + @pytest.fixture(name="github_client") -def github_client_fixture(job_stats_raw: JobStatsRawData) -> GithubClient: +def github_client_fixture( + job_stats_raw: JobStatsRawData, urllib_urlopen_mock: MagicMock +) -> GithubClient: """Create a GithubClient object with a mocked GhApi object.""" gh_client = GithubClient("token") gh_client._client = MagicMock() @@ -49,6 +62,9 @@ def github_client_fixture(job_stats_raw: JobStatsRawData) -> GithubClient: } ] } + urllib_urlopen_mock.return_value.__enter__.return_value.read.return_value = json.dumps( + TEST_URLLIB_RESPONSE_JSON + ).encode("utf-8") return gh_client @@ -205,3 +221,23 @@ def test_github_api_http_error(github_client: GithubClient, job_stats_raw: JobSt workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, ) + + +def test_github_client_get(github_client: GithubClient): + """ + arrange: A mocked Github Client that returns a response. + act: Call get. + assert: The response is returned. + """ + response = github_client.get("https://api.github.com/test") + assert response == TEST_URLLIB_RESPONSE_JSON + + +def test_github_client_get_wrong_url(github_client: GithubClient): + """ + arrange: A mocked Github Client that returns a response. + act: Call get with a wrong url, not starting with https://api.github.com. + assert: An exception is raised. + """ + with pytest.raises(WrongUrlError): + github_client.get("https://example.com/test") From cab8ee50bd3e8351cc4062b71755081f5068dd6f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 17 Sep 2024 10:18:08 +0200 Subject: [PATCH 10/18] add cleanup --- src/github_runner_manager/manager/runner_scaler.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/github_runner_manager/manager/runner_scaler.py b/src/github_runner_manager/manager/runner_scaler.py index 35ffdf9..95531e6 100644 --- a/src/github_runner_manager/manager/runner_scaler.py +++ b/src/github_runner_manager/manager/runner_scaler.py @@ -260,6 +260,7 @@ def _reconcile_reactive(self, quantity: int) -> int: this number is never negative as additional processes should terminate after a timeout. """ logger.info("Reactive mode is experimental and not yet fully implemented.") + self._manager.cleanup() return reactive_runner_manager.reconcile( quantity=quantity, reactive_config=self._reactive_config, From 63077e81c5d62a70f928c364b3816c43f35e341b Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 17 Sep 2024 13:35:50 +0200 Subject: [PATCH 11/18] write get_job_info method --- src-docs/errors.md | 17 +--- src-docs/github_client.md | 41 ++++---- src-docs/reactive.consumer.md | 10 +- src-docs/types_.github.md | 35 +++++-- src/github_runner_manager/errors.py | 4 - src/github_runner_manager/github_client.py | 97 +++++++++---------- src/github_runner_manager/metrics/github.py | 2 +- .../reactive/consumer.py | 14 ++- src/github_runner_manager/types_/github.py | 24 ++++- tests/unit/metrics/test_github.py | 8 +- tests/unit/reactive/test_consumer.py | 31 +++++- tests/unit/test_github_client.py | 85 +++++++++------- 12 files changed, 218 insertions(+), 150 deletions(-) diff --git a/src-docs/errors.md b/src-docs/errors.md index 1256246..c15e359 100644 --- a/src-docs/errors.md +++ b/src-docs/errors.md @@ -198,17 +198,6 @@ Represents an error when the job could not be found on GitHub. -## class `WrongUrlError` -Represents an error when the URL is invalid. - - - - - ---- - - - ## class `OpenStackError` Base class for OpenStack errors. @@ -218,7 +207,7 @@ Base class for OpenStack errors. --- - + ## class `OpenStackInvalidConfigError` Represents an invalid OpenStack configuration. @@ -229,7 +218,7 @@ Represents an invalid OpenStack configuration. --- - + ## class `SSHError` Represents an error while interacting with SSH. @@ -240,7 +229,7 @@ Represents an error while interacting with SSH. --- - + ## class `KeyfileError` Represents missing keyfile for SSH. diff --git a/src-docs/github_client.md b/src-docs/github_client.md index 764867f..7e06d56 100644 --- a/src-docs/github_client.md +++ b/src-docs/github_client.md @@ -10,7 +10,7 @@ Migrate to PyGithub in the future. PyGithub is still lacking some API such as re --- - + ## function `catch_http_errors` @@ -36,12 +36,12 @@ Catch HTTP errors and raise custom exceptions. --- - + ## class `GithubClient` GitHub API client. - + ### method `__init__` @@ -62,7 +62,7 @@ Instantiate the GiHub API client. --- - + ### method `delete_runner` @@ -81,27 +81,22 @@ Delete the self-hosted runner from GitHub. --- - + -### method `get` +### method `get_job_info` ```python -get(url: HttpUrl) → dict | list | str | int | float | bool | None +get_job_info(path: GitHubRepo, job_id: str) → JobInfo ``` -Make a GET call to the GitHub API. +Get information about a job identified by the job id. **Args:** - - `url`: The URL to call. - - - -**Raises:** - - - `WrongUrlError`: If the URL is not a GitHub API URL. + - `path`: GitHub repository path in the format '/'. + - `job_id`: The job id. @@ -110,16 +105,16 @@ Make a GET call to the GitHub API. --- - + -### method `get_job_stats` +### method `get_job_info_by_runner_name` ```python -get_job_stats( +get_job_info_by_runner_name( path: GitHubRepo, workflow_run_id: str, runner_name: str -) → JobStats +) → JobInfo ``` Get information about a job for a specific workflow run identified by the runner name. @@ -136,7 +131,7 @@ Get information about a job for a specific workflow run identified by the runner **Raises:** - - `TokenError`: if there was an error with the Github token crdential provided. + - `TokenError`: if there was an error with the Github token credential provided. - `JobNotFoundError`: If no jobs were found. @@ -146,7 +141,7 @@ Get information about a job for a specific workflow run identified by the runner --- - + ### method `get_runner_github_info` @@ -169,7 +164,7 @@ Get runner information on GitHub under a repo or org. --- - + ### method `get_runner_registration_token` @@ -192,7 +187,7 @@ Get token from GitHub used for registering runners. --- - + ### method `get_runner_remove_token` diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index 3978e4f..64d31da 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -8,7 +8,7 @@ Module responsible for consuming jobs from the message queue. --- - + ## function `consume` @@ -41,7 +41,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` @@ -62,7 +62,7 @@ The signal handler exits the process. --- - + ## class `JobPickedUpStates` The states of a job that indicate it has been picked up. @@ -80,7 +80,7 @@ The states of a job that indicate it has been picked up. --- - + ## class `JobDetails` A class to translate the payload. @@ -98,7 +98,7 @@ A class to translate the payload. --- - + ## class `JobError` Raised when a job error occurs. diff --git a/src-docs/types_.github.md b/src-docs/types_.github.md index d410455..195a459 100644 --- a/src-docs/types_.github.md +++ b/src-docs/types_.github.md @@ -8,7 +8,7 @@ Module containing GitHub API related types. --- - + ## function `parse_github_path` @@ -201,7 +201,29 @@ See :https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2022-11-28 -## class `JobStats` +## class `JobStatus` +Status of a job on GitHub. + + + +**Attributes:** + + - `QUEUED`: Represents a job that is queued. + - `IN_PROGRESS`: Represents a job that is in progress. + - `COMPLETED`: Represents a job that is completed. + - `WAITING`: Represents a job that is waiting. + - `REQUESTED`: Represents a job that is requested. + - `PENDING`: Represents a job that is pending. + + + + + +--- + + + +## class `JobInfo` Stats for a job on GitHub. @@ -212,6 +234,7 @@ Stats for a job on GitHub. - `created_at`: The time the job was created. - `started_at`: The time the job was started. - `conclusion`: The end result of a job. + - `status`: The status of the job. @@ -219,7 +242,7 @@ Stats for a job on GitHub. --- - + ## class `GitHubRepo` Represent GitHub repository. @@ -248,7 +271,7 @@ __init__(owner: 'str', repo: 'str') → None --- - + ### method `path` @@ -266,7 +289,7 @@ Return a string representing the path. --- - + ## class `GitHubOrg` Represent GitHub organization. @@ -295,7 +318,7 @@ __init__(org: 'str', group: 'str') → None --- - + ### method `path` diff --git a/src/github_runner_manager/errors.py b/src/github_runner_manager/errors.py index abbc45e..a91555b 100644 --- a/src/github_runner_manager/errors.py +++ b/src/github_runner_manager/errors.py @@ -73,10 +73,6 @@ class JobNotFoundError(GithubClientError): """Represents an error when the job could not be found on GitHub.""" -class WrongUrlError(GithubClientError): - """Represents an error when the URL is invalid.""" - - class OpenStackError(Exception): """Base class for OpenStack errors.""" diff --git a/src/github_runner_manager/github_client.py b/src/github_runner_manager/github_client.py index bc258b5..4b4ebbe 100644 --- a/src/github_runner_manager/github_client.py +++ b/src/github_runner_manager/github_client.py @@ -7,29 +7,21 @@ remove token for runner. """ import functools -import json import logging -import urllib.request from datetime import datetime from typing import Callable, ParamSpec, TypeVar from urllib.error import HTTPError from ghapi.all import GhApi, pages from ghapi.page import paged -from pydantic import HttpUrl from typing_extensions import assert_never -from github_runner_manager.errors import ( - GithubApiError, - JobNotFoundError, - TokenError, - WrongUrlError, -) +from github_runner_manager.errors import GithubApiError, JobNotFoundError, TokenError from github_runner_manager.types_.github import ( GitHubOrg, GitHubPath, GitHubRepo, - JobStats, + JobInfo, RegistrationToken, RemoveToken, SelfHostedRunner, @@ -42,8 +34,6 @@ # Return type of the function decorated with retry ReturnT = TypeVar("ReturnT") -Json = dict | list | str | int | float | bool | None - def catch_http_errors(func: Callable[ParamT, ReturnT]) -> Callable[ParamT, ReturnT]: """Catch HTTP errors and raise custom exceptions. @@ -210,7 +200,9 @@ def delete_runner(self, path: GitHubPath, runner_id: int) -> None: runner_id=runner_id, ) - def get_job_stats(self, path: GitHubRepo, workflow_run_id: str, runner_name: str) -> JobStats: + def get_job_info_by_runner_name( + self, path: GitHubRepo, workflow_run_id: str, runner_name: str + ) -> JobInfo: """Get information about a job for a specific workflow run identified by the runner name. Args: @@ -219,7 +211,7 @@ def get_job_stats(self, path: GitHubRepo, workflow_run_id: str, runner_name: str runner_name: Name of the runner. Raises: - TokenError: if there was an error with the Github token crdential provided. + TokenError: if there was an error with the Github token credential provided. JobNotFoundError: If no jobs were found. Returns: @@ -237,27 +229,7 @@ def get_job_stats(self, path: GitHubRepo, workflow_run_id: str, runner_name: str break for job in jobs: if job["runner_name"] == runner_name: - # datetime strings should be in ISO 8601 format, - # but they can also use Z instead of - # +00:00, which is not supported by datetime.fromisoformat - created_at = datetime.fromisoformat( - job["created_at"].replace("Z", "+00:00") - ) - started_at = datetime.fromisoformat( - job["started_at"].replace("Z", "+00:00") - ) - # conclusion could be null per api schema, so we need to handle that - # though we would assume that it should always be present, - # as the job should be finished - conclusion = job.get("conclusion", None) - - job_id = job["id"] - return JobStats( - job_id=job_id, - created_at=created_at, - started_at=started_at, - conclusion=conclusion, - ) + return self._to_job_info(job) except HTTPError as exc: if exc.code in (401, 403): @@ -270,24 +242,49 @@ def get_job_stats(self, path: GitHubRepo, workflow_run_id: str, runner_name: str raise JobNotFoundError(f"Could not find job for runner {runner_name}.") @catch_http_errors - def get(self, url: HttpUrl) -> Json: - """Make a GET call to the GitHub API. + def get_job_info(self, path: GitHubRepo, job_id: str) -> JobInfo: + """Get information about a job identified by the job id. Args: - url: The URL to call. - - Raises: - WrongUrlError: If the URL is not a GitHub API URL. + path: GitHub repository path in the format '/'. + job_id: The job id. Returns: The JSON response from the API. """ - if not url.startswith("https://api.github.com"): - raise WrongUrlError("Only GitHub API URLs are allowed.") - # use urllib to make an authenticated requests using the github token - request = urllib.request.Request(url, headers={"Authorization": f"token {self._token}"}) - request.add_header("Accept", "application/vnd.github+json") - request.add_header("X-GitHub-Api-Version", "2022-11-28") - # We have checked that the url is a valid github api url, so we can safely call the API. - with urllib.request.urlopen(request) as response: # nosec - return json.loads(response.read()) + job_raw = self._client.actions.get_job_for_workflow_run( + owner=path.owner, + repo=path.repo, + job_id=job_id, + ) + return self._to_job_info(job_raw) + + @staticmethod + def _to_job_info(job: dict) -> JobInfo: + """Convert the job dict to JobInfo. + + Args: + job: The job dict. + + Returns: + The JobInfo object. + """ + # datetime strings should be in ISO 8601 format, + # but they can also use Z instead of + # +00:00, which is not supported by datetime.fromisoformat + created_at = datetime.fromisoformat(job["created_at"].replace("Z", "+00:00")) + started_at = datetime.fromisoformat(job["started_at"].replace("Z", "+00:00")) + # conclusion could be null per api schema, so we need to handle that + # though we would assume that it should always be present, + # as the job should be finished + conclusion = job.get("conclusion", None) + + status = job["status"] + job_id = job["id"] + return JobInfo( + job_id=job_id, + created_at=created_at, + started_at=started_at, + conclusion=conclusion, + status=status, + ) diff --git a/src/github_runner_manager/metrics/github.py b/src/github_runner_manager/metrics/github.py index 0c4c8c5..1afc294 100644 --- a/src/github_runner_manager/metrics/github.py +++ b/src/github_runner_manager/metrics/github.py @@ -34,7 +34,7 @@ def job( owner, repo = pre_job_metrics.repository.split("/", maxsplit=1) try: - job_info = github_client.get_job_stats( + job_info = github_client.get_job_info_by_runner_name( path=GitHubRepo(owner=owner, repo=repo), workflow_run_id=pre_job_metrics.workflow_run_id, runner_name=runner_name, diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index 295ca6c..3a638ff 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -19,6 +19,7 @@ from github_runner_manager.github_client import GithubClient from github_runner_manager.manager.runner_manager import RunnerManager from github_runner_manager.reactive.types_ import QueueConfig +from github_runner_manager.types_.github import GitHubRepo logger = logging.getLogger(__name__) @@ -129,10 +130,19 @@ def _check_job_been_picked_up(job_url: HttpUrl, github_client: GithubClient) -> Returns: True if the job has been picked up, False otherwise. """ + # job_url has the format: + # "https://api.github.com/repos/cbartz/gh-runner-test/actions/jobs/22428484402" + path = job_url.path + job_url_path_parts = path.split("/") + job_id = job_url_path_parts[-1] + owner = job_url_path_parts[1] + repo = job_url_path_parts[2] + # See response format: # https://docs.github.com/en/rest/actions/workflow-jobs?apiVersion=2022-11-28#get-a-job-for-a-workflow-run - response = github_client.get(job_url) - return response["status"] in [*JobPickedUpStates] + + job_info = github_client.get_job_info(path=GitHubRepo(owner=owner, repo=repo), job_id=job_id) + return job_info.status in [*JobPickedUpStates] @contextlib.contextmanager diff --git a/src/github_runner_manager/types_/github.py b/src/github_runner_manager/types_/github.py index 89196a7..d0198d8 100644 --- a/src/github_runner_manager/types_/github.py +++ b/src/github_runner_manager/types_/github.py @@ -149,7 +149,27 @@ class JobConclusion(str, Enum): TIMED_OUT = "timed_out" -class JobStats(BaseModel): +class JobStatus(str, Enum): + """Status of a job on GitHub. + + Attributes: + QUEUED: Represents a job that is queued. + IN_PROGRESS: Represents a job that is in progress. + COMPLETED: Represents a job that is completed. + WAITING: Represents a job that is waiting. + REQUESTED: Represents a job that is requested. + PENDING: Represents a job that is pending. + """ + + QUEUED = "queued" + IN_PROGRESS = "in_progress" + COMPLETED = "completed" + WAITING = "waiting" + REQUESTED = "requested" + PENDING = "pending" + + +class JobInfo(BaseModel): """Stats for a job on GitHub. Attributes: @@ -157,12 +177,14 @@ class JobStats(BaseModel): created_at: The time the job was created. started_at: The time the job was started. conclusion: The end result of a job. + status: The status of the job. """ job_id: int created_at: datetime started_at: datetime conclusion: Optional[JobConclusion] + status: JobStatus @dataclasses.dataclass diff --git a/tests/unit/metrics/test_github.py b/tests/unit/metrics/test_github.py index 16ff74c..44ce86d 100644 --- a/tests/unit/metrics/test_github.py +++ b/tests/unit/metrics/test_github.py @@ -11,7 +11,7 @@ from github_runner_manager.github_client import GithubClient from github_runner_manager.metrics import github as github_metrics from github_runner_manager.metrics.runner import PreJobMetrics -from github_runner_manager.types_.github import JobConclusion, JobStats +from github_runner_manager.types_.github import JobConclusion, JobInfo, JobStatus @pytest.fixture(name="pre_job_metrics") @@ -38,11 +38,11 @@ def test_job(pre_job_metrics: PreJobMetrics): runner_name = secrets.token_hex(16) created_at = datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc) started_at = created_at + timedelta(seconds=3600) - github_client.get_job_stats.return_value = JobStats( + github_client.get_job_info_by_runner_name.return_value = JobInfo( created_at=created_at, started_at=started_at, - runner_name=runner_name, conclusion=JobConclusion.SUCCESS, + status=JobStatus.COMPLETED, job_id=randint(1, 1000), ) @@ -62,7 +62,7 @@ def test_job_job_not_found(pre_job_metrics: PreJobMetrics): """ github_client = MagicMock(spec=GithubClient) runner_name = secrets.token_hex(16) - github_client.get_job_stats.side_effect = JobNotFoundError("Job not found") + github_client.get_job_info_by_runner_name.side_effect = JobNotFoundError("Job not found") with pytest.raises(GithubMetricsError): github_metrics.job( diff --git a/tests/unit/reactive/test_consumer.py b/tests/unit/reactive/test_consumer.py index e7612e3..48c5ab7 100644 --- a/tests/unit/reactive/test_consumer.py +++ b/tests/unit/reactive/test_consumer.py @@ -3,7 +3,9 @@ import secrets from contextlib import closing +from datetime import datetime, timedelta, timezone from queue import Empty +from random import randint from unittest.mock import MagicMock import pytest @@ -12,6 +14,7 @@ from github_runner_manager.reactive import consumer from github_runner_manager.reactive.consumer import JobError from github_runner_manager.reactive.types_ import QueueConfig +from github_runner_manager.types_.github import JobInfo, JobConclusion, JobStatus IN_MEMORY_URI = "memory://" FAKE_RUN_URL = "https://api.github.com/repos/fakeusergh-runner-test/actions/runs/8200803099" @@ -46,9 +49,9 @@ def test_consume(monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig): runner_manager_mock = MagicMock(spec=consumer.RunnerManager) github_client_mock = MagicMock(spec=consumer.GithubClient) - github_client_mock.get.side_effect = [ - {"status": "queued"}, - {"status": consumer.JobPickedUpStates.IN_PROGRESS}, + github_client_mock.get_job_info.side_effect = [ + _create_job_info(JobStatus.QUEUED), + _create_job_info(JobStatus.IN_PROGRESS), ] consumer.consume( @@ -64,6 +67,8 @@ def test_consume(monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig): _consume_from_queue(queue_config.queue_name) + + def test_consume_reject_if_job_gets_not_picked_up( monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig ): @@ -80,7 +85,7 @@ def test_consume_reject_if_job_gets_not_picked_up( runner_manager_mock = MagicMock(spec=consumer.RunnerManager) github_client_mock = MagicMock(spec=consumer.GithubClient) - github_client_mock.get.return_value = {"status": "queued"} + github_client_mock.get_job_info.return_value = _create_job_info(JobStatus.QUEUED) consumer.consume( queue_config=queue_config, @@ -116,7 +121,7 @@ def test_job_details_validation_error(job_str: str, queue_config: QueueConfig): runner_manager_mock = MagicMock(spec=consumer.RunnerManager) github_client_mock = MagicMock(spec=consumer.GithubClient) - github_client_mock.get.return_value = {"status": consumer.JobPickedUpStates.IN_PROGRESS} + github_client_mock.get_job_info.return_value = _create_job_info(JobStatus.IN_PROGRESS) with pytest.raises(JobError) as exc_info: consumer.consume( @@ -130,6 +135,22 @@ def test_job_details_validation_error(job_str: str, queue_config: QueueConfig): msg = _consume_from_queue(queue_name) assert msg.payload == job_str +def _create_job_info(status: JobStatus) -> JobInfo: + """Create a JobInfo object with the given status. + + Args: + status: The status of the job. + + Returns: + The JobInfo object. + """ + return JobInfo( + created_at=datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc), + started_at=datetime(2021, 10, 1, 1, 0, 0, tzinfo=timezone.utc), + conclusion=JobConclusion.SUCCESS, + status=status, + job_id=randint(1, 1000), + ) def _put_in_queue(msg: str, queue_name: str) -> None: """Put a job in the message queue. diff --git a/tests/unit/test_github_client.py b/tests/unit/test_github_client.py index 4556aa0..3f249be 100644 --- a/tests/unit/test_github_client.py +++ b/tests/unit/test_github_client.py @@ -11,13 +11,13 @@ import pytest -from github_runner_manager.errors import JobNotFoundError, WrongUrlError +from github_runner_manager.errors import JobNotFoundError from github_runner_manager.github_client import GithubClient -from github_runner_manager.types_.github import GitHubRepo, JobConclusion, JobStats +from github_runner_manager.types_.github import GitHubRepo, JobConclusion, JobInfo, JobStatus JobStatsRawData = namedtuple( "JobStatsRawData", - ["created_at", "started_at", "runner_name", "conclusion", "id"], + ["created_at", "started_at", "runner_name", "conclusion", "id", "status"], ) TEST_URLLIB_RESPONSE_JSON = {"test": "test"} @@ -31,6 +31,7 @@ def job_stats_fixture() -> JobStatsRawData: created_at="2021-10-01T00:00:00Z", started_at="2021-10-01T01:00:00Z", conclusion="success", + status="completed", runner_name=runner_name, id=random.randint(1, 1000), ) @@ -58,6 +59,7 @@ def github_client_fixture( "started_at": job_stats_raw.started_at, "runner_name": job_stats_raw.runner_name, "conclusion": job_stats_raw.conclusion, + "status": job_stats_raw.status, "id": job_stats_raw.id, } ] @@ -94,6 +96,7 @@ def _mock_multiple_pages_for_job_response( "started_at": job_stats_raw.started_at, "runner_name": runner_names[i * no_of_jobs_per_page + j], "conclusion": job_stats_raw.conclusion, + "status": job_stats_raw.status, "id": job_stats_raw.id, } for j in range(no_of_jobs_per_page) @@ -103,33 +106,34 @@ def _mock_multiple_pages_for_job_response( ] + [{"jobs": []}] -def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawData): +def test_get_job_info_by_runner_name(github_client: GithubClient, job_stats_raw: JobStatsRawData): """ arrange: A mocked Github Client that returns one page of jobs containing one job \ with the runner. - act: Call get_job_info. + act: Call get_job_info_by_runner_name. assert: The correct JobStats object is returned. """ github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_stats( + job_stats = github_client.get_job_info_by_runner_name( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, ) - assert job_stats == JobStats( + assert job_stats == JobInfo( created_at=datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc), started_at=datetime(2021, 10, 1, 1, 0, 0, tzinfo=timezone.utc), runner_name=job_stats_raw.runner_name, conclusion=JobConclusion.SUCCESS, + status=JobStatus.COMPLETED, job_id=job_stats_raw.id, ) -def test_get_job_info_no_conclusion(github_client: GithubClient, job_stats_raw: JobStatsRawData): +def test_get_job_info_by_runner_name_no_conclusion(github_client: GithubClient, job_stats_raw: JobStatsRawData): """ arrange: A mocked Github Client that returns one page of jobs containing one job \ with the runner with conclusion set to None. - act: Call get_job_info. + act: Call get_job_info_by_runner_name. assert: JobStats object with conclusion set to None is returned. """ github_client._client.actions.list_jobs_for_workflow_run.return_value = { @@ -139,21 +143,51 @@ def test_get_job_info_no_conclusion(github_client: GithubClient, job_stats_raw: "started_at": job_stats_raw.started_at, "runner_name": job_stats_raw.runner_name, "conclusion": None, + "status": job_stats_raw.status, "id": job_stats_raw.id, } ] } github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_stats( + job_stats = github_client.get_job_info_by_runner_name( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, ) - assert job_stats == JobStats( + assert job_stats == JobInfo( created_at=datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc), started_at=datetime(2021, 10, 1, 1, 0, 0, tzinfo=timezone.utc), runner_name=job_stats_raw.runner_name, conclusion=None, + status=JobStatus.COMPLETED, + job_id=job_stats_raw.id, + ) + +def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawData): + """ + arrange: A mocked Github Client that returns a response. + act: Call get_job_info. + assert: The response is returned. + """ + github_client._client.actions.get_job_for_workflow_run.return_value = { + "created_at": job_stats_raw.created_at, + "started_at": job_stats_raw.started_at, + "runner_name": job_stats_raw.runner_name, + "conclusion": job_stats_raw.conclusion, + "status": job_stats_raw.status, + "id": job_stats_raw.id, + } + github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) + job_stats = github_client.get_job_info( + path=github_repo, + job_id=job_stats_raw.id + ) + assert job_stats == JobInfo( + created_at=datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc), + started_at=datetime(2021, 10, 1, 1, 0, 0, tzinfo=timezone.utc), + runner_name=job_stats_raw.runner_name, + conclusion=JobConclusion.SUCCESS, + status=JobStatus.COMPLETED, job_id=job_stats_raw.id, ) @@ -172,16 +206,17 @@ def test_github_api_pagination_multiple_pages( ) github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_stats( + job_stats = github_client.get_job_info_by_runner_name( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, ) - assert job_stats == JobStats( + assert job_stats == JobInfo( created_at=datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc), started_at=datetime(2021, 10, 1, 1, 0, 0, tzinfo=timezone.utc), runner_name=job_stats_raw.runner_name, conclusion=JobConclusion.SUCCESS, + status=JobStatus.COMPLETED, job_id=job_stats_raw.id, ) @@ -202,7 +237,7 @@ def test_github_api_pagination_job_not_found( github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) with pytest.raises(JobNotFoundError): - github_client.get_job_stats( + github_client.get_job_info_by_runner_name( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, @@ -216,28 +251,8 @@ def test_github_api_http_error(github_client: GithubClient, job_stats_raw: JobSt github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) with pytest.raises(JobNotFoundError): - github_client.get_job_stats( + github_client.get_job_info_by_runner_name( path=github_repo, workflow_run_id=secrets.token_hex(16), runner_name=job_stats_raw.runner_name, ) - - -def test_github_client_get(github_client: GithubClient): - """ - arrange: A mocked Github Client that returns a response. - act: Call get. - assert: The response is returned. - """ - response = github_client.get("https://api.github.com/test") - assert response == TEST_URLLIB_RESPONSE_JSON - - -def test_github_client_get_wrong_url(github_client: GithubClient): - """ - arrange: A mocked Github Client that returns a response. - act: Call get with a wrong url, not starting with https://api.github.com. - assert: An exception is raised. - """ - with pytest.raises(WrongUrlError): - github_client.get("https://example.com/test") From bd0b1a3760906176e37f2c1cf55ff7c8fc1e93a8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 17 Sep 2024 13:57:36 +0200 Subject: [PATCH 12/18] rename run_url -> job_url --- src-docs/reactive.consumer.md | 2 +- src/github_runner_manager/reactive/consumer.py | 8 ++++---- tests/unit/reactive/test_consumer.py | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index 64d31da..d8f2f7a 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -90,7 +90,7 @@ A class to translate the payload. **Attributes:** - `labels`: The labels of the job. - - `run_url`: The URL of the job. + - `job_url`: The URL of the job. diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index 3a638ff..5638d9e 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -41,11 +41,11 @@ class JobDetails(BaseModel): Attributes: labels: The labels of the job. - run_url: The URL of the job. + job_url: The URL of the job. """ labels: list[str] - run_url: HttpUrl + job_url: HttpUrl class JobError(Exception): @@ -80,11 +80,11 @@ def consume( logger.info( "Received job with labels %s and run_url %s", job_details.labels, - job_details.run_url, + job_details.job_url, ) _spawn_runner( runner_manager=runner_manager, - job_url=job_details.run_url, + job_url=job_details.job_url, msg=msg, github_client=github_client, ) diff --git a/tests/unit/reactive/test_consumer.py b/tests/unit/reactive/test_consumer.py index 48c5ab7..211f0a9 100644 --- a/tests/unit/reactive/test_consumer.py +++ b/tests/unit/reactive/test_consumer.py @@ -17,7 +17,7 @@ from github_runner_manager.types_.github import JobInfo, JobConclusion, JobStatus IN_MEMORY_URI = "memory://" -FAKE_RUN_URL = "https://api.github.com/repos/fakeusergh-runner-test/actions/runs/8200803099" +FAKE_JOB_URL = "https://api.github.com/repos/fakeuser/gh-runner-test/actions/runs/8200803099" @pytest.fixture(name="queue_config") @@ -43,7 +43,7 @@ def test_consume(monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig): """ job_details = consumer.JobDetails( labels=[secrets.token_hex(16), secrets.token_hex(16)], - run_url=FAKE_RUN_URL, + job_url=FAKE_JOB_URL, ) _put_in_queue(job_details.json(), queue_config.queue_name) @@ -79,7 +79,7 @@ def test_consume_reject_if_job_gets_not_picked_up( """ job_details = consumer.JobDetails( labels=[secrets.token_hex(16), secrets.token_hex(16)], - run_url=FAKE_RUN_URL, + job_url=FAKE_JOB_URL, ) _put_in_queue(job_details.json(), queue_config.queue_name) From af162a5dc9a622754af760a1985858956b97242e Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 17 Sep 2024 15:46:17 +0200 Subject: [PATCH 13/18] Fix parsing of job_url --- src-docs/reactive.consumer.md | 2 +- .../reactive/consumer.py | 9 ++++--- tests/unit/reactive/test_consumer.py | 6 ++--- tests/unit/test_github_client.py | 24 +++++++++---------- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index d8f2f7a..9e279ef 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -41,7 +41,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index 5638d9e..be8a6d1 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -78,7 +78,7 @@ def consume( msg.reject(requeue=True) raise JobError(f"Invalid job details: {msg.payload}") from exc logger.info( - "Received job with labels %s and run_url %s", + "Received job with labels %s and job_url %s", job_details.labels, job_details.job_url, ) @@ -135,8 +135,11 @@ def _check_job_been_picked_up(job_url: HttpUrl, github_client: GithubClient) -> path = job_url.path job_url_path_parts = path.split("/") job_id = job_url_path_parts[-1] - owner = job_url_path_parts[1] - repo = job_url_path_parts[2] + owner = job_url_path_parts[2] + repo = job_url_path_parts[3] + logging.debug( + "Parsed job_id: %s, owner: %s, repo: %s from job_url path %s", job_id, owner, repo, path + ) # See response format: # https://docs.github.com/en/rest/actions/workflow-jobs?apiVersion=2022-11-28#get-a-job-for-a-workflow-run diff --git a/tests/unit/reactive/test_consumer.py b/tests/unit/reactive/test_consumer.py index 211f0a9..5cc63c2 100644 --- a/tests/unit/reactive/test_consumer.py +++ b/tests/unit/reactive/test_consumer.py @@ -14,7 +14,7 @@ from github_runner_manager.reactive import consumer from github_runner_manager.reactive.consumer import JobError from github_runner_manager.reactive.types_ import QueueConfig -from github_runner_manager.types_.github import JobInfo, JobConclusion, JobStatus +from github_runner_manager.types_.github import JobConclusion, JobInfo, JobStatus IN_MEMORY_URI = "memory://" FAKE_JOB_URL = "https://api.github.com/repos/fakeuser/gh-runner-test/actions/runs/8200803099" @@ -67,8 +67,6 @@ def test_consume(monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig): _consume_from_queue(queue_config.queue_name) - - def test_consume_reject_if_job_gets_not_picked_up( monkeypatch: pytest.MonkeyPatch, queue_config: QueueConfig ): @@ -135,6 +133,7 @@ def test_job_details_validation_error(job_str: str, queue_config: QueueConfig): msg = _consume_from_queue(queue_name) assert msg.payload == job_str + def _create_job_info(status: JobStatus) -> JobInfo: """Create a JobInfo object with the given status. @@ -152,6 +151,7 @@ def _create_job_info(status: JobStatus) -> JobInfo: job_id=randint(1, 1000), ) + def _put_in_queue(msg: str, queue_name: str) -> None: """Put a job in the message queue. diff --git a/tests/unit/test_github_client.py b/tests/unit/test_github_client.py index 3f249be..e184cfb 100644 --- a/tests/unit/test_github_client.py +++ b/tests/unit/test_github_client.py @@ -129,7 +129,9 @@ def test_get_job_info_by_runner_name(github_client: GithubClient, job_stats_raw: ) -def test_get_job_info_by_runner_name_no_conclusion(github_client: GithubClient, job_stats_raw: JobStatsRawData): +def test_get_job_info_by_runner_name_no_conclusion( + github_client: GithubClient, job_stats_raw: JobStatsRawData +): """ arrange: A mocked Github Client that returns one page of jobs containing one job \ with the runner with conclusion set to None. @@ -163,6 +165,7 @@ def test_get_job_info_by_runner_name_no_conclusion(github_client: GithubClient, job_id=job_stats_raw.id, ) + def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawData): """ arrange: A mocked Github Client that returns a response. @@ -170,18 +173,15 @@ def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawDat assert: The response is returned. """ github_client._client.actions.get_job_for_workflow_run.return_value = { - "created_at": job_stats_raw.created_at, - "started_at": job_stats_raw.started_at, - "runner_name": job_stats_raw.runner_name, - "conclusion": job_stats_raw.conclusion, - "status": job_stats_raw.status, - "id": job_stats_raw.id, - } + "created_at": job_stats_raw.created_at, + "started_at": job_stats_raw.started_at, + "runner_name": job_stats_raw.runner_name, + "conclusion": job_stats_raw.conclusion, + "status": job_stats_raw.status, + "id": job_stats_raw.id, + } github_repo = GitHubRepo(owner=secrets.token_hex(16), repo=secrets.token_hex(16)) - job_stats = github_client.get_job_info( - path=github_repo, - job_id=job_stats_raw.id - ) + job_stats = github_client.get_job_info(path=github_repo, job_id=job_stats_raw.id) assert job_stats == JobInfo( created_at=datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc), started_at=datetime(2021, 10, 1, 1, 0, 0, tzinfo=timezone.utc), From ae8a5202cc2e2676edfbee63e22e7e26b3241ba6 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 18 Sep 2024 11:05:12 +0200 Subject: [PATCH 14/18] validate path of job_url --- src-docs/reactive.consumer.md | 35 +++++++++++++++++-- .../reactive/consumer.py | 23 ++++++++++-- tests/unit/reactive/test_consumer.py | 11 ++++-- 3 files changed, 61 insertions(+), 8 deletions(-) diff --git a/src-docs/reactive.consumer.md b/src-docs/reactive.consumer.md index 9e279ef..b8433b4 100644 --- a/src-docs/reactive.consumer.md +++ b/src-docs/reactive.consumer.md @@ -8,7 +8,7 @@ Module responsible for consuming jobs from the message queue. --- - + ## function `consume` @@ -41,7 +41,7 @@ Log the job details and acknowledge the message. If the job details are invalid, --- - + ## function `signal_handler` @@ -95,10 +95,39 @@ A class to translate the payload. +--- + + + +### classmethod `check_job_url_path_is_not_empty` + +```python +check_job_url_path_is_not_empty(v: HttpUrl) → HttpUrl +``` + +Check that the job_url path is not empty. + + + +**Args:** + + - `v`: The job_url to check. + + + +**Returns:** + The job_url if it is valid. + + + +**Raises:** + + - `ValueError`: If the job_url path is empty. + --- - + ## class `JobError` Raised when a job error occurs. diff --git a/src/github_runner_manager/reactive/consumer.py b/src/github_runner_manager/reactive/consumer.py index be8a6d1..d35b8f1 100644 --- a/src/github_runner_manager/reactive/consumer.py +++ b/src/github_runner_manager/reactive/consumer.py @@ -14,7 +14,7 @@ from kombu import Connection, Message from kombu.simple import SimpleQueue -from pydantic import BaseModel, HttpUrl, ValidationError +from pydantic import BaseModel, HttpUrl, ValidationError, validator from github_runner_manager.github_client import GithubClient from github_runner_manager.manager.runner_manager import RunnerManager @@ -47,6 +47,24 @@ class JobDetails(BaseModel): labels: list[str] job_url: HttpUrl + @validator("job_url") + @classmethod + def check_job_url_path_is_not_empty(cls, v: HttpUrl) -> HttpUrl: + """Check that the job_url path is not empty. + + Args: + v: The job_url to check. + + Returns: + The job_url if it is valid. + + Raises: + ValueError: If the job_url path is empty. + """ + if not v.path: + raise ValueError("path must be provided") + return v + class JobError(Exception): """Raised when a job error occurs.""" @@ -133,7 +151,8 @@ def _check_job_been_picked_up(job_url: HttpUrl, github_client: GithubClient) -> # job_url has the format: # "https://api.github.com/repos/cbartz/gh-runner-test/actions/jobs/22428484402" path = job_url.path - job_url_path_parts = path.split("/") + # we know that path is not empty as it is validated by the JobDetails model + job_url_path_parts = path.split("/") # type: ignore job_id = job_url_path_parts[-1] owner = job_url_path_parts[2] repo = job_url_path_parts[3] diff --git a/tests/unit/reactive/test_consumer.py b/tests/unit/reactive/test_consumer.py index 5cc63c2..7450535 100644 --- a/tests/unit/reactive/test_consumer.py +++ b/tests/unit/reactive/test_consumer.py @@ -3,7 +3,7 @@ import secrets from contextlib import closing -from datetime import datetime, timedelta, timezone +from datetime import datetime, timezone from queue import Empty from random import randint from unittest.mock import MagicMock @@ -100,10 +100,15 @@ def test_consume_reject_if_job_gets_not_picked_up( "job_str", [ pytest.param( - '{"labels": ["label1", "label2"], "status": "completed"}', id="run_url missing" + '{"labels": ["label1", "label2"], "status": "completed"}', id="job_url missing" ), pytest.param( - '{"status": "completed", "run_url": "https://example.com"}', id="labels missing" + '{"status": "completed", "job_url": "https://example.com/path"}', id="labels missing" + ), + pytest.param( + '{"labels": ["label1", "label2"], "status": "completed", ' + '"job_url": "https://example.com"}', + id="job_url without path", ), pytest.param("no json at all", id="invalid json"), ], From 237ad2562b7263b165fe9acad723a50f07598033 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 18 Sep 2024 12:06:25 +0200 Subject: [PATCH 15/18] rename reactive_config -> runner_config --- src-docs/reactive.runner_manager.md | 6 +++--- src/github_runner_manager/manager/runner_scaler.py | 2 +- src/github_runner_manager/reactive/runner_manager.py | 7 +++---- tests/unit/reactive/test_runner_manager.py | 10 +++++----- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src-docs/reactive.runner_manager.md b/src-docs/reactive.runner_manager.md index 4c5f696..b426d52 100644 --- a/src-docs/reactive.runner_manager.md +++ b/src-docs/reactive.runner_manager.md @@ -17,12 +17,12 @@ Module for managing reactive runners. --- - + ## function `reconcile` ```python -reconcile(quantity: int, reactive_config: RunnerConfig) → int +reconcile(quantity: int, runner_config: RunnerConfig) → int ``` Spawn a runner reactively. @@ -32,7 +32,7 @@ Spawn a runner reactively. **Args:** - `quantity`: The number of runners to spawn. - - `reactive_config`: The reactive runner configuration. + - `runner_config`: The reactive runner configuration. Raises a ReactiveRunnerError if the runner fails to spawn. diff --git a/src/github_runner_manager/manager/runner_scaler.py b/src/github_runner_manager/manager/runner_scaler.py index 95531e6..c13a9c3 100644 --- a/src/github_runner_manager/manager/runner_scaler.py +++ b/src/github_runner_manager/manager/runner_scaler.py @@ -263,5 +263,5 @@ def _reconcile_reactive(self, quantity: int) -> int: self._manager.cleanup() return reactive_runner_manager.reconcile( quantity=quantity, - reactive_config=self._reactive_config, + runner_config=self._reactive_config, ) diff --git a/src/github_runner_manager/reactive/runner_manager.py b/src/github_runner_manager/reactive/runner_manager.py index f214cef..051a081 100644 --- a/src/github_runner_manager/reactive/runner_manager.py +++ b/src/github_runner_manager/reactive/runner_manager.py @@ -37,13 +37,12 @@ class ReactiveRunnerError(Exception): """Raised when a reactive runner error occurs.""" -# TODO: rename reactive_config argument to runner_config -def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: +def reconcile(quantity: int, runner_config: RunnerConfig) -> int: """Spawn a runner reactively. Args: quantity: The number of runners to spawn. - reactive_config: The reactive runner configuration. + runner_config: The reactive runner configuration. Raises a ReactiveRunnerError if the runner fails to spawn. @@ -58,7 +57,7 @@ def reconcile(quantity: int, reactive_config: RunnerConfig) -> int: logger.info("Will spawn %d new reactive runner process(es)", delta) _setup_logging_for_processes() for _ in range(delta): - _spawn_runner(reactive_config) + _spawn_runner(runner_config) elif delta < 0: logger.info("Will kill %d process(es).", -delta) for pid in pids[:-delta]: diff --git a/tests/unit/reactive/test_runner_manager.py b/tests/unit/reactive/test_runner_manager.py index 8d88c95..714523b 100644 --- a/tests/unit/reactive/test_runner_manager.py +++ b/tests/unit/reactive/test_runner_manager.py @@ -87,7 +87,7 @@ def test_reconcile_spawns_runners( """ _arrange_reactive_processes(secure_run_subprocess_mock, count=2) - delta = reconcile(5, reactive_config=runner_config) + delta = reconcile(5, runner_config=runner_config) assert delta == 3 assert subprocess_popen_mock.call_count == 3 @@ -106,7 +106,7 @@ def test_reconcile_does_not_spawn_runners( """ _arrange_reactive_processes(secure_run_subprocess_mock, count=2) - delta = reconcile(2, reactive_config=runner_config) + delta = reconcile(2, runner_config=runner_config) assert delta == 0 assert subprocess_popen_mock.call_count == 0 @@ -124,7 +124,7 @@ def test_reconcile_kills_processes_for_too_many_processes( assert: 2 processes are killed. """ _arrange_reactive_processes(secure_run_subprocess_mock, count=3) - delta = reconcile(1, reactive_config=runner_config) + delta = reconcile(1, runner_config=runner_config) assert delta == -2 assert subprocess_popen_mock.call_count == 0 @@ -144,7 +144,7 @@ def test_reconcile_ignore_process_not_found_on_kill( """ _arrange_reactive_processes(secure_run_subprocess_mock, count=3) os_kill_mock.side_effect = [None, ProcessLookupError] - delta = reconcile(1, reactive_config=runner_config) + delta = reconcile(1, runner_config=runner_config) assert delta == -2 assert subprocess_popen_mock.call_count == 0 @@ -167,7 +167,7 @@ def test_reconcile_raises_reactive_runner_error_on_ps_failure( ) with pytest.raises(ReactiveRunnerError) as err: - reconcile(1, reactive_config=runner_config) + reconcile(1, runner_config=runner_config) assert "Failed to get list of processes" in str(err.value) From e78f285ce47f006ba4302564581e82ce576640b2 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 18 Sep 2024 12:21:04 +0200 Subject: [PATCH 16/18] rename manager_name -> name --- src-docs/openstack_cloud.openstack_runner_manager.md | 4 ++-- .../openstack_cloud/openstack_runner_manager.py | 6 +++--- tests/unit/test_openstack_runner_manager.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src-docs/openstack_cloud.openstack_runner_manager.md b/src-docs/openstack_cloud.openstack_runner_manager.md index ee45ee0..d041411 100644 --- a/src-docs/openstack_cloud.openstack_runner_manager.md +++ b/src-docs/openstack_cloud.openstack_runner_manager.md @@ -87,7 +87,7 @@ Configuration for OpenStack runner manager. **Attributes:** - - `manager_name`: The name of the manager. + - `name`: The name of the manager. - `prefix`: The prefix of the runner names. - `cloud_config`: The configuration for OpenStack cloud. - `server_config`: The configuration for OpenStack server. @@ -100,7 +100,7 @@ Configuration for OpenStack runner manager. ```python __init__( - manager_name: str, + name: str, prefix: str, cloud_config: OpenStackCloudConfig, server_config: OpenStackServerConfig | None, diff --git a/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index 849e7b3..c89c120 100644 --- a/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -103,7 +103,7 @@ class OpenStackRunnerManagerConfig: """Configuration for OpenStack runner manager. Attributes: - manager_name: The name of the manager. + name: The name of the manager. prefix: The prefix of the runner names. cloud_config: The configuration for OpenStack cloud. server_config: The configuration for OpenStack server. @@ -111,7 +111,7 @@ class OpenStackRunnerManagerConfig: service_config: The configuration for supporting services. """ - manager_name: str + name: str prefix: str cloud_config: OpenStackCloudConfig server_config: OpenStackServerConfig | None @@ -148,7 +148,7 @@ def __init__( # pylint: disable=R0913 Args: config: The configuration for the openstack runner manager. """ - self._manager_name = config.manager_name + self._manager_name = config.name self._prefix = config.prefix self._cloud_config = config.cloud_config self._server_config = config.server_config diff --git a/tests/unit/test_openstack_runner_manager.py b/tests/unit/test_openstack_runner_manager.py index a5809e7..c16ea62 100644 --- a/tests/unit/test_openstack_runner_manager.py +++ b/tests/unit/test_openstack_runner_manager.py @@ -17,7 +17,7 @@ def mock_openstack_runner_manager_fixture(): """The mocked OpenStackRunnerManager instance.""" config = openstack_runner_manager.OpenStackRunnerManagerConfig( - manager_name="mock-manager", + name="mock-manager", prefix="mock-manager", cloud_config=openstack_runner_manager.OpenStackCloudConfig( clouds_config={ From 71d2ec24e04259de9c6f28b048f9f08f5da999f1 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 18 Sep 2024 13:13:37 +0200 Subject: [PATCH 17/18] add pymongo depedency --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index 15847d7..df2f853 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,3 +4,4 @@ jinja2 kombu==5.4.1 openstacksdk>=3,<4 pydantic < 2 +pymongo==4.8.0 From 132f7485d2580476072e9432e617a6fb82bd2223 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 18 Sep 2024 13:34:15 +0200 Subject: [PATCH 18/18] bump version in pyproject.toml --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 851692b..9f775f0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -3,7 +3,7 @@ [project] name = "github-runner-manager" -version = "0.1.4" +version = "0.2.0" authors = [ { name = "Canonical IS DevOps", email = "is-devops-team@canonical.com" }, ]