fix: skip warm pool sandboxes without a backing pod

[noeljackson]

What this PR does / why we need it:

During a warm-pool rotation, a Sandbox can remain queued while its backing Pod is being recreated. Adopting that Sandbox before the controller has observed Pod networking leaves the claim bound to unusable capacity.

This PR keeps static ownership and validity checks in isAdoptable and treats cached Status.PodIPs as a transient selection signal in getCandidate:

• Candidates without observed Pod IPs are skipped, requeued, and retain their node-placement metadata.
• If every otherwise-valid candidate is waiting for Pod IPs, a newly-created claim briefly requeues at 500 ms intervals for at most two seconds instead of immediately creating a burst of cold Pods.
• A genuinely empty pool still cold-starts immediately.
• When the grace period expires, reconciliation emits a structured level-0 log with the fixed reason warm_candidates_network_pending before falling back to cold creation.
• A candidate that has Pod IPs remains eligible even when it is not Ready, since it can still be more useful than a cold start.
• An already-recorded assignment is completed independently of this selection gate, so a transient empty PodIPs status cannot abandon it.

Status.PodIPs is cached evidence, not an authoritative Pod-existence check: it narrows the rotation race but cannot eliminate every concurrent deletion window. The bounded retry is intentionally write-free and uses the claim creation timestamp as its deadline, preserving bounded claim startup latency and controller restart safety.

This is a resubmission of #519 with the unused adoptionStrategy API removed. Selection strategy should be designed separately once there is a concrete second strategy.

Which issue(s) this PR is related to:

Ref #491
Ref #519

Testing

• go test ./extensions/controllers -count=1
• make lint-go
• make build
• TMPDIR=$PWD/dev/tools/tmp make test-unit

Regression coverage includes pool exhaustion, all-candidates-network-pending, grace expiry, adoption after networking becomes observable, queue preservation, and assigned-sandbox recovery.

Release Note

SandboxClaims now briefly wait for rotating warm-pool Sandboxes to report Pod networking before falling back to cold creation.

[netlify]

✅ Deploy Preview for agent-sandbox canceled.

Name	Link
🔨 Latest commit	47458e0
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/6a7f42233188880008523c08

[k8s-ci-robot]

Hi @noeljackson. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[Copilot]

Pull request overview

This PR tightens warm-pool sandbox adoption to avoid adopting sandboxes during pod-rotation gaps (when the backing Pod is absent), and introduces an API placeholder for future warm-pool adoption strategies on SandboxTemplate.

Changes:

• Update warm-pool adoptability checks to require Sandbox.Status.PodIPs to be non-empty (proxy for “backing Pod exists and is networked”).
• Add SandboxTemplate.spec.adoptionStrategy (enum; default/current value OldestReady) as an API surface for future strategy extensions.
• Update/extend SandboxClaim adoption tests and fixtures to reflect the new adoptability signal.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
k8s/crds/extensions.agents.x-k8s.io_sandboxtemplates.yaml	Adds CRD schema for spec.adoptionStrategy with default/enum.
extensions/api/v1alpha1/sandboxtemplate_types.go	Introduces WarmPoolAdoptionStrategy type/const and SandboxTemplateSpec.AdoptionStrategy.
extensions/controllers/sandboxclaim_controller.go	Tightens isAdoptable to skip candidates without Status.PodIPs.
extensions/controllers/sandboxclaim_controller_test.go	Updates fixtures and adds regression scenarios around rotation/no-backing-pod behavior.
extensions/controllers/sandboxclaim_pod_exclusivity_test.go	Updates warm pool sandbox fixtures to include Status.PodIPs so they remain adoptable.

[noeljackson]

Force-pushed addressing both Copilot inline comments and rebased onto current main.

1. Strategy naming (comment) — Renamed WarmPoolAdoptionStrategyOldestReady → WarmPoolAdoptionStrategyOldestAdoptable and the enum value OldestReady → OldestAdoptable. The bot was right that the old name implied a Ready-first sort, but the FIFO queue actually pops in enqueue order over candidates that pass isAdoptable (which now requires PodIPs but not Ready=True). The new name reflects the actual semantics — and a future strategy that does prefer Ready first can be added under a different name without breaking the field.

