Skip to content

fix(sandboxclaim): avoid claim identity collisions#590

Open
chacha923 wants to merge 2 commits into
openkruise:masterfrom
chacha923:fix/sandboxclaim-identity-count
Open

fix(sandboxclaim): avoid claim identity collisions#590
chacha923 wants to merge 2 commits into
openkruise:masterfrom
chacha923:fix/sandboxclaim-identity-count

Conversation

@chacha923

Copy link
Copy Markdown
Contributor

Ⅰ. Describe what this PR does

This PR fixes SandboxClaim claimed-sandbox counting so a claim is identified by both internal claim-name bookkeeping and the claim UID.

The previous flow could become unstable because agents.kruise.io/claim-name is an internal label but user-provided spec.labels could override it, while claimed-sandbox recovery did not narrow the cache query by claim name. This could make recreated same-name claims or later reconciliations report incorrect ClaimedReplicas.

Key changes:

  1. Add a claim-name sandbox cache index and extend ListSandboxesOptions with ClaimName.
  2. Apply claim-name filtering together with the existing claim UID owner filter in ListSandboxes and CountActiveSandboxes.
  3. Pass ClaimName: claim.Name from SandboxClaim claimed-sandbox recovery.
  4. Preserve the internal agents.kruise.io/claim-name label after applying user labels so spec.labels cannot override it.
  5. Add regression tests for recreated same-name claims, combined claim-name/UID filtering, dead sandbox exclusion, and user-provided claim-name label overrides.

Ⅱ. Does this pull request fix one issue?

Fixes #311

Ⅲ. Describe how to verify it

go test -count=1 ./pkg/cache/... ./pkg/controller/sandboxclaim/core/...
git diff --check

Ⅳ. Special notes for reviews

  • No CRD/API changes are introduced, so make generate and make manifests are not required.
  • The new claim-name index is registered through GetIndexFuncs, keeping production cache and test cache index definitions consistent.

@kruise-bot kruise-bot requested review from furykerry and zmberg June 30, 2026 07:42
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign furykerry for approval by writing /assign @furykerry in a comment. For more information see:The Kubernetes 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:

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

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.96%. Comparing base (3a3000b) to head (c9a1771).

Files with missing lines Patch % Lines
pkg/cache/cache.go 84.61% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   79.94%   79.96%   +0.02%     
==========================================
  Files         202      202              
  Lines       14883    14902      +19     
==========================================
+ Hits        11898    11917      +19     
  Misses       2556     2556              
  Partials      429      429              
Flag Coverage Δ
unittests 79.96% <90.90%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

SandboxClaim claimed-sandbox recovery now uses stable claim identity instead of depending on a user-overridable claim-name label alone.

Changes:
- Add a claim-name sandbox cache index and ClaimName filter to sandbox list/count options.
- Combine claim-name filtering with claim UID owner matching when counting active claimed sandboxes.
- Preserve the internal agents.kruise.io/claim-name label after applying user labels so spec.labels cannot override it.
- Add regression tests for recreated same-name claims, claim-name/UID filtering, and internal label override attempts.

Refs openkruise#311

Signed-off-by: zhuangzhewei <zhuangzhewei09@dingtalk.com>
…xclaim-identity-count

Resolve the cache interface conflict by preserving the upstream reserved-failed sandbox documentation together with the SandboxClaim claim-name filter documentation.

Signed-off-by: zhuangzhewei <zhuangzhewei09@dingtalk.com>
@chacha923 chacha923 marked this pull request as ready for review June 30, 2026 11:44
@kruise-bot kruise-bot requested a review from PersistentJZH June 30, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature request] Avoid identity collisions in SandboxClaim

2 participants