fix: enforce strict Sandbox-to-Pod mapping

[alanhuangch]

What this PR does / why we need it:

The Sandbox controller could create an additional Pod when the pod-name annotation was stale or missing even though a Pod controlled by the same Sandbox UID still existed.

This PR treats the tracking label as a candidate index and the controller owner-reference UID as the authoritative Sandbox-to-Pod mapping. It recovers a unique owned Pod instead of adopting or creating another Pod, and waits for an owned terminating Pod rather than overlapping it.

If multiple Pods are controlled by the same Sandbox UID, reconciliation now fails closed: it does not select, create, or delete a Pod; it does not create or modify the routing Service; and it reports Ready=False with reason MultiplePods plus a Warning Event. This preserves stateful Pods for operator inspection and avoids hot-loop retries while the conflict remains unchanged.

Which issue(s) this PR is related to:

Fixes #1332
Follow-up to #127.
Related to #1297.

Release Note

Fixed Sandbox reconciliation to avoid creating an additional Pod when the pod-name annotation is stale or an owned Pod is terminating. Multiple owned Pods now fail closed and report Ready=False with reason MultiplePods.

Summary by CodeRabbit

• Bug Fixes

  • Improved sandbox Pod selection using ownership information and repaired stale tracking metadata.
  • Prevented duplicate Pod creation when an owned Pod exists or is terminating.
  • Added safe handling for multiple owned Pods, including Ready status updates and warning events.
  • Prevented Service reconciliation when Pod ownership is ambiguous.

• Tests

  • Added coverage for Pod recovery, ownership filtering, duplicate prevention, termination handling, and conflict reporting.

[netlify]

✅ Deploy Preview for agent-sandbox canceled.

Name	Link
🔨 Latest commit	7c48605
🔍 Latest deploy log	https://app.netlify.com/projects/agent-sandbox/deploys/6a7d56e6be306b000887bc91

[kubernetes-prow]

Hi @alanhuangch. 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.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d060053b-9ffe-4e06-a870-7031029e91f8

📥 Commits

Reviewing files that changed from the base of the PR and between b8bbb62 and 7c48605.

📒 Files selected for processing (3)

• cmd/agent-sandbox-controller/main.go
• controllers/sandbox_controller.go
• controllers/sandbox_controller_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• controllers/sandbox_controller.go
• controllers/sandbox_controller_test.go
• cmd/agent-sandbox-controller/main.go

---

📝 Walkthrough

📝 Walkthrough

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The implementation addresses issue #1332 requirements for authoritative ownership, recovery, fail-closed conflicts, status reporting, events, and Service protection.
Out of Scope Changes check	✅ Passed	The changes remain within the linked issue scope and add only related controller behavior, event wiring, and regression tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: enforcing strict Sandbox-to-Pod mapping.
Description check	✅ Passed	The description covers the change, rationale, linked issues, behavior, conflict handling, and release note required by the template.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

controllers/sandbox_controller_test.go (1)

3158-3160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test preservation of an existing Service.

This test proves that reconciliation does not create a Service. It does not prove that reconciliation leaves an existing Service unchanged. Seed a Service with distinct metadata and spec, then assert it is unchanged after the ownership conflict.

As per coding guidelines, new behavior requires a unit test.

🤖 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 `@controllers/sandbox_controller_test.go` around lines 3158 - 3160, Extend the
test around the ambiguous Pod mapping reconciliation to seed an existing Service
with distinct metadata and spec before reconciliation, then retrieve it
afterward and assert those fields remain unchanged. Preserve the existing
not-found assertion only for the no-Service case, and use the relevant
reconciliation/test setup symbols already present to verify ownership conflict
does not modify the preexisting Service.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@controllers/sandbox_controller_test.go`:
- Around line 3158-3160: Extend the test around the ambiguous Pod mapping
reconciliation to seed an existing Service with distinct metadata and spec
before reconciliation, then retrieve it afterward and assert those fields remain
unchanged. Preserve the existing not-found assertion only for the no-Service
case, and use the relevant reconciliation/test setup symbols already present to
verify ownership conflict does not modify the preexisting Service.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 41f2b67f-a02d-471e-9b69-274481f39a03

📥 Commits

Reviewing files that changed from the base of the PR and between ac67d37 and b8bbb62.

📒 Files selected for processing (4)

• api/v1beta1/sandbox_types.go
• cmd/agent-sandbox-controller/main.go
• controllers/sandbox_controller.go
• controllers/sandbox_controller_test.go

[Copilot]

Pull request overview

This PR tightens the Sandbox controller’s invariant that a Sandbox maps to at most one backing Pod by treating the tracking label as an index and using the controller ownerReference UID as the authoritative mapping. It also introduces a fail-closed behavior (Ready=False with reason MultiplePods + Warning Event) when multiple Pods are controlled by the same Sandbox UID, avoiding further Pod/Service reconciliation until the conflict is resolved.

Changes:

• Classify Pod candidates by controller owner UID, recover a unique owned Pod when the tracked pod-name annotation is stale/missing, and refuse to act when multiple owned Pods exist.
• Suppress routing Service reconciliation during Pod-mapping ambiguity and surface a stable Ready=False reason (MultiplePods) plus a Warning Event.
• Add unit tests covering owned-Pod recovery, stale adoption avoidance, terminating owned Pod handling, and multiple-owned-Pod fail-closed behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
controllers/sandbox_controller.go	Adds owned-Pod filtering/recovery, fail-closed error type + Event emission, and condition handling for MultiplePods conflicts.
controllers/sandbox_controller_test.go	Adds regression/unit tests for owned-Pod recovery, conflict behavior, and event/condition surfacing.
cmd/agent-sandbox-controller/main.go	Wires an EventRecorder into the Sandbox reconciler.
api/v1beta1/sandbox_types.go	Introduces SandboxReasonMultiplePods for Ready condition reporting.

[GonzaloLuminary]

I think requiring manual remediation to fix this scenario is not ideal

[alanhuangch]

@GonzaloLuminary Thanks — I agree that requiring manual remediation as the normal recovery path would not be ideal.

This PR prevents the controller from creating this state. Manual action is only required for a pre-existing or externally corrupted state with multiple non-terminating Pods. If an extra Pod is already terminating, its deletion event requeues the Sandbox and reconciliation recovers automatically.

I chose fail-closed because all conflicting Pods have the current Sandbox UID as their controller owner and may be stateful. The owner UID proves membership, but neither the owner reference nor the tracking label identifies which Pod contains the canonical state. Choosing by annotation, name, readiness, or age could delete the wrong Pod in compatibility and upgrade scenarios.

Would you prefer a specific deterministic cleanup rule in this PR? Issue #1297 is moving toward enforcing Pod.Name == Sandbox.Name. Once that invariant and its migration behavior are established, keeping the same-name Pod and deleting other owned Pods would be much safer.

My preference is to keep this fix focused on preventing new duplicates and surfacing existing conflicts, then design automatic remediation together with #1297, unless maintainers prefer a specific deletion policy here.

[aditya-shantanu]

/lgtm
/ok-to-test

[alanhuangch]

/retest

[alanhuangch]

@aditya-shantanu Rebased the PR onto the latest upstream/main. Could you please take another look at the updated changes?

[kubernetes-prow]

@alanhuangch: 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	7c48605	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 NOT APPROVED

This pull-request has been approved by: aditya-shantanu, alanhuangch
Once this PR has been reviewed and has the lgtm label, please assign janetkuo for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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