2. Test seeding (comment) — Changed the test queue seeder to enqueue every warm-pool-labelled sandbox regardless of current adoptability, instead of pre-filtering with isAdoptable. This mirrors production: sandboxEventHandler enqueues sandboxes when they become adoptable, and the queue can hold stale entries for sandboxes whose backing Pod was later deleted (rotation). The pop-side filter (verifySandboxCandidate → isAdoptable) is what actually guards adoption. The two regression cases now genuinely exercise the skip-on-pop path.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[aditya-shantanu]

/lgtm

[aditya-shantanu]

/lgtm

PodIPs is the right adoption boundary here — the core controller clears Status.PodIPs whenever the backing Pod is gone and repopulates it from Pod status otherwise, so this gate is accurate. Requeue-via-skipped keeps un-networked candidates available for retry without starving the loop, and the assigned-sandbox preservation test covers the regression from #519. Thanks for the narrowed re-submission.

[noeljackson]

/retest

[aditya-shantanu]

The narrower boundary is the right call, and the mechanics check out: the core Sandbox controller clears Status.PodIPs when the backing Pod is gone and repopulates it from pod status, skipped keys are only re-added after the pop loop exits (no livelock), and the fallback/skipped interplay in getCandidate is preserved. One advisory consideration inline; not blocking.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

extensions/controllers/sandboxclaim_controller.go:1997

• This code introduces a bounded grace period (requeue via warmCandidatesPendingError) when all warm candidates are skipped for missing PodIPs. The PR description currently says the claim “falls through to cold creation for that reconcile” when every queued candidate is skipped; that’s not accurate during the grace window.

Suggested action: update the PR description to explicitly mention the grace-period requeue behavior (and that cold creation happens only after the grace expires), so reviewers/operators don’t misinterpret the new control flow.

	if pendingNetworkCandidates > 0 {
		if retryAfter, ok := warmCandidateRetryAfter(claim, time.Now()); ok {
			return nil, &warmCandidatesPendingError{
				pendingCandidates: pendingNetworkCandidates,
				retryAfter:        retryAfter,

extensions/controllers/sandboxclaim_controller_test.go:2138

• This regression case currently relies on the shared claim fixture having a zero CreationTimestamp, which bypasses the new warm-candidate grace logic and forces an immediate cold start. In real clusters CreationTimestamp is always set, so this test won’t exercise the production path unless it’s explicitly simulating a claim that’s already past the grace deadline.

Consider setting a non-zero CreationTimestamp here (e.g., older than warmCandidateGracePeriod) to make the intent explicit and keep the fixture closer to real API-server objects.

Reminder: please don’t use GitHub’s “Commit suggestion” button for AI suggestions (it can add an AI co-author and break the Kubernetes CLA check); apply the change locally instead.

				claim,

[noeljackson]

@janetkuo The requested changes and all inline threads are now addressed at 69c533b. The all-candidates-unnetworked case uses a bounded two-second grace with 500 ms retries, distinguishes genuine pool exhaustion, and emits a fixed-reason fallback log before cold creation. Full local tests and fast presubmits are green; long e2e/benchmark jobs remain in progress. Could you please re-review? @aditya-shantanu, your advisory PodIPs/fallback thread is also addressed and resolved.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[aditya-shantanu]

/lgtm

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[noeljackson]

Rebased onto current upstream/main at 47458e0. The final grace-period thread is addressed and resolved: the two-second Pod IPAM window and bounded cold-start fallback are now documented. Local go test ./extensions/controllers/... and make lint-go pass; fresh presubmits are running. @aditya-shantanu, could you please reapply /lgtm after the rewritten head is reviewed? @janetkuo, the old CHANGES_REQUESTED review is still active even though its original threads are resolved; could you please re-review? @barney-s, this will still need /approve.

[kubernetes-prow]

@noeljackson: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
presubmit-agent-sandbox-benchmarks-kops-gcp-kindnet	47458e0	link	false	/test presubmit-agent-sandbox-benchmarks-kops-gcp-kindnet

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[aditya-shantanu]

/lgtm

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aditya-shantanu, janetkuo, noeljackson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• extensions/controllers/OWNERS [aditya-shantanu,janetkuo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment