feat: client cache for configurable list of gvks - #1042
Open
SoWieMarkus wants to merge 13 commits into
Open
Conversation
📝 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 | ✅ 5✅ Passed checks (5 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)
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.
![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
SoWieMarkus
force-pushed
the
feat/client-cache
branch
from
August 7, 2026 08:29
18796b2 to
2c201bc
Compare
Signed-off-by: Markus Wieland <markus.wieland@sap.com>
SoWieMarkus
force-pushed
the
feat/client-cache
branch
from
August 10, 2026 07:22
5101ce2 to
8f93636
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.
flowchart TB Ctrl["Controller"] CC["CachingClient\nOverlay: {namespace, name} → entry\n• live entry (write pending)\n• tombstone (delete pending)"] MCL["multicluster.Client\n(Routing)"] Home[("Home Cluster\nfoo/my-vm")] R1[("Remote A\nfoo/my-vm ⚠ same key!")] R2[("Remote B")] Ctrl -->|"Write → live entry\nDelete → tombstone"| CC CC -->|"Get: tombstone → NotFound\nGet: live entry → overlay wins"| Ctrl CC --> MCL MCL --> Home MCL --> R1 MCL --> R2 Home -. "Informer Events → evict entry/tombstone" .-> CC R1 -. "Informer Events → evict entry/tombstone" .-> CC R2 -. "Informer Events → evict entry/tombstone" .-> CC