fix(sandboxclaim): default Delete+TTL=0 lifecycle for warm pool claims - #1309
fix(sandboxclaim): default Delete+TTL=0 lifecycle for warm pool claims#1309vvoronko wants to merge 7 commits into
Conversation
kubernetes-prow
Bot
added
the
do-not-merge/invalid-commit-message
✅ Deploy Preview for agent-sandbox canceled.
|
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @vvoronko. 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 Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
kubernetes-prow
Bot
added
the
size/L
📝 WalkthroughWalkthroughWarmPool-backed SandboxClaims without an explicit lifecycle now use an effective Delete lifecycle after completion. Claims without a WarmPool reference remain active, and explicit ChangesWarmPool lifecycle behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
…s with nil Lifecycle
When a SandboxClaim references a WarmPool but omits spec.lifecycle,
checkExpiration() short-circuits at the nil check and returns (false, 0).
The expiration reconciler never runs, so the claim, sandbox, pod, and VM
persist indefinitely — leaking VPC IPs, auto-refreshing SA tokens, and
holding cloud IMDS access with no expiration path.
Inject an in-memory default Lifecycle{ShutdownPolicy: Delete, TTL: 0}
for warm-pool-sourced claims that omit it. The default is deterministic
and reconcile-local (not persisted to the API server). Non-warm-pool
claims retain the existing behavior (immortal when Lifecycle is nil).
Explicit Lifecycle settings (e.g. Retain for debugging) are never
overridden. A follow-up could add a configurable default reaper TTL
for explicit Retain claims to prevent stale object accumulation.
For: kubernetes-sigs#1306
Signed-off-by: vvoronko <vvoronko@redhat.com>
9593683 to
82e065b
Compare
kubernetes-prow
Bot
removed
the
do-not-merge/invalid-commit-message
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@extensions/controllers/sandboxclaim_controller_test.go`:
- Around line 1766-1769: Replace the fake.NewClientBuilder setup in this
extension reconciler test with the existing envtest-style extension harness and
fixtures. Preserve the configured scheme, objects, and status-subresource
behavior through the harness APIs, and use its reconciler client so status and
patch operations run against the API server.
In `@extensions/controllers/sandboxclaim_controller.go`:
- Around line 426-439: The warm-pool lifecycle default must remain in-memory
only and must not be persisted to the Claim. Update checkExpiration in
sandboxclaim_controller.go so its temporary lifecycle value is not retained on
the claim used by active reconciliation, then extend the active WarmPool
regression coverage in sandboxclaim_controller_test.go to reconcile a claim
without an explicit lifecycle and assert updatedClaim.Spec.Lifecycle remains nil
across reconcile cycles.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2b956d5-efd2-4eee-a605-a01f4197d050
📒 Files selected for processing (2)
extensions/controllers/sandboxclaim_controller.goextensions/controllers/sandboxclaim_controller_test.go
There was a problem hiding this comment.
🟡 Not ready to approve
The new unit test uses invalid Kubernetes object names (spaces/overlength), which can mask real-world behavior and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a warm-pool SandboxClaim lifecycle leak by ensuring warm-pool-sourced claims without spec.lifecycle get an in-memory default lifecycle that expires immediately after the workload finishes, allowing the controller to delete the claim (and associated resources) instead of retaining them forever.
Changes:
- Inject a warm-pool-only default lifecycle (
ShutdownPolicy=Delete,TTLSecondsAfterFinished=0) whenspec.lifecycleis nil. - Preserve existing behavior for non-warm-pool claims with nil lifecycle (no expiration) and never override explicitly set lifecycles.
- Add unit tests covering warm-pool defaulting, explicit retain, and the non-warm-pool nil-lifecycle case.
File summaries
| File | Description |
|---|---|
| extensions/controllers/sandboxclaim_controller.go | Defaults nil lifecycle for warm-pool claims so expiration and deletion can run. |
| extensions/controllers/sandboxclaim_controller_test.go | Adds tests to validate the warm-pool default lifecycle behavior and non-regression cases. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Add a defensive assertion to TestSandboxClaimWarmPoolDefaultLifecycle verifying that Spec.Lifecycle remains nil in the API server after reconciliation cycles. This enforces the contract that checkExpiration's warm-pool lifecycle injection is in-memory only and cannot leak through status subresource patches. For: kubernetes-sigs#1306 Signed-off-by: vvoronko <vvoronko@redhat.com>
Update checkExpiration godoc to describe the in-memory lifecycle injection for warm-pool claims. Use short DNS-1123-valid slugs for test object names while keeping descriptive subtest labels. For: kubernetes-sigs#1306 Signed-off-by: vvoronko <vvoronko@redhat.com>
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
One blocking issue: the injected default can leak into the persisted spec.
| // finished claims accumulate indefinitely — leaking VMs, pod IPs, | ||
| // and auto-refreshing SA tokens. See #1306. | ||
| ttl := int32(0) | ||
| claim.Spec.Lifecycle = &extensionsv1beta1.Lifecycle{ |
There was a problem hiding this comment.
This injection is not actually "never persisted": Reconcile runs checkExpiration before reconcileActive, and the warm-pool adoption path then does a full-object r.Update(ctx, claim) (~line 1008), which writes this Delete+TTL=0 default into the user's spec. Compute the effective lifecycle in a local variable instead of mutating claim.Spec.
There was a problem hiding this comment.
Thanks for catching this, Aditya — great attention to detail. You're absolutely right that r.Update(ctx, claim) at line 1008 (adoption path) is a full-object write and would persist the injected lifecycle.
I've refactored checkExpiration to return the effective lifecycle as a third value instead of mutating claim.Spec. The caller uses it only for the policy decision at lines 254-258. Here's the case-by-case trace through the reconcile loop:
Case 1: Warm-pool claim, nil lifecycle, workload finished
checkExpirationsynthesizes{Delete, TTL=0}in a locallc.TimeLeft→(true, 0).- First reconcile: sets Expired condition via
updateStatus(status subresource only), requeues. - Second reconcile: line 254 checks
effectiveLifecycle.ShutdownPolicy == Delete→r.Delete(ctx, claim). claim.Spec.Lifecycleremains nil throughout. ✓
Case 2: Warm-pool claim, nil lifecycle, workload active
checkExpirationsynthesizes{Delete, TTL=0}. No finished condition →TimeLeftreturns(false, ...).claimExpired=false→reconcileActive→ adoption path →r.Update(ctx, claim).claim.Spec.Lifecycleis nil — never mutated. Full-object update is safe. ✓
Case 3: Warm-pool claim, explicit Retain (no TTL), workload finished
checkExpirationusesclaim.Spec.Lifecycledirectly.TTLSecondsAfterFinished=nil→NeedsCleanup()returns false →ExpireAtreturns nil →TimeLeftreturns(false, 0).claimExpired=false→reconcileActive. Explicit Retain honored, no override. ✓
Case 4: Non-warm-pool claim, nil lifecycle, workload finished
checkExpiration:WarmPoolRef.Name=""→ early return(false, 0, nil).- Immortal behavior preserved — unchanged from original code. ✓
Case 5: Warm-pool claim, explicit Delete + TTL=300, finished 1m ago
checkExpirationusesclaim.Spec.Lifecycle. TTL expires at finished+300s (4m from now).TimeLeftreturns(false, ~4min). Requeued for later. ✓
Case 6: Post-reconcile expiration check (line 290)
- After
reconcileActive, sandbox may have transitioned to Finished mid-reconcile. - Second
checkExpirationcall: lifecycle discarded (_), onlyexpiredused to set condition and requeue. Policy action happens on the next reconcile cycle at lines 254-272. - No spec mutation. ✓
Summary matrix:
| Scenario | WarmPoolRef | Lifecycle | Finished | effectiveLC | expired | Path | Spec mutated? |
|---|---|---|---|---|---|---|---|
| Warm+nil+finished | yes | nil | yes | Delete+TTL=0 | true | delete claim | no |
| Warm+nil+active | yes | nil | no | Delete+TTL=0 | false | reconcileActive | no |
| Warm+Retain | yes | Retain | yes | Retain | false | reconcileActive | no |
| No pool+nil | no | nil | yes | nil | false | reconcileActive | no |
| Warm+Delete+TTL | yes | Delete+300 | yes (1m) | Delete+300 | false | reconcileActive | no |
| Post-reconcile | any | any | transitions | discarded | maybe | set condition | no |
The ttl local inside checkExpiration is stack-scoped — the pointer in lc.TTLSecondsAfterFinished lives for the duration of Reconcile and falls out of scope at the end. No stashing, no leak.
Fixed.
There was a problem hiding this comment.
@aditya-shantanu FYI — updated test coverage matrix across all lifecycle cases (our 4 new + 3 existing):
| Case | WarmPoolRef | Lifecycle | Finished | Expected outcome | Asserts |
|---|---|---|---|---|---|
| warm+nil+finished | yes | nil | yes | claim deleted | IsNotFound |
| warm+nil+active | yes | nil | no | claim stays | Spec.Lifecycle == nil, no expired condition |
| warm+explicit Retain | yes | Retain | yes | claim stays | ShutdownPolicy preserved as Retain |
| non-warm+nil | no | nil | yes | claim stays (immortal) | Spec.Lifecycle == nil, no expired condition |
| explicit Retain+TTL=0 | yes | Retain+TTL=0 | yes | sandbox deleted, claim kept | expired condition set, finished preserved |
| explicit DeleteForeground+TTL=0 | yes | DeleteForeground+TTL=0 | yes | claim deleted | IsNotFound |
| two-reconcile persistence | yes | Delete+TTL=0 | yes | expired condition persisted before delete | first reconcile sets condition, second deletes |
False-positive guards:
- Active claims: assert no expired condition is set (prevents premature deletion)
- Nil lifecycle: assert
Spec.Lifecycleremains nil after reconciliation (prevents spec leak viar.Update) - Explicit lifecycle: assert
ShutdownPolicyis preserved (prevents synthesized default from overriding user intent)
…ing claim.Spec checkExpiration now returns (expired, timeLeft, *Lifecycle) so the caller uses the effective lifecycle for policy decisions without mutating claim.Spec. This prevents the synthesized Delete+TTL=0 default from leaking into the API server via the full-object r.Update in the warm-pool adoption path (line 1008). Test coverage across 7 lifecycle cases (our 4 + 3 existing): - warm+nil+finished: synthesized default deletes the claim - warm+nil+active: claim stays, Spec.Lifecycle remains nil, no expired condition (guards premature deletion and spec leak) - warm+explicit Retain: policy preserved after reconciliation - non-warm+nil: immortal behavior unchanged - explicit Retain+TTL=0: sandbox deleted, claim kept (existing) - explicit DeleteForeground+TTL=0: claim deleted (existing) - two-reconcile expired persistence dance (existing) For: kubernetes-sigs#1306 Signed-off-by: vvoronko <vvoronko@redhat.com>
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
Reviewed at 11627ae. Effective-lifecycle handling now covers all policy decision sites and the default is not persisted to spec - thanks for addressing the earlier feedback. One non-blocking doc note inline.
| // are cleaned up. The default is never written back to claim.Spec — callers | ||
| // must use the returned lifecycle for policy decisions. See #1306. | ||
| func (r *SandboxClaimReconciler) checkExpiration(claim *extensionsv1beta1.SandboxClaim) (bool, time.Duration, *extensionsv1beta1.Lifecycle) { | ||
| lc := claim.Spec.Lifecycle |
There was a problem hiding this comment.
The warm-pool Delete+TTL=0 default is a user-visible behavior change documented only in this code comment; please also note it in the Lifecycle field docs / CRD description so users aren't surprised when finished warm-pool claims are auto-deleted. Non-blocking.
There was a problem hiding this comment.
Good catch — you're right that a behavior change like this should be discoverable from the API surface, not just the controller code. Users shouldn't have to read the implementation to understand why their finished warm-pool claims are being cleaned up.
Added the default to the Lifecycle field doc in SandboxClaimSpec (bae52e0) so it shows up in kubectl explain and generated API references. Thank you for the user-facing perspective — it makes the experience much more transparent.
|
/lgtm |
kubernetes-prow
Bot
added
ok-to-test
kubernetes-prow
Bot
added
the
lgtm
…ifecycle field Addresses review feedback: note the controller-injected Delete+TTL=0 default in the CRD field description so the behavior is visible to users via kubectl explain and generated API docs. Signed-off-by: vvoronko <vvoronko@redhat.com>
kubernetes-prow
Bot
removed
the
lgtm
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@extensions/api/v1beta1/sandboxclaim_types.go`:
- Around line 115-117: Before applying the new default described by the
warmPoolRef/Lifecycle behavior, add a compatibility migration for persisted
WarmPool claims with nil Lifecycle so existing claims remain retained during a
deprecation period. Update the controller’s defaulting or reconciliation logic
to distinguish migrated legacy claims from newly created claims, and only
interpret omission as ShutdownPolicy=Delete with TTLSecondsAfterFinished=0 after
the migration path is established.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 27bba214-a6fc-4e91-a5ce-e627126798ba
📒 Files selected for processing (3)
extensions/api/v1beta1/sandboxclaim_types.goextensions/controllers/sandboxclaim_controller.goextensions/controllers/sandboxclaim_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/controllers/sandboxclaim_controller_test.go
|
/retest-required |
janetkuo
added
ready-for-review
and removed
action-required: resolve-copilot-comments
labels
Signed-off-by: vvoronko <vvoronko@redhat.com>
TestWarmPoolSandboxWatcher deletes the pod and observes the sandbox not-ready condition. With the new Delete+TTL=0 default for warm-pool claims, the controller deletes the claim before the test can observe that state. Set explicit Retain so the object survives for inspection. Signed-off-by: vvoronko <vvoronko@redhat.com>
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
Defaulting is now purely in-memory with the non-persistence covered by a regression test, and the behavior change is documented in the API. Thanks for the quick turnaround.
|
/lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aditya-shantanu, vvoronko The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@aditya-shantanu sorry, I have an issue with https://inviter.co/kubernetes slack, could you help me to join the slack channel? Looks like this fix require more discussion. Thanks! |
Summary
SandboxClaimreferences aWarmPoolbut omitsspec.lifecycle,checkExpiration()short-circuits at the nil check and returns(false, 0)— the expiration reconciler never runs, so the claim, sandbox, pod, and VM persist indefinitelyLifecycle{ShutdownPolicy: Delete, TTLSecondsAfterFinished: 0}for warm-pool-sourced claims that omit it — the default is deterministic and reconcile-local (not persisted to the API server)Retainfor debugging) are never overriddenDetails
The root cause is in
checkExpiration(sandboxclaim_controller.go:427):This is correct for bare
Sandboxobjects (GitOps safety — see #201), but warm pool claims are ephemeral by definition and should not accumulate indefinitely. The fix adds a warm pool check before the early return:Follow-up (not in this PR)
A configurable default reaper TTL for explicit
Retainclaims could prevent stale object accumulation for debugging scenarios. See #1306 comment for the full security analysis.Fixes #1306
Test plan
go build ./...passesgo vet ./...passesSummary by CodeRabbit
Bug Fixes
Documentation