Repository navigation
Conversation
scalePool only scaled in when the desired pool size, max(allocated + supply + desired buffer, poolMin), dropped below the current size. poolMax was applied only to scale-up, as a limit on new pods. When poolMax is lowered below what the buffer band or poolMin asks for, the pool stops growing but keeps every idle pod it already has: with four ready idle pods and bufferMax 4, setting poolMax to 2 leaves four pods running indefinitely, although poolMax is documented as the bound on the whole pool. Scale in down to min(desired, poolMax). pickPodsToDelete only picks idle pods, so allocated pods are never deleted to meet the bound, and the existing delete batch limit still paces the scale-in. Scale-up is unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
component/k8s
![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: 0c5f8cbbc3
ℹ️ 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 target := min(desiredSchedulableCnt, pool.Spec.CapacitySpec.PoolMax); target < schedulableCnt { | ||
| scaleIn = schedulableCnt - target |
There was a problem hiding this comment.
When lowering poolMax coincides with a recycle or rolling-update entry in toDeletePods, this computes the full schedulableCnt - target even though that planned deletion is included in schedulableCnt. pickPodsToDelete appends all explicit deletions and then selects the full scaleIn again; with five idle pods, bufferMin: 4, poolMax: 4, one recycle deletion, and a delete budget of at least two, it deletes two pods, drops below the buffer minimum, and must create one back. Subtract already-selected deletions from the PoolMax excess before choosing additional idle pods.
AGENTS.md reference: kubernetes/AGENTS.md:L157-L157
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, thanks. Fixed in b4c42f9. The existing desired-size scale-in is back to what it was, because it already accounts for rolling-update replacements through the supply count. The PoolMax excess is added on top, measured after the planned deletions: max(schedulable - desired, schedulable - planned - poolMax). TestScalePoolCountsPlannedDeletionsTowardPoolMax covers your scenario with a recycled pod that is still listed as idle, and with a rolling-update deletion that has a replacement in supplyCnt. In both cases only the planned pod is deleted.
The scale-in target was min(desired, poolMax) applied to every pod in the pool, but pods that recycling or a rolling update already planned to delete are part of that count, and pickPodsToDelete deletes them in addition to the idle pods chosen for scale-in. With five idle pods, bufferMin 4, poolMax 4 and one planned deletion, the pool deleted two pods, dropped below its buffer minimum and had to create one back. Keep the existing desired-size scale-in unchanged, since it already accounts for rolling-update replacements through the supply count, and add the PoolMax excess separately, measured after the planned deletions.
scalePool counted planned deletions by calling pickPodsToDelete with no idle pods and no scale-in, which only works because of how that function happens to be built. Move the selection of planned deletions (pods that still exist, are not terminating, once each) into plannedDeletions, and use it from both pickPodsToDelete and scalePool.
github-actions
Bot
added
size/S
scalePoolscales in only when the desired pool size,max(allocated + supply + desiredBuffer, poolMin), drops below the current number of schedulable pods.poolMaxis applied only on the way up, as a limit on how many new pods to create (maxNewPods).So when
poolMaxis lowered below what the buffer band orpoolMinasks for, the pool stops growing but keeps every idle pod it already has. For example, with four ready idle pods andbufferMin: 1, bufferMax: 4, changingpoolMaxfrom 4 to 2 leaves all four pods running indefinitely. The CRD has no cross-field validation that would reject this, anddocs/architecture/control-plane/operator.mddescribespoolMaxas a bound on the whole pool.Summary
desiredSchedulableCnt, and also scale in the excess abovepoolMax. Pods that recycling or a rolling update already planned to delete count toward that excess, so the pool does not delete one of them plus an extra idle pod and then create one back.pickPodsToDeleteonly picks idle pods, so allocated pods are never deleted to meet the bound. If the allocated pods alone exceedpoolMax, the pool deletes its idle pods and then waits for sandboxes to be released.scaleStrategy.maxUnavailable, at least one pod per batch) still paces the scale-in.desiredSchedulableCntitself on purpose: that value also sizes the scale-up batch, and capping it would slow scale-up whenever demand exceedspoolMax. The desired-size scale-in is also unchanged, since it already accounts for rolling-update replacements through the supply count.Testing
Not run (explain why)
Unit tests
Integration tests
e2e / manual verification
New table test,
TestScalePoolScalesInDownToPoolMax. It covers three cases: a buffer band above a loweredpoolMax(4 idle pods, 2 deleted),poolMinabovepoolMax(4 idle, 2 deleted), and allocated pods abovepoolMax(3 allocated and 1 idle, only the idle one deleted). Every case checks that only idle pods are deleted. All three fail on current main with no pod deleted.TestScalePoolCountsPlannedDeletionsTowardPoolMax: five idle pods,poolMax: 4and one planned deletion. It covers a recycled pod that is still listed as idle, and a rolling-update deletion with a replacement insupplyCnt. Only the planned pod is deleted.The existing envtest specs that change
poolMax, such as "should successfully scale out buffer limit by pool max" (poolMax: 2, bufferMin: 3, expects 2 pods), still hold with this change. I could not run the envtest suite locally (no etcd).go test ./internal/controller/ -skip TestControllers: the only failures areTestReconcileDeletingTaskSandbox,TestReconcileDeletingPodandTestReconcilePodRecovery, which fail the same way on current main in my environment.go vetand golangci-lint (v2, repo config) are clean.Breaking Changes
Pools whose
poolMaxis below their current idle pod count will now delete the extra idle pods, which is whatpoolMaxis documented to do.Checklist