-
Notifications
You must be signed in to change notification settings - Fork 28
Cleanup: fix deletion hang #176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,4 +123,7 @@ fmt: | |
| lint: | ||
| cargo clippy | ||
|
|
||
| test: | ||
| cargo test --all-targets | ||
|
|
||
| check: fmt lint | ||
86 changes: 86 additions & 0 deletions
86
release-notes/unreleased/172-restatedeployment-deletion-hang.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| # Release Notes for Issue #172: RestateDeployment deletion no longer hangs forever | ||
|
|
||
| ## Bug Fix | ||
|
|
||
| ### What Changed | ||
|
|
||
| Deleting a `RestateDeployment` no longer blocks indefinitely on its own latest version. | ||
|
|
||
| Cleanup previously asked Restate a single question — "is this deployment active?" — where | ||
| "active" meant *either* that a service still pointed at the deployment *or* that it had | ||
| unfinished invocations. During a rollout that conflation is harmless: a newer version | ||
| eventually takes over as the service's endpoint and the old one goes inactive. During a | ||
| deletion it was fatal. Nothing is coming to supersede the endpoint, so the latest version | ||
| stayed "active" forever, cleanup never deregistered it, `active_count` never reached zero, | ||
| and the finalizer requeued every 30 seconds without end. | ||
|
|
||
| The operator now tracks the two facts separately and weighs them by why cleanup is | ||
| running. Being a service's current endpoint holds a version through a rollout but is | ||
| ignored during a deletion; only unfinished invocations — which drain on their own — can | ||
| hold a deletion. The version then goes through the normal drain, deregistration and | ||
| teardown path. | ||
|
|
||
| Alongside that: | ||
|
|
||
| - **Unfinished invocations not yet bound to a deployment now count.** Paused, queued and | ||
| scheduled work carries no `pinned_deployment_id`, so the old query scored it as zero and | ||
| a deletion could tear the endpoint out from under it. It is now attributed through the | ||
| target service. Only a deletion asks: that attribution costs a second scan of | ||
| `sys_invocation_status` and a per-row decode of the invocation target, and during a | ||
| rollout it can only ever name the deployment that is already the service's endpoint. The | ||
| reconcile path's query is unchanged. | ||
| - **A blocked deletion says what is blocking it.** The `DeploymentInUse` event now names | ||
| each version and its pinned/unpinned invocation counts instead of a generic message. | ||
| - **A blocked deletion backs off.** Retries start at the usual 30 seconds and stretch to | ||
| five minutes the longer the wait runs, so a deletion parked behind a scheduled invocation | ||
| days out stops re-running that query twice a minute for the duration. | ||
| - **Drain deadlines are now honoured.** The requeue interval derived from a version's | ||
| remove-at time was being discarded, because errors reach the controller's error policy | ||
| wrapped by the finalizer machinery; a short `drainDelaySeconds` cost up to 30 seconds per | ||
| version regardless. It is now unwrapped, and floored at one second so a sub-second | ||
| deadline cannot spin the reconciler. | ||
| - **A draining version keeps its autoscaler.** Removal of an inactive version's | ||
| operator-managed HPA moved to the point where the version is actually scaled to zero. | ||
| Previously a deletion stripped the HPA from every version on the first reconcile, while | ||
| those versions were still serving traffic for the whole drain window. | ||
|
|
||
| ### Why This Matters | ||
|
|
||
| Before this fix, `kubectl delete restatedeployment` never returned for a | ||
| `RestateDeployment` whose services were still registered — which is the normal state of any | ||
| healthy deployment. The only workaround was to remove the finalizer by hand, which left the | ||
| deployment registered in Restate with no pods behind it. Namespace deletion inherited the | ||
| same hang. | ||
|
|
||
| ### Impact on Users | ||
|
|
||
| - **Existing deployments:** no configuration change. Deletions that were previously wedged | ||
| will proceed the next time the operator reconciles them; ones whose finalizer was removed | ||
| by hand may have left a stale registration behind in Restate. | ||
| - **Deletion now takes at least `spec.restate.drainDelaySeconds` (default 300s).** The | ||
| latest version is put through the same drain as any superseded version, so teardown waits | ||
| out the drain window even when the deployment never served an invocation. This is the | ||
| interval that was previously unbounded. | ||
| - **Deletion still waits on unfinished invocations, and that wait has no upper bound.** | ||
|
darkmuggle marked this conversation as resolved.
|
||
| Scheduled invocations are the sharp edge: a delayed call whose execution time is days out | ||
| counts as unfinished and holds the deletion until it fires. The `DeploymentInUse` event | ||
| reports the counts so this is diagnosable. Bounding or skipping the wait is the job of the | ||
| planned `deletionPolicy` field, not of manual intervention. | ||
| - **New deployments:** no impact. | ||
|
|
||
| ### Migration Guidance | ||
|
|
||
| None required. If a `RestateDeployment` is currently stuck deleting, upgrading the operator | ||
| is sufficient — no manual finalizer edits. | ||
|
|
||
| To check what is holding a deletion: | ||
|
|
||
| ```bash | ||
| kubectl describe restatedeployment <name> -n <namespace> | ||
| # Warning FailedReconcile ... This RestateDeployment is backing active versions in | ||
| # Restate: greeter-7f9c4d (0 pinned, 3 unpinned invocations). ... | ||
| ``` | ||
|
|
||
| ### Related Issues | ||
|
|
||
| - Issue #172: RestateDeployment finalizer never completes | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very neat!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this lands, the next PR is for a richer status including the inflight innovations.