fix(recycle): delete pod on release when NoopRecycler is used#1046
fix(recycle): delete pod on release when NoopRecycler is used#1046ashishpatel26 wants to merge 2 commits into
Conversation
When a sandbox backed by a pre-warmed pool is destroyed, the pod was returned directly to the pool without being rebuilt because NoopRecycler returned StateSucceeded immediately — causing the pod (with stale user data) to be treated as available for the next sandbox. NoopRecycler now sets NeedDelete=true so the pod is deleted on release, matching the behaviour of DeleteRecycler. The pool controller then creates a fresh replacement pod, ensuring no data from a previous run persists. Fixes opensandbox-group#743
ashishpatel26
requested review from
Generalwin,
Pangjiping,
Spground,
fengcone,
hittyt,
kevinlynx and
ninan-nn
as code owners
|
Changed directories: kubernetes. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the Kubernetes “noop” recycle handler to avoid reusing potentially stale pods by signaling the caller to delete existing pods, and aligns unit tests with the new state-machine behavior.
Changes:
- Change
NoopRecycler.TryRecycleto returnStateRecyclingwithNeedDelete=truewhen a pod still exists. - Treat nil / already-deleting pods as
StateSucceeded. - Expand
noop_test.goto cover the new expected states andNeedDeletebehavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| kubernetes/internal/controller/recycle/noop.go | Updates Noop recycler semantics to request pod deletion for existing pods and adjusts status/messages. |
| kubernetes/internal/controller/recycle/noop_test.go | Updates tests to assert the new status state machine and deletion intent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if pod == nil || pod.DeletionTimestamp != nil { | ||
| return &Status{ | ||
| State: StateSucceeded, | ||
| Message: "noop recycler: pod is deleted", | ||
| }, nil | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f5ca47a13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| State: StateRecycling, | ||
| Message: "noop recycler: pod marked for deletion", | ||
| NeedDelete: true, |
There was a problem hiding this comment.
When a Pool explicitly sets recycleStrategy.type: Noop, the public CRD contract still says RecycleTypeNoop does nothing, and the e2e flow at kubernetes/test/e2e/e2e_test.go:2550 verifies those pods are not deleted. Returning NeedDelete: true for every live pod makes collectRecycleResults pass the pod to scalePool, which deletes it, so Noop now behaves like Delete and breaks users/tests that selected Noop to keep warm pods alive; if delete-on-release is the desired behavior, the public contract and Noop-specific tests need to change together rather than silently redefining this enum.
Useful? React with 👍 / 👎.
|
Thanks for the reviews. Addressing feedback: Codex P1 / Copilot – Noop contract and e2e tests: This is the core design question. The original If the pool's Requesting maintainer guidance: should Copilot – |
A pod with DeletionTimestamp is still running/terminating. Returning StateSucceeded allowed the pool to re-allocate the pod before it fully disappeared, risking data leakage between sandbox runs.
Summary
NoopRecycler.TryRecyclereturnedStateSucceeded + NeedDelete: falsefor any pod, including live onescollectRecycleResultstreatedStateSucceededas "recycling complete → return pod to pool", so the pod re-entered the pool carrying all state from the previous sandbox runStateRecycling + NeedDelete: truefor live pods (matchingDeleteRecyclersemantics); the pool controller deletes the pod and scales up a fresh replacementTest plan
go test ./internal/controller/recycle/...— 3 cases: nil pod → Succeeded; pod with DeletionTimestamp → Succeeded; live pod → Recycling + NeedDeleteFixes #743