fix(storage): cache the CollapseConfiguration read instead of doing it inside the write lock#343
Conversation
matthyx
added
the
ai-assisted
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR reduces lock hold time during writes by adding a small TTL cache around reading CollapseConfiguration/default from storage, so PreSave no longer performs an uncached storage Get while holding the per-key write lock.
Changes:
- Add a 10s TTL cache (atomic pointer + mutex refresh) to
NewCRDCollapseSettingsProviderto avoid repeated backing storage reads. - Update and expand unit tests to validate cache hit behavior, refresh after expiry, and live-update semantics under eventual consistency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/registry/file/collapse_config_provider.go | Wraps CRD-backed settings provider with an atomic+TTL cache to reduce read overhead during write-critical sections. |
| pkg/registry/file/collapse_config_provider_test.go | Updates existing live-update test for TTL semantics and adds tests for cache-hit and refresh-after-expiry behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return func() dynamicpathdetector.CollapseSettings { | ||
| if c := cache.Load(); c != nil && time.Now().Before(c.expiresAt) { | ||
| return c.settings | ||
| } | ||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| if c := cache.Load(); c != nil && time.Now().Before(c.expiresAt) { // re-check under lock | ||
| return c.settings | ||
| } | ||
| settings := load() | ||
| cache.Store(&cachedCollapseSettings{settings: settings, expiresAt: time.Now().Add(collapseSettingsTTL)}) | ||
| return settings | ||
| } |
| // duration of the test so it doesn't wait a real 10s. | ||
| func TestNewCRDCollapseSettingsProvider_LiveUpdate(t *testing.T) { | ||
| oldTTL := collapseSettingsTTL | ||
| collapseSettingsTTL = 5 * time.Millisecond |
|
Summary:
|
…t inside the write lock PreSave calls CollapseSettings() on every non-time-series Create/Update, which in production resolves to NewCRDCollapseSettingsProvider doing an uncached storage Get against the single shared collapseconfigurations/default key. Since PreSave runs inside s.locks.Lock(key) (CreateWithConn/GuaranteedUpdateWithConn), this disk read extended the exclusive critical section on every write, directly contributing to the lock-hold-time regression behind the ContainerProfile 504s. Wrap the provider's Get in a 10s TTL cache: an atomic.Pointer holds the last resolved settings, refreshed under a mutex with a double-check so only one refresh runs per expiry. Matches the provider's own already-documented tradeoff that next-deflate consistency isn't critical, so bounded staleness (<=10s) is an acceptable relaxation of strictly-fresh reads. No watch/informer wiring — the CR changes rarely enough that TTL is sufficient and much smaller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
fix/collapseconfig-ttl-cache
branch
from
e4149ee to
6abe45d
Compare
|
Summary:
|
Summary
PreSavecallsCollapseSettings()on every non-time-series Create/Update, which in production resolves toNewCRDCollapseSettingsProviderdoing an uncached storageGetagainst the single sharedcollapseconfigurations/defaultkey.PreSaveruns insides.locks.Lock(key), this disk read extended the exclusive critical section on every write — directly contributing to the lock-hold-time regression behind the ContainerProfile 504s.Getin a 10s TTL cache: anatomic.Pointerholds the last resolved settings, refreshed under a mutex with a double-check so only one refresh runs per expiry.Context
Stacked on #342 (lock contention fail-fast). Part of a stack of independently-revertable fixes for the ContainerProfile 504 regression; see #342 for the shared root-cause context and validation methodology.
Test plan
go build ./...,go vet,gofmtcleango test ./pkg/registry/file/...and-racevariant pass (confirms lock-free TTL read path is race-clean)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com