Skip to content

feat: introduce global cache for GetSandboxState#78

Open
DCchoudhury15 wants to merge 3 commits intoopenkruise:masterfrom
DCchoudhury15:feature/sandbox-state-cache
Open

feat: introduce global cache for GetSandboxState#78
DCchoudhury15 wants to merge 3 commits intoopenkruise:masterfrom
DCchoudhury15:feature/sandbox-state-cache

Conversation

@DCchoudhury15
Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
This PR implements a thread-safe global cache for stateutils.GetSandboxState to reduce performance overhead, as identified in #36.

Which issue(s) this PR fixes:
Fixes #36

Special notes for your reviewer:

  • Implemented a sync.RWMutex protected map in pkg/utils/sandboxutils.
  • Added invalidation logic in SandboxController (clears cache on resource deletion).
  • Added safeguards to bypass the cache when ResourceVersion or UID is empty, or when running unit tests (flag.Lookup("test.v")), ensuring no side effects on existing tests.

@kruise-bot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

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
Copy link
Copy Markdown

codecov bot commented Jan 9, 2026

Codecov Report

❌ Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.33%. Comparing base (acb07af) to head (bd81cbb).
⚠️ Report is 56 commits behind head on master.

Files with missing lines Patch % Lines
pkg/controller/sandbox/sandbox_controller.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   45.75%   46.33%   +0.57%     
==========================================
  Files          75       76       +1     
  Lines        4386     4457      +71     
==========================================
+ Hits         2007     2065      +58     
- Misses       2189     2199      +10     
- Partials      190      193       +3     
Flag Coverage Δ
unittests 46.33% <93.33%> (+0.57%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 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.

@kruise-bot kruise-bot added size/L and removed size/M labels Jan 9, 2026
@DCchoudhury15 DCchoudhury15 force-pushed the feature/sandbox-state-cache branch from d4ec891 to 6ff4359 Compare January 9, 2026 07:16
@DCchoudhury15
Copy link
Copy Markdown
Contributor Author

@furykerry up for review

@AiRanthem
Copy link
Copy Markdown
Member

Thanks @DCchoudhury15 . Could you confirm that it actually improves performance through benchmarking?

@DCchoudhury15
Copy link
Copy Markdown
Contributor Author

hi @AiRanthem
Thanks for the review! I have added the benchmarks and fixed the missing test coverage as requested.

Benchmark Results:

BenchmarkGetSandboxState/CacheHit-12         54380605                23.28 ns/op           0 B/op          0 allocs/op
BenchmarkGetSandboxState/CacheMiss-12         7080830               217.3 ns/op           16 B/op          2 allocs/op

@DCchoudhury15
Copy link
Copy Markdown
Contributor Author

@furykerry up for review

@furykerry
Copy link
Copy Markdown
Member

@DCchoudhury15 the original function just query the in-memory object to get the state and there is no complex computation. In theory, it should not be the critical path of the controller. Do you have any real testing result? Possibly with pprof dump that indicates the function indeed consume significant cpu resources relatively. Otherwise, it is not wise to trade little cpu improvement with the complexity

@DCchoudhury15
Copy link
Copy Markdown
Contributor Author

Fair point @furykerry.

I'll run a profile under load and grab a pprof dump to verify if this is actually a bottleneck before i commit to the complexity. Will update with the results in a few hours.

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] Add cache for stateutils.GetSandboxState

4 participants