From ff1a35df7781bca2410c046d9ae1e1e7aaf386b2 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 2 Jul 2025 05:14:05 +0000 Subject: [PATCH 01/50] feat: parallel deletion of runners --- .../github_runner_manager/manager/models.py | 3 + .../manager/runner_manager.py | 144 +++++++++++++----- 2 files changed, 113 insertions(+), 34 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/models.py b/github-runner-manager/src/github_runner_manager/manager/models.py index ef81f38175..6dd10430c6 100644 --- a/github-runner-manager/src/github_runner_manager/manager/models.py +++ b/github-runner-manager/src/github_runner_manager/manager/models.py @@ -13,6 +13,8 @@ class InstanceIDInvalidError(Exception): """Raised when the InstanceID naming will break the provider of GitHub.""" +# 20250702 TODO: The InstanceID should be renamed InstanceName and additionally +# have the platform information. @dataclass(eq=True, frozen=True, order=True) class InstanceID: """Main identifier for a runner instance among all clouds and GitHub. @@ -168,6 +170,7 @@ class RunnerMetadata: url: URL for the runner. """ + # 20250702 TODO: Supported platforms should be enumerated. platform_name: str = "github" runner_id: str | None = None url: str | None = None diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index 8f20fc0183..d6fdc2509c 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -104,6 +104,28 @@ def __init__( self.cloud_state = cloud_instance.state +@dataclass +class _DeleteRunnerConfig: + """Configuration for deleting runner. + + This dataclass is a wrapper to allow parallel deletion of runners via subprocess pool calls. + + Attributes: + instance_id: The cloud VM ID. + runner_id: The platform runner ID. + delete_busy: Whether to delete busy runners. + """ + + instance_id: InstanceID + # 2025/06/02 TODO: The platform should be encoded into a InstanceID. + platform: str | None + runner_id: str | None + delete_busy: bool + + platform_service: PlatformProvider + cloud_service: CloudRunnerManager + + class RunnerManager: """Manage the runners. @@ -317,42 +339,96 @@ def _delete_cloud_runners( If delete_busy_runners is False, when the platform provider fails in deleting the runner because it can be busy, will mean that that runner should not be deleted. """ - extracted_runner_metrics = [] - health_runners_map = {health.identity.instance_id: health for health in runners_health} - for cloud_runner in cloud_runners: - logging.info("Trying to delete cloud_runner %s", cloud_runner) - runner_health = health_runners_map.get(cloud_runner.instance_id) - if runner_health and runner_health.runner_in_platform: + runner_identity_map = { + health_info.identity.instance_id: health_info.identity + for health_info in runners_health + } + delete_runner_configs = [ + _DeleteRunnerConfig( + instance_id=runner.instance_id, + platform=( + runner_identity_map[runner.instance_id].metadata.platform_name + if runner.instance_id in runner_identity_map + else None + ), + runner_id=( + runner_identity_map[runner.instance_id].metadata.runner_id + if runner.instance_id in runner_identity_map + else None + ), + delete_busy=delete_busy_runners, + platform_service=self._platform, + cloud_service=self._cloud, + ) + for runner in cloud_runners + ] + extracted_runner_metrics: list[runner_metrics.RunnerMetrics] = [] + with Pool(processes=min(len(cloud_runners), 30)) as pool: + jobs = pool.imap_unordered( + func=RunnerManager._delete_cloud_runner, iterable=delete_runner_configs + ) + for _ in range(len(delete_runner_configs)): try: - self._platform.delete_runner(runner_health.identity) - except DeleteRunnerBusyError: - if not delete_busy_runners: - logger.warning( - "Skipping deletion as the runner is busy. %s", cloud_runner.instance_id - ) - continue - logger.info("Deleting busy runner: %s", cloud_runner.instance_id) - except PlatformApiError as exc: - if not delete_busy_runners: - logger.warning( - "Failed to delete platform runner %s. %s. Skipping.", - cloud_runner.instance_id, - exc, - ) - continue - logger.warning( - "Deleting runner: %s after platform failure %s.", - cloud_runner.instance_id, - exc, - ) + extracted_metrics = next(jobs) + except RunnerError: + logger.exception("Failed to delete a runner.") + except StopIteration: + break - logging.info("Delete runner in cloud: %s", cloud_runner.instance_id) - runner_metric = self._cloud.delete_runner(cloud_runner.instance_id) - if not runner_metric: - logger.error("No metrics returned after deleting %s", cloud_runner.instance_id) - else: - extracted_runner_metrics.append(runner_metric) - return extracted_runner_metrics + if extracted_metrics: + extracted_runner_metrics.append(extracted_metrics) + return tuple(extracted_runner_metrics) + + @staticmethod + def _delete_cloud_runner( + delete_runner_config: _DeleteRunnerConfig, + ) -> runner_metrics.RunnerMetrics | None: + logging.info("Deleting cloud runner: %s", delete_runner_config.runner_id) + if delete_runner_config.runner_id and delete_runner_config.platform: + logger.info( + "Deleting runner from platform: %s, %s", + delete_runner_config.platform, + delete_runner_config.runner_id, + ) + try: + delete_runner_config.platform_service.delete_runner( + runner_identity=RunnerIdentity( + instance_id=delete_runner_config.instance_id, + metadata=RunnerMetadata( + platform_name=delete_runner_config.platform, + runner_id=delete_runner_config.runner_id, + ), + ) + ) + except DeleteRunnerBusyError: + if not delete_runner_config.delete_busy: + logger.info( + "Skipped deletion of busy runner: %s", delete_runner_config.runner_id + ) + return None + logger.info( + "Busy runner scheduled for VM deletion: %s", delete_runner_config.runner_id + ) + except PlatformApiError: + logger.exception( + "Failed to delete runner in platform. Runner: %s, delete_busy: %s", + delete_runner_config.runner_id, + delete_runner_config.delete_busy, + ) + return None + logger.info( + "Deleted runner from platform: %s, %s", + delete_runner_config.platform, + delete_runner_config.runner_id, + ) + logger.info("Deleting cloud VM: %s", delete_runner_config.instance_id) + extracted_metrics = delete_runner_config.cloud_service.delete_runner( + instance_id=delete_runner_config.instance_id + ) + logger.info("Deleted cloud VM: %s", delete_runner_config.instance_id) + if not extracted_metrics: + logger.warning("No metrics extracted from runner: %s", delete_runner_config.runner_id) + return extracted_metrics def _clean_platform_runners(self, runners: list[RunnerIdentity]) -> None: """Clean the specified runners in the platform.""" From 8a8667329c6bbe2c5fe25d9251afd2c37bf14c08 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:00:44 +0000 Subject: [PATCH 02/50] chore: minor refactor for imports --- .../src/github_runner_manager/github_client.py | 6 +----- .../src/github_runner_manager/manager/runner_scaler.py | 5 +---- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/github_client.py b/github-runner-manager/src/github_runner_manager/github_client.py index dba153df10..a54d0e0336 100644 --- a/github-runner-manager/src/github_runner_manager/github_client.py +++ b/github-runner-manager/src/github_runner_manager/github_client.py @@ -23,11 +23,7 @@ from requests import RequestException from typing_extensions import assert_never -from github_runner_manager.configuration.github import ( - GitHubOrg, - GitHubPath, - GitHubRepo, -) +from github_runner_manager.configuration.github import GitHubOrg, GitHubPath, GitHubRepo from github_runner_manager.manager.models import InstanceID from github_runner_manager.platform.platform_provider import ( DeleteRunnerBusyError, diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py b/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py index 34efa53e79..5c0fbc356e 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py @@ -8,10 +8,7 @@ from dataclasses import dataclass import github_runner_manager.reactive.runner_manager as reactive_runner_manager -from github_runner_manager.configuration import ( - ApplicationConfiguration, - UserInfo, -) +from github_runner_manager.configuration import ApplicationConfiguration, UserInfo from github_runner_manager.constants import GITHUB_SELF_HOSTED_ARCH_LABELS from github_runner_manager.errors import ( CloudError, From f70eb62c11e57f21b532029874b82561d2b7728a Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:01:24 +0000 Subject: [PATCH 03/50] feat: add delete vms and extract metrics interface --- .../manager/cloud_runner_manager.py | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py index 7cd7641110..a724210aaa 100644 --- a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py @@ -282,6 +282,32 @@ def delete_runner(self, instance_id: InstanceID) -> RunnerMetrics | None: instance_id: The instance id of the runner to delete. """ + @abc.abstractmethod + def delete_vms(self, instance_ids: Sequence[InstanceID]) -> list[InstanceID]: + """Delete cloud VM instances. + + Args: + instance_ids: The ID of the VMs to request deletion. + + Returns: + The deleted instance IDs. + """ + + @abc.abstractmethod + def extract_metrics(self, instance_ids: Sequence[InstanceID]) -> list[RunnerMetrics]: + """Extract metrics from cloud VMs. + + 2025/07/03 TODO: This method should really live in another metrics extractor class (that + doesn't exist yet). Hence, this method is subject to refactor when the caller classes are + tidied up. + + Args: + instance_ids: The VM instance IDs to fetch the metrics from. + + Returns: + The fetched runner metrics. + """ + @abc.abstractmethod def cleanup(self) -> None: """Cleanup runner dangling resources on the cloud.""" From 2094b7b04982831a010c6455c4c272a3742b6e4b Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:01:32 +0000 Subject: [PATCH 04/50] feat: add delete runners interface --- .../github_runner_manager/platform/platform_provider.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/github-runner-manager/src/github_runner_manager/platform/platform_provider.py b/github-runner-manager/src/github_runner_manager/platform/platform_provider.py index c0a9c63293..6e47e08f84 100644 --- a/github-runner-manager/src/github_runner_manager/platform/platform_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/platform_provider.py @@ -80,6 +80,14 @@ def delete_runner(self, runner_identity: RunnerIdentity) -> None: runner_identity: Runner to delete. """ + @abc.abstractmethod + def delete_runners(self, runner_ids: list[str]) -> list[str]: + """Delete runners. + + Args: + runner_ids: Runner IDs to delete. + """ + @abc.abstractmethod def get_runner_context( self, metadata: RunnerMetadata, instance_id: InstanceID, labels: list[str] From a4a412022bc8f9cec383f6dc79ebd52afa7087f4 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:07:58 +0000 Subject: [PATCH 05/50] feat: delete runner controller implementation --- .../manager/runner_manager.py | 146 +++++------------- 1 file changed, 41 insertions(+), 105 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index d6fdc2509c..8205987cf1 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -104,28 +104,6 @@ def __init__( self.cloud_state = cloud_instance.state -@dataclass -class _DeleteRunnerConfig: - """Configuration for deleting runner. - - This dataclass is a wrapper to allow parallel deletion of runners via subprocess pool calls. - - Attributes: - instance_id: The cloud VM ID. - runner_id: The platform runner ID. - delete_busy: Whether to delete busy runners. - """ - - instance_id: InstanceID - # 2025/06/02 TODO: The platform should be encoded into a InstanceID. - platform: str | None - runner_id: str | None - delete_busy: bool - - platform_service: PlatformProvider - cloud_service: CloudRunnerManager - - class RunnerManager: """Manage the runners. @@ -334,101 +312,59 @@ def _delete_cloud_runners( runners_health: Sequence[PlatformRunnerHealth], delete_busy_runners: bool = False, ) -> Iterable[runner_metrics.RunnerMetrics]: - """Delete runners in the platform ant the cloud. + """Delete runners in the platform and the cloud. If delete_busy_runners is False, when the platform provider fails in deleting the runner because it can be busy, will mean that that runner should not be deleted. + + Runners without health information should not be deleted. """ + if not cloud_runners: + return [] + runner_identity_map = { health_info.identity.instance_id: health_info.identity for health_info in runners_health } - delete_runner_configs = [ - _DeleteRunnerConfig( - instance_id=runner.instance_id, - platform=( - runner_identity_map[runner.instance_id].metadata.platform_name - if runner.instance_id in runner_identity_map - else None - ), - runner_id=( - runner_identity_map[runner.instance_id].metadata.runner_id - if runner.instance_id in runner_identity_map - else None - ), - delete_busy=delete_busy_runners, - platform_service=self._platform, - cloud_service=self._cloud, - ) + platform_runner_ids_to_delete = [ + # The runner_id cannot be None due to the if condition. the type system + # isn't able to catch that. + cast(str, runner_identity_map[runner.instance_id].metadata.runner_id) for runner in cloud_runners + if runner.instance_id in runner_identity_map + and runner_identity_map[runner.instance_id].metadata.runner_id ] - extracted_runner_metrics: list[runner_metrics.RunnerMetrics] = [] - with Pool(processes=min(len(cloud_runners), 30)) as pool: - jobs = pool.imap_unordered( - func=RunnerManager._delete_cloud_runner, iterable=delete_runner_configs - ) - for _ in range(len(delete_runner_configs)): - try: - extracted_metrics = next(jobs) - except RunnerError: - logger.exception("Failed to delete a runner.") - except StopIteration: - break - - if extracted_metrics: - extracted_runner_metrics.append(extracted_metrics) - return tuple(extracted_runner_metrics) + logger.info("Deleting runners from platform: %s", platform_runner_ids_to_delete) + deleted_runner_ids = self._platform.delete_runners( + runner_ids=platform_runner_ids_to_delete + ) + logger.info( + "Deleted runners from platform: %s (diff: %s)", + deleted_runner_ids, + set(platform_runner_ids_to_delete) - set(deleted_runner_ids), + ) - @staticmethod - def _delete_cloud_runner( - delete_runner_config: _DeleteRunnerConfig, - ) -> runner_metrics.RunnerMetrics | None: - logging.info("Deleting cloud runner: %s", delete_runner_config.runner_id) - if delete_runner_config.runner_id and delete_runner_config.platform: - logger.info( - "Deleting runner from platform: %s, %s", - delete_runner_config.platform, - delete_runner_config.runner_id, - ) - try: - delete_runner_config.platform_service.delete_runner( - runner_identity=RunnerIdentity( - instance_id=delete_runner_config.instance_id, - metadata=RunnerMetadata( - platform_name=delete_runner_config.platform, - runner_id=delete_runner_config.runner_id, - ), - ) - ) - except DeleteRunnerBusyError: - if not delete_runner_config.delete_busy: - logger.info( - "Skipped deletion of busy runner: %s", delete_runner_config.runner_id - ) - return None - logger.info( - "Busy runner scheduled for VM deletion: %s", delete_runner_config.runner_id - ) - except PlatformApiError: - logger.exception( - "Failed to delete runner in platform. Runner: %s, delete_busy: %s", - delete_runner_config.runner_id, - delete_runner_config.delete_busy, - ) - return None - logger.info( - "Deleted runner from platform: %s, %s", - delete_runner_config.platform, - delete_runner_config.runner_id, - ) - logger.info("Deleting cloud VM: %s", delete_runner_config.instance_id) - extracted_metrics = delete_runner_config.cloud_service.delete_runner( - instance_id=delete_runner_config.instance_id + cloud_vm_ids_to_delete = [ + runner.instance_id + for runner in cloud_runners + # We can delete all VMs if delete_busy_runners is True + if delete_busy_runners + # We can delete the VM if no runner is associated with it + or not runner.metadata.runner_id + # We can delete the VM if it has been deleted from the Platform provider. + or runner.metadata.runner_id in deleted_runner_ids + ] + logger.info("Extracting metrics from cloud VMs: %s", cloud_vm_ids_to_delete) + extracted_metrics = self._cloud.extract_metrics(instance_ids=cloud_vm_ids_to_delete) + logger.info("Extracted metrics from cloud VMs.") + logger.info("Deleting VMs %s", cloud_vm_ids_to_delete) + deleted_vm_ids = self._cloud.delete_vms(instance_ids=cloud_vm_ids_to_delete) + logger.info( + "Deleted VMs: %s, (diff: %s)", + deleted_vm_ids, + set(cloud_vm_ids_to_delete) - set(deleted_vm_ids), ) - logger.info("Deleted cloud VM: %s", delete_runner_config.instance_id) - if not extracted_metrics: - logger.warning("No metrics extracted from runner: %s", delete_runner_config.runner_id) - return extracted_metrics + return tuple(extracted_metrics) def _clean_platform_runners(self, runners: list[RunnerIdentity]) -> None: """Clean the specified runners in the platform.""" From f0dcf64d0776a5a413e7780d73b627bba7689a5c Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:08:35 +0000 Subject: [PATCH 06/50] feat: delete runner implementation for GitHub platform --- .../platform/github_provider.py | 78 ++++++++++++++++++- 1 file changed, 75 insertions(+), 3 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index 247979df5d..c9b4ab5710 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -4,12 +4,18 @@ """Client for managing self-hosted runner on GitHub side.""" import logging +import multiprocessing +from dataclasses import dataclass from enum import Enum from pydantic import HttpUrl -from github_runner_manager.configuration.github import GitHubConfiguration, GitHubRepo -from github_runner_manager.github_client import GithubClient, GithubRunnerNotFoundError +from github_runner_manager.configuration.github import GitHubConfiguration, GitHubPath, GitHubRepo +from github_runner_manager.github_client import ( + DeleteRunnerBusyError, + GithubClient, + GithubRunnerNotFoundError, +) from github_runner_manager.manager.models import ( InstanceID, RunnerContext, @@ -28,10 +34,25 @@ logger = logging.getLogger(__name__) +@dataclass +class _DeleteRunnerConfig: + """Configurations for deleting a runner. + + Attributes: + runner_id: The ID of the runner to delete. + path: The path (repository/org) in which the the runner was registered to. + github_client: The GitHub client to use to call delete runner. + """ + + runner_id: str + path: GitHubPath + github_client: GithubClient + + class GitHubRunnerPlatform(PlatformProvider): """Manage self-hosted runner on GitHub side.""" - def __init__(self, prefix: str, path: str, github_client: GithubClient): + def __init__(self, prefix: str, path: GitHubPath, github_client: GithubClient): """Construct the object. Args: @@ -39,6 +60,7 @@ def __init__(self, prefix: str, path: str, github_client: GithubClient): path: GitHub path. github_client: GitHub client. """ + self._prefix = prefix self._path = path self._client = github_client @@ -155,6 +177,56 @@ def delete_runner(self, runner_identity: RunnerIdentity) -> None: logger.info("Delete runner in GitHub: %s", runner_identity) self._client.delete_runner(self._path, int(runner_identity.metadata.runner_id)) + def delete_runners(self, runner_ids: list[str]) -> list[str]: + """Delete runners from GitHub. + + This method will ignore DeleteRunnerBusyErrors and print a warning log. + + Args: + runner_ids: The GitHub runner IDs to delete. + + Returns: + The runner IDs that were deleted successfully. + """ + logger.info("Delete runners from GitHub provider: %s", runner_ids) + # Guard multiprocessing.Pool from having 0 processes which will raise an error. + if not runner_ids: + return [] + + delete_configs = [ + _DeleteRunnerConfig(runner_id=runner_id, path=self._path, github_client=self._client) + for runner_id in runner_ids + ] + deleted_runner_ids: list[str] = [] + with multiprocessing.Pool(min(len(runner_ids), 30)) as pool: + for deleted_runner_id in pool.imap_unordered( + GitHubRunnerPlatform._delete_runner, delete_configs + ): + if not deleted_runner_id: + continue + deleted_runner_ids.append(deleted_runner_id) + return deleted_runner_ids + + @staticmethod + def _delete_runner(delete_runner_config: _DeleteRunnerConfig) -> str | None: + """Delete a single runner from GitHub. + + This method is a wrapper to be called via multiprocessing pool for parallel deletion. + + Args: + delete_runner_config: The configuration to use for deleting the runner. + + Returns: + The runner ID of the deleted runner + """ + try: + delete_runner_config.github_client.delete_runner( + path=delete_runner_config.path, runner_id=int(delete_runner_config.runner_id) + ) + except DeleteRunnerBusyError: + return None + return delete_runner_config.runner_id + def get_runner_context( self, metadata: RunnerMetadata, instance_id: InstanceID, labels: list[str] ) -> tuple[RunnerContext, SelfHostedRunner]: From 1f1f730e852be38fb39643bdc5a6ebf6e1d6efe8 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:09:43 +0000 Subject: [PATCH 07/50] feat: delete runner implementation on jobmanager --- .../platform/jobmanager_provider.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py index abf8a2641e..f6654d9eee 100644 --- a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py @@ -152,6 +152,17 @@ def delete_runner(self, runner_identity: RunnerIdentity) -> None: """ logger.debug("No need to delete runners in the jobmanager.") + def delete_runners(self, runner_ids: list[str]) -> list[str]: + """Delete a runner from jobmanager. + + This method does nothing, as the jobmanager does not implement it. + + Args: + runner_ids: The runner IDs to delete. + """ + logger.debug("No need to delete runners in the jobmanager.") + return runner_ids + def get_runner_context( self, metadata: RunnerMetadata, instance_id: InstanceID, labels: list[str] ) -> tuple[RunnerContext, SelfHostedRunner]: From cb4cbfcbb4353b0cfde1db2d8fa59b0ac19c4363 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 08:26:36 +0000 Subject: [PATCH 08/50] feat: parallel vm deletion and metrics extraction --- .../github_runner_manager/metrics/runner.py | 149 +++++++++++++---- .../openstack_cloud/openstack_cloud.py | 156 +++++++++++++++--- .../openstack_runner_manager.py | 56 +++++-- 3 files changed, 295 insertions(+), 66 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/metrics/runner.py b/github-runner-manager/src/github_runner_manager/metrics/runner.py index 6426819f1a..826cde2989 100644 --- a/github-runner-manager/src/github_runner_manager/metrics/runner.py +++ b/github-runner-manager/src/github_runner_manager/metrics/runner.py @@ -6,10 +6,10 @@ import io import json import logging +import multiprocessing from dataclasses import dataclass -from datetime import datetime from json import JSONDecodeError -from typing import Optional, Type +from typing import Optional, Sequence, Type import paramiko import paramiko.ssh_exception @@ -18,7 +18,6 @@ from github_runner_manager.errors import IssueMetricEventError, RunnerMetricsError, SSHError from github_runner_manager.manager.cloud_runner_manager import ( - CloudRunnerInstance, PostJobMetrics, PreJobMetrics, RunnerMetrics, @@ -31,6 +30,7 @@ PRE_JOB_METRICS_FILE_NAME, RUNNER_INSTALLED_TS_FILE_NAME, ) +from github_runner_manager.openstack_cloud.openstack_cloud import OpenstackCloud, OpenstackInstance logger = logging.getLogger(__name__) @@ -41,42 +41,119 @@ class PullFileError(Exception): """Represents an error while pulling a file from the runner instance.""" -def pull_runner_metrics(instance_id: InstanceID, ssh_conn: SSHConnection) -> "PulledMetrics": +@dataclass +class _PullRunnerMetricsConfig: + """Configurations for pulling runner metrics from a VM. + + Attributes: + cloud_service: The OpenStack cloud service. + instance_id: The instance ID to fetch the runner metric from. + """ + + cloud_service: OpenstackCloud + instance_id: InstanceID + + +# 2025/07/03 TODO: This should really be a service class with OpenStack service injected. The +# interface will accept the openstack_service and instance_id to reduce the scope of refactoring. +def pull_runner_metrics( + cloud_service: OpenstackCloud, instance_ids: Sequence[InstanceID] +) -> "list[PulledMetrics]": """Pull metrics from runner. + This function uses multiprocessing to fetch metrics in parallel. + Args: - instance_id: The name of the runner. ssh_conn: The SSH connection to the runner. Returns: Metrics pulled from the instance. """ - logger.debug("Pulling metrics for %s", instance_id) - pulled_metrics = PulledMetrics() + if not instance_ids: + return [] + pull_metrics_configs = [ + _PullRunnerMetricsConfig(cloud_service=cloud_service, instance_id=instance_id) + for instance_id in instance_ids + ] + pulled_metrics: list[PulledMetrics] = [] + with multiprocessing.Pool(min(len(instance_ids), 10)) as pool: + for metrics in pool.imap_unordered(_pull_runner_metrics, pull_metrics_configs): + if not metrics: + continue + pulled_metrics.append(metrics) + return pulled_metrics - try: - pulled_metrics.runner_installed = ssh_pull_file( - ssh_conn=ssh_conn, - remote_path=str(RUNNER_INSTALLED_TS_FILE_NAME), - max_size=MAX_METRICS_FILE_SIZE, - ) - pulled_metrics.pre_job_metrics = ssh_pull_file( - ssh_conn=ssh_conn, - remote_path=str(PRE_JOB_METRICS_FILE_NAME), - max_size=MAX_METRICS_FILE_SIZE, - ) - pulled_metrics.post_job_metrics = ssh_pull_file( - ssh_conn=ssh_conn, - remote_path=str(POST_JOB_METRICS_FILE_NAME), - max_size=MAX_METRICS_FILE_SIZE, + +def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetrics | None": + """Pull metrics from a single runner via SSH file pull. + + Args: + pull_runner_metrics_config: Configurations for pulling the runner metrics. + + Returns: + PulledMetrics if metrics were available. None otherwise. + """ + instance = pull_config.cloud_service.get_instance(instance_id=pull_config.instance_id) + if not instance: + logger.warning( + "Skipping fetching metrics, instance not found: %s", pull_config.instance_id ) - except PullFileError as exc: + return None + + pulled_metrics = PulledMetrics(instance=instance) + try: + with pull_config.cloud_service.get_ssh_connection(instance=instance) as ssh_conn: + try: + pulled_metrics.runner_installed = ssh_pull_file( + ssh_conn=ssh_conn, + remote_path=str(RUNNER_INSTALLED_TS_FILE_NAME), + max_size=MAX_METRICS_FILE_SIZE, + ) + except PullFileError as exc: + logger.warning( + "Failed to pull runner_installed metrics for %s: %s.", + pull_config.instance_id, + exc, + ) + try: + pulled_metrics.pre_job_metrics = ssh_pull_file( + ssh_conn=ssh_conn, + remote_path=str(PRE_JOB_METRICS_FILE_NAME), + max_size=MAX_METRICS_FILE_SIZE, + ) + except PullFileError as exc: + logger.warning( + "Failed to pull pre_job metrics for %s: %s.", + pull_config.instance_id, + exc, + ) + try: + pulled_metrics.post_job_metrics = ssh_pull_file( + ssh_conn=ssh_conn, + remote_path=str(POST_JOB_METRICS_FILE_NAME), + max_size=MAX_METRICS_FILE_SIZE, + ) + except PullFileError as exc: + logger.warning( + "Failed to pull post_job metrics for %s: %s.", + pull_config.instance_id, + exc, + ) + except SSHError: logger.warning( - "Failed to pull metrics for %s: %s . Will not be able to issue all metrics", - instance_id, - exc, + "Failed to create SSH connection for pulling metrics: %s", instance.instance_id ) - return pulled_metrics + return None + + return ( + pulled_metrics + if ( + pulled_metrics.runner_installed + or pulled_metrics.pre_job_metrics + or pulled_metrics.post_job_metrics + ) + else None + ) def ssh_pull_file(ssh_conn: SSHConnection, remote_path: str, max_size: int) -> str: @@ -145,28 +222,27 @@ class PulledMetrics: """Metrics pulled from a runner. Attributes: + instance_id: The instance in which the metrics were pulled from. runner_installed: String with the runner-installed file. pre_job_metrics: String with the pre-job-metrics file. post_job_metrics: String with the post-job-metrics file. """ + instance: OpenstackInstance runner_installed: str | None = None pre_job_metrics: str | None = None post_job_metrics: str | None = None - def to_runner_metrics( - self, instance: CloudRunnerInstance, installation_start: datetime - ) -> RunnerMetrics | None: + def to_runner_metrics(self) -> RunnerMetrics | None: """. Args: instance: Cloud runner instance. - installation_start: Creation time of the runner. Returns: The RunnerMetrics object for the runner or None if it can not be built. """ - instance_id = instance.instance_id + instance_id = self.instance.instance_id if self.runner_installed is None: logger.error( "Invalid pulled metrics. No runner_installed information for %s.", instance_id @@ -203,7 +279,7 @@ def to_runner_metrics( try: return RunnerMetrics( - installation_start_timestamp=installation_start.timestamp(), + installation_start_timestamp=self.instance.created_at.timestamp(), installed_timestamp=float(self.runner_installed), pre_job=( # pylint: disable=not-a-mapping PreJobMetrics(**pre_job_metrics) if pre_job_metrics else None @@ -212,11 +288,14 @@ def to_runner_metrics( PostJobMetrics(**post_job_metrics) if post_job_metrics else None ), instance_id=instance_id, - metadata=instance.metadata, + metadata=self.instance.metadata, ) except ValueError: logger.exception( - "Error creating RunnerMetrics %s, %s, %s", instance_id, installation_start, self + "Error creating RunnerMetrics %s, %s, %s", + instance_id, + self.instance.created_at, + self, ) return None diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 19e24038f5..3e42a15812 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -6,13 +6,14 @@ import copy import functools import logging +import multiprocessing import shutil from contextlib import contextmanager from dataclasses import dataclass from datetime import datetime, timezone from functools import reduce from pathlib import Path -from typing import Any, Callable, Iterable, Iterator, ParamSpec, TypeVar, cast +from typing import Any, Callable, Iterable, Iterator, ParamSpec, Sequence, TypeVar, cast import keystoneauth1.exceptions import openstack @@ -180,6 +181,40 @@ def _get_openstack_connection(credentials: OpenStackCredentials) -> Iterator[Ope yield conn +@dataclass +class _DeleteVMConfig: + """Configurations for deleting a VM. + + Attributes: + instance_id: The ID of the VM to request deletion. + conn: The OpenStack connection instance. + keys_dir: The path to the directory in which the SSH key files are stored. + wait: Whether to wait for the VM delete to complete. + timeout: Timeout in seconds for VM deletion to complete. + """ + + instance_id: InstanceID + conn: OpenstackConnection + keys_dir: Path + wait: bool = False + timeout: int = 10 * 60 + + +@dataclass +class _DeleteKeypairConfig: + """Configurations for deleting an OpenStack keypair. + + Attributes: + keys_dir: The path to the directory in which the SSH key files are stored. + instance_id: The instance ID of the key owner. + conn: The OpenStack connection instance. + """ + + keys_dir: Path + instance_id: InstanceID + conn: OpenstackConnection + + class OpenstackCloud: """Client to interact with OpenStack cloud. @@ -262,11 +297,17 @@ def launch_instance( "Attempting clean up of openstack server %s that timeout during creation", instance_id, ) - self._delete_instance(conn, instance_id) + OpenstackCloud._delete_instance( + _DeleteVMConfig(instance_id=instance_id, conn=conn, keys_dir=self._ssh_key_dir) + ) raise OpenStackError(f"Timeout creating openstack server {instance_id}") from err except openstack.exceptions.SDKException as err: logger.exception("Failed to create openstack server %s", instance_id) - self._delete_keypair(conn, instance_id) + OpenstackCloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=self._ssh_key_dir, instance_id=instance_id, conn=conn + ) + ) raise OpenStackError(f"Failed to create openstack server {instance_id}") from err return OpenstackInstance(server, self.prefix) @@ -299,9 +340,12 @@ def delete_instance(self, instance_id: InstanceID) -> None: logger.info("Deleting openstack server with %s", instance_id) with _get_openstack_connection(credentials=self._credentials) as conn: - self._delete_instance(conn, instance_id) + OpenstackCloud._delete_instance( + _DeleteVMConfig(instance_id=instance_id, conn=conn, keys_dir=self._ssh_key_dir) + ) - def _delete_instance(self, conn: OpenstackConnection, instance_id: InstanceID) -> None: + @staticmethod + def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: """Delete a openstack instance. Raises: @@ -312,14 +356,69 @@ def _delete_instance(self, conn: OpenstackConnection, instance_id: InstanceID) - instance_id: The full name of the server. """ try: - res = conn.delete_server(name_or_id=instance_id.name) - logger.info("openstack delete result for %s: %s", instance_id, res) - self._delete_keypair(conn, instance_id) + logger.info("Deleting server %s", delete_config.instance_id.name) + res = delete_config.conn.delete_server(name_or_id=delete_config.instance_id.name) + logger.info("Deleted server %s: %s", delete_config.instance_id.name, res) except ( openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout, - ) as err: - raise OpenStackError(f"Failed to remove openstack runner {instance_id}") from err + ): + logger.exception( + "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name + ) + return None + + OpenstackCloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=delete_config.keys_dir, + instance_id=delete_config.instance_id, + conn=delete_config.conn, + ) + ) + return delete_config.instance_id if res else None + + def delete_instances( + self, instance_ids: Sequence[InstanceID], wait: bool = False, timeout: int = 60 * 10 + ) -> list[InstanceID]: + """Delete Openstack VM instances. + + Args: + instance_ids: The VM instance IDs to requeest deletion. + wait: Whether to wait for VM deletion to complete. + timeout: Timeout in seconds to wait for VM deletion to complete. + + Returns: + The deleted VM instance IDs if wait is True, deleted requested VM instance IDs + otherwise. + """ + deleted_instance_ids: list[InstanceID] = [] + + # Guard no instance IDs since multiprocessing Pool may raise an exception. + if not instance_ids: + return deleted_instance_ids + + with ( + _get_openstack_connection(credentials=self._credentials) as conn, + multiprocessing.Pool(min(len(instance_ids), 30)) as pool, + ): + delete_configs = [ + _DeleteVMConfig( + instance_id=instance_id, + conn=conn, + keys_dir=self._ssh_key_dir, + wait=wait, + timeout=timeout, + ) + for instance_id in instance_ids + ] + for deleted_instance_id in pool.imap_unordered( + OpenstackCloud._delete_instance, delete_configs + ): + if not deleted_instance_id: + continue + deleted_instance_ids.append(deleted_instance_id) + + return deleted_instance_ids @_catch_openstack_errors @contextlib.contextmanager @@ -336,7 +435,7 @@ def get_ssh_connection(self, instance: OpenstackInstance) -> Iterator[SSHConnect Yields: SSH connection object. """ - key_path = self._get_key_path(instance.instance_id.name) + key_path = self._get_key_path(instance.instance_id) if not key_path.exists(): raise KeyfileError( @@ -485,7 +584,13 @@ def _cleanup_openstack_keypairs( if str(key.name) in exclude_keys: continue try: - self._delete_keypair(conn, InstanceID.build_from_name(self.prefix, key.name)) + OpenstackCloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=self._ssh_key_dir, + instance_id=InstanceID.build_from_name(self.prefix, key.name), + conn=conn, + ) + ) except openstack.exceptions.SDKException: logger.warning( "Unable to delete OpenStack keypair associated with deleted key file %s ", @@ -592,23 +697,34 @@ def _setup_keypair( key_path.chmod(0o400) return keypair - def _delete_keypair(self, conn: OpenstackConnection, instance_id: InstanceID) -> None: + @staticmethod + def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> str | None: """Delete OpenStack keypair. Args: - conn: The connection object to access OpenStack cloud. - instance_id: The name of the keypair. + delete_keypair_config: Configurations for deleting the KeyPair. + + Returns: + Name of the successfully deleted key. None otherwise. """ - logger.debug("Deleting keypair for %s", instance_id) + logger.info("Deleting key: %s", delete_keypair_config.instance_id) try: # Keypair have unique names, access by ID is not needed. - if not conn.delete_keypair(instance_id.name): - logger.warning("Unable to delete keypair for %s", instance_id) + if not delete_keypair_config.conn.delete_keypair( + delete_keypair_config.instance_id.name + ): + logger.warning("Failed to delete key: %s", delete_keypair_config.instance_id.name) + return None except (openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout): - logger.warning("Unable to delete keypair for %s", instance_id, stack_info=True) + logger.warning( + "Failed to delete key: %s", delete_keypair_config.instance_id.name, stack_info=True + ) + return None - key_path = self._get_key_path(instance_id.name) + key_path = delete_keypair_config.keys_dir / f"{delete_keypair_config.instance_id}.key" key_path.unlink(missing_ok=True) + logger.info("Deleted key: %s", delete_keypair_config.instance_id) + return delete_keypair_config.instance_id.name @staticmethod def _ensure_security_group( diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index bab7672508..8a71d5c6bf 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -22,12 +22,9 @@ CloudRunnerInstance, CloudRunnerManager, CloudRunnerState, + RunnerMetrics, ) -from github_runner_manager.manager.models import ( - InstanceID, - RunnerContext, - RunnerIdentity, -) +from github_runner_manager.manager.models import InstanceID, RunnerContext, RunnerIdentity from github_runner_manager.manager.runner_manager import HealthState from github_runner_manager.metrics import runner as runner_metrics from github_runner_manager.openstack_cloud.constants import ( @@ -202,8 +199,7 @@ def delete_runner(self, instance_id: InstanceID) -> runner_metrics.RunnerMetrics ) logger.debug("Instance deleted successfully %s %s", instance_id, instance.instance_id) logger.debug("Extract metrics for runner %s %s", instance_id, instance.instance_id) - cloud_instance = self._build_cloud_runner_instance(instance) - return pulled_metrics.to_runner_metrics(cloud_instance, instance.created_at) + return pulled_metrics.to_runner_metrics() def _delete_runner(self, instance: OpenstackInstance) -> runner_metrics.PulledMetrics: """Delete self-hosted runners by openstack instance. @@ -211,10 +207,10 @@ def _delete_runner(self, instance: OpenstackInstance) -> runner_metrics.PulledMe Args: instance: The OpenStack instance. """ - pulled_metrics = runner_metrics.PulledMetrics() try: - with self._openstack_cloud.get_ssh_connection(instance) as ssh_conn: - pulled_metrics = runner_metrics.pull_runner_metrics(instance.instance_id, ssh_conn) + pulled_metrics = runner_metrics.pull_runner_metrics( + cloud_service=self._openstack_cloud, instance_ids=[instance.instance_id] + ) except SSHError: logger.exception( "Failed to get SSH connection while removing %s", instance.instance_id @@ -229,7 +225,11 @@ def _delete_runner(self, instance: OpenstackInstance) -> runner_metrics.PulledMe logger.exception( "Unable to delete openstack instance for runner %s", instance.instance_id ) - return pulled_metrics + # 2025/07/03 TODO: This is done to keep the previous implementation which instantiates + # PulledMetrics and then assigns it. It is weird but it was what it was. + if not pulled_metrics: + return runner_metrics.PulledMetrics(instance=instance) + return pulled_metrics[0] def _generate_cloud_init(self, runner_context: RunnerContext) -> str: """Generate cloud init userdata. @@ -308,3 +308,37 @@ def _get_repo_policy_compliance_client(self) -> RepoPolicyComplianceClient | Non service_config.repo_policy_compliance.token, ) return None + + def delete_vms( + self, instance_ids: Sequence[InstanceID], wait: bool = False, timeout: int = 60 * 10 + ) -> list[InstanceID]: + """Delete VMs. + + Args: + instance_ids: The ID of the VMs to request deletion. + wait: Whether to wait for the delete to be complete. + timeout: Timeout in seconds to wait for the deletion to complete. + + Returns: + The instance IDs requested for deletion. + """ + return self._openstack_cloud.delete_instances( + instance_ids=instance_ids, wait=wait, timeout=timeout + ) + + def extract_metrics(self, instance_ids: Sequence[InstanceID]) -> list[RunnerMetrics]: + """Extract metrics from cloud VMs. + + Args: + instance_ids: The ID of the VMs to fetch metrics from. + + Returns: + Metrics from VMs. + """ + return [ + converted_metrics + for pulled_metrics in runner_metrics.pull_runner_metrics( + cloud_service=self._openstack_cloud, instance_ids=instance_ids + ) + if (converted_metrics := pulled_metrics.to_runner_metrics()) + ] From fbb96d52bb23a653187341ad9a9f8b681a0f5a6f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 12:20:37 +0000 Subject: [PATCH 09/50] feat: cloud manager + cloud delete vms implementation --- .../openstack_cloud/openstack_cloud.py | 16 +- .../openstack_runner_manager.py | 57 ------ .../openstack_cloud/test_openstack_cloud.py | 128 ++++++++++++ .../test_openstack_runner_manager.py | 185 ++---------------- 4 files changed, 158 insertions(+), 228 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 3e42a15812..df271b2abf 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -348,17 +348,16 @@ def delete_instance(self, instance_id: InstanceID) -> None: def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: """Delete a openstack instance. - Raises: - OpenStackError: Unable to delete OpenStack server. - Args: - conn: The openstack connection to use. - instance_id: The full name of the server. + delete_config: The configuration used to delete a cloud VM instance. + + Returns: + The deleted Instance ID. """ try: logger.info("Deleting server %s", delete_config.instance_id.name) res = delete_config.conn.delete_server(name_or_id=delete_config.instance_id.name) - logger.info("Deleted server %s: %s", delete_config.instance_id.name, res) + logger.info("Deleted server %s (true delete: %s)", delete_config.instance_id.name, res) except ( openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout, @@ -397,6 +396,7 @@ def delete_instances( if not instance_ids: return deleted_instance_ids + print(multiprocessing.Pool) with ( _get_openstack_connection(credentials=self._credentials) as conn, multiprocessing.Pool(min(len(instance_ids), 30)) as pool, @@ -717,7 +717,9 @@ def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> str | None: return None except (openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout): logger.warning( - "Failed to delete key: %s", delete_keypair_config.instance_id.name, stack_info=True + "Error attempting to delete key: %s", + delete_keypair_config.instance_id.name, + stack_info=True, ) return None diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index 8a71d5c6bf..a6c23d4894 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -16,7 +16,6 @@ MissingServerConfigError, OpenStackError, RunnerCreateError, - SSHError, ) from github_runner_manager.manager.cloud_runner_manager import ( CloudRunnerInstance, @@ -175,62 +174,6 @@ def _build_cloud_runner_instance( created_at=instance.created_at, ) - def delete_runner(self, instance_id: InstanceID) -> runner_metrics.RunnerMetrics | None: - """Delete self-hosted runners. - - Args: - instance_id: The instance id of the runner to delete. - - Returns: - Any metrics collected during the deletion of the runner. - """ - logger.debug("Delete instance %s", instance_id) - instance = self._openstack_cloud.get_instance(instance_id) - if instance is None: - logger.warning( - "Unable to delete instance %s as it is not found", - instance_id, - ) - return None - - pulled_metrics = self._delete_runner(instance) - logger.debug( - "Metrics extracted, deleting instance %s %s", instance_id, instance.instance_id - ) - logger.debug("Instance deleted successfully %s %s", instance_id, instance.instance_id) - logger.debug("Extract metrics for runner %s %s", instance_id, instance.instance_id) - return pulled_metrics.to_runner_metrics() - - def _delete_runner(self, instance: OpenstackInstance) -> runner_metrics.PulledMetrics: - """Delete self-hosted runners by openstack instance. - - Args: - instance: The OpenStack instance. - """ - try: - pulled_metrics = runner_metrics.pull_runner_metrics( - cloud_service=self._openstack_cloud, instance_ids=[instance.instance_id] - ) - except SSHError: - logger.exception( - "Failed to get SSH connection while removing %s", instance.instance_id - ) - logger.warning( - "Skipping runner remove script for %s due to SSH issues", instance.instance_id - ) - - try: - self._openstack_cloud.delete_instance(instance.instance_id) - except OpenStackError: - logger.exception( - "Unable to delete openstack instance for runner %s", instance.instance_id - ) - # 2025/07/03 TODO: This is done to keep the previous implementation which instantiates - # PulledMetrics and then assigns it. It is weird but it was what it was. - if not pulled_metrics: - return runner_metrics.PulledMetrics(instance=instance) - return pulled_metrics[0] - def _generate_cloud_init(self, runner_context: RunnerContext) -> str: """Generate cloud init userdata. diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index 2e3d627c17..53c6976685 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -11,19 +11,24 @@ import keystoneauth1.exceptions import openstack +import openstack.exceptions import pytest from openstack.compute.v2.keypair import Keypair from openstack.connection import Connection from openstack.network.v2.security_group import SecurityGroup as OpenstackSecurityGroup from openstack.network.v2.security_group_rule import SecurityGroupRule +from pytest import LogCaptureFixture +import github_runner_manager.openstack_cloud.openstack_cloud from github_runner_manager.errors import OpenStackError, SSHError from github_runner_manager.openstack_cloud.openstack_cloud import ( _MIN_KEYPAIR_AGE_IN_SECONDS_BEFORE_DELETION, _TEST_STRING, DEFAULT_SECURITY_RULES, + InstanceID, OpenstackCloud, OpenStackCredentials, + _DeleteKeypairConfig, get_missing_security_rules, ) @@ -51,6 +56,34 @@ def openstack_cloud_fixture(monkeypatch): return OpenstackCloud(creds, FAKE_PREFIX, FAKE_ARG) +@pytest.fixture(name="patch_multiprocess_pool", scope="function") +def patch_multiprocess_pool_fixture(monkeypatch: pytest.MonkeyPatch): + """Patch multiprocessing pool call to call the function directly.""" + + def call_direct(func_var, params): + for param in params: + yield func_var(param) + + pool_mock = MagicMock() + pool_mock.return_value = pool_mock + pool_mock.__enter__ = pool_mock + pool_mock.imap_unordered = call_direct + monkeypatch.setattr("multiprocessing.pool.Pool", pool_mock) + + +@pytest.fixture(name="mock_openstack_conn", scope="function") +def mock_openstack_conn_fixture(monkeypatch: pytest.MonkeyPatch): + """Patch OpenStack connection.""" + connection_mock = MagicMock() + connection_mock.__enter__.return_value = connection_mock + monkeypatch.setattr( + github_runner_manager.openstack_cloud.openstack_cloud, + "_get_openstack_connection", + MagicMock(return_value=connection_mock), + ) + return connection_mock + + @pytest.mark.parametrize( "public_method, args", [ @@ -302,3 +335,98 @@ def mock_ssh_connection(*_args, **_kwargs) -> MagicMock: pass assert "No connectable SSH addresses found" in str(err.value) + + +# We test this internal method because this fails silently without bubbling up exceptions due to +# it's non-critical nature. +def test__delete_keypair_fail( + openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture +): + """ + arrange: given a mocked openstack delete_keypair method that returns False + act: when _delete_keypair method is called. + assert: None is returned and the failure is logged. + """ + mock_openstack_conn.delete_keypair = MagicMock(return_value=False) + test_key_instance_id = InstanceID(prefix="test-key-delete", reactive=False, suffix="fail") + + assert ( + openstack_cloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=MagicMock(), instance_id=test_key_instance_id, conn=mock_openstack_conn + ) + ) + is None + ) + assert f"Failed to delete key: {test_key_instance_id.name}" in caplog.messages + + +def test__delete_keypair_error( + openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture +): + """ + arrange: given a mocked openstack delete_keypair method that returns False + act: when _delete_keypair method is called. + assert: None is returned and the failure is logged. + """ + mock_openstack_conn.delete_keypair = MagicMock( + side_effect=[openstack.exceptions.ResourceTimeout()] + ) + test_key_instance_id = InstanceID(prefix="test-key-delete", reactive=False, suffix="fail") + + assert ( + openstack_cloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=MagicMock(), instance_id=test_key_instance_id, conn=mock_openstack_conn + ) + ) + is None + ) + assert f"Error attempting to delete key: {test_key_instance_id.name}" in caplog.messages + + +@pytest.mark.usefixtures("patch_multiprocess_pool") +def test_delete_instances_partial_server_delete_failure( + openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture +): + """ + arrange: given a mocked openstack connection that errors on few failed requests. + act: when delete_instances method is called. + assert: successfully deleted instance IDs are returned and failed instances are logged. + """ + mock_openstack_conn.delete_server = MagicMock( + side_effect=[True, False, openstack.exceptions.ResourceTimeout()] + ) + successful_delete_id = InstanceID(prefix="success", reactive=False, suffix="") + already_deleted_id = InstanceID(prefix="already_deleted", reactive=False, suffix="") + timeout_id = InstanceID(prefix="timeout error", reactive=False, suffix="") + + deleted_instance_ids = openstack_cloud.delete_instances( + instance_ids=[successful_delete_id, already_deleted_id, timeout_id] + ) + + assert successful_delete_id in deleted_instance_ids + assert already_deleted_id not in deleted_instance_ids + assert timeout_id not in deleted_instance_ids + assert f"Failed to delete OpenStack VM instance: {timeout_id}" in caplog.messages + + +@pytest.mark.usefixtures("patch_multiprocess_pool") +def test_delete_instances( + openstack_cloud: OpenstackCloud, + mock_openstack_conn: MagicMock, +): + """ + arrange: given a mocked openstack connection. + act: when delete_instances method is called. + assert: deleted instance IDs are returned. + """ + mock_openstack_conn.delete_server = MagicMock(side_effect=[True, False]) + successful_delete_id = InstanceID(prefix="success", reactive=False, suffix="") + already_deleted_id = InstanceID(prefix="already_deleted", reactive=False, suffix="") + + deleted_instance_ids = openstack_cloud.delete_instances( + instance_ids=[successful_delete_id, already_deleted_id] + ) + + assert [successful_delete_id] == deleted_instance_ids diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py index bd3f71c2c0..6526ce838f 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py @@ -3,20 +3,11 @@ """Module for unit-testing OpenStack runner manager.""" import logging -from datetime import datetime, timezone -from typing import Iterable from unittest.mock import MagicMock import pytest from github_runner_manager.configuration import ProxyConfig, SupportServiceConfig, UserInfo -from github_runner_manager.manager.cloud_runner_manager import ( - CodeInformation, - PostJobMetrics, - PostJobStatus, - PreJobMetrics, - RunnerMetrics, -) from github_runner_manager.manager.models import ( InstanceID, RunnerContext, @@ -24,19 +15,12 @@ RunnerMetadata, ) from github_runner_manager.metrics import runner -from github_runner_manager.metrics.runner import PullFileError -from github_runner_manager.openstack_cloud import openstack_cloud -from github_runner_manager.openstack_cloud.constants import ( - POST_JOB_METRICS_FILE_NAME, - PRE_JOB_METRICS_FILE_NAME, - RUNNER_INSTALLED_TS_FILE_NAME, -) from github_runner_manager.openstack_cloud.openstack_cloud import OpenstackCloud from github_runner_manager.openstack_cloud.openstack_runner_manager import ( OpenStackRunnerManager, OpenStackRunnerManagerConfig, + runner_metrics, ) -from tests.unit.factories import openstack_factory logger = logging.getLogger(__name__) @@ -134,158 +118,31 @@ def test_create_runner_without_aproxy( assert "aproxy" not in openstack_cloud.launch_instance.call_args.kwargs["cloud_init"] -def _params_test_delete_extract_metrics(): - """Builds parametrized input for the test_delete_extract_metrics. - - The following values are returned: - runner_installed_metrics,pre_job_metrics,post_job_metrics,result +def test_delete_vms(runner_manager: OpenStackRunnerManager): """ - openstack_created_at = ( - datetime.strptime(openstack_factory.SERVER_CREATED_AT, "%Y-%m-%dT%H:%M:%SZ") - .replace(tzinfo=timezone.utc) - .timestamp() - ) - openstack_installed_at = openstack_created_at + 20 - pre_job_timestamp = openstack_installed_at + 20 - post_job_timestamp = openstack_installed_at + 20 - pre_job_metrics_str = f"""{{ - "timestamp": {pre_job_timestamp}, - "workflow": "Workflow Dispatch Tests", - "workflow_run_id": "13831611664", - "repository": "canonical/github-runner-operator", - "event": "workflow_dispatch" - }}""" - post_job_metrics_str = f"""{{ - "timestamp": {post_job_timestamp}, "status": "normal", "status_info": {{"code" : "200"}} - }}""" + arrange: given a mocked cloud service. + act: when delete_vms method is called. + assert: the mocked service call is made and the deleted instance IDs are returned. + """ + test_instance_ids = [InstanceID(prefix="test-prefix", reactive=None, suffix="test-suffix")] + mock_cloud = MagicMock() + mock_cloud.delete_instances = MagicMock(return_value=test_instance_ids) + runner_manager._openstack_cloud = mock_cloud - return [ - pytest.param(None, None, None, None, id="All None. No metrics returned."), - pytest.param( - "", None, None, None, id="Invalid runner-installed metrics. No metrics returned." - ), - pytest.param( - str(openstack_installed_at), - None, - None, - RunnerMetrics( - instance_id=InstanceID( - prefix=OPENSTACK_INSTANCE_PREFIX, reactive=False, suffix="unhealthy" - ), - installation_start_timestamp=openstack_created_at, - installed_timestamp=openstack_installed_at, - metadata=RunnerMetadata(), - ), - id="Only installed_timestamp. Metric returned.", - ), - pytest.param( - str(openstack_installed_at), - pre_job_metrics_str, - None, - RunnerMetrics( - instance_id=InstanceID( - prefix=OPENSTACK_INSTANCE_PREFIX, reactive=False, suffix="unhealthy" - ), - installation_start_timestamp=openstack_created_at, - installed_timestamp=openstack_installed_at, - pre_job=PreJobMetrics( - timestamp=pre_job_timestamp, - workflow="Workflow Dispatch Tests", - workflow_run_id="13831611664", - repository="canonical/github-runner-operator", - event="workflow_dispatch", - ), - metadata=RunnerMetadata(), - ), - id="installed_timestamp and pre_job_metrics. Metric returned.", - ), - pytest.param( - str(openstack_installed_at), - pre_job_metrics_str, - post_job_metrics_str, - RunnerMetrics( - metadata=RunnerMetadata(), - instance_id=InstanceID( - prefix=OPENSTACK_INSTANCE_PREFIX, reactive=False, suffix="unhealthy" - ), - installation_start_timestamp=openstack_created_at, - installed_timestamp=openstack_installed_at, - pre_job=PreJobMetrics( - timestamp=pre_job_timestamp, - workflow="Workflow Dispatch Tests", - workflow_run_id="13831611664", - repository="canonical/github-runner-operator", - event="workflow_dispatch", - ), - post_job=PostJobMetrics( - timestamp=post_job_timestamp, - status=PostJobStatus.NORMAL, - status_info=CodeInformation(code=200), - ), - ), - id="installed_timestamp, pre_job_metrics and post_job_metrics. Metric returned", - ), - ] + assert test_instance_ids == runner_manager.delete_vms(instance_ids=test_instance_ids) + mock_cloud.delete_instances.assert_called_once() -@pytest.mark.parametrize( - "runner_installed_metrics,pre_job_metrics,post_job_metrics,result", - _params_test_delete_extract_metrics(), -) -def test_delete_extract_metrics( - runner_manager: OpenStackRunnerManager, - runner_installed_metrics: str | None, - pre_job_metrics: str | None, - post_job_metrics: str | None, - result: Iterable[RunnerMetrics], - monkeypatch: pytest.MonkeyPatch, -): +def test_extract_metrics(runner_manager: OpenStackRunnerManager, monkeypatch: pytest.MonkeyPatch): """ - arrange: Given different values for values of metrics for a runner. - act: Delete the runner for those metrics. - assert: The expected RunnerMetrics object is obtained, or None if there should not be one. + arrange: given a mocked metrics service. + act: when extract_metrics method is called. + assert: converted metrics are returned. """ - ssh_pull_file_mock = MagicMock() - monkeypatch.setattr( - "github_runner_manager.metrics.runner.ssh_pull_file", - ssh_pull_file_mock, + pull_metrics_mock = MagicMock( + return_value=[(test_metric_one := MagicMock()), (test_metric_two := MagicMock())] ) + monkeypatch.setattr(runner_metrics, "pull_runner_metrics", pull_metrics_mock) - def _ssh_pull_file(remote_path, *args, **kwargs): - """Get a file from the runner.""" - logger.info("ssh_pull_file: remote_path %s", remote_path) - res = None - if remote_path == str(PRE_JOB_METRICS_FILE_NAME): - res = pre_job_metrics - elif remote_path == str(POST_JOB_METRICS_FILE_NAME): - res = post_job_metrics - elif remote_path == str(RUNNER_INSTALLED_TS_FILE_NAME): - res = runner_installed_metrics - if res is None: - raise PullFileError("Nothing found or invalid file.") - return res - - ssh_pull_file_mock.side_effect = _ssh_pull_file - - instance_id = InstanceID( - prefix=OPENSTACK_INSTANCE_PREFIX, reactive=False, suffix="unhealthy" - ).name - openstack_cloud_mock = _create_openstack_cloud_mock(instance_id) - runner_manager._openstack_cloud = openstack_cloud_mock - - runner_metrics = runner_manager.delete_runner(instance_id) - - assert runner_metrics == result - - -def _create_openstack_cloud_mock(instance_name: str) -> MagicMock: - """Create an OpenstackCloud mock which returns servers with a given list of server names.""" - openstack_cloud_mock = MagicMock(spec=OpenstackCloud) - openstack_cloud_mock.get_instance.return_value = openstack_cloud.OpenstackInstance( - server=openstack_factory.ServerFactory( - status="ACTIVE", - name=instance_name, - ), - prefix=OPENSTACK_INSTANCE_PREFIX, - ) - return openstack_cloud_mock + metrics = runner_manager.extract_metrics(instance_ids=MagicMock()) + assert metrics == [test_metric_one.to_runner_metrics(), test_metric_two.to_runner_metrics()] From aa7c2c18f78bc62dcdbc9c88c084387edbcdaeca Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 12:22:40 +0000 Subject: [PATCH 10/50] chore: reorder helper func --- .../tests/unit/metrics/test_runner.py | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/github-runner-manager/tests/unit/metrics/test_runner.py b/github-runner-manager/tests/unit/metrics/test_runner.py index dfcadd80ee..33bdfb1c18 100644 --- a/github-runner-manager/tests/unit/metrics/test_runner.py +++ b/github-runner-manager/tests/unit/metrics/test_runner.py @@ -41,31 +41,6 @@ def runner_fs_base_fixture(tmp_path: Path) -> Path: return runner_fs_base -def _create_metrics_data(instance_id: InstanceID) -> RunnerMetrics: - """Create a RunnerMetrics object that is suitable for most tests. - - Args: - instance_id: The test runner name. - - Returns: - Test metrics data. - """ - return RunnerMetrics( - installation_start_timestamp=1, - installed_timestamp=2, - pre_job=PreJobMetrics( - timestamp=3, - workflow="workflow1", - workflow_run_id="workflow_run_id1", - repository="org1/repository1", - event="push", - ), - post_job=PostJobMetrics(timestamp=3, status=PostJobStatus.NORMAL), - instance_id=instance_id, - metadata=RunnerMetadata(), - ) - - def test_issue_events(issue_event_mock: MagicMock): """ arrange: A runner with all metrics. @@ -126,6 +101,31 @@ def test_issue_events(issue_event_mock: MagicMock): ) +def _create_metrics_data(instance_id: InstanceID) -> RunnerMetrics: + """Create a RunnerMetrics object that is suitable for most tests. + + Args: + instance_id: The test runner name. + + Returns: + Test metrics data. + """ + return RunnerMetrics( + installation_start_timestamp=1, + installed_timestamp=2, + pre_job=PreJobMetrics( + timestamp=3, + workflow="workflow1", + workflow_run_id="workflow_run_id1", + repository="org1/repository1", + event="push", + ), + post_job=PostJobMetrics(timestamp=3, status=PostJobStatus.NORMAL), + instance_id=instance_id, + metadata=RunnerMetadata(), + ) + + def test_issue_events_pre_job_before_runner_installed(issue_event_mock: MagicMock): """ arrange: A runner with pre-job timestamp smaller than installed timestamp. From d9032e1bd16b6e738bd432f4938ea0eb17050a5d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 13:19:45 +0000 Subject: [PATCH 11/50] test: runner pull metrics --- .../manager/runner_manager.py | 3 +- .../github_runner_manager/metrics/runner.py | 20 ++- github-runner-manager/tests/unit/conftest.py | 16 +++ .../tests/unit/metrics/test_runner.py | 114 +++++++++++++++++- .../openstack_cloud/test_openstack_cloud.py | 19 +-- 5 files changed, 139 insertions(+), 33 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index 8205987cf1..9ca45cad04 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -336,7 +336,8 @@ def _delete_cloud_runners( ] logger.info("Deleting runners from platform: %s", platform_runner_ids_to_delete) deleted_runner_ids = self._platform.delete_runners( - runner_ids=platform_runner_ids_to_delete + runner_ids=platform_runner_ids_to_delete, + platform=cloud_runners[0].metadata.platform_name, ) logger.info( "Deleted runners from platform: %s (diff: %s)", diff --git a/github-runner-manager/src/github_runner_manager/metrics/runner.py b/github-runner-manager/src/github_runner_manager/metrics/runner.py index 826cde2989..37f360739a 100644 --- a/github-runner-manager/src/github_runner_manager/metrics/runner.py +++ b/github-runner-manager/src/github_runner_manager/metrics/runner.py @@ -64,7 +64,8 @@ def pull_runner_metrics( This function uses multiprocessing to fetch metrics in parallel. Args: - ssh_conn: The SSH connection to the runner. + cloud_service: The OpenStack cloud service. + instance_ids: The instance IDs to fetch the metrics from. Returns: Metrics pulled from the instance. @@ -88,7 +89,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric """Pull metrics from a single runner via SSH file pull. Args: - pull_runner_metrics_config: Configurations for pulling the runner metrics. + pull_config: Configurations for pulling the runner metrics. Returns: PulledMetrics if metrics were available. None otherwise. @@ -104,7 +105,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric try: with pull_config.cloud_service.get_ssh_connection(instance=instance) as ssh_conn: try: - pulled_metrics.runner_installed = ssh_pull_file( + pulled_metrics.runner_installed = _ssh_pull_file( ssh_conn=ssh_conn, remote_path=str(RUNNER_INSTALLED_TS_FILE_NAME), max_size=MAX_METRICS_FILE_SIZE, @@ -116,7 +117,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric exc, ) try: - pulled_metrics.pre_job_metrics = ssh_pull_file( + pulled_metrics.pre_job_metrics = _ssh_pull_file( ssh_conn=ssh_conn, remote_path=str(PRE_JOB_METRICS_FILE_NAME), max_size=MAX_METRICS_FILE_SIZE, @@ -128,7 +129,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric exc, ) try: - pulled_metrics.post_job_metrics = ssh_pull_file( + pulled_metrics.post_job_metrics = _ssh_pull_file( ssh_conn=ssh_conn, remote_path=str(POST_JOB_METRICS_FILE_NAME), max_size=MAX_METRICS_FILE_SIZE, @@ -156,7 +157,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric ) -def ssh_pull_file(ssh_conn: SSHConnection, remote_path: str, max_size: int) -> str: +def _ssh_pull_file(ssh_conn: SSHConnection, remote_path: str, max_size: int) -> str: """Pull file from the runner instance. Args: @@ -222,7 +223,7 @@ class PulledMetrics: """Metrics pulled from a runner. Attributes: - instance_id: The instance in which the metrics were pulled from. + instance: The instance in which the metrics were pulled from. runner_installed: String with the runner-installed file. pre_job_metrics: String with the pre-job-metrics file. post_job_metrics: String with the post-job-metrics file. @@ -234,10 +235,7 @@ class PulledMetrics: post_job_metrics: str | None = None def to_runner_metrics(self) -> RunnerMetrics | None: - """. - - Args: - instance: Cloud runner instance. + """Convert PulledMetrics to RunnerMetrics instance. Returns: The RunnerMetrics object for the runner or None if it can not be built. diff --git a/github-runner-manager/tests/unit/conftest.py b/github-runner-manager/tests/unit/conftest.py index d276b286ad..c993441398 100644 --- a/github-runner-manager/tests/unit/conftest.py +++ b/github-runner-manager/tests/unit/conftest.py @@ -6,6 +6,7 @@ import getpass import grp import os +from unittest.mock import MagicMock import pytest @@ -15,3 +16,18 @@ @pytest.fixture(name="user_info", scope="module") def user_info_fixture(): return UserInfo(getpass.getuser(), grp.getgrgid(os.getgid()).gr_name) + + +@pytest.fixture(name="patch_multiprocess_pool_imap_unordered", scope="function") +def patch_multiprocess_pool_imap_unordered_fixture(monkeypatch: pytest.MonkeyPatch): + """Patch multiprocessing pool call to call the function directly.""" + + def call_direct(func_var, params): + for param in params: + yield func_var(param) + + pool_mock = MagicMock() + pool_mock.return_value = pool_mock + pool_mock.__enter__ = pool_mock + pool_mock.imap_unordered = call_direct + monkeypatch.setattr("multiprocessing.pool.Pool", pool_mock) diff --git a/github-runner-manager/tests/unit/metrics/test_runner.py b/github-runner-manager/tests/unit/metrics/test_runner.py index 33bdfb1c18..593745423e 100644 --- a/github-runner-manager/tests/unit/metrics/test_runner.py +++ b/github-runner-manager/tests/unit/metrics/test_runner.py @@ -21,7 +21,13 @@ from github_runner_manager.metrics import runner as runner_metrics from github_runner_manager.metrics import type as metrics_type from github_runner_manager.metrics.events import RunnerInstalled, RunnerStart, RunnerStop -from github_runner_manager.metrics.runner import PullFileError, ssh_pull_file +from github_runner_manager.metrics.runner import ( + PulledMetrics, + PullFileError, + SSHError, + _ssh_pull_file, + pull_runner_metrics, +) from github_runner_manager.types_.github import JobConclusion @@ -41,6 +47,106 @@ def runner_fs_base_fixture(tmp_path: Path) -> Path: return runner_fs_base +@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") +def test_pull_runner_metrics_errors(caplog: pytest.LogCaptureFixture): + """ + arrange: given a mocked cloud service that raises exceptions are different points. + act: when pull_runner_metrics function is called. + assert: no metrics are pulled and errors are logged. + """ + test_instances = [] + get_instance_side_effects = [] + get_ssh_connection_side_effects = [] + # Setup for instance not exists + test_instances.append( + ( + not_exists_instance := InstanceID( + prefix="instance-not-exists", reactive=False, suffix="1" + ) + ) + ) + get_instance_side_effects.append(None) + # Setup for instance fail ssh connection + test_instances.append( + (fail_ssh_conn_instance := InstanceID(prefix="fail-ssh-conn", reactive=False, suffix="2")) + ) + fail_ssh_conn_instance_mock = MagicMock() + fail_ssh_conn_instance_mock.instance_id = fail_ssh_conn_instance + get_instance_side_effects.append(fail_ssh_conn_instance_mock) + get_ssh_connection_side_effects.append(SSHError()) + # Setup for instance fail pull file + test_instances.append( + ( + fail_pull_file_instance := InstanceID( + prefix="fail-pull-file", reactive=False, suffix="3" + ) + ) + ) + fail_pull_file_instance_mock = MagicMock() + fail_pull_file_instance_mock.instance_id = fail_pull_file_instance + get_instance_side_effects.append(fail_pull_file_instance_mock) + ssh_connection_mock = MagicMock() + ssh_connection_mock.return_value = ssh_connection_mock + ssh_connection_mock.__enter__ = ssh_connection_mock + ssh_connection_mock.run.side_effect = [TimeoutError] + get_ssh_connection_side_effects.append(ssh_connection_mock) + # Mock cloud service setup + mock_cloud_service = MagicMock() + mock_cloud_service.get_instance = MagicMock(side_effect=get_instance_side_effects) + mock_cloud_service.get_ssh_connection = MagicMock(side_effect=get_ssh_connection_side_effects) + + assert pull_runner_metrics(cloud_service=mock_cloud_service, instance_ids=test_instances) == [] + assert ( + f"Skipping fetching metrics, instance not found: {not_exists_instance}" in caplog.messages + ) + assert ( + f"Failed to create SSH connection for pulling metrics: {fail_ssh_conn_instance}" + in caplog.messages + ) + assert ( + f"Failed to create SSH connection for pulling metrics: {fail_pull_file_instance}" + in caplog.messages + ) + + +@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") +def test_pull_runner_metrics(): + """ + arrange: given a mock cloud service get_instance method and get_ssh_connection method. + act: when pull_runner_metrics function is called. + assert: metrics are pulled from corresponding instances correctly. + """ + mock_cloud_service = MagicMock() + mock_ssh_conn = MagicMock() + mock_ssh_conn.return_value = mock_ssh_conn + mock_ssh_conn.__enter__ = mock_ssh_conn + test_remote_file_contents = "test-contents" + mock_ssh_conn.get = lambda remote, local: local.write( + bytes(test_remote_file_contents, encoding="utf-8") + ) + mock_cloud_service.get_ssh_connection = mock_ssh_conn + mock_instance_one, mock_instance_two = (MagicMock(), MagicMock()) + mock_cloud_service.get_instance.side_effect = [mock_instance_one, mock_instance_two] + + assert pull_runner_metrics( + cloud_service=mock_cloud_service, + instance_ids=[mock_instance_one.instance_id, mock_instance_two.instance_id], + ) == [ + PulledMetrics( + instance=mock_instance_one, + runner_installed=test_remote_file_contents, + pre_job_metrics=test_remote_file_contents, + post_job_metrics=test_remote_file_contents, + ), + PulledMetrics( + instance=mock_instance_two, + runner_installed=test_remote_file_contents, + pre_job_metrics=test_remote_file_contents, + post_job_metrics=test_remote_file_contents, + ), + ] + + def test_issue_events(issue_event_mock: MagicMock): """ arrange: A runner with all metrics. @@ -363,7 +469,7 @@ def _ssh_get(remote, local) -> None: ssh_conn.get.side_effect = _ssh_get - response = ssh_pull_file(ssh_conn, remote_path, max_size) + response = _ssh_pull_file(ssh_conn, remote_path, max_size) assert response == "content from the file" @@ -400,7 +506,7 @@ def _ssh_get(remote, local) -> None: ssh_conn.get.side_effect = _ssh_get with pytest.raises(PullFileError) as exc: - _ = ssh_pull_file(ssh_conn, remote_path, max_size) + _ = _ssh_pull_file(ssh_conn, remote_path, max_size) assert "max" in str(exc) @@ -426,5 +532,5 @@ def _ssh_run(command, **kwargs) -> Optional[Result]: ssh_conn.run.side_effect = _ssh_run with pytest.raises(PullFileError) as exc: - _ = ssh_pull_file(ssh_conn, remote_path, max_size) + _ = _ssh_pull_file(ssh_conn, remote_path, max_size) assert "too large" in str(exc) diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index 53c6976685..68c1eabd00 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -56,21 +56,6 @@ def openstack_cloud_fixture(monkeypatch): return OpenstackCloud(creds, FAKE_PREFIX, FAKE_ARG) -@pytest.fixture(name="patch_multiprocess_pool", scope="function") -def patch_multiprocess_pool_fixture(monkeypatch: pytest.MonkeyPatch): - """Patch multiprocessing pool call to call the function directly.""" - - def call_direct(func_var, params): - for param in params: - yield func_var(param) - - pool_mock = MagicMock() - pool_mock.return_value = pool_mock - pool_mock.__enter__ = pool_mock - pool_mock.imap_unordered = call_direct - monkeypatch.setattr("multiprocessing.pool.Pool", pool_mock) - - @pytest.fixture(name="mock_openstack_conn", scope="function") def mock_openstack_conn_fixture(monkeypatch: pytest.MonkeyPatch): """Patch OpenStack connection.""" @@ -385,7 +370,7 @@ def test__delete_keypair_error( assert f"Error attempting to delete key: {test_key_instance_id.name}" in caplog.messages -@pytest.mark.usefixtures("patch_multiprocess_pool") +@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") def test_delete_instances_partial_server_delete_failure( openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture ): @@ -411,7 +396,7 @@ def test_delete_instances_partial_server_delete_failure( assert f"Failed to delete OpenStack VM instance: {timeout_id}" in caplog.messages -@pytest.mark.usefixtures("patch_multiprocess_pool") +@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") def test_delete_instances( openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, From 9ca696b1428e9bb46de20707128d3fe3d85b16b4 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 13:25:20 +0000 Subject: [PATCH 12/50] feat: replace singular delete with parallel deletes --- .../manager/cloud_runner_manager.py | 8 ------- .../manager/runner_manager.py | 16 +++++++------- .../platform/github_provider.py | 21 +++++++------------ .../platform/jobmanager_provider.py | 18 +++++++--------- .../platform/multiplexer_provider.py | 13 +++++++++++- .../platform/platform_provider.py | 13 ++---------- 6 files changed, 37 insertions(+), 52 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py index a724210aaa..75968d0817 100644 --- a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py @@ -274,14 +274,6 @@ def create_runner( def get_runners(self) -> Sequence[CloudRunnerInstance]: """Get cloud self-hosted runners.""" - @abc.abstractmethod - def delete_runner(self, instance_id: InstanceID) -> RunnerMetrics | None: - """Delete self-hosted runner. - - Args: - instance_id: The instance id of the runner to delete. - """ - @abc.abstractmethod def delete_vms(self, instance_ids: Sequence[InstanceID]) -> list[InstanceID]: """Delete cloud VM instances. diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index 9ca45cad04..b37d54cfac 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -369,13 +369,15 @@ def _delete_cloud_runners( def _clean_platform_runners(self, runners: list[RunnerIdentity]) -> None: """Clean the specified runners in the platform.""" - for runner in runners: - try: - self._platform.delete_runner(runner) - except DeleteRunnerBusyError: - logger.warning("Tried to delete busy runner in cleanup %s", runner) - except PlatformApiError: - logger.warning("Failed to delete platform runner %s", runner) + if not runners: + return + + runner_ids_to_delete = [ + runner.metadata.runner_id for runner in runners if runner.metadata.runner_id + ] + self._platform.delete_runners( + runner_ids=runner_ids_to_delete, platform=runners[0].metadata.platform_name + ) @staticmethod def _spawn_runners( diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index c9b4ab5710..37efd8f770 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -60,7 +60,6 @@ def __init__(self, prefix: str, path: GitHubPath, github_client: GithubClient): path: GitHub path. github_client: GitHub client. """ - self._prefix = prefix self._path = path self._client = github_client @@ -165,25 +164,16 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners non_requested_runners=non_requested_runners, ) - def delete_runner(self, runner_identity: RunnerIdentity) -> None: - """Delete a runner from GitHub. - - This method will raise DeleteRunnerBusyError if the runner is not deletable, that is, - if it is busy. If the runner does not exist it will not fail. - - Args: - runner_identity: Identity of the runner to delete. - """ - logger.info("Delete runner in GitHub: %s", runner_identity) - self._client.delete_runner(self._path, int(runner_identity.metadata.runner_id)) - - def delete_runners(self, runner_ids: list[str]) -> list[str]: + def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: """Delete runners from GitHub. This method will ignore DeleteRunnerBusyErrors and print a warning log. Args: runner_ids: The GitHub runner IDs to delete. + platform: TODO: Unused argument due to the poor architecture of the provider + classes. The multiplexer provider should be a wrapper around the platforms, not on + the same level. Returns: The runner IDs that were deleted successfully. @@ -224,6 +214,9 @@ def _delete_runner(delete_runner_config: _DeleteRunnerConfig) -> str | None: path=delete_runner_config.path, runner_id=int(delete_runner_config.runner_id) ) except DeleteRunnerBusyError: + logger.warning( + "Delete runner attempt failed, busy runner: %s", delete_runner_config.runner_id + ) return None return delete_runner_config.runner_id diff --git a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py index f6654d9eee..cbe52d2b0c 100644 --- a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py @@ -142,23 +142,19 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners failed_requested_runners=failed_runners, ) - def delete_runner(self, runner_identity: RunnerIdentity) -> None: - """Delete a runner from jobmanager. - - This method does nothing, as the jobmanager does not implement it. - - Args: - runner_identity: The identity of the runner to delete. - """ - logger.debug("No need to delete runners in the jobmanager.") - - def delete_runners(self, runner_ids: list[str]) -> list[str]: + def delete_runners(self, runner_ids: list[str], platform: str = "jobmanager") -> list[str]: """Delete a runner from jobmanager. This method does nothing, as the jobmanager does not implement it. Args: runner_ids: The runner IDs to delete. + platform: TODO: Unused argument due to the poor architecture of the provider + classes. The multiplexer provider should be a wrapper around the platforms, not on + the same level. + + Returns: + The runner IDs requested for deletion. """ logger.debug("No need to delete runners in the jobmanager.") return runner_ids diff --git a/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py b/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py index 4158ddd160..43441fb65c 100644 --- a/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py @@ -140,13 +140,24 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners return response def delete_runner(self, runner_identity: RunnerIdentity) -> None: - """Delete a runner. + """Delete a runner. Args: runner_identity: Runner to delete. """ self._get_provider(runner_identity.metadata).delete_runner(runner_identity) + def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: + """Delete runners from a provider. + + Args: + runner_ids: The runner IDs to delete. + + Returns: + List of deleted runner IDs. + """ + return self._providers[platform].delete_runners(runner_ids) + def get_runner_context( self, metadata: RunnerMetadata, instance_id: InstanceID, labels: list[str] ) -> tuple[RunnerContext, SelfHostedRunner]: diff --git a/github-runner-manager/src/github_runner_manager/platform/platform_provider.py b/github-runner-manager/src/github_runner_manager/platform/platform_provider.py index 6e47e08f84..262c911088 100644 --- a/github-runner-manager/src/github_runner_manager/platform/platform_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/platform_provider.py @@ -71,21 +71,12 @@ def get_runners_health( """ @abc.abstractmethod - def delete_runner(self, runner_identity: RunnerIdentity) -> None: - """Delete a runner. - - Can raise DeleteRunnerBusyError - - Args: - runner_identity: Runner to delete. - """ - - @abc.abstractmethod - def delete_runners(self, runner_ids: list[str]) -> list[str]: + def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: """Delete runners. Args: runner_ids: Runner IDs to delete. + platform: The Platform in which to delete the runners in. """ @abc.abstractmethod From d7a0496c520d6e5bf8e407c71e85ab32aa6f788f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 13:25:35 +0000 Subject: [PATCH 13/50] chore: ignore docstring errors in abc --- github-runner-manager/pyproject.toml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/github-runner-manager/pyproject.toml b/github-runner-manager/pyproject.toml index 86f71efcc9..e07792642c 100644 --- a/github-runner-manager/pyproject.toml +++ b/github-runner-manager/pyproject.toml @@ -80,7 +80,11 @@ select = ["E", "W", "F", "C", "N", "R", "D", "H"] # Ignore D107 Missing docstring in __init__ ignore = ["W503", "D107", "E203"] # D100, D101, D102, D103, D104: Ignore docstring style issues in tests -per-file-ignores = ["tests/*:D100,D101,D102,D103,D104,D205,D212"] +per-file-ignores = [ + # Ignore no return values (DCO031) in docstring for abstract methods + "src/github_runner_manager/manager/cloud_runner_manager.py:DCO031", + "tests/*:D100,D101,D102,D103,D104,D205,D212" +] docstring-convention = "google" # Check for properly formatted copyright header in each file copyright-check = "True" From e6064dde894309db91e8a7ca7088c71b9cb8d256 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 3 Jul 2025 13:28:45 +0000 Subject: [PATCH 14/50] chore: lint fixes --- .../src/github_runner_manager/manager/runner_manager.py | 1 - .../platform/multiplexer_provider.py | 9 +-------- github-runner-manager/tests/unit/conftest.py | 9 +++++++++ github-runner-manager/tests/unit/metrics/test_runner.py | 2 +- .../tests/unit/openstack_cloud/test_openstack_cloud.py | 4 ++-- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index b37d54cfac..8274f67d56 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -26,7 +26,6 @@ from github_runner_manager.metrics.runner import RunnerMetrics from github_runner_manager.openstack_cloud.constants import CREATE_SERVER_TIMEOUT from github_runner_manager.platform.platform_provider import ( - DeleteRunnerBusyError, PlatformApiError, PlatformProvider, PlatformRunnerHealth, diff --git a/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py b/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py index 43441fb65c..d7bba855d9 100644 --- a/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/multiplexer_provider.py @@ -139,19 +139,12 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners response.append(provider_health_response) return response - def delete_runner(self, runner_identity: RunnerIdentity) -> None: - """Delete a runner. - - Args: - runner_identity: Runner to delete. - """ - self._get_provider(runner_identity.metadata).delete_runner(runner_identity) - def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: """Delete runners from a provider. Args: runner_ids: The runner IDs to delete. + platform: The platform in which the runners belong to. Returns: List of deleted runner IDs. diff --git a/github-runner-manager/tests/unit/conftest.py b/github-runner-manager/tests/unit/conftest.py index c993441398..e08d29515a 100644 --- a/github-runner-manager/tests/unit/conftest.py +++ b/github-runner-manager/tests/unit/conftest.py @@ -23,6 +23,15 @@ def patch_multiprocess_pool_imap_unordered_fixture(monkeypatch: pytest.MonkeyPat """Patch multiprocessing pool call to call the function directly.""" def call_direct(func_var, params): + """Function to replace imap_unordered with, by calling functions directly. + + Args: + func_var: The function to call in imap_unordered call. + params: The iterable parameters for target function. + + Yields: + The function return value. + """ for param in params: yield func_var(param) diff --git a/github-runner-manager/tests/unit/metrics/test_runner.py b/github-runner-manager/tests/unit/metrics/test_runner.py index 593745423e..97348088da 100644 --- a/github-runner-manager/tests/unit/metrics/test_runner.py +++ b/github-runner-manager/tests/unit/metrics/test_runner.py @@ -55,7 +55,7 @@ def test_pull_runner_metrics_errors(caplog: pytest.LogCaptureFixture): assert: no metrics are pulled and errors are logged. """ test_instances = [] - get_instance_side_effects = [] + get_instance_side_effects: list[None | Exception] = [] get_ssh_connection_side_effects = [] # Setup for instance not exists test_instances.append( diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index 68c1eabd00..04b0dd0cc9 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -328,7 +328,7 @@ def test__delete_keypair_fail( openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture ): """ - arrange: given a mocked openstack delete_keypair method that returns False + arrange: given a mocked openstack delete_keypair method that returns False. act: when _delete_keypair method is called. assert: None is returned and the failure is logged. """ @@ -350,7 +350,7 @@ def test__delete_keypair_error( openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture ): """ - arrange: given a mocked openstack delete_keypair method that returns False + arrange: given a mocked openstack delete_keypair method that returns False. act: when _delete_keypair method is called. assert: None is returned and the failure is logged. """ From 49f1a0a473fc5b694580e07a56e27f2804594c88 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 13 Jul 2025 07:41:54 +0000 Subject: [PATCH 15/50] test: redo tests --- github-runner-manager/pyproject.toml | 2 + .../manager/cloud_runner_manager.py | 14 - .../manager/runner_manager.py | 46 +- .../manager/runner_scaler.py | 4 +- .../openstack_runner_manager.py | 6 +- .../tests/unit/factories/metrics_factory.py | 78 +++ .../unit/factories/runner_instance_factory.py | 214 ++++++++ .../tests/unit/manager/test_runner_manager.py | 384 +++++++++++--- .../tests/unit/mock_runner_managers.py | 287 +++++------ .../openstack_cloud/test_openstack_cloud.py | 2 +- .../tests/unit/test_runner_scaler.py | 487 ++++++------------ 11 files changed, 943 insertions(+), 581 deletions(-) create mode 100644 github-runner-manager/tests/unit/factories/metrics_factory.py create mode 100644 github-runner-manager/tests/unit/factories/runner_instance_factory.py diff --git a/github-runner-manager/pyproject.toml b/github-runner-manager/pyproject.toml index b8b8664b6d..db840eaca6 100644 --- a/github-runner-manager/pyproject.toml +++ b/github-runner-manager/pyproject.toml @@ -81,6 +81,8 @@ select = ["E", "W", "F", "C", "N", "R", "D", "H"] ignore = ["W503", "D107", "E203"] # D100, D101, D102, D103, D104: Ignore docstring style issues in tests per-file-ignores = [ + # Ignore factory methods attributes docstring + "tests/unit/factories/*:DCO060", # Ignore no return values (DCO031) in docstring for abstract methods "src/github_runner_manager/manager/cloud_runner_manager.py:DCO031", "tests/*:D100,D101,D102,D103,D104,D205,D212" diff --git a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py index 75968d0817..7aa30170bf 100644 --- a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py @@ -35,20 +35,6 @@ class HealthState(Enum): UNHEALTHY = auto() UNKNOWN = auto() - @staticmethod - def from_value(health: bool | None) -> "HealthState": - """Create from a health value. - - Args: - health: The health value as boolean or None. - - Returns: - The health state. - """ - if health is None: - return HealthState.UNKNOWN - return HealthState.HEALTHY if health else HealthState.UNHEALTHY - class CloudRunnerState(str, Enum): """Represent state of the instance hosting the runner. diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index f8b14e77ab..fe5ad7e5c1 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -23,7 +23,6 @@ from github_runner_manager.metrics import events as metric_events from github_runner_manager.metrics import github as github_metrics from github_runner_manager.metrics import runner as runner_metrics -from github_runner_manager.metrics.reconcile import CLEANED_RUNNERS_TOTAL from github_runner_manager.metrics.runner import RunnerMetrics from github_runner_manager.openstack_cloud.constants import CREATE_SERVER_TIMEOUT from github_runner_manager.platform.platform_provider import ( @@ -81,27 +80,33 @@ class RunnerInstance: platform_state: PlatformRunnerState | None cloud_state: CloudRunnerState - def __init__( - self, + @classmethod + def from_cloud_and_platform_health( + cls, cloud_instance: CloudRunnerInstance, platform_health_state: PlatformRunnerHealth | None, - ): + ) -> "RunnerInstance": """Construct an instance. Args: cloud_instance: Information on the cloud instance. platform_health_state: Health state in the platform provider. + + Returns: + The RunnerInstance instantiated from cloud instance and platform state. """ - self.name = cloud_instance.name - self.instance_id = cloud_instance.instance_id - self.metadata = cloud_instance.metadata - self.health = cloud_instance.health - self.platform_state = ( - PlatformRunnerState.from_platform_health(platform_health_state) - if platform_health_state is not None - else None + return cls( + name=cloud_instance.name, + instance_id=cloud_instance.instance_id, + metadata=cloud_instance.metadata, + health=cloud_instance.health, + platform_state=( + PlatformRunnerState.from_platform_health(platform_health_state) + if platform_health_state is not None + else None + ), + cloud_state=cloud_instance.state, ) - self.cloud_state = cloud_instance.state class RunnerManager: @@ -182,7 +187,7 @@ def get_runners(self) -> tuple[RunnerInstance, ...]: health_runners_map = {runner.identity.instance_id: runner for runner in runners_health} for cloud_runner in cloud_runners: if cloud_runner.instance_id not in health_runners_map: - runner_instance = RunnerInstance(cloud_runner, None) + runner_instance = RunnerInstance.from_cloud_and_platform_health(cloud_runner, None) runner_instance.health = HealthState.UNKNOWN runner_instances.append(runner_instance) continue @@ -193,7 +198,9 @@ def get_runners(self) -> tuple[RunnerInstance, ...]: cloud_runner.health = HealthState.HEALTHY else: cloud_runner.health = HealthState.UNHEALTHY - runner_instance = RunnerInstance(cloud_runner, health_runner) + runner_instance = RunnerInstance.from_cloud_and_platform_health( + cloud_runner, health_runner + ) runner_instances.append(runner_instance) return cast(tuple[RunnerInstance], tuple(runner_instances)) @@ -267,7 +274,9 @@ def _cleanup_resources( ) cloud_runners = self._cloud.get_runners() logger.info("cleanup cloud_runners %s", cloud_runners) - runners_health_response = self._platform.get_runners_health(cloud_runners) + runners_health_response = self._platform.get_runners_health( + requested_runners=cloud_runners + ) logger.info("cleanup health_response %s", runners_health_response) # Clean dangling resources in the cloud @@ -294,7 +303,7 @@ def _cleanup_resources( ) ) - if maximum_runners_to_delete: + if maximum_runners_to_delete is not None: cloud_runners_to_delete.sort( key=partial(_runner_deletion_sort_key, health_runners_map) ) @@ -345,6 +354,7 @@ def _delete_cloud_runners( set(platform_runner_ids_to_delete) - set(deleted_runner_ids), ) + logger.info("Cloud runners: %s", cloud_runners) cloud_vm_ids_to_delete = [ runner.instance_id for runner in cloud_runners @@ -538,7 +548,7 @@ def _create_runner(args: _CreateRunnerArgs) -> InstanceID: ) except RunnerError: logger.warning("Deleting runner %s from platform after creation failed", instance_id) - args.platform_provider.delete_runner(runner_info.identity) + args.platform_provider.delete_runners(runner_ids=[args.metadata.runner_id]) raise return instance_id diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py b/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py index f33920edb1..7603890789 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py @@ -94,7 +94,7 @@ class _ReconcileMetricData: start_timestamp: float end_timestamp: float metric_stats: IssuedMetricEventsStats - runner_list: tuple[RunnerInstance] + runner_list: tuple[RunnerInstance, ...] flavor: str expected_runner_quantity: int @@ -373,7 +373,7 @@ def _reconcile_non_reactive(self, expected_quantity: int) -> _ReconcileResult: return _ReconcileResult(runner_diff=runner_diff, metric_stats=metric_stats) @staticmethod - def _log_runners(runner_list: tuple[RunnerInstance]) -> None: + def _log_runners(runner_list: tuple[RunnerInstance, ...]) -> None: """Log information about the runners found. Args: diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py index 9fc653f4f4..0ceed7c1d6 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_runner_manager.py @@ -160,16 +160,14 @@ def cleanup(self) -> None: """Cleanup runner and resource on the cloud.""" self._openstack_cloud.cleanup() - def _build_cloud_runner_instance( - self, instance: OpenstackInstance, healthy: bool | None = None - ) -> CloudRunnerInstance: + def _build_cloud_runner_instance(self, instance: OpenstackInstance) -> CloudRunnerInstance: """Build a new cloud runner instance from an openstack instance.""" metadata = instance.metadata return CloudRunnerInstance( name=instance.instance_id.name, metadata=metadata, instance_id=instance.instance_id, - health=HealthState.from_value(healthy), + health=HealthState.UNKNOWN, state=CloudRunnerState.from_openstack_server_status(instance.status), created_at=instance.created_at, ) diff --git a/github-runner-manager/tests/unit/factories/metrics_factory.py b/github-runner-manager/tests/unit/factories/metrics_factory.py new file mode 100644 index 0000000000..883e920386 --- /dev/null +++ b/github-runner-manager/tests/unit/factories/metrics_factory.py @@ -0,0 +1,78 @@ +# Copyright 2025 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Factories for Metrics objects.""" + +import factory + +from github_runner_manager.manager.cloud_runner_manager import CodeInformation +from github_runner_manager.metrics.events import Event, RunnerInstalled, RunnerStop + + +class EventFactory(factory.Factory): + """Factory for creating Event instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = Event + + timestamp = factory.Faker("unix_time", end_datetime="now") + event = factory.LazyAttribute(lambda obj: obj.__class__.__name__.lower()) + + +class RunnerInstalledFactory(EventFactory): + """Factory for creating RunnerInstalled instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = RunnerInstalled + + flavor = factory.Faker("word", ext_word_list=["large", "xlarge"]) + duration = factory.Faker("random_int", min=1, max=3600) + + +class CodeInformationFactory(factory.Factory): + """Factory for creating CodeInformation instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = CodeInformation + + code = factory.Faker("random_int", min=100, max=599) # Status code in the rang + + +class RunnerStopFactory(EventFactory): + """Factory for creating RunnerStop instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = RunnerStop + + flavor = factory.Faker("word", ext_word_list=["large", "xlarge"]) + workflow = factory.Faker("sentence", nb_words=3) + repo = factory.Faker("word") + github_event = factory.Faker("word") + status = factory.Faker("sentence", nb_words=5) + status_info = factory.SubFactory(CodeInformationFactory) + job_duration = factory.Faker("random_int", min=1, max=3600) + job_conclusion = factory.Faker("word", ext_word_list=["success", "failure", "cancelled"]) diff --git a/github-runner-manager/tests/unit/factories/runner_instance_factory.py b/github-runner-manager/tests/unit/factories/runner_instance_factory.py new file mode 100644 index 0000000000..203a3fed4d --- /dev/null +++ b/github-runner-manager/tests/unit/factories/runner_instance_factory.py @@ -0,0 +1,214 @@ +# Copyright 2025 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Factories for Runner instance objects.""" + +import secrets +from datetime import datetime, timezone + +import factory + +from github_runner_manager.manager.cloud_runner_manager import ( + CloudRunnerInstance, + CloudRunnerState, +) +from github_runner_manager.manager.cloud_runner_manager import HealthState as CloudHelathState +from github_runner_manager.manager.models import InstanceID, RunnerIdentity, RunnerMetadata +from github_runner_manager.manager.runner_manager import RunnerInstance +from github_runner_manager.platform.platform_provider import ( + PlatformRunnerHealth, + PlatformRunnerState, +) +from github_runner_manager.types_.github import SelfHostedRunner, SelfHostedRunnerLabel + + +class InstanceIDFactory(factory.Factory): + """Factory class for creating InstanceID.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = InstanceID + + prefix = factory.Faker("word") + reactive = factory.Iterator([True, False, None]) + suffix = factory.LazyAttribute(lambda _: secrets.token_hex(6)) + + +class RunnerMetadataFactory(factory.Factory): + """Factory for creating RunnerMetadata instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = RunnerMetadata + + platform_name = factory.Faker("word", ext_word_list=["github", "jobmanager"]) + runner_id = str(factory.Faker("random_int", min=1, max=10000)) + url = factory.Faker("url") + + +class CloudRunnerInstanceFactory(factory.Factory): + """Factory for creating CloudRunnerInstance instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = CloudRunnerInstance + + name = factory.Faker("word") + instance_id = factory.SubFactory(InstanceIDFactory) + metadata = factory.SubFactory(RunnerMetadataFactory) + health = CloudHelathState.HEALTHY + state = CloudRunnerState.ACTIVE + created_at = factory.LazyFunction(lambda: datetime.now(tz=timezone.utc)) + + @classmethod + def from_self_hosted_runner(cls, self_hosted_runner: SelfHostedRunner) -> CloudRunnerInstance: + """Construct CloudRunnerInstance associated to self hosted runner. + + Args: + self_hosted_runner: The target self hosted runner to associate. + + Returns: + The Instantiated CloudRunnerInstance. + """ + return CloudRunnerInstanceFactory( + instance_id=self_hosted_runner.identity.instance_id, + metadata=RunnerMetadataFactory(runner_id=str(self_hosted_runner.id)), + ) + + +class RunnerIdentityFactory(factory.Factory): + """Factory for creating RunnerIdentity instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = RunnerIdentity + + instance_id = factory.SubFactory(InstanceIDFactory) + metadata = factory.SubFactory(RunnerMetadataFactory) + + +class PlatformRunnerHealthFactory(factory.Factory): + """Factory for creating PlatformRunnerHealth instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = PlatformRunnerHealth + + identity = factory.SubFactory(RunnerIdentityFactory) # Create a related RunnerIdentity + online = factory.Faker("boolean") # Random boolean for online status + busy = factory.Faker("boolean") # Random boolean for busy status + deletable = factory.Faker("boolean") # Random boolean for deletable status + runner_in_platform = factory.Faker("boolean", chance_of_getting_true=90) # Mostly true + + +class RunnerInstanceFactory(factory.Factory): + """Factory for creating RunnerInstance instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = RunnerInstance + + name = factory.Faker("name") + instance_id = factory.SubFactory(InstanceIDFactory) + metadata = factory.LazyAttribute(lambda _: {"key": "value"}) + health = CloudHelathState.HEALTHY + platform_state = platform_state = factory.LazyFunction( + lambda: secrets.choice(list(PlatformRunnerState)) + ) + cloud_state = CloudRunnerState.ACTIVE + + @classmethod + def from_state( + cls, cloud_runner: CloudRunnerInstance, platform_health: PlatformRunnerHealth | None = None + ) -> RunnerInstance: + """Generate RunnerInstance from cloud runner and platform runner states. + + Args: + cloud_runner: The cloud runner to generate state from. + platform_health: The platform runner to generate state from. + + Returns: + The generated RunnerInstance. + """ + return RunnerInstance( + name=cloud_runner.name, + instance_id=cloud_runner.instance_id, + metadata=cloud_runner.metadata, + health=cloud_runner.health, + platform_state=( + PlatformRunnerState.from_platform_health(platform_health) + if platform_health is not None + else None + ), + cloud_state=cloud_runner.state, + ) + + +class SelfHostedRunnerLabelFactory(factory.Factory): + """Factory for creating SelfHostedRunnerLabel instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = SelfHostedRunnerLabel + + name = factory.Faker("word") # Generate a random word for the label name + + +class SelfHostedRunnerFactory(factory.Factory): + """Factory for creating SelfHostedRunner instances.""" + + class Meta: + """Meta class for RunnerInstance. + + Attributes: + model: The metadata reference model. + """ + + model = SelfHostedRunner + + busy = factory.Faker("boolean") + id = factory.Faker("random_int", min=1, max=10000) + labels = factory.List([factory.SubFactory(SelfHostedRunnerLabelFactory) for _ in range(3)]) + status = factory.Faker("word", ext_word_list=["online", "offline"]) + deletable = factory.Faker("boolean", chance_of_getting_true=50) + # identity.metadata.runner_id should be equal to the id attribute. + identity = factory.LazyAttribute( + lambda obj: RunnerIdentityFactory( + metadata=RunnerMetadata(platform_name="github", runner_id=obj.id), + ) + ) diff --git a/github-runner-manager/tests/unit/manager/test_runner_manager.py b/github-runner-manager/tests/unit/manager/test_runner_manager.py index 7cc6cd1dae..d29df45e4b 100644 --- a/github-runner-manager/tests/unit/manager/test_runner_manager.py +++ b/github-runner-manager/tests/unit/manager/test_runner_manager.py @@ -7,102 +7,192 @@ import pytest -from github_runner_manager.errors import RunnerCreateError from github_runner_manager.manager.cloud_runner_manager import ( CloudRunnerInstance, CloudRunnerManager, ) -from github_runner_manager.manager.models import ( - InstanceID, - RunnerContext, - RunnerIdentity, - RunnerMetadata, +from github_runner_manager.manager.models import RunnerMetadata +from github_runner_manager.manager.runner_manager import FlushMode, RunnerInstance, RunnerManager +from github_runner_manager.platform.platform_provider import PlatformProvider +from github_runner_manager.types_.github import SelfHostedRunner +from tests.unit.factories.runner_instance_factory import ( + CloudRunnerInstanceFactory, + RunnerInstanceFactory, + SelfHostedRunnerFactory, ) -from github_runner_manager.manager.runner_manager import RunnerManager -from github_runner_manager.platform.platform_provider import ( - PlatformProvider, - RunnersHealthResponse, -) -from github_runner_manager.types_.github import GitHubRunnerStatus, SelfHostedRunner +from tests.unit.mock_runner_managers import MockCloudRunnerManager, MockGitHubRunnerPlatform -def test_cleanup_removes_runners_in_platform_not_in_cloud(monkeypatch: pytest.MonkeyPatch): +@pytest.mark.parametrize( + "initial_runners, initial_cloud_runners, expected_runners, expected_cloud_runners, flush_mode", + [ + pytest.param( + [SelfHostedRunnerFactory()], + [], + [], + [], + FlushMode.FLUSH_IDLE, + id="one platform runner not in cloud is cleaned up", + ), + pytest.param( + [ + ( + idle_runner_with_cloud := SelfHostedRunnerFactory( + busy=False, + status="online", + ) + ), + ], + [ + runner_with_platform := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=idle_runner_with_cloud + ) + ], + [], + [], + FlushMode.FLUSH_IDLE, + id="one idle platform runner, matching cloud runner in cloud flushed", + ), + pytest.param( + [ + ( + busy_runner_with_cloud := SelfHostedRunnerFactory( + busy=True, + status="online", + deletable=False, + ) + ), + ], + [ + runner_with_platform := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=busy_runner_with_cloud + ) + ], + [busy_runner_with_cloud], + [runner_with_platform], + FlushMode.FLUSH_IDLE, + id="one busy platform runner, matching cloud runner in cloud is not flushed", + ), + pytest.param( + [ + ( + busy_runner_with_cloud := SelfHostedRunnerFactory( + busy=True, + status="online", + deletable=False, + ) + ), + ], + [ + runner_with_platform := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=busy_runner_with_cloud + ) + ], + [], + [], + FlushMode.FLUSH_BUSY, + id="one busy platform runner, matching cloud runner in cloud is flushed in flush busy", + ), + ], +) +def test_flush_runners( + initial_runners: list[SelfHostedRunner], + initial_cloud_runners: list[CloudRunnerInstance], + expected_runners: list[SelfHostedRunner], + expected_cloud_runners: list[CloudRunnerInstance], + flush_mode: FlushMode, +): """ - arrange: Given a runner in GitHub that is not in the cloud provider. - act: Call cleanup in the RunnerManager instance. - assert: The github runner should be deleted. + arrange: Given GitHub runners and Cloud runners. + act: Call flush in the RunnerManager instance. + assert: Expected github runners and cloud runners are flushed. """ - instance_id = InstanceID.build("prefix-0") - github_runner_identity = RunnerIdentity( - instance_id=instance_id, metadata=RunnerMetadata(platform_name="github", runner_id="1") - ) - - cloud_runner_manager = MagicMock() - cloud_runner_manager.get_runners.return_value = [] - github_provider = MagicMock() - runner_manager = RunnerManager( - "managername", - platform_provider=github_provider, - cloud_runner_manager=cloud_runner_manager, - labels=["label1", "label2"], + mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + manager = RunnerManager( + "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) - github_provider.get_runners_health.return_value = RunnersHealthResponse( - non_requested_runners=[github_runner_identity] - ) - - runner_manager.cleanup() + manager.flush_runners(flush_mode=flush_mode) - github_provider.delete_runner.assert_called_with(github_runner_identity) - cloud_runner_manager.delete_runner.assert_not_called() + assert list(mock_platform._runners.values()) == expected_runners + assert list(mock_cloud._cloud_runners.values()) == expected_cloud_runners -def test_failed_runner_in_openstack_cleans_github(monkeypatch: pytest.MonkeyPatch): +@pytest.mark.parametrize( + "initial_runners, initial_cloud_runners, expected_runners, expected_cloud_runners", + [ + pytest.param( + [SelfHostedRunnerFactory()], [], [], [], id="one platform runner not in cloud" + ), + pytest.param( + [ + (runner_with_cloud := SelfHostedRunnerFactory()), + SelfHostedRunnerFactory(), + ], + [ + runner_with_platform := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=runner_with_cloud + ) + ], + [runner_with_cloud], + [runner_with_platform], + id="one platform runner not in cloud, one in cloud", + ), + pytest.param( + [], + [runner_without_platform := CloudRunnerInstanceFactory()], + [], + [runner_without_platform], + id="cloud runner only in cloud", + ), + pytest.param( + [], + [runner_with_platform], + [], + [runner_with_platform], + id="cloud runner with platform runner only in cloud", + ), + pytest.param( + [SelfHostedRunnerFactory(), SelfHostedRunnerFactory(), SelfHostedRunnerFactory()], + [], + [], + [], + id="multiple runners not in cloud, none in cloud", + ), + pytest.param( + [runner_with_cloud, SelfHostedRunnerFactory()], + [runner_with_platform, runner_without_platform := CloudRunnerInstanceFactory()], + [runner_with_cloud], + [runner_with_platform, runner_without_platform], + id="some in cloud, some not in cloud", + ), + ], +) +def test_runner_maanger_cleanup( + initial_runners: list[SelfHostedRunner], + initial_cloud_runners: list[CloudRunnerInstance], + expected_runners: list[SelfHostedRunner], + expected_cloud_runners: list[CloudRunnerInstance], +): """ - arrange: Prepare a RunnerManager with a cloud manager that will fail when creating a runner. - act: Create a Runner. - assert: When there was a failure to create a runner in the cloud manager, - only that github runner will be deleted in GitHub. + arrange: Given GitHub runners and Cloud runners. + act: Call cleanup in the RunnerManager instance. + assert: Expected github runners and cloud runners cleanup is run. """ - cloud_instances: tuple[CloudRunnerInstance, ...] = () - cloud_runner_manager = MagicMock() - cloud_runner_manager.get_runners.return_value = cloud_instances - cloud_runner_manager.name_prefix = "unit-0" - github_provider = MagicMock(spec=PlatformProvider) - - runner_manager = RunnerManager( - "managername", - platform_provider=github_provider, - cloud_runner_manager=cloud_runner_manager, - labels=["label1", "label2"], - ) - - identity = RunnerIdentity( - instance_id=InstanceID.build("invalid"), - metadata=RunnerMetadata(platform_name="github", runner_id="1"), + mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + manager = RunnerManager( + "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) - github_runner = SelfHostedRunner( - identity=identity, - id=1, - labels=[], - status=GitHubRunnerStatus.OFFLINE, - busy=True, - ) - - def _get_runner_context(instance_id, metadata, labels): - """Return the runner context.""" - nonlocal github_runner - github_runner.identity.instance_id = instance_id - return RunnerContext(shell_run_script="agent"), github_runner - github_provider.get_runner_context.side_effect = _get_runner_context - cloud_runner_manager.create_runner.side_effect = RunnerCreateError("") + manager.cleanup() - _ = runner_manager.create_runners(1, RunnerMetadata(), True) - github_provider.delete_runner.assert_called_once_with(github_runner.identity) + assert list(mock_platform._runners.values()) == expected_runners + assert list(mock_cloud._cloud_runners.values()) == expected_cloud_runners -def test_create_runner() -> None: +def test_runner_manager_create_runners() -> None: """ arrange: None. act: call runner_manager.create_runners. @@ -127,3 +217,147 @@ def test_create_runner() -> None: assert instance_id cloud_runner_manager.create_runner.assert_called_once() + + +@pytest.mark.parametrize( + "initial_runners, initial_cloud_runners, expected_runner_instances", + [ + pytest.param([], [], (), id="no runners"), + pytest.param( + [SelfHostedRunnerFactory()], [], (), id="platform runner without cloud runner" + ), + pytest.param( + [], + [cloud_runner := CloudRunnerInstanceFactory()], + (RunnerInstanceFactory(name=cloud_runner.name),), + id="cloud runner without platform runner", + ), + pytest.param( + [runner_with_cloud := SelfHostedRunnerFactory()], + [ + cloud_runner := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=runner_with_cloud + ) + ], + (RunnerInstanceFactory(name=cloud_runner.name),), + id="platform runner with cloud runner", + ), + ], +) +def test_runner_manager_get_runners( + initial_runners: list[SelfHostedRunner], + initial_cloud_runners: list[CloudRunnerInstance], + expected_runner_instances: tuple[RunnerInstance], +): + """ + arrange: Given GitHub runners and Cloud runners. + act: when RunnerManager.get_runners is called. + assert: expected RunnerInstances are returned. + """ + mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + manager = RunnerManager( + "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] + ) + + # Test for number of runners matching and that the runner belongs to the cloud instance + # provided. The instance contents cannot be tested due to coupled logic in how the + # RunnerInstance is generated - which would require business logic in the test of setting up + # RunnerInstances from cloud runners and self hosted runners. + result = manager.get_runners() + assert len(result) == len(expected_runner_instances) + runner_names = {runner.name for runner in result} + assert all(runner.name in runner_names for runner in expected_runner_instances) + + +@pytest.mark.parametrize( + "initial_runners, initial_cloud_runners, num_delete, expected_runners, expected_cloud_runners", + [ + pytest.param([], [], 1, [], [], id="no runners to delete"), + pytest.param( + [runner_with_cloud := SelfHostedRunnerFactory()], + [ + runner_with_platform := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=runner_with_cloud + ) + ], + 0, + [runner_with_cloud], + [runner_with_platform], + id="num delete runners 0", + ), + pytest.param( + [runner_with_cloud], + [runner_with_platform], + 1, + [], + [], + id="delete 1 runner", + ), + ], +) +def test_runner_manager_deterministic_delete_runners( + initial_runners: list[SelfHostedRunner], + initial_cloud_runners: list[CloudRunnerInstance], + num_delete: int, + expected_runners: list[SelfHostedRunner], + expected_cloud_runners: list[CloudRunnerInstance], +): + """ + arrange: given initial runners (platform and cloud). + act: when RunnerManager.delete_runners is called. + assert: expected cloud & platform runners remain. + """ + mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + manager = RunnerManager( + "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] + ) + + manager.delete_runners(num_delete) + + assert list(mock_platform._runners.values()) == expected_runners + assert list(mock_cloud._cloud_runners.values()) == expected_cloud_runners + + +@pytest.mark.parametrize( + "initial_runners, initial_cloud_runners, num_delete," + "expected_runners_count, expected_cloud_runners_count", + [ + pytest.param( + [runner_with_cloud, runner_with_cloud_two := SelfHostedRunnerFactory()], + [ + runner_with_platform, + runner_with_platform_two := CloudRunnerInstanceFactory.from_self_hosted_runner( + self_hosted_runner=runner_with_cloud_two + ), + ], + 1, + 1, + 1, + id="delete 1 runner out of two runners", + ), + ], +) +def test_runner_manager_non_deterministic_delete_runners( + initial_runners: list[SelfHostedRunner], + initial_cloud_runners: list[CloudRunnerInstance], + num_delete: int, + expected_runners_count: int, + expected_cloud_runners_count: int, +): + """ + arrange: given initial runners (platform and cloud). + act: when RunnerManager.delete_runners is called. + assert: expected cloud & platform runners remain. + """ + mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + manager = RunnerManager( + "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] + ) + + manager.delete_runners(num_delete) + + assert len(mock_platform._runners.values()) == expected_runners_count + assert len(mock_platform._runners.values()) == expected_cloud_runners_count diff --git a/github-runner-manager/tests/unit/mock_runner_managers.py b/github-runner-manager/tests/unit/mock_runner_managers.py index 051b2b2efe..a8f156fe43 100644 --- a/github-runner-manager/tests/unit/mock_runner_managers.py +++ b/github-runner-manager/tests/unit/mock_runner_managers.py @@ -2,17 +2,13 @@ # See LICENSE file for licensing details. import hashlib import logging -import random import secrets from dataclasses import dataclass from datetime import datetime, timezone -from typing import Iterable, Sequence -from unittest.mock import MagicMock +from typing import Sequence from pydantic import HttpUrl -from github_runner_manager.configuration.github import GitHubPath -from github_runner_manager.github_client import GithubClient from github_runner_manager.manager.cloud_runner_manager import ( CloudRunnerInstance, CloudRunnerManager, @@ -24,10 +20,9 @@ RunnerIdentity, RunnerMetadata, ) +from github_runner_manager.manager.runner_manager import RunnerInstance from github_runner_manager.metrics.runner import RunnerMetrics -from github_runner_manager.platform.github_provider import ( - PlatformRunnerState, -) +from github_runner_manager.platform.github_provider import PlatformRunnerState from github_runner_manager.platform.platform_provider import ( JobInfo, PlatformProvider, @@ -40,6 +35,7 @@ RunnerApplication, SelfHostedRunner, ) +from tests.unit.factories.runner_instance_factory import CloudRunnerInstanceFactory logger = logging.getLogger(__name__) @@ -270,222 +266,217 @@ class MockCloudRunnerManager(CloudRunnerManager): Attributes: name_prefix: The naming prefix for runners managed. - prefix: The naming prefix for runners managed. - state: The shared state between mocks runner managers. """ - def __init__(self, state: SharedMockRunnerManagerState): - """Construct the object. + @property + def name_prefix(self) -> str: + """The naming prefix for runners managed.""" + return "mock_cloud_runner_manager" + + def __init__(self, initial_cloud_runners: list[CloudRunnerInstance]) -> None: + """Initialize the Cloud Runner Manager. Args: - state: The shared state between cloud and github runner managers. + initial_cloud_runners: A list of initial Cloud Runner Instances. """ - self.prefix = f"mock_{secrets.token_hex(4)}" - self.state = state - - @property - def name_prefix(self) -> str: - """Get the name prefix of the self-hosted runners.""" - return self.prefix + self._cloud_runners = {runner.instance_id: runner for runner in initial_cloud_runners} def create_runner( - self, - runner_identity: RunnerIdentity, - runner_context: RunnerContext, - ) -> None: - """Create a self-hosted runner. + self, runner_identity: RunnerIdentity, runner_context: RunnerContext + ) -> CloudRunnerInstance: + """Create a runner instance for the given runner identity and context. Args: - runner_identity: Identity of the runner to create. - runner_context: Context for the runner. + runner_identity: The runner identity to create a runner for. + runner_context: The context for the runner to create a runner for. Returns: - The CloudRunnerInstance for the runner + The created runner instance. """ - runner = MockRunner(runner_identity.instance_id) - self.state.runners[runner_identity.instance_id] = runner - return runner.to_cloud_runner() + created_runner = CloudRunnerInstanceFactory(instance_id=runner_identity.instance_id) + self._cloud_runners[runner_identity.instance_id] = created_runner + return created_runner def get_runners(self) -> Sequence[CloudRunnerInstance]: - """Get cloud self-hosted runners. + """Get all the cloud runner instances managed by the manager. + + Returns: + A list of cloud runner instances. + """ + return list(self._cloud_runners.values()) + + def delete_vms(self, instance_ids: Sequence[InstanceID]) -> list[InstanceID]: + """Delete VMs with given instance ids. + + Args: + instance_ids: A list of instance ids to delete. Returns: - Information on the runner instances. + A list of instance ids that were deleted. """ - return [runner.to_cloud_runner() for runner in self.state.runners.values()] + deleted_instance_ids: list[InstanceID] = [] + for instance_id in instance_ids: + cloud_runner = self._cloud_runners.pop(instance_id, None) + if not cloud_runner: + continue + deleted_instance_ids.append(cloud_runner.instance_id) + return deleted_instance_ids - def delete_runner(self, instance_id: InstanceID) -> RunnerMetrics | None: - """Delete self-hosted runner. + def extract_metrics(self, instance_ids: Sequence[InstanceID]) -> list[RunnerMetrics]: + """Extract metrics from VMs with given instance ids. + + The mock runner manager does not implement this. Args: - instance_id: The instance id of the runner to delete. + instance_ids: A list of instance ids to extract metrics from. Returns: - Any runner metrics produced during deletion. + A list of metrics extracted from VMs with given instance ids. """ - runner = self.state.runners.pop(instance_id, None) - if runner is not None: - return MagicMock() return [] def cleanup(self) -> None: - """Cleanup runner dangling resources on the cloud.""" + """Cleanup cloud resources. + The mock runner manager does not implement this. + """ + pass -class MockGitHubRunnerPlatform(PlatformProvider): - """Mock of GitHubRunnerPlatform. - Attributes: - github: The GitHub client. - name_prefix: The naming prefix for runner managed. - state: The shared state between mock runner managers. - path: The GitHub path to register the runners under. - """ +class MockGitHubRunnerPlatform(PlatformProvider): + """Mock GitHub platform provider.""" - def __init__(self, name_prefix: str, path: GitHubPath, state: SharedMockRunnerManagerState): - """Construct the object. + def __init__(self, initial_runners: Sequence[SelfHostedRunner]) -> None: + """Initialize the mock platform. Args: - name_prefix: The naming prefix for runner managed. - path: The GitHub path to register the runners under. - state: The shared state between mock runner managers. + initial_runners: Runners to instantiate the platform with. """ - self.github = GithubClient("mock_token") - self.github._client = MockGhapiClient("mock_token") - self.name_prefix = name_prefix - self.state = state - self.path = path - - def get_runner_health( - self, - runner_identity: RunnerIdentity, - ) -> PlatformRunnerHealth: - """Get info on self-hosted runner. + self._runners = {runner.identity.instance_id: runner for runner in initial_runners} + + def get_runner_health(self, runner_identity: RunnerIdentity) -> PlatformRunnerHealth: + """Get runner health of a runner with given runner identity. Args: - runner_identity: Identity of the runner. + runner_identity: The identity of the runner to query health status. Returns: - Information about the health of the runner + The PlatformRunnerHealth status of the runner. """ - if runner_identity.instance_id in self.state.runners: - runner = self.state.runners[runner_identity.instance_id] + runner = self._runners.get(runner_identity.instance_id, None) + if not runner: return PlatformRunnerHealth( identity=runner_identity, - online=runner.platform_state != PlatformRunnerState.OFFLINE, - busy=runner.platform_state == PlatformRunnerState.BUSY, - deletable=runner.deletable, + online=False, + busy=False, + deletable=True, + runner_in_platform=False, ) return PlatformRunnerHealth( identity=runner_identity, - online=False, - busy=False, - deletable=True, + online=runner.status == GitHubRunnerStatus.ONLINE, + busy=runner.busy, + deletable=False, ) - def get_runners_health( - self, requested_runners: list[RunnerIdentity] - ) -> "list[PlatformRunnerHealth]": - """Get information from the requested runners health. + def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> RunnersHealthResponse: + """Batch get runners health. Args: - requested_runners: List of runners to get health information for. + requested_runners: The runners to get. the health information for. Returns: - Health information for the runners. + The requested runners health info. """ - found_identities = [] - for identity in requested_runners: - if identity.instance_id in self.state.runners: - runner = self.state.runners[identity.instance_id] - if runner.health: - found_identities.append(identity) - requested_runners = [self.get_runner_health(identity) for identity in found_identities] - return RunnersHealthResponse(requested_runners=requested_runners) - - def get_runner_context( - self, metadata: RunnerMetadata, instance_id: str, labels: list[str] - ) -> tuple[RunnerContext, SelfHostedRunner]: - """Get the registration JIT token for registering runners on GitHub. + response = RunnersHealthResponse() + + for requested_runner in requested_runners: + runner = self._runners.get(requested_runner.instance_id, None) + if runner: + response.requested_runners.append( + self.get_runner_health(runner_identity=runner.identity) + ) + continue + response.failed_requested_runners.append(requested_runner) + + requested_runner_ids = set(runner.instance_id for runner in requested_runners) + for instance_id, runner in self._runners.items(): + if instance_id in requested_runner_ids: + continue + response.non_requested_runners.append(runner.identity) + return response + + def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: + """Delete runners from platform. Args: - metadata: Metadata of the server. - instance_id: Instance ID of the runner. - labels: Labels for the runner. + runner_ids: The runner IDs to delete. + platform: The target platform. Returns: - The registration token and the SelfHostedRunner + The successfully deleted runners. """ - runner = MagicMock(spec=list(SelfHostedRunner.__fields__.keys())) - runner.id = 5 - return RunnerContext(shell_run_script="fake-agent"), runner + deleted_runner_ids: list[str] = [] + runner_id_map = {str(runner.id): runner for runner in self._runners.values()} + for runner_id in runner_ids: + runner = runner_id_map.get(runner_id, None) + if not runner: + continue + self._runners.pop(runner.identity.instance_id) + deleted_runner_ids.append(runner_id) + return deleted_runner_ids - def get_runners( - self, states: Iterable[PlatformRunnerState] | None = None - ) -> tuple[SelfHostedRunner, ...]: - """Get the runners. + def get_runner_context( + self, metadata: RunnerMetadata, instance_id: InstanceID, labels: list[str] + ) -> tuple[RunnerContext, SelfHostedRunner]: + """Get a context for a runner. Args: - states: The states to filter for. - - Returns: - List of runners. - """ - if states is None: - states = [member.value for member in PlatformRunnerState] - - platform_state_set = set(states) - runner_id = random.randint(1, 1000000) - return tuple( - SelfHostedRunner( - busy=runner.platform_state == PlatformRunnerState.BUSY, - id=runner_id, - labels=[], - instance_id=InstanceID.build_from_name(self.name_prefix, runner.name), - status=( - GitHubRunnerStatus.OFFLINE - if runner.platform_state == PlatformRunnerState.OFFLINE - else GitHubRunnerStatus.ONLINE - ), - metadata=RunnerMetadata(platform_name="github", runner_id=str(runner_id)), - ) - for runner in self.state.runners.values() - if runner.platform_state in platform_state_set - ) + metadata: The runner's metadata. + instance_id: The ID of the instance. + labels: The labels of the instance. - def delete_runner(self, runner_identity: RunnerIdentity) -> None: - """Delete a runner. - - Args: - runner_identity: Runner to delete. + Raises: + NotImplementedError: This method is not tested with this mock. """ - if runner_identity.instance_id in self.state.runners: - del self.state.runners[runner_identity.instance_id] + raise NotImplementedError def check_job_been_picked_up(self, metadata: RunnerMetadata, job_url: HttpUrl) -> bool: - """Check if the job has already been picked up. + """Check if a job has been picked up by the runner. Args: - metadata: Metadata of the instance. + metadata: The metadata of the runner. job_url: The URL of the job. Raises: - NotImplementedError: Work in progress. + NotImplementedError: This method is not tested with this mock. """ raise NotImplementedError def get_job_info( self, metadata: RunnerMetadata, repository: str, workflow_run_id: str, runner: InstanceID ) -> JobInfo: - """Get the Job info from the provider. + """Get information about a job. Args: - metadata: Metadata of the runner. - repository: repository to get the job from. - workflow_run_id: workflow run id of the job. - runner: runner to get the job from. + metadata: The metadata of the runner. + repository: The name of the repository. + workflow_run_id: The ID of the workflow run. + runner: The ID of the runner. Raises: - NotImplementedError: Work in progress. + NotImplementedError: This method is not tested with this mock. """ raise NotImplementedError + + +class MockRunnerManager: + """Mock Runner manager for testing.""" + + def __init__(self, runners: Sequence[RunnerInstance]) -> None: + """Initialize the mock runner manager. + + Args: + runners: The runners to initialize the RunnerManager with. + """ + self._runners = runners diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index 8a3c6a3957..e606803ba6 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -83,7 +83,7 @@ def mock_openstack_conn_fixture(monkeypatch: pytest.MonkeyPatch): id="launch_instance", ), pytest.param("get_instance", {"instance_id": FAKE_ARG}, id="get_instance"), - pytest.param("delete_instance", {"instance_id": FAKE_ARG}, id="delete_instance"), + pytest.param("delete_instances", {"instance_ids": FAKE_ARG}, id="delete_instances"), pytest.param("get_instances", {}, id="get_instances"), pytest.param("cleanup", {}, id="cleanup"), ], diff --git a/github-runner-manager/tests/unit/test_runner_scaler.py b/github-runner-manager/tests/unit/test_runner_scaler.py index 4b0fe90fac..dd415daa61 100644 --- a/github-runner-manager/tests/unit/test_runner_scaler.py +++ b/github-runner-manager/tests/unit/test_runner_scaler.py @@ -30,13 +30,16 @@ GitHubPath, GitHubRepo, ) -from github_runner_manager.errors import CloudError, ReconcileError from github_runner_manager.manager import runner_manager as runner_manager_module from github_runner_manager.manager.cloud_runner_manager import CloudRunnerState from github_runner_manager.manager.models import InstanceID -from github_runner_manager.manager.runner_manager import FlushMode, RunnerManager -from github_runner_manager.manager.runner_scaler import RunnerScaler -from github_runner_manager.metrics.events import Reconciliation +from github_runner_manager.manager.runner_manager import ( + IssuedMetricEventsStats, + RunnerInstance, + RunnerManager, +) +from github_runner_manager.manager.runner_scaler import FlushMode, RunnerInfo, RunnerScaler +from github_runner_manager.metrics.events import RunnerStart, RunnerStop from github_runner_manager.openstack_cloud.configuration import ( OpenStackConfiguration, OpenStackCredentials, @@ -47,11 +50,7 @@ ) from github_runner_manager.platform.github_provider import PlatformRunnerState from github_runner_manager.reactive.types_ import ReactiveProcessConfig -from tests.unit.mock_runner_managers import ( - MockCloudRunnerManager, - MockGitHubRunnerPlatform, - SharedMockRunnerManagerState, -) +from tests.unit.factories.runner_instance_factory import RunnerInstanceFactory logger = logging.getLogger(__name__) @@ -79,16 +78,6 @@ def github_path_fixture() -> GitHubPath: return GitHubRepo(owner="mock_owner", repo="mock_repo") -@pytest.fixture(scope="function", name="mock_runner_managers") -def mock_runner_managers_fixture( - github_path: GitHubPath, -) -> tuple[MockCloudRunnerManager, MockGitHubRunnerPlatform]: - state = SharedMockRunnerManagerState() - mock_cloud = MockCloudRunnerManager(state) - mock_github = MockGitHubRunnerPlatform(mock_cloud.name_prefix, github_path, state) - return (mock_cloud, mock_github) - - @pytest.fixture(scope="function", name="issue_events_mock") def issue_events_mock_fixture(monkeypatch: pytest.MonkeyPatch): issue_events_mock = MagicMock() @@ -377,328 +366,188 @@ def test_build_runner_scaler( ) -def test_get_no_runner(runner_manager: RunnerManager, user_info: UserInfo): - """ - Arrange: A RunnerScaler with no runners. - Act: Get runner information. - Assert: Information should contain no runners. - """ - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=0, max_quantity=0) - assert_runner_info(runner_scaler, online=0) - - -def test_flush_no_runner(runner_manager: RunnerManager, user_info: UserInfo): - """ - Arrange: A RunnerScaler with no runners. - Act: - 1. Flush idle runners. - 2. Flush busy runners. - Assert: - 1. No change in number of runners. Runner info should contain no runners. - 2. No change in number of runners. - """ - # 1. - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=0, max_quantity=0) - diff = runner_scaler.flush(flush_mode=FlushMode.FLUSH_IDLE) - assert diff == 0 - assert_runner_info(runner_scaler, online=0) - - # 2. - diff = runner_scaler.flush(flush_mode=FlushMode.FLUSH_BUSY) - assert diff == 0 - assert_runner_info(runner_scaler, online=0) - - -def test_reconcile_runner_create_one(runner_manager: RunnerManager, user_info: UserInfo): - """ - Arrange: A RunnerScaler with no runners. - Act: Reconcile to no runners. - Assert: No changes. Runner info should contain no runners. - """ - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=0, max_quantity=0) - diff = runner_scaler.reconcile() - assert diff == 0 - assert_runner_info(runner_scaler, online=0) - - -def test_reconcile_runner_create_one_reactive( - monkeypatch: pytest.MonkeyPatch, runner_manager: RunnerManager, user_info: UserInfo +@pytest.mark.parametrize( + "runners, expected_runner_info", + [ + pytest.param( + [], + RunnerInfo(online=0, busy=0, offline=0, unknown=0, runners=(), busy_runners=()), + id="No runners", + ), + pytest.param( + [busy_runner := RunnerInstanceFactory(platform_state=PlatformRunnerState.BUSY)], + RunnerInfo( + online=1, + busy=1, + offline=0, + unknown=0, + runners=(busy_runner.name,), + busy_runners=(busy_runner.name,), + ), + id="One busy runner", + ), + pytest.param( + [idle_runner := RunnerInstanceFactory(platform_state=PlatformRunnerState.IDLE)], + RunnerInfo( + online=1, + busy=0, + offline=0, + unknown=0, + runners=(idle_runner.name,), + busy_runners=(), + ), + id="One idle runner", + ), + pytest.param( + [offline_runner := RunnerInstanceFactory(platform_state=PlatformRunnerState.OFFLINE)], + RunnerInfo( + online=0, + busy=0, + offline=1, + unknown=0, + runners=(), + busy_runners=(), + ), + id="One offline runner", + ), + pytest.param( + [unknown_runner := RunnerInstanceFactory(platform_state=None)], + RunnerInfo( + online=0, + busy=0, + offline=0, + unknown=1, + runners=(), + busy_runners=(), + ), + id="One unknown runner", + ), + pytest.param( + [busy_runner, idle_runner, offline_runner, unknown_runner], + RunnerInfo( + online=2, + busy=1, + offline=1, + unknown=1, + runners=(busy_runner.name, idle_runner.name), + busy_runners=(busy_runner.name,), + ), + id="One runner of each type", + ), + ], +) +def test_runner_scaler_get_runner_info( + runners: list[RunnerInstance], expected_runner_info: RunnerInfo ): """ - Arrange: Prepare one RunnerScaler in reactive mode. - Fake the reconcile function in reactive to return its input. - Act: Call reconcile with base quantity 0 and max quantity 5. - Assert: 5 processes should be returned in the result of the reconcile. + arrange: given a mock runner manager. + act: when RunnerScaler.get_runner_info is called. + assert the expected runner info is extracted. """ - reactive_process_config = MagicMock() + runner_manager = MagicMock() + runner_manager.get_runners.return_value = runners runner_scaler = RunnerScaler( - runner_manager, reactive_process_config, user_info, base_quantity=0, max_quantity=5 - ) - - from github_runner_manager.reactive.runner_manager import ReconcileResult - - def _fake_reactive_reconcile( - expected_quantity: int, runner_manager, reactive_process_config, user, python_path - ): - """Reactive reconcile fake.""" - return ReconcileResult(processes_diff=expected_quantity, metric_stats={"event": ""}) - - monkeypatch.setattr( - "github_runner_manager.reactive.runner_manager.reconcile", - MagicMock(side_effect=_fake_reactive_reconcile), + runner_manager=runner_manager, + reactive_process_config=None, + user=MagicMock(), + base_quantity=0, + max_quantity=0, ) - diff = runner_scaler.reconcile() - assert diff == 5 - assert_runner_info(runner_scaler, online=0) - -def test_reconcile_error_still_issue_metrics( - runner_manager: RunnerManager, - monkeypatch: pytest.MonkeyPatch, - issue_events_mock: MagicMock, - user_info: UserInfo, + assert runner_scaler.get_runner_info() == expected_runner_info + + +@pytest.mark.parametrize( + "cleanup_metrics, flush_metrics, expected_flushed", + [ + pytest.param({}, {}, 0, id="No changes"), + pytest.param({RunnerStart: 1}, {}, 0, id="No runner stop metrics"), + pytest.param({RunnerStop: 1}, {}, 1, id="Runner stop metric from cleanup"), + pytest.param({}, {RunnerStop: 1}, 1, id="Runner stop metric from flush"), + pytest.param( + {RunnerStop: 1}, + {RunnerStop: 1}, + 2, + id="Runner stop metrics from cleanup and flush", + ), + ], +) +def test_runner_scaler_flush_extract_metrics( + cleanup_metrics: IssuedMetricEventsStats, + flush_metrics: IssuedMetricEventsStats, + expected_flushed: int, ): """ - Arrange: A RunnerScaler with no runners which raises an error on reconcile. - Act: Reconcile to one runner. - Assert: ReconciliationEvent should be issued. + arrange: given a mocked runner manager with that returns the given metrics. + act: when RunnerScaler.flush is called. + assert: the expected number of flushed runners from metrics is returned. """ - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=1, max_quantity=0) - monkeypatch.setattr( - runner_scaler._manager, "cleanup", MagicMock(side_effect=Exception("Mock error")) - ) - with pytest.raises(Exception): - runner_scaler.reconcile() - issue_events_mock.assert_called_once() - issued_event = issue_events_mock.call_args[0][0] - assert isinstance(issued_event, Reconciliation) - + runner_manager = MagicMock() + runner_manager.cleanup.return_value = cleanup_metrics + runner_manager.flush_runners.return_value = flush_metrics -def test_reconcile_raises_reconcile_error( - runner_manager: RunnerManager, - monkeypatch: pytest.MonkeyPatch, - issue_events_mock: MagicMock, - user_info: UserInfo, -): - """ - Arrange: A RunnerScaler with no runners which raises a Cloud error on reconcile. - Act: Reconcile to one runner. - Assert: ReconcileError should be raised. - """ - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=1, max_quantity=0) - monkeypatch.setattr( - runner_scaler._manager, "cleanup", MagicMock(side_effect=CloudError("Mock error")) + runner_scaler = RunnerScaler( + runner_manager=runner_manager, + reactive_process_config=None, + user=MagicMock(), + base_quantity=0, + max_quantity=0, ) - with pytest.raises(ReconcileError) as exc: - runner_scaler.reconcile() - assert "Failed to reconcile runners." in str(exc.value) + assert runner_scaler.flush() == expected_flushed -def test_one_runner(runner_manager: RunnerManager, user_info: UserInfo): - """ - Arrange: A RunnerScaler with no runners. - Act: - 1. Reconcile to one runner. - 2. Reconcile to one runner. - 3. Flush idle runners. - 4. Reconcile to one runner. - Assert: - 1. Runner info has one runner. - 2. No changes to number of runner. - 3. Runner info has one runner. - """ - # 1. - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=1, max_quantity=0) - diff = runner_scaler.reconcile() - assert diff == 1 - assert_runner_info(runner_scaler, online=1) - # 2. - diff = runner_scaler.reconcile() - assert diff == 0 - assert_runner_info(runner_scaler, online=1) - - # 3. - runner_scaler.flush(flush_mode=FlushMode.FLUSH_IDLE) - assert_runner_info(runner_scaler, online=0) - - # 3. - diff = runner_scaler.reconcile() - assert diff == 1 - assert_runner_info(runner_scaler, online=1) - - -def test_flush_busy_on_idle_runner(runner_scaler_one_runner: RunnerScaler): - """ - Arrange: A RunnerScaler with one idle runner. - Act: Run flush busy runner. - Assert: No runners. - """ - runner_scaler = runner_scaler_one_runner - - runner_scaler.flush(flush_mode=FlushMode.FLUSH_BUSY) - assert_runner_info(runner_scaler, online=0) - - -def test_flush_busy_on_busy_runner( - runner_scaler_one_runner: RunnerScaler, -): - """ - Arrange: A RunnerScaler with one busy runner. - Act: Run flush busy runner. - Assert: No runners. - """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.BUSY) - - runner_scaler.flush(flush_mode=FlushMode.FLUSH_BUSY) - assert_runner_info(runner_scaler, online=0) - - -def test_get_runner_one_busy_runner( - runner_scaler_one_runner: RunnerScaler, -): - """ - Arrange: A RunnerScaler with one busy runner. - Act: Run get runners. - Assert: One busy runner. - """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.BUSY) - - assert_runner_info(runner_scaler=runner_scaler, online=1, busy=1) - - -def test_get_runner_offline_runner(runner_scaler_one_runner: RunnerScaler): - """ - Arrange: A RunnerScaler with one offline runner. - Act: Run get runners. - Assert: One offline runner. - """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.OFFLINE) - - assert_runner_info(runner_scaler=runner_scaler, offline=1) - - -def test_get_runner_unknown_runner(runner_scaler_one_runner: RunnerScaler): - """ - Arrange: A RunnerScaler with one offline runner. - Act: Run get runners. - Assert: One offline runner. - """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, health=False) - assert_runner_info(runner_scaler=runner_scaler, unknown=1) - - -def test_flush_idle_on_starting_offline_runner( - runner_scaler_one_runner: RunnerScaler, - runner_manager: RunnerManager, -): - """ - Arrange: A RunnerScaler with one offline runner that could be starting. - Act: Run flush idle runner. - Assert: The runner should not be deleted. - """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.OFFLINE) - runners_before = runner_manager.get_runners() - - runner_scaler.flush(flush_mode=FlushMode.FLUSH_IDLE) - assert_runner_info(runner_scaler, offline=1) - runners_after = runner_manager.get_runners() - assert runners_before[0].instance_id == runners_after[0].instance_id - - -def test_flush_idle_on_old_offline_runner( - runner_scaler_one_runner: RunnerScaler, - runner_manager: RunnerManager, -): +@pytest.mark.parametrize( + "flush_mode, expected_flush_mode", + [ + pytest.param(FlushMode.FLUSH_IDLE, FlushMode.FLUSH_IDLE, id="flush_idle"), + pytest.param(FlushMode.FLUSH_BUSY, FlushMode.FLUSH_BUSY, id="flush_busy"), + ], +) +def test_runner_scaler_flush_mode(flush_mode: FlushMode, expected_flush_mode: FlushMode): """ - Arrange: A RunnerScaler with one offline runner that had enough time to start. - Act: Run flush idle runner. - Assert: The runner should be deleted. No one should be created. + arrange: given a mocked runner manager. + act: when RunnerScaler.flush is called with the given flush mode. + assert: flush_runners is called with expected mode. """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.OFFLINE, old_runner=True) - - runner_scaler.flush(flush_mode=FlushMode.FLUSH_IDLE) - assert_runner_info(runner_scaler, offline=0) - + runner_manager = MagicMock() -def test_reconcile_on_starting_offline_runner( - runner_scaler_one_runner: RunnerScaler, - runner_manager: RunnerManager, -): - """ - Arrange: A RunnerScaler with one offline runner that could be starting. - Act: Run reconcile. - Assert: The runner should not be deleted. - """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.OFFLINE) - runners_before = runner_manager.get_runners() + RunnerScaler( + runner_manager=runner_manager, + reactive_process_config=None, + user=MagicMock(), + base_quantity=0, + max_quantity=0, + ).flush(flush_mode=flush_mode) - runner_scaler.reconcile() - assert_runner_info(runner_scaler, offline=1) - runners_after = runner_manager.get_runners() - assert runners_before[0].instance_id == runners_after[0].instance_id + runner_manager.flush_runners.assert_called_with(flush_mode=expected_flush_mode) -def test_reconcile_on_old_offline_runner( - runner_scaler_one_runner: RunnerScaler, - runner_manager: RunnerManager, +@pytest.mark.parametrize( + "runners, quantity, expected_diff", + [ + pytest.param([], 0, 0, id="no difference"), + pytest.param([], 1, 1, id="scale up one runner"), + pytest.param([RunnerInstanceFactory()], 0, -1, id="scale down one runner"), + ], +) +def test_runner_scaler__reconcile_non_reactive( + runners: list[RunnerInstance], quantity: int, expected_diff: int ): """ - Arrange: A RunnerScaler with one offline not busy runner that could have failed to start. - Act: Run reconcile. - Assert: The runner should be deleted. Another one will be created. + arrange: given a mocked runner manager. + act: when RunnerScaler._reconcile_non_reactive is called. + assert: expected runner diff is returned. """ - runner_scaler = runner_scaler_one_runner - set_one_runner_state(runner_scaler, PlatformRunnerState.OFFLINE, old_runner=True) - runners_before = runner_manager.get_runners() - - runner_scaler.reconcile() - assert_runner_info(runner_scaler, online=1, offline=0) - runners_after = runner_manager.get_runners() - assert runners_before[0].instance_id != runners_after[0].instance_id + runner_manager = MagicMock() + runner_manager.get_runners.return_value = runners + result = RunnerScaler( + runner_manager=runner_manager, + reactive_process_config=None, + user=MagicMock(), + base_quantity=0, + max_quantity=0, + )._reconcile_non_reactive(expected_quantity=quantity) -def test_delete_some_runners_in_reconcile(runner_manager: RunnerManager, user_info: UserInfo): - """ - Arrange: Run a reconcile to get 5 runners online. - Act: In a different runner_scaler, reconcile with 2 runners. - Assert: 3 runners should be delete and 2 runners should be online. The busy runner and the - runner without health information should be retained based on the desired ordering. - """ - runner_scaler = RunnerScaler(runner_manager, None, user_info, base_quantity=5, max_quantity=0) - diff = runner_scaler.reconcile() - assert diff == 5 - assert_runner_info(runner_scaler, online=5) - - # Update the runner_dict, so we can check that runners are deleted in order of "inconvenience". - # This test depends on the preservation of insertion order. - # See https://docs.python.org/3.7/library/stdtypes.html#dict.values - runner_dict = runner_scaler._manager._platform.state.runners - initial_mock_runners = list(runner_dict.values()) - initial_mock_runners[0].platform_state = PlatformRunnerState.IDLE - initial_mock_runners[1].platform_state = PlatformRunnerState.OFFLINE - initial_mock_runners[2].platform_state = PlatformRunnerState.BUSY - initial_mock_runners[3].deletable = True - initial_mock_runners[4].health = False # Runner without health information - - second_runner_scaler = RunnerScaler( - runner_manager, None, user_info, base_quantity=2, max_quantity=0 - ) - diff = second_runner_scaler.reconcile() - # Even as 3 runners were deleted, the deletable one was deleted in the cleanup, so - # the runner_scaler returns -2. - assert diff == -2 - assert_runner_info(second_runner_scaler, online=1, busy=1, unknown=1) - - assert len(runner_dict) == 2 - # The busy runner should not be deleted. - assert initial_mock_runners[2].instance_id in runner_dict - # The runner without health information should not be deleted - assert initial_mock_runners[4].instance_id in runner_dict + assert result.runner_diff == expected_diff From 6697dd1f0ddd9be82d0043e05149df881e001c66 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 13 Jul 2025 09:45:30 +0000 Subject: [PATCH 16/50] test: fix openstack fixture --- .../tests/unit/openstack_cloud/test_openstack_cloud.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index e606803ba6..56b5e483ff 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -63,8 +63,8 @@ def mock_openstack_conn_fixture(monkeypatch: pytest.MonkeyPatch): connection_mock = MagicMock() connection_mock.__enter__.return_value = connection_mock monkeypatch.setattr( - github_runner_manager.openstack_cloud.openstack_cloud, - "_get_openstack_connection", + github_runner_manager.openstack_cloud.openstack_cloud.openstack, + "connect", MagicMock(return_value=connection_mock), ) return connection_mock @@ -83,7 +83,6 @@ def mock_openstack_conn_fixture(monkeypatch: pytest.MonkeyPatch): id="launch_instance", ), pytest.param("get_instance", {"instance_id": FAKE_ARG}, id="get_instance"), - pytest.param("delete_instances", {"instance_ids": FAKE_ARG}, id="delete_instances"), pytest.param("get_instances", {}, id="get_instances"), pytest.param("cleanup", {}, id="cleanup"), ], From afc5c03cf3e6ee8effe2e2512758cb90ae1b4142 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 13 Jul 2025 15:02:42 +0000 Subject: [PATCH 17/50] chore: log extracted metric s --- .../src/github_runner_manager/manager/runner_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index fe5ad7e5c1..55824df7d1 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -367,7 +367,7 @@ def _delete_cloud_runners( ] logger.info("Extracting metrics from cloud VMs: %s", cloud_vm_ids_to_delete) extracted_metrics = self._cloud.extract_metrics(instance_ids=cloud_vm_ids_to_delete) - logger.info("Extracted metrics from cloud VMs.") + logger.info("Extracted metrics from cloud VMs: %s", extracted_metrics) logger.info("Deleting VMs %s", cloud_vm_ids_to_delete) deleted_vm_ids = self._cloud.delete_vms(instance_ids=cloud_vm_ids_to_delete) logger.info( From e952f64f2361b676772fd8f9eb3c8fb27002fe9e Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 13 Jul 2025 15:59:45 +0000 Subject: [PATCH 18/50] debug --- .github/workflows/integration_test.yaml | 33 ++++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index 649d47df23..3fc151e025 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -20,26 +20,29 @@ jobs: juju-channel: 3.6/stable provider: lxd test-tox-env: integration-juju3.6 - modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' - extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' - self-hosted-runner: true - self-hosted-runner-label: stg-private-endpoint - openstack-integration-tests-cross-controller-private-endpoint: - name: Cross controller integration test using private-endpoint - uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main - secrets: inherit - with: - juju-channel: 3.6/stable - pre-run-script: tests/integration/setup-integration-tests.sh - provider: lxd - test-tox-env: integration-juju3.6 - modules: '["test_prometheus_metrics"]' + # modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' + modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_path_change", "test_debug_ssh", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' self-hosted-runner: true self-hosted-runner-label: stg-private-endpoint + tmate-debug: true + tmate-timeout: 90 + # openstack-integration-tests-cross-controller-private-endpoint: + # name: Cross controller integration test using private-endpoint + # uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main + # secrets: inherit + # with: + # juju-channel: 3.6/stable + # pre-run-script: tests/integration/setup-integration-tests.sh + # provider: lxd + # test-tox-env: integration-juju3.6 + # modules: '["test_prometheus_metrics"]' + # extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' + # self-hosted-runner: true + # self-hosted-runner-label: stg-private-endpoint allure-report: if: ${{ (success() || failure()) && github.event_name == 'schedule' }} needs: - openstack-integration-tests-private-endpoint - - openstack-integration-tests-cross-controller-private-endpoint + # - openstack-integration-tests-cross-controller-private-endpoint uses: canonical/operator-workflows/.github/workflows/allure_report.yaml@main From 2dae73009392d1336046f1302a19f39c1310fc5f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 13 Jul 2025 17:00:15 +0000 Subject: [PATCH 19/50] chore: debug log --- .../src/github_runner_manager/manager/models.py | 4 ++-- .../github_runner_manager/openstack_cloud/openstack_cloud.py | 1 + tests/integration/test_charm_runner.py | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/models.py b/github-runner-manager/src/github_runner_manager/manager/models.py index 6dd10430c6..4e14f72226 100644 --- a/github-runner-manager/src/github_runner_manager/manager/models.py +++ b/github-runner-manager/src/github_runner_manager/manager/models.py @@ -29,13 +29,13 @@ class InstanceID: Attributes: name: Name of the instance to use. prefix: Prefix corresponding to the application (charm application unit). - reactive: Identifies if the runner is reactive. suffix: Random suffix for the InstanceID. + reactive: Identifies if the runner is reactive. """ prefix: str - reactive: bool | None suffix: str + reactive: bool = False @property def name(self) -> str: diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 90cdb639df..4ec3c99585 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -374,6 +374,7 @@ def delete_instances( ) for instance_id in instance_ids ] + logger.info("Deleting instances: %s", delete_configs) for deleted_instance_id in pool.imap_unordered( OpenstackCloud._delete_instance, delete_configs ): diff --git a/tests/integration/test_charm_runner.py b/tests/integration/test_charm_runner.py index 38a0cd13ab..aa10203467 100644 --- a/tests/integration/test_charm_runner.py +++ b/tests/integration/test_charm_runner.py @@ -16,6 +16,7 @@ DISPATCH_TEST_WORKFLOW_FILENAME, DISPATCH_WAIT_TEST_WORKFLOW_FILENAME, dispatch_workflow, + get_file_content, get_job_logs, wait_for, wait_for_reconcile, From 986b8a12132724f09db989f5221cc960089832c2 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 02:15:26 +0000 Subject: [PATCH 20/50] chore: try logging with file --- .../openstack_cloud/openstack_cloud.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 4ec3c99585..edb8f58686 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -319,9 +319,15 @@ def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: The deleted Instance ID. """ try: - logger.info("Deleting server %s", delete_config.instance_id.name) + Path("~/github-runner-delete.log").write_text( + f"Deleting server {delete_config.instance_id.name}" + ) + # logger.info("Deleting server %s", delete_config.instance_id.name) res = delete_config.conn.delete_server(name_or_id=delete_config.instance_id.name) - logger.info("Deleted server %s (true delete: %s)", delete_config.instance_id.name, res) + Path("~/github-runner-delete.log").write_text( + f"Deleted server {delete_config.instance_id.name} (true delete: {res})", + ) + # logger.info("Deleted server %s (true delete: %s)", delete_config.instance_id.name, res) except ( openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout, From cf076e9ab748386f57a500129c700629f66165d2 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 02:47:00 +0000 Subject: [PATCH 21/50] general exception catch w/ traceback --- .../openstack_cloud/openstack_cloud.py | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index edb8f58686..7ec4d62f27 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -8,6 +8,7 @@ import logging import multiprocessing import shutil +import traceback from contextlib import contextmanager from dataclasses import dataclass from datetime import datetime, timezone @@ -319,32 +320,32 @@ def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: The deleted Instance ID. """ try: - Path("~/github-runner-delete.log").write_text( - f"Deleting server {delete_config.instance_id.name}" - ) - # logger.info("Deleting server %s", delete_config.instance_id.name) - res = delete_config.conn.delete_server(name_or_id=delete_config.instance_id.name) - Path("~/github-runner-delete.log").write_text( - f"Deleted server {delete_config.instance_id.name} (true delete: {res})", - ) - # logger.info("Deleted server %s (true delete: %s)", delete_config.instance_id.name, res) - except ( - openstack.exceptions.SDKException, - openstack.exceptions.ResourceTimeout, - ): - logger.exception( - "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name - ) - return None + try: + logger.info("Deleting server %s", delete_config.instance_id.name) + res = delete_config.conn.delete_server(name_or_id=delete_config.instance_id.name) + logger.info( + "Deleted server %s (true delete: %s)", delete_config.instance_id.name, res + ) + except ( + openstack.exceptions.SDKException, + openstack.exceptions.ResourceTimeout, + ): + logger.exception( + "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name + ) + return None - OpenstackCloud._delete_keypair( - _DeleteKeypairConfig( - keys_dir=delete_config.keys_dir, - instance_id=delete_config.instance_id, - conn=delete_config.conn, + OpenstackCloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=delete_config.keys_dir, + instance_id=delete_config.instance_id, + conn=delete_config.conn, + ) ) - ) - return delete_config.instance_id if res else None + return delete_config.instance_id if res else None + except Exception: + tb = traceback.format_exc() + Path("~/exception.log").write_text(tb, encoding="utf-8") def delete_instances( self, instance_ids: Sequence[InstanceID], wait: bool = False, timeout: int = 60 * 10 From b23b08e33addad423043cae05aca3d0f4578ce20 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 06:15:09 +0000 Subject: [PATCH 22/50] fix: construct connection on subprocess call --- .../github_runner_manager/manager/models.py | 4 +- .../manager/runner_scaler.py | 1 - .../openstack_cloud/openstack_cloud.py | 80 ++++++++++++------- 3 files changed, 52 insertions(+), 33 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/models.py b/github-runner-manager/src/github_runner_manager/manager/models.py index 4e14f72226..eed5036f1f 100644 --- a/github-runner-manager/src/github_runner_manager/manager/models.py +++ b/github-runner-manager/src/github_runner_manager/manager/models.py @@ -29,13 +29,13 @@ class InstanceID: Attributes: name: Name of the instance to use. prefix: Prefix corresponding to the application (charm application unit). - suffix: Random suffix for the InstanceID. reactive: Identifies if the runner is reactive. + suffix: Random suffix for the InstanceID. """ prefix: str suffix: str - reactive: bool = False + reactive: bool | None = None @property def name(self) -> str: diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py b/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py index 7603890789..3a58873b71 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_scaler.py @@ -444,7 +444,6 @@ def _issue_reconciliation_metric( IDLE_RUNNERS_COUNT.labels(manager_name).set(len(idle_runners)) try: - metric_events.issue_event( metric_events.Reconciliation( timestamp=time.time(), diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 7ec4d62f27..6249b1748c 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -8,7 +8,6 @@ import logging import multiprocessing import shutil -import traceback from contextlib import contextmanager from dataclasses import dataclass from datetime import datetime, timezone @@ -166,14 +165,16 @@ class _DeleteVMConfig: Attributes: instance_id: The ID of the VM to request deletion. - conn: The OpenStack connection instance. + credentials: The OpenStack connection credentials. + max_api_version: The OpenStack maximum compute API version. keys_dir: The path to the directory in which the SSH key files are stored. wait: Whether to wait for the VM delete to complete. timeout: Timeout in seconds for VM deletion to complete. """ instance_id: InstanceID - conn: OpenstackConnection + credentials: OpenStackCredentials + max_api_version: str keys_dir: Path wait: bool = False timeout: int = 10 * 60 @@ -277,7 +278,13 @@ def launch_instance( instance_id, ) OpenstackCloud._delete_instance( - _DeleteVMConfig(instance_id=instance_id, conn=conn, keys_dir=self._ssh_key_dir) + _DeleteVMConfig( + instance_id=instance_id, + credentials=self._credentials, + max_api_version=self._max_compute_api_version, + keys_dir=self._ssh_key_dir, + ), + conn=conn, ) raise OpenStackError(f"Timeout creating openstack server {instance_id}") from err except openstack.exceptions.SDKException as err: @@ -310,42 +317,57 @@ def get_instance(self, instance_id: InstanceID) -> OpenstackInstance | None: return None @staticmethod - def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: + def _delete_instance( + delete_config: _DeleteVMConfig, conn: OpenstackConnection | None = None + ) -> InstanceID | None: """Delete a openstack instance. Args: delete_config: The configuration used to delete a cloud VM instance. + conn: The shared OpenStack connection instance if not running as as subprocess. Returns: The deleted Instance ID. """ - try: - try: - logger.info("Deleting server %s", delete_config.instance_id.name) - res = delete_config.conn.delete_server(name_or_id=delete_config.instance_id.name) - logger.info( - "Deleted server %s (true delete: %s)", delete_config.instance_id.name, res - ) - except ( - openstack.exceptions.SDKException, - openstack.exceptions.ResourceTimeout, - ): - logger.exception( - "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name - ) - return None + if conn is None: + conn = openstack.connect( + auth_url=delete_config.credentials.auth_url, + project_name=delete_config.credentials.project_name, + username=delete_config.credentials.username, + password=delete_config.credentials.password, + region_name=delete_config.credentials.region_name, + user_domain_name=delete_config.credentials.user_domain_name, + project_domain_name=delete_config.credentials.project_domain_name, + compute_api_version=delete_config.max_api_version, + ) + close_connection = True + else: + close_connection = False + try: + logger.info("Deleting server %s", delete_config.instance_id.name) + res = conn.delete_server(name_or_id=delete_config.instance_id.name) + logger.info("Deleted server %s (true delete: %s)", delete_config.instance_id.name, res) OpenstackCloud._delete_keypair( _DeleteKeypairConfig( keys_dir=delete_config.keys_dir, instance_id=delete_config.instance_id, - conn=delete_config.conn, + conn=conn, ) ) - return delete_config.instance_id if res else None - except Exception: - tb = traceback.format_exc() - Path("~/exception.log").write_text(tb, encoding="utf-8") + except ( + openstack.exceptions.SDKException, + openstack.exceptions.ResourceTimeout, + ): + logger.exception( + "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name + ) + return None + finally: + if close_connection: + conn.close() + + return delete_config.instance_id if res else None def delete_instances( self, instance_ids: Sequence[InstanceID], wait: bool = False, timeout: int = 60 * 10 @@ -367,14 +389,12 @@ def delete_instances( if not instance_ids: return deleted_instance_ids - with ( - self._get_openstack_connection() as conn, - multiprocessing.Pool(min(len(instance_ids), 30)) as pool, - ): + with multiprocessing.Pool(min(len(instance_ids), 30)) as pool: delete_configs = [ _DeleteVMConfig( instance_id=instance_id, - conn=conn, + credentials=self._credentials, + max_api_version=self._max_compute_api_version, keys_dir=self._ssh_key_dir, wait=wait, timeout=timeout, From ebd09d2bc1894ff2346a7bb578a4f130ca788d88 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 06:18:55 +0000 Subject: [PATCH 23/50] chore: minor lint fixes --- tests/integration/test_charm_runner.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/test_charm_runner.py b/tests/integration/test_charm_runner.py index aa10203467..38a0cd13ab 100644 --- a/tests/integration/test_charm_runner.py +++ b/tests/integration/test_charm_runner.py @@ -16,7 +16,6 @@ DISPATCH_TEST_WORKFLOW_FILENAME, DISPATCH_WAIT_TEST_WORKFLOW_FILENAME, dispatch_workflow, - get_file_content, get_job_logs, wait_for, wait_for_reconcile, From b3d7ec0bc6fb73c723851af48d54165f09154728 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 06:46:47 +0000 Subject: [PATCH 24/50] chore: remove comments --- .../tests/unit/factories/runner_instance_factory.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/github-runner-manager/tests/unit/factories/runner_instance_factory.py b/github-runner-manager/tests/unit/factories/runner_instance_factory.py index 203a3fed4d..0af9aae976 100644 --- a/github-runner-manager/tests/unit/factories/runner_instance_factory.py +++ b/github-runner-manager/tests/unit/factories/runner_instance_factory.py @@ -119,11 +119,11 @@ class Meta: model = PlatformRunnerHealth - identity = factory.SubFactory(RunnerIdentityFactory) # Create a related RunnerIdentity - online = factory.Faker("boolean") # Random boolean for online status - busy = factory.Faker("boolean") # Random boolean for busy status - deletable = factory.Faker("boolean") # Random boolean for deletable status - runner_in_platform = factory.Faker("boolean", chance_of_getting_true=90) # Mostly true + identity = factory.SubFactory(RunnerIdentityFactory) + online = factory.Faker("boolean") + busy = factory.Faker("boolean") + deletable = factory.Faker("boolean") + runner_in_platform = factory.Faker("boolean") class RunnerInstanceFactory(factory.Factory): From 4bb1a6f7765b91797555a0b92a226a4051c6773f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 07:58:09 +0000 Subject: [PATCH 25/50] chore: revert debug workflow --- .github/workflows/integration_test.yaml | 33 +++++++++++-------------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index 3fc151e025..649d47df23 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -20,29 +20,26 @@ jobs: juju-channel: 3.6/stable provider: lxd test-tox-env: integration-juju3.6 - # modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' - modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_path_change", "test_debug_ssh", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' + modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' + extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' + self-hosted-runner: true + self-hosted-runner-label: stg-private-endpoint + openstack-integration-tests-cross-controller-private-endpoint: + name: Cross controller integration test using private-endpoint + uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main + secrets: inherit + with: + juju-channel: 3.6/stable + pre-run-script: tests/integration/setup-integration-tests.sh + provider: lxd + test-tox-env: integration-juju3.6 + modules: '["test_prometheus_metrics"]' extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' self-hosted-runner: true self-hosted-runner-label: stg-private-endpoint - tmate-debug: true - tmate-timeout: 90 - # openstack-integration-tests-cross-controller-private-endpoint: - # name: Cross controller integration test using private-endpoint - # uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main - # secrets: inherit - # with: - # juju-channel: 3.6/stable - # pre-run-script: tests/integration/setup-integration-tests.sh - # provider: lxd - # test-tox-env: integration-juju3.6 - # modules: '["test_prometheus_metrics"]' - # extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' - # self-hosted-runner: true - # self-hosted-runner-label: stg-private-endpoint allure-report: if: ${{ (success() || failure()) && github.event_name == 'schedule' }} needs: - openstack-integration-tests-private-endpoint - # - openstack-integration-tests-cross-controller-private-endpoint + - openstack-integration-tests-cross-controller-private-endpoint uses: canonical/operator-workflows/.github/workflows/allure_report.yaml@main From 01f6f9023d3c7cda642cdd315a0c0c21c1b7c7fb Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 14 Jul 2025 07:58:22 +0000 Subject: [PATCH 26/50] chore: tidy up factory comments --- github-runner-manager/tests/unit/factories/metrics_factory.py | 2 +- .../tests/unit/factories/runner_instance_factory.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/github-runner-manager/tests/unit/factories/metrics_factory.py b/github-runner-manager/tests/unit/factories/metrics_factory.py index 883e920386..9f3e163d0e 100644 --- a/github-runner-manager/tests/unit/factories/metrics_factory.py +++ b/github-runner-manager/tests/unit/factories/metrics_factory.py @@ -53,7 +53,7 @@ class Meta: model = CodeInformation - code = factory.Faker("random_int", min=100, max=599) # Status code in the rang + code = factory.Faker("random_int", min=100, max=599) class RunnerStopFactory(EventFactory): diff --git a/github-runner-manager/tests/unit/factories/runner_instance_factory.py b/github-runner-manager/tests/unit/factories/runner_instance_factory.py index 0af9aae976..55d76a2a2b 100644 --- a/github-runner-manager/tests/unit/factories/runner_instance_factory.py +++ b/github-runner-manager/tests/unit/factories/runner_instance_factory.py @@ -186,7 +186,7 @@ class Meta: model = SelfHostedRunnerLabel - name = factory.Faker("word") # Generate a random word for the label name + name = factory.Faker("word") class SelfHostedRunnerFactory(factory.Factory): From ceb68eebc7d9d539cffba7a6d197ebe2a7c73688 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 03:30:28 +0000 Subject: [PATCH 27/50] chore: remove reusing connection --- .../openstack_cloud/openstack_cloud.py | 73 ++++++++----------- 1 file changed, 32 insertions(+), 41 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 6249b1748c..0ec0dabd1c 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -283,8 +283,7 @@ def launch_instance( credentials=self._credentials, max_api_version=self._max_compute_api_version, keys_dir=self._ssh_key_dir, - ), - conn=conn, + ) ) raise OpenStackError(f"Timeout creating openstack server {instance_id}") from err except openstack.exceptions.SDKException as err: @@ -317,55 +316,47 @@ def get_instance(self, instance_id: InstanceID) -> OpenstackInstance | None: return None @staticmethod - def _delete_instance( - delete_config: _DeleteVMConfig, conn: OpenstackConnection | None = None - ) -> InstanceID | None: + def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: """Delete a openstack instance. Args: delete_config: The configuration used to delete a cloud VM instance. - conn: The shared OpenStack connection instance if not running as as subprocess. Returns: The deleted Instance ID. """ - if conn is None: - conn = openstack.connect( - auth_url=delete_config.credentials.auth_url, - project_name=delete_config.credentials.project_name, - username=delete_config.credentials.username, - password=delete_config.credentials.password, - region_name=delete_config.credentials.region_name, - user_domain_name=delete_config.credentials.user_domain_name, - project_domain_name=delete_config.credentials.project_domain_name, - compute_api_version=delete_config.max_api_version, - ) - close_connection = True - else: - close_connection = False + with openstack.connect( + auth_url=delete_config.credentials.auth_url, + project_name=delete_config.credentials.project_name, + username=delete_config.credentials.username, + password=delete_config.credentials.password, + region_name=delete_config.credentials.region_name, + user_domain_name=delete_config.credentials.user_domain_name, + project_domain_name=delete_config.credentials.project_domain_name, + compute_api_version=delete_config.max_api_version, + ) as conn: - try: - logger.info("Deleting server %s", delete_config.instance_id.name) - res = conn.delete_server(name_or_id=delete_config.instance_id.name) - logger.info("Deleted server %s (true delete: %s)", delete_config.instance_id.name, res) - OpenstackCloud._delete_keypair( - _DeleteKeypairConfig( - keys_dir=delete_config.keys_dir, - instance_id=delete_config.instance_id, - conn=conn, + try: + logger.info("Deleting server %s", delete_config.instance_id.name) + res = conn.delete_server(name_or_id=delete_config.instance_id.name) + logger.info( + "Deleted server %s (true delete: %s)", delete_config.instance_id.name, res ) - ) - except ( - openstack.exceptions.SDKException, - openstack.exceptions.ResourceTimeout, - ): - logger.exception( - "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name - ) - return None - finally: - if close_connection: - conn.close() + OpenstackCloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=delete_config.keys_dir, + instance_id=delete_config.instance_id, + conn=conn, + ) + ) + except ( + openstack.exceptions.SDKException, + openstack.exceptions.ResourceTimeout, + ): + logger.exception( + "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name + ) + return None return delete_config.instance_id if res else None From 41d9ff2bbcc3a4a380125314e64a0c858ceaef3c Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 03:30:47 +0000 Subject: [PATCH 28/50] fix: pyproject merge conflict fix --- github-runner-manager/pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github-runner-manager/pyproject.toml b/github-runner-manager/pyproject.toml index d545cf5ba0..40e1fc6ded 100644 --- a/github-runner-manager/pyproject.toml +++ b/github-runner-manager/pyproject.toml @@ -85,9 +85,9 @@ per-file-ignores = [ # Ignore no return values (DCO031) in docstring for abstract methods "src/github_runner_manager/manager/cloud_runner_manager.py:DCO031", # D100, D101, D102, D103, D104: Ignore docstring style issues in tests - "tests/*:D100,D101,D102,D103,D104,D205,D212" + "tests/*:D100,D101,D102,D103,D104,D205,D212", # DCO020, DCO030, DCO050: Ignore docstring argument,returns,raises sections in tests - "tests/*:D100,D101,D102,D103,D104,D205,D212, DCO020, DCO030, DCO050" + "tests/*:D100,D101,D102,D103,D104,D205,D212, DCO020, DCO030, DCO050", ] docstring-convention = "google" # Check for properly formatted copyright header in each file From ed7d185179d4359605054e346b340af1acfec46a Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 03:47:08 +0000 Subject: [PATCH 29/50] fix: adapt changes to SelfHostedRunnerLabels --- .../unit/factories/runner_instance_factory.py | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/github-runner-manager/tests/unit/factories/runner_instance_factory.py b/github-runner-manager/tests/unit/factories/runner_instance_factory.py index 55d76a2a2b..423e7614ed 100644 --- a/github-runner-manager/tests/unit/factories/runner_instance_factory.py +++ b/github-runner-manager/tests/unit/factories/runner_instance_factory.py @@ -19,7 +19,7 @@ PlatformRunnerHealth, PlatformRunnerState, ) -from github_runner_manager.types_.github import SelfHostedRunner, SelfHostedRunnerLabel +from github_runner_manager.types_.github import SelfHostedRunner class InstanceIDFactory(factory.Factory): @@ -174,21 +174,6 @@ def from_state( ) -class SelfHostedRunnerLabelFactory(factory.Factory): - """Factory for creating SelfHostedRunnerLabel instances.""" - - class Meta: - """Meta class for RunnerInstance. - - Attributes: - model: The metadata reference model. - """ - - model = SelfHostedRunnerLabel - - name = factory.Faker("word") - - class SelfHostedRunnerFactory(factory.Factory): """Factory for creating SelfHostedRunner instances.""" @@ -203,9 +188,9 @@ class Meta: busy = factory.Faker("boolean") id = factory.Faker("random_int", min=1, max=10000) - labels = factory.List([factory.SubFactory(SelfHostedRunnerLabelFactory) for _ in range(3)]) + labels = factory.List([factory.Faker("word") for _ in range(3)]) status = factory.Faker("word", ext_word_list=["online", "offline"]) - deletable = factory.Faker("boolean", chance_of_getting_true=50) + deletable = factory.Faker("boolean") # identity.metadata.runner_id should be equal to the id attribute. identity = factory.LazyAttribute( lambda obj: RunnerIdentityFactory( From a37bb2dbc8ba99f19f9fb93eccc3af990e0e005b Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 03:47:30 +0000 Subject: [PATCH 30/50] fix: pyproject toml conflicting test ignores --- github-runner-manager/pyproject.toml | 2 -- 1 file changed, 2 deletions(-) diff --git a/github-runner-manager/pyproject.toml b/github-runner-manager/pyproject.toml index 40e1fc6ded..f19cf75c5a 100644 --- a/github-runner-manager/pyproject.toml +++ b/github-runner-manager/pyproject.toml @@ -84,8 +84,6 @@ per-file-ignores = [ "tests/unit/factories/*:DCO060", # Ignore no return values (DCO031) in docstring for abstract methods "src/github_runner_manager/manager/cloud_runner_manager.py:DCO031", - # D100, D101, D102, D103, D104: Ignore docstring style issues in tests - "tests/*:D100,D101,D102,D103,D104,D205,D212", # DCO020, DCO030, DCO050: Ignore docstring argument,returns,raises sections in tests "tests/*:D100,D101,D102,D103,D104,D205,D212, DCO020, DCO030, DCO050", ] From 0cf5a988ec35fafa0e5d5e227b09bfd4961f0cff Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 04:02:29 +0000 Subject: [PATCH 31/50] chore: remove todo comments --- .../github_runner_manager/manager/cloud_runner_manager.py | 4 ---- .../src/github_runner_manager/manager/models.py | 3 --- .../src/github_runner_manager/metrics/runner.py | 2 -- .../src/github_runner_manager/platform/github_provider.py | 2 +- .../github_runner_manager/platform/jobmanager_provider.py | 7 ++----- 5 files changed, 3 insertions(+), 15 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py index 7aa30170bf..06532e2c27 100644 --- a/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/cloud_runner_manager.py @@ -275,10 +275,6 @@ def delete_vms(self, instance_ids: Sequence[InstanceID]) -> list[InstanceID]: def extract_metrics(self, instance_ids: Sequence[InstanceID]) -> list[RunnerMetrics]: """Extract metrics from cloud VMs. - 2025/07/03 TODO: This method should really live in another metrics extractor class (that - doesn't exist yet). Hence, this method is subject to refactor when the caller classes are - tidied up. - Args: instance_ids: The VM instance IDs to fetch the metrics from. diff --git a/github-runner-manager/src/github_runner_manager/manager/models.py b/github-runner-manager/src/github_runner_manager/manager/models.py index eed5036f1f..fe4b591aee 100644 --- a/github-runner-manager/src/github_runner_manager/manager/models.py +++ b/github-runner-manager/src/github_runner_manager/manager/models.py @@ -13,8 +13,6 @@ class InstanceIDInvalidError(Exception): """Raised when the InstanceID naming will break the provider of GitHub.""" -# 20250702 TODO: The InstanceID should be renamed InstanceName and additionally -# have the platform information. @dataclass(eq=True, frozen=True, order=True) class InstanceID: """Main identifier for a runner instance among all clouds and GitHub. @@ -170,7 +168,6 @@ class RunnerMetadata: url: URL for the runner. """ - # 20250702 TODO: Supported platforms should be enumerated. platform_name: str = "github" runner_id: str | None = None url: str | None = None diff --git a/github-runner-manager/src/github_runner_manager/metrics/runner.py b/github-runner-manager/src/github_runner_manager/metrics/runner.py index 37f360739a..70bd9231c7 100644 --- a/github-runner-manager/src/github_runner_manager/metrics/runner.py +++ b/github-runner-manager/src/github_runner_manager/metrics/runner.py @@ -54,8 +54,6 @@ class _PullRunnerMetricsConfig: instance_id: InstanceID -# 2025/07/03 TODO: This should really be a service class with OpenStack service injected. The -# interface will accept the openstack_service and instance_id to reduce the scope of refactoring. def pull_runner_metrics( cloud_service: OpenstackCloud, instance_ids: Sequence[InstanceID] ) -> "list[PulledMetrics]": diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index 37efd8f770..248edaf962 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -171,7 +171,7 @@ def delete_runners(self, runner_ids: list[str], platform: str = "github") -> lis Args: runner_ids: The GitHub runner IDs to delete. - platform: TODO: Unused argument due to the poor architecture of the provider + platform: Unused argument due to the poor architecture of the provider classes. The multiplexer provider should be a wrapper around the platforms, not on the same level. diff --git a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py index a1ebc28535..1c8302aedc 100644 --- a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py @@ -28,10 +28,7 @@ PlatformRunnerHealth, RunnersHealthResponse, ) -from github_runner_manager.types_.github import ( - GitHubRunnerStatus, - SelfHostedRunner, -) +from github_runner_manager.types_.github import GitHubRunnerStatus, SelfHostedRunner logger = logging.getLogger(__name__) @@ -143,7 +140,7 @@ def delete_runners(self, runner_ids: list[str], platform: str = "jobmanager") -> Args: runner_ids: The runner IDs to delete. - platform: TODO: Unused argument due to the poor architecture of the provider + platform: Unused argument due to the poor architecture of the provider classes. The multiplexer provider should be a wrapper around the platforms, not on the same level. From f5848e83cf1b0e27970a91877bee99167b68aba9 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 11:15:19 +0000 Subject: [PATCH 32/50] feat: use thread pool for concurrency --- github-runner-manager/pyproject.toml | 5 +- .../openstack_cloud/openstack_cloud.py | 103 ++++--- .../tests/unit/mock_runner_managers.py | 268 ++++-------------- .../openstack_cloud/test_openstack_cloud.py | 17 +- 4 files changed, 138 insertions(+), 255 deletions(-) diff --git a/github-runner-manager/pyproject.toml b/github-runner-manager/pyproject.toml index f19cf75c5a..ca4a0fea8f 100644 --- a/github-runner-manager/pyproject.toml +++ b/github-runner-manager/pyproject.toml @@ -84,8 +84,9 @@ per-file-ignores = [ "tests/unit/factories/*:DCO060", # Ignore no return values (DCO031) in docstring for abstract methods "src/github_runner_manager/manager/cloud_runner_manager.py:DCO031", - # DCO020, DCO030, DCO050: Ignore docstring argument,returns,raises sections in tests - "tests/*:D100,D101,D102,D103,D104,D205,D212, DCO020, DCO030, DCO050", + # DCO020, DCO030, DCO050, DCO060: Ignore docstring argument, returns, raises, attribute + # sections in tests + "tests/*:D100,D101,D102,D103,D104,D205,D212,DCO020,DCO030,DCO050,DCO060", ] docstring-convention = "google" # Check for properly formatted copyright header in each file diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 0ec0dabd1c..b292d326db 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -2,11 +2,11 @@ # See LICENSE file for licensing details. """Class for accessing OpenStack API for managing servers.""" +import concurrent.futures import contextlib import copy import functools import logging -import multiprocessing import shutil from contextlib import contextmanager from dataclasses import dataclass @@ -76,6 +76,29 @@ _MIN_KEYPAIR_AGE_IN_SECONDS_BEFORE_DELETION = 60 +class OpenStackVMDeleteError(openstack.exceptions.SDKException): + """Represents an error while deleting a VM instance. + + Attributes: + instance_id: The instance ID that was failed to delete. + """ + + instance_id: InstanceID + + def __init__( + self, instance_id: InstanceID, message: str | None = None, extra_data: Any = None + ): + """Initialize the OpenstackVMDeleteError. + + Args: + instance_id: The instance ID of the failed delete VM. + message: The delete error message for parent SDKException. + extra_data: Extra data for parent SDKException if any. + """ + self.instance_id = instance_id + super().__init__(message, extra_data) + + @dataclass class OpenstackInstance: """Represents an OpenStack instance. @@ -316,14 +339,14 @@ def get_instance(self, instance_id: InstanceID) -> OpenstackInstance | None: return None @staticmethod - def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: + def _delete_instance(delete_config: _DeleteVMConfig) -> bool: """Delete a openstack instance. Args: delete_config: The configuration used to delete a cloud VM instance. - Returns: - The deleted Instance ID. + Raises: + OpenStackVMDeleteError: If there was an error deleting the VM instance. """ with openstack.connect( auth_url=delete_config.credentials.auth_url, @@ -335,30 +358,30 @@ def _delete_instance(delete_config: _DeleteVMConfig) -> InstanceID | None: project_domain_name=delete_config.credentials.project_domain_name, compute_api_version=delete_config.max_api_version, ) as conn: - try: logger.info("Deleting server %s", delete_config.instance_id.name) - res = conn.delete_server(name_or_id=delete_config.instance_id.name) + deleted = conn.delete_server(name_or_id=delete_config.instance_id.name) logger.info( - "Deleted server %s (true delete: %s)", delete_config.instance_id.name, res - ) - OpenstackCloud._delete_keypair( - _DeleteKeypairConfig( - keys_dir=delete_config.keys_dir, - instance_id=delete_config.instance_id, - conn=conn, - ) + "Deleted server %s (true delete: %s)", delete_config.instance_id.name, deleted ) except ( openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout, - ): - logger.exception( - "Failed to delete OpenStack VM instance: %s", delete_config.instance_id.name + ) as e: + raise OpenStackVMDeleteError( + instance_id=delete_config.instance_id, + message=f"Failed to delete server {delete_config.instance_id.name}", + ) from e + + OpenstackCloud._delete_keypair( + _DeleteKeypairConfig( + keys_dir=delete_config.keys_dir, + instance_id=delete_config.instance_id, + conn=conn, ) - return None + ) - return delete_config.instance_id if res else None + return deleted def delete_instances( self, instance_ids: Sequence[InstanceID], wait: bool = False, timeout: int = 60 * 10 @@ -380,25 +403,29 @@ def delete_instances( if not instance_ids: return deleted_instance_ids - with multiprocessing.Pool(min(len(instance_ids), 30)) as pool: - delete_configs = [ - _DeleteVMConfig( - instance_id=instance_id, - credentials=self._credentials, - max_api_version=self._max_compute_api_version, - keys_dir=self._ssh_key_dir, - wait=wait, - timeout=timeout, - ) - for instance_id in instance_ids - ] - logger.info("Deleting instances: %s", delete_configs) - for deleted_instance_id in pool.imap_unordered( - OpenstackCloud._delete_instance, delete_configs - ): - if not deleted_instance_id: - continue - deleted_instance_ids.append(deleted_instance_id) + delete_configs = [ + _DeleteVMConfig( + instance_id=instance_id, + credentials=self._credentials, + max_api_version=self._max_compute_api_version, + keys_dir=self._ssh_key_dir, + wait=wait, + timeout=timeout, + ) + for instance_id in instance_ids + ] + with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor: + submitted_future_config_map = { + executor.submit(OpenstackCloud._delete_instance, config): config + for config in delete_configs + } + for future in concurrent.futures.as_completed(submitted_future_config_map): + delete_config = submitted_future_config_map[future] + try: + if future.result(): + deleted_instance_ids.append(delete_config.instance_id) + except OpenStackVMDeleteError as e: + logger.error("Failed to delete OpenStack VM instance: %s", e.instance_id) return deleted_instance_ids diff --git a/github-runner-manager/tests/unit/mock_runner_managers.py b/github-runner-manager/tests/unit/mock_runner_managers.py index a8f156fe43..1b159f3c0c 100644 --- a/github-runner-manager/tests/unit/mock_runner_managers.py +++ b/github-runner-manager/tests/unit/mock_runner_managers.py @@ -1,18 +1,14 @@ # Copyright 2025 Canonical Ltd. # See LICENSE file for licensing details. -import hashlib import logging -import secrets -from dataclasses import dataclass -from datetime import datetime, timezone from typing import Sequence +from unittest.mock import MagicMock from pydantic import HttpUrl from github_runner_manager.manager.cloud_runner_manager import ( CloudRunnerInstance, CloudRunnerManager, - CloudRunnerState, ) from github_runner_manager.manager.models import ( InstanceID, @@ -22,19 +18,14 @@ ) from github_runner_manager.manager.runner_manager import RunnerInstance from github_runner_manager.metrics.runner import RunnerMetrics -from github_runner_manager.platform.github_provider import PlatformRunnerState +from github_runner_manager.openstack_cloud.openstack_cloud import _MAX_NOVA_COMPUTE_API_VERSION from github_runner_manager.platform.platform_provider import ( JobInfo, PlatformProvider, PlatformRunnerHealth, RunnersHealthResponse, ) -from github_runner_manager.types_.github import ( - GitHubRunnerStatus, - JITConfig, - RunnerApplication, - SelfHostedRunner, -) +from github_runner_manager.types_.github import GitHubRunnerStatus, SelfHostedRunner from tests.unit.factories.runner_instance_factory import CloudRunnerInstanceFactory logger = logging.getLogger(__name__) @@ -51,214 +42,73 @@ ) -class MockGhapiClient: - """Mock for Ghapi client.""" - - def __init__(self, token: str): - """Initialization method for GhapiClient fake. - - Args: - token: The client token value. - """ - self.token = token - self.actions = MockGhapiActions() - - def last_page(self) -> int: - """Last page number stub. - - Returns: - Always zero. - """ - return 0 - - -class MockGhapiActions: - """Mock for actions in Ghapi client.""" - - def __init__(self): - """A placeholder method for test stub/fakes initialization.""" - hash = hashlib.sha256() - hash.update(TEST_BINARY) - self.test_hash = hash.hexdigest() - self.registration_token_repo = secrets.token_hex() - self.registration_token_org = secrets.token_hex() - - def _list_runner_applications(self): - """A placeholder method for test fake. - - Returns: - A fake runner applications list. - """ - runners = [] - runners.append( - RunnerApplication( - os="linux", - architecture="x64", - download_url="https://www.example.com", - filename="test_runner_binary", - sha256_checksum=self.test_hash, - ) - ) - return runners - - def list_runner_applications_for_repo(self, owner: str, repo: str): - """A placeholder method for test stub. - - Args: - owner: Placeholder for repository owner. - repo: Placeholder for repository name. - - Returns: - A fake runner applications list. - """ - return self._list_runner_applications() - - def list_runner_applications_for_org(self, org: str): - """A placeholder method for test stub. - - Args: - org: Placeholder for repository owner. - - Returns: - A fake runner applications list. - """ - return self._list_runner_applications() - - def create_registration_token_for_repo(self, owner: str, repo: str): - """A placeholder method for test stub. - - Args: - owner: Placeholder for repository owner. - repo: Placeholder for repository name. - - Returns: - Registration token stub. - """ - return JITConfig( - {"token": self.registration_token_repo, "expires_at": "2020-01-22T12:13:35.123-08:00"} - ) - - def list_self_hosted_runners_for_repo( - self, owner: str, repo: str, per_page: int, page: int = 0 - ): - """A placeholder method for test stub. - - Args: - owner: Placeholder for repository owner. - repo: Placeholder for repository name. - per_page: Placeholder for responses per page. - page: Placeholder for response page number. +class MockOpenstackCloud: + """Mock of OpenstackCloud.""" - Returns: - Empty runners stub. - """ - return {"runners": []} + _MOCK_COMPUTE_ENDPOINT = "mock-compute-endpoint" + _MOCK_COMPUTE_ENDPOINT_RESPONSE = {"version": {"version": _MAX_NOVA_COMPUTE_API_VERSION}} - def list_self_hosted_runners_for_org(self, org: str, per_page: int, page: int = 0): - """A placeholder method for test stub. + def __init__( + self, + initial_servers: list[InstanceID], + server_to_errors: dict[InstanceID, Exception] | None = None, + ) -> None: + """Initialize the OpenstackCloud mock object.""" + self.servers = {instance.name: instance for instance in initial_servers} + self._injected_errors = { + instance.name: exc for instance, exc in (server_to_errors or {}).items() + } - Args: - org: Placeholder for repository owner. - per_page: Placeholder for responses per page. - page: Placeholder for response page number. + def __enter__(self) -> "MockOpenstackCloud": + """Mock enter method for context entering.""" + return self - Returns: - Empty runners stub. - """ - return {"runners": []} + def __exit__(self, *args, **kwargs) -> None: + """Mock exit method for context exiting.""" + return - def delete_self_hosted_runner_from_repo(self, owner: str, repo: str, runner_id: str): - """A placeholder method for test stub. + def connect(self) -> "MockOpenstackCloud": + """Mock OpenStack lib's connect function.""" + return self - Args: - owner: Placeholder for repository owner. - repo: Placeholder for repository name. - runner_id: Placeholder for runenr_id. - """ - pass + @property + def compute(self) -> "MockOpenstackCloud": + """Mock the compute API attribute.""" + return self - def delete_self_hosted_runner_from_org(self, org: str, runner_id: str): - """A placeholder method for test stub. + def get_endpoint(self) -> str: + """Mock endpoint string for compute endpoint.""" + return self._MOCK_COMPUTE_ENDPOINT - Args: - org: Placeholder for organisation. - runner_id: Placeholder for runner id. - """ + @property + def session(self) -> dict: + """Mock the connection session attribute.""" + compute_endpoint_mock = MagicMock() + compute_endpoint_mock.json.return_value = self._MOCK_COMPUTE_ENDPOINT_RESPONSE + return {self._MOCK_COMPUTE_ENDPOINT: compute_endpoint_mock} + + def delete_server( + self, + name_or_id: str, + wait: bool = False, + timeout: int = 180, + delete_ips: bool = False, + delete_ip_retry: int = 1, + ) -> bool: + """Mock method for deleting server.""" + injected_test_error = self._injected_errors.pop(name_or_id, None) + if injected_test_error: + raise injected_test_error + + if self.servers.pop(name_or_id, None): + return True + return False + + def delete_keypair(self, *args, **kwargs): + """Mock delete keypair method.""" pass -@dataclass -class MockRunner: - """Mock of a runner. - - Attributes: - name: The name of the runner. - instance_id: The instance id of the runner. - metadata: Metadata of the server. - cloud_state: The cloud state of the runner. - platform_state: The github state of the runner. - health: The health state of the runner. - created_at: The cloud creation time of the runner. - deletable: If the runner is deletable. - """ - - name: str - instance_id: InstanceID - metadata: RunnerMetadata - cloud_state: CloudRunnerState - platform_state: PlatformRunnerState - health: bool - created_at: datetime - deletable: bool = False - - def __init__(self, instance_id: InstanceID): - """Construct the object. - - Args: - instance_id: InstanceID of the runner. - """ - self.name = instance_id.name - self.instance_id = instance_id - self.metadata = RunnerMetadata() - self.cloud_state = CloudRunnerState.ACTIVE - self.platform_state = PlatformRunnerState.IDLE - self.health = True - # By default a runner that has just being created. - self.created_at = datetime.now(timezone.utc) - - def to_cloud_runner(self) -> CloudRunnerInstance: - """Construct CloudRunnerInstance from this object. - - Returns: - The CloudRunnerInstance instance. - """ - return CloudRunnerInstance( - name=self.name, - metadata=self.metadata, - instance_id=self.instance_id, - health=self.health, - state=self.cloud_state, - created_at=self.created_at, - ) - - -@dataclass -class SharedMockRunnerManagerState: - """State shared by mock runner managers. - - For sharing the mock runner states between MockCloudRunnerManager and MockGitHubRunnerPlatform. - - Attributes: - runners: The runners. - """ - - runners: dict[InstanceID, MockRunner] - - def __init__(self): - """Construct the object.""" - self.runners = {} - - class MockCloudRunnerManager(CloudRunnerManager): """Mock of CloudRunnerManager. diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index 56b5e483ff..ae335f1e0e 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -32,6 +32,7 @@ _DeleteKeypairConfig, get_missing_security_rules, ) +from tests.unit.mock_runner_managers import MockOpenstackCloud FAKE_ARG = "fake" FAKE_PREFIX = "fake_prefix" @@ -370,21 +371,26 @@ def test__delete_keypair_error( assert f"Error attempting to delete key: {test_key_instance_id.name}" in caplog.messages -@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") def test_delete_instances_partial_server_delete_failure( - openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, caplog: LogCaptureFixture + monkeypatch: pytest.MonkeyPatch, openstack_cloud: OpenstackCloud, caplog: LogCaptureFixture ): """ arrange: given a mocked openstack connection that errors on few failed requests. act: when delete_instances method is called. assert: successfully deleted instance IDs are returned and failed instances are logged. """ - mock_openstack_conn.delete_server = MagicMock( - side_effect=[True, False, openstack.exceptions.ResourceTimeout()] - ) successful_delete_id = InstanceID(prefix="success", reactive=False, suffix="") already_deleted_id = InstanceID(prefix="already_deleted", reactive=False, suffix="") timeout_id = InstanceID(prefix="timeout error", reactive=False, suffix="") + mock_cloud = MockOpenstackCloud( + initial_servers=[successful_delete_id, timeout_id], + server_to_errors={timeout_id: openstack.exceptions.ResourceTimeout()}, + ) + monkeypatch.setattr( + github_runner_manager.openstack_cloud.openstack_cloud.openstack, + "connect", + MagicMock(return_value=mock_cloud), + ) deleted_instance_ids = openstack_cloud.delete_instances( instance_ids=[successful_delete_id, already_deleted_id, timeout_id] @@ -396,7 +402,6 @@ def test_delete_instances_partial_server_delete_failure( assert f"Failed to delete OpenStack VM instance: {timeout_id}" in caplog.messages -@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") def test_delete_instances( openstack_cloud: OpenstackCloud, mock_openstack_conn: MagicMock, From 9bad113d5f550167b178101f23a1712a43a11e89 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 11:17:45 +0000 Subject: [PATCH 33/50] feat: limit max workers --- .../github_runner_manager/openstack_cloud/openstack_cloud.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index b292d326db..85e42cbadf 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -414,7 +414,9 @@ def delete_instances( ) for instance_id in instance_ids ] - with concurrent.futures.ThreadPoolExecutor(max_workers=3) as executor: + with concurrent.futures.ThreadPoolExecutor( + max_workers=min(len(instance_ids), 30) + ) as executor: submitted_future_config_map = { executor.submit(OpenstackCloud._delete_instance, config): config for config in delete_configs From c528e839a82e74f9a9ab61b9ec82311340723322 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 11:23:48 +0000 Subject: [PATCH 34/50] feat: concurrent github runner delete request using threadpool --- .../openstack_cloud/openstack_cloud.py | 6 +-- .../platform/github_provider.py | 45 ++++++++++--------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 85e42cbadf..04b5097985 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -417,12 +417,12 @@ def delete_instances( with concurrent.futures.ThreadPoolExecutor( max_workers=min(len(instance_ids), 30) ) as executor: - submitted_future_config_map = { + future_to_delete_instance_config = { executor.submit(OpenstackCloud._delete_instance, config): config for config in delete_configs } - for future in concurrent.futures.as_completed(submitted_future_config_map): - delete_config = submitted_future_config_map[future] + for future in concurrent.futures.as_completed(future_to_delete_instance_config): + delete_config = future_to_delete_instance_config[future] try: if future.result(): deleted_instance_ids.append(delete_config.instance_id) diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index 248edaf962..828677f531 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -3,8 +3,8 @@ """Client for managing self-hosted runner on GitHub side.""" +import concurrent.futures import logging -import multiprocessing from dataclasses import dataclass from enum import Enum @@ -188,37 +188,38 @@ def delete_runners(self, runner_ids: list[str], platform: str = "github") -> lis for runner_id in runner_ids ] deleted_runner_ids: list[str] = [] - with multiprocessing.Pool(min(len(runner_ids), 30)) as pool: - for deleted_runner_id in pool.imap_unordered( - GitHubRunnerPlatform._delete_runner, delete_configs - ): - if not deleted_runner_id: - continue - deleted_runner_ids.append(deleted_runner_id) + with concurrent.futures.ThreadPoolExecutor( + max_workers=min(len(runner_ids), 30) + ) as executor: + future_to_delete_runner_config = { + executor.submit(GitHubRunnerPlatform._delete_runner, config): config + for config in delete_configs + } + for future in concurrent.futures.as_completed(future_to_delete_runner_config): + delete_config = future_to_delete_runner_config[future] + try: + if future.result(): + deleted_runner_ids.append(delete_config.runner_id) + except DeleteRunnerBusyError: + logger.warning( + "Delete runner attempt failed, busy runner: %s", + delete_config.runner_id, + ) + return deleted_runner_ids @staticmethod - def _delete_runner(delete_runner_config: _DeleteRunnerConfig) -> str | None: + def _delete_runner(delete_runner_config: _DeleteRunnerConfig) -> None: """Delete a single runner from GitHub. This method is a wrapper to be called via multiprocessing pool for parallel deletion. Args: delete_runner_config: The configuration to use for deleting the runner. - - Returns: - The runner ID of the deleted runner """ - try: - delete_runner_config.github_client.delete_runner( - path=delete_runner_config.path, runner_id=int(delete_runner_config.runner_id) - ) - except DeleteRunnerBusyError: - logger.warning( - "Delete runner attempt failed, busy runner: %s", delete_runner_config.runner_id - ) - return None - return delete_runner_config.runner_id + delete_runner_config.github_client.delete_runner( + path=delete_runner_config.path, runner_id=int(delete_runner_config.runner_id) + ) def get_runner_context( self, metadata: RunnerMetadata, instance_id: InstanceID, labels: list[str] From 2b5589317389a3afe526dfcb8fc808448e00d9a1 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 16 Jul 2025 11:36:18 +0000 Subject: [PATCH 35/50] feat: concurrent metrics fetching using multithreading --- .../github_runner_manager/metrics/runner.py | 40 ++++++++++-------- github-runner-manager/tests/unit/conftest.py | 25 ----------- .../tests/unit/metrics/test_runner.py | 41 ++++++++++--------- 3 files changed, 46 insertions(+), 60 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/metrics/runner.py b/github-runner-manager/src/github_runner_manager/metrics/runner.py index 70bd9231c7..93cb0b9d2e 100644 --- a/github-runner-manager/src/github_runner_manager/metrics/runner.py +++ b/github-runner-manager/src/github_runner_manager/metrics/runner.py @@ -3,10 +3,10 @@ """Classes and function to extract the metrics from storage and issue runner metrics events.""" +import concurrent.futures import io import json import logging -import multiprocessing from dataclasses import dataclass from json import JSONDecodeError from typing import Optional, Sequence, Type @@ -75,11 +75,19 @@ def pull_runner_metrics( for instance_id in instance_ids ] pulled_metrics: list[PulledMetrics] = [] - with multiprocessing.Pool(min(len(instance_ids), 10)) as pool: - for metrics in pool.imap_unordered(_pull_runner_metrics, pull_metrics_configs): - if not metrics: - continue - pulled_metrics.append(metrics) + with concurrent.futures.ThreadPoolExecutor(max_workers=min(len(instance_ids), 30)) as executor: + future_to_pull_metrics_config = { + executor.submit(_pull_runner_metrics, config): config + for config in pull_metrics_configs + } + for future in concurrent.futures.as_completed(future_to_pull_metrics_config): + pull_config = future_to_pull_metrics_config[future] + metric = future.result() + if not metric: + logger.warning("No metrics pulled for %s", pull_config.instance_id) + else: + pulled_metrics.append(metric) + return pulled_metrics @@ -99,11 +107,10 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric ) return None - pulled_metrics = PulledMetrics(instance=instance) try: with pull_config.cloud_service.get_ssh_connection(instance=instance) as ssh_conn: try: - pulled_metrics.runner_installed = _ssh_pull_file( + runner_installed = _ssh_pull_file( ssh_conn=ssh_conn, remote_path=str(RUNNER_INSTALLED_TS_FILE_NAME), max_size=MAX_METRICS_FILE_SIZE, @@ -115,7 +122,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric exc, ) try: - pulled_metrics.pre_job_metrics = _ssh_pull_file( + pre_job_metrics = _ssh_pull_file( ssh_conn=ssh_conn, remote_path=str(PRE_JOB_METRICS_FILE_NAME), max_size=MAX_METRICS_FILE_SIZE, @@ -127,7 +134,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric exc, ) try: - pulled_metrics.post_job_metrics = _ssh_pull_file( + post_job_metrics = _ssh_pull_file( ssh_conn=ssh_conn, remote_path=str(POST_JOB_METRICS_FILE_NAME), max_size=MAX_METRICS_FILE_SIZE, @@ -145,12 +152,13 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric return None return ( - pulled_metrics - if ( - pulled_metrics.runner_installed - or pulled_metrics.pre_job_metrics - or pulled_metrics.post_job_metrics + PulledMetrics( + instance=instance, + runner_installed=runner_installed, + pre_job_metrics=pre_job_metrics, + post_job_metrics=post_job_metrics, ) + if (runner_installed or pre_job_metrics or post_job_metrics) else None ) @@ -216,7 +224,7 @@ def _ssh_pull_file(ssh_conn: SSHConnection, remote_path: str, max_size: int) -> return value -@dataclass +@dataclass(frozen=True) class PulledMetrics: """Metrics pulled from a runner. diff --git a/github-runner-manager/tests/unit/conftest.py b/github-runner-manager/tests/unit/conftest.py index e08d29515a..d276b286ad 100644 --- a/github-runner-manager/tests/unit/conftest.py +++ b/github-runner-manager/tests/unit/conftest.py @@ -6,7 +6,6 @@ import getpass import grp import os -from unittest.mock import MagicMock import pytest @@ -16,27 +15,3 @@ @pytest.fixture(name="user_info", scope="module") def user_info_fixture(): return UserInfo(getpass.getuser(), grp.getgrgid(os.getgid()).gr_name) - - -@pytest.fixture(name="patch_multiprocess_pool_imap_unordered", scope="function") -def patch_multiprocess_pool_imap_unordered_fixture(monkeypatch: pytest.MonkeyPatch): - """Patch multiprocessing pool call to call the function directly.""" - - def call_direct(func_var, params): - """Function to replace imap_unordered with, by calling functions directly. - - Args: - func_var: The function to call in imap_unordered call. - params: The iterable parameters for target function. - - Yields: - The function return value. - """ - for param in params: - yield func_var(param) - - pool_mock = MagicMock() - pool_mock.return_value = pool_mock - pool_mock.__enter__ = pool_mock - pool_mock.imap_unordered = call_direct - monkeypatch.setattr("multiprocessing.pool.Pool", pool_mock) diff --git a/github-runner-manager/tests/unit/metrics/test_runner.py b/github-runner-manager/tests/unit/metrics/test_runner.py index 97348088da..f26ec3974a 100644 --- a/github-runner-manager/tests/unit/metrics/test_runner.py +++ b/github-runner-manager/tests/unit/metrics/test_runner.py @@ -47,7 +47,6 @@ def runner_fs_base_fixture(tmp_path: Path) -> Path: return runner_fs_base -@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") def test_pull_runner_metrics_errors(caplog: pytest.LogCaptureFixture): """ arrange: given a mocked cloud service that raises exceptions are different points. @@ -109,7 +108,6 @@ def test_pull_runner_metrics_errors(caplog: pytest.LogCaptureFixture): ) -@pytest.mark.usefixtures("patch_multiprocess_pool_imap_unordered") def test_pull_runner_metrics(): """ arrange: given a mock cloud service get_instance method and get_ssh_connection method. @@ -128,23 +126,28 @@ def test_pull_runner_metrics(): mock_instance_one, mock_instance_two = (MagicMock(), MagicMock()) mock_cloud_service.get_instance.side_effect = [mock_instance_one, mock_instance_two] - assert pull_runner_metrics( - cloud_service=mock_cloud_service, - instance_ids=[mock_instance_one.instance_id, mock_instance_two.instance_id], - ) == [ - PulledMetrics( - instance=mock_instance_one, - runner_installed=test_remote_file_contents, - pre_job_metrics=test_remote_file_contents, - post_job_metrics=test_remote_file_contents, - ), - PulledMetrics( - instance=mock_instance_two, - runner_installed=test_remote_file_contents, - pre_job_metrics=test_remote_file_contents, - post_job_metrics=test_remote_file_contents, - ), - ] + # Compare the set as the order is not guaranteed but it does not matter. + assert set( + pull_runner_metrics( + cloud_service=mock_cloud_service, + instance_ids=[mock_instance_one.instance_id, mock_instance_two.instance_id], + ) + ) == set( + [ + PulledMetrics( + instance=mock_instance_one, + runner_installed=test_remote_file_contents, + pre_job_metrics=test_remote_file_contents, + post_job_metrics=test_remote_file_contents, + ), + PulledMetrics( + instance=mock_instance_two, + runner_installed=test_remote_file_contents, + pre_job_metrics=test_remote_file_contents, + post_job_metrics=test_remote_file_contents, + ), + ] + ) def test_issue_events(issue_event_mock: MagicMock): From e76f9a9448df102569eb922a1d5d56df8be2ac53 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 17 Jul 2025 02:43:20 +0000 Subject: [PATCH 36/50] chore: rename DeleteVMError --- .../openstack_cloud/openstack_cloud.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 04b5097985..99690b40c4 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -76,7 +76,7 @@ _MIN_KEYPAIR_AGE_IN_SECONDS_BEFORE_DELETION = 60 -class OpenStackVMDeleteError(openstack.exceptions.SDKException): +class DeleteVMError(openstack.exceptions.SDKException): """Represents an error while deleting a VM instance. Attributes: @@ -367,11 +367,11 @@ def _delete_instance(delete_config: _DeleteVMConfig) -> bool: except ( openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout, - ) as e: - raise OpenStackVMDeleteError( + ) as exc: + raise DeleteVMError( instance_id=delete_config.instance_id, message=f"Failed to delete server {delete_config.instance_id.name}", - ) from e + ) from exc OpenstackCloud._delete_keypair( _DeleteKeypairConfig( @@ -426,8 +426,8 @@ def delete_instances( try: if future.result(): deleted_instance_ids.append(delete_config.instance_id) - except OpenStackVMDeleteError as e: - logger.error("Failed to delete OpenStack VM instance: %s", e.instance_id) + except DeleteVMError as exc: + logger.error("Failed to delete OpenStack VM instance: %s", exc.instance_id) return deleted_instance_ids From d219e0e679352e7c57ac84b4fd2c6076d831d54f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 17 Jul 2025 02:43:41 +0000 Subject: [PATCH 37/50] chore: update unused argument comment --- .../src/github_runner_manager/platform/github_provider.py | 4 +--- .../src/github_runner_manager/platform/jobmanager_provider.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index 828677f531..2cca793ed4 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -171,9 +171,7 @@ def delete_runners(self, runner_ids: list[str], platform: str = "github") -> lis Args: runner_ids: The GitHub runner IDs to delete. - platform: Unused argument due to the poor architecture of the provider - classes. The multiplexer provider should be a wrapper around the platforms, not on - the same level. + platform: Unused argument. Returns: The runner IDs that were deleted successfully. diff --git a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py index 1c8302aedc..3a4b0a162e 100644 --- a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py @@ -140,9 +140,7 @@ def delete_runners(self, runner_ids: list[str], platform: str = "jobmanager") -> Args: runner_ids: The runner IDs to delete. - platform: Unused argument due to the poor architecture of the provider - classes. The multiplexer provider should be a wrapper around the platforms, not on - the same level. + platform: Unused argument. Returns: The runner IDs requested for deletion. From 5692a66b042293b77166ab260347023896171a74 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 17 Jul 2025 03:18:19 +0000 Subject: [PATCH 38/50] chore: rename mock to fake --- ...er_managers.py => fake_runner_managers.py} | 57 +++++++------------ .../tests/unit/manager/test_runner_manager.py | 22 +++---- .../openstack_cloud/test_openstack_cloud.py | 4 +- 3 files changed, 35 insertions(+), 48 deletions(-) rename github-runner-manager/tests/unit/{mock_runner_managers.py => fake_runner_managers.py} (87%) diff --git a/github-runner-manager/tests/unit/mock_runner_managers.py b/github-runner-manager/tests/unit/fake_runner_managers.py similarity index 87% rename from github-runner-manager/tests/unit/mock_runner_managers.py rename to github-runner-manager/tests/unit/fake_runner_managers.py index 1b159f3c0c..7f2f53d978 100644 --- a/github-runner-manager/tests/unit/mock_runner_managers.py +++ b/github-runner-manager/tests/unit/fake_runner_managers.py @@ -16,7 +16,6 @@ RunnerIdentity, RunnerMetadata, ) -from github_runner_manager.manager.runner_manager import RunnerInstance from github_runner_manager.metrics.runner import RunnerMetrics from github_runner_manager.openstack_cloud.openstack_cloud import _MAX_NOVA_COMPUTE_API_VERSION from github_runner_manager.platform.platform_provider import ( @@ -42,8 +41,8 @@ ) -class MockOpenstackCloud: - """Mock of OpenstackCloud.""" +class FakeOpenstackCloud: + """Fake implementation of OpenstackCloud.""" _MOCK_COMPUTE_ENDPOINT = "mock-compute-endpoint" _MOCK_COMPUTE_ENDPOINT_RESPONSE = {"version": {"version": _MAX_NOVA_COMPUTE_API_VERSION}} @@ -59,30 +58,30 @@ def __init__( instance.name: exc for instance, exc in (server_to_errors or {}).items() } - def __enter__(self) -> "MockOpenstackCloud": - """Mock enter method for context entering.""" + def __enter__(self) -> "FakeOpenstackCloud": + """Fake enter method for context entering.""" return self def __exit__(self, *args, **kwargs) -> None: - """Mock exit method for context exiting.""" + """Fake exit method for context exiting.""" return - def connect(self) -> "MockOpenstackCloud": - """Mock OpenStack lib's connect function.""" + def connect(self) -> "FakeOpenstackCloud": + """Fake OpenStack lib's connect function.""" return self @property - def compute(self) -> "MockOpenstackCloud": - """Mock the compute API attribute.""" + def compute(self) -> "FakeOpenstackCloud": + """Fake the compute API attribute.""" return self def get_endpoint(self) -> str: - """Mock endpoint string for compute endpoint.""" + """Fake endpoint string for compute endpoint.""" return self._MOCK_COMPUTE_ENDPOINT @property def session(self) -> dict: - """Mock the connection session attribute.""" + """Fake the connection session attribute.""" compute_endpoint_mock = MagicMock() compute_endpoint_mock.json.return_value = self._MOCK_COMPUTE_ENDPOINT_RESPONSE return {self._MOCK_COMPUTE_ENDPOINT: compute_endpoint_mock} @@ -95,7 +94,7 @@ def delete_server( delete_ips: bool = False, delete_ip_retry: int = 1, ) -> bool: - """Mock method for deleting server.""" + """Fake method for deleting server.""" injected_test_error = self._injected_errors.pop(name_or_id, None) if injected_test_error: raise injected_test_error @@ -105,14 +104,14 @@ def delete_server( return False def delete_keypair(self, *args, **kwargs): - """Mock delete keypair method.""" + """Fake delete keypair method.""" pass -class MockCloudRunnerManager(CloudRunnerManager): - """Mock of CloudRunnerManager. +class FakeCloudRunnerManager(CloudRunnerManager): + """Fake of CloudRunnerManager. - Metrics is not supported in this mock. + Metrics is not supported in this fake. Attributes: name_prefix: The naming prefix for runners managed. @@ -121,7 +120,7 @@ class MockCloudRunnerManager(CloudRunnerManager): @property def name_prefix(self) -> str: """The naming prefix for runners managed.""" - return "mock_cloud_runner_manager" + return "fake_cloud_runner_manager" def __init__(self, initial_cloud_runners: list[CloudRunnerInstance]) -> None: """Initialize the Cloud Runner Manager. @@ -175,7 +174,7 @@ def delete_vms(self, instance_ids: Sequence[InstanceID]) -> list[InstanceID]: def extract_metrics(self, instance_ids: Sequence[InstanceID]) -> list[RunnerMetrics]: """Extract metrics from VMs with given instance ids. - The mock runner manager does not implement this. + The fake runner manager does not implement this. Args: instance_ids: A list of instance ids to extract metrics from. @@ -188,16 +187,16 @@ def extract_metrics(self, instance_ids: Sequence[InstanceID]) -> list[RunnerMetr def cleanup(self) -> None: """Cleanup cloud resources. - The mock runner manager does not implement this. + The fake runner manager does not implement this. """ pass -class MockGitHubRunnerPlatform(PlatformProvider): - """Mock GitHub platform provider.""" +class FakeGitHubRunnerPlatform(PlatformProvider): + """Fake GitHub platform provider.""" def __init__(self, initial_runners: Sequence[SelfHostedRunner]) -> None: - """Initialize the mock platform. + """Initialize the fake platform. Args: initial_runners: Runners to instantiate the platform with. @@ -318,15 +317,3 @@ def get_job_info( NotImplementedError: This method is not tested with this mock. """ raise NotImplementedError - - -class MockRunnerManager: - """Mock Runner manager for testing.""" - - def __init__(self, runners: Sequence[RunnerInstance]) -> None: - """Initialize the mock runner manager. - - Args: - runners: The runners to initialize the RunnerManager with. - """ - self._runners = runners diff --git a/github-runner-manager/tests/unit/manager/test_runner_manager.py b/github-runner-manager/tests/unit/manager/test_runner_manager.py index d29df45e4b..090522e2a3 100644 --- a/github-runner-manager/tests/unit/manager/test_runner_manager.py +++ b/github-runner-manager/tests/unit/manager/test_runner_manager.py @@ -20,7 +20,7 @@ RunnerInstanceFactory, SelfHostedRunnerFactory, ) -from tests.unit.mock_runner_managers import MockCloudRunnerManager, MockGitHubRunnerPlatform +from tests.unit.fake_runner_managers import FakeCloudRunnerManager, FakeGitHubRunnerPlatform @pytest.mark.parametrize( @@ -107,8 +107,8 @@ def test_flush_runners( act: Call flush in the RunnerManager instance. assert: Expected github runners and cloud runners are flushed. """ - mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) - mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + mock_platform = FakeGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = FakeCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) manager = RunnerManager( "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) @@ -180,8 +180,8 @@ def test_runner_maanger_cleanup( act: Call cleanup in the RunnerManager instance. assert: Expected github runners and cloud runners cleanup is run. """ - mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) - mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + mock_platform = FakeGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = FakeCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) manager = RunnerManager( "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) @@ -254,8 +254,8 @@ def test_runner_manager_get_runners( act: when RunnerManager.get_runners is called. assert: expected RunnerInstances are returned. """ - mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) - mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + mock_platform = FakeGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = FakeCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) manager = RunnerManager( "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) @@ -308,8 +308,8 @@ def test_runner_manager_deterministic_delete_runners( act: when RunnerManager.delete_runners is called. assert: expected cloud & platform runners remain. """ - mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) - mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + mock_platform = FakeGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = FakeCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) manager = RunnerManager( "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) @@ -351,8 +351,8 @@ def test_runner_manager_non_deterministic_delete_runners( act: when RunnerManager.delete_runners is called. assert: expected cloud & platform runners remain. """ - mock_platform = MockGitHubRunnerPlatform(initial_runners=initial_runners) - mock_cloud = MockCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) + mock_platform = FakeGitHubRunnerPlatform(initial_runners=initial_runners) + mock_cloud = FakeCloudRunnerManager(initial_cloud_runners=initial_cloud_runners) manager = RunnerManager( "test-manager", platform_provider=mock_platform, cloud_runner_manager=mock_cloud, labels=[] ) diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index ae335f1e0e..3e953877e1 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -32,7 +32,7 @@ _DeleteKeypairConfig, get_missing_security_rules, ) -from tests.unit.mock_runner_managers import MockOpenstackCloud +from tests.unit.fake_runner_managers import FakeOpenstackCloud FAKE_ARG = "fake" FAKE_PREFIX = "fake_prefix" @@ -382,7 +382,7 @@ def test_delete_instances_partial_server_delete_failure( successful_delete_id = InstanceID(prefix="success", reactive=False, suffix="") already_deleted_id = InstanceID(prefix="already_deleted", reactive=False, suffix="") timeout_id = InstanceID(prefix="timeout error", reactive=False, suffix="") - mock_cloud = MockOpenstackCloud( + mock_cloud = FakeOpenstackCloud( initial_servers=[successful_delete_id, timeout_id], server_to_errors={timeout_id: openstack.exceptions.ResourceTimeout()}, ) From 6dabc41ff3550aa634e2ed677ecbc7629424a9be Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 17 Jul 2025 03:38:21 +0000 Subject: [PATCH 39/50] fix: lint issues w docstring --- .../github_runner_manager/openstack_cloud/openstack_cloud.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 99690b40c4..c5f57fb928 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -346,7 +346,7 @@ def _delete_instance(delete_config: _DeleteVMConfig) -> bool: delete_config: The configuration used to delete a cloud VM instance. Raises: - OpenStackVMDeleteError: If there was an error deleting the VM instance. + DeleteVMError: If there was an error deleting the VM instance. """ with openstack.connect( auth_url=delete_config.credentials.auth_url, From 65fc56d1b5b01effdf79296ea9b77a8bf8fbff69 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 01:34:19 +0000 Subject: [PATCH 40/50] debug --- .github/workflows/integration_test.yaml | 33 ++++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index 649d47df23..53afd74758 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -20,26 +20,29 @@ jobs: juju-channel: 3.6/stable provider: lxd test-tox-env: integration-juju3.6 - modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' - extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' - self-hosted-runner: true - self-hosted-runner-label: stg-private-endpoint - openstack-integration-tests-cross-controller-private-endpoint: - name: Cross controller integration test using private-endpoint - uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main - secrets: inherit - with: - juju-channel: 3.6/stable - pre-run-script: tests/integration/setup-integration-tests.sh - provider: lxd - test-tox-env: integration-juju3.6 - modules: '["test_prometheus_metrics"]' + # modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' + modules: '["test_charm_metrics_success", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_reactive", "test_jobmanager_reactive"]' extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' self-hosted-runner: true self-hosted-runner-label: stg-private-endpoint + tmate-debug: true + tmate-timeout: 90 + # openstack-integration-tests-cross-controller-private-endpoint: + # name: Cross controller integration test using private-endpoint + # uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main + # secrets: inherit + # with: + # juju-channel: 3.6/stable + # pre-run-script: tests/integration/setup-integration-tests.sh + # provider: lxd + # test-tox-env: integration-juju3.6 + # modules: '["test_prometheus_metrics"]' + # extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' + # self-hosted-runner: true + # self-hosted-runner-label: stg-private-endpoint allure-report: if: ${{ (success() || failure()) && github.event_name == 'schedule' }} needs: - openstack-integration-tests-private-endpoint - - openstack-integration-tests-cross-controller-private-endpoint + # - openstack-integration-tests-cross-controller-private-endpoint uses: canonical/operator-workflows/.github/workflows/allure_report.yaml@main From 79437f6467c07e3503458aa89101c193c2703056 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 02:25:37 +0000 Subject: [PATCH 41/50] fix: instantiate vars before assignment --- .../src/github_runner_manager/metrics/runner.py | 1 + 1 file changed, 1 insertion(+) diff --git a/github-runner-manager/src/github_runner_manager/metrics/runner.py b/github-runner-manager/src/github_runner_manager/metrics/runner.py index 93cb0b9d2e..983ca1dfd0 100644 --- a/github-runner-manager/src/github_runner_manager/metrics/runner.py +++ b/github-runner-manager/src/github_runner_manager/metrics/runner.py @@ -107,6 +107,7 @@ def _pull_runner_metrics(pull_config: _PullRunnerMetricsConfig) -> "PulledMetric ) return None + runner_installed, pre_job_metrics, post_job_metrics = "", "", "" try: with pull_config.cloud_service.get_ssh_connection(instance=instance) as ssh_conn: try: From 722b383779a494b5523dd752ba118a66dd02d9d4 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 02:25:48 +0000 Subject: [PATCH 42/50] chore: delete unused test bin --- .../tests/unit/fake_runner_managers.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/github-runner-manager/tests/unit/fake_runner_managers.py b/github-runner-manager/tests/unit/fake_runner_managers.py index 7f2f53d978..ae65f919aa 100644 --- a/github-runner-manager/tests/unit/fake_runner_managers.py +++ b/github-runner-manager/tests/unit/fake_runner_managers.py @@ -29,17 +29,6 @@ logger = logging.getLogger(__name__) -# Compressed tar file for testing. -# Python `tarfile` module works on only files. -# Hardcoding a sample tar file is simpler. -TEST_BINARY = ( - b"\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03\xed\xd1\xb1\t\xc30\x14\x04P\xd5\x99B\x13\x04\xc9" - b"\xb6\xacyRx\x01[\x86\x8c\x1f\x05\x12HeHaB\xe0\xbd\xe6\x8a\x7f\xc5\xc1o\xcb\xd6\xae\xed\xde" - b"\xc2\x89R7\xcf\xd33s-\xe93_J\xc8\xd3X{\xa9\x96\xa1\xf7r\x1e\x87\x1ab:s\xd4\xdb\xbe\xb5\xdb" - b"\x1ac\xcfe=\xee\x1d\xdf\xffT\xeb\xff\xbf\xfcz\x04\x00\x00\x00\x00\x00\x00\x00\x00\x00_{\x00" - b"\xc4\x07\x85\xe8\x00(\x00\x00" -) - class FakeOpenstackCloud: """Fake implementation of OpenstackCloud.""" From 53df57c1f205b0651eada8cccb0414463e942181 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 02:26:02 +0000 Subject: [PATCH 43/50] chore: pass down delete timeout --- .../openstack_cloud/openstack_cloud.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index c5f57fb928..9c2c89941a 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -360,7 +360,11 @@ def _delete_instance(delete_config: _DeleteVMConfig) -> bool: ) as conn: try: logger.info("Deleting server %s", delete_config.instance_id.name) - deleted = conn.delete_server(name_or_id=delete_config.instance_id.name) + deleted = conn.delete_server( + name_or_id=delete_config.instance_id.name, + wait=delete_config.wait, + timeout=delete_config.timeout, + ) logger.info( "Deleted server %s (true delete: %s)", delete_config.instance_id.name, deleted ) From 346a038c04d7621fc5002e26fa830357a0deabf0 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 03:27:43 +0000 Subject: [PATCH 44/50] chore: update keypair delete to fire and forget --- .../github_runner_manager/openstack_cloud/openstack_cloud.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 9c2c89941a..9875871f63 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -713,7 +713,7 @@ def _setup_keypair( return keypair @staticmethod - def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> str | None: + def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> None: """Delete OpenStack keypair. Args: @@ -741,7 +741,6 @@ def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> str | None: key_path = delete_keypair_config.keys_dir / f"{delete_keypair_config.instance_id}.key" key_path.unlink(missing_ok=True) logger.info("Deleted key: %s", delete_keypair_config.instance_id) - return delete_keypair_config.instance_id.name @staticmethod def _ensure_security_group( From 728362850572899b04fe4d269e8641978223ede1 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 03:32:58 +0000 Subject: [PATCH 45/50] chore: undo debug --- .github/workflows/integration_test.yaml | 33 +++++++++++-------------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index 53afd74758..649d47df23 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -20,29 +20,26 @@ jobs: juju-channel: 3.6/stable provider: lxd test-tox-env: integration-juju3.6 - # modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' - modules: '["test_charm_metrics_success", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_reactive", "test_jobmanager_reactive"]' + modules: '["test_charm_metrics_failure", "test_charm_metrics_success", "test_charm_fork_repo", "test_charm_fork_path_change", "test_charm_no_runner", "test_charm_runner", "test_debug_ssh", "test_charm_upgrade", "test_reactive", "test_jobmanager_prespawned", "test_jobmanager_reactive"]' + extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' + self-hosted-runner: true + self-hosted-runner-label: stg-private-endpoint + openstack-integration-tests-cross-controller-private-endpoint: + name: Cross controller integration test using private-endpoint + uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main + secrets: inherit + with: + juju-channel: 3.6/stable + pre-run-script: tests/integration/setup-integration-tests.sh + provider: lxd + test-tox-env: integration-juju3.6 + modules: '["test_prometheus_metrics"]' extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' self-hosted-runner: true self-hosted-runner-label: stg-private-endpoint - tmate-debug: true - tmate-timeout: 90 - # openstack-integration-tests-cross-controller-private-endpoint: - # name: Cross controller integration test using private-endpoint - # uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main - # secrets: inherit - # with: - # juju-channel: 3.6/stable - # pre-run-script: tests/integration/setup-integration-tests.sh - # provider: lxd - # test-tox-env: integration-juju3.6 - # modules: '["test_prometheus_metrics"]' - # extra-arguments: '-m openstack --log-format="%(asctime)s %(levelname)s %(message)s"' - # self-hosted-runner: true - # self-hosted-runner-label: stg-private-endpoint allure-report: if: ${{ (success() || failure()) && github.event_name == 'schedule' }} needs: - openstack-integration-tests-private-endpoint - # - openstack-integration-tests-cross-controller-private-endpoint + - openstack-integration-tests-cross-controller-private-endpoint uses: canonical/operator-workflows/.github/workflows/allure_report.yaml@main From e009961b72ee62a213eac6de7f948a14d2dee264 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 03:48:09 +0000 Subject: [PATCH 46/50] fix: lint rules --- .../openstack_cloud/openstack_cloud.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index 9875871f63..d6bc3496a0 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -718,9 +718,6 @@ def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> None: Args: delete_keypair_config: Configurations for deleting the KeyPair. - - Returns: - Name of the successfully deleted key. None otherwise. """ logger.info("Deleting key: %s", delete_keypair_config.instance_id) try: @@ -729,14 +726,14 @@ def _delete_keypair(delete_keypair_config: _DeleteKeypairConfig) -> None: delete_keypair_config.instance_id.name ): logger.warning("Failed to delete key: %s", delete_keypair_config.instance_id.name) - return None + return except (openstack.exceptions.SDKException, openstack.exceptions.ResourceTimeout): logger.warning( "Error attempting to delete key: %s", delete_keypair_config.instance_id.name, stack_info=True, ) - return None + return key_path = delete_keypair_config.keys_dir / f"{delete_keypair_config.instance_id}.key" key_path.unlink(missing_ok=True) From 2975bf1098fac4d3d9c6474554be3fe16b9c1604 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Mon, 21 Jul 2025 04:42:20 +0000 Subject: [PATCH 47/50] chore: fix meta docstring --- .../tests/unit/factories/metrics_factory.py | 8 ++++---- .../tests/unit/factories/runner_instance_factory.py | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/github-runner-manager/tests/unit/factories/metrics_factory.py b/github-runner-manager/tests/unit/factories/metrics_factory.py index 9f3e163d0e..43b649a6c4 100644 --- a/github-runner-manager/tests/unit/factories/metrics_factory.py +++ b/github-runner-manager/tests/unit/factories/metrics_factory.py @@ -13,7 +13,7 @@ class EventFactory(factory.Factory): """Factory for creating Event instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for Event. Attributes: model: The metadata reference model. @@ -29,7 +29,7 @@ class RunnerInstalledFactory(EventFactory): """Factory for creating RunnerInstalled instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for RunnerInstalled. Attributes: model: The metadata reference model. @@ -45,7 +45,7 @@ class CodeInformationFactory(factory.Factory): """Factory for creating CodeInformation instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for CodeInformation. Attributes: model: The metadata reference model. @@ -60,7 +60,7 @@ class RunnerStopFactory(EventFactory): """Factory for creating RunnerStop instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for RunnerStop. Attributes: model: The metadata reference model. diff --git a/github-runner-manager/tests/unit/factories/runner_instance_factory.py b/github-runner-manager/tests/unit/factories/runner_instance_factory.py index 423e7614ed..cf594127e2 100644 --- a/github-runner-manager/tests/unit/factories/runner_instance_factory.py +++ b/github-runner-manager/tests/unit/factories/runner_instance_factory.py @@ -26,7 +26,7 @@ class InstanceIDFactory(factory.Factory): """Factory class for creating InstanceID.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for InstanceID. Attributes: model: The metadata reference model. @@ -43,7 +43,7 @@ class RunnerMetadataFactory(factory.Factory): """Factory for creating RunnerMetadata instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for RunnerMetadata. Attributes: model: The metadata reference model. @@ -60,7 +60,7 @@ class CloudRunnerInstanceFactory(factory.Factory): """Factory for creating CloudRunnerInstance instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for CloudRunnerInstance. Attributes: model: The metadata reference model. @@ -95,7 +95,7 @@ class RunnerIdentityFactory(factory.Factory): """Factory for creating RunnerIdentity instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for RunnerIdentity. Attributes: model: The metadata reference model. @@ -111,7 +111,7 @@ class PlatformRunnerHealthFactory(factory.Factory): """Factory for creating PlatformRunnerHealth instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for PlatformRunnerHealth. Attributes: model: The metadata reference model. @@ -178,7 +178,7 @@ class SelfHostedRunnerFactory(factory.Factory): """Factory for creating SelfHostedRunner instances.""" class Meta: - """Meta class for RunnerInstance. + """Meta class for SelfHostedRunner. Attributes: model: The metadata reference model. From c13faece332a403b288dd4429803a6f449b744c9 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 23 Jul 2025 06:02:58 +0000 Subject: [PATCH 48/50] chore: remove unused platform argument after multiplexer ejection --- .../src/github_runner_manager/manager/runner_manager.py | 7 ++----- .../src/github_runner_manager/platform/github_provider.py | 3 +-- .../github_runner_manager/platform/jobmanager_provider.py | 3 +-- .../github_runner_manager/platform/platform_provider.py | 3 +-- github-runner-manager/tests/unit/fake_runner_managers.py | 3 +-- 5 files changed, 6 insertions(+), 13 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py index 55824df7d1..e6509d51f3 100644 --- a/github-runner-manager/src/github_runner_manager/manager/runner_manager.py +++ b/github-runner-manager/src/github_runner_manager/manager/runner_manager.py @@ -345,8 +345,7 @@ def _delete_cloud_runners( ] logger.info("Deleting runners from platform: %s", platform_runner_ids_to_delete) deleted_runner_ids = self._platform.delete_runners( - runner_ids=platform_runner_ids_to_delete, - platform=cloud_runners[0].metadata.platform_name, + runner_ids=platform_runner_ids_to_delete ) logger.info( "Deleted runners from platform: %s (diff: %s)", @@ -385,9 +384,7 @@ def _clean_platform_runners(self, runners: list[RunnerIdentity]) -> None: runner_ids_to_delete = [ runner.metadata.runner_id for runner in runners if runner.metadata.runner_id ] - self._platform.delete_runners( - runner_ids=runner_ids_to_delete, platform=runners[0].metadata.platform_name - ) + self._platform.delete_runners(runner_ids=runner_ids_to_delete) @staticmethod def _spawn_runners( diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index 2cca793ed4..5b16163a21 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -164,14 +164,13 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners non_requested_runners=non_requested_runners, ) - def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: + def delete_runners(self, runner_ids: list[str]) -> list[str]: """Delete runners from GitHub. This method will ignore DeleteRunnerBusyErrors and print a warning log. Args: runner_ids: The GitHub runner IDs to delete. - platform: Unused argument. Returns: The runner IDs that were deleted successfully. diff --git a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py index 3a4b0a162e..91c4f4c5fa 100644 --- a/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/jobmanager_provider.py @@ -133,14 +133,13 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners failed_requested_runners=failed_runners, ) - def delete_runners(self, runner_ids: list[str], platform: str = "jobmanager") -> list[str]: + def delete_runners(self, runner_ids: list[str]) -> list[str]: """Delete a runner from jobmanager. This method does nothing, as the jobmanager does not implement it. Args: runner_ids: The runner IDs to delete. - platform: Unused argument. Returns: The runner IDs requested for deletion. diff --git a/github-runner-manager/src/github_runner_manager/platform/platform_provider.py b/github-runner-manager/src/github_runner_manager/platform/platform_provider.py index 2e59d63713..8b5c5fa4bd 100644 --- a/github-runner-manager/src/github_runner_manager/platform/platform_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/platform_provider.py @@ -86,12 +86,11 @@ def get_runners_health( """ @abc.abstractmethod - def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: + def delete_runners(self, runner_ids: list[str]) -> list[str]: """Delete runners. Args: runner_ids: Runner IDs to delete. - platform: The Platform in which to delete the runners in. """ @abc.abstractmethod diff --git a/github-runner-manager/tests/unit/fake_runner_managers.py b/github-runner-manager/tests/unit/fake_runner_managers.py index ae65f919aa..d879946882 100644 --- a/github-runner-manager/tests/unit/fake_runner_managers.py +++ b/github-runner-manager/tests/unit/fake_runner_managers.py @@ -244,12 +244,11 @@ def get_runners_health(self, requested_runners: list[RunnerIdentity]) -> Runners response.non_requested_runners.append(runner.identity) return response - def delete_runners(self, runner_ids: list[str], platform: str = "github") -> list[str]: + def delete_runners(self, runner_ids: list[str]) -> list[str]: """Delete runners from platform. Args: runner_ids: The runner IDs to delete. - platform: The target platform. Returns: The successfully deleted runners. From fcf7cb1c0f628ce0119538479f408f4bae276e8a Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 24 Jul 2025 02:10:56 +0000 Subject: [PATCH 49/50] fix: result returns none --- .../openstack_cloud/openstack_cloud.py | 4 +-- .../platform/github_provider.py | 4 +-- .../unit/platform/test_github_provider.py | 32 +++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index d6bc3496a0..a8fc159486 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -428,8 +428,8 @@ def delete_instances( for future in concurrent.futures.as_completed(future_to_delete_instance_config): delete_config = future_to_delete_instance_config[future] try: - if future.result(): - deleted_instance_ids.append(delete_config.instance_id) + future.result() + deleted_instance_ids.append(delete_config.instance_id) except DeleteVMError as exc: logger.error("Failed to delete OpenStack VM instance: %s", exc.instance_id) diff --git a/github-runner-manager/src/github_runner_manager/platform/github_provider.py b/github-runner-manager/src/github_runner_manager/platform/github_provider.py index 5b16163a21..11eabf49ba 100644 --- a/github-runner-manager/src/github_runner_manager/platform/github_provider.py +++ b/github-runner-manager/src/github_runner_manager/platform/github_provider.py @@ -195,8 +195,8 @@ def delete_runners(self, runner_ids: list[str]) -> list[str]: for future in concurrent.futures.as_completed(future_to_delete_runner_config): delete_config = future_to_delete_runner_config[future] try: - if future.result(): - deleted_runner_ids.append(delete_config.runner_id) + future.result() + deleted_runner_ids.append(delete_config.runner_id) except DeleteRunnerBusyError: logger.warning( "Delete runner attempt failed, busy runner: %s", diff --git a/github-runner-manager/tests/unit/platform/test_github_provider.py b/github-runner-manager/tests/unit/platform/test_github_provider.py index 3175938b4e..23588792d6 100644 --- a/github-runner-manager/tests/unit/platform/test_github_provider.py +++ b/github-runner-manager/tests/unit/platform/test_github_provider.py @@ -15,6 +15,7 @@ GitHubRunnerPlatform, ) from github_runner_manager.platform.platform_provider import ( + DeleteRunnerBusyError, PlatformRunnerHealth, RunnersHealthResponse, ) @@ -239,3 +240,34 @@ def test_get_runners_health( runners_health_response = platform.get_runners_health(requested_runners) assert runners_health_response == expected_health_response + + +def test_github_provider_delete_busy_runner_error(): + """ + arrange: given a mocked GitHub client that raises DeleteRunnerBusyError. + act: when GitHubRunnerPlatform.delete_runners is called. + assert: act: no ids are returned. + """ + mock_github_client = MagicMock() + mock_github_client.delete_runner.side_effect = DeleteRunnerBusyError + github_provider = GitHubRunnerPlatform( + prefix="test", path="test", github_client=mock_github_client + ) + test_delete_ids = ["1", "2", "3"] + + assert github_provider.delete_runners(test_delete_ids) == [] + + +def test_github_provider_delete_runners(): + """ + arrange: given a mocked GitHub client. + act: when GitHubRunnerPlatform.delete_runners is called. + assert: act: the deleted runner IDs are returned. + """ + mock_github_client = MagicMock() + github_provider = GitHubRunnerPlatform( + prefix="test", path="test", github_client=mock_github_client + ) + test_delete_ids = ["1", "2", "3"] + + assert sorted(github_provider.delete_runners(test_delete_ids)) == sorted(test_delete_ids) From b06bf0b7c4201f1e030229999dea83e63c3878f2 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 24 Jul 2025 02:20:35 +0000 Subject: [PATCH 50/50] fix: only return truely deleted openstack IDs --- .../github_runner_manager/openstack_cloud/openstack_cloud.py | 3 ++- .../tests/unit/openstack_cloud/test_openstack_cloud.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py index a8fc159486..083eb0a549 100644 --- a/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py +++ b/github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py @@ -428,7 +428,8 @@ def delete_instances( for future in concurrent.futures.as_completed(future_to_delete_instance_config): delete_config = future_to_delete_instance_config[future] try: - future.result() + if not future.result(): + continue deleted_instance_ids.append(delete_config.instance_id) except DeleteVMError as exc: logger.error("Failed to delete OpenStack VM instance: %s", exc.instance_id) diff --git a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py index 3e953877e1..9f169e6411 100644 --- a/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py +++ b/github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py @@ -419,7 +419,7 @@ def test_delete_instances( instance_ids=[successful_delete_id, already_deleted_id] ) - assert [successful_delete_id] == deleted_instance_ids + assert deleted_instance_ids == [successful_delete_id] @pytest.mark.parametrize(