fix(k8s): implement BatchSandbox scale-down for both pooled and non-pooled modes#1159
Conversation
…ooled modes Previously, reducing spec.replicas on a BatchSandbox had no effect on the actual pod count: - Non-pooled mode: scaleBatchSandbox() only iterated 0..replicas to find pods to create, never found pods with index >= replicas to delete. - Pooled mode: getSandboxRequest() computed supplement based on allocated (full historical list including released pods) and never computed toRelease for scale-down. Changes: - Non-pooled: find pods with index >= replicas and delete them directly via r.Delete(). No ScaleExpectation is set because there is no watch mechanism to observe the actual deletion event. Return early after scale-down to avoid mixing create/delete in one reconcile cycle. - Pooled: compute activeAllocated by excluding alloc-released (recycle completed) and alloc-release (recycle in progress) from allocated. Use activeAllocated for supplement and excess calculations. Add excess active pods to ToRelease so the Pool controller can recycle them. Co-Authored-By: Claude <noreply@anthropic.com>
yuyujiang92-code
requested review from
Generalwin,
Pangjiping,
Spground,
fengcone,
hittyt,
kevinlynx and
ninan-nn
as code owners
![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: 0ce52cb6ee
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if replica-int32(len(activeAllocated)) > 0 { | ||
| supplement = replica - int32(len(activeAllocated)) |
There was a problem hiding this comment.
When alloc-release already contains a pod that is not yet in alloc-released (for example after task completion or a prior scale-down), activeAllocated excludes that pod, but this condition still sets PodSupplement. Because PoolReconciler.scheduleSandbox applies ToAllocate before ToRelease (kubernetes/internal/controller/pool_controller.go:613-638), the same reconcile can allocate/create a replacement before the old pod is recycled, violating the intended release-then-allocate state machine and temporarily exceeding spec.replicas. Add the missing pending-release guard (e.g. len(toRelease)==0) before supplementing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 46f73f6. Supplementation and pooled scale-down release generation are now gated on len(toRelease) == 0, so the allocator will not request replacement pods or queue additional scale-down releases while release work is pending. I also adjusted activeAllocated so pods in alloc-release still count as allocated until recycle completes and they move to alloc-released, preserving the release-then-allocate state machine.
| // calling ObserveScale immediately after r.Delete would be a no-op. | ||
| // Duplicate Delete calls on an already-terminating pod are harmless — | ||
| // the API server returns NotFound which we handle below. | ||
| if err := r.Delete(ctx, pod); err != nil { |
There was a problem hiding this comment.
When a non-pooled BatchSandbox with a TaskTemplate is scaled down, this delete removes the high-index pod but the existing in-memory task scheduler is never trimmed; getTaskScheduler only updates pods and adds new task specs, while StopTask only runs during BatchSandbox deletion. If the sandbox is later scaled back up, the old task node still has the deleted pod name and stale IP, so the newly recreated pod with the same deterministic name is treated as already assigned and the task keeps polling/scheduling against the dead endpoint. Mark surplus tasks releasing or remove them before deleting their pods.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 46f73f6. Non-pooled scale-down now clears the in-memory task scheduler before deleting surplus pods, so stale task-to-pod/IP bindings are not reused if the BatchSandbox later scales back out.
| if _, isReleased := releasedSet[pod]; isReleased { | ||
| continue |
There was a problem hiding this comment.
For pools that keep released pods alive, such as Noop recycle, the same pod can be returned to the available set and allocated back to the same BatchSandbox. SyncSandboxAllocation appends that new allocation to alloc-status but does not remove the pod from the append-only alloc-released annotation, so this check continues to drop every occurrence of the reallocated pod from activeAllocated; subsequent schedules think the sandbox is still short and keep requesting/allocating extra pods. Clear the released marker for newly allocated pod names or track allocations with a generation rather than filtering only by pod name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. I intentionally did not address this in 46f73f6 to keep this patch focused on release gating, BatchSandbox pod listing, helper naming, and non-pooled scale-down scheduler cleanup. Reallocating a previously released pod needs coordinated changes in getLatestAllocated and SyncSandboxAllocation, plus pod-reuse regression coverage, so I plan to handle it in a follow-up patch.
| toRelease = append(toRelease, activeAllocated[i]) | ||
| excessCnt-- |
There was a problem hiding this comment.
These scale-down releases are only returned as an allocator action and are never written to the alloc-release annotation; after recycle, SyncSandboxReleased writes alloc-released instead. BatchSandboxReconciler.listPods filters pooled pods using parseSandboxReleased, which reads alloc-release, so when the pool uses a recycle strategy that leaves pods alive (for example Noop or successful restart), the scaled-down pod remains in the BatchSandbox status and endpoints indefinitely even though it was released from the pool allocation. Persist the release request or have the BatchSandbox pod listing also exclude completed releases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 46f73f6 via the BatchSandbox pod listing path. listPods now excludes both alloc-release and alloc-released in pooled mode, so completed releases are no longer kept in BatchSandbox status/endpoints just because they still appear in alloc-status. I also renamed the helpers so parseSandboxRelease reads alloc-release and parseSandboxReleased reads alloc-released.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46f73f6732
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| excessCnt := int32(len(activeAllocated)) - replica | ||
| // Release from the end of the active list (highest index first). | ||
| for i := len(activeAllocated) - 1; i >= 0 && excessCnt > 0; i-- { | ||
| toRelease = append(toRelease, activeAllocated[i]) |
There was a problem hiding this comment.
When a pooled BatchSandbox has a TaskTemplate, this new scale-down path returns pods to the pool without updating or deleting its in-memory TaskScheduler. getTaskScheduler only calls UpdatePods/AddTasks and never removes task nodes above the current replica count, so after a Noop or Restart recycle the old task node keeps the released pod's IP and can keep setting work into a pod that is idle or reallocated to another sandbox. Stop or trim the scheduler tasks before queuing these pooled scale-down releases.
Useful? React with 👍 / 👎.
| // handle scale-up (if needed). This avoids creating pods in the same reconcile | ||
| // that deletes them, which could confuse expectation tracking. | ||
| if len(needDeleteIndex) > 0 { | ||
| return nil |
There was a problem hiding this comment.
Fresh evidence after the prior scheduler fix: this returns only from scaleBatchSandbox, while Reconcile continues with the pre-delete pods slice and immediately calls reconcileTasks. In a task-scheduled non-pooled sandbox scaled from 2 to 1 while the retained pod has no IP but the deleted high-index pod does, the newly recreated scheduler can assign the remaining task to the just-deleted pod and then keep polling that dead IP after the pod disappears. Have the caller stop reconciliation or relist pods after scale-down deletes.
Useful? React with 👍 / 👎.
1 participant
Summary
What is changing:
Implement scale-down logic for BatchSandbox in both pooled and non-pooled modes. When
spec.replicasis reduced, excess pods are now properly deleted (non-pooled) or returned to the pool (pooled).Why:
Previously, reducing
spec.replicason a BatchSandbox had no effect on the actual pod count:scaleBatchSandbox()only iterated0..replicasto find pods to create, never found pods withindex >= replicasto delete. Excess pods continued running indefinitely, wasting cluster resources.getSandboxRequest()had two issues:allocatedvariable comes from thealloc-statusannotation, which is a append-only historical record containing all pods ever allocated, including those already released. Thealloc-statusannotation never shrinks — even after a pod is returned to the pool (tracked inalloc-released), it remains in this list. Usingallocateddirectly forsupplementcalculation caused already-released pods to be double-counted, overestimating the actual active allocation.toReleasefor scale-down, so the Pool allocation map was never updated, andPool.Status.Allocatedstayed stale after replicas was reduced.Impact:
r.Delete().activeAllocated = allocated - released - releasecomputes the true active allocation by excluding pods inalloc-released(recycle completed) andalloc-release(recycle in progress). Excess pods are added toToReleasefor the Pool controller to recycle, andPool.Status.Allocatedcorrectly decreases.Testing
TestSchedule,TestSpreadSchedule,TestPackedSchedule,Test_parseIndex,Test_calPodIndex)go build ./internal/controller/)Breaking Changes
Checklist
batchsandbox_controller.go:532andallocator.go:575)replicasis reduced, no impact on existing behaviorChanges Detail
Non-pooled mode (
batchsandbox_controller.go, +46/-2)Problem:
scaleBatchSandbox()only iterated0..replicasto find pods to create. Pods withindex >= replicaswere never deleted.Fix:
needCreateIndex, also findneedDeleteIndex(pods withindex >= replicas).ScaleExpectationfor delete — there is no watch-based mechanism to observe the actual deletion event; callingObserveScaleimmediately afterr.Deletewould be a no-op. DuplicateDeletecalls on an already-terminating pod are harmless (API server returnsNotFound).Pooled mode (
allocator.go, +43/-2)Problem: The
allocatedvariable ingetSandboxRequest()comes from thealloc-statusannotation, which is an append-only historical record containing all pods ever allocated — including those already released (tracked inalloc-released) or currently being released (tracked inalloc-release). Usingallocateddirectly forsupplementcalculation caused already-released pods to be double-counted.Fix:
activeAllocatedby excluding pods inalloc-released(recycle completed) andalloc-release(recycle in progress) fromallocated, yielding the true current active allocation.supplementbased onactiveAllocatedinstead ofallocated. Only compute whenlen(toRelease) == 0to preserve the state machine semantics that release and allocate do not happen in the same reconcile cycle.activeAllocated > replicaand no pending releases, compute excess pods from the tail ofactiveAllocatedand add them toToReleasefor the Pool controller to recycle.Files changed:
kubernetes/internal/controller/batchsandbox_controller.go(+46/-2)kubernetes/internal/controller/allocator.go(+43/-2)**Total: 89 lines added, 4 lines removed