fix(sandbox): reconcile agent resources on resume - #1546
Open
0xAlcibiades wants to merge 2 commits into
Open
Conversation
A Sandbox CR stores the pod template rendered when the session was created, and every pod recreation re-renders from that stored template. So a change to the configured agent resources never reaches a session that already exists, even when its pod is destroyed and recreated: the replacement comes back on the old limits. That inverts the usual remedy. Recycling the pod does not migrate the session; only recreating the CR does, and that destroys the workspace the session is holding. The sessions that outlive a resources change are the long-running ones with the largest accumulated state, which are exactly the ones a memory raise is meant to protect -- one was OOM-killed at three and a half hours on a limit that had already been raised for everyone else. resume now brings the stored template back in line with the configured resources before setting replicas to 1. It runs while the sandbox is paused and no pod exists, so nothing is restarted; the reconciled template is simply what the next pod is built from. Only the agent container's resources is touched. The rest of the template is session identity -- harness args, env, principal annotations -- and re-rendering it from current config would rewrite a session's own setup underneath it. A container name that does not match is left alone rather than patched by index. The patch is positional JSON rather than a merge, because a merge on containers replaces the whole array; that is what the new kube jsonpatch feature is for. A failed reconcile warns instead of failing the resume: a stale limit is better than a turn that cannot start.
replace errors when the container has no resources member (the case a session created before this setting exists hits), so a resume would fail to reconcile and warn instead of bringing the limits in line. add inserts the member when absent and replaces it when present.
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
1 participant
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 #1545
Change
AgentSandboxConfiggainsdefault_resources, populated from the sameSESSION_SANDBOX_RESOURCESthe create path renders.resumereconciles thestored pod template against it before setting
replicas = 1.resumeonly receives a sandbox id, not a spec, which is why the configuredvalue has to reach the backend through config rather than being re-resolved.
Four decisions worth reviewing
It runs while the sandbox is paused.
replicasis still 0 and no podexists, so nothing is restarted — the reconciled template is simply what the
next pod is built from. That is what makes this safe to do unconditionally on
resume rather than gating it behind an explicit migration.
Only the agent container's
resourcesis touched. The rest of the templateis session identity — harness args, env, principal annotations — and
re-rendering it from current config would rewrite a session's own setup
underneath it. That is the difference between reconciling fleet policy and
resetting the session.
A container whose name does not match is left alone, rather than patched by
index. Guessing would rewrite whichever container happened to sit there; a test
covers the agent container not being index 0.
The patch is positional JSON, not a merge. A merge patch on
containersreplaces the whole array, which would mean round-tripping every container
through the generated types and risking anything they do not model. This is the
one thing here that touches
Cargo.toml: it enables kube'sjsonpatchfeature.If you would rather not take that dependency I can rewrite it as a read-modify-
write of the full array, but the positional patch is the smaller blast radius.
A failed reconcile warns rather than failing the resume. A stale limit is worse
than a current one, but both are better than a turn that cannot start.
Testing
Four new tests over the drift decision, extracted as a pure function so it needs
no cluster: drift reported with the correct index when the agent container is
not first, no drift when already current, no drift (and no patch) when no
container matches the configured name, and a container with no resources at all
being filled in.
cargo test -p centaur-sandbox-agent-k8s: 68 pass.cargo fmt --all --checkandcargo clippy --all-targets -- -D warningsclean on both touched crates.The reconcile itself talks to the API server, so it is not exercised here — the
tested part is which container to patch and whether to patch at all.