[SANDBOX-1788] refactor the resource cache out of the OwnerFetcher#525
Conversation
… used for other purposes and shared
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughIntroduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OwnerFetcher
participant ResourceCache
participant DiscoveryClient
participant DynamicClient
participant KubeAPI
Caller->>OwnerFetcher: GetOwners(kind, apiVersion)
OwnerFetcher->>ResourceCache: GVRForKind(kind, apiVersion)
ResourceCache->>ResourceCache: ensureResourceList()
alt cache empty
ResourceCache->>DiscoveryClient: ServerPreferredResources()
DiscoveryClient-->>ResourceCache: APIResourceList[]
ResourceCache-->>ResourceCache: cache lists
end
ResourceCache-->>OwnerFetcher: GroupVersionResource (gvr), found
OwnerFetcher->>DynamicClient: Resource(gvr).Get(...)
DynamicClient->>KubeAPI: API request for gvr
KubeAPI-->>DynamicClient: API response (owners)
DynamicClient-->>OwnerFetcher: owners
OwnerFetcher-->>Caller: owners
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client/resource_cache.go`:
- Around line 93-95: The code currently returns immediately when
rc.discoveryClient.ServerPreferredResources() returns an error, dropping partial
resourceList data; change the flow to always cache the returned resourceList
into the resource cache (the same structure used by your GVRForKind/GVKForGR
lookups) even when err != nil, and only treat the error as fatal if a subsequent
lookup (GVRForKind or GVKForGR) cannot be satisfied from that cached data;
detect partial failures using discovery.IsGroupDiscoveryFailedError(err) and
propagate the original err only when a lookup actually fails, otherwise ignore
the discovery partial-error after caching the results.
- Around line 11-14: The ResourceCache lazy init is racy: guard access to
resourceLists with a sync.RWMutex field on ResourceCache and use a
double-checked pattern inside ensureResourceList() — first RLock to test
resourceLists, if nil RUnlock then Lock, check again and if still nil call
discoveryClient.ServerPreferredResources() and assign the returned slice to
resourceLists while holding the Lock; release the Lock and use RLock for
readers. Also, when ServerPreferredResources() returns (lists, err) do not
discard lists on error — assign any partial lists to resourceLists and
surface/log the error instead of returning early so GVRForKind and GVKForGR can
use the partial discovery results.
In `@pkg/owners/fetcher.go`:
- Around line 76-78: The error construction in pkg/owners/fetcher.go incorrectly
takes the addresses of ownerReference.APIVersion and ownerReference.Kind; remove
the address-of operators so the fmt.Errorf call uses the string values directly
(i.e., use ownerReference.APIVersion and ownerReference.Kind in the "GVR not
found for owner reference: %s/%s" message) so the formatted error shows the
actual APIVersion and Kind rather than pointer diagnostics.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ef49d8f2-dc27-4487-988a-22ac96a64301
📒 Files selected for processing (3)
pkg/client/resource_cache.gopkg/client/resource_cache_test.gopkg/owners/fetcher.go
…enable correct concurrent access
|
2 participants
refactor the resource cache out of the OwnerFetcher so that it can be used for other purposes and shared.
I intend to use the new ResourceCache in the Guardian Cockpit for the reverse of what it was used for in the OwnerFetcher.
Summary by CodeRabbit
Improvements
Bug Fixes
Tests