fix: default cleanupRemovedNodes to auto with node-removal grace period - #2696
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
How to use the Graphite Merge QueueAdd the label main-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Reviewing PR
|
|
Claude finished @rugggger's task in 5m 37s —— View job PR Review
The design is sound and the code is unusually well documented — the why comments on Blocking1. Status update after Worth addressing2. Invalid config fails open into the destructive mode · 3. 4. Cloud inference from any surviving node · 5. Grace periods are hardcoded · 6. Default flip needs a release note. Deployments without TestsSolid coverage of the state matrix (mode × node present/absent × cloud/non-cloud, plus the 40m boundary case). Two gaps:
I did not run |
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/02/26)3 reviewers were added to this PR based on Anton Bykov's automation. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if err := r.Delete(ctx, r.container); err != nil { | ||
| return err | ||
| } | ||
| delete(r.container.Status.Timestamps, nodeRemovedKey) | ||
| if err := r.Status().Update(ctx, r.container); err != nil { | ||
| return err | ||
| } | ||
| return lifecycle.NewWaitError(errors.New("backend node removed past grace period, deleting container")) |
There was a problem hiding this comment.
Status update after Delete will fail — the WaitError on line 223 is effectively unreachable.
r.Delete makes the API server write the object (adds deletionTimestamp, since WekaContainer carries a finalizer), which bumps resourceVersion. The in-memory r.container still holds the pre-delete resourceVersion, so r.Status().Update here returns a 409 Conflict (or 404 NotFound if the object had no finalizers and was reaped outright). The step therefore returns a real error instead of the intended wait error, every time grace elapses — surfacing an error status / warning event on a deletion that actually succeeded.
Also, clearing nodeRemovedKey on an object that is already being deleted has no purpose — the object is going away. The immediate-delete path at lines 121-125 gets this right (delete, then return the wait error, no status write).
| if err := r.Delete(ctx, r.container); err != nil { | |
| return err | |
| } | |
| delete(r.container.Status.Timestamps, nodeRemovedKey) | |
| if err := r.Status().Update(ctx, r.container); err != nil { | |
| return err | |
| } | |
| return lifecycle.NewWaitError(errors.New("backend node removed past grace period, deleting container")) | |
| if err := r.Delete(ctx, r.container); err != nil { | |
| return err | |
| } | |
| return lifecycle.NewWaitError(errors.New("backend node removed past grace period, deleting container")) |
Note the test at funcs_handle_node_statuses_test.go:151 doesn't catch this because it only asserts Expect(err).To(HaveOccurred()) — a conflict error and a wait error are indistinguishable to that assertion.
| func getCleanupRemovedNodesMode() CleanupRemovedNodesMode { | ||
| val := env.GetString("CLEANUP_REMOVED_NODES", string(CleanupRemovedNodesAuto)) | ||
| val = strings.ToLower(strings.TrimSpace(val)) | ||
|
|
||
| switch CleanupRemovedNodesMode(val) { | ||
| case CleanupRemovedNodesOff: | ||
| return CleanupRemovedNodesOff | ||
| case CleanupRemovedNodesOn: | ||
| return CleanupRemovedNodesOn | ||
| case CleanupRemovedNodesAuto: | ||
| return CleanupRemovedNodesAuto | ||
| default: | ||
| klog.Errorf("invalid CLEANUP_REMOVED_NODES value %q, defaulting to %q", val, CleanupRemovedNodesAuto) | ||
| return CleanupRemovedNodesAuto | ||
| } | ||
| } |
There was a problem hiding this comment.
Invalid config fails open into the destructive mode. A typo (autoo, on, 1, yes) logs an error and then falls back to auto, i.e. the operator will start deleting backend containers after the grace period. For a setting that destroys state, failing closed (off) — or refusing to start — is the safer contract; auto should only be reached when the value is genuinely absent.
Two separate cases are also conflated here: unset/empty (expected — the chart renders value: "" if a user sets cleanupRemovedNodes: "" or null) and garbage. The empty case logging klog.Errorf("invalid CLEANUP_REMOVED_NODES value \"\"") at every operator start is noise. Suggest short-circuiting val == "" to the default before the switch, and reserving the error log (fail-closed) for actual garbage.
| // gone). This runs before the deactivate/resign steps precisely so it is not blocked by them. | ||
| &lifecycle.SimpleStep{ | ||
| Name: "ReleaseTerminalPodOnDeletion", | ||
| Run: r.releaseTerminalPodOnDeletion, | ||
| Predicates: lifecycle.Predicates{ | ||
| lifecycle.IsNotFunc(r.PodNotSet), | ||
| r.container.IsBackend, | ||
| func() bool { | ||
| return r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Two things on this step:
-
The predicate is broader than the problem it solves. The motivating case is a terminal pod wedged in
Terminatingbehind the deactivate steps. As written it fires for any terminal backend pod during deletion, including the ordinary teardown where the pod exitsSucceededafterweka local stop. In that path the pod is deleted here, thenensurePodOnDeletion(line ~76, predicatePodNotSet && NodeIsSet && ShouldDeactivate && !Deactivated) recreates it for deactivation — delete/recreate churn on the healthy path, and a potential recreate →Failed→ reap → recreate cycle when the pod cannot stay up. Narrowing tor.pod.GetDeletionTimestamp() != nil(or additionally gating on node gone / NotReady) targets the wedged case precisely. -
The third predicate dereferences
r.podand relies on the engine short-circuitingIsNotFunc(r.PodNotSet)first. Other predicates in this file guard inline (r.wekaClient != nil && ...), which suggests not depending on evaluation order:
| // gone). This runs before the deactivate/resign steps precisely so it is not blocked by them. | |
| &lifecycle.SimpleStep{ | |
| Name: "ReleaseTerminalPodOnDeletion", | |
| Run: r.releaseTerminalPodOnDeletion, | |
| Predicates: lifecycle.Predicates{ | |
| lifecycle.IsNotFunc(r.PodNotSet), | |
| r.container.IsBackend, | |
| func() bool { | |
| return r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed | |
| }, | |
| }, | |
| &lifecycle.SimpleStep{ | |
| Name: "ReleaseTerminalPodOnDeletion", | |
| Run: r.releaseTerminalPodOnDeletion, | |
| Predicates: lifecycle.Predicates{ | |
| lifecycle.IsNotFunc(r.PodNotSet), | |
| r.container.IsBackend, | |
| func() bool { | |
| return r.pod != nil && (r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed) | |
| }, | |
| }, | |
| }, |
| func (r *containerReconcilerLoop) removedNodeOnSupportedCloud(ctx context.Context, node *v1.Node) (bool, error) { | ||
| if node != nil { | ||
| return discovery.IsSupportedCloudProvider(node.Spec.ProviderID), nil | ||
| } | ||
|
|
||
| nodes, err := r.KubeService.GetNodes(ctx, nil) | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to list nodes to determine cloud provider: %w", err) | ||
| } | ||
| for i := range nodes { | ||
| if discovery.IsSupportedCloudProvider(nodes[i].Spec.ProviderID) { | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Two concerns with inferring the provider from surviving nodes:
- Correctness of the homogeneity assumption: any single node in the cluster with an aws/oci ProviderID makes every removed node "managed cloud" and shortens its grace from 24h to 30m. Mixed clusters (bare-metal Weka backends alongside a cloud node pool, or self-managed instances in EKS registered without a cloud provider) get the wrong — and more aggressive — window. The removed node's own ProviderID is available while the node exists; capturing it once (annotation or status field, alongside the
NodeRemovedstamp) would remove the guess entirely and make the decision stable across the whole window. - Cost: this runs on the fallback path on every reconcile — the grace loop requeues every 30s, for up to 24h, per Stale container — and each call does an unfiltered
Listof all nodes. Cached or not, computing this once when the stamp is written and reusing it avoids the repetition.
| err := r.deleteIfNoNode(context.Background()) | ||
|
|
||
| Expect(err).To(HaveOccurred()) | ||
| Expect(containerExists(r)).To(BeTrue()) | ||
| Expect(container.Status.Status).To(Equal(weka.Stale)) | ||
| Expect(container.Status.Timestamps).To(HaveKey(nodeRemovedKey)) | ||
| }) |
There was a problem hiding this comment.
Every path here asserts only Expect(err).To(HaveOccurred()), which cannot distinguish the intended lifecycle.WaitError from a genuine failure — that's precisely what hides the post-Delete status-update conflict flagged on funcs_handle_node_statuses.go:216-222. Worth asserting the error is a wait error (and ideally its requeue duration), so a real error can't silently pass as "expected".
|
|
||
| const ( | ||
| nodeRemovedKey = "NodeRemoved" | ||
| nodeRemovalGracePeriod = 24 * time.Hour // non-cloud: node may return slowly | ||
| nodeRemovalGracePeriodManagedCloud = 30 * time.Minute // managed cloud (aws/oci); aligns w/ managedNodesPodTerminationTimeout |
There was a problem hiding this comment.
Two notes on these constants:
- Not configurable. Given the two-orders-of-magnitude spread (30m vs 24h) and that this is the new default behavior, an env override (e.g.
NODE_REMOVAL_GRACE_PERIOD) would make field triage much easier than shipping a new operator image to change it. - 24h keeps the cluster degraded for a day. During the Stale window the container is neither deactivated nor resigned, so its drives stay in the cluster and the failure domain stays down. That's the deliberate trade-off for "the node might come back", but on-prem 24h is a long time to defer redundancy restoration — is that the intended product decision, or would a few hours (with
trueavailable for operators who want immediate cleanup) be a better default?
Also: nodeRemovedKey is a bare string, while the other timestamp keys in this package come from typed API constants (weka.TimestampStopAttempt). Since it's persisted into Status.Timestamps on real objects, promoting it to the API constants alongside the others would keep it discoverable.
141ba68 to
4b23882
Compare
|
Claude finished @rugggger's task in 3m 33s —— View job Re-review (revision 2)
The branch was amended since my 11:34 review. The only code change in this revision is the fix to finding #1, and it's correct: if err := r.Delete(ctx, r.container); err != nil {
return err
}
return lifecycle.NewWaitError(errors.New("backend node removed past grace period, deleting container"))
Still open from the previous round
On #2 specifically, worth confirming: Tests were not updated for the assertion gap: every grace-path case still asserts only New in this passReap gate keys on phase, not on VerdictThe blocking bug is resolved. Nothing else new is blocking — #2 (fail-open on a destructive default) is the one I'd still want addressed before this ships as the default, since it turns a config typo into eventual backend deletion. I did not run |
| // The backend pod on a removed node has no live process and holds no drives; reap it immediately | ||
| // (strip the do-not-force-delete finalizer + delete) so it is not stuck in Terminating for the whole | ||
| // Stale window. The container itself stays Stale — only the dead pod object is removed. | ||
| if r.pod != nil && (r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed) { |
There was a problem hiding this comment.
The reap gate is phase-based (Succeeded/Failed), but the pod this is meant to unstick is defined by its deletionTimestamp, not its phase.
When a Node object is deleted, PodGC force-deletes the pods bound to it. On recent Kubernetes it patches status.phase = Failed first (so this predicate matches), but the pod that is actually wedged is any pod carrying deletionTimestamp + WekaFinalizer — and a pod whose kubelet vanished before the status patch lands, or one force-deleted by an admin/ASG tooling, can sit in Terminating while still reporting phase Running. Those linger with the finalizer for the entire Stale window (up to 24h) — exactly the state this block exists to prevent.
deletePod already handles the already-Terminating case correctly (strips the finalizer before the deletionTimestamp short-circuit), so widening the gate costs nothing:
| if r.pod != nil && (r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed) { | |
| if r.pod != nil && (r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed || r.pod.GetDeletionTimestamp() != nil) { |
(Same observation applies to the ReleaseTerminalPodOnDeletion predicate in flow_deleting_state.go — there deletionTimestamp != nil is arguably the only condition you want, per my earlier comment.)
4b23882 to
0acded3
Compare
|
Claude finished @rugggger's task in 3m 37s —— View job Re-review (revision 3)
Three of the open findings were addressed in this revision, and the fixes are the right ones. Resolved#2 — config now fails closed · case CleanupRemovedNodesAuto, "": // "" == set-but-empty, treat as default
return CleanupRemovedNodesAuto
default:
klog.Warningf("invalid CLEANUP_REMOVED_NODES value %q, disabling removed-node cleanup; ...")
return CleanupRemovedNodesOffSet-but-empty (what the chart renders for #3 — return r.pod.GetDeletionTimestamp() != nil &&
(r.pod.Status.Phase == v1.PodSucceeded || r.pod.Status.Phase == v1.PodFailed)This targets the actual wedge (pod held in #6 — docs / release note ✅ New in this passDoc drift on Still open
On the reap gate: now that the deleting flow requires On the test assertions: VerdictNothing blocking. The remaining items are hardening (#4, #5, the phase-vs-deletionTimestamp gate) and test-assertion strength; the fail-open config issue that concerned me about shipping this as the default is fixed. I did not run |
| } | ||
|
|
||
| // releaseTerminalPodOnDeletion strips the do-not-force-delete finalizer from, and reaps, a backend pod | ||
| // that has already reached a terminal phase (Succeeded/Failed) while its container is being deleted. |
There was a problem hiding this comment.
Doc drift after the predicate narrowing: the caller now also requires deletionTimestamp != nil (flow_deleting_state.go:72), but this doc block still describes the trigger as "has already reached a terminal phase (Succeeded/Failed) while its container is being deleted". Anyone reading this function in isolation would conclude any terminal pod gets reaped here, which is exactly the behavior the new gate deliberately excludes.
| // that has already reached a terminal phase (Succeeded/Failed) while its container is being deleted. | |
| // that has already reached a terminal phase (Succeeded/Failed) and is already being force-removed | |
| // (deletionTimestamp set) while its container is being deleted. A pod that merely exited mid-teardown | |
| // without a deletionTimestamp is left to the normal flow — see the predicate in flow_deleting_state.go. |
0acded3 to
c28cdb7
Compare
|
Claude finished @rugggger's task in 1m 59s —— View job Re-review (revision 4) — no code change since my last pass
This push is a rebase onto current
The only shift is in Rebase-safety checks I did run
Still open (carried over, all non-blocking)
I did not run |
c28cdb7 to
7aab4a2
Compare
|
Claude finished @rugggger's task in 1m 36s —— View job Re-review (revision 5) — rebase only, no content change
This push is another rebase onto current
So the logic needs no re-review — my revision-3 verdict stands: nothing blocking. Rebase-safety checks I did run
Still open (carried over, all non-blocking)
The commit message now carries an explicit I did not run |
rugggger
changed the title
7aab4a2 to
7a245a9
Compare
Re-review (revision 6)
|
7a245a9 to
ef8695e
Compare
|
Claude finished @rugggger's task in 1m 29s —— View job Re-review (revision 6) — rebase only, no content change
This push is another rebase, this time onto a
So the logic needs no re-review — my revision-3 verdict stands: nothing blocking. Rebase-safety checks I did run
Still open (carried over, all non-blocking)
I did not run |
ef8695e to
d91dba2
Compare
|
Claude finished @rugggger's task in 1m 32s —— View job Re-review (revision 7) — rebase only, no content change
This push is another rebase, now onto
So no re-review of the logic is warranted — my revision-3 verdict stands: nothing blocking. Rebase-safety checks I did runFour
Still open (carried over, all non-blocking)
I did not run |
Introduce a tri-state cleanupRemovedNodes config (false/true/auto) and make auto the shipped default. Under auto, a backend WekaContainer whose target node disappears from the cluster is held in Stale status for a grace period before deletion, instead of being torn down immediately: - managed cloud (AWS/EKS, OCI/OKE): 30m grace, aligned with managedNodesPodTerminationTimeout - everything else: 24h grace If the node returns within the window the grace stamp is cleared and the container recovers to Running, so a transient node loss (reboot, brief API-server blip, ASG churn) no longer forces an unnecessary deactivate/resign/rebuild. "true" keeps the previous immediate-delete behavior; "false" disables cleanup entirely. Cloud-provider detection falls back to inferring from surviving cluster nodes once the removed node object is gone. While a container sits Stale, its already-exited backend pod is reaped immediately (do-not-force-delete finalizer stripped + deleted) instead of lingering in Terminating for the whole window. Also reap a terminal backend pod at the start of the deleting flow: a new ReleaseTerminalPodOnDeletion step strips the do-not-force-delete finalizer from a Succeeded/Failed backend pod and removes it before the deactivate/resign steps, so it is not wedged in Terminating behind a DeactivateWekaContainer that can never complete once the node/process is gone. Note: CLEANUP_REMOVED_NODES now defaults to "auto" instead of the previous effective "false". Deployments that do not set cleanupRemovedNodes explicitly will now delete a backend WekaContainer after its node has been gone for the grace period (30m on managed cloud, 24h otherwise), whereas previously such containers were never deleted on node removal. Set cleanupRemovedNodes: "false" to retain the old behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d91dba2 to
3d831de
Compare
|
Claude finished @rugggger's task in 50s —— View job Re-review (revision 8) — no change since my last pass
This push is a no-op re-push: the branch base is still Spot-checked every construct I've been tracking; all byte-identical at the same lines:
Per the exit-early instruction, no re-review of the logic — my revision-3 verdict stands: nothing blocking. Still open (carried over, all non-blocking)
The doc-drift item on I did not run |
1 participant
Introduce a tri-state
cleanupRemovedNodesconfig (false/true/auto) and makeautothe shipped default.Node-removal grace (
auto)Under
auto, a backend WekaContainer whose target node disappears from the cluster is held inStalestatus for a grace period before deletion, instead of being torn down immediately:managedNodesPodTerminationTimeoutIf the node returns within the window the grace stamp is cleared and the container recovers to
Running, so a transient node loss (reboot, brief API-server blip, ASG churn) no longer forces an unnecessary deactivate/resign/rebuild.truekeeps the previous immediate-delete behaviorfalsedisables cleanup entirelyWhile a container sits
Stale, its already-exited backend pod is reaped immediately (do-not-force-delete finalizer stripped + deleted) instead of lingering inTerminatingfor the whole window.Terminal-pod reap on deletion
A new
ReleaseTerminalPodOnDeletionstep at the start of the deleting flow strips the do-not-force-delete finalizer from aSucceeded/Failedbackend pod and removes it before the deactivate/resign steps, so it is not wedged inTerminatingbehind aDeactivateWekaContainerthat can never complete once the node/process is gone.Tests
internal/config/cleanup_removed_nodes_test.go— mode parsing / defaultfuncs_handle_node_statuses_test.go— Stale grace, cloud vs non-cloud boundary, node-return recovery, reap-during-Stalefuncs_common_deletion_finalizer_test.go— finalizer strip + terminal-pod reap🤖 Generated with Claude Code