feat: client cache for configurable list of gvks - #1042
Open
SoWieMarkus wants to merge 13 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change adds an in-process client cache for configured Kubernetes resources. It overlays writes on informer reads, evicts entries from informer events, cleans expired entries, and routes manager components through the cache. Reservation caching is enabled in the Helm configuration. ChangesClient cache integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant CachingClient
participant MulticlusterClient
participant Informers
Manager->>CachingClient: Start(ctx)
CachingClient->>MulticlusterClient: GetInformersForKind(ctx, object)
MulticlusterClient->>Informers: Retrieve configured cluster informers
Informers-->>CachingClient: Deliver add/update events
CachingClient->>CachingClient: Evict entries and clean expired data
Manager->>CachingClient: Create, update, get, or list Reservation
CachingClient->>MulticlusterClient: Delegate client operation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
mblos
marked this pull request as ready for review
July 16, 2026 08:11
mblos
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
July 16, 2026 08:11
mblos
reviewed
Jul 16, 2026
mblos
left a comment
mblos
left a comment
Collaborator
There was a problem hiding this comment.
Very nice work :) some first comments (still not completed with reviewing)
| } | ||
|
|
||
| func keyForObject(obj client.Object) objectKey { | ||
| return objectKey{namespace: obj.GetNamespace(), name: obj.GetName()} |
Collaborator
There was a problem hiding this comment.
What if a crd resource has the same name across lh clusters?
Collaborator
Author
There was a problem hiding this comment.
Fair point. Do you have any idea how to resolve that?
I mean we would have to extend objectKey with a cluster discriminator. Problem: the cache is deliberately decoupled from multicluster and Get/List don't carry any ref to the cluster in their requests. Writes also only map that via the routers of the multi cluster client.
Only "solution" I see is to merge cache and multi cluster client, but I don't think that we want that.
PhilippMatthes
requested changes
Jul 20, 2026
PhilippMatthes
left a comment
PhilippMatthes
left a comment
Member
There was a problem hiding this comment.
Thank you for incorporating my feedback from #1015 -- especially, the informer-cache idea and reusing metav1.Duration! I've copied over some thoughts and questions and had some new ones along the way. Thanks for considering my feedback.
| hypervisorOvercommitController.Client = multiclusterClient | ||
| if err := hypervisorOvercommitController.SetupWithManager(mgr); err != nil { | ||
| hypervisorOvercommitController.Client = cachingClient | ||
| if err := hypervisorOvercommitController.SetupWithManager(mgr, multiclusterClient); err != nil { |
Member
There was a problem hiding this comment.
Can this cause issues? Inside the SetupWithManager function, aren't there calls that would need to be tunneled by the caching client, such as defining indexes or resource handlers?
Is it possible to wrap it the other way around? Instead of ctrl.Client -> mcl.Client -> clientcache.Client, do ctrl.Client -> clientcache.Client -> mcl.Client? This wouldn't break the pattern here
Collaborator
Author
There was a problem hiding this comment.
I did it that way, because multiclusterclient has multiple clients. All of these would need to be wrapped.
But I see the issue with the index.
| return err | ||
| } | ||
| if gvk, cached := c.gvkFor(obj); cached { | ||
| c.overlay.upsert(gvk, obj) |
Member
There was a problem hiding this comment.
Is it intended that this method call cannot fail when the expected resource has not been created yet? Doesn't this violate the expected controller-runtime client interface protocol?
Collaborator
Author
There was a problem hiding this comment.
Can you maybe explain a little bit more what you concern is here, because I think I don't understand?
In Patch and Update we first call the inner client method. If that fails, we early return with the error. If there is no error we apply the change also in our cache.
Member
There was a problem hiding this comment.
If the resource is not inside the cache, this points toward a data inconsistency and the method should fail instead. Not?
Collaborator
Author
There was a problem hiding this comment.
The cache only tracks objects that were written during runtime. So if I understand your point correctly, that is the correct behavior
| return nil, fmt.Errorf("clientcache: object for gvk %s does not implement client.Object", gvk) | ||
| } | ||
| return obj, nil | ||
| } |
Member
There was a problem hiding this comment.
Any specific reason why this code is outside client.go?
Collaborator
Author
There was a problem hiding this comment.
The split keeps data structure logic in client.go and controller-runtime lifecycle (manager.Runnable, informer wiring, ticker) in a separate file. I think that makes the code easier to understand. But no strong opinion on that. Can also move this to client.go
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/manager/main.go (1)
520-531: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute the inflight reservation controller through the caching client.
inflight.Controller.SetupWithManagerchecks thatc.Clientis*multicluster.Client, soController{Client: multiclusterClient}bypassescachingClient. Sinceclientcacheconfig enablescortex.cloud/v1alpha1/Reservation, the inflight reservation reconciler can observe informer-laggedReservationreads while writes go directly throughmulticlusterClient; assign an inner caching client instead of a bare multicluster client.🤖 Prompt for 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. In `@cmd/manager/main.go` around lines 520 - 531, Update the inflight controller initialization in the controller setup block to pass the inner caching client configured for Reservation resources, rather than the bare multiclusterClient. Preserve the existing VMClient and SetupWithManager flow, and ensure the assigned Client remains compatible with inflight.Controller’s expected multicluster client type.
🧹 Nitpick comments (2)
pkg/clientcache/cache.go (1)
199-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
fieldSetLockedkeeps only the first value per indexed field.
fields.Setmaps one value per field, so an overlay-only object with several index values matches only one of them. The controller-runtime informer index matches any of the values. AMatchingFieldsquery can therefore miss an overlay-only object. Match the selector against each indexed value instead of building a singlefields.Set.♻️ Proposed change to match any indexed value
- if lo.FieldSelector != nil && !lo.FieldSelector.Empty() { - set := o.fieldSetLocked(gvk, obj) - if !lo.FieldSelector.Matches(set) { - return false - } - } + if lo.FieldSelector != nil && !lo.FieldSelector.Empty() { + if !o.matchesFieldsLocked(gvk, obj, lo.FieldSelector) { + return false + } + }// matchesFieldsLocked reports whether any combination of indexed values for // the GVK satisfies the selector. Callers must hold at least the read lock. func (o *overlay) matchesFieldsLocked(gvk schema.GroupVersionKind, obj client.Object, sel fields.Selector) bool { for _, req := range sel.Requirements() { fn, ok := o.indexers[gvk][req.Field] if !ok { return false } if !slices.Contains(fn(obj), req.Value) { return false } } return true }🤖 Prompt for 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. In `@pkg/clientcache/cache.go` around lines 199 - 212, Replace the single-value fieldSetLocked approach with selector matching that evaluates every indexed value for each requirement. Add or update an overlay method such as matchesFieldsLocked to resolve each requested field, require all selector requirements to pass, and accept a requirement when any value returned by its indexer matches; preserve failure for unregistered fields.internal/scheduling/nova/hypervisor_overcommit_controller.go (1)
220-230: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemoved client validation leaves both the setup path and its test without a deterministic failure.
SetupWithManagerno longer validates the client it builds watches with, and the test that covered that validation now passesniland relies on config loading failing first.
internal/scheduling/nova/hypervisor_overcommit_controller.go#L220-L230: add an explicitmcl == nilguard that returnserrors.New("multicluster client must not be nil")before the config load.internal/scheduling/nova/hypervisor_overcommit_controller_test.go#L710-L734: rename the test to describe the nil-client case and assert the specific returned error instead of accepting any error.🤖 Prompt for 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. In `@internal/scheduling/nova/hypervisor_overcommit_controller.go` around lines 220 - 230, The SetupWithManager path must deterministically reject a nil multicluster client before loading configuration. In internal/scheduling/nova/hypervisor_overcommit_controller.go:220-230, add the specified nil guard returning errors.New("multicluster client must not be nil"); in internal/scheduling/nova/hypervisor_overcommit_controller_test.go:710-734, rename the test to describe the nil-client case and assert that exact error instead of accepting any error.
🤖 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 `@cmd/manager/main.go`:
- Around line 400-414: Update the index registration flow to call IndexField on
cachingClient rather than mcl, ensuring registrations populate overlay.indexers
and MatchingFields includes overlay-only objects. Locate the existing index
setup calls and route each through the clientcache wrapper while preserving
their current fields and index functions.
In `@pkg/clientcache/client.go`:
- Around line 185-189: Update the live overlay handling in Get() to deep-copy
e.obj via DeepCopyObject() before passing it to scheme.Convert, then convert the
copied object into obj. Preserve the existing conversion error propagation and
successful return behavior, ensuring callers cannot mutate the cached overlay
entry through shared maps, slices, or metadata.
In `@pkg/clientcache/runnable.go`:
- Around line 20-55: Add NeedLeaderElection() bool to CachingClient, returning
false, so its Start lifecycle—including eviction handlers and TTL cleanup—runs
on every replica regardless of leader election. Ensure AsRunnable() exposes this
method through the manager.Runnable implementation.
---
Outside diff comments:
In `@cmd/manager/main.go`:
- Around line 520-531: Update the inflight controller initialization in the
controller setup block to pass the inner caching client configured for
Reservation resources, rather than the bare multiclusterClient. Preserve the
existing VMClient and SetupWithManager flow, and ensure the assigned Client
remains compatible with inflight.Controller’s expected multicluster client type.
---
Nitpick comments:
In `@internal/scheduling/nova/hypervisor_overcommit_controller.go`:
- Around line 220-230: The SetupWithManager path must deterministically reject a
nil multicluster client before loading configuration. In
internal/scheduling/nova/hypervisor_overcommit_controller.go:220-230, add the
specified nil guard returning errors.New("multicluster client must not be nil");
in internal/scheduling/nova/hypervisor_overcommit_controller_test.go:710-734,
rename the test to describe the nil-client case and assert that exact error
instead of accepting any error.
In `@pkg/clientcache/cache.go`:
- Around line 199-212: Replace the single-value fieldSetLocked approach with
selector matching that evaluates every indexed value for each requirement. Add
or update an overlay method such as matchesFieldsLocked to resolve each
requested field, require all selector requirements to pass, and accept a
requirement when any value returned by its indexer matches; preserve failure for
unregistered fields.
🪄 Autofix
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: 404cddc7-8f51-4e05-a9bc-9fb4f2791c3b
📒 Files selected for processing (12)
cmd/manager/main.gohelm/bundles/cortex-nova/values.yamlinternal/scheduling/nova/hypervisor_overcommit_controller.gointernal/scheduling/nova/hypervisor_overcommit_controller_test.gopkg/clientcache/cache.gopkg/clientcache/cache_test.gopkg/clientcache/client.gopkg/clientcache/client_test.gopkg/clientcache/config.gopkg/clientcache/interfaces.gopkg/clientcache/runnable.gopkg/multicluster/client.go
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
…erlay stale reads during concurrent updates
…ject in Get method
…nagement across replicas
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
SoWieMarkus
force-pushed
the
feat/client-cache
branch
from
August 7, 2026 08:29
18796b2 to
2c201bc
Compare
Contributor
Test Coverage ReportTest Coverage 📊: 70.7% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.