From 659af6c1b45a7f0c2ef7855acaa3c4066b46fee9 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Fri, 3 Jul 2026 12:41:51 +0800 Subject: [PATCH 1/2] feat: pressure non streaming mode --- charmcraft.yaml | 6 ++ .../configuration/base.py | 4 +- .../manager/pressure_reconciler.py | 62 +++++++++++----- .../github_runner_manager/planner_client.py | 36 ++++++++++ .../unit/manager/test_pressure_reconciler.py | 72 ++++++++++++++----- src/charm_state.py | 14 ++++ src/factories.py | 1 + tests/unit/conftest.py | 1 + tests/unit/factories.py | 2 + tests/unit/test_charm_state.py | 16 +++++ tests/unit/test_factories.py | 1 + 11 files changed, 178 insertions(+), 37 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 910e91ae8e..398d084686 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -126,6 +126,12 @@ config: Minutes between each reconciliation of the current runners state and their targeted state. On reconciliation, the charm polls the state of runners and see if actions are needed. The value should be kept low, unless Github API rate limiting is encountered. + planner-pressure-mode: + type: string + default: stream + description: >- + Planner pressure fetch mode. Set to `stream` to consume continuous planner updates, + or `request` to poll the planner with periodic one-shot requests. allow-external-contributor: type: boolean default: True diff --git a/github-runner-manager/src/github_runner_manager/configuration/base.py b/github-runner-manager/src/github_runner_manager/configuration/base.py index 7b227aff98..c87c0898e3 100644 --- a/github-runner-manager/src/github_runner_manager/configuration/base.py +++ b/github-runner-manager/src/github_runner_manager/configuration/base.py @@ -5,7 +5,7 @@ import logging from dataclasses import dataclass -from typing import Optional, TextIO +from typing import Literal, Optional, TextIO import yaml from pydantic import AnyHttpUrl, BaseModel, Field, IPvAnyAddress, root_validator @@ -52,6 +52,7 @@ class ApplicationConfiguration(BaseModel): planner_url: Base URL of the planner service. planner_token: Bearer token to authenticate against the planner service. reconcile_interval: Minutes to wait between reconciliation. + planner_pressure_mode: Pressure fetch mode (`stream` or `request`). """ allow_external_contributor: bool = False @@ -64,6 +65,7 @@ class ApplicationConfiguration(BaseModel): planner_url: Optional[AnyHttpUrl] = None planner_token: Optional[str] = None reconcile_interval: int = Field(ge=1) + planner_pressure_mode: Literal["stream", "request"] = "stream" @staticmethod def from_yaml_file(file: TextIO) -> "ApplicationConfiguration": diff --git a/github-runner-manager/src/github_runner_manager/manager/pressure_reconciler.py b/github-runner-manager/src/github_runner_manager/manager/pressure_reconciler.py index 21ccaa37d4..a28ff7444b 100644 --- a/github-runner-manager/src/github_runner_manager/manager/pressure_reconciler.py +++ b/github-runner-manager/src/github_runner_manager/manager/pressure_reconciler.py @@ -17,7 +17,7 @@ import time from dataclasses import dataclass from threading import Event, Lock -from typing import Optional +from typing import Literal, Optional from github_runner_manager.configuration import ApplicationConfiguration from github_runner_manager.configuration.base import RunnerCombination, UserInfo @@ -59,6 +59,7 @@ class PressureReconcilerConfig: Attributes: flavor_name: Name of the planner flavor to reconcile. reconcile_interval: Minutes between timer-based delete reconciliations. + planner_pressure_mode: Planner pressure fetch mode (`stream` or `request`). min_pressure: Minimum desired runner count (floor) for the flavor. Also used as fallback when the planner is unavailable. max_pressure: Maximum desired runner count (ceiling). 0 means no cap. @@ -66,6 +67,7 @@ class PressureReconcilerConfig: flavor_name: str reconcile_interval: int = 5 + planner_pressure_mode: Literal["stream", "request"] = "stream" min_pressure: int = 0 max_pressure: int = 0 @@ -95,16 +97,15 @@ class PressureReconciler: # pylint: disable=too-few-public-methods,too-many-ins The reconcile loop uses the last pressure seen by the create loop rather than fetching a fresh value, so it may act on a stale reading if pressure changed - between stream events. This is an accepted trade-off: the window is bounded - by the stream update frequency. + between planner updates. This is an accepted trade-off. Attributes: _manager: Runner manager used to list, create, and clean up runners. - _planner: Client used to stream pressure updates. + _planner: Client used to fetch pressure updates. _config: Reconciler configuration. _lock: Shared lock to serialize operations with other reconcile loops. - _stop: Event used to signal streaming loops to stop gracefully. - _last_pressure: Last pressure value seen in the create stream. + _stop: Event used to signal reconciliation loops to stop gracefully. + _last_pressure: Last pressure value seen in the create loop. _runner_count: In-memory runner count used by the create loop. _create_paused: True when creation returned zero IDs, cleared by reconcile loop. """ @@ -121,7 +122,7 @@ def __init__( Args: manager: Runner manager interface for creating, cleaning up, and listing runners. - planner_client: Client used to stream pressure updates. + planner_client: Client used to fetch pressure updates. None when no planner relation is configured. config: Reconciler configuration. lock: Shared lock to serialize operations with other reconcile loops. @@ -152,29 +153,51 @@ def start_create_loop(self) -> None: return while not self._stop.is_set(): try: - for update in self._planner.stream_pressure(self._config.flavor_name): + if self._config.planner_pressure_mode == "request": + update = self._planner.get_pressure(self._config.flavor_name) if self._stop.is_set(): return self._handle_create_runners(update.pressure) + self._stop.wait(5) + else: + for update in self._planner.stream_pressure(self._config.flavor_name): + if self._stop.is_set(): + return + self._handle_create_runners(update.pressure) except PlannerConnectionError as exc: fallback = max(self._last_pressure or 0, self._config.min_pressure) - logger.warning( - "Pressure stream interrupted for flavor %s (%s), falling back to %s runners.", - self._config.flavor_name, - exc, - fallback, - ) + if self._config.planner_pressure_mode == "request": + logger.warning( + "Pressure request failed for flavor %s (%s), falling back to %s runners.", + self._config.flavor_name, + exc, + fallback, + ) + else: + logger.warning( + "Pressure stream interrupted for flavor %s (%s), falling back to %s runners.", + self._config.flavor_name, + exc, + fallback, + ) if self._stop.is_set(): return self._handle_create_runners(fallback) self._stop.wait(5) except PlannerApiError: fallback = max(self._last_pressure or 0, self._config.min_pressure) - logger.exception( - "Error in pressure stream loop for flavor %s, falling back to %s runners.", - self._config.flavor_name, - fallback, - ) + if self._config.planner_pressure_mode == "request": + logger.exception( + "Error in pressure request loop for flavor %s, falling back to %s runners.", + self._config.flavor_name, + fallback, + ) + else: + logger.exception( + "Error in pressure stream loop for flavor %s, falling back to %s runners.", + self._config.flavor_name, + fallback, + ) if self._stop.is_set(): return self._handle_create_runners(fallback) @@ -439,6 +462,7 @@ def build_pressure_reconciler( config=PressureReconcilerConfig( flavor_name=config.name, reconcile_interval=config.reconcile_interval, + planner_pressure_mode=config.planner_pressure_mode, min_pressure=first.base_virtual_machines, max_pressure=first.max_total_virtual_machines, ), diff --git a/github-runner-manager/src/github_runner_manager/planner_client.py b/github-runner-manager/src/github_runner_manager/planner_client.py index 6ff371d1f5..53726f5c3d 100644 --- a/github-runner-manager/src/github_runner_manager/planner_client.py +++ b/github-runner-manager/src/github_runner_manager/planner_client.py @@ -63,6 +63,42 @@ def __init__(self, config: PlannerConfiguration) -> None: self._session = self._create_session() self._config = config + def get_pressure(self, name: str) -> PressureInfo: + """Get pressure for the given flavor with a single request. + + Args: + name: Flavor name. + + Returns: + Parsed pressure info. + + Raises: + PlannerConnectionError: On transient connection failures or timeouts. + PlannerApiError: On HTTP errors or invalid payloads. + """ + base = str(self._config.base_url).rstrip("/") + "/" + url = urljoin(base, f"api/v1/flavors/{name}/pressure") + try: + response = self._session.get( + url, + headers={"Authorization": f"Bearer {self._config.token}"}, + timeout=self._config.timeout, + ) + response.raise_for_status() + + data = response.json() + if not isinstance(data, dict) or name not in data: + raise PlannerApiError(f"Unexpected pressure response payload: {data}") + + try: + return PressureInfo(pressure=int(data[name])) + except (TypeError, ValueError) as exc: + raise PlannerApiError(f"Invalid pressure value in payload: {data}") from exc + except (requests.ConnectionError, requests.Timeout) as exc: + raise PlannerConnectionError(str(exc)) from exc + except requests.RequestException as exc: + raise PlannerApiError(str(exc)) from exc + def stream_pressure(self, name: str) -> Iterable[PressureInfo]: """Stream pressure updates for the given flavor. diff --git a/github-runner-manager/tests/unit/manager/test_pressure_reconciler.py b/github-runner-manager/tests/unit/manager/test_pressure_reconciler.py index 4ead51b420..d97d7b3a56 100644 --- a/github-runner-manager/tests/unit/manager/test_pressure_reconciler.py +++ b/github-runner-manager/tests/unit/manager/test_pressure_reconciler.py @@ -82,23 +82,36 @@ class _FakePlanner: def __init__( self, - stream_updates: list[int] | None = None, - stream_exception: Exception | None = None, + pressure_updates: list[int] | None = None, + pressure_exception: Exception | None = None, ): - """Initialize with configurable stream behavior.""" - self._stream_updates = stream_updates or [] - self._stream_exception = stream_exception + """Initialize with configurable pressure request behavior.""" + self._pressure_updates = pressure_updates or [] + self._pressure_exception = pressure_exception + self._index = 0 def stream_pressure(self, name: str): # noqa: ARG002 - """Yield pressure updates or raise the configured exception. + """Yield pressure updates for stream mode tests.""" + if self._pressure_exception is not None: + raise self._pressure_exception + for pressure in self._pressure_updates: + yield SimpleNamespace(pressure=pressure) - Yields: - Namespace objects with a pressure attribute. - """ - if self._stream_exception is not None: - raise self._stream_exception - for p in self._stream_updates: - yield SimpleNamespace(pressure=p) + def get_pressure(self, name: str): # noqa: ARG002 + """Return pressure update or raise the configured exception.""" + if self._pressure_exception is not None: + raise self._pressure_exception + + if self._pressure_updates: + if self._index < len(self._pressure_updates): + pressure = self._pressure_updates[self._index] + self._index += 1 + else: + pressure = self._pressure_updates[-1] + else: + pressure = 0 + + return SimpleNamespace(pressure=pressure) @pytest.mark.parametrize( @@ -117,7 +130,7 @@ def test_min_pressure_used_as_fallback_when_stream_errors( assert: min_pressure is used as fallback to create runners. """ mgr = _FakeManager() - planner = _FakePlanner(stream_exception=planner_error) + planner = _FakePlanner(pressure_exception=planner_error) cfg = PressureReconcilerConfig(flavor_name="small", min_pressure=2) reconciler = PressureReconciler(mgr, planner, cfg, lock=Lock()) @@ -148,7 +161,7 @@ def test_fallback_preserves_last_pressure_when_higher( assert: The higher last_pressure is used as fallback instead of min_pressure. """ mgr = _FakeManager() - planner = _FakePlanner(stream_exception=planner_error) + planner = _FakePlanner(pressure_exception=planner_error) cfg = PressureReconcilerConfig(flavor_name="small", min_pressure=2) reconciler = PressureReconciler(mgr, planner, cfg, lock=Lock()) reconciler._last_pressure = 10 @@ -422,7 +435,7 @@ def test_create_loop_syncs_runner_count_on_start(monkeypatch: pytest.MonkeyPatch so no unnecessary runners are created. """ mgr = _FakeManager(runners_count=3) - planner = _FakePlanner(stream_updates=[3]) + planner = _FakePlanner(pressure_updates=[3]) cfg = PressureReconcilerConfig(flavor_name="small") reconciler = PressureReconciler(mgr, planner, cfg, lock=Lock()) @@ -430,7 +443,7 @@ def test_create_loop_syncs_runner_count_on_start(monkeypatch: pytest.MonkeyPatch original_stream = planner.stream_pressure def _stream_once(name): - """Yield from original stream, then stop the reconciler.""" + """Yield one stream batch, then stop the reconciler.""" yield from original_stream(name) reconciler.stop() @@ -441,6 +454,31 @@ def _stream_once(name): assert mgr.created_args == [] +def test_create_loop_request_mode_uses_get_pressure(monkeypatch: pytest.MonkeyPatch): + """ + arrange: A reconciler in request mode with no existing runners and pressure=2. + act: Run start_create_loop for one request cycle. + assert: It creates runners from get_pressure and does not require stream updates. + """ + mgr = _FakeManager(runners_count=0) + planner = _FakePlanner(pressure_updates=[2]) + cfg = PressureReconcilerConfig(flavor_name="small", planner_pressure_mode="request") + reconciler = PressureReconciler(mgr, planner, cfg, lock=Lock()) + + wait_calls = {"count": 0} + + def _wait(_interval: int) -> bool: + """Stop after the first request-mode polling wait.""" + wait_calls["count"] += 1 + reconciler.stop() + return True + + monkeypatch.setattr(reconciler._stop, "wait", _wait) + reconciler.start_create_loop() + + assert mgr.created_args == [2] + + def test_timer_reconcile_scales_down_with_soft_delete(): """ arrange: A reconciler with 5 runners and a lower desired pressure of 2. diff --git a/src/charm_state.py b/src/charm_state.py index b6ee705cd8..2fbee64cc1 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -87,7 +87,10 @@ PLANNER_DEFAULT_PLATFORM: Final[str] = "github" PLANNER_DEFAULT_PRIORITY: Final[int] = 50 +PLANNER_PRESSURE_MODE_CONFIG_NAME = "planner-pressure-mode" + LogLevel = Literal["CRITICAL", "FATAL", "ERROR", "WARNING", "INFO", "DEBUG"] +PlannerPressureMode = Literal["stream", "request"] @dataclasses.dataclass(frozen=True) @@ -319,6 +322,7 @@ class CharmConfig(BaseModel): path: GitHub repository path in the format '/', or the GitHub organization name. reconcile_interval: Time between each reconciliation of runners in minutes. + planner_pressure_mode: Planner pressure fetch mode (`stream` or `request`). token: GitHub personal access token for GitHub API. app_client_id: GitHub App Client ID for GitHub API. installation_id: GitHub App installation ID for GitHub API. @@ -338,6 +342,7 @@ class CharmConfig(BaseModel): openstack_clouds_yaml: OpenStackCloudsYAML path: GitHubPath | None reconcile_interval: int + planner_pressure_mode: PlannerPressureMode = "stream" token: str | None app_client_id: str | None installation_id: int | None @@ -612,6 +617,14 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": f"The {RECONCILE_INTERVAL_CONFIG_NAME} config must be greater than or equal to 1" ) + planner_pressure_mode = cast( + str, charm.config.get(PLANNER_PRESSURE_MODE_CONFIG_NAME, "stream") + ) + if planner_pressure_mode not in {"stream", "request"}: + raise CharmConfigInvalidError( + f"The {PLANNER_PRESSURE_MODE_CONFIG_NAME} config must be one of: stream, request" + ) + dockerhub_mirror = cast(str, charm.config.get(DOCKERHUB_MIRROR_CONFIG_NAME, "")) or None openstack_clouds_yaml = cls._parse_openstack_clouds_config(charm) @@ -641,6 +654,7 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": openstack_clouds_yaml=openstack_clouds_yaml, path=github_config.path, reconcile_interval=reconcile_interval, + planner_pressure_mode=cast(PlannerPressureMode, planner_pressure_mode), token=github_config.token, app_client_id=github_config.app_client_id, installation_id=github_config.installation_id, diff --git a/src/factories.py b/src/factories.py index acb767a985..241a0b5980 100644 --- a/src/factories.py +++ b/src/factories.py @@ -72,6 +72,7 @@ def create_application_configuration( planner_url=state.planner_config.endpoint if state.planner_config else None, planner_token=state.planner_config.token if state.planner_config else None, reconcile_interval=state.charm_config.reconcile_interval, + planner_pressure_mode=state.charm_config.planner_pressure_mode, ) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 8ef6020cac..a8a68ac81d 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -156,6 +156,7 @@ def complete_charm_state_fixture(): ), path=GitHubOrg(org="canonical", group="group"), reconcile_interval=5, + planner_pressure_mode="stream", token="githubtoken", app_client_id=None, installation_id=None, diff --git a/tests/unit/factories.py b/tests/unit/factories.py index c5b965aeb3..56d67290ab 100644 --- a/tests/unit/factories.py +++ b/tests/unit/factories.py @@ -31,6 +31,7 @@ OPENSTACK_FLAVOR_CONFIG_NAME, OPENSTACK_NETWORK_CONFIG_NAME, PATH_CONFIG_NAME, + PLANNER_PRESSURE_MODE_CONFIG_NAME, PLANNER_INTEGRATION_NAME, RECONCILE_INTERVAL_CONFIG_NAME, RUNNER_MANAGER_LOG_LEVEL_CONFIG_NAME, @@ -154,6 +155,7 @@ class Meta: OPENSTACK_NETWORK_CONFIG_NAME: "external", OPENSTACK_FLAVOR_CONFIG_NAME: "m1.small", PATH_CONFIG_NAME: factory.Sequence(lambda n: f"mock_path_{n}"), + PLANNER_PRESSURE_MODE_CONFIG_NAME: "stream", RECONCILE_INTERVAL_CONFIG_NAME: 10, TEST_MODE_CONFIG_NAME: "", TOKEN_CONFIG_NAME: factory.Sequence(lambda n: f"mock_token_{n}"), diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index 3ebbf05a13..064a9dc753 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -36,6 +36,7 @@ OTEL_COLLECTOR_ENDPOINT_CONFIG_NAME, PATH_CONFIG_NAME, PLANNER_INTEGRATION_NAME, + PLANNER_PRESSURE_MODE_CONFIG_NAME, RECONCILE_INTERVAL_CONFIG_NAME, RUNNER_HTTP_PROXY_CONFIG_NAME, RUNNER_MANAGER_LOG_LEVEL_CONFIG_NAME, @@ -573,6 +574,19 @@ def test_charm_config_from_charm_reconcile_interval_too_low(): CharmConfig.from_charm(mock_charm) +def test_charm_config_from_charm_invalid_planner_pressure_mode(): + """ + arrange: Create a mock CharmBase instance with an invalid planner-pressure-mode. + act: Call from_charm. + assert: CharmConfigInvalidError is raised. + """ + mock_charm = MockGithubRunnerCharmFactory() + mock_charm.config[PLANNER_PRESSURE_MODE_CONFIG_NAME] = "invalid-mode" + + with pytest.raises(CharmConfigInvalidError, match="planner-pressure-mode"): + CharmConfig.from_charm(mock_charm) + + def test_charm_config_from_charm_invalid_labels(): """ arrange: Create a mock CharmBase instance with an invalid reconcile interval. @@ -598,6 +612,7 @@ def test_charm_config_from_charm_valid(): ALLOW_EXTERNAL_CONTRIBUTOR_CONFIG_NAME: "False", PATH_CONFIG_NAME: "owner/repo", RECONCILE_INTERVAL_CONFIG_NAME: "5", + PLANNER_PRESSURE_MODE_CONFIG_NAME: "request", DOCKERHUB_MIRROR_CONFIG_NAME: "https://example.com", # "clouds: { openstack: { auth: { username: 'admin' }}}" OPENSTACK_CLOUDS_YAML_CONFIG_NAME: yaml.safe_dump( @@ -639,6 +654,7 @@ def test_charm_config_from_charm_valid(): assert result.path == GitHubRepo(owner="owner", repo="repo") assert result.reconcile_interval == 5 + assert result.planner_pressure_mode == "request" assert result.dockerhub_mirror == "https://example.com" assert result.openstack_clouds_yaml == test_openstack_config assert result.labels == ("label1", "label2", "label3") diff --git a/tests/unit/test_factories.py b/tests/unit/test_factories.py index ec88294a52..6ceaa7c71a 100644 --- a/tests/unit/test_factories.py +++ b/tests/unit/test_factories.py @@ -148,6 +148,7 @@ def test_create_application_configuration_with_planner( assert str(app_configuration.planner_url) == "http://planner.example.com" assert app_configuration.planner_token == "planner-token-value" + assert app_configuration.planner_pressure_mode == "stream" def test_create_application_configuration_with_otel_collector_config( From 394840d36fd39f2a54a8779ab24323f94261106e Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Fri, 3 Jul 2026 12:43:08 +0800 Subject: [PATCH 2/2] docs: changelog --- docs/changelog.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.md b/docs/changelog.md index 70faf71a80..23ff74928d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,6 +2,11 @@ This changelog documents user-relevant changes to the GitHub runner charm. +## 2026-07-03 + +- Added charm configuration option `planner-pressure-mode` with `stream` (default) and `request` values to control how planner pressure is fetched. +- Updated pressure reconciler behavior to support both continuous planner streaming and periodic single-request polling based on the configured mode. + ## 2026-06-26 - Fixed runners whose cloud VM entered an error state being kept until the creation timeout (~23 minutes) before cleanup. Such VMs are now cleaned up immediately, freeing the slot for a replacement runner.