SANDBOX-1811: Implement warm pod pool - #14
Conversation
- Add WarmPool struct in pkg/session/pool.go with ReconcilePool, ClaimPod, TriggerReplenish, and StartReconciler - ClaimPod uses resourceVersion-based optimistic locking for first-writer-wins concurrency with full rollback on failure - ReconcilePool maintains target pool size and cleans stale unassigned pods (age > 2× IdleTimeout) - Integrate pool into SessionManager.GetOrCreatePod between label discovery and on-demand creation with graceful fallback - Add ReconcileInterval field to SandboxConfig (default 30s) - Add 26 unit tests covering pool spec, reconciliation, claiming, rollback, replenishment, and manager integration Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
WalkthroughAdds optional warm-pool support for sandbox sessions, including reconcile timing, manager wiring, warm pod claiming, shared pod and secret construction, and tests for the new flow. ChangesSession warm-pool flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/session/manager.go (1)
132-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
Warn-level logging on every pool claim miss.Warm-pool exhaustion or optimistic-lock contention on
ClaimPodis an expected, routine condition under load (analogous to k8s resourceVersion conflicts, which are normal, not failures), not an anomaly. Logging atWarnon every miss will flood logs once the pool depletes during traffic spikes, drowning out real signal. Consider demoting toDebug/Infofor expected exhaustion, or only warning when the underlying error indicates a genuine (non-capacity) failure.♻️ Suggested adjustment
if m.pool != nil { ip, podName, claimErr := m.pool.ClaimPod(ctx, sessionID) if claimErr == nil { m.cache.Set(sessionID, ip, podName) return ip, nil } - m.logger.Warn("warm pool claim failed, falling back to on-demand creation", "session", sessionID, "error", claimErr) + m.logger.Debug("warm pool claim failed, falling back to on-demand creation", "session", sessionID, "error", claimErr) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/session/manager.go` around lines 132 - 139, The ClaimPod fallback in manager.go is logging every pool claim miss at Warn, which will spam logs under normal warm-pool exhaustion or contention. Update the logging in m.pool.ClaimPod handling inside the session manager flow to use Debug or Info for expected claim failures, and reserve Warn only for genuine unexpected errors; if needed, branch on claimErr to distinguish capacity/optimistic-lock misses from real failures. Keep the cache.Set and fallback behavior unchanged.pkg/session/pool_test.go (1)
441-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test fail when replenish handling is broken.
Initial reconciliation already fills the pool to 3 before
TriggerReplenish, so this assertion can pass even if the replenish channel case never runs. Delete one pod after initial reconcile, then trigger replenish and assert it returns to the target.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/session/pool_test.go` around lines 441 - 463, The test in the replenish signal case can pass even if the replenish path is broken because initial reconciliation already reaches the target. In `TestPool`’s "responds to replenish signal" subtest, remove one pod after the initial `StartReconciler` reconciliation using the pool/clientset, then call `TriggerReplenish()` and assert the pod count returns to the target so the `TriggerReplenish` handling is actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/session/manager.go`:
- Around line 82-88: The warm-pool reconciler is never started during agent
bootstrap, so pool claims won’t run; update the startup flow in
cmd/agent/main.go to call SessionManager.StartPool once the SessionManager is
constructed. Use the existing StartPool method on SessionManager after
initialization so the pool reconciler begins running when the pool is enabled.
In `@pkg/session/pool.go`:
- Around line 286-294: The fallback in p.assignToken is unreachable for pods
without an IP, so warm pods are being rolled back too early; update the flow in
p.assignToken/patched handling so that waitForIP runs before attempting the
/assign call when patched.Status.PodIP is empty. Keep the rollback/deleteSecret
path for real assign failures, but make sure the empty-IP case waits briefly for
a PodIP first and only then proceeds with assignment.
- Around line 75-82: The pod filtering in the pool listing logic is dropping
terminal pods too early, which prevents stale cleanup from ever seeing them.
Update the pod collection in the session pool code so it returns all pods from
the list, and move the Failed/Succeeded exclusion into ReconcilePool when
computing the live pod count after stale-delete handling. Keep the fix centered
around the pod list loop and ReconcilePool so terminal warm pods can still be
deleted when stale.
- Around line 280-284: The auth Secret creation path in pool.go is accepting
k8serrors.IsAlreadyExists without validating the existing Secret, which can
leave stale token/label state behind. Update the claimed-pod secret handling in
the Create flow to fetch the pre-existing Secret, verify its token and labels
match the current claim, and update or replace it if they do not; only proceed
silently when the existing Secret is already correct. If validation or
reconciliation fails, keep the rollbackLabel/pod cleanup behavior and return an
error from the same secret-creation block.
- Around line 207-212: The stale-pod cleanup in reconcile() currently logs
delete failures but still continues, which can let the pool refill above
WarmPoolSize while the old pod remains. In pool.go, update the stale deletion
branch so a non-NotFound error from CoreV1().Pods(...).Delete causes
reconciliation to stop or return an error instead of continuing, and keep the
existing logging around the failed delete for visibility.
---
Nitpick comments:
In `@pkg/session/manager.go`:
- Around line 132-139: The ClaimPod fallback in manager.go is logging every pool
claim miss at Warn, which will spam logs under normal warm-pool exhaustion or
contention. Update the logging in m.pool.ClaimPod handling inside the session
manager flow to use Debug or Info for expected claim failures, and reserve Warn
only for genuine unexpected errors; if needed, branch on claimErr to distinguish
capacity/optimistic-lock misses from real failures. Keep the cache.Set and
fallback behavior unchanged.
In `@pkg/session/pool_test.go`:
- Around line 441-463: The test in the replenish signal case can pass even if
the replenish path is broken because initial reconciliation already reaches the
target. In `TestPool`’s "responds to replenish signal" subtest, remove one pod
after the initial `StartReconciler` reconciliation using the pool/clientset,
then call `TriggerReplenish()` and assert the pod count returns to the target so
the `TriggerReplenish` handling is actually exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7669373e-138f-474e-8398-1e559a192828
📒 Files selected for processing (4)
pkg/session/config.gopkg/session/manager.gopkg/session/pool.gopkg/session/pool_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/session/config.gopkg/session/pool_test.gopkg/session/manager.gopkg/session/pool.go
🪛 ast-grep (0.44.0)
pkg/session/pool.go
[warning] 138-138: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(p.config.AgentPort)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🔇 Additional comments (5)
pkg/session/manager.go (4)
48-55: LGTM!
69-93: LGTM!
114-158: LGTM!
73-78: 🩺 Stability & AvailabilityWarmPool uses its own 10s HTTP client for
/assign;SessionManager.httpClientis separate and not shared here.> Likely an incorrect or invalid review comment.pkg/session/config.go (1)
15-32: LGTM!
- Return all pods (including terminal) from listUnassignedPods so ReconcilePool can delete stale Failed/Succeeded pods; skip terminal pods only in ClaimPod and when counting live pods for deficit - Abort reconciliation refill when stale pod deletion fails (non-404) to prevent overshooting WarmPoolSize - Reject pre-existing auth Secrets instead of silently tolerating AlreadyExists to avoid stale token/label inconsistencies - Move waitForIP before assignToken in tryClaimPod so pods without an IP wait for one before the /assign call instead of failing - Demote pool-claim-miss log from Warn to Info (expected under load) - Strengthen replenish test: delete a pod after initial reconciliation before triggering replenish to actually exercise the signal path Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
rajivnathan
left a comment
rajivnathan
left a comment
There was a problem hiding this comment.
Looks like there's some opportunity for reuse with the pod creation functions that are in the session manager, both for the
Address Rajiv's review: deduplicate pod spec and auth secret construction between SessionManager and WarmPool by extracting shared package-level helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
…lpers - Add name param to buildBasePodSpec so callers don't set it after - Set last-activity annotation in base spec for all pods (warm + session) - Extract bestEffortDeleteSecret as shared package-level function - Update tests to match new base spec behavior Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Assisted by : Cursor
Summary by CodeRabbit