Skip to content

Clear the stale drain deadline on a re-adopted ReplicaSet - #179

Open
darkmuggle wants to merge 1 commit into
mainfrom
bh/174-drain-deadline
Open

Clear the stale drain deadline on a re-adopted ReplicaSet#179
darkmuggle wants to merge 1 commit into
mainfrom
bh/174-drain-deadline

Conversation

@darkmuggle

Copy link
Copy Markdown
Contributor

A ReplicaSet re-adopted as the current version was previously superseded, so it can still carry the removal deadline stamped on it while it drained. Nothing else clears it: the annotation patch on that path uses a different field manager, so server-side apply won't prune it, and cleanup_old_replicasets skips the current version via except_rs.

Left in place, an already-past deadline makes the next rollout tear this version down immediately instead of giving it drainDelaySeconds, stranding whatever is still pinned to it.

Refs #174

@darkmuggle
darkmuggle requested a review from lukebond August 12, 2026 15:44
@lukebond

Copy link
Copy Markdown
Contributor

you clear the stale remove-version-at with Patch::Apply({…: null}).force() under a new field manager (restate-operator/remove-version-at), while the deadline itself is stamped by a different manager. i feel like there could have been an alternative way that might be simpler, but if we go this route i have a question.

first, the question: after this fix force-removes the annotation, the operator will later have to write it back on; the next time this version is superseded, the original field manager stamps the removal deadline again. does that later write land cleanly? or does this fix having force-taken ownership of the (now-removed) annotation make that write conflict and get silently dropped, so the deadline is never written and the version never drains? i was thinking of suggesting a test for it but it would be e2e only. and i appreciate that it's only here because i asked you to split it out from #178.

the alternative: was SSA-apply chosen over a Patch::Merge with the annotation set to null? a merge-patch removes the key with no managedFields footprint, which sidesteps the ownership/re-stamp question entirely. is there a reason to prefer the SSA apply here?

@darkmuggle
darkmuggle force-pushed the bh/174-drain-deadline branch from 739ad56 to dfd4fb9 Compare August 18, 2026 19:50
A ReplicaSet re-adopted as the current version was previously superseded,
so it can still carry the removal deadline stamped on it while it drained.

This ensures that the annotation is removed via a SSA.

Refs #174
@darkmuggle
darkmuggle force-pushed the bh/174-drain-deadline branch from dfd4fb9 to e1fab7a Compare August 18, 2026 19:53
@darkmuggle

Copy link
Copy Markdown
Contributor Author

@lukebond great call out on using the field manager -- using a SSA is cleaner. Patch has been updated.

@lukebond

Copy link
Copy Markdown
Contributor

@darkmuggle i think you misread me! i can't tell what you changed due to the force-push, but what i was saying is not "use SSA and field manager" but rather "given you've used SSA and field manager, it means there is this corner case that needs to be tested; either switch away from SSA to a merge patch and not have to deal with the field manager thing, or otherwise ensure that this corner case is handled".

if you reread what i said above in that light, does it make sense? i'm a fan of SSA, i don't really mean to push back on it, and it would inconsistent to be the only bit that doesn't use it, i'm just pointing out that i can't be sure it's correct without somehow demonstrating. even writing a test for it is a lot of boilerplate as it needs to be e2e.

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.

2 participants