diff --git a/docs/changelog.md b/docs/changelog.md index 200e06f05..07369049b 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,5 +1,10 @@ # Changelog +### 2024-09-18 + +- Changed code to be able to spawn a runner in reactive mode. +- Removed reactive mode support for LXD as it is not currently in development. + ## 2024-09-09 - Added changelog for tracking user-relevant changes. diff --git a/requirements.txt b/requirements.txt index 48fc691a8..34c61f262 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,6 +12,4 @@ cosl ==0.0.15 # juju 3.1.2.0 depends on pyyaml<=6.0 and >=5.1.2 PyYAML ==6.0.* pyOpenSSL==24.2.1 -kombu==5.4.1 -pymongo==4.8.0 -github_runner_manager @ git+https://github.com/canonical/github-runner-manager.git@6ee40e296f92f191dd12b1e24a613dfdbdc70f99 +github_runner_manager @ git+https://github.com/canonical/github-runner-manager.git@e53ed35048e124e3046b860bc0b8c7d89abd8b18 diff --git a/src-docs/charm.md b/src-docs/charm.md index 5b1540fee..5458c6a13 100644 --- a/src-docs/charm.md +++ b/src-docs/charm.md @@ -21,7 +21,7 @@ Charm for creating and managing GitHub self-hosted runner instances. --- - + ## function `catch_charm_errors` @@ -47,7 +47,7 @@ Catch common errors in charm. --- - + ## function `catch_action_errors` @@ -73,7 +73,7 @@ Catch common errors in actions. --- - + ## class `ReconcileRunnersEvent` Event representing a periodic check to ensure runners are ok. @@ -84,7 +84,7 @@ Event representing a periodic check to ensure runners are ok. --- - + ## class `GithubRunnerCharm` Charm for managing GitHub self-hosted runners. @@ -101,7 +101,7 @@ Charm for managing GitHub self-hosted runners. - `ram_pool_path`: The path to memdisk storage. - `kernel_module_path`: The path to kernel modules. - + ### method `__init__` diff --git a/src-docs/runner_manager.md b/src-docs/runner_manager.md index 883745753..ebf6806b8 100644 --- a/src-docs/runner_manager.md +++ b/src-docs/runner_manager.md @@ -13,7 +13,7 @@ Runner Manager manages the runners on LXD and GitHub. --- - + ## class `LXDRunnerManager` Manage a group of runners according to configuration. @@ -25,7 +25,7 @@ Manage a group of runners according to configuration. - `runner_bin_path`: The github runner app scripts path. - `cron_path`: The path to runner build image cron job. - + ### method `__init__` @@ -52,7 +52,7 @@ Construct RunnerManager object for creating and managing runners. --- - + ### method `build_runner_image` @@ -72,7 +72,7 @@ Build container image in test mode, else virtual machine image. --- - + ### method `check_runner_bin` @@ -89,7 +89,7 @@ Check if runner binary exists. --- - + ### method `flush` @@ -118,7 +118,7 @@ Remove existing runners. --- - + ### method `get_github_info` @@ -135,7 +135,7 @@ Get information on the runners from GitHub. --- - + ### method `get_latest_runner_bin_url` @@ -166,7 +166,7 @@ The runner binary URL changes when a new version is available. --- - + ### method `has_runner_image` @@ -183,7 +183,7 @@ Check if the runner image exists. --- - + ### method `reconcile` @@ -207,7 +207,7 @@ Bring runners in line with target. --- - + ### method `schedule_build_runner_image` @@ -219,7 +219,7 @@ Install cron job for building runner image. --- - + ### method `update_runner_bin` diff --git a/src-docs/runner_manager_type.md b/src-docs/runner_manager_type.md index cd7eaf5d2..7cf5c5fe9 100644 --- a/src-docs/runner_manager_type.md +++ b/src-docs/runner_manager_type.md @@ -9,7 +9,7 @@ Types used by RunnerManager class. --- - + ## class `LXDFlushMode` Strategy for flushing runners. @@ -32,7 +32,7 @@ During pre-job (repo-check), the runners are marked as idle and if the pre-job f --- - + ## class `RunnerManagerClients` Clients for accessing various services. @@ -69,7 +69,7 @@ __init__( --- - + ## class `LXDRunnerManagerConfig` Configuration of runner manager. @@ -121,54 +121,7 @@ Whether metrics for the runners should be collected. --- - - -## class `OpenstackRunnerManagerConfig` -Configuration of runner manager. - - - -**Attributes:** - - - `charm_state`: The state of the charm. - - `path`: GitHub repository path in the format '/', or the GitHub organization name. - - `labels`: Additional labels for the runners. - - `token`: GitHub personal access token to register runner to the repository or organization. - - `flavor`: OpenStack flavor for defining the runner resources. - - `image`: Openstack image id to boot the runner with. - - `network`: OpenStack network for runner network access. - - `dockerhub_mirror`: URL of dockerhub mirror to use. - - `reactive_config`: The configuration to spawn runners reactively. - - - -### method `__init__` - -```python -__init__( - charm_state: CharmState, - path: GitHubOrg | GitHubRepo, - labels: Iterable[str], - token: str, - flavor: str, - image: str, - network: str, - dockerhub_mirror: str | None, - reactive_config: ReactiveConfig | None = None -) → None -``` - - - - - - - - - ---- - - + ## class `RunnerInfo` Information from GitHub of a runner. diff --git a/src/charm.py b/src/charm.py index 9dae00059..410b8951c 100755 --- a/src/charm.py +++ b/src/charm.py @@ -20,8 +20,11 @@ from github_runner_manager.openstack_cloud.openstack_runner_manager import ( OpenStackCloudConfig, OpenStackRunnerManager, + OpenStackRunnerManagerConfig, OpenStackServerConfig, ) +from github_runner_manager.reactive.types_ import QueueConfig as ReactiveQueueConfig +from github_runner_manager.reactive.types_ import RunnerConfig as ReactiveRunnerConfig from github_runner_manager.types_.github import GitHubPath, GitHubRunnerStatus, parse_github_path from utilities import bytes_with_unit_to_kib, execute_command, remove_residual_venv_dirs, retry @@ -1246,6 +1249,46 @@ def _get_runner_scaler( if path is None: path = state.charm_config.path + openstack_runner_manager_config = self._create_openstack_runner_manager_config(path, state) + openstack_runner_manager = OpenStackRunnerManager( + config=openstack_runner_manager_config, + ) + runner_manager_config = RunnerManagerConfig( + name=self.app.name, + token=token, + path=path, + ) + runner_manager = RunnerManager( + cloud_runner_manager=openstack_runner_manager, + config=runner_manager_config, + ) + reactive_runner_config = None + if reactive_config := state.reactive_config: + reactive_runner_config = ReactiveRunnerConfig( + queue=ReactiveQueueConfig( + mongodb_uri=reactive_config.mq_uri, queue_name=self.app.name + ), + runner_manager=runner_manager_config, + cloud_runner_manager=openstack_runner_manager_config, + github_token=token, + ) + return RunnerScaler( + runner_manager=runner_manager, reactive_runner_config=reactive_runner_config + ) + + def _create_openstack_runner_manager_config( + self, path: GitHubPath, state: CharmState + ) -> OpenStackRunnerManagerConfig: + """Create OpenStackRunnerManagerConfig instance. + + Args: + path: GitHub repository path in the format '/', or the GitHub organization + name. + state: Charm state. + + Returns: + An instance of OpenStackRunnerManagerConfig. + """ clouds = list(state.charm_config.openstack_clouds_yaml["clouds"].keys()) if len(clouds) > 1: logger.warning( @@ -1266,7 +1309,6 @@ def _get_runner_scaler( ) if image.tags: image_labels += image.tags - runner_config = GitHubRunnerConfig( github_path=path, labels=(*state.charm_config.labels, *image_labels) ) @@ -1276,25 +1318,16 @@ def _get_runner_scaler( ssh_debug_connections=state.ssh_debug_connections, repo_policy_compliance=state.charm_config.repo_policy_compliance, ) - # The prefix is set to f"{application_name}-{unit number}" - openstack_runner_manager = OpenStackRunnerManager( - manager_name=self.app.name, + openstack_runner_manager_config = OpenStackRunnerManagerConfig( + name=self.app.name, + # The prefix is set to f"{application_name}-{unit number}" prefix=self.unit.name.replace("/", "-"), cloud_config=cloud_config, server_config=server_config, runner_config=runner_config, service_config=service_config, ) - runner_manager_config = RunnerManagerConfig( - token=token, - path=path, - ) - runner_manager = RunnerManager( - manager_name=self.app.name, - cloud_runner_manager=openstack_runner_manager, - config=runner_manager_config, - ) - return RunnerScaler(runner_manager=runner_manager, reactive_config=state.reactive_config) + return openstack_runner_manager_config if __name__ == "__main__": diff --git a/src/charm_state.py b/src/charm_state.py index 6ae46d386..afe174388 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -38,6 +38,11 @@ from firewall import FirewallEntry from utilities import get_env_var +REACTIVE_MODE_NOT_SUPPORTED_WITH_LXD_ERR_MSG = ( + "Reactive mode not supported for local LXD instances. " + "Please remove the mongodb integration." +) + logger = logging.getLogger(__name__) ARCHITECTURES_ARM64 = {"aarch64", "arm64"} @@ -1196,6 +1201,10 @@ def from_charm( # noqa: C901 reactive_config = ReactiveConfig.from_database(database) + if instance_type == InstanceType.LOCAL_LXD and reactive_config: + logger.error(REACTIVE_MODE_NOT_SUPPORTED_WITH_LXD_ERR_MSG) + raise CharmConfigInvalidError(REACTIVE_MODE_NOT_SUPPORTED_WITH_LXD_ERR_MSG) + state = cls( arch=arch, is_metrics_logging_available=bool(charm.model.relations[COS_AGENT_INTEGRATION_NAME]), diff --git a/src/runner_manager.py b/src/runner_manager.py index 66a7e03d3..ab4b232c5 100644 --- a/src/runner_manager.py +++ b/src/runner_manager.py @@ -13,7 +13,6 @@ from pathlib import Path from typing import Iterator, Optional, Type -import github_runner_manager.reactive.runner_manager as reactive_runner_manager import jinja2 import requests import requests.adapters @@ -534,9 +533,6 @@ def reconcile(self, quantity: int, resources: VirtualMachineResources) -> int: Returns: Difference between intended runners and actual runners. """ - if self.config.reactive_config: - logger.info("Reactive configuration detected, going into experimental reactive mode.") - return self._reconcile_reactive(quantity) start_ts = time.time() runners = self._get_runners() @@ -581,21 +577,6 @@ def reconcile(self, quantity: int, resources: VirtualMachineResources) -> int: ) return delta - def _reconcile_reactive(self, quantity: int) -> int: - """Reconcile runners reactively. - - Args: - quantity: Number of intended runners. - - Returns: - The difference between intended runners and actual runners. In reactive mode - this number is never negative as additional processes should terminate after a timeout. - """ - logger.info("Reactive mode is experimental and not yet fully implemented.") - return reactive_runner_manager.reconcile( - quantity=quantity, mq_uri=self.config.reactive_config.mq_uri, queue_name=self.app_name - ) - def _runners_in_pre_job(self) -> bool: """Check there exist runners in the pre-job script stage. diff --git a/src/runner_manager_type.py b/src/runner_manager_type.py index deb30540b..4c5ca9820 100644 --- a/src/runner_manager_type.py +++ b/src/runner_manager_type.py @@ -6,7 +6,6 @@ from dataclasses import dataclass from enum import Enum, auto from pathlib import Path -from typing import Iterable import jinja2 from github_runner_manager.repo_policy_compliance_client import RepoPolicyComplianceClient @@ -93,36 +92,6 @@ def are_metrics_enabled(self) -> bool: return self.charm_state.is_metrics_logging_available -# This class is subject to refactor. -@dataclass -class OpenstackRunnerManagerConfig: # pylint: disable=too-many-instance-attributes - """Configuration of runner manager. - - Attributes: - charm_state: The state of the charm. - path: GitHub repository path in the format '/', or the - GitHub organization name. - labels: Additional labels for the runners. - token: GitHub personal access token to register runner to the - repository or organization. - flavor: OpenStack flavor for defining the runner resources. - image: Openstack image id to boot the runner with. - network: OpenStack network for runner network access. - dockerhub_mirror: URL of dockerhub mirror to use. - reactive_config: The configuration to spawn runners reactively. - """ - - charm_state: CharmState - path: GitHubPath - labels: Iterable[str] - token: str - flavor: str - image: str - network: str - dockerhub_mirror: str | None - reactive_config: ReactiveConfig | None = None - - @dataclass class RunnerInfo: """Information from GitHub of a runner. diff --git a/tests/integration/helpers/common.py b/tests/integration/helpers/common.py index 495c952b3..95c8b051b 100644 --- a/tests/integration/helpers/common.py +++ b/tests/integration/helpers/common.py @@ -392,6 +392,7 @@ async def dispatch_workflow( branch: The branch to dispatch the workflow on. github_repository: The github repository to dispatch the workflow on. conclusion: The expected workflow run conclusion. + This argument is ignored if wait is False. workflow_id_or_name: The workflow filename in .github/workflows in main branch to run or its id. dispatch_input: Workflow input values. @@ -420,14 +421,28 @@ async def dispatch_workflow( if not wait: return run - await wait_for(partial(_is_workflow_run_complete, run=run), timeout=60 * 30, check_interval=60) + await wait_for_completion(run=run, conclusion=conclusion) + + return run + + +async def wait_for_completion(run: WorkflowRun, conclusion: str) -> None: + """Wait for the workflow run to complete. + + Args: + run: The workflow run to wait for. + conclusion: The expected conclusion of the run. + """ + await wait_for( + partial(_is_workflow_run_complete, run=run), + timeout=60 * 30, + check_interval=60, + ) # The run object is updated by _is_workflow_run_complete function above. assert ( run.conclusion == conclusion ), f"Unexpected run conclusion, expected: {conclusion}, got: {run.conclusion}" - return run - P = ParamSpec("P") R = TypeVar("R") diff --git a/tests/integration/requirements.txt b/tests/integration/requirements.txt index 8b05cbf57..c682c9050 100644 --- a/tests/integration/requirements.txt +++ b/tests/integration/requirements.txt @@ -1,2 +1,4 @@ GitPython>3,<4 pygithub +kombu==5.4.* +pymongo==4.8.* diff --git a/tests/integration/test_reactive.py b/tests/integration/test_reactive.py index 06dc6e48c..461ec687c 100644 --- a/tests/integration/test_reactive.py +++ b/tests/integration/test_reactive.py @@ -6,21 +6,29 @@ import secrets import pytest +from github import Branch, Repository from github_runner_manager.reactive.consumer import JobDetails -from github_runner_manager.reactive.runner_manager import REACTIVE_RUNNER_LOG_DIR from juju.application import Application from juju.model import Model from juju.unit import Unit from kombu import Connection from pytest_operator.plugin import OpsTest -from tests.integration.helpers.common import get_file_content, reconcile, run_in_unit - -FAKE_URL = "http://example.com" +from tests.integration.helpers.common import ( + DISPATCH_TEST_WORKFLOW_FILENAME, + dispatch_workflow, + reconcile, + wait_for_completion, +) @pytest.mark.openstack -async def test_reactive_mode_consumes_jobs(ops_test: OpsTest, app_for_reactive: Application): +async def test_reactive_mode_consumes_jobs( + ops_test: OpsTest, + app_for_reactive: Application, + github_repository: Repository, + test_github_branch: Branch, +): """ arrange: A charm integrated with mongodb and a message is added to the queue. act: Call reconcile. @@ -32,8 +40,20 @@ async def test_reactive_mode_consumes_jobs(ops_test: OpsTest, app_for_reactive: mongodb_uri = await _get_mongodb_uri_from_secrets(ops_test, unit.model) assert mongodb_uri, "mongodb uri not found in integration data or secret" + run = await dispatch_workflow( + app=app_for_reactive, + branch=test_github_branch, + github_repository=github_repository, + conclusion="success", + workflow_id_or_name=DISPATCH_TEST_WORKFLOW_FILENAME, + wait=False, + ) + jobs = list(run.jobs()) + assert len(jobs) == 1, "Expected 1 job to be created" + job = jobs[0] + job_url = job.url job = JobDetails( - labels=[secrets.token_hex(16) for _ in range(random.randint(1, 4))], run_url=FAKE_URL + labels=[secrets.token_hex(16) for _ in range(random.randint(1, 4))], job_url=job_url ) _add_to_queue( json.dumps(job.dict() | {"ignored_noise": "foobar"}), @@ -42,8 +62,12 @@ async def test_reactive_mode_consumes_jobs(ops_test: OpsTest, app_for_reactive: ) await reconcile(app_for_reactive, app_for_reactive.model) + + await wait_for_completion(run, conclusion="success") _assert_queue_is_empty(mongodb_uri, app_for_reactive.name) - await _assert_job_details_in_reactive_log(unit, jobs=[job]) + + # Call reconcile to enable cleanup of the runner + await reconcile(app_for_reactive, app_for_reactive.model) async def _get_mongodb_uri_from_integration_data(ops_test: OpsTest, unit: Unit) -> str | None: @@ -122,27 +146,3 @@ def _assert_queue_is_empty(mongodb_uri: str, queue_name: str): with Connection(mongodb_uri) as conn: with conn.SimpleQueue(queue_name) as simple_queue: assert simple_queue.qsize() == 0 - - -async def _assert_job_details_in_reactive_log(unit: Unit, jobs: list[JobDetails]): - """Assert that the job details are in the reactive log. - - Args: - unit: The juju unit. - jobs: The list of job details to check. - """ - retcode, stdout, stderr = await run_in_unit( - unit=unit, - command=f"ls {REACTIVE_RUNNER_LOG_DIR}", - ) - assert retcode == 0, f"Failed to list the reactive log dir: {stderr}" - assert stdout, "No log files found in the reactive log dir." - log_files = [log_file for log_file in stdout.split()] - - reactive_logs = "" - for log_file in log_files: - reactive_logs += await get_file_content(unit, REACTIVE_RUNNER_LOG_DIR / log_file) - - for job in jobs: - assert job.run_url in reactive_logs - assert str(job.labels) in reactive_logs diff --git a/tests/integration/test_runner_manager_openstack.py b/tests/integration/test_runner_manager_openstack.py index cb88d84ba..be7d324b8 100644 --- a/tests/integration/test_runner_manager_openstack.py +++ b/tests/integration/test_runner_manager_openstack.py @@ -31,7 +31,7 @@ from github_runner_manager.openstack_cloud.openstack_runner_manager import ( OpenStackCloudConfig, OpenStackRunnerManager, - OpenStackServerConfig, + OpenStackServerConfig, OpenStackRunnerManagerConfig, ) from github_runner_manager.types_.github import GitHubPath, parse_github_path from openstack.connection import Connection as OpenstackConnection @@ -133,8 +133,18 @@ async def openstack_runner_manager_fixture( ssh_debug_connections=None, repo_policy_compliance=None, ) + + openstack_runner_manager_config = OpenStackRunnerManagerConfig( + name=app_name, + prefix=f"{app_name}-0", + cloud_config=cloud_config, + server_config=server_config, + runner_config=runner_config, + service_config=service_config, + ) + return OpenStackRunnerManager( - app_name, f"{app_name}-0", cloud_config, server_config, runner_config, service_config + config=openstack_runner_manager_config, ) diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index b7df8a5dc..52b19781c 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -48,6 +48,7 @@ OpenstackImage, OpenstackRunnerConfig, ProxyConfig, + ReactiveConfig, RunnerStorage, SSHDebugConnection, UnsupportedArchitectureError, @@ -1288,3 +1289,38 @@ def test_charm_state__log_prev_state_redacts_sensitive_information( assert mock_charm_state_data["charm_config"]["token"] not in caplog.text assert charm_state.SENSITIVE_PLACEHOLDER in caplog.text + + +def test_charm_state_from_charm_reactive_with_lxd_raises_error(monkeypatch: pytest.MonkeyPatch): + """ + arrange: Mock CharmBase and necessary methods to enable reactive config and lxd storage. + act: Call CharmState.from_charm. + assert: Ensure an error is raised + """ + mock_charm = MockGithubRunnerCharmFactory() + mock_database = MagicMock(spec=DatabaseRequires) + + monkeypatch.setattr( + ReactiveConfig, + "from_database", + MagicMock(return_value=ReactiveConfig(mq_uri="mongodb://localhost:27017")), + ) + charm_config_mock = MagicMock() + charm_config_mock.openstack_clouds_yaml = None + monkeypatch.setattr(CharmConfig, "from_charm", MagicMock(return_value=charm_config_mock)) + + # mock all other required methods + monkeypatch.setattr(ProxyConfig, "from_charm", MagicMock()) + monkeypatch.setattr(OpenstackRunnerConfig, "from_charm", MagicMock()) + monkeypatch.setattr(LocalLxdRunnerConfig, "from_charm", MagicMock()) + monkeypatch.setattr(CharmState, "_check_immutable_config_change", MagicMock()) + monkeypatch.setattr(charm_state, "_get_supported_arch", MagicMock()) + monkeypatch.setattr(SSHDebugConnection, "from_charm", MagicMock()) + monkeypatch.setattr(json, "loads", MagicMock()) + monkeypatch.setattr(json, "dumps", MagicMock()) + monkeypatch.setattr(charm_state, "CHARM_STATE_PATH", MagicMock()) + + with pytest.raises(CharmConfigInvalidError) as exc: + CharmState.from_charm(mock_charm, mock_database) + + assert "Reactive mode not supported for local LXD instances" in str(exc.value) diff --git a/tests/unit/test_lxd_runner_manager.py b/tests/unit/test_lxd_runner_manager.py index 215cbe7e0..2c5aca46c 100644 --- a/tests/unit/test_lxd_runner_manager.py +++ b/tests/unit/test_lxd_runner_manager.py @@ -18,17 +18,10 @@ from github_runner_manager.metrics.runner import RUNNER_INSTALLED_TS_FILE_NAME from github_runner_manager.metrics.storage import MetricsStorage from github_runner_manager.types_.github import GitHubOrg, GitHubRepo, RunnerApplication -from pytest import LogCaptureFixture, MonkeyPatch +from pytest import MonkeyPatch import shared_fs -from charm_state import ( - Arch, - CharmConfig, - CharmState, - ProxyConfig, - ReactiveConfig, - VirtualMachineResources, -) +from charm_state import Arch, CharmConfig, CharmState, ProxyConfig, VirtualMachineResources from errors import IssueMetricEventError, RunnerBinaryError from runner import Runner, RunnerStatus from runner_manager import BUILD_IMAGE_SCRIPT_FILENAME, LXDRunnerManager, LXDRunnerManagerConfig @@ -524,26 +517,6 @@ def test_reconcile_places_no_timestamp_in_newly_created_runner_if_metrics_disabl assert not (fs.path / RUNNER_INSTALLED_TS_FILE_NAME).exists() -def test_reconcile_reactive_mode( - runner_manager: LXDRunnerManager, - reactive_reconcile_mock: MagicMock, - caplog: LogCaptureFixture, -): - """ - arrange: Enable reactive mode and mock the job class to return a job. - act: Call reconcile with a random quantity n. - assert: The mocked job is picked up n times and the expected log message is present. - """ - count = random.randint(0, 5) - runner_manager.config.reactive_config = ReactiveConfig(mq_uri=FAKE_MONGODB_URI) - actual_count = runner_manager.reconcile(count, VirtualMachineResources(2, "7GiB", "10Gib")) - - assert actual_count == count - reactive_reconcile_mock.assert_called_with( - quantity=count, mq_uri=FAKE_MONGODB_URI, queue_name=runner_manager.app_name - ) - - def test_schedule_build_runner_image( runner_manager: LXDRunnerManager, tmp_path: Path, diff --git a/tests/unit/test_runner_scaler.py b/tests/unit/test_runner_scaler.py index f3199fd99..5cfae4c4f 100644 --- a/tests/unit/test_runner_scaler.py +++ b/tests/unit/test_runner_scaler.py @@ -73,8 +73,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