fix(controller): apply spec changes even when Forgejo is unreachable #54
No reviewers
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
brooktrails/forgejo-runner-operator!54
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/reconcile-order-forgejo"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A revoked Forgejo admin token froze every pool's spec.
trusted-gllm-goran a two-month-old image while its RunnerPool carried a new one, and its status saidDeployment rollout is progressing.What happened
reconcileKubernetesenabled repo Actions and minted a registration token before building the Deployment, and returned early when either failed:The admin token was revoked on 2026-08-05. Every repo-scoped pool has returned at that call since -- so no pool has reached its Deployment reconcile in two months.
trusted-gllm-gowas simply the first one whose spec changed afterwards (a new runner image for gllm's Go 1.27 bump).Two status bugs hid it:
setConditionreplaced a condition only when itsStatusflipped. A pool that started failing for a new reason kept reporting the old one, with the original timestamp -- hence aStalledcondition still describing a July rollout.observedGeneration, so a pool at generation 3 whose Deployment was built from generation 1 looked exactly like one fully applied.The change
Readyrather than discarding spec changes already applied. A broken token now costs new runner registrations, not every future spec change.The Deployment reads the registration token through a
secretKeyRef, never an embedded value, which is what makes the order safe. A brand-new pool gets its Secret moments later in the same reconcile, and kubelet retries the pod until it exists.setConditionalways writes reason, message andobservedGeneration;LastTransitionTimemoves only on a real status change.ReadyrecordsobservedGeneration, so a spec that has not been applied is visible inkubectl get runnerpool -o yamland alertable.ActiveRunners/ReadyRunners-- the runners are up and serving jobs.Tests
TestReconcileKubernetes_AppliesSpecWhenForgejoFails: every Forgejo endpoint 401s; the Deployment still moves to the new image, andReadyreportsEnableActionsErrorat the current generation.TestSetConditionUpdatesReasonAndMessage: reason/message/generation update on an unchanged status whileLastTransitionTimeholds; a status flip moves it.TestReconcileKubernetes_ImageChangeUpdatesDeploymentstill covers the healthy path.Rolling out
publish.ymlbuilds and pushes the controller image on merge; infra pinsforgejo-runner-controller:0.8.3and needs bumping to the new version. The admin token is deliberately still revoked, so the first reconcile after rollout is the live test:trusted-gllm-goshould move toforgejo-runner-go:12-e68242ewhileReadystays False withEnableActionsError.Remote backend
The same ordering was in
reconcileRemoteand is fixed here too:EnableRepoActionsran before any VM work, so a revoked token stranded scale-down (excess VMs kept running) andsetErrorStatusreported zero runners while VMs were up.Provisioning genuinely needs Forgejo -- each VM registers with its own token -- so a failure now skips scale-up for that pass and nothing else: deprovisioning and the VM-derived status still run, with the failure recorded as a degraded status over counts that reflect what is running. The provisioning loop already broke on a token error, so the early return bought nothing.
RemoteBackendErrorandListVMsErrorstill return early: without a VM list there is no state to reconcile, and those are provisioner-side failures rather than the Forgejo-token class this change is about.No test covers the remote path: there are none today, and
NewRemoteBackendwants a CA secret and a gRPC provisioner, which is a bigger fixture than this change should drag in.🤖 Generated with Claude Code
reconcileKubernetes enabled repo Actions and minted a registration token before it built the Deployment, and returned early when either failed. So a Forgejo outage or a revoked admin token froze every pool's spec: the Deployment kept running whatever image it was created with, and nothing said otherwise. Seen on trusted-gllm-go, whose pod ran a two-month-old image while its spec carried a new one. The admin token had been revoked on 2026-08-05, and every repo-scoped pool had been returning at the EnableRepoActions call ever since. The cluster-side objects come from the spec alone, so they are reconciled first: ConfigMap, PVC, Deployment, status. Forgejo-dependent work runs after, and reports failures in the Ready condition instead of discarding the spec changes already applied. A broken token now costs new runner registrations, not every future spec change. The Deployment reads the registration token through a secretKeyRef, never an embedded value, which is what makes the order safe; a brand-new pool gets the Secret moments later in the same reconcile, and kubelet retries the pod until it exists. Two status bugs kept that invisible: - setCondition replaced a condition only when its Status flipped, so a pool that began failing for a new reason kept reporting the old one with its original timestamp. Reason, message and observedGeneration are now always written; LastTransitionTime still moves only on a real status change. - Conditions carried no observedGeneration, so a pool at generation 3 whose Deployment was built from generation 1 looked identical to one fully applied. Ready now records the generation it was evaluated against. Forgejo failures after the Deployment is applied no longer zero ActiveRunners and ReadyRunners: the runners are up and serving jobs, and claiming otherwise sent readers looking for a rollout that was not happening. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Automated review by pr-reviewer v0.47.3 | Safety Check | Claude Sonnet 5 | tracking id
r-ab9312-618d65This is an AI-generated review and may contain mistakes.
Status: ✅ Completed
✅ Verdict: LGTM — The flagged "secret" is a placeholder string in a test against a local mock server, and the reordering logic is sound and well-tested with no signs of injected credentials or breaking changes.
Reviewed the diff carefully given the "possible secret detected" flag from the automated scan.
forgejo.NewClient(forgejoSrv.URL, "revoked")in the new test file — this is a literal placeholder string used against a localhttptest.NewServerthat always returns 401, not a real credential. No actual secret is present.secretKeyRef, so reordering doesn't embed stale/missing tokens into the Deployment spec itself.setConditionchange looks correct: always updates reason/message/observedGeneration, only holdsLastTransitionTimewhen status is unchanged.setErrorStatusvssetDegradedStatussplit is a sensible way to avoid zeroing out active/ready counts when runners are healthy but Forgejo calls fail.Nothing here looks like an accidentally committed credential, an injection vector, or a change that would break the service — the fallback path (existing Deployment/replicas untouched on Forgejo failure) is safer than before, not more dangerous.
Automated review by pr-reviewer v0.47.3 | Full Review | Claude Sonnet 5 | tracking id
r-ab93b1-eefdbaThis is an AI-generated review and may contain mistakes.
Status: ✅ Completed
⚠️ Verdict: Request Changes — The Kubernetes-backend reordering and condition-tracking fixes are correct, well-tested, and directly address the described incident, but the remote backend still has the same pre-fix ordering (EnableRepoActions before provisioning, setErrorStatus zeroing counts on Forgejo failure) which looks like the same bug left unaddressed and should be clarified before merge.
Summary
This is a well-targeted fix for a real production incident, and the reordering logic is sound: spec-derived objects (ConfigMap, PVC, Deployment, status) are now applied before anything that depends on Forgejo, so a broken token degrades new-runner registration instead of freezing the whole reconcile. The
setConditionfix (always update reason/message/observedGeneration, only moveLastTransitionTimeon an actual status flip) and the addition ofobservedGenerationto written conditions are both correct and address exactly the blind spots described in the PR body. Test coverage for the new ordering and for the condition-update semantics is solid and exercises the actual regression (401s from Forgejo, generation mismatch, stale timestamp).A few things worth double-checking before merge:
1.
reconcileRemotestill has the old ordering/behaviorreconcileKuberneteswas reordered, butreconcileRemote(lines ~350-410) still callsEnableRepoActionsbefore provisioning and, on failure, callssetErrorStatus(which zeroesActiveRunners/ReadyRunners) rather thansetDegradedStatus. If the same "Forgejo unreachable freezes everything" failure mode applies to remote-backend pools — and it looks like it does, sinceEnableRepoActionsgilts every reconcile beforeProvision/Deprovisioneven run — this PR only fixes half the bug. Existing VMs also get their active/ready counts zeroed on this path even thoughupdateStatusFromVMsis never reached, which seems like exactly the kind of misleading "no runners" status this PR is trying to eliminate on the Kubernetes side. Worth confirming whether remote is out of scope intentionally (e.g. deliberately deferred, or the incident was Kubernetes-only) or whether it should get the same treatment in this PR or a fast follow-up.2. Automated "possible secret" flag
The scanner flagged
runnerpool_controller.gofor a possible credential. Skimming the diff, this is almost certainly thetokenSecret.Data["registration-token"] = []byte(regToken)line, which already existed before this PR and is a legitimate write of a secret into a KubernetesSecretobject (not a hardcoded credential or a log/leak). I don't see a new leak introduced by this diff — the reordering doesn't add any new place where the token gets written to logs, events, or Sentry. Worth a quick manual sanity check but I don't think this blocks the PR.3. Degraded-status window before the registration Secret exists
When enabling Actions or minting a token fails partway through a brand-new pool's first reconcile, the Deployment is created and its pods will crash-loop or block on the init container waiting for the
-registrationSecret, exactly as described in the PR body ("kubelet retries the pod until it exists"). That's a reasonable design tradeoff and is explicitly called out in the comments, so no objection — just flagging that this behavior (new pods spinning without a ready Secret until Forgejo recovers) is worth mentioning in an operator runbook/troubleshooting doc if one exists, since restart-backoff churn on a new pool could be confusing.4. Minor: comment/behavior drift risk between
setErrorStatusandsetDegradedStatusBoth now funnel through
setNotReady, and the only behavioral difference is whether counts get zeroed. This is clean, but nothing enforces that a future contributor won't add a third path that forgets to zero (or wrongly zeroes) counts. Not a real problem today — just a note that the semantic distinction lives entirely in caller discipline and comments, not in the type system. Fine as-is given the size of this codebase.5. Test coverage
The two new tests (
TestReconcileKubernetes_AppliesSpecWhenForgejoFails,TestSetConditionUpdatesReasonAndMessage) directly target the regression and are well-constructed — they check persisted state via the fake client rather than just the in-memory struct, and the generation/observedGeneration assertions are exactly the right thing to pin down. The gap is the remote backend path (see #1) and thesetDegradedStatusbehavior specifically (i.e., an assertion thatActiveRunners/ReadyRunnersare not zeroed after a degraded Forgejo failure) isn't directly tested — only theReadycondition is checked in the new test. A small addition asserting the counts survive a degraded failure would nail down the fourth bullet in the PR description ("no longer zero ActiveRunners/ReadyRunners").Nits
setDegradedStatusis currently a thin wrapper with no added logic beyondsetNotReady— arguably it could just besetNotReadycalled directly with a comment, but keeping the named wrapper self-documents the call sites, which is a reasonable readability tradeoff.None of the above are blocking on their own, but I'd like to understand whether the remote backend's ordering issue (#1) is intentionally out of scope or an oversight, since it's the same class of bug this PR is designed to close on the Kubernetes side.
📚 Context used in this review
Project guidance files:
CLAUDE.mdGood catch -- fixed rather than documented, in
ed65fe6.reconcileRemotehad the same shape:EnableRepoActionsbefore any VM work, so a revoked token also stranded scale-down (excess VMs kept running), andsetErrorStatusreported zero runners while the VMs were up and taking jobs.Provisioning genuinely needs Forgejo, since each VM registers with its own token, so a failure now skips scale-up for that pass and nothing else: deprovisioning and the VM-derived status still run, and the failure lands as a degraded status over counts that reflect what is actually running. The provisioning loop already broke on a token error, so the early return bought nothing it did not also cost.
Deliberately unchanged:
RemoteBackendErrorandListVMsErrorstill return early, since without a VM list there is no state to reconcile -- provisioner-side failures, not the Forgejo-token class this PR is about.No test for the remote path: there are none today, and
NewRemoteBackendwants a CA secret and a gRPC provisioner, which is a larger fixture than this change should pull in. The Kubernetes path's behavior is pinned byTestReconcileKubernetes_AppliesSpecWhenForgejoFails.Automated review by pr-reviewer v0.47.3 | Full Review | Claude Sonnet 5 | tracking id
r-ab9592-ba7fccThis is an AI-generated review and may contain mistakes.
Status: ✅ Completed
✅ Verdict: Approve — The reconcile-order fix and condition-update fix are correct, well-tested for the primary (Kubernetes) path, and clearly explained; the untested remote-backend change is a pre-existing gap the PR openly acknowledges rather than a new regression.
Summary
This PR reorders
reconcileKubernetes/reconcileRemoteso spec-derived Kubernetes objects (ConfigMap, PVC, Deployment, status) are reconciled before Forgejo-dependent work (enable Actions, registration token, Secret). It also fixessetConditionto always refresh reason/message/observedGeneration (only holdingLastTransitionTimesteady when status doesn't flip), and addsobservedGenerationto written conditions. The rationale and tests are clear and the root-cause narrative in the description matches the diff well.Correctness of the core fix
The reordering logic is sound: the Deployment/ConfigMap/PVC really are pure functions of spec and don't need Forgejo, so moving them earlier is safe, and the safety argument (Deployment reads the token via
secretKeyRef, not an embedded value) holds up givenBuildDeploymentisn't shown as changed here — I'll trust the PR's claim on that since it's not part of this diff.One subtlety worth flagging: on a brand-new pool,
updateStatusFromDeploymentruns and callsdeploymentReady()/deploymentStalled()before the registration Secret exists. Since the Deployment was just created, it won't be "ready" yet, so this isn't a functional bug, but it does mean the very first status write for a new pool briefly reportsReady=False, reason=DeploymentStatus(not yet reflecting anything Forgejo-related) and then a second status write follows a few lines later once the Forgejo work completes/fails. That's just twoStatus().Updatecalls in one reconcile instead of one — not wrong, just worth being aware of as extra API server writes on every reconcile pass now (previously Forgejo failures short-circuited before any Kubernetes object work, so this is a net new baseline cost, not a regression on the happy path).reconcileRemotebehaviorThe change to continue past an
EnableRepoActionsfailure and skip only scale-up (via theforgejoErrlocal) is reasonable and matches the stated intent — deprovisioning/status shouldn't be gated on Forgejo reachability. Good catch that the previous code even returned early before deprovisioning excess VMs, which is worse than what this fixes for the Kubernetes path.One asymmetry: in
reconcileRemote,forgejoErronly guards scale-up (current < desired && forgejoErr == ""), but scale-down (current > desired) proceeds regardless — which is intended per the PR text. That's fine, but notesetDegradedStatusis called unconditionally wheneverforgejoErr != "", even in a reconcile where scale-down happened successfully and nothing is otherwise wrong outside of enabling Actions. That matches the description ("failure recorded as a degraded status over counts that reflect what is running"), so this looks intentional and correctly implemented.setConditionsemanticsThe new
setConditionalways overwrites reason/message/observedGeneration and only preservesLastTransitionTimewhenStatusis unchanged. This is a good, minimal fix for the described bug (stale timestamp/reason persisting for two months). The added testTestSetConditionUpdatesReasonAndMessagedirectly pins this behavior, including the transition-time-moves-on-flip case. No concerns here.Test coverage
TestReconcileKubernetes_AppliesSpecWhenForgejoFailsis a solid regression test for the primary bug: it verifies the Deployment image updates despite 401s, and thatReadycorrectly reportsEnableActionsErrorwith the rightObservedGeneration.TestSetConditionUpdatesReasonAndMessagecovers the condition-write fix in isolation.TestReconcileKubernetes_ImageChangeUpdatesDeployment(pre-existing, updated only for the new registration-token mock) still validates the healthy path.reconcileRemotehas zero test coverage (pre-existing gap, not introduced here) — the described reasoning for not adding a fixture-heavy test in this PR is reasonable given the scope, but it does mean the newforgejoErr/scale-down-continues logic inreconcileRemoteis currently verified only by inspection. I'd call this an acceptable near-term tradeoff given the note aboutNewRemoteBackend's dependencies, but it's worth a tracked follow-up since this is exactly the kind of ordering logic that regresses silently.Minor observations
setErrorStatusandsetDegradedStatusare now both thin wrappers aroundsetNotReady, withsetErrorStatusadditionally zeroing metrics gauges. This is fine and the comments clearly explain the distinction (zeroing counts vs. leaving them alone), but the two-function split for what's essentially "should I zero the metrics" is a little indirect — a boolean parameter to a single function would be equally clear and slightly less API surface. Not blocking.tokenSecret.Data["registration-token"] = []byte(regToken)is pre-existing code just relocated, and test tokens are clearly fake placeholder strings ("revoked", "reg-token", "irrelevant").Verdict
The change is well-reasoned, narrowly scoped to the actual bug, backed by comments that explain intent, and includes targeted regression tests for both the reconcile-ordering bug and the condition-update bug. The
reconcileRemotepath lacks test coverage, but that's a pre-existing gap the PR is honest about rather than a regression, and the logic there is a straightforward, well-explained mirror of the same fix. I don't see any bugs, breaking changes, or security issues introduced.📚 Context used in this review
Project guidance files:
CLAUDE.md