feat: pressure non streaming mode - #806
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a non-streaming (polling) mode for fetching planner “pressure”, allowing deployments with unreliable streaming connectivity to still scale runner creation based on planner demand.
Changes:
- Introduces new charm config
planner-pressure-mode(streamdefault,requestpolling) and validates it in charm state. - Plumbs
planner_pressure_modeinto the generated managerApplicationConfigurationand intoPressureReconcilerConfig. - Implements
PlannerClient.get_pressure()and updatesPressureReconcilercreate loop to support request-mode polling; updates unit tests and changelog accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_factories.py | Asserts planner_pressure_mode is included in generated application config. |
| tests/unit/test_charm_state.py | Adds validation test for invalid mode; asserts valid mode is stored in CharmConfig. |
| tests/unit/factories.py | Sets default planner-pressure-mode in mock charm config. |
| tests/unit/conftest.py | Updates charm state fixture to include planner_pressure_mode. |
| src/factories.py | Plumbs state.charm_config.planner_pressure_mode into ApplicationConfiguration. |
| src/charm_state.py | Adds planner-pressure-mode config name, type, default, and validation/parsing. |
| github-runner-manager/tests/unit/manager/test_pressure_reconciler.py | Updates fake planner for request-mode support and adds request-mode create loop test. |
| github-runner-manager/src/github_runner_manager/planner_client.py | Adds get_pressure() one-shot pressure fetch method. |
| github-runner-manager/src/github_runner_manager/manager/pressure_reconciler.py | Adds planner_pressure_mode to config and implements request-mode polling path in create loop. |
| github-runner-manager/src/github_runner_manager/configuration/base.py | Extends ApplicationConfiguration with planner_pressure_mode. |
| docs/changelog.md | Documents the new config option and reconciler behavior change. |
| charmcraft.yaml | Adds planner-pressure-mode charm config option with default and description. |
| data = response.json() | ||
| if not isinstance(data, dict) or name not in data: | ||
| raise PlannerApiError(f"Unexpected pressure response payload: {data}") | ||
|
|
| 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") |
| return | ||
| self._handle_create_runners(update.pressure) | ||
| self._stop.wait(5) | ||
| else: |
yhaliaw
left a comment
yhaliaw
left a comment
There was a problem hiding this comment.
Approved with minor changes needed
🤝 Human review with AI assistance.
| response = self._session.get( | ||
| url, | ||
| headers={"Authorization": f"Bearer {self._config.token}"}, | ||
| timeout=self._config.timeout, |
There was a problem hiding this comment.
get_pressure() uses self._config.timeout, whose default is 5 * 60 (300s, line 42) — a value chosen for long-lived streaming connections, not one-shot polls. build_pressure_reconciler constructs PlannerConfiguration without overriding it, so request mode inherits the streaming timeout unchanged.
A slow or hung planner can block the create loop for up to 300 seconds per poll attempt, compounded by the session's urllib3.Retry(total=3, backoff_factor=0.3) on 5xx responses. That makes the effective poll cadence wildly variable under exactly the flaky-network conditions this PR exists to handle, and delays the fallback-to-min_pressure behavior meant to keep runners flowing when the planner is unreachable.
Pass a short explicit timeout for the one-shot request path (a few seconds, consistent with the 5-second poll interval) — either a separate request_timeout field on PlannerConfiguration or an explicit argument at the get_pressure call site.
🤖 AI-assisted
| PLANNER_PRESSURE_MODE_CONFIG_NAME, | ||
| PLANNER_INTEGRATION_NAME, |
There was a problem hiding this comment.
PLANNER_PRESSURE_MODE_CONFIG_NAME is imported before PLANNER_INTEGRATION_NAME, breaking the alphabetical ordering isort enforces here (every other name in this block is correctly ordered).
The lint tox env runs isort --check-only --diff, so this will fail CI lint as-is.
| PLANNER_PRESSURE_MODE_CONFIG_NAME, | |
| PLANNER_INTEGRATION_NAME, | |
| PLANNER_INTEGRATION_NAME, | |
| PLANNER_PRESSURE_MODE_CONFIG_NAME, |
🤖 AI-assisted
What this PR does
Introduce non streaming mode for pressure
Why we need it
Flaky network infrastructure will always fail on pressure stream
Checklist
docs/changelog.mdwith user-relevant changesterraform fmtpasses andtflintreports no errorsgithub-runner-manager/pyproject.toml.