fix(sandbox): bound paused proxies to node capacity - #1542
Open
0xAlcibiades wants to merge 3 commits into
Open
Conversation
…node-budget binding Review fixes for the paused-proxy retention sweep: - The ClusterRoleBinding's ServiceAccount subject lacked a namespace, which the API server rejects for a cluster-scoped binding, leaving the sweep's pods+nodes reads ungranted. - The pod census excluded every iron-proxy pod from per-node load, so the proxies of running sandboxes were not counted and the sweep retained more than the node headroom allowed. Count all scheduled pods as load and take back out only the evictable paused proxies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1541
Bounded retention of paused sandboxes' iron-proxy pods, sized from node capacity. Two commits, the Rust half independently cherry-pickable.
Read this first: what #1502 changed
#1502 made
pause()tear the proxy down, so steady-state retention is now zero and this is not the fix for the incident in #1541 on an up-to-date deployment. I would rather say that up front than have you find it in review.What it still covers:
pause()patchesreplicas: 0then deletes the proxy. A death between the two leaves a paused CR with a live proxy that nothing reclaims.FailedSchedulingevents.If you read #1502 as making this redundant, say so and I will close it. I think 1 and 2 are real and unaddressed, and 3 is worth having regardless.
Commit 1 —
fix(sandbox): bound paused proxies to node capacityNew
paused_proxy_retentionmodule incentaur-sandbox-agent-k8s:status.allocatable.podsand scheduled (non-terminating) pod load, and identifies retained paused proxies: iron-proxy pods whose Sandbox CR is atreplicas: 0.allocatable − load − marginslots; beyond that the longest-paused go first, ordered by the existingcentaur.ai/paused-atannotation with a sandbox-id tiebreak so selection is deterministic.resume()entry and cleared on every exit path), unorderable pauses, or nodes with an unreadable budget. That last one is fail-open on purpose: evicting against an unknown budget is worse than not evicting.Commit 2 —
feat(chart): paused-proxy retention knobs and node-budget RBACRenders the three env vars, adds the read-only ClusterRole/Binding (
pods,nodesget/list) that the namespace-scoped sandbox-manager Role cannot provide, documents the knobs, and adds them tovalues.schema.json.The split keeps commit 1 cherry-pickable on its own: without the chart half the sweep cannot read nodes, fails open, and evicts nothing.
Testing
10 new tests — 8 on eviction selection (headroom, saturation to zero, cap interaction, cap-never-expands-headroom, unobservable node, tie-breaking, config toggle) and 2 on arg parsing.
cargo test -p centaur-sandbox-agent-k8s -p centaur-api-server: 122 and 56 pass.cargo fmt --all --checkandcargo clippy --all-targets -- -D warningsclean.helm lintpasses andhelm templaterenders the env block and both RBAC objects.