Feat/scaleset label change - #306
Conversation
`tox -e fmt` runs `ruff format` over `src`, which includes the Python snippet in the generated `garm_client_README.md`. The snippet had never been formatted, so `tox -e lint` (`ruff format --check`) failed on it regardless of what a change touched. This is generated output, so `scripts/generate_client.sh` will undo it on the next client regeneration — running `tox -e fmt` afterwards restores it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Labels are immutable once a GitHub scale set exists, so a label change in the garm-configurator relation data only logged a warning telling the operator to remove and re-add the relation. A label change is now applied by replacement. The live scaleset name carries a hash of its labels, so the name a spec should have is a pure function of the spec: every reconcile re-derives which live scaleset is current and which are replaced predecessors, with no charm-side state to persist, which is what makes the flow restart-safe and idempotent. The changeover is staged across reconciles and never blocks a hook: - the replacement is created first, so every label carried by both generations is served throughout; - the old scaleset is disabled only once the replacement is observed live, which closes its listener session and stops it receiving new jobs while runners already mid-job are left to finish; - it is deleted only after its runner count reaches zero. A scaleset draining past DRAIN_DEADLINE (7h, beyond GitHub's 6h job cap) holds a runner that is stuck rather than busy, so the delete is attempted regardless; GARM rejects the call while runners are genuinely active, so this cannot cut a job short but does stop a permanently faulted instance pinning a dead scaleset on GitHub. Progress is reported as a MaintenanceStatus naming the scaleset the operator configured and the phase it is in (creating replacement / draining N runners / awaiting deletion), returning to active once the replacement is serving and the old scaleset is gone. Also hardens the surrounding pass: a GarmApiError on one spec no longer aborts the others or the orphan sweep (it is re-raised after the pass, so the charm still reports the sync failed), a connection error still aborts immediately, and duplicate desired names are dropped rather than letting two specs retire each other's scaleset forever. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reconciler uses "spec" for a ScalesetSpec throughout, so "the desired specs" read as charm-side desired state rather than the GARM extra_specs the sentence is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the garm charm’s scaleset reconciliation logic to support GitHub scaleset label changes via a blue/green-style replacement flow (create replacement, hand over, drain, delete), and documents the design decision via a new ADR.
Changes:
- Implement label-hash-based “generation” naming for scalesets and a multi-reconcile replacement/drain/delete workflow in the scaleset reconciler.
- Surface in-progress replacement state to Juju unit status and update integration/unit tests to assert the new behavior.
- Add documentation (ADR + changelog) describing the rationale, operational behavior, and constraints.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/changelog.md | Adds a user-facing changelog entry describing automatic scaleset label-change handling. |
| docs/adr/003_scaleset_label_change_by_replacement.md | New ADR documenting the replacement-based approach and its operational constraints. |
| charms/tests/integration/test_garm.py | Updates integration assertions/helpers to handle label-hashed live scaleset names. |
| charms/garm/tests/unit/test_scaleset_reconciler.py | Adds extensive unit coverage for replacement/drain semantics and edge cases. |
| charms/garm/tests/unit/test_garm_api.py | Adds unit tests for the new instances-listing client call used to gate deletion. |
| charms/garm/tests/unit/test_charm.py | Adds unit coverage for reporting replacement progress in Juju status. |
| charms/garm/src/scaleset_reconciler.py | Core implementation: generation naming, family resolution, replacement/drain orchestration, and defensive deletion gating. |
| charms/garm/src/garm_client_README.md | Updates the generated client README snippet (currently contains a garbled line in the snippet). |
| charms/garm/src/garm_api.py | Adds list_scaleset_instances wrapper around the Instances API for runner counting. |
| charms/garm/src/charm.py | Uses reconciler progress to emit maintenance status while replacements are in flight. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| digest = hashlib.sha256(logical_name.encode("utf-8")).hexdigest()[:LABEL_HASH_LENGTH] | ||
| return f"{logical_name[: limit - LABEL_HASH_LENGTH - 1]}-{digest}" |
There was a problem hiding this comment.
i understand the algo, just a question: if the goal of hashing is to avoid collision when the truncated name share the same prefix, then why not just return the hash itself for the entirety of the limit length
There was a problem hiding this comment.
There's a name length limit i believe on GARM/GitHub side
There was a problem hiding this comment.
right now we only have 10 chars for a scaleset name , until the GARM maintainer fixes it, see https://garm-hq.slack.com/archives/C05DK44A664/p1783599290001029 for the rationale
| if observed_name == logical_name: | ||
| return True |
There was a problem hiding this comment.
what about for the scalesets created before label-hashed naming that carries the unsuffixed name n also <= limit length? it will fall into this and be marked as true (shld it be false bc it's not generated?)
There was a problem hiding this comment.
There is a code path for the unsuffixed legacy names.
| The specs with duplicate names removed, first occurrence winning. Two | ||
| specs sharing a name would each own the other's live scaleset and retire | ||
| it on every reconcile, so they would replace each other forever. |
There was a problem hiding this comment.
forever as in forever? is there a way to just have one spec per a unique name?
There was a problem hiding this comment.
I think this should be caused by multiple deployments using the same scaleset name. I do not think it can be avoided via charm code. In deployments, we should not be doing that.
| its deletion, which is a distinct thing to be waiting on. | ||
| """ | ||
| if not progress.handed_over: | ||
| return "creating replacement" |
There was a problem hiding this comment.
Would it help separate the phases to be a constant or an enum?
There was a problem hiding this comment.
Changed to constants.
| self._reconcile_one( | ||
| spec, providers, observed, templates, families.get(spec.name, []) | ||
| ) |
There was a problem hiding this comment.
Just out of curiosity, how long is this operation expected to take? I'm asking because if it's a long running operation, if there would be a way to do fire & forget?
| create_params = self._to_create_params(spec) | ||
| create_params = self._to_create_params(spec, active_name) | ||
| except Exception as exc: | ||
| logger.warning("Skipping scaleset %s: spec validation failed: %s", spec.name, exc) |
There was a problem hiding this comment.
Should this be an error log? Warning may go unnoticed it seems
There was a problem hiding this comment.
Changed to error level.
There was a problem hiding this comment.
Just OOC, i'm guessing we won't test blue/green deployment since its a huge costly test?
There was a problem hiding this comment.
It would be a long test. Maybe in the release gate?
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
Read the ADR first as suggested — the design is sound and the naming/family logic is well covered by the unit tests. Two things I'd want resolved before this lands: a wire-format issue that I believe stops the changeover from ever completing, and one claim in the ADR that the cited GARM source doesn't actually establish. Details inline.
One non-blocking aside that didn't fit on a diff line — _needs_update compares observed.min_idle_runners != spec.min_idle_runners, and MinIdleRunners is omitempty in GARM (params/params.go:637), so a scaleset configured with min_idle_runners: 0 reads back as None and None != 0 makes the check true on every pass — a no-op update call each reconcile, forever. Pre-existing, not from this PR, and the same root cause as the first inline comment. Fine to split out.
This review was generated with AI assistance.
GARM tags ScaleSet.Enabled `omitempty` with no custom marshaller, so a disabled scaleset comes back with no `enabled` key and the generated client reads it as None. `old.enabled is not False` was therefore never false in production: the drain re-entered _retire on every pass and never reached the runner-count check or the delete, leaving the predecessor disabled forever and registered on GitHub. Compare truthily instead. The unit fixture set `enabled=False`, a shape the API cannot return, which is why the suite stayed green against it; it now drops the key like the wire does. The same comparison in the integration helper _find_scaleset counted a draining predecessor as still serving, and is fixed alongside. Also from review: - Re-raise GarmConnectionError from the three drain helpers. It subclasses GarmApiError, so the containing handlers reported drain progress while GARM was down, diverging from reconcile()'s documented behaviour. - Report the drain as active rather than maintenance. Every label is served throughout, so blocking `juju wait-for` and integration tests on hours of healthy background convergence is the wrong trade. - Cover the changeover end to end in the integration suite: no VMs are involved, so the drain completes as fast as the charm reconciles. - Log a malformed spec at error level; unlike the provider/entity gates it will not resolve on its own. - Name the replacement phases; apply the review suggestion for `claimed`. - ADR: claim only what the GARM source establishes about the listener session, and record the GitHub-side job-assignment question as open. Note that both generations carry min_idle_runners for the whole drain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
charms/garm/src/charm.py:156
- The
_scaleset_replacement_statusdocstring says each scaleset is “named by the logical name” and that live names are only logged by the reconciler, but the status message actually includes the livereplacement_name(hash-suffixed) vialogical_name -> replacement_name. Updating the docstring will prevent confusion when operators see hashed names in Juju status.
A status message naming at most two scalesets, so it stays readable when
several are replaced at once. Each is named by the logical name the operator
configured — the live names carry a label hash the operator never chose — and
the live names are logged in full by the reconciler.
| return {} | ||
|
|
||
|
|
||
| def _scaleset_replacement_phase(progress: ScalesetProgress) -> str: |
There was a problem hiding this comment.
nit: we have the step down rule (callees should be below caller fcts)
| digest = hashlib.sha256(logical_name.encode("utf-8")).hexdigest()[:LABEL_HASH_LENGTH] | ||
| return f"{logical_name[: limit - LABEL_HASH_LENGTH - 1]}-{digest}" |
There was a problem hiding this comment.
right now we only have 10 chars for a scaleset name , until the GARM maintainer fixes it, see https://garm-hq.slack.com/archives/C05DK44A664/p1783599290001029 for the rationale
|
|
||
| # GitHub rejects over-long scale set names; keep the generated name inside a | ||
| # conservative bound by truncating the operator-supplied part, never the hash. | ||
| MAX_SCALESET_NAME_LENGTH = 64 |
There was a problem hiding this comment.
Can you double check if we can really use this amount of chars, I remember discussing with the maintainer of GARM in https://garm-hq.slack.com/archives/C05DK44A664/p1783599290001029 , and right now the limit are 10 chars (he wanted to fix that soon).
There was a problem hiding this comment.
right. I have not yet sent a PR for this. Just finished a 10 mo project and will be on vacation for a couple of weeks. Apologies for the delay on this. Recovering from a bit of burnout. I have not forgotten about this.
There was a problem hiding this comment.
it seems we don't follow step-down rule (callees below caller) . This should be mentioned in our contribution guide. Might be worth to update the AGENTS.md with that explicitly
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
One design question on the naming path, inline — not blocking, and it's a scope call rather than a defect.
Separately, and worth doing either way (no line to anchor this on, the file didn't change there): _existing_scaleset defaults name="my-scaleset" while _spec() names the spec my-scaleset too. The target name my-scaleset-<hash> is therefore never in observed, so those tests reach the scaleset through _resolve_active_name's legacy-adoption branch. Roughly 14 update/template tests verify their assertions against the compat path rather than the hashed-name path they look like they're exercising. Repointing that default to target_scaleset_name("my-scaleset", []) moves them onto the real path and they pass unchanged.
This review was generated with AI assistance.
| return datetime.datetime.now(datetime.timezone.utc) - retired_at > DRAIN_DEADLINE | ||
|
|
||
|
|
||
| def _resolve_active_name(spec: ScalesetSpec, observed: dict[str, ScaleSet]) -> str: |
There was a problem hiding this comment.
_resolve_active_name is the only place where "which scaleset is ours" depends on what's live in GARM rather than being a pure function of (spec.name, spec.labels). Everything downstream — families, the orphan fence, the retire state machine — reasons about names, and this is the one name that can be something other than what the spec computes.
Given our GARM deployment is for testing and the charm publishes to edge only, is that branch worth keeping?
To be honest about the cost, it is not a complexity win: reconcile stays at 10 and no surviving function's cyclomatic complexity changes. What goes is 39 lines and three concepts — the function itself, _is_family_member's "…and the bare logical name counts too" special case, and _resolve_families' targets/reserved/others entirely. That last one surprised me, so I checked: reserved only guards against a bare logical name being read as a sibling's generation. Hash-suffixed siblings cannot collide (my-scaleset-1a2b3c4d's generations are my-scaleset-1a2b3c4d-<hash8>, which cannot match ^my-scaleset-[0-9a-f]{8}$), and test_a_similarly_named_scaleset_is_not_claimed_as_a_generation still passes without it. 269 of 270 unit tests pass unchanged; the one failure is the adoption test itself.
No manual redeploy needed either: the un-suffixed scaleset falls to the orphan sweep, and since the spec loop runs before the sweep the replacement is created first in the same pass.
Happy to leave it if you'd rather keep the upgrade path — mainly want the trade-off recorded.
This review was generated with AI assistance.
|
Convert this PR to draft as temp measure, since:
|
A refused cutover left _retire silent about it, so the predecessor stayed enabled while the status reported "draining N runners" — a changeover that is not advancing, and is running both generations' min_idle_runners, read as one quietly making progress. _retire now returns whether the scaleset is disabled, and the unfinished hand-over states are named: ScalesetProgress carries a Handover enum (PENDING/FAILED/DONE) rather than a handed_over flag, and FAILED surfaces as "retiring predecessor". Also from review: - Collapse a scaleset's generations into one status entry. A label change during an earlier drain leaves several predecessors handing over to the same replacement; to the operator that is one changeover, so it is named once — with the runner counts summed and the least advanced phase — and cannot repeat itself and push the other scalesets behind "+N more". - Keep an orphan's runner template when GARM refuses the delete. The scaleset still exists and its runners were built from that template, and the next reconcile's retry needs it; _delete_orphaned now reports whether GARM still holds it. - Re-raise GarmConnectionError from the orphan sweep, as the drain helpers already do: an outage is not a per-scaleset failure and must not read as a sweep that found nothing left to do. - Warn on a duplicate desired name only when the specs conflict. The scaleset config is app-level, so every configurator unit's databag yields the same spec and a multi-unit configurator would warn on every hook. - Log the two id-less cases (_retire, _delete_orphaned) instead of returning silently. - ADR: record why a disabled scaleset's updated_at is the moment it was disabled, which is what the drain deadline measures from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What this PR does
AI summary
Copilot kept crashing generating summary...
Why we need it
This closes the issue with changing labels for scaleset.
Checklist
CONTRIBUTING.mdhas been updated upon changes to the contribution/development process (e.g. changes to the way tests are run)docs/changelog.mdwith user-relevant changes(e.g., in
.github/workflows/integration_tests.yaml, ensure themoduleslist is correct)terraform fmtpasses andtflintreports no errorsAGENTS.md.copilot-collections.yamlor.github/instructions/: I re-checked whether theAGENTS.md"12-factor divergences" guidance still matches the upstream copilot-collections guidance