Skip to content

[FLINK-40092] BlueGreen: finalize forward when a prior finalize didn't complete#1161

Draft
fqshopify wants to merge 1 commit into
apache:mainfrom
Shopify:fix_forward_if_finalize_failed
Draft

[FLINK-40092] BlueGreen: finalize forward when a prior finalize didn't complete#1161
fqshopify wants to merge 1 commit into
apache:mainfrom
Shopify:fix_forward_if_finalize_failed

Conversation

@fqshopify

Copy link
Copy Markdown
Contributor

What is the purpose of the change

During a Blue/Green transition, deleting the previous deployment (child) and persisting the finalized parent status are not atomic. If the reconcile crashes or the status patch 409s after the delete is accepted but before the finalize persists, recovery resumes with the previous deployment already gone but the state still TRANSITIONING_TO_* and the abort timer set. The dispatcher only checked the next deployment's readiness, so a momentarily not-ready next deployment could trigger a rollback to an already-deleted target.

This PR closes that gap structurally: once the previous deployment is gone mid-transition and the deletion deadline has passed, that is treated as the point of no return and the transition always finalizes forward instead of aborting.

(Follow-up to the discussion in #1154 over here)

Brief change log

  • Added an early check in monitorTransition: if the previous deployment is gone/terminating and past the deletion deadline, finalize forward instead of dispatching to abort logic
  • Extracted the cutover timing math into a small CutoverSchedule helper

Verifying this change

This change added tests and can be verified as follows:

  • Added verifyGreenFinalizesForwardWhenBlueDeletedButTransitionNotFinalized, which reproduces the crash-gap recovery (previous deployment deleted, status still TRANSITIONING_TO_GREEN, next deployment briefly not-ready) and asserts the transition commits forward to ACTIVE_GREEN (including the ingress cutover) rather than rolling back

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: yes

Documentation

  • Does this pull request introduce a new feature? no

// If an earlier BlueGreenDeployment deleted the previous deployment but failed before
// finalizing the transition, there is nothing to roll back to so we must finalize forward
// instead of aborting to a deleted target.
if (isGoneOrTerminating(transitionState.currentDeployment)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting together this follow-up. This is a clean, schema-free way to close the important crash/status-patch gap after #1154. One question about the remaining read-after-write window: transitionState.currentDeployment comes from the JOSDK secondary-resource informer cache. If deletion was accepted but the ACTIVE status patch failed, a reconcile could still see cached Blue without deletionTimestamp; if Green is not ready and the abort deadline has expired, we could still reach shouldWeAbort(). Would it make sense to perform a live GET of the previous FlinkDeployment only at that destructive abort boundary (ready timestamp recorded and deletion/abort deadlines passed), finalizing forward if the API reports it absent or terminating? If the GET fails, we could reschedule rather than abort. This should add negligible API load because it is limited to a rare recovery path. A test with stale cached Blue while the API reports it terminating would also be valuable. Really appreciate the thoughtful fix here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants