k8s/api: auto-rollback failed deploys to the prior revision#7
Conversation
Polls Deployment.Status every 2s and returns when either: - all updated replicas are ready and available - the Progressing condition flips to False/ProgressDeadlineExceeded (returns an error wrapping ErrRolloutFailed, so callers can distinguish a hard failure from a ctx-timeout) - ctx is cancelled or deadlines Wired into deployApp in a follow-up commit. This commit is standalone: the new file is not yet imported from internal/api. Polling (vs a kube watch) keeps tests identical to production — the fake clientset does not emit Modify events for Status subresource updates.
Centralises the DeployRequest field mapping that deployApp was doing inline. Adds a parallel revision→DeployRequest helper for the upcoming auto-rollback path (unused in this commit). No behavior change: existing tests stay green.
After client.DeployApp returns nil, transition the app to 'verifying' and wait for WatchRollout to report the Deployment actually became healthy. On failure, mark app + revision failed; on success, keep the existing transitions (UpdateAppRevision, status=running, revision=success). No rollback yet — that's the next commit. Ships cleanly on its own: without the watch, 'running' was a lie for any deploy that crash-looped or got stuck pulling an image.
When WatchRollout reports the new revision never became healthy
(ProgressDeadlineExceeded or ctx timeout), redeploy revision N-1
inline — same client.DeployApp path, no watch on the rollback itself
(v1; if it fails we mark the app failed and stop).
Revision-number rules:
- N == 1 → no prior revision exists. Log first-deploy-cannot-rollback
and leave the app in 'failed'. No redeploy attempt.
- N > 1 → redeploy from the N-1 snapshot. On success, app status →
'running', app.current_revision → N-1, revision N → 'rolled_back'
(with the original watch error in the message). On failure, both
errors are logged and persisted; app stays 'failed'.
Runs inside the existing per-app deploy goroutine under the mutex
from PR #6, so a rollback cannot race a new deploy for the same app.
Worst-case window doubles (progress deadline + grace, then the rollback
DeployApp) — accepted.
Tests: four scenarios at the k8s-client level — happy path, watch-
fails-rollback-succeeds, first-deploy-fail, and rollback-also-fails.
A comment at the top of the test file explains why the tests live at
the k8s primitive layer instead of the api-handler layer (there is no
injection seam for *db.DB or k8s.Client today; adding one is
out of scope).
CRITICAL / WARNING items addressed: - C2 — Document the DeleteOldRevisions(keep=10) invariant at the call site so future aggressive pruning can't silently break autoRollback's ability to load revision N-1. - W1 — Tighten auto_rollback_test.go header: enumerate exactly which DB status transitions are NOT exercised (app.status strings, rolled_back marker, CurrentRevision update, N<=1 guard, secret re-resolution, ingress re-sync). Tracked in ROADMAP as handler-test- harness follow-up. - W3 — Soften DeploymentProgressDeadline log: "using default progress deadline" prefix instead of "lookup failed" (the fallback is correct behavior during the expected race on first-ever deploy, not an error). - W4 (real bug) — Extract syncCustomDomainIngress helper that takes an explicit port *int. Happy path passes app.Port; autoRollback passes prior.Port. App fields aren't re-copied from revisions on rollback, so the live app.Port still reflects the broken revision after a rollback succeeds — we need the revision's port. - W5 — Update autoRollback doc: 2 x progressDeadline upper bound on mutex hold time; concurrent deploys queue behind it. INFO items addressed: - I2 — Drop ErrRolloutFailed sentinel. Unused by callers (dead code). Tests now assert on "rollout failed:" prefix via strings.HasPrefix. - I3 — Log style: reason=%v matches the rest of the codebase's key=%v convention. Not fixed in code (tracked in ROADMAP Phase 2.11 follow-ups): - C1 — Orphaned-verifying recovery (boot-time reconciler). - W2 — Snapshot secret key names in app_revisions so rollback can detect deleted keys. - Handler-level test harness (W1 coverage gap). ROADMAP: check off auto-rollback + per-app deploy mutex (PR #6) + rollingUpdateBudget effective-fleet fix (PR #6). Phase 2.11 progress 30% -> 60%. Add three new follow-up items. Tests: full internal/k8s + internal/api suites pass. go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Trivial nit caught by the second-pass review — match the key=value convention used everywhere else. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| secretName := "" | ||
| if secrets, _ := h.db.GetSecretsByAppID(ctx, appID); len(secrets) > 0 { | ||
| secretName = app.Name + "-secrets" | ||
| } |
There was a problem hiding this comment.
The forward deployApp path calls client.CreateOrUpdateSecret(...) to write secret values into the cluster before deploying. autoRollback only records the secret name and skips CreateOrUpdateSecret. If secret values in the DB changed between the time the failed deploy ran and the time the rollback fires (e.g., an operator rotated a credential during the watch window), the K8s Secret object will retain stale values from the forward deploy — the rollback pods will use incorrect credentials at runtime.
The fix is to decrypt and re-sync secrets in autoRollback the same way deployApp does, before calling client.DeployApp.
| func rolloutReady(d *appsv1.Deployment) bool { | ||
| if d.Spec.Replicas == nil { | ||
| return false | ||
| } | ||
| if d.Status.ObservedGeneration < d.Generation { | ||
| return false | ||
| } | ||
| desired := *d.Spec.Replicas | ||
| return d.Status.UpdatedReplicas == desired && | ||
| d.Status.ReadyReplicas == desired && | ||
| d.Status.AvailableReplicas == desired | ||
| } |
There was a problem hiding this comment.
rolloutReady returns true immediately for zero-replica deployments
When *d.Spec.Replicas == 0, the comparison 0 == 0 && 0 == 0 && 0 == 0 trivially satisfies the ready check, so WatchRollout returns nil on the first poll. This is technically correct Kubernetes semantics (a scaled-to-zero deployment is considered healthy), but if the system ever deploys with 0 replicas (e.g., during an HPA-off transition or a misconfigured CreateApp), the deploy will be marked "running" instantly without any actual pods. A comment documenting this behaviour would help future readers distinguish intentional correctness from an oversight.
- handlers.go: extract syncSecretsToCluster helper; autoRollback now re-syncs the cluster Secret object from current DB state before the rollback DeployApp. Forward path refactored to use the same helper (no behavior change there — secrets aren't versioned in revisions, so "current DB state" IS the authoritative source). Closes the idempotency gap Greptile flagged where a secret rotation during the watch window would leave stale values in the cluster Secret when the rollback fired. - rollout.go: document zero-replica rolloutReady semantics — a Deployment scaled to 0 is trivially ready once the generation is observed (matches kube + kubectl rollout status). No behavior change; adds the "why this is intentional" comment Greptile asked for so future readers don't flag it as an oversight. Tests: full internal/k8s + internal/api suites green, go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| dep, err := c.clientset.AppsV1().Deployments(namespace).Get(ctx, name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if ctx.Err() != nil { | ||
| return ctx.Err() | ||
| } | ||
| return fmt.Errorf("watch rollout: get deployment: %w", err) | ||
| } |
There was a problem hiding this comment.
Any single transient error from the kube API server (network blip, 5xx, etcd leader election) causes WatchRollout to return a non-nil error immediately, which then triggers autoRollback in deployApp even though the deployment's pods may be healthy and converging. A healthy deploy that hits one transient Get will be silently rolled back to the prior image.
The fix is to distinguish between transient errors (retry with backoff) and definitive failures (fail-fast). At minimum the error path should retry a configurable number of times before surfacing, similar to how kubectl rollout status behaves:
dep, err := c.clientset.AppsV1().Deployments(namespace).Get(ctx, name, metav1.GetOptions{})
if err != nil {
if ctx.Err() != nil {
return ctx.Err()
}
// Transient API error — log and wait for next poll rather than
// returning immediately and triggering an unwarranted rollback.
log.Printf("watch rollout: transient get error (will retry): %v", err)
select {
case <-ctx.Done():
return ctx.Err()
case <-ticker.C:
}
continue
}
|
Addressed both Greptile P2 findings in 0c09d86:
Tests green, go vet clean. |
| ticker := time.NewTicker(2 * time.Second) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| dep, err := c.clientset.AppsV1().Deployments(namespace).Get(ctx, name, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if ctx.Err() != nil { | ||
| return ctx.Err() | ||
| } | ||
| return fmt.Errorf("watch rollout: get deployment: %w", err) |
There was a problem hiding this comment.
WatchRollout returns a non-nil error on the very first Get failure, which causes deployApp to invoke autoRollback — even when the deployment's pods are healthy and still converging. A one-off network blip, etcd leader election hiccup, or transient 5xx will silently roll a working deploy back to the prior image.
The fix is to retry transient errors a bounded number of times before surfacing:
dep, err := c.clientset.AppsV1().Deployments(namespace).Get(ctx, name, metav1.GetOptions{})
if err != nil {
if ctx.Err() != nil {
return ctx.Err()
}
// Transient API error — log and wait for next poll rather than
// returning immediately and triggering an unwarranted rollback.
log.Printf("watch rollout: transient get error (will retry): %v", err)
// fall through to the select/ticker to retry on next tick
} else {
if rolloutReady(dep) { return nil }
if reason, failed := rolloutFailed(dep); failed {
return fmt.Errorf("rollout failed: %s", reason)
}
}kubectl rollout status uses a similar retry-on-error approach to distinguish transient API failures from definitive rollout failures.
Before: a single transient apiserver error (network blip, etcd leader election, 5xx) from the polling Get immediately surfaced from WatchRollout, which caused deployApp to invoke autoRollback on a deploy whose pods were still converging fine. Real production-risk behavior — an unrelated control-plane hiccup during a 600s watch window would silently roll a healthy deploy back. Now: non-ctx Get errors are logged and the watch loop continues to the next tick. The watch ctx (progressDeadline + 10s ≈ 610s) is the total retry budget. If the apiserver is unreachable for the entire window, ctx.Err() fires naturally and the rollout is treated as a timeout — which IS correct for an unreachable control plane. Tests: TestWatchRollout_GetErrorSurfaces renamed to TestWatchRollout_PersistentGetErrorHitsCtxTimeout (asserts ctx.Err() instead of the old typed-error surface). New TestWatchRollout_TransientGetErrorRecovers (regression guard) injects one transient Get failure followed by a ready deployment and asserts WatchRollout returns nil without rollback. Closes Greptile P1 (PR #7 comment 3106354508). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed the Greptile P1 (transient Get → spurious rollback) in 5669aba.
Tests:
Full |
Before this fix, any app that had ever had an auto-rollback fire would
have its NEXT deploy fail at CreateRevision with a UNIQUE(app_id,
revision_number) violation. Reproduce:
state: CurrentRevision=5, revisions [1..5]
deploy 6 → breaks → autoRollback sets CurrentRevision=5 and marks
revision 6 rolled_back
next deploy → newRevision = CurrentRevision+1 = 6
CreateRevision(6) → UNIQUE violation, app stuck
Two coupled fixes:
1. db: new GetNextRevisionNumber(appID) returning COALESCE(MAX, 0)+1.
deployApp switches from app.CurrentRevision+1 to this. CurrentRevision
tracks the last *successful* deploy; after a rollback it regresses,
so CurrentRevision+1 collides with the rolled_back revision that
still exists.
2. handlers: autoRollback's rollback target is now app.CurrentRevision
(last success), not newRevision-1. With MAX+1 allocation in place,
newRevision-1 can be a previously-rolled-back revision from an
earlier incident — rolling back to it would redeploy broken code.
Since app.CurrentRevision is only written on successful deploys, it
is always a safe target.
Also addresses a smaller issue Greptile flagged on RollbackApp (line
~1018): the ignored error from h.db.GetApp would pass nil into
deployApp and nil-deref on first field access. Now: log and fall back
to the pre-update `app` snapshot. deployApp's own in-goroutine re-fetch
will retry shortly after.
Test coverage: the sequence "deploy → autoRollback → next deploy"
requires a handler-level DB harness which this repo doesn't have (see
ROADMAP Phase 2.11 "Handler-level test harness"). The fix is exercised
by code review + the existing k8s-level tests which continue to pass
unchanged. File a follow-up once the harness lands to add the full
collision regression test.
Tests: internal/k8s + internal/api suites pass under -race. go vet clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed both Greptile findings in a770fdf. Critical: revision-number collision after auto-rollbackReproduced the bug: Two coupled fixes:
Minor: error-ignored GetApp in RollbackAppNow: log the error and fall back to the pre- Test coverageThe collision regression (full "deploy → autoRollback → next deploy" sequence) needs a handler-level DB harness this repo doesn't have yet; tracked in ROADMAP Phase 2.11 as "Handler-level test harness" follow-up. The two fixes are small and well-commented; existing k8s-level tests continue to pass unchanged under |
Elite PR review on top of a770fdf caught two critical bugs in the manual-rollback path, both introduced or amplified by the auto-rollback feature: C1 — Manual RollbackApp → watch fails → autoRollback redeploys the broken revision the user was escaping from. RollbackApp.UpdateApp copied the target revision's fields onto the apps row but never updated current_revision. The row still pointed at the broken revision. If deployApp's watch timed out during the rollback (transient control-plane issue, slow image pull), autoRollback would read app.CurrentRevision — now the broken revision — and redeploy it. App marked "running" on the broken image. Fix: UpdateAppRevision(targetRevision.RevisionNumber) right after UpdateApp, before deployApp fires. Now CurrentRevision points at the rollback target, so if autoRollback triggers it lands on the correct revision. C2 — Default manual rollback (no revision specified) uses CurrentRevision-1, which with MAX+1 allocation can be a rolled_back or failed revision from a prior incident. Sequence: rev 1 success → auto-rollback on rev 2 (current stays 1) → rev 3 success (current=3) → user hits default rollback → GetRevision(3-1=2) → broken spec re-applied. Fix: new db.GetLastSuccessfulRevisionBefore(appID, below) query that selects the highest revision_number with deploy_status='success' strictly below the given number. RollbackApp's default path uses it instead of CurrentRevision-1. Returns ErrNoRows for apps with no prior success; surfaces as 404 "no previous successful revision to rollback to". Closes elite-pr-review findings C1 and C2 on PR #7. Tests: internal/k8s + internal/api suites pass under -race. go vet clean. Handler-level integration test for the full (deploy → autoRollback → manual-rollback → watch-timeout → correct target) sequence requires the DB harness tracked in ROADMAP Phase 2.11. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Fixed both CRITICAL elite-pr-review findings in 880c347: C1 — Manual C2 — Default rollback picked Tests: internal/k8s + internal/api suites green under |
Elite review of 880c347 approved 4/5 with one narrow window worth closing and one doc polish: - RollbackApp: after UpdateAppRevision succeeds in the DB, also mutate the pre-update `app` snapshot's CurrentRevision in memory. If the post-update GetApp AND deployApp's in-goroutine GetApp both fail (narrow same-outage window), deployApp still sees the correct CurrentRevision and autoRollback targets the rollback destination rather than the broken revision. Closes the "re-fetch fallback re- opens C1" window the reviewer flagged. - GetLastSuccessfulRevisionBefore doc comment: note that the returned pointer is non-nil even on ErrNoRows (consistent with the rest of the file) and callers must check err before dereferencing. Tests: internal/k8s + internal/api suites green under -race, go vet clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Closes the top remaining Phase 2.11 safety gap: shipit now auto-rolls-back a deploy whose pods never become ready.
Before this PR,
DeployApp()returned success the moment kube accepted the spec — even if the new pods immediately crash-looped onImagePullBackOff. Status was markedrunningon a deployment that wasn't running. Operators had to notice and fireRollbackAppmanually.Now: every deploy is followed by a bounded rollout watch; on failure, the prior revision is redeployed inline; the UI reflects the actual cluster state.
What lands
Five bisectable commits:
5977bdck8s/rollout.go—WatchRollout2s polling +DeploymentProgressDeadlinehelper; tests for happy/timeout/ProgressDeadlineExceeded/ctx-cancel/Get-error paths2241472buildDeployRequestFromApp+buildDeployRequestFromRevisionhelpers; de-dupes field mapping fromdeployAppandRollbackApp0ce583fWatchRolloutintodeployApp; new app statusverifying; on failure →failedwith rollout error97b56f0autoRollbackmethod: loads revision N-1 on failure, redeploys inline; new statusesrolling_back/rolled_back749dbfaErrRolloutFailedsentinel, misc comment/log cleanups, ROADMAP follow-upsNo DB migration. All new statuses are free-form strings. FE treats unknown statuses as in-progress ("info" variant).
Behavior
Key design decisions
client.DeployApp— not recursing intodeployAppor callingRollbackApp. No recursion hazard.Update()-applied statuses; polling keeps prod/test parity.progressDeadlineSeconds(~20 min).RollbackApphandler.Test plan
Automated (in this PR — all green,
go vetclean):TestWatchRollout_HappyPath— ready status → returns nilTestWatchRollout_GenerationLagTimesOut—Status.ObservedGeneration < Generation→ never ready → ctx deadlineTestWatchRollout_ProgressDeadlineExceededFailsFast— condition set → fast-fail withrollout failed:prefixTestWatchRollout_CtxCancelledExternally— external cancel →context.CanceledsurfacedTestWatchRollout_GetErrorSurfaces— reactor injects 500 on Get → surfacesTestProgressDeadline_UsesDefaultWhenUnset/ReadsSpecValueTestAutoRollback_HappyPath_NoRollback— happy path; no rollback triggeredTestAutoRollback_WatchFails_RollbackSucceeds— broken deploy → rollback redeploys prior imageTestAutoRollback_FirstDeployFails_NoRollback— N==1 case; no phantom rollbackTestAutoRollback_RollbackDeployFails— reactor rejects rollback update; both errors surfaceManual (after merge — non-prod first):
verifying→rolling_back→runningon prior revision in the UIfailed, logs showfirst-deploy-cannot-rollbackKnown v1 gaps (tracked in ROADMAP Phase 2.11)
verifying— no boot-time reconciler. Tracked as "Orphaned-verifying recovery" follow-up.Heads-up for FE team
Three new app status strings may appear:
verifying,rolling_back,rolled_back. CurrentStatusBadge(web/src/components/ui/Badge.tsx:62) falls through unknown statuses to the blue "info" variant — the literal string will render. Worth a small UX pass in a follow-up PR to give these distinct icons/colors.🤖 Generated with Claude Code
Greptile Summary
This PR closes the Phase 2.11 auto-rollback safety gap:
deployAppnow watches the rollout after every deploy, and onProgressDeadlineExceededit redeploys the last successful revision inline. All four previously-reported P1 issues (transient-error spurious rollback, revision-number collision, default-rollback landing on a failed revision, and manualRollbackAppfeedingautoRollbackthe broken revision) have been addressed in the follow-up commits via retry logic inWatchRollout,GetNextRevisionNumber(MAX+1),GetLastSuccessfulRevisionBefore, and pre-settingcurrent_revisionbeforedeployAppfires.Confidence Score: 5/5
Safe to merge — all previously reported P1 issues are resolved; remaining findings are P2 style observations that do not affect correctness.
Every P1 from the prior review round has a concrete fix: transient-error retry loop in WatchRollout, MAX+1 revision allocation, GetLastSuccessfulRevisionBefore skipping failed/rolled_back rows, and pre-setting current_revision before deployApp fires during RollbackApp. The two remaining observations are P2 design/style points that do not cause data loss or incorrect cluster state.
No files require special attention for merge safety.
Important Files Changed
Comments Outside Diff (4)
internal/api/handlers.go, line 999-1004 (link)RollbackAppcallsdeployApp, which now includesWatchRollout+autoRollback. If the manual rollback deploy fails its watch,autoRollbackis invoked withnewRevision = N+1and fetchesGetRevision(N)— the revision the user was rolling back from. It then redeploys that revision and marks the app"running", silently undoing the user's intent and (re-)applying the known-broken spec.Concretely: app is at revision N (broken) → user calls
RollbackAppto N-1 →deployAppcreates revision N+1 with N-1's spec → watch fails (transient or infra issue) →autoRollbackdeploys revision N → app reports"running"on the broken revision.A guard in
autoRollbackdistinguishing calls originating from a user-initiated rollback vs. a forward deploy — or simply not routingRollbackAppthroughdeployApp's watch loop — would prevent this behavior.internal/api/handlers.go, line 1018-1019 (link)RollbackApproutes throughdeployApp— auto-rollback fires on the wrong revisiongo h.deployApp(appID, updatedApp, kubeconfig)is the same code path used by the forward deploy. SincedeployAppnow wires inWatchRollout+autoRollback, if the manual rollback deploy fails its watch (including for a transient API error),autoRollbackis invoked withnewRevision = N+1and fetchesGetRevision(N)— the broken revision the user was rolling back from. It then marks the app"running"on the exact broken image the user just tried to escape.Concrete sequence: app at revision N (broken) → user calls
RollbackAppto N-1 →deployAppcreates N+1 (N-1 spec) → watch fails →autoRollback(N+1)→GetRevision(N)→ redeploys broken image →status = "running".A guard in
autoRollback— or a separatedeployForRollbackthat skips the watch+autoRollback chain — would prevent this loop.internal/api/handlers.go, line 525 (link)autoRollbackcallsUpdateAppRevision(ctx, appID, prior.RevisionNumber)(line 760), settingapps.current_revision = N-1. The next forward deploy then computesnewRevision = app.CurrentRevision + 1 = N. But revision N already exists inapp_revisionswith status"rolled_back"— and the table enforcesUNIQUE(app_id, revision_number)(migration004_app_revisions.sql).CreateRevisionwill hit a unique constraint violation, the error is caught at line 556–559, and the deploy is immediately marked"failed"before any k8s work happens.Every deploy after a successful auto-rollback will silently fail at the revision-creation step until someone manually reconciles
current_revisionor deletes the conflicting row.One fix is to keep
current_revisionat N rather than rolling it back, so the next deploy correctly creates N+1:internal/api/handlers.go, line 967-976 (link)GetRevision(CurrentRevision - 1)assumes revision numbers are sequential. With this PR'sGetNextRevisionNumber(MAX+1), gaps are intentionally created: after an auto-rollback,current_revisionstays at the last success (say N), the next deploy creates revision N+2 (not N+1), and on success setscurrent_revision = N+2. A defaultRollbackAppcall then fetches revisionN+2 - 1 = N+1— the failed/rolled-back revision — and redeploys it, undoing the user's intent.Concrete sequence:
current_revision = 1current_revisionstays 1current_revision = 3RollbackApp(no revision specified) →GetRevision(3−1=2)→ broken rev 2 gets redeployedThe fix is to find the highest-numbered revision below
current_revisionthat hasdeploy_status = 'success', or to add aprevious_revisioncolumn to theappstable.Reviews (8): Last reviewed commit: "api/db: belt-and-suspenders for C1 + doc..." | Re-trigger Greptile