Repository navigation
fix(garm): keep a departing configurator's provider until its scaleset is gone (ISD-7262) - #439
Conversation
e8c06cb to
3064fde
Compare
…t is gone Removing a garm-configurator relation built GARM_PROVIDERS_JSON from the relations that remained, so the removed configurator's provider was dropped and GARM replanned without it before the drain started. GARM fixes its provider map per process, so the orphaned scaleset then had no scaleset worker to purge its instances and no provider worker to tear a VM down, while GARM refuses to delete a runner in the terminal `deleted` status or a scaleset that still lists runners. The teardown could never finish. restart() now runs the provider lifecycle in one pass: read the recorded set, add every provider the relations publish, start GARM with it, reconcile scalesets, then trim and re-record what no relation and no surviving scaleset names. The record is the charm's only knowledge of a departed configurator's credentials - Juju hides its unit from relation-departed onwards - so a record that cannot be read or written raises out of the hook instead of yielding a guessed set: publishing from an incomplete record, or running a set the record does not hold, is exactly what strands a scaleset. A reconcile pass that cannot establish which scalesets GARM holds trims nothing. The trim takes effect at the following replan, so a retirement does not restart GARM twice. Refs: ISD-7262
3064fde to
f8613b4
Compare
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
The design is sound: keeping a departed configurator's provider published until its last scaleset has drained is the right fix, and failing safe on an unknown scaleset state is the right default. One gap needs to be closed before merge.
To address before merge
- Removing the last configurator bypasses the record. When no relation publishes provider data,
restart()returns before_published_providersreads the record. The drain then only works while the old Pebble layer survives: if the pod is recreated mid-drain,pebble-readyre-runsrestart(), which returns early again, so no provider is republished and the scaleset deadlocks as before. The record is also never trimmed, so the departed provider's credentials stay forever and are republished as soon as a new configurator relates. Suggest taking the early return only when there are no relations and the record is empty; otherwise publish the recorded set and trim as usual (with nothing related, the existing trim keeps the provider while its scaleset survives and drops it afterwards). Please add a unit test for this path. - Upgrade note. Please add a short note for operators (release notes or the upgrade docs) that deployments already stuck do not recover on upgrade, and are recovered by relating the same configurator again, as the description explains.
Suggestions
- Consider running the GARM E2E test from this branch before merge: the unit tests mock GARM, so they cannot show a real GARM purging the runners and deleting the scaleset once the provider is retained. Your call.
- Each change to the set writes a new revision of
garm-providers, and old revisions (holding old passwords) are never removed. Consider pruning superseded revisions afterset_content. - The hard fail on an unreadable/unwritable record is fine as is — a visible error beats a silently stranded scaleset.
Nits
get_content(refresh=True)on the charm's owngarm-providerssecret: the repo convention for owned secrets is plainget_content(), orpeek_content()to read the latest revision without changing tracking (which also covers non-leader units)._PROVIDER_RECORD_FIELDSduplicatesgarm_entrypoint._PROVIDER_REQUIRED_FIELDS, which lives in the samesrc/and could be imported, so the two can't drift._referenced_providersreturnsNoneas soon as one surviving scaleset has no provider name; if that scaleset persists, trimming is disabled indefinitely with only a per-hook warning. Fine as the safe default, but worth an explicit note or a clearer log line that says trimming stays off until it's gone.ISD-7262is referenced in code comments, the secret description andAGENTS.md. The tracker isn't public, so outside readers can't follow it; the surrounding text already explains the concern, so the id can be dropped from shipped text (it's fine in the PR title/commit).- Several docstrings carry design rationale and the bug's history (
ProviderRecordError,restart(),_read_provider_record,_trimmed_providers). Consider keeping docstrings to the interface and moving the rationale into one concise comment at the point of use (or theAGENTS.mdentry, which already has it). _by_provider_nameis a one-line wrapper aroundsorted; inlining it at its two call sites reads just as clearly.
AI-generated message.
- Publish the recorded provider set even when no relation publishes provider data, so a pod replaced mid-drain still starts GARM with the providers that drain needs. The waiting return now needs no related providers AND an empty record; a configurator that is related but silent stays a waiting state, so the sweep cannot prune the scalesets it is about to describe. - Read the record with peek_content() (latest revision, no tracking) and remove the revision each write supersedes: the leader is the only writer and every reader peeks, so no unit depends on an older revision. - Import PROVIDER_REQUIRED_FIELDS from the entrypoint instead of duplicating it, inline the one-line ordering helper, and keep docstrings to the interface. - Say in the log that a scaleset naming no provider keeps trimming off until it is gone, and drop the internal tracker id from shipped text. - Release note: stuck deployments do not recover on upgrade; relate the same configurator again to republish the provider and resume the drain. - Tests: the last-configurator path across a replaced pod, and the record being emptied once its last scaleset is gone.
|
Thanks for the review — all points are addressed in 1. Last configurator bypassing the record — fixed, with one extra condition Agreed, this was a real gap: the early return skipped the record entirely, so a replaced pod re-published nothing, and the record was never trimmed. The gate is now
Two tests cover it: 2. Upgrade note — added to 3. E2E — dispatched on this branch: https://github.com/canonical/github-runner-operators/actions/runs/39718034089. I will report the result. 4. Pruning superseded revisions — done, now that the single-writer assumption holds. The leader is the only writer and every reader uses 5–10. Nits — all applied:
Unit tests: 460 pass (with |
Replaces the copied-then-updated mapping with one literal, so the published set is a new value rather than an edited one.
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
Thanks for the thorough follow-up. Both earlier blocking points are fixed (the last-configurator path now publishes and trims the record, and the changelog has the upgrade note), and the GARM E2E run on d147d58 passed. One thing is left before merge.
To address before merge
- The PR description describes a different change. The body covers a
garm-configuratorsecurity-groupoption (ISD-7035), not the provider retention this PR implements. Please replace it with a summary of this fix: the recorded provider set and when a provider leaves it, the test plan (unit tests and the E2E run), and the upgrade note for deployments that are already stuck. Reviewers and release-notes readers rely on it.
Nits
docs/changelog.md: Vale's spellcheck rejects "configurators", which fails the docs check. Rephrase it (e.g. "configurator applications") or add it todocs/.custom_wordlist.txt._by_provider_nameincharm.pyis still defined, but nothing calls it since it was inlined. It can be deleted._reconcile_referencedintest_scaleset_reconciler.pywas added, but no test uses it. SoScalesetReconciler._referenced_providershas no direct test: returningNonefor a surviving scaleset with no provider name, excluding scalesets deleted in the pass, and counting providers from the desired specs. The charm tests mock the reconciler's return value, so they don't cover these paths.
AI-generated message.
Review follow-ups: - Delete the `_by_provider_name` helper left behind after it was inlined. - Changelog: say "configurator applications" so Vale's spellcheck accepts the plural; only the singular is in the custom word list. - Test `_referenced_providers` directly: a desired spec's provider counts before its scaleset exists, a scaleset deleted in the pass drops out, a scaleset whose delete GARM refused keeps its provider, and a surviving scaleset with no provider name yields None. The fake client now forwards `provider_name`.
|
GARM E2E: passed — run 39718034089: What it covers, precisely: one Why that matters for this change: every reconcile now reads the provider record, publishes it, reconciles, trims and writes it back, so a defect in that path shows up here — a read failure would error the hook, and a provider dropped while its scaleset is live would stop the scaleset being created, and the dispatched job would never run. Green means the new lifecycle does not regress the end-to-end flow against real GARM, Juju and OpenStack. What it does not cover: the topology of the bug itself, which needs two configurators, one removed, with its scaleset still draining. The suite deploys one configurator and never removes it. That invariant is covered by the unit tests added here ( |
|
Thanks, the follow-ups in 7be9b8f look good and CI is green. The one thing still blocking merge is the PR description: it still describes the The two-configurator removal scenario for the E2E suite sounds like a good follow-up. AI-generated message. |
|
Replaced. The description had the
Nothing else changed in |
|
@cbartz One more constraint before merge: this will not be merged until the local teardown test is done, so please hold approval-triggered merge until then. Concretely, that test is the deletion pipeline itself, which neither the unit tests nor the E2E exercise end to end: two configurators with distinct scale set names and I will report the result here (pass, or the failure with logs) before asking for merge. If it turns up a defect, it lands as a follow-up commit on this branch, not a new PR. |
cbartz
left a comment
cbartz
left a comment
There was a problem hiding this comment.
LGTM. The provider-retention fix, the follow-ups and the rewritten description all look good, and the E2E run passed.
Approving now, with your hold respected: please don't merge until you've posted the local two-configurator teardown result. If it turns up a defect, the fix commit on this branch will get another look.
AI-generated message.
…early restart() had two gates that skipped the publish and the reconcile when nothing was related, which pinned the last published provider set in GARM's environment forever once the last configurator left. The chain already iterates the desired state, so an empty one reconciles to zero: publish record U related (possibly empty), restart the workload with it, then reconcile credentials, entities and scalesets against that same state. One condition is left, because every reconciler deletes what it is not told to keep: a configurator relation that has not published yet must not be read as "nothing is desired", or it would lose the credentials, entities and scalesets it is about to describe. Only a relation that has published, or no configurator at all, is a complete desired state. The unit still reports "Waiting for garm-configurator relation" while nothing is related, set after the reconcile so its Active cannot bury it.
…iders ScalesetReconciler.reconcile() now returns the scalesets GARM still holds after the pass, not a derived set of provider names. Reporting observed state keeps the policy in the charm: it maps provider_name off the objects it is given and decides what an unmappable scaleset means, instead of the reconciler deciding which providers are safe to drop. It also removes a redundant input. The reconciler used to add every desired spec's provider to the returned set, but a spec's provider is by definition one the relations publish, which the trim already keeps — only the surviving scalesets say anything the relations do not. The unmappable case moves with it: a scaleset naming no provider is returned like any other, and _trimmed_providers keeps every provider when it sees one, with the warning naming the scalesets.
The reconciler now returns the state of the world after the pass — what GARM listed and kept, plus what this pass created, minus what it deleted — instead of the survivors of the pre-pass observation. A caller never has to reason about which half of a pass a scaleset came from, and a scaleset created in the pass is no longer invisible to it. _create() returns the scaleset GARM answers with, and the pass accumulates those so the live set can be assembled in one place. `_live_scalesets()` replaces `_surviving_scalesets()`. The fake client now returns a scaleset from its create methods, as the real one does; a fake that returned None hid the created half of the live set.
The provider config travelled as dict[str, str] with string keys repeated across charm.py — built from relation data, merged, trimmed, serialised and parsed back. ProviderConfig (charm_state.py, per the relation-derived value object rule) names the fields, keeps the wire contract in one place via as_dict(), and makes the merge, trim and log lines read as config.unit_name instead of config["unit_name"]. The published form is unchanged — same keys, same JSON — so the entrypoint and the record keep reading what they read before. A test pins the dataclass fields against the entrypoint's required-field tuple, so a field added to one without the other fails here rather than in a workload that cannot start.
A live scaleset that names no provider made the trim give up and keep every provider. That protects nothing: GARM resolves a scaleset's provider from the same field, so an unattributable row cannot become attributable because a provider stays published — it simply cannot be serviced either way. What it did do is disable trimming for the whole application: the record never shrank and a departed configurator's credentials stayed published for as long as the corrupt row existed. The unmapped scaleset is now reported as an orphan and skipped, and the trim carries on with the providers that can be accounted for — the ones the relations publish and the ones live scalesets name. The only remaining reason to keep everything is an unknown observation (the reconcile pass could not run).
_reconcile_runners reconciled far more than runners: the controller URLs GARM needs before it will answer any operational call, the garm-agent binaries, the GitHub App credentials, the org/repo entities, the runner template, and finally the scalesets that own the runners. _reconcile_garm_state names the object it brings into line — GARM's state, as described by CharmState — and matches the vocabulary the rest of the module already uses for this pass.
restart()'s tail read as two conditions in a row — one deciding whether to reconcile, one deciding what to report — with an else branch holding a single log line. Guarding the abnormal case and returning makes the happy path read top to bottom: publish, reconcile, record, report. The waiting status had to be reported from both exits, so it moves into `_report_waiting_for_configurator()`, which also keeps the message in one place instead of two inline statuses with the same text.
|
DO NOT MERGE yet, I've pushed additional commits to refactor - but GitHub has not yet reconciled the PR to reflect this. Manual tests to relate 2 configurators and remove-relation of the 2 configurators have worked successfully. |
What this fixes
Removing one
garm-configuratorrelation could never finish its teardown.restart()rebuiltGARM_PROVIDERS_JSONfrom the relations that remained, so the departing configurator's provider was dropped and GARM restarted without it before the drain began. GARM fixes its provider map per process, so the orphaned scale set was left with no worker to purge its terminaldeletedrunner row and no worker to delete its VM — while GARM refuses to delete either the runner or the scale set. It retried untilDRAIN_DEADLINE(5 days) logged the unrecoverable error (ISD-7262).The provider set is now a record with a lifecycle instead of a projection of the relations.
Provider lifecycle
restart()runs that once per hook: read the record, add the relations' providers, publish, reconcile scale sets, trim and save. Relation data wins for a provider that is still related, so a rotated password reaches GARM. A reconcile pass that cannot establish which scale sets GARM holds trims nothing, because "nothing references it" cannot be concluded from "could not tell". A record that cannot be read or written fails the hook, so the unit goes to error state rather than publishing a guessed set.Test plan
Unit tests —
cd charms/garm && PYTHONPATH=lib:src:tests .venv/bin/python -m pytest tests/unit→ 463 passed. The lifecycle tests cover: the provider kept while its scale set survives, trimmed once it is gone (and published one hook later), unknown state keeps everything, the record survives a replaced pod, relation data beats recorded credentials, an unrebuildable provider is logged, the last-configurator path keeps the drain alive, and both record failures fail the hook. With the trim or the record read stubbed out, those tests fail — they pin the fix, not the plumbing.GARM E2E — run 39718034089: success, 38m36s, deploying the charms built from this branch (
gh workflow run garm_e2e.yaml --ref fix/isd-7262-retain-provider). It proves the new path against real GARM, Juju and OpenStack: every reconcile reads, publishes and writes the record, and a provider dropped too early would stop the scale set being created, so the dispatched job would never run.What the E2E does not cover — the topology of the bug. The suite deploys one configurator and never removes it, so the retention window is not entered. Manual equivalent: two configurators with distinct scale set names and
min-idle-runner: 1, thenjuju remove-applicationon one and watch the order — runners deleted, VM deleted, scale set deleted, and only then the provider leavingGARM_PROVIDERS_JSON.Upgrade note
Deployments already stuck before this release do not recover on upgrade: the removed configurator's credentials are no longer available to the charm. Relate the same configurator application again — its unit name reproduces the missing provider name — so the provider is republished and the drain resumes. A scale set whose runner is stuck in the
deletedstatus can also be cleared by hand. Also noted indocs/changelog.md.Notes on the ticket's proposal
relation-departedonwards. The record is where they come from.relation-departedis the first hook that strips the provider, notrelation-broken. The fix sits inrestart(), which both hooks call.Active" is already true while deadlocked, so it cannot distinguish fixed from broken. Assert the publishedGARM_PROVIDERS_JSONand a successful scale set delete instead.Follow-ups
juju statusstaysActive. Agreed with the reviewer, not in this PR.scaleset_reconciler._disable()writes the scale set on every pass; if a future GARM bumpsupdated_aton a no-op update, the drain deadline would never elapse. Out of scope here.