Move setLastReconciledSpec to after successful transition start#1072
Open
jennifer-xiong25 wants to merge 1 commit into
Open
Move setLastReconciledSpec to after successful transition start#1072jennifer-xiong25 wants to merge 1 commit into
jennifer-xiong25 wants to merge 1 commit into
Conversation
Contributor
2 tasks
Contributor
|
cc @schongloo |
Contributor
|
@jennifer-xiong25 can you please open a jira ticket for this? |
3 tasks
jhung-lyft
added a commit
to lyft/flink-kubernetes-operator
that referenced
this pull request
May 22, 2026
## TL;DR Fixes three bugs in `FlinkBlueGreenDeployment` that compound to leave a transition silently restoring from a stale savepoint: | # | Bug | Fix | |---|-----|-----| | 1 | `savepointTriggerId` not cleared on abort → next deploy stuck FAILING+IGNORE | Clear it in `abortDeployment` (mirrors upstream [a00bf27](apache@a00bf27)) | | 2 | `setLastReconciledSpec` runs before `startTransition` → failed spec marked reconciled, retries no-op | Stamp only on success (mirrors upstream [apache#1072](apache#1072)) | | 3 | Suspended child carries stale `status.upgradeSavepointPath` → JM restores from old savepoint on retry | Delete the failed child in `abortDeployment` so the next transition fresh-creates it | Verified in unit test and end-to-end on `data-stg` — see [Validation](#Staging-validation-on-data-stg) below. ## Issue 1 — `savepointTriggerId` not cleared on abort `abortDeployment` left `status.savepointTriggerId` in place. The next reconcile tried to re-fetch the trigger from the active side's JM, which had TTL-evicted it (~300s), threw `Could not fetch savepoint with triggerId: X`, and left FBGD stuck in FAILING + IGNORE. Required a `restartNonce` bump to escape. ## Issue 2 — `setLastReconciledSpec` stamped before `startTransition` returns In `checkAndInitiateDeployment`, `setLastReconciledSpec` ran *before* `startTransition`. If `startTransition` threw (e.g., from Issue 1), the failed spec was already marked reconciled, so the next reconcile saw `specDiff = IGNORE` and silently did nothing until another spec change came in. ## Issue 3 — Stale `status.upgradeSavepointPath` shadows fresh `spec.initialSavepointPath` `AbstractJobReconciler.restoreJob()` reads `status.jobStatus.upgradeSavepointPath`, not `spec.initialSavepointPath`, in the `terminal && !isHaMetadataAvailable` upgrade branch. A suspended child retained its stale `upgradeSavepointPath` from the previous failed deploy. On retry, FBGD wrote a fresh `spec.initialSavepointPath`, but `createOrReplace` does not touch the status subresource — so the stale status value won and the JM restored from an arbitrarily old savepoint while the transition appeared successful. Upstream [apache#1073](apache#1073) does NOT fix this: its `!isHaMetadataAvailable || isJmAccessible` gate is satisfied with empty HA, so the operator still routes through the stale-status path. **Fix**: `abortDeployment` now deletes the failed child instead of suspending it. The next transition fresh-creates the child via `createOrReplace`'s POST path, with no status to be stale. Falls back to `suspendFlinkDeployment` if the delete call fails, to avoid an inconsistent state. ## Tests - `verifyFailedChildDeletedOnAbort` (new): forces a failed transition, asserts the failed child is gone, then asserts the retry fresh-creates GREEN with the fresh savepoint in spec and no carry-over status. - `verifyFailureDuringTransition` / `verifyFailureBeforeFirstDeployment`: updated to assert the failed child is absent (not SUSPENDED) after abort. - `verifySavepointFetchFailureRecovery`: asserts the failed spec is NOT present in `lastReconciledSpec` (Issue 2). ## Test plan - [x] `FlinkBlueGreenDeploymentControllerTest` → 60/60 pass - [x] `*ControllerTest` → 148/148 pass (no regressions in FlinkDeployment / SessionJob / StateSnapshot) - [x] Deployed to `data-stg`; forced abort + retry on `beamperfk8s1`; verified all three fixes ([details below](#Staging-validation-on-data-stg)) ## Staging validation on data-stg Forced a failed transition on `beamperfk8s1` with a bad image tag, then retried with a good image. | Time (UTC) | Event | |------------|-------| | 15:08:40 | First transition: triggerId `5d75a09efb99...`, savepoint `6966a02076a5` → new GREEN | | 15:09:42 | `Aborting deployment 'beamperfk8s1-green', rolling B/G deployment back to ACTIVE_BLUE` | | 15:09:44 | GREEN audit: `Status[Job] \| Error \| DELETED \| ErrImagePull` | | 15:11:40 | Retry: NEW triggerId `5f1d597953fe...` (different from first) | | 15:11:55 | Fresh savepoint `b0d173acab2e` → fresh GREEN | | 15:12:56 | GREEN JM log: `Starting job ... from savepoint .../savepoint-4d93e8-b0d173acab2e` | | 15:13:23 | Old BLUE deleted (normal post-transition cleanup) | - **1**: retry used a new triggerId, not the TTL-evicted first one - **2**: retry transition actually ran (would have been IGNORE'd if the failed spec had been stamped) - **3**: failed GREEN was DELETED (not suspended); fresh GREEN's JM restored from the fresh `b0d173acab2e`, not the prior attempt's `6966a02076a5` ## Production incident this resolves May 7 `demandingestion`: a transient deploy failure left stale `savepointTriggerId` and `status.upgradeSavepointPath` on a suspended GREEN; the failed spec was stamped as reconciled, freezing retries; when the trigger TTL evicted and a deploy went through, the JM silently restored from a 9-day-old savepoint. Full RCA: [Deploy Failure Investigation](https://docs.google.com/document/d/1rwN2TABe-3VtkHWBttLbLlW_P6Q-TMs0cuAs6fd-cQA/edit). --------- Co-authored-by: jennifer-xiong25 <jennifer.xiong@shopify.com>
Contributor
|
@jennifer-xiong25 @jhung-lyft would be nice if we could wrap this PR up and get the fix in |
|
Hi @gyfora, thanks for checking in! Just to clarify, I cherry-picked @jennifer-xiong25's fix internally, so she'll still be driving this pr to completion here. Also, we found and fixed another bug (stale status.upgradeSavepointPath causing JM to restore from an old savepoint on retry). We’d love to submit this upstream as a follow-up PR once our internal CLA process is cleared up. Thank you! |
Contributor
|
thanks @jhung-lyft , looking forward to it! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a bug where the last reconciled spec on the flink b/g deployment shows an updated spec even when the update failed.
Summary
setLastReconciledSpec()to afterstartTransition()andstartSavepointRedeployTransition()succeed, so it's not stamped if the transition failslastReconciledSpecwas set before the transition call — if the transition failed or was later aborted,lastReconciledSpecreflected the new spec while the active child still ran the old one. Subsequent deploys would see no diff and fall into PATCH_CHILD (in-place upgrade) instead of a proper B/G transitionverifySavepointFetchFailureRecoveryconfirminglastReconciledSpecdoes not contain the failed spec changeTest plan
verifySavepointFetchFailureRecoverypasses with new assertion