fix(allocator): rebind replacement Pod when allocated Pod is deleted externally#1048
fix(allocator): rebind replacement Pod when allocated Pod is deleted externally#1048ashishpatel26 wants to merge 2 commits into
Conversation
…externally When a Pod recorded in the alloc-status annotation no longer exists in the pool's live Pod set (manual delete, node eviction, OOM kill), exclude it from the effective-allocation count so that supplement > 0 and the pool schedules a replacement. Previously, stale annotation entries kept supplement at zero, leaving the BatchSandbox permanently unavailable. Fixes opensandbox-group#954
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.
This PR fixes a scheduling edge case where sandboxes could fail to replace pods that were deleted externally but still recorded in allocation annotations/state, by excluding stale allocation entries from the effective allocation count.
Changes:
- Compute a live-pod name set from the pool’s current pod list and thread it through request construction.
- Recompute sandbox “effective allocated” based on live pods so supplement triggers replacement when allocations are stale.
- Add a regression test covering externally deleted allocated pods (issue #954).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| kubernetes/internal/controller/allocator.go | Builds/uses livePodSet to ignore stale alloc-status pod names when computing supplement. |
| kubernetes/internal/controller/allocator_test.go | Adds regression test ensuring replacements are scheduled when allocated pods were deleted externally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Info("Detected stale allocation entry: pod no longer exists, excluding from effective count", | ||
| "sandbox", sandbox.Name, "pod", p) | ||
| } |
| // Count only pods that still exist in the live pool pod set. | ||
| // Pods deleted externally (eviction, OOM kill, manual delete) remain in the alloc-status | ||
| // annotation but are absent from livePodSet; excluding them makes supplement > 0 so the | ||
| // pool schedules a replacement (fixes issue #954). | ||
| effectiveAllocated := int32(0) | ||
| for _, p := range allocated { | ||
| if _, alive := livePodSet[p]; alive { | ||
| effectiveAllocated++ | ||
| } else { | ||
| log.Info("Detected stale allocation entry: pod no longer exists, excluding from effective count", | ||
| "sandbox", sandbox.Name, "pod", p) | ||
| } | ||
| } | ||
|
|
||
| supplement := int32(0) | ||
| if replica-int32(len(allocated)) > 0 { | ||
| supplement = replica - int32(len(allocated)) | ||
| if replica-effectiveAllocated > 0 { | ||
| supplement = replica - effectiveAllocated | ||
| } |
![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: 546af02017
ℹ️ 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".
| if replica-effectiveAllocated > 0 { | ||
| supplement = replica - effectiveAllocated |
There was a problem hiding this comment.
When an allocated pod disappears and a replacement is scheduled, this path only lowers the supplement count; it still returns the original allocated slice, so getLatestAllocated appends the new pod to the stale annotation and SyncSandboxAllocation writes both the deleted pod and the replacement back into the in-memory pool allocation. In the inspected pool reconcile flow, LatestAllocation is then used for allocatedCnt and status, so a one-replica sandbox whose old pod was deleted can permanently look like it has two allocated pods and can over-scale/retain extra idle pods. The stale names should be removed from the allocation being persisted or released from the store when they are detected.
Useful? React with 👍 / 👎.
|
Thanks for the reviews. Addressing feedback: Codex P2 / Copilot – Return filtered Copilot – Log verbosity for stale pods: Valid in busy clusters. Will downgrade the per-pod stale log to |
Summary
In Pool mode, when an allocated Pod is deleted externally (manual delete, node eviction, OOM kill), the BatchSandbox never recovers.
Root cause:
getSandboxRequestcomputedsupplement = replicas - len(allocated)whereallocatedcame from thealloc-statusannotation — stale pod names stayed in the annotation after deletion, keepingsupplement = 0.A TODO comment in the source confirms this gap:
// TODO consider supply Pods if Pods is deleted unexpectedlyFix: build a
livePodSetfromspec.Pods(the pool's live pod list) and count only pods present in that set as effectively allocated. Stale annotation entries are excluded, makingsupplement > 0and triggering re-allocation.Test plan
go test ./internal/controller/... -run TestSchedule— new table case: allocated pod absent from live set → supplement = 1, replacement scheduledFixes #954