fix(controller): stop starting the next stage after a cancel command#6979
Open
Ahmad-Faraj wants to merge 2 commits into
Open
fix(controller): stop starting the next stage after a cancel command#6979Ahmad-Faraj wants to merge 2 commits into
Ahmad-Faraj wants to merge 2 commits into
Conversation
When a cancel arrives while a stage is finishing, the stage can still report success and the scheduler continues to the next stage. The next select then receives nil from the closed cancelledCh and falls through without cancelling or waiting for the just-started stage, leaving it running while rollback begins. Make a received cancel command terminate the deployment even when the stage reported success, and treat a nil receive as a cancellation. Signed-off-by: Ahmad Faraj <ahmedfrag4040@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR improves cancellation handling in the Piped controllers’ schedulers so that a user cancel reliably stops stage progression and avoids leaving a forward stage running while rollback begins.
Changes:
- Ensure a received cancel ends the deployment even if the current stage reports
STAGE_SUCCESS. - Treat receives from a closed
cancelledChas a cancellation signal by always cancelling/waiting for the current stage. - Apply the same scheduler behavior fix in both
pipedandpipedv1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/app/pipedv1/controller/scheduler.go | Adjusts cancel-channel handling and cancellation precedence after stage completion. |
| pkg/app/piped/controller/scheduler.go | Mirrors the same cancel precedence and closed-channel handling changes for legacy piped. |
Comment on lines
+389
to
+395
| // A received cancel command wins even if the stage finished successfully, | ||
| // otherwise the loop would continue to the next stage after the user cancelled. | ||
| if cancelCommand != nil { | ||
| deploymentStatus = model.DeploymentStatus_DEPLOYMENT_CANCELLED | ||
| statusReason = fmt.Sprintf("Cancelled by %s while executing stage %s", cancelCommander, ps.Id) | ||
| break | ||
| } |
Comment on lines
+390
to
+396
| // A received cancel command wins even if the stage finished successfully, | ||
| // otherwise the loop would continue to the next stage after the user cancelled. | ||
| if cancelCommand != nil { | ||
| deploymentStatus = model.DeploymentStatus_DEPLOYMENT_CANCELLED | ||
| statusReason = fmt.Sprintf("Cancelled by %s while executing stage %s", cancelCommander, ps.Id) | ||
| break | ||
| } |
Comment on lines
372
to
+380
| case cmd := <-s.cancelledCh: | ||
| // A nil command means the channel was closed after delivering | ||
| // a cancel command, so treat it as a cancellation as well. | ||
| if cmd != nil { | ||
| cancelCommand = cmd | ||
| cancelCommander = cmd.Commander | ||
| handler.Cancel() | ||
| <-doneCh | ||
| } | ||
| handler.Cancel() | ||
| <-doneCh |
…age completes The select can pick doneCh while a cancel command is already buffered, which would let the next stage start before the cancel is observed. Drain the channel non-blocking after the stage completes. Signed-off-by: Ahmad Faraj <ahmedfrag4040@gmail.com>
Contributor
|
The problem is we need to determine what we should do in case case cmd := <-s.cancelledCh:
if cmd != nil {
cancelCommand = cmd
cancelCommander = cmd.Commander
}
handler.Cancel()
<- doneCh
....
if s.cancelled && result == model.StageStatus_STAGE_SUCCESS {
// What should we do here ?
// Remain STAGE_SUCCESS or change to STAGE_CANCELLED
} |
Author
|
I would keep STAGE_SUCCESS. The stage really did finish and executeStage has already committed that status by the time the loop sees the cancel. Rewriting it would misstate what ran on the target. A cancel that lands while the stage is still running already becomes STAGE_CANCELLED via determineStageStatus. So the stage status stays truthful and the cancel is honored at the deployment level with DEPLOYMENT_CANCELLED plus auto rollback when it is configured. |
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
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.
What this PR does:
A cancel that lands while a stage is finishing can be ignored, the stage still reports success and the scheduler moves on. The next iteration then receives nil from the closed cancelledCh and abandons the stage it just started while rollback begins. This makes a received cancel end the deployment even when the stage reported success, and treats a nil receive as a cancellation. Same fix in piped and pipedv1.
Why we need it:
A cancelled deployment should not keep executing stages, and rollback should not run in parallel with a forward stage.
Which issue(s) this PR fixes:
Fixes #6978
Does this PR introduce a user-facing change?: