From 37f5dd9e5fb05e5320170b35913a9a45b9982975 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 14 Feb 2026 19:58:28 -0500 Subject: [PATCH 01/25] Start --- pkg/cache/cache.go | 5 + pkg/client/consistency.go | 214 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+) create mode 100644 pkg/client/consistency.go diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 2a6a6c6be0..926b792d27 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -133,6 +133,11 @@ type Informer interface { // IsStopped returns true if the informer has been stopped. IsStopped() bool + + // LastSyncResourceVersion is the resource version observed when last synced with the underlying + // store. The value returned is not synchronized with access to the underlying store and is not + // thread-safe. + LastSyncResourceVersion() string } // AllNamespaces should be used as the map key to deliminate namespace settings diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go new file mode 100644 index 0000000000..a2d087d3fd --- /dev/null +++ b/pkg/client/consistency.go @@ -0,0 +1,214 @@ +package client + +import ( + "context" + "errors" + "fmt" + "strconv" + "sync" + "time" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + toolscache "k8s.io/client-go/tools/cache" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" +) + +type informerGetter interface { + GetInformer(context.Context, schema.GroupVersionKind) (informer, error) +} + +type informer interface { + AddEventHandler(handler toolscache.ResourceEventHandler) (toolscache.ResourceEventHandlerRegistration, error) + LastSyncResourceVersion() string +} + +func ConsistentClient(upstream Client, ig informerGetter) Client { + +} + +type consistentClient struct { + informerGetter informerGetter + scheme *runtime.Scheme + + trackers map[schema.GroupVersionKind]*highestSeenRVTracker + trackersLock sync.Mutex + + lockedKeysByGVK map[schema.GroupVersionKind]*lockedKeys + lockedKeysLock sync.Mutex +} + +type lockedKeys struct { + lock sync.RWMutex + lockedKeys map[types.NamespacedName]rvWaiter +} + +type rvWaiter struct { + waitingForRV int64 + done chan struct{} +} + +func (c *consistentClient) waitForObject(ctx context.Context, obj Object) error { + gvk, err := apiutil.GVKForObject(obj, c.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + } + key := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + + c.lockedKeysLock.Lock() + keys := c.lockedKeysByGVK[gvk] + c.lockedKeysLock.Unlock() + if keys == nil { + return nil + } + + done := func() chan struct{} { + keys.lock.RLock() + defer keys.lock.RUnlock() + + waiter, found := keys.lockedKeys[key] + if !found { + return nil + } + + return waiter.done + }() + + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.NewTimer(5 * time.Second).C: + return errors.New("timed out waiting for cache to have latest changes to object") + case <-done: + return nil + } +} + +func (c *consistentClient) blockForObject(ctx context.Context, obj Object) error { + gvk, err := apiutil.GVKForObject(obj, c.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + } + + rvRaw := obj.GetResourceVersion() + rv, err := strconv.ParseInt(rvRaw, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + } + + tracker, err := c.trackerFor(ctx, gvk) + if err != nil { + return fmt.Errorf("failed to set up tracker for %v: %v", obj, err) + } + + key := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + c.lockedKeysLock.Lock() + if _, exists := c.lockedKeysByGVK[gvk]; !exists { + c.lockedKeysByGVK[gvk] = &lockedKeys{lockedKeys: make(map[types.NamespacedName]rvWaiter)} + } + keys := c.lockedKeysByGVK[gvk] + c.lockedKeysLock.Unlock() + + keys.lock.Lock() + defer keys.lock.Unlock() + if existing, alreadyWaiting := keys.lockedKeys[key]; alreadyWaiting && existing.waitingForRV >= rv { + return nil + } + + keys.lockedKeys[key] = rvWaiter{ + waitingForRV: rv, + done: tracker.blockUntil(ctx, rv), + } + + return nil +} + +func (c *consistentClient) trackerFor(ctx context.Context, obj schema.GroupVersionKind) (*highestSeenRVTracker, error) { + c.trackersLock.Lock() + defer c.trackersLock.Unlock() + + tracker, ok := c.trackers[obj] + if ok { + return tracker, nil + } + + informer, err := c.informerGetter.GetInformer(ctx, obj) + if err != nil { + return nil, fmt.Errorf("Failed to get informer for %v: %v", obj, err) + } + + stringRV := informer.LastSyncResourceVersion() + rv, err := strconv.ParseInt(stringRV, 10, 64) + if err != nil { + return nil, fmt.Errorf("Failed to parse resource version %s: %v", stringRV, err) + } + + c.trackers[obj] = &highestSeenRVTracker{ + rv: rv, + cond: &sync.Cond{}, + } + + return c.trackers[obj], nil +} + +type highestSeenRVTracker struct { + rv int64 + cond *sync.Cond +} + +func (h *highestSeenRVTracker) blockUntil(ctx context.Context, rv int64) chan struct{} { + c := make(chan struct{}) + go func() { + h.cond.L.Lock() + for h.rv < rv { + h.cond.Wait() + } + h.cond.L.Unlock() + close(c) + }() + + return c +} + +func (h *highestSeenRVTracker) update(rv string) { + parsed, err := strconv.ParseInt(rv, 10, 64) + if err != nil { + fmt.Printf("Failed to parse resource version %s: %v\n", rv, err) + return + } + + h.cond.L.Lock() + defer h.cond.L.Unlock() + + if parsed > h.rv { + h.rv = parsed + h.cond.Broadcast() + } +} + +func (h *highestSeenRVTracker) OnAdd(raw interface{}, isInInitialList bool) { + obj, ok := raw.(Object) + if !ok { + // Never expected, should we log an error? + return + } + go func() { h.update(obj.GetResourceVersion()) }() +} +func (h *highestSeenRVTracker) OnUpdate(oldObj, newObj interface{}) { + obj, ok := newObj.(Object) + if !ok { + // Never expected, should we log an error? + return + } + go func() { h.update(obj.GetResourceVersion()) }() +} +func (h *highestSeenRVTracker) OnDelete(raw interface{}) { + obj, ok := raw.(Object) + if !ok { + // Could be cache.DeletedFinalStateUnknown, will we + // get the latest RV through `OnUpdate` if that happens? + return + } + go func() { h.update(obj.GetResourceVersion()) }() +} From 79b69a87db35a109e948ccc2eb196c9c9271985e Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Thu, 5 Mar 2026 20:06:26 +0100 Subject: [PATCH 02/25] Changes --- designs/read_your_own_write_client.md | 60 ++++++++ pkg/client/consistency.go | 193 ++++++++++++++++++-------- 2 files changed, 197 insertions(+), 56 deletions(-) create mode 100644 designs/read_your_own_write_client.md diff --git a/designs/read_your_own_write_client.md b/designs/read_your_own_write_client.md new file mode 100644 index 0000000000..f8a381a087 --- /dev/null +++ b/designs/read_your_own_write_client.md @@ -0,0 +1,60 @@ +Read your own write client +========================== + +## Goals + +* Any read from the client (`Get` or `List`) that starts after a write started will observe that write. The + reason we consider the start and not the end time of the write to be the cutoff where reads must observe + it is that we don't know when exactly the server applied it, as there is delay between the write being + applied and the writer getting to know its write got applied. +* Reads do not get blocked on writes that started after they started, as otherwise they may end up getting + blocked indefinitely if many writes are happening. +* Writes themselves do not get blocked waiting for them to be observable by reads because: + * This may lead to successful writes returning an error. While that should not result in incorrect behavior + of a controller as controllers are expected to be idempotent, it would lead to performance degradation, as + the error will typical lead to a retry with backoff and any work done before encountering it has to be + re-done + * Waiting for the read to make it back into the cache may entail setting up an informer, which takes + significant time + +## Non-Goals + +* Any sort of consistency with writes originating from a different client - The only way to do this would be + to do a get or list against the api at which point it doesn't make sense to have a cache-backed client at + all +* Client reads observe writes that succeeded, but where the information if the write succeeded didn't make it + back to the client, because for example the connection was interrupted. This is primarily for practical + purposes, if the write doesn't get a response indicating success or failure back, it is impossible to tell if + the write did or did not succeed +* Dealing with configurations where the cache doesn't contain all objects that are written, for example due to + label or field selectors: This is essentially a misconfiguration and non-trivial to detect. We may or may not + look into detecting this in the future + +## Implementation + +The implementation is gated behind a new and default-off client option `ConsistentReads bool`. If set, mutating +operations will: +* get sequentialized by gvk+key: This is purely to simplify implementation, we may or may not change this in the future +* block following get calls for the same gvk and object key and list calls for the gvk + +Once the operation returned, it will: +1. Update the cache through a new internal-only `SetMinimumRVForGVKAndKey` method, which instructs it to block + all subsequent Get calls for the given GVK and key and all list calls for the given gvk until the passed RV was observed +2. Unblock reads + +A special challenge are Delete calls. There, the response is either the object including new RV if the object was +not deleted from storage or a metav1.Status without RV if the object was deleted from storage. To deal with this, +we will start deserializing the delete response into an unstructured and test which of the two it is. If it is the +object, we follow the same RV-waiting approach as for the other operations. If it is a success status, we will: +1. Update the cache through a new internal-only `AddRequiredDeleteForGVKKeyAndUID` method, which will make the cache + append the UID to a per-gvk and key set. Whenever a delete event for a gvk and key is observed, the uid will be + removed from the set. The cache then blocks gets on the gvk+key set being empty and lists on the gvk set being empty +2. Unblock reads + +*Note:* The above only fulfills the goal `Reads do not get blocked on writes that started after they started` by assuming +that reads read the RV/deleted object set in the cache before any subsequent write calls `SetMinimumRVForGVKAndKey` or +`AddRequiredDeleteForGVKKeyAndUID`. The underlying information in the cache will be protected by a mutex and copied for +reads. While golang mutexes do not guarantee any acquisition order, writes entail a networking roundtrip, so we assume +that the read will always acquire the mutex before subsequent writes do. In the extremely unlikely case that they do not, +the result would only be a performance degradation (we wait for additional writes), not a correctness issue which is deemed +acceptable. diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index a2d087d3fd..1dc06753f8 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -2,11 +2,9 @@ package client import ( "context" - "errors" "fmt" "strconv" "sync" - "time" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -29,99 +27,141 @@ func ConsistentClient(upstream Client, ig informerGetter) Client { } type consistentClient struct { + upstream Client informerGetter informerGetter scheme *runtime.Scheme trackers map[schema.GroupVersionKind]*highestSeenRVTracker trackersLock sync.Mutex - lockedKeysByGVK map[schema.GroupVersionKind]*lockedKeys - lockedKeysLock sync.Mutex + // lockedKeysByGVK maps gvk -> key -> keyLocker + lockedKeysByGVK threadSafeMap[schema.GroupVersionKind, *threadSafeMap[types.NamespacedName, *keyLocker]] } -type lockedKeys struct { - lock sync.RWMutex - lockedKeys map[types.NamespacedName]rvWaiter -} - -type rvWaiter struct { - waitingForRV int64 - done chan struct{} -} - -func (c *consistentClient) waitForObject(ctx context.Context, obj Object) error { +func (c *consistentClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error { gvk, err := apiutil.GVKForObject(obj, c.scheme) if err != nil { - return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + return fmt.Errorf("failed to get GVK for object %T: %v", obj, err) } - key := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} - c.lockedKeysLock.Lock() - keys := c.lockedKeysByGVK[gvk] - c.lockedKeysLock.Unlock() - if keys == nil { - return nil + keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(key) + if err := keyLock.wait(ctx); err != nil { + return err } - done := func() chan struct{} { - keys.lock.RLock() - defer keys.lock.RUnlock() - - waiter, found := keys.lockedKeys[key] - if !found { - return nil - } + return c.upstream.Get(ctx, key, obj, opts...) +} - return waiter.done - }() +func (c *consistentClient) List(ctx context.Context, list ObjectList, opts ...ListOption) error { + gvk, err := apiutil.GVKForObject(list, c.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for list %T: %v", list, err) + } - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.NewTimer(5 * time.Second).C: - return errors.New("timed out waiting for cache to have latest changes to object") - case <-done: - return nil + keys := c.lockedKeysByGVK.getOrCreate(gvk).allValues() + for _, keyLock := range keys { + if err := keyLock.wait(ctx); err != nil { + return err + } } + + return c.upstream.List(ctx, list, opts...) } -func (c *consistentClient) blockForObject(ctx context.Context, obj Object) error { +func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error { gvk, err := apiutil.GVKForObject(obj, c.scheme) if err != nil { return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) } + namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + + keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) + keyLock.lock() + defer keyLock.unlock() + + if err := c.upstream.Update(ctx, obj, opts...); err != nil { + return err + } + rvRaw := obj.GetResourceVersion() rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) } - tracker, err := c.trackerFor(ctx, gvk) - if err != nil { - return fmt.Errorf("failed to set up tracker for %v: %v", obj, err) + go func() { + _ = c.blockFor(ctx, gvk, rv) + }() + return nil +} + +type lockedKeys struct { + lock sync.RWMutex + lockedKeys map[types.NamespacedName]*keyLocker +} + +type keyLocker struct { + // mutex must be held to access any other field + mutex sync.Mutex + + // holders counts the holders. If zero, done + // may be nil + holders int + done chan struct{} +} + +func (l *keyLocker) lock() { + l.mutex.Lock() + defer l.mutex.Unlock() + + if l.holders == 0 { + l.done = make(chan struct{}) } + l.holders++ +} - key := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} - c.lockedKeysLock.Lock() - if _, exists := c.lockedKeysByGVK[gvk]; !exists { - c.lockedKeysByGVK[gvk] = &lockedKeys{lockedKeys: make(map[types.NamespacedName]rvWaiter)} +func (l *keyLocker) unlock() { + l.mutex.Lock() + defer l.mutex.Unlock() + + l.holders-- + if l.holders == 0 { + close(l.done) } - keys := c.lockedKeysByGVK[gvk] - c.lockedKeysLock.Unlock() +} - keys.lock.Lock() - defer keys.lock.Unlock() - if existing, alreadyWaiting := keys.lockedKeys[key]; alreadyWaiting && existing.waitingForRV >= rv { +func (l *keyLocker) wait(ctx context.Context) error { + l.mutex.Lock() + if l.holders == 0 { + l.mutex.Unlock() return nil } + done := l.done + l.mutex.Unlock() - keys.lockedKeys[key] = rvWaiter{ - waitingForRV: rv, - done: tracker.blockUntil(ctx, rv), + select { + case <-ctx.Done(): + return ctx.Err() + case <-done: + return nil } +} - return nil +// blockForObject blocks until the context is done or the RV in obj is observed. It may end up +// creating an informer. +func (c *consistentClient) blockFor(ctx context.Context, gvk schema.GroupVersionKind, rv int64) error { + tracker, err := c.trackerFor(ctx, gvk) + if err != nil { + return fmt.Errorf("failed to set up tracker for %v: %v", gvk, err) + } + + select { + case <-tracker.blockUntil(ctx, rv): + return nil + case <-ctx.Done(): + return ctx.Err() + } } func (c *consistentClient) trackerFor(ctx context.Context, obj schema.GroupVersionKind) (*highestSeenRVTracker, error) { @@ -212,3 +252,44 @@ func (h *highestSeenRVTracker) OnDelete(raw interface{}) { } go func() { h.update(obj.GetResourceVersion()) }() } + +type threadSafeMap[k comparable, v any] struct { + lock sync.Mutex + data map[k]v +} + +func (t *threadSafeMap[k, v]) getOrCreate(key k) v { + t.lock.Lock() + defer t.lock.Unlock() + + val, exists := t.data[key] + if !exists { + if t.data == nil { + t.data = make(map[k]v) + } + t.data[key] = *new(v) + val = t.data[key] + } + + return val +} + +func (t *threadSafeMap[k, v]) get(key k) (v, bool) { + t.lock.Lock() + defer t.lock.Unlock() + + val, ok := t.data[key] + return val, ok +} + +func (t *threadSafeMap[k, v]) allValues() []v { + t.lock.Lock() + defer t.lock.Unlock() + + result := make([]v, 0, len(t.data)) + for _, val := range t.data { + result = append(result, val) + } + + return result +} From 4ec70a5e0d826df9805b22d732050cfd925dbb27 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Fri, 6 Mar 2026 16:36:13 +0100 Subject: [PATCH 03/25] Cache: Implement SetMinimumRVForGVKAndKey --- pkg/cache/cache.go | 15 +++ pkg/cache/delegating_by_gvk_cache.go | 4 + pkg/cache/informer_cache.go | 67 +++++++++++++ pkg/cache/informertest/fake_cache.go | 4 + pkg/cache/multi_namespace_cache.go | 25 +++++ pkg/cache/rv_tracker.go | 139 +++++++++++++++++++++++++++ 6 files changed, 254 insertions(+) create mode 100644 pkg/cache/rv_tracker.go diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 926b792d27..c6083155b0 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -68,6 +68,19 @@ type Cache interface { // Informers loads informers and adds field indices. Informers + + // SetMinimumRVForGVKAndKey causes subsequent Get requests for the given + // GVK and key to block until the informer for that GVK has observed a + // resource version >= rv (or the context times out). For List requests + // on the given GVK, it blocks until the highest minimum RV across all + // keys for that GVK has been observed. + // + // The rv value is snapshotted at the time of the Get/List call, so + // calling SetMinimumRVForGVKAndKey again while a Get/List is already + // waiting does not affect the in-flight wait. + // + // TODO: This shouldn't be part of the public interface + SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) } // Informers knows how to create or fetch informers for different @@ -531,6 +544,8 @@ func newCache(restConfig *rest.Config, opts Options) newCacheFunc { NewInformer: opts.NewInformer, }), readerFailOnMissingInformer: opts.ReaderFailOnMissingInformer, + minimums: newMinimumRVStore(), + trackers: make(map[schema.GroupVersionKind]*highestSeenRVTracker), } } } diff --git a/pkg/cache/delegating_by_gvk_cache.go b/pkg/cache/delegating_by_gvk_cache.go index adc5d957a4..0485c18ca7 100644 --- a/pkg/cache/delegating_by_gvk_cache.go +++ b/pkg/cache/delegating_by_gvk_cache.go @@ -69,6 +69,10 @@ func (dbt *delegatingByGVKCache) GetInformer(ctx context.Context, obj client.Obj return cache.GetInformer(ctx, obj, opts...) } +func (dbt *delegatingByGVKCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { + dbt.cacheForGVK(gvk).SetMinimumRVForGVKAndKey(gvk, key, rv) +} + func (dbt *delegatingByGVKCache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, opts ...InformerGetOption) (Informer, error) { return dbt.cacheForGVK(gvk).GetInformerForKind(ctx, gvk, opts...) } diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index f8a1faa7b9..d29ebc608c 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -19,7 +19,9 @@ package cache import ( "context" "fmt" + "strconv" "strings" + "sync" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -59,6 +61,10 @@ type informerCache struct { scheme *runtime.Scheme *internal.Informers readerFailOnMissingInformer bool + + minimums *minimumRVStore + trackers map[schema.GroupVersionKind]*highestSeenRVTracker + trackersLock sync.Mutex } // Get implements Reader. @@ -76,6 +82,20 @@ func (ic *informerCache) Get(ctx context.Context, key client.ObjectKey, out clie if !started { return &ErrCacheNotStarted{} } + + if minRV, ok := ic.minimums.GetForKey(gvk, key); ok { + tracker, err := ic.trackerFor(ctx, gvk) + if err != nil { + return err + } + select { + case <-tracker.blockUntil(minRV): + case <-ctx.Done(): + return ctx.Err() + } + ic.minimums.Cleanup(gvk, minRV) + } + return cache.Reader.Get(ctx, key, out, opts...) } @@ -95,6 +115,19 @@ func (ic *informerCache) List(ctx context.Context, out client.ObjectList, opts . return &ErrCacheNotStarted{} } + if minRV, ok := ic.minimums.GetMaxForGVK(*gvk); ok { + tracker, err := ic.trackerFor(ctx, *gvk) + if err != nil { + return err + } + select { + case <-tracker.blockUntil(minRV): + case <-ctx.Done(): + return ctx.Err() + } + ic.minimums.Cleanup(*gvk, minRV) + } + return cache.Reader.List(ctx, out, opts...) } @@ -177,6 +210,40 @@ func (ic *informerCache) getInformerForKind(ctx context.Context, gvk schema.Grou return started, cache, nil } +func (ic *informerCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { + ic.minimums.Set(gvk, key, rv) +} + +func (ic *informerCache) trackerFor(ctx context.Context, gvk schema.GroupVersionKind) (*highestSeenRVTracker, error) { + ic.trackersLock.Lock() + defer ic.trackersLock.Unlock() + + if tracker, ok := ic.trackers[gvk]; ok { + return tracker, nil + } + + informer, err := ic.GetInformerForKind(ctx, gvk, BlockUntilSynced(false)) + if err != nil { + return nil, fmt.Errorf("failed to get informer for %v: %w", gvk, err) + } + + var initialRV int64 + if lastSynced := informer.LastSyncResourceVersion(); lastSynced != "" { + initialRV, err = strconv.ParseInt(lastSynced, 10, 64) + if err != nil { + return nil, fmt.Errorf("failed to parse last synced resource version %q for %v: %w", lastSynced, gvk, err) + } + } + + tracker := newHighestSeenRVTracker(initialRV) + if _, err := informer.AddEventHandler(tracker); err != nil { + return nil, fmt.Errorf("failed to add event handler for %v: %w", gvk, err) + } + + ic.trackers[gvk] = tracker + return tracker, nil +} + // RemoveInformer deactivates and removes the informer from the cache. func (ic *informerCache) RemoveInformer(_ context.Context, obj client.Object) error { gvk, err := apiutil.GVKForObject(obj, ic.scheme) diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index 3ca4705e57..2f933eaa64 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -131,6 +131,10 @@ func (c *FakeInformers) IndexField(ctx context.Context, obj client.Object, field return nil } +// SetMinimumRVForGVKAndKey implements Cache. +func (c *FakeInformers) SetMinimumRVForGVKAndKey(_ schema.GroupVersionKind, _ client.ObjectKey, _ int64) { +} + // Get implements Cache. func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { return nil diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go index 592519c35d..807b0e9f84 100644 --- a/pkg/cache/multi_namespace_cache.go +++ b/pkg/cache/multi_namespace_cache.go @@ -220,6 +220,22 @@ func (c *multiNamespaceCache) IndexField(ctx context.Context, obj client.Object, return nil } +func (c *multiNamespaceCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { + if key.Namespace == "" { + if c.clusterCache != nil { + c.clusterCache.SetMinimumRVForGVKAndKey(gvk, key, rv) + } + return + } + if cache, ok := c.namespaceToCache[key.Namespace]; ok { + cache.SetMinimumRVForGVKAndKey(gvk, key, rv) + return + } + if global, ok := c.namespaceToCache[metav1.NamespaceAll]; ok { + global.SetMinimumRVForGVKAndKey(gvk, key, rv) + } +} + func (c *multiNamespaceCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { isNamespaced, err := apiutil.IsObjectNamespaced(obj, c.Scheme, c.RESTMapper) if err != nil { @@ -499,3 +515,12 @@ func (i *multiNamespaceInformer) IsStopped() bool { } return true } + +// LastSyncResourceVersion returns the resource version from the last namespace informer. +func (i *multiNamespaceInformer) LastSyncResourceVersion() string { + var rv string + for _, informer := range i.namespaceToInformer { + rv = informer.LastSyncResourceVersion() + } + return rv +} diff --git a/pkg/cache/rv_tracker.go b/pkg/cache/rv_tracker.go new file mode 100644 index 0000000000..29ee1b95fc --- /dev/null +++ b/pkg/cache/rv_tracker.go @@ -0,0 +1,139 @@ +package cache + +import ( + "strconv" + "sync" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type minimumRVStore struct { + mu sync.Mutex + minimums map[schema.GroupVersionKind]map[client.ObjectKey]int64 +} + +func newMinimumRVStore() *minimumRVStore { + return &minimumRVStore{ + minimums: make(map[schema.GroupVersionKind]map[client.ObjectKey]int64), + } +} + +func (s *minimumRVStore) Set(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.minimums[gvk] == nil { + s.minimums[gvk] = make(map[client.ObjectKey]int64) + } + s.minimums[gvk][key] = rv +} + +func (s *minimumRVStore) GetForKey(gvk schema.GroupVersionKind, key client.ObjectKey) (int64, bool) { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok { + return 0, false + } + rv, ok := keys[key] + return rv, ok +} + +func (s *minimumRVStore) GetMaxForGVK(gvk schema.GroupVersionKind) (int64, bool) { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok || len(keys) == 0 { + return 0, false + } + var maxRV int64 + for _, rv := range keys { + if rv > maxRV { + maxRV = rv + } + } + return maxRV, true +} + +func (s *minimumRVStore) Cleanup(gvk schema.GroupVersionKind, currentRV int64) { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok { + return + } + for key, rv := range keys { + if rv <= currentRV { + delete(keys, key) + } + } +} + +type highestSeenRVTracker struct { + rv int64 + cond *sync.Cond +} + +func newHighestSeenRVTracker(initialRV int64) *highestSeenRVTracker { + return &highestSeenRVTracker{ + rv: initialRV, + cond: sync.NewCond(&sync.Mutex{}), + } +} + +func (h *highestSeenRVTracker) blockUntil(rv int64) <-chan struct{} { + ch := make(chan struct{}) + go func() { + h.cond.L.Lock() + for h.rv < rv { + h.cond.Wait() + } + h.cond.L.Unlock() + close(ch) + }() + return ch +} + +func (h *highestSeenRVTracker) update(rv string) { + parsed, err := strconv.ParseInt(rv, 10, 64) + if err != nil { + return + } + + h.cond.L.Lock() + defer h.cond.L.Unlock() + + if parsed > h.rv { + h.rv = parsed + h.cond.Broadcast() + } +} + +func (h *highestSeenRVTracker) OnAdd(raw any, _ bool) { + obj, ok := raw.(client.Object) + if !ok { + return + } + go func() { h.update(obj.GetResourceVersion()) }() +} + +func (h *highestSeenRVTracker) OnUpdate(_, newObj any) { + obj, ok := newObj.(client.Object) + if !ok { + return + } + go func() { h.update(obj.GetResourceVersion()) }() +} + +func (h *highestSeenRVTracker) OnDelete(raw any) { + obj, ok := raw.(client.Object) + if !ok { + return + } + go func() { h.update(obj.GetResourceVersion()) }() +} From 7615bb2dca6fcdcdac38747c7fb96ce90b2e931f Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Fri, 6 Mar 2026 21:40:25 +0100 Subject: [PATCH 04/25] Implement root resource get/list/patch/update --- pkg/cache/rv_tracker.go | 6 ++ pkg/client/consistency.go | 209 +++++++++++++------------------------- 2 files changed, 74 insertions(+), 141 deletions(-) diff --git a/pkg/cache/rv_tracker.go b/pkg/cache/rv_tracker.go index 29ee1b95fc..1b4031edd7 100644 --- a/pkg/cache/rv_tracker.go +++ b/pkg/cache/rv_tracker.go @@ -1,6 +1,7 @@ package cache import ( + "fmt" "strconv" "sync" @@ -102,6 +103,7 @@ func (h *highestSeenRVTracker) blockUntil(rv int64) <-chan struct{} { func (h *highestSeenRVTracker) update(rv string) { parsed, err := strconv.ParseInt(rv, 10, 64) if err != nil { + fmt.Printf("Failed to parse resource version %s: %v\n", rv, err) return } @@ -117,6 +119,7 @@ func (h *highestSeenRVTracker) update(rv string) { func (h *highestSeenRVTracker) OnAdd(raw any, _ bool) { obj, ok := raw.(client.Object) if !ok { + // Never expected, should we log an error? return } go func() { h.update(obj.GetResourceVersion()) }() @@ -125,6 +128,7 @@ func (h *highestSeenRVTracker) OnAdd(raw any, _ bool) { func (h *highestSeenRVTracker) OnUpdate(_, newObj any) { obj, ok := newObj.(client.Object) if !ok { + // Never expected, should we log an error? return } go func() { h.update(obj.GetResourceVersion()) }() @@ -133,6 +137,8 @@ func (h *highestSeenRVTracker) OnUpdate(_, newObj any) { func (h *highestSeenRVTracker) OnDelete(raw any) { obj, ok := raw.(client.Object) if !ok { + // Could be cache.DeletedFinalStateUnknown, will we + // get the latest RV through `OnUpdate` if that happens? return } go func() { h.update(obj.GetResourceVersion()) }() diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index 1dc06753f8..b7025611d2 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -9,30 +9,21 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - toolscache "k8s.io/client-go/tools/cache" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) -type informerGetter interface { - GetInformer(context.Context, schema.GroupVersionKind) (informer, error) +type cache interface { + SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key ObjectKey, rv int64) } -type informer interface { - AddEventHandler(handler toolscache.ResourceEventHandler) (toolscache.ResourceEventHandlerRegistration, error) - LastSyncResourceVersion() string -} - -func ConsistentClient(upstream Client, ig informerGetter) Client { +func ConsistentClient(upstream Client, cache cache) Client { } type consistentClient struct { - upstream Client - informerGetter informerGetter - scheme *runtime.Scheme - - trackers map[schema.GroupVersionKind]*highestSeenRVTracker - trackersLock sync.Mutex + upstream Client + cache cache + scheme *runtime.Scheme // lockedKeysByGVK maps gvk -> key -> keyLocker lockedKeysByGVK threadSafeMap[schema.GroupVersionKind, *threadSafeMap[types.NamespacedName, *keyLocker]] @@ -77,7 +68,9 @@ func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...Updat namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) - keyLock.lock() + if err := keyLock.lock(ctx); err != nil { + return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + } defer keyLock.unlock() if err := c.upstream.Update(ctx, obj, opts...); err != nil { @@ -89,10 +82,36 @@ func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...Updat if err != nil { return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) } + c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) + + return nil +} + +func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error { + gvk, err := apiutil.GVKForObject(obj, c.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + } + + namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + + keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) + if err := keyLock.lock(ctx); err != nil { + return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + } + defer keyLock.unlock() + + if err := c.upstream.Patch(ctx, obj, patch, opts...); err != nil { + return err + } + + rvRaw := obj.GetResourceVersion() + rv, err := strconv.ParseInt(rvRaw, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + } + c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) - go func() { - _ = c.blockFor(ctx, gvk, rv) - }() return nil } @@ -101,158 +120,66 @@ type lockedKeys struct { lockedKeys map[types.NamespacedName]*keyLocker } +// keyLocker implements a mutex with context support +// that also allows to wait for the current lock to +// be released. +// TODO: find a better name type keyLocker struct { - // mutex must be held to access any other field + // mutex must be held to access done mutex sync.Mutex - - // holders counts the holders. If zero, done - // may be nil - holders int - done chan struct{} + // done is nil when no one is holding the lock + done chan struct{} } -func (l *keyLocker) lock() { - l.mutex.Lock() - defer l.mutex.Unlock() +func (l *keyLocker) lock(ctx context.Context) error { + for { + l.mutex.Lock() + if l.done == nil { + l.done = make(chan struct{}) + l.mutex.Unlock() + return nil + } - if l.holders == 0 { - l.done = make(chan struct{}) + done := l.done + l.mutex.Unlock() + select { + case <-done: // released, try acquire + case <-ctx.Done(): + return ctx.Err() + } } - l.holders++ } func (l *keyLocker) unlock() { l.mutex.Lock() defer l.mutex.Unlock() - l.holders-- - if l.holders == 0 { - close(l.done) + if l.done == nil { + panic("unlock of unlocked mutex") } + close(l.done) + l.done = nil } +// wait waits for the current lock holder if any to +// release the lock. func (l *keyLocker) wait(ctx context.Context) error { l.mutex.Lock() - if l.holders == 0 { - l.mutex.Unlock() - return nil - } done := l.done l.mutex.Unlock() - select { - case <-ctx.Done(): - return ctx.Err() - case <-done: + if done == nil { return nil } -} - -// blockForObject blocks until the context is done or the RV in obj is observed. It may end up -// creating an informer. -func (c *consistentClient) blockFor(ctx context.Context, gvk schema.GroupVersionKind, rv int64) error { - tracker, err := c.trackerFor(ctx, gvk) - if err != nil { - return fmt.Errorf("failed to set up tracker for %v: %v", gvk, err) - } select { - case <-tracker.blockUntil(ctx, rv): + case <-done: return nil case <-ctx.Done(): return ctx.Err() } } -func (c *consistentClient) trackerFor(ctx context.Context, obj schema.GroupVersionKind) (*highestSeenRVTracker, error) { - c.trackersLock.Lock() - defer c.trackersLock.Unlock() - - tracker, ok := c.trackers[obj] - if ok { - return tracker, nil - } - - informer, err := c.informerGetter.GetInformer(ctx, obj) - if err != nil { - return nil, fmt.Errorf("Failed to get informer for %v: %v", obj, err) - } - - stringRV := informer.LastSyncResourceVersion() - rv, err := strconv.ParseInt(stringRV, 10, 64) - if err != nil { - return nil, fmt.Errorf("Failed to parse resource version %s: %v", stringRV, err) - } - - c.trackers[obj] = &highestSeenRVTracker{ - rv: rv, - cond: &sync.Cond{}, - } - - return c.trackers[obj], nil -} - -type highestSeenRVTracker struct { - rv int64 - cond *sync.Cond -} - -func (h *highestSeenRVTracker) blockUntil(ctx context.Context, rv int64) chan struct{} { - c := make(chan struct{}) - go func() { - h.cond.L.Lock() - for h.rv < rv { - h.cond.Wait() - } - h.cond.L.Unlock() - close(c) - }() - - return c -} - -func (h *highestSeenRVTracker) update(rv string) { - parsed, err := strconv.ParseInt(rv, 10, 64) - if err != nil { - fmt.Printf("Failed to parse resource version %s: %v\n", rv, err) - return - } - - h.cond.L.Lock() - defer h.cond.L.Unlock() - - if parsed > h.rv { - h.rv = parsed - h.cond.Broadcast() - } -} - -func (h *highestSeenRVTracker) OnAdd(raw interface{}, isInInitialList bool) { - obj, ok := raw.(Object) - if !ok { - // Never expected, should we log an error? - return - } - go func() { h.update(obj.GetResourceVersion()) }() -} -func (h *highestSeenRVTracker) OnUpdate(oldObj, newObj interface{}) { - obj, ok := newObj.(Object) - if !ok { - // Never expected, should we log an error? - return - } - go func() { h.update(obj.GetResourceVersion()) }() -} -func (h *highestSeenRVTracker) OnDelete(raw interface{}) { - obj, ok := raw.(Object) - if !ok { - // Could be cache.DeletedFinalStateUnknown, will we - // get the latest RV through `OnUpdate` if that happens? - return - } - go func() { h.update(obj.GetResourceVersion()) }() -} - type threadSafeMap[k comparable, v any] struct { lock sync.Mutex data map[k]v From 6ac0ae330d70947d4b753702debb000d247c45f7 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Fri, 6 Mar 2026 21:48:58 +0100 Subject: [PATCH 05/25] Create --- pkg/client/consistency.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index b7025611d2..0e91799339 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -59,6 +59,34 @@ func (c *consistentClient) List(ctx context.Context, list ObjectList, opts ...Li return c.upstream.List(ctx, list, opts...) } +func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...CreateOption) error { + gvk, err := apiutil.GVKForObject(obj, c.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + } + + namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + + keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) + if err := keyLock.lock(ctx); err != nil { + return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + } + defer keyLock.unlock() + + if err := c.upstream.Create(ctx, obj, opts...); err != nil { + return err + } + + rvRaw := obj.GetResourceVersion() + rv, err := strconv.ParseInt(rvRaw, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + } + c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) + + return nil +} + func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error { gvk, err := apiutil.GVKForObject(obj, c.scheme) if err != nil { From cefa8af2fd0498a65519bd790b805f0ff62b4b03 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Fri, 6 Mar 2026 22:12:58 +0100 Subject: [PATCH 06/25] Implement basic delete --- pkg/client/client.go | 13 ++++++++++--- pkg/client/consistency.go | 32 ++++++++++++++++++++++++++++++- pkg/client/typed_client.go | 10 ++++++---- pkg/client/unstructured_client.go | 14 ++++++-------- 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index ad946daeaa..85c3ca0422 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -319,11 +320,17 @@ func (c *client) Update(ctx context.Context, obj Object, opts ...UpdateOption) e // Delete implements client.Client. func (c *client) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error { + _, err := c.delete(ctx, obj, opts...) + return err +} + +// delete issues a delete call and returns the response or an error. The response +// gets deserialized into an unstructured and is either a metav1.Status if the object +// is gone from storage or the object if it remains, for example because of finalizers. +func (c *client) delete(ctx context.Context, obj Object, opts ...DeleteOption) (*unstructured.Unstructured, error) { switch obj.(type) { - case runtime.Unstructured: + case runtime.Unstructured, *metav1.PartialObjectMetadata: return c.unstructuredClient.Delete(ctx, obj, opts...) - case *metav1.PartialObjectMetadata: - return c.metadataClient.Delete(ctx, obj, opts...) default: return c.typedClient.Delete(ctx, obj, opts...) } diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index 0e91799339..e2e6ba0672 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -21,7 +21,7 @@ func ConsistentClient(upstream Client, cache cache) Client { } type consistentClient struct { - upstream Client + upstream *client cache cache scheme *runtime.Scheme @@ -143,6 +143,36 @@ func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, o return nil } +func (c *consistentClient) Delete(ctx context.Context, obj Object, patch Patch, opts ...DeleteOption) error { + gvk, err := apiutil.GVKForObject(obj, c.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + } + + namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} + + keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) + if err := keyLock.lock(ctx); err != nil { + return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + } + defer keyLock.unlock() + + response, err := c.upstream.delete(ctx, obj, opts...) + if err != nil { + return err + } + + if rvRaw := response.GetResourceVersion(); rvRaw != "" { + rv, err := strconv.ParseInt(rvRaw, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + } + c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) + } + + return nil +} + type lockedKeys struct { lock sync.RWMutex lockedKeys map[types.NamespacedName]*keyLocker diff --git a/pkg/client/typed_client.go b/pkg/client/typed_client.go index 5a85941725..bbc767d175 100644 --- a/pkg/client/typed_client.go +++ b/pkg/client/typed_client.go @@ -20,6 +20,7 @@ import ( "context" "fmt" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/json" "k8s.io/client-go/util/apply" @@ -73,22 +74,23 @@ func (c *typedClient) Update(ctx context.Context, obj Object, opts ...UpdateOpti } // Delete implements client.Client. -func (c *typedClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error { +func (c *typedClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) (*unstructured.Unstructured, error) { o, err := c.resources.getObjMeta(obj) if err != nil { - return err + return nil, err } deleteOpts := DeleteOptions{} deleteOpts.ApplyOptions(opts) - return o.Delete(). + response := &unstructured.Unstructured{} + return response, o.Delete(). NamespaceIfScoped(o.namespace, o.isNamespaced()). Resource(o.resource()). Name(o.name). Body(deleteOpts.AsDeleteOptions()). Do(ctx). - Error() + Into(response) } // DeleteAllOf implements client.Client. diff --git a/pkg/client/unstructured_client.go b/pkg/client/unstructured_client.go index d2ea6d7a32..4f6da43a92 100644 --- a/pkg/client/unstructured_client.go +++ b/pkg/client/unstructured_client.go @@ -21,6 +21,7 @@ import ( "fmt" "strings" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/util/apply" ) @@ -93,26 +94,23 @@ func (uc *unstructuredClient) Update(ctx context.Context, obj Object, opts ...Up } // Delete implements client.Client. -func (uc *unstructuredClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error { - if _, ok := obj.(runtime.Unstructured); !ok { - return fmt.Errorf("unstructured client did not understand object: %T", obj) - } - +func (uc *unstructuredClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) (*unstructured.Unstructured, error) { o, err := uc.resources.getObjMeta(obj) if err != nil { - return err + return nil, err } deleteOpts := DeleteOptions{} deleteOpts.ApplyOptions(opts) - return o.Delete(). + response := &unstructured.Unstructured{} + return response, o.Delete(). NamespaceIfScoped(o.namespace, o.isNamespaced()). Resource(o.resource()). Name(o.name). Body(deleteOpts.AsDeleteOptions()). Do(ctx). - Error() + Into(response) } // DeleteAllOf implements client.Client. From 416a7b16afe3557f52f34376ee04841a657d3df1 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 7 Mar 2026 16:21:59 +0100 Subject: [PATCH 07/25] Basic cache implementation --- pkg/cache/cache.go | 19 +- pkg/cache/delegating_by_gvk_cache.go | 8 + pkg/cache/informer_cache.go | 144 +++++++----- pkg/cache/informertest/fake_cache.go | 5 + pkg/cache/internal/cache_reader.go | 18 +- pkg/cache/internal/informers.go | 15 +- .../readerconsistency/readerconsistency.go | 218 ++++++++++++++++++ pkg/cache/multi_namespace_cache.go | 10 + pkg/cache/rv_tracker.go | 145 ------------ pkg/client/consistency.go | 16 +- pkg/client/typed_client.go | 1 - pkg/client/unstructured_client.go | 1 - 12 files changed, 372 insertions(+), 228 deletions(-) create mode 100644 pkg/cache/internal/readerconsistency/readerconsistency.go delete mode 100644 pkg/cache/rv_tracker.go diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index c6083155b0..bfb73db305 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -75,12 +75,20 @@ type Cache interface { // on the given GVK, it blocks until the highest minimum RV across all // keys for that GVK has been observed. // - // The rv value is snapshotted at the time of the Get/List call, so - // calling SetMinimumRVForGVKAndKey again while a Get/List is already - // waiting does not affect the in-flight wait. - // // TODO: This shouldn't be part of the public interface SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) + + // AddRequiredDeleteForGVKKeyAndUID causes subsequent Get requests for the + // given GVK and key to block until the UID has been observed as deleted. + // For List requests on the given GVK, it blocks until all pending delete + // UIDs across all keys for that GVK have been observed. + // + // + // An informer for the given object must have been created before the delete + // added here was executed, otherwise this will cause a deadlock. + // + // TODO: This shouldn't be part of the public interface + AddRequiredDeleteForObject(obj client.Object) error } // Informers knows how to create or fetch informers for different @@ -544,8 +552,7 @@ func newCache(restConfig *rest.Config, opts Options) newCacheFunc { NewInformer: opts.NewInformer, }), readerFailOnMissingInformer: opts.ReaderFailOnMissingInformer, - minimums: newMinimumRVStore(), - trackers: make(map[schema.GroupVersionKind]*highestSeenRVTracker), + minimumRVs: newMinimumRVStore(), } } } diff --git a/pkg/cache/delegating_by_gvk_cache.go b/pkg/cache/delegating_by_gvk_cache.go index 0485c18ca7..a530af807d 100644 --- a/pkg/cache/delegating_by_gvk_cache.go +++ b/pkg/cache/delegating_by_gvk_cache.go @@ -73,6 +73,14 @@ func (dbt *delegatingByGVKCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersio dbt.cacheForGVK(gvk).SetMinimumRVForGVKAndKey(gvk, key, rv) } +func (dbt *delegatingByGVKCache) AddRequiredDeleteForObject(obj client.Object) error { + cache, err := dbt.cacheForObject(obj) + if err != nil { + return err + } + return cache.AddRequiredDeleteForObject(obj) +} + func (dbt *delegatingByGVKCache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, opts ...InformerGetOption) (Informer, error) { return dbt.cacheForGVK(gvk).GetInformerForKind(ctx, gvk, opts...) } diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index d29ebc608c..db6fa5e4e4 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -19,7 +19,6 @@ package cache import ( "context" "fmt" - "strconv" "strings" "sync" @@ -62,9 +61,11 @@ type informerCache struct { *internal.Informers readerFailOnMissingInformer bool - minimums *minimumRVStore - trackers map[schema.GroupVersionKind]*highestSeenRVTracker - trackersLock sync.Mutex + // minimumRVs stores the minimum RVs we must have seen before returning reads. Due to RV + // being just a version, we can store this before we have an informer. This is + // different from deletes, where we can only observe deletes once we do have an informer, + // so we only allow storing required deletes as part of the informer. + minimumRVs *minimumRVStore } // Get implements Reader. @@ -83,20 +84,7 @@ func (ic *informerCache) Get(ctx context.Context, key client.ObjectKey, out clie return &ErrCacheNotStarted{} } - if minRV, ok := ic.minimums.GetForKey(gvk, key); ok { - tracker, err := ic.trackerFor(ctx, gvk) - if err != nil { - return err - } - select { - case <-tracker.blockUntil(minRV): - case <-ctx.Done(): - return ctx.Err() - } - ic.minimums.Cleanup(gvk, minRV) - } - - return cache.Reader.Get(ctx, key, out, opts...) + return cache.Reader.Get(ctx, key, out, ic.minimumRVs.GetForKey(gvk, key), opts...) } // List implements Reader. @@ -115,20 +103,7 @@ func (ic *informerCache) List(ctx context.Context, out client.ObjectList, opts . return &ErrCacheNotStarted{} } - if minRV, ok := ic.minimums.GetMaxForGVK(*gvk); ok { - tracker, err := ic.trackerFor(ctx, *gvk) - if err != nil { - return err - } - select { - case <-tracker.blockUntil(minRV): - case <-ctx.Done(): - return ctx.Err() - } - ic.minimums.Cleanup(*gvk, minRV) - } - - return cache.Reader.List(ctx, out, opts...) + return cache.Reader.List(ctx, out, ic.minimumRVs.GetMaxForGVK(*gvk), opts...) } // objectTypeForListObject tries to find the runtime.Object and associated GVK @@ -211,37 +186,30 @@ func (ic *informerCache) getInformerForKind(ctx context.Context, gvk schema.Grou } func (ic *informerCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { - ic.minimums.Set(gvk, key, rv) + ic.minimumRVs.Set(gvk, key, rv) } -func (ic *informerCache) trackerFor(ctx context.Context, gvk schema.GroupVersionKind) (*highestSeenRVTracker, error) { - ic.trackersLock.Lock() - defer ic.trackersLock.Unlock() - - if tracker, ok := ic.trackers[gvk]; ok { - return tracker, nil - } - - informer, err := ic.GetInformerForKind(ctx, gvk, BlockUntilSynced(false)) +func (ic *informerCache) AddRequiredDeleteForObject(obj client.Object) error { + gvk, err := apiutil.GVKForObject(obj, ic.scheme) if err != nil { - return nil, fmt.Errorf("failed to get informer for %v: %w", gvk, err) + return err } - - var initialRV int64 - if lastSynced := informer.LastSyncResourceVersion(); lastSynced != "" { - initialRV, err = strconv.ParseInt(lastSynced, 10, 64) - if err != nil { - return nil, fmt.Errorf("failed to parse last synced resource version %q for %v: %w", lastSynced, gvk, err) - } + cache, started, ok := ic.Peek(gvk, obj) + if !ok { + return fmt.Errorf("informer for GVK %v not found in cache", gvk) } - - tracker := newHighestSeenRVTracker(initialRV) - if _, err := informer.AddEventHandler(tracker); err != nil { - return nil, fmt.Errorf("failed to add event handler for %v: %w", gvk, err) + if !started { + return &ErrCacheNotStarted{} + } + if !cache.Informer.HasSynced() { + return fmt.Errorf("informer for GVK %v is not synced", gvk) } - ic.trackers[gvk] = tracker - return tracker, nil + cache.Reader.ConsistencyHandler.AddPendingDelete( + client.ObjectKey{Namespace: obj.GetNamespace(), Name: obj.GetName()}, + obj.GetUID(), + ) + return nil } // RemoveInformer deactivates and removes the informer from the cache. @@ -312,3 +280,67 @@ func indexByField(informer Informer, field string, extractValue client.IndexerFu return informer.AddIndexers(cache.Indexers{internal.FieldIndexName(field): indexFunc}) } + +type minimumRVStore struct { + mu sync.Mutex + minimums map[schema.GroupVersionKind]map[client.ObjectKey]int64 +} + +func newMinimumRVStore() *minimumRVStore { + return &minimumRVStore{ + minimums: make(map[schema.GroupVersionKind]map[client.ObjectKey]int64), + } +} + +func (s *minimumRVStore) Set(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.minimums[gvk] == nil { + s.minimums[gvk] = make(map[client.ObjectKey]int64) + } + s.minimums[gvk][key] = rv +} + +func (s *minimumRVStore) GetForKey(gvk schema.GroupVersionKind, key client.ObjectKey) int64 { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok { + return 0 + } + return keys[key] +} + +func (s *minimumRVStore) GetMaxForGVK(gvk schema.GroupVersionKind) int64 { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok || len(keys) == 0 { + return 0 + } + var maxRV int64 + for _, rv := range keys { + if rv > maxRV { + maxRV = rv + } + } + return maxRV +} + +func (s *minimumRVStore) Cleanup(gvk schema.GroupVersionKind, currentRV int64) { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok { + return + } + for key, rv := range keys { + if rv <= currentRV { + delete(keys, key) + } + } +} diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index 2f933eaa64..e4fabb38af 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -135,6 +135,11 @@ func (c *FakeInformers) IndexField(ctx context.Context, obj client.Object, field func (c *FakeInformers) SetMinimumRVForGVKAndKey(_ schema.GroupVersionKind, _ client.ObjectKey, _ int64) { } +// AddRequiredDeleteForGVKKeyAndUID implements Cache. +func (c *FakeInformers) AddRequiredDeleteForObject(client.Object) error { + return nil +} + // Get implements Cache. func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { return nil diff --git a/pkg/cache/internal/cache_reader.go b/pkg/cache/internal/cache_reader.go index 624869f590..32c8f8cbb7 100644 --- a/pkg/cache/internal/cache_reader.go +++ b/pkg/cache/internal/cache_reader.go @@ -30,13 +30,11 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/cache" + "sigs.k8s.io/controller-runtime/pkg/cache/internal/readerconsistency" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/internal/field/selector" ) -// CacheReader is a client.Reader. -var _ client.Reader = &CacheReader{} - // CacheReader wraps a cache.Index to implement the client.Reader interface for a single type. type CacheReader struct { // indexer is the underlying indexer wrapped by this cache. @@ -52,10 +50,12 @@ type CacheReader struct { // Be very careful with this, when enabled you must DeepCopy any object before mutating it, // otherwise you will mutate the object in the cache. disableDeepCopy bool + + *readerconsistency.ConsistencyHandler } // Get checks the indexer for the object and writes a copy of it if found. -func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Object, opts ...client.GetOption) error { +func (c *CacheReader) Get(ctx context.Context, key client.ObjectKey, out client.Object, minRV int64, opts ...client.GetOption) error { getOpts := client.GetOptions{} getOpts.ApplyOptions(opts) @@ -64,6 +64,10 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Ob } storeKey := objectKeyToStoreKey(key) + if err := c.ConsistencyHandler.WaitForGet(ctx, key, minRV); err != nil { + return err + } + // Lookup the object from the indexer cache obj, exists, err := c.indexer.GetByKey(storeKey) if err != nil { @@ -109,7 +113,7 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Ob } // List lists items out of the indexer and writes them to out. -func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...client.ListOption) error { +func (c *CacheReader) List(ctx context.Context, out client.ObjectList, minRV int64, opts ...client.ListOption) error { var objs []any var err error @@ -120,6 +124,10 @@ func (c *CacheReader) List(_ context.Context, out client.ObjectList, opts ...cli return fmt.Errorf("continue list option is not supported by the cache") } + if err := c.ConsistencyHandler.WaitForList(ctx, minRV); err != nil { + return err + } + switch { case listOpts.FieldSelector != nil: requiresExact := selector.RequiresExactMatch(listOpts.FieldSelector) diff --git a/pkg/cache/internal/informers.go b/pkg/cache/internal/informers.go index 619e36abd3..fd7dab8690 100644 --- a/pkg/cache/internal/informers.go +++ b/pkg/cache/internal/informers.go @@ -38,6 +38,7 @@ import ( "k8s.io/client-go/metadata" "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" + "sigs.k8s.io/controller-runtime/pkg/cache/internal/readerconsistency" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/internal/syncs" @@ -412,14 +413,20 @@ func (ip *Informers) addInformerToMap(gvk schema.GroupVersionKind, obj runtime.O return nil, false, err } + consistencyHandler := readerconsistency.NewHandler() + if _, err := sharedIndexInformer.AddEventHandler(consistencyHandler); err != nil { + return nil, false, fmt.Errorf("failed to add readerconsistency handler: %w", err) + } + // Create the new entry and set it in the map. i := &Cache{ Informer: sharedIndexInformer, Reader: CacheReader{ - indexer: sharedIndexInformer.GetIndexer(), - groupVersionKind: gvk, - scopeName: mapping.Scope.Name(), - disableDeepCopy: ip.unsafeDisableDeepCopy, + indexer: sharedIndexInformer.GetIndexer(), + groupVersionKind: gvk, + scopeName: mapping.Scope.Name(), + disableDeepCopy: ip.unsafeDisableDeepCopy, + ConsistencyHandler: consistencyHandler, }, stop: make(chan struct{}), } diff --git a/pkg/cache/internal/readerconsistency/readerconsistency.go b/pkg/cache/internal/readerconsistency/readerconsistency.go new file mode 100644 index 0000000000..ef54a6b803 --- /dev/null +++ b/pkg/cache/internal/readerconsistency/readerconsistency.go @@ -0,0 +1,218 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package readerconsistency + +import ( + "context" + "fmt" + "maps" + "strconv" + "sync" + + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/tools/cache" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func NewHandler() *ConsistencyHandler { + return &ConsistencyHandler{ + rvCond: sync.NewCond(&sync.Mutex{}), + pendingDeletesCond: sync.NewCond(&sync.Mutex{}), + pendingDeletes: make(map[client.ObjectKey]sets.Set[types.UID]), + } +} + +type ConsistencyHandler struct { + rvCond *sync.Cond + observedRV int64 + + pendingDeletesCond *sync.Cond + // pendingDeletes holds pending deletes. Must only be acquired when holding pendingDeletesCond.L + pendingDeletes map[client.ObjectKey]sets.Set[types.UID] +} + +func (h *ConsistencyHandler) AddPendingDelete(key client.ObjectKey, uid types.UID) { + h.pendingDeletesCond.L.Lock() + defer h.pendingDeletesCond.L.Unlock() + + if h.pendingDeletes[key] == nil { + h.pendingDeletes[key] = sets.New(uid) + return + } + h.pendingDeletes[key].Insert(uid) +} + +func (h *ConsistencyHandler) WaitForList(ctx context.Context, minRV int64) error { + if err := h.waitForRV(ctx, minRV); err != nil { + return err + } + + return h.waitAllDeletes(ctx) +} + +func (h *ConsistencyHandler) WaitForGet(ctx context.Context, key client.ObjectKey, minRV int64) error { + if err := h.waitForRV(ctx, minRV); err != nil { + return err + } + + return h.waitDeletesForKey(ctx, key) +} + +// waitDeletesForKey blocks until all pending deletes at the time of calling it were observed or context times out +func (h *ConsistencyHandler) waitDeletesForKey(ctx context.Context, key client.ObjectKey) error { + h.pendingDeletesCond.L.Lock() + pendingDeletes := maps.Clone(h.pendingDeletes[key]) + h.pendingDeletesCond.L.Unlock() + + return h.waitDeletes(ctx, pendingDeletes) +} + +// waitDeletesForGVK blocks until all pending deletes at the time of calling it were observed or context times out +func (h *ConsistencyHandler) waitAllDeletes(ctx context.Context) error { + h.pendingDeletesCond.L.Lock() + pendingDeletes := sets.Set[types.UID]{} + for _, uids := range h.pendingDeletes { + maps.Copy(pendingDeletes, uids) + } + h.pendingDeletesCond.L.Unlock() + + return h.waitDeletes(ctx, pendingDeletes) +} + +func (h *ConsistencyHandler) waitDeletes(ctx context.Context, uids sets.Set[types.UID]) error { + if len(uids) == 0 { + return nil + } + + allDeleted := make(chan struct{}) + go func() { + h.pendingDeletesCond.L.Lock() + for !h.allDeletedLocked(uids) { + if ctx.Err() != nil { + break + } + h.pendingDeletesCond.Wait() + } + h.pendingDeletesCond.L.Unlock() + close(allDeleted) + }() + + select { + case <-ctx.Done(): + return ctx.Err() + case <-allDeleted: + return nil + } +} + +func (h *ConsistencyHandler) allDeletedLocked(uids sets.Set[types.UID]) bool { + for wantDeleted := range uids { + for _, notDeletedUIDs := range h.pendingDeletes { + if notDeletedUIDs.Has(wantDeleted) { + return false + } + } + } + + return true +} + +func (h *ConsistencyHandler) observeDeletion(obj client.Object) { + key := client.ObjectKey{Namespace: obj.GetNamespace(), Name: obj.GetName()} + h.pendingDeletesCond.L.Lock() + defer h.pendingDeletesCond.L.Unlock() + + if h.pendingDeletes[key].Has(obj.GetUID()) { + h.pendingDeletes[key].Delete(obj.GetUID()) + h.pendingDeletesCond.Broadcast() + } + if len(h.pendingDeletes[key]) == 0 { + delete(h.pendingDeletes, key) + } +} + +func (h *ConsistencyHandler) waitForRV(ctx context.Context, rv int64) error { + observed := make(chan struct{}) + go func() { + h.rvCond.L.Lock() + for h.observedRV <= rv { + if ctx.Err() != nil { + break + } + h.rvCond.Wait() + } + h.rvCond.L.Unlock() + close(observed) + }() + + select { + case <-ctx.Done(): + return ctx.Err() + case <-observed: + return nil + } +} + +func (h *ConsistencyHandler) observeResourceVersion(rv string) { + parsed, err := strconv.ParseInt(rv, 10, 64) + if err != nil { + fmt.Printf("Failed to parse resource version %s: %v\n", rv, err) + return + } + + h.rvCond.L.Lock() + defer h.rvCond.L.Unlock() + + if parsed > h.observedRV { + h.observedRV = parsed + h.rvCond.Broadcast() + } +} + +func (h *ConsistencyHandler) OnAdd(raw any, _ bool) { + obj, ok := raw.(client.Object) + if !ok { + // TODO: Should never happen, log an error? + return + } + go func() { h.observeResourceVersion(obj.GetResourceVersion()) }() +} + +func (h *ConsistencyHandler) OnUpdate(_, newObj any) { + obj, ok := newObj.(client.Object) + if !ok { + // TODO: Should never happen, log an error? + return + } + go func() { h.observeResourceVersion(obj.GetResourceVersion()) }() +} + +func (h *ConsistencyHandler) OnDelete(raw any) { + var obj client.Object + switch t := raw.(type) { + case client.Object: + obj = t + case cache.DeletedFinalStateUnknown: + obj = t.Obj.(client.Object) + default: + // TODO: Should never happen, log an error? + return + } + go func() { h.observeResourceVersion(obj.GetResourceVersion()) }() + go func() { h.observeDeletion(obj) }() +} diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go index 807b0e9f84..12a152c0b8 100644 --- a/pkg/cache/multi_namespace_cache.go +++ b/pkg/cache/multi_namespace_cache.go @@ -236,6 +236,16 @@ func (c *multiNamespaceCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKi } } +func (c *multiNamespaceCache) AddRequiredDeleteForObject(obj client.Object) error { + if ns := obj.GetNamespace(); ns == "" && c.clusterCache != nil { + return c.clusterCache.AddRequiredDeleteForObject(obj) + } else if cache, ok := c.namespaceToCache[ns]; ok { + cache.AddRequiredDeleteForObject(obj) + } + + return nil +} + func (c *multiNamespaceCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { isNamespaced, err := apiutil.IsObjectNamespaced(obj, c.Scheme, c.RESTMapper) if err != nil { diff --git a/pkg/cache/rv_tracker.go b/pkg/cache/rv_tracker.go deleted file mode 100644 index 1b4031edd7..0000000000 --- a/pkg/cache/rv_tracker.go +++ /dev/null @@ -1,145 +0,0 @@ -package cache - -import ( - "fmt" - "strconv" - "sync" - - "k8s.io/apimachinery/pkg/runtime/schema" - - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type minimumRVStore struct { - mu sync.Mutex - minimums map[schema.GroupVersionKind]map[client.ObjectKey]int64 -} - -func newMinimumRVStore() *minimumRVStore { - return &minimumRVStore{ - minimums: make(map[schema.GroupVersionKind]map[client.ObjectKey]int64), - } -} - -func (s *minimumRVStore) Set(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { - s.mu.Lock() - defer s.mu.Unlock() - - if s.minimums[gvk] == nil { - s.minimums[gvk] = make(map[client.ObjectKey]int64) - } - s.minimums[gvk][key] = rv -} - -func (s *minimumRVStore) GetForKey(gvk schema.GroupVersionKind, key client.ObjectKey) (int64, bool) { - s.mu.Lock() - defer s.mu.Unlock() - - keys, ok := s.minimums[gvk] - if !ok { - return 0, false - } - rv, ok := keys[key] - return rv, ok -} - -func (s *minimumRVStore) GetMaxForGVK(gvk schema.GroupVersionKind) (int64, bool) { - s.mu.Lock() - defer s.mu.Unlock() - - keys, ok := s.minimums[gvk] - if !ok || len(keys) == 0 { - return 0, false - } - var maxRV int64 - for _, rv := range keys { - if rv > maxRV { - maxRV = rv - } - } - return maxRV, true -} - -func (s *minimumRVStore) Cleanup(gvk schema.GroupVersionKind, currentRV int64) { - s.mu.Lock() - defer s.mu.Unlock() - - keys, ok := s.minimums[gvk] - if !ok { - return - } - for key, rv := range keys { - if rv <= currentRV { - delete(keys, key) - } - } -} - -type highestSeenRVTracker struct { - rv int64 - cond *sync.Cond -} - -func newHighestSeenRVTracker(initialRV int64) *highestSeenRVTracker { - return &highestSeenRVTracker{ - rv: initialRV, - cond: sync.NewCond(&sync.Mutex{}), - } -} - -func (h *highestSeenRVTracker) blockUntil(rv int64) <-chan struct{} { - ch := make(chan struct{}) - go func() { - h.cond.L.Lock() - for h.rv < rv { - h.cond.Wait() - } - h.cond.L.Unlock() - close(ch) - }() - return ch -} - -func (h *highestSeenRVTracker) update(rv string) { - parsed, err := strconv.ParseInt(rv, 10, 64) - if err != nil { - fmt.Printf("Failed to parse resource version %s: %v\n", rv, err) - return - } - - h.cond.L.Lock() - defer h.cond.L.Unlock() - - if parsed > h.rv { - h.rv = parsed - h.cond.Broadcast() - } -} - -func (h *highestSeenRVTracker) OnAdd(raw any, _ bool) { - obj, ok := raw.(client.Object) - if !ok { - // Never expected, should we log an error? - return - } - go func() { h.update(obj.GetResourceVersion()) }() -} - -func (h *highestSeenRVTracker) OnUpdate(_, newObj any) { - obj, ok := newObj.(client.Object) - if !ok { - // Never expected, should we log an error? - return - } - go func() { h.update(obj.GetResourceVersion()) }() -} - -func (h *highestSeenRVTracker) OnDelete(raw any) { - obj, ok := raw.(client.Object) - if !ok { - // Could be cache.DeletedFinalStateUnknown, will we - // get the latest RV through `OnUpdate` if that happens? - return - } - go func() { h.update(obj.GetResourceVersion()) }() -} diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index e2e6ba0672..87453d41af 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -14,10 +14,7 @@ import ( type cache interface { SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key ObjectKey, rv int64) -} - -func ConsistentClient(upstream Client, cache cache) Client { - + AddRequiredDeleteForObject(Object) error } type consistentClient struct { @@ -143,7 +140,7 @@ func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, o return nil } -func (c *consistentClient) Delete(ctx context.Context, obj Object, patch Patch, opts ...DeleteOption) error { +func (c *consistentClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error { gvk, err := apiutil.GVKForObject(obj, c.scheme) if err != nil { return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) @@ -168,16 +165,15 @@ func (c *consistentClient) Delete(ctx context.Context, obj Object, patch Patch, return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) + } else { + if err := c.cache.AddRequiredDeleteForObject(obj); err != nil { + return fmt.Errorf("failed to add required delete for object: %v", err) + } } return nil } -type lockedKeys struct { - lock sync.RWMutex - lockedKeys map[types.NamespacedName]*keyLocker -} - // keyLocker implements a mutex with context support // that also allows to wait for the current lock to // be released. diff --git a/pkg/client/typed_client.go b/pkg/client/typed_client.go index bbc767d175..99c069995f 100644 --- a/pkg/client/typed_client.go +++ b/pkg/client/typed_client.go @@ -27,7 +27,6 @@ import ( ) var _ Reader = &typedClient{} -var _ Writer = &typedClient{} type typedClient struct { resources *clientRestResources diff --git a/pkg/client/unstructured_client.go b/pkg/client/unstructured_client.go index 4f6da43a92..31e935e557 100644 --- a/pkg/client/unstructured_client.go +++ b/pkg/client/unstructured_client.go @@ -27,7 +27,6 @@ import ( ) var _ Reader = &unstructuredClient{} -var _ Writer = &unstructuredClient{} type unstructuredClient struct { resources *clientRestResources From c390e7ec1b2e701218ad5ba80f282c019b72ce5e Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 7 Mar 2026 16:35:27 +0100 Subject: [PATCH 08/25] Thread minimum RV through --- pkg/cache/cache.go | 1 - pkg/cache/informer_cache.go | 77 +------------------ pkg/cache/internal/informers.go | 76 +++++++++++++++++- .../readerconsistency/readerconsistency.go | 7 +- 4 files changed, 84 insertions(+), 77 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index bfb73db305..bd013d17c5 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -552,7 +552,6 @@ func newCache(restConfig *rest.Config, opts Options) newCacheFunc { NewInformer: opts.NewInformer, }), readerFailOnMissingInformer: opts.ReaderFailOnMissingInformer, - minimumRVs: newMinimumRVStore(), } } } diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index db6fa5e4e4..ed9d804340 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "strings" - "sync" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -60,12 +59,6 @@ type informerCache struct { scheme *runtime.Scheme *internal.Informers readerFailOnMissingInformer bool - - // minimumRVs stores the minimum RVs we must have seen before returning reads. Due to RV - // being just a version, we can store this before we have an informer. This is - // different from deletes, where we can only observe deletes once we do have an informer, - // so we only allow storing required deletes as part of the informer. - minimumRVs *minimumRVStore } // Get implements Reader. @@ -84,7 +77,7 @@ func (ic *informerCache) Get(ctx context.Context, key client.ObjectKey, out clie return &ErrCacheNotStarted{} } - return cache.Reader.Get(ctx, key, out, ic.minimumRVs.GetForKey(gvk, key), opts...) + return cache.Reader.Get(ctx, key, out, ic.Informers.MinimumRVs.GetForKey(gvk, key), opts...) } // List implements Reader. @@ -103,7 +96,7 @@ func (ic *informerCache) List(ctx context.Context, out client.ObjectList, opts . return &ErrCacheNotStarted{} } - return cache.Reader.List(ctx, out, ic.minimumRVs.GetMaxForGVK(*gvk), opts...) + return cache.Reader.List(ctx, out, ic.Informers.MinimumRVs.GetMaxForGVK(*gvk), opts...) } // objectTypeForListObject tries to find the runtime.Object and associated GVK @@ -186,7 +179,7 @@ func (ic *informerCache) getInformerForKind(ctx context.Context, gvk schema.Grou } func (ic *informerCache) SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { - ic.minimumRVs.Set(gvk, key, rv) + ic.MinimumRVs.Set(gvk, key, rv) } func (ic *informerCache) AddRequiredDeleteForObject(obj client.Object) error { @@ -280,67 +273,3 @@ func indexByField(informer Informer, field string, extractValue client.IndexerFu return informer.AddIndexers(cache.Indexers{internal.FieldIndexName(field): indexFunc}) } - -type minimumRVStore struct { - mu sync.Mutex - minimums map[schema.GroupVersionKind]map[client.ObjectKey]int64 -} - -func newMinimumRVStore() *minimumRVStore { - return &minimumRVStore{ - minimums: make(map[schema.GroupVersionKind]map[client.ObjectKey]int64), - } -} - -func (s *minimumRVStore) Set(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { - s.mu.Lock() - defer s.mu.Unlock() - - if s.minimums[gvk] == nil { - s.minimums[gvk] = make(map[client.ObjectKey]int64) - } - s.minimums[gvk][key] = rv -} - -func (s *minimumRVStore) GetForKey(gvk schema.GroupVersionKind, key client.ObjectKey) int64 { - s.mu.Lock() - defer s.mu.Unlock() - - keys, ok := s.minimums[gvk] - if !ok { - return 0 - } - return keys[key] -} - -func (s *minimumRVStore) GetMaxForGVK(gvk schema.GroupVersionKind) int64 { - s.mu.Lock() - defer s.mu.Unlock() - - keys, ok := s.minimums[gvk] - if !ok || len(keys) == 0 { - return 0 - } - var maxRV int64 - for _, rv := range keys { - if rv > maxRV { - maxRV = rv - } - } - return maxRV -} - -func (s *minimumRVStore) Cleanup(gvk schema.GroupVersionKind, currentRV int64) { - s.mu.Lock() - defer s.mu.Unlock() - - keys, ok := s.minimums[gvk] - if !ok { - return - } - for key, rv := range keys { - if rv <= currentRV { - delete(keys, key) - } - } -} diff --git a/pkg/cache/internal/informers.go b/pkg/cache/internal/informers.go index fd7dab8690..7c34c5ec7e 100644 --- a/pkg/cache/internal/informers.go +++ b/pkg/cache/internal/informers.go @@ -39,6 +39,7 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/cache" "sigs.k8s.io/controller-runtime/pkg/cache/internal/readerconsistency" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" logf "sigs.k8s.io/controller-runtime/pkg/internal/log" "sigs.k8s.io/controller-runtime/pkg/internal/syncs" @@ -102,6 +103,7 @@ func NewInformers(config *rest.Config, options *InformersOpts) *Informers { enableWatchBookmarks: options.EnableWatchBookmarks, newInformer: newInformer, watchErrorHandler: options.WatchErrorHandler, + MinimumRVs: newMinimumRVStore(), } } @@ -206,6 +208,12 @@ type Informers struct { // watchErrorHandler to be set by overriding the options // or to use the default watchErrorHandler watchErrorHandler cache.WatchErrorHandlerWithContext + + // MinimumRVs stores the minimum RVs we must have seen before returning reads. Due to RV + // being just a version, we can store this before we have an informer. This is + // different from deletes, where we can only observe deletes once we do have an informer, + // so we only allow storing required deletes as part of the informer. + MinimumRVs *minimumRVStore } // Start calls Run on each of the informers and sets started to true. Blocks on the context. @@ -413,7 +421,9 @@ func (ip *Informers) addInformerToMap(gvk schema.GroupVersionKind, obj runtime.O return nil, false, err } - consistencyHandler := readerconsistency.NewHandler() + consistencyHandler := readerconsistency.NewHandler(func(currentRV int64) { + ip.MinimumRVs.Cleanup(gvk, currentRV) + }) if _, err := sharedIndexInformer.AddEventHandler(consistencyHandler); err != nil { return nil, false, fmt.Errorf("failed to add readerconsistency handler: %w", err) } @@ -636,3 +646,67 @@ func restrictNamespaceBySelector(namespaceOpt string, s Selector) string { } return "" } + +type minimumRVStore struct { + mu sync.Mutex + minimums map[schema.GroupVersionKind]map[client.ObjectKey]int64 +} + +func newMinimumRVStore() *minimumRVStore { + return &minimumRVStore{ + minimums: make(map[schema.GroupVersionKind]map[client.ObjectKey]int64), + } +} + +func (s *minimumRVStore) Set(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.minimums[gvk] == nil { + s.minimums[gvk] = make(map[client.ObjectKey]int64) + } + s.minimums[gvk][key] = rv +} + +func (s *minimumRVStore) GetForKey(gvk schema.GroupVersionKind, key client.ObjectKey) int64 { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok { + return 0 + } + return keys[key] +} + +func (s *minimumRVStore) GetMaxForGVK(gvk schema.GroupVersionKind) int64 { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok || len(keys) == 0 { + return 0 + } + var maxRV int64 + for _, rv := range keys { + if rv > maxRV { + maxRV = rv + } + } + return maxRV +} + +func (s *minimumRVStore) Cleanup(gvk schema.GroupVersionKind, currentRV int64) { + s.mu.Lock() + defer s.mu.Unlock() + + keys, ok := s.minimums[gvk] + if !ok { + return + } + for key, rv := range keys { + if rv <= currentRV { + delete(keys, key) + } + } +} diff --git a/pkg/cache/internal/readerconsistency/readerconsistency.go b/pkg/cache/internal/readerconsistency/readerconsistency.go index ef54a6b803..f431164c75 100644 --- a/pkg/cache/internal/readerconsistency/readerconsistency.go +++ b/pkg/cache/internal/readerconsistency/readerconsistency.go @@ -29,11 +29,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func NewHandler() *ConsistencyHandler { +func NewHandler(rvCleanup func(int64)) *ConsistencyHandler { return &ConsistencyHandler{ rvCond: sync.NewCond(&sync.Mutex{}), pendingDeletesCond: sync.NewCond(&sync.Mutex{}), pendingDeletes: make(map[client.ObjectKey]sets.Set[types.UID]), + rvCleanup: rvCleanup, } } @@ -44,6 +45,8 @@ type ConsistencyHandler struct { pendingDeletesCond *sync.Cond // pendingDeletes holds pending deletes. Must only be acquired when holding pendingDeletesCond.L pendingDeletes map[client.ObjectKey]sets.Set[types.UID] + + rvCleanup func(int64) } func (h *ConsistencyHandler) AddPendingDelete(key client.ObjectKey, uid types.UID) { @@ -182,6 +185,8 @@ func (h *ConsistencyHandler) observeResourceVersion(rv string) { h.observedRV = parsed h.rvCond.Broadcast() } + + go h.rvCleanup(parsed) } func (h *ConsistencyHandler) OnAdd(raw any, _ bool) { From 7049fdff779767f3b18c9f0fc8065208bd1c0cfd Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 7 Mar 2026 22:03:08 +0100 Subject: [PATCH 09/25] Make consistent client a client --- pkg/client/client.go | 20 ++++++- pkg/client/consistency.go | 119 +++++++++++++++++++++++++++++++++++--- pkg/client/watch.go | 4 +- 3 files changed, 132 insertions(+), 11 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 85c3ca0422..c2623709da 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/metadata" "k8s.io/client-go/rest" @@ -86,6 +87,8 @@ type CacheOptions struct { // read unstructured objects or lists from the cache. // If false, unstructured objects will always result in a live lookup. Unstructured bool + + ReadYourOwnWriteConsistencyEnabled bool } // NewClientFunc allows a user to define how to create a client. @@ -129,7 +132,7 @@ func New(config *rest.Config, options Options) (c Client, err error) { return c, err } -func newClient(config *rest.Config, options Options) (*client, error) { +func newClient(config *rest.Config, options Options) (Client, error) { if config == nil { return nil, fmt.Errorf("must provide non-nil rest.Config to client.New") } @@ -220,7 +223,20 @@ func newClient(config *rest.Config, options Options) (*client, error) { } c.uncachedGVKs[gvk] = struct{}{} } - return c, nil + + if !options.Cache.ReadYourOwnWriteConsistencyEnabled { + return c, nil + } + + informerCache, isCache := options.Cache.Reader.(cache) + if !isCache { + return nil, fmt.Errorf("cache reader does not implement %T, can not provide ReadYourOwnWriteConsistency", cache(nil)) + } + return &consistentClient{ + upstream: c, + cache: informerCache, + lockedKeysByGVK: threadSafeMap[schema.GroupVersionKind, *threadSafeMap[types.NamespacedName, *keyLocker]]{}, + }, nil } var _ Client = &client{} diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index 87453d41af..d2b49fe818 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -2,10 +2,13 @@ package client import ( "context" + "errors" "fmt" + "reflect" "strconv" "sync" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -17,17 +20,18 @@ type cache interface { AddRequiredDeleteForObject(Object) error } +var _ Client = (*consistentClient)(nil) + type consistentClient struct { upstream *client cache cache - scheme *runtime.Scheme // lockedKeysByGVK maps gvk -> key -> keyLocker lockedKeysByGVK threadSafeMap[schema.GroupVersionKind, *threadSafeMap[types.NamespacedName, *keyLocker]] } func (c *consistentClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error { - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { return fmt.Errorf("failed to get GVK for object %T: %v", obj, err) } @@ -41,7 +45,7 @@ func (c *consistentClient) Get(ctx context.Context, key ObjectKey, obj Object, o } func (c *consistentClient) List(ctx context.Context, list ObjectList, opts ...ListOption) error { - gvk, err := apiutil.GVKForObject(list, c.scheme) + gvk, err := apiutil.GVKForObject(list, c.upstream.Scheme()) if err != nil { return fmt.Errorf("failed to get GVK for list %T: %v", list, err) } @@ -57,7 +61,7 @@ func (c *consistentClient) List(ctx context.Context, list ObjectList, opts ...Li } func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...CreateOption) error { - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) } @@ -85,7 +89,7 @@ func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...Creat } func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error { - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) } @@ -113,7 +117,7 @@ func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...Updat } func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error { - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) } @@ -140,8 +144,81 @@ func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, o return nil } +func (c *consistentClient) Apply(ctx context.Context, obj runtime.ApplyConfiguration, opts ...ApplyOption) error { + var gvk schema.GroupVersionKind + var namespacedName types.NamespacedName + var getResourceVersion func() (string, error) + switch t := obj.(type) { + case *unstructuredApplyConfiguration: + gvk = t.Unstructured.GroupVersionKind() + namespacedName.Namespace = t.Unstructured.GetNamespace() + namespacedName.Name = t.Unstructured.GetName() + getResourceVersion = func() (string, error) { + return t.Unstructured.GetResourceVersion(), nil + } + case applyConfiguration: + gv, err := schema.ParseGroupVersion(*t.GetAPIVersion()) + if err != nil { + return fmt.Errorf("failed to parse group version %s: %v", *t.GetAPIVersion(), err) + } + gvk.Group = gv.Group + gvk.Version = gv.Version + gvk.Kind = *t.GetKind() + namespacedName.Namespace = *t.GetNamespace() + namespacedName.Name = *t.GetName() + getResourceVersion = func() (string, error) { + return resourceVersionFromApplyConfiguration(t) + } + default: + return fmt.Errorf("unsupported type for Apply: %T, must be either %T or %T", obj, &unstructuredApplyConfiguration{}, applyConfiguration(nil)) + } + + keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) + if err := keyLock.lock(ctx); err != nil { + return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + } + defer keyLock.unlock() + + if err := c.upstream.Apply(ctx, obj, opts...); err != nil { + return err + } + + rvRaw, err := getResourceVersion() + if err != nil { + return fmt.Errorf("failed to get resource version from apply configuration: %v", err) + } + rv, err := strconv.ParseInt(rvRaw, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + } + c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) + + return nil +} + +func resourceVersionFromApplyConfiguration(obj applyConfiguration) (string, error) { + v := reflect.ValueOf(obj) + for v.Kind() == reflect.Ptr { + v = v.Elem() + } + if v.Kind() != reflect.Struct { + return "", fmt.Errorf("expected struct, got %s", v.Kind()) + } + rv := v.FieldByName("ResourceVersion") + if !rv.IsValid() { + return "", fmt.Errorf("type %T has no ResourceVersion field", obj) + } + if rv.Kind() != reflect.Ptr || rv.Type().Elem().Kind() != reflect.String { + return "", fmt.Errorf("ResourceVersion field in %T is not *string", obj) + } + if rv.IsNil() { + return "", fmt.Errorf("ResourceVersion field in %T is nil", obj) + } + return rv.Elem().String(), nil +} + func (c *consistentClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error { - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) } @@ -174,6 +251,34 @@ func (c *consistentClient) Delete(ctx context.Context, obj Object, opts ...Delet return nil } +func (c *consistentClient) DeleteAllOf(ctx context.Context, obj Object, opts ...DeleteAllOfOption) error { + return errors.New("DeleteAllOf is not supported by consistentClient, please use List and Delete instead") +} + +func (c *consistentClient) Status() SubResourceWriter { + return c.SubResource("status") +} + +func (c *consistentClient) SubResource(subResource string) SubResourceClient { + panic("not implemented") +} + +func (c *consistentClient) Scheme() *runtime.Scheme { + return c.upstream.Scheme() +} + +func (c *consistentClient) RESTMapper() meta.RESTMapper { + return c.upstream.RESTMapper() +} + +func (c *consistentClient) GroupVersionKindFor(obj runtime.Object) (schema.GroupVersionKind, error) { + return c.upstream.GroupVersionKindFor(obj) +} + +func (c *consistentClient) IsObjectNamespaced(obj runtime.Object) (bool, error) { + return c.upstream.IsObjectNamespaced(obj) +} + // keyLocker implements a mutex with context support // that also allows to wait for the current lock to // be released. diff --git a/pkg/client/watch.go b/pkg/client/watch.go index 181b22a673..d9d26ab1be 100644 --- a/pkg/client/watch.go +++ b/pkg/client/watch.go @@ -28,11 +28,11 @@ import ( // NewWithWatch returns a new WithWatch. func NewWithWatch(config *rest.Config, options Options) (WithWatch, error) { - client, err := newClient(config, options) + c, err := newClient(config, options) if err != nil { return nil, err } - return &watchingClient{client: client}, nil + return &watchingClient{client: c.(*client)}, nil // TODO: This will panic if consistency is enabled } type watchingClient struct { From 0bddbd72b23825c15830eaf0af701d5f7a927c06 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 08:53:33 +0100 Subject: [PATCH 10/25] Some basic tests --- pkg/cache/cache.go | 5 + pkg/cache/delegating_by_gvk_cache.go | 11 + pkg/cache/informer_cache.go | 17 ++ pkg/cache/informertest/fake_cache.go | 5 + .../readerconsistency/readerconsistency.go | 15 +- pkg/cache/multi_namespace_cache.go | 10 + pkg/client/client.go | 2 +- pkg/client/consistency.go | 29 +- pkg/client/consistency_envtest_test.go | 271 ++++++++++++++++++ pkg/client/typed_client.go | 18 +- 10 files changed, 364 insertions(+), 19 deletions(-) create mode 100644 pkg/client/consistency_envtest_test.go diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index bd013d17c5..377519316d 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -89,6 +89,11 @@ type Cache interface { // // TODO: This shouldn't be part of the public interface AddRequiredDeleteForObject(obj client.Object) error + + // RemoveRequiredDeleteForObject removes a previously added pending delete. + // + // TODO: This shouldn't be part of the public interface + RemoveRequiredDeleteForObject(obj client.Object) error } // Informers knows how to create or fetch informers for different diff --git a/pkg/cache/delegating_by_gvk_cache.go b/pkg/cache/delegating_by_gvk_cache.go index a530af807d..da0e1a2aca 100644 --- a/pkg/cache/delegating_by_gvk_cache.go +++ b/pkg/cache/delegating_by_gvk_cache.go @@ -18,6 +18,7 @@ package cache import ( "context" + "fmt" "maps" "slices" "strings" @@ -81,6 +82,16 @@ func (dbt *delegatingByGVKCache) AddRequiredDeleteForObject(obj client.Object) e return cache.AddRequiredDeleteForObject(obj) } +func (dbt *delegatingByGVKCache) RemoveRequiredDeleteForObject(obj client.Object) error { + cache, err := dbt.cacheForObject(obj) + if err != nil { + return fmt.Errorf("getting cache for object: %w", err) + } + cache.RemoveRequiredDeleteForObject(obj) + + return nil +} + func (dbt *delegatingByGVKCache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, opts ...InformerGetOption) (Informer, error) { return dbt.cacheForGVK(gvk).GetInformerForKind(ctx, gvk, opts...) } diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index ed9d804340..28a1100e13 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -205,6 +205,23 @@ func (ic *informerCache) AddRequiredDeleteForObject(obj client.Object) error { return nil } +func (ic *informerCache) RemoveRequiredDeleteForObject(obj client.Object) error { + gvk, err := apiutil.GVKForObject(obj, ic.scheme) + if err != nil { + return fmt.Errorf("failed to get GVK for object: %w", err) + } + cache, _, ok := ic.Peek(gvk, obj) + if !ok { + return fmt.Errorf("informer for GVK %v not found in cache", gvk) + } + cache.Reader.ConsistencyHandler.RemovePendingDelete( + client.ObjectKey{Namespace: obj.GetNamespace(), Name: obj.GetName()}, + obj.GetUID(), + ) + + return nil +} + // RemoveInformer deactivates and removes the informer from the cache. func (ic *informerCache) RemoveInformer(_ context.Context, obj client.Object) error { gvk, err := apiutil.GVKForObject(obj, ic.scheme) diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index e4fabb38af..329dde1b2f 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -140,6 +140,11 @@ func (c *FakeInformers) AddRequiredDeleteForObject(client.Object) error { return nil } +// RemoveRequiredDeleteForObject implements Cache. +func (c *FakeInformers) RemoveRequiredDeleteForObject(client.Object) error { + return nil +} + // Get implements Cache. func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { return nil diff --git a/pkg/cache/internal/readerconsistency/readerconsistency.go b/pkg/cache/internal/readerconsistency/readerconsistency.go index f431164c75..4f85241b9c 100644 --- a/pkg/cache/internal/readerconsistency/readerconsistency.go +++ b/pkg/cache/internal/readerconsistency/readerconsistency.go @@ -60,6 +60,19 @@ func (h *ConsistencyHandler) AddPendingDelete(key client.ObjectKey, uid types.UI h.pendingDeletes[key].Insert(uid) } +func (h *ConsistencyHandler) RemovePendingDelete(key client.ObjectKey, uid types.UID) { + h.pendingDeletesCond.L.Lock() + defer h.pendingDeletesCond.L.Unlock() + + if h.pendingDeletes[key] != nil { + h.pendingDeletes[key].Delete(uid) + if len(h.pendingDeletes[key]) == 0 { + delete(h.pendingDeletes, key) + } + h.pendingDeletesCond.Broadcast() + } +} + func (h *ConsistencyHandler) WaitForList(ctx context.Context, minRV int64) error { if err := h.waitForRV(ctx, minRV); err != nil { return err @@ -153,7 +166,7 @@ func (h *ConsistencyHandler) waitForRV(ctx context.Context, rv int64) error { observed := make(chan struct{}) go func() { h.rvCond.L.Lock() - for h.observedRV <= rv { + for h.observedRV < rv { if ctx.Err() != nil { break } diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go index 12a152c0b8..1d41f9d281 100644 --- a/pkg/cache/multi_namespace_cache.go +++ b/pkg/cache/multi_namespace_cache.go @@ -246,6 +246,16 @@ func (c *multiNamespaceCache) AddRequiredDeleteForObject(obj client.Object) erro return nil } +func (c *multiNamespaceCache) RemoveRequiredDeleteForObject(obj client.Object) error { + if ns := obj.GetNamespace(); ns == "" && c.clusterCache != nil { + return c.clusterCache.RemoveRequiredDeleteForObject(obj) + } else if cache, ok := c.namespaceToCache[ns]; ok { + return cache.RemoveRequiredDeleteForObject(obj) + } + + return nil +} + func (c *multiNamespaceCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { isNamespaced, err := apiutil.IsObjectNamespaced(obj, c.Scheme, c.RESTMapper) if err != nil { diff --git a/pkg/client/client.go b/pkg/client/client.go index c2623709da..2e8ed9dfeb 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -345,7 +345,7 @@ func (c *client) Delete(ctx context.Context, obj Object, opts ...DeleteOption) e // is gone from storage or the object if it remains, for example because of finalizers. func (c *client) delete(ctx context.Context, obj Object, opts ...DeleteOption) (*unstructured.Unstructured, error) { switch obj.(type) { - case runtime.Unstructured, *metav1.PartialObjectMetadata: + case runtime.Unstructured: return c.unstructuredClient.Delete(ctx, obj, opts...) default: return c.typedClient.Delete(ctx, obj, opts...) diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index d2b49fe818..92bcf8a07b 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -18,6 +18,7 @@ import ( type cache interface { SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key ObjectKey, rv int64) AddRequiredDeleteForObject(Object) error + RemoveRequiredDeleteForObject(Object) error } var _ Client = (*consistentClient)(nil) @@ -231,21 +232,29 @@ func (c *consistentClient) Delete(ctx context.Context, obj Object, opts ...Delet } defer keyLock.unlock() + // Register the delete before we execute it, otherwise it may be in the cache + // before we register it, causing a deadlock. + if err := c.cache.AddRequiredDeleteForObject(obj); err != nil { + return fmt.Errorf("failed to add required delete for object: %v", err) + } + response, err := c.upstream.delete(ctx, obj, opts...) if err != nil { + if removeErr := c.cache.RemoveRequiredDeleteForObject(obj); removeErr != nil { + return errors.Join(err, fmt.Errorf("failed to remove required delete for object after delete error: %v", removeErr)) + } return err } if rvRaw := response.GetResourceVersion(); rvRaw != "" { + if err := c.cache.RemoveRequiredDeleteForObject(obj); err != nil { + return fmt.Errorf("failed to remove required delete for object after successful delete: %v", err) + } rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) - } else { - if err := c.cache.AddRequiredDeleteForObject(obj); err != nil { - return fmt.Errorf("failed to add required delete for object: %v", err) - } } return nil @@ -353,21 +362,13 @@ func (t *threadSafeMap[k, v]) getOrCreate(key k) v { if t.data == nil { t.data = make(map[k]v) } - t.data[key] = *new(v) - val = t.data[key] + val = reflect.New(reflect.TypeOf(val).Elem()).Interface().(v) + t.data[key] = val } return val } -func (t *threadSafeMap[k, v]) get(key k) (v, bool) { - t.lock.Lock() - defer t.lock.Unlock() - - val, ok := t.data[key] - return val, ok -} - func (t *threadSafeMap[k, v]) allValues() []v { t.lock.Lock() defer t.lock.Unlock() diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go new file mode 100644 index 0000000000..f3c393415b --- /dev/null +++ b/pkg/client/consistency_envtest_test.go @@ -0,0 +1,271 @@ +package client_test + +import ( + "context" + "fmt" + "sync/atomic" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kscheme "k8s.io/client-go/kubernetes/scheme" + + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("ConsistentClient", func() { + var ( + cl client.Client + ctx context.Context + cancel context.CancelFunc + counter uint64 + ) + + BeforeEach(func() { + ctx, cancel = context.WithCancel(context.Background()) + + c, err := cache.New(cfg, cache.Options{Scheme: kscheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + + go func() { + defer GinkgoRecover() + Expect(c.Start(ctx)).To(Succeed()) + }() + Expect(c.WaitForCacheSync(ctx)).To(BeTrue()) + + cl, err = client.New(cfg, client.Options{ + Scheme: kscheme.Scheme, + Cache: &client.CacheOptions{ + Reader: c, + ReadYourOwnWriteConsistencyEnabled: true, + }, + }) + Expect(err).NotTo(HaveOccurred()) + }) + + AfterEach(func() { + cancel() + }) + + newConfigMap := func(ns string) *corev1.ConfigMap { + n := atomic.AddUint64(&counter, 1) + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("consistency-test-%d", n), + Namespace: ns, + }, + Data: map[string]string{"key": "value"}, + } + } + + Describe("Create then Get", func() { + It("should immediately observe the created object", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + Expect(got.Name).To(Equal(cm.Name)) + Expect(got.Data).To(Equal(cm.Data)) + }) + }) + + Describe("Create then List", func() { + It("should immediately observe the created object in a list", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + list := &corev1.ConfigMapList{} + Expect(cl.List(ctx, list, client.InNamespace("default"))).To(Succeed()) + + var found bool + for _, item := range list.Items { + if item.Name == cm.Name { + found = true + Expect(item.Data).To(Equal(cm.Data)) + break + } + } + Expect(found).To(BeTrue(), "created ConfigMap should appear in list") + }) + }) + + Describe("Update then Get", func() { + It("should immediately observe the updated object", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + + got.Data["key"] = "updated" + Expect(cl.Update(ctx, got)).To(Succeed()) + + updated := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), updated)).To(Succeed()) + Expect(updated.Data["key"]).To(Equal("updated")) + }) + }) + + Describe("Update then List", func() { + It("should immediately observe the updated object in a list", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + + got.Data["key"] = "updated-for-list" + Expect(cl.Update(ctx, got)).To(Succeed()) + + list := &corev1.ConfigMapList{} + Expect(cl.List(ctx, list, client.InNamespace("default"))).To(Succeed()) + + var found bool + for _, item := range list.Items { + if item.Name == cm.Name { + found = true + Expect(item.Data["key"]).To(Equal("updated-for-list")) + break + } + } + Expect(found).To(BeTrue(), "updated ConfigMap should appear in list") + }) + }) + + Describe("Patch then Get", func() { + It("should immediately observe the patched object", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + + patch := client.MergeFrom(got.DeepCopy()) + got.Data["patched"] = "yes" + Expect(cl.Patch(ctx, got, patch)).To(Succeed()) + + patched := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), patched)).To(Succeed()) + Expect(patched.Data["patched"]).To(Equal("yes")) + }) + }) + + Describe("Delete then Get", func() { + It("should immediately observe the object as deleted", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + + Expect(cl.Delete(ctx, cm)).To(Succeed()) + + err := cl.Get(ctx, client.ObjectKeyFromObject(cm), &corev1.ConfigMap{}) + Expect(apierrors.IsNotFound(err)).To(BeTrue(), "expected NotFound after delete, got: %v", err) + }) + }) + + Describe("Delete then List", func() { + It("should immediately not include the deleted object in a list", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + + Expect(cl.Delete(ctx, cm)).To(Succeed()) + + list := &corev1.ConfigMapList{} + Expect(cl.List(ctx, list, client.InNamespace("default"))).To(Succeed()) + + for _, item := range list.Items { + Expect(item.Name).NotTo(Equal(cm.Name), "deleted ConfigMap should not appear in list") + } + }) + }) + + Describe("Multiple sequential writes then Get", func() { + It("should observe the final state after multiple updates", func() { + cm := newConfigMap("default") + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + for i := range 5 { + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + got.Data["key"] = fmt.Sprintf("iteration-%d", i) + Expect(cl.Update(ctx, got)).To(Succeed()) + } + + final := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), final)).To(Succeed()) + Expect(final.Data["key"]).To(Equal("iteration-4")) + }) + }) + + Describe("Create with namespace-scoped object", func() { + It("should work across different namespaces", func() { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("consistency-ns-%d", atomic.AddUint64(&counter, 1))}} + Expect(cl.Create(ctx, ns)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, ns))).To(Succeed()) + }) + + cm := newConfigMap(ns.Name) + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + }) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + Expect(got.Name).To(Equal(cm.Name)) + Expect(got.Namespace).To(Equal(ns.Name)) + }) + }) + + Describe("Delete object with finalizer then Get", func() { + It("should observe the updated object with deletion timestamp after delete", func() { + cm := newConfigMap("default") + cm.Finalizers = []string{"test.io/hold"} + Expect(cl.Create(ctx, cm)).To(Succeed()) + DeferCleanup(func(ctx context.Context) { + got := &corev1.ConfigMap{} + if err := cl.Get(ctx, client.ObjectKeyFromObject(cm), got); err == nil { + got.Finalizers = nil + Expect(cl.Update(ctx, got)).To(Succeed()) + } + }) + + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + + Expect(cl.Delete(ctx, cm)).To(Succeed()) + + afterDelete := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), afterDelete)).To(Succeed()) + Expect(afterDelete.DeletionTimestamp).NotTo(BeNil(), "should have a deletion timestamp") + }) + }) +}) diff --git a/pkg/client/typed_client.go b/pkg/client/typed_client.go index 99c069995f..0840ac6998 100644 --- a/pkg/client/typed_client.go +++ b/pkg/client/typed_client.go @@ -18,6 +18,7 @@ package client import ( "context" + "encoding/json" "fmt" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -82,14 +83,25 @@ func (c *typedClient) Delete(ctx context.Context, obj Object, opts ...DeleteOpti deleteOpts := DeleteOptions{} deleteOpts.ApplyOptions(opts) - response := &unstructured.Unstructured{} - return response, o.Delete(). + runtimeObj, err := o.Delete(). NamespaceIfScoped(o.namespace, o.isNamespaced()). Resource(o.resource()). Name(o.name). Body(deleteOpts.AsDeleteOptions()). Do(ctx). - Into(response) + Get() + if err != nil { + return nil, err + } + data, err := json.Marshal(runtimeObj) + if err != nil { + return nil, fmt.Errorf("failed to marshal delete response: %w", err) + } + response := &unstructured.Unstructured{} + if err := json.Unmarshal(data, &response.Object); err != nil { + return nil, fmt.Errorf("failed to unmarshal delete response: %w", err) + } + return response, nil } // DeleteAllOf implements client.Client. From c0f29c01f9995a2f266e05b019f8d6c252186638 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 09:01:16 +0100 Subject: [PATCH 11/25] Fix cleanup --- pkg/client/consistency_envtest_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index f3c393415b..0f0e259809 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -30,6 +30,10 @@ var _ = Describe("ConsistentClient", func() { c, err := cache.New(cfg, cache.Options{Scheme: kscheme.Scheme}) Expect(err).NotTo(HaveOccurred()) + // Set up a Namespace informer as tests will delete namespaces through the consistent client. + _, err = c.GetInformer(ctx, &corev1.Namespace{}) + Expect(err).NotTo(HaveOccurred()) + go func() { defer GinkgoRecover() Expect(c.Start(ctx)).To(Succeed()) From 4e8938e93c1fc5bf617b1d125640f9a4aae2173a Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 09:15:25 +0100 Subject: [PATCH 12/25] Make tests tabledriven --- pkg/client/consistency_envtest_test.go | 220 ++++++++++++------------- 1 file changed, 101 insertions(+), 119 deletions(-) diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index 0f0e259809..b90fca3788 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -10,6 +10,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1ac "k8s.io/client-go/applyconfigurations/core/v1" kscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/cache" @@ -30,9 +31,11 @@ var _ = Describe("ConsistentClient", func() { c, err := cache.New(cfg, cache.Options{Scheme: kscheme.Scheme}) Expect(err).NotTo(HaveOccurred()) - // Set up a Namespace informer as tests will delete namespaces through the consistent client. + // Set up informers for types used through the consistent client. _, err = c.GetInformer(ctx, &corev1.Namespace{}) Expect(err).NotTo(HaveOccurred()) + _, err = c.GetInformer(ctx, &corev1.ConfigMap{}) + Expect(err).NotTo(HaveOccurred()) go func() { defer GinkgoRecover() @@ -65,147 +68,126 @@ var _ = Describe("ConsistentClient", func() { } } - Describe("Create then Get", func() { - It("should immediately observe the created object", func() { - cm := newConfigMap("default") - Expect(cl.Create(ctx, cm)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { - Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) - }) - - got := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) - Expect(got.Name).To(Equal(cm.Name)) - Expect(got.Data).To(Equal(cm.Data)) - }) - }) - - Describe("Create then List", func() { - It("should immediately observe the created object in a list", func() { - cm := newConfigMap("default") - Expect(cl.Create(ctx, cm)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { - Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) - }) - - list := &corev1.ConfigMapList{} - Expect(cl.List(ctx, list, client.InNamespace("default"))).To(Succeed()) - - var found bool - for _, item := range list.Items { - if item.Name == cm.Name { - found = true - Expect(item.Data).To(Equal(cm.Data)) - break - } - } - Expect(found).To(BeTrue(), "created ConfigMap should appear in list") - }) - }) + type writeResult struct { + name string + deleted bool + data map[string]string + } - Describe("Update then Get", func() { - It("should immediately observe the updated object", func() { - cm := newConfigMap("default") - Expect(cl.Create(ctx, cm)).To(Succeed()) + DescribeTable("write then read", + func(ctx context.Context, write func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error)) { + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("consistency-wtr-%d", atomic.AddUint64(&counter, 1))}} + Expect(cl.Create(ctx, ns)).To(Succeed()) DeferCleanup(func(ctx context.Context) { - Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) + Expect(client.IgnoreNotFound(cl.Delete(ctx, ns))).To(Succeed()) }) - got := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) - - got.Data["key"] = "updated" - Expect(cl.Update(ctx, got)).To(Succeed()) - - updated := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), updated)).To(Succeed()) - Expect(updated.Data["key"]).To(Equal("updated")) - }) - }) - - Describe("Update then List", func() { - It("should immediately observe the updated object in a list", func() { - cm := newConfigMap("default") + cm := newConfigMap(ns.Name) Expect(cl.Create(ctx, cm)).To(Succeed()) DeferCleanup(func(ctx context.Context) { Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) }) - got := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + result, err := write(ctx, cl, cm) + Expect(err).NotTo(HaveOccurred()) - got.Data["key"] = "updated-for-list" - Expect(cl.Update(ctx, got)).To(Succeed()) + done := make(chan struct{}) - list := &corev1.ConfigMapList{} - Expect(cl.List(ctx, list, client.InNamespace("default"))).To(Succeed()) + go func() { + defer GinkgoRecover() + defer func() { done <- struct{}{} }() - var found bool - for _, item := range list.Items { - if item.Name == cm.Name { - found = true - Expect(item.Data["key"]).To(Equal("updated-for-list")) - break + if result.deleted { + err := cl.Get(ctx, client.ObjectKeyFromObject(cm), &corev1.ConfigMap{}) + Expect(apierrors.IsNotFound(err)).To(BeTrue(), "expected NotFound after delete, got: %v", err) + } else { + got := &corev1.ConfigMap{} + Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + Expect(got.Name).To(Equal(result.name)) + Expect(got.Data).To(Equal(result.data)) } - } - Expect(found).To(BeTrue(), "updated ConfigMap should appear in list") - }) - }) + }() - Describe("Patch then Get", func() { - It("should immediately observe the patched object", func() { - cm := newConfigMap("default") - Expect(cl.Create(ctx, cm)).To(Succeed()) - DeferCleanup(func(ctx context.Context) { - Expect(client.IgnoreNotFound(cl.Delete(ctx, cm))).To(Succeed()) - }) + go func() { + defer GinkgoRecover() + defer func() { done <- struct{}{} }() - got := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) + list := &corev1.ConfigMapList{} + Expect(cl.List(ctx, list, client.InNamespace(ns.Name))).To(Succeed()) - patch := client.MergeFrom(got.DeepCopy()) - got.Data["patched"] = "yes" - Expect(cl.Patch(ctx, got, patch)).To(Succeed()) + if result.deleted { + Expect(list.Items).To(BeEmpty(), "list should be empty after delete") + } else { + Expect(list.Items).To(HaveLen(1), "list should contain exactly one ConfigMap") + Expect(list.Items[0].Name).To(Equal(result.name)) + Expect(list.Items[0].Data).To(Equal(result.data)) + } + }() - patched := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), patched)).To(Succeed()) - Expect(patched.Data["patched"]).To(Equal("yes")) - }) - }) + <-done + <-done + }, - Describe("Delete then Get", func() { - It("should immediately observe the object as deleted", func() { - cm := newConfigMap("default") - Expect(cl.Create(ctx, cm)).To(Succeed()) + Entry("create", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { + return writeResult{ + name: cm.Name, + data: cm.Data, + }, nil // already created in the setup + }), + Entry("update", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { got := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) - - Expect(cl.Delete(ctx, cm)).To(Succeed()) - - err := cl.Get(ctx, client.ObjectKeyFromObject(cm), &corev1.ConfigMap{}) - Expect(apierrors.IsNotFound(err)).To(BeTrue(), "expected NotFound after delete, got: %v", err) - }) - }) - - Describe("Delete then List", func() { - It("should immediately not include the deleted object in a list", func() { - cm := newConfigMap("default") - Expect(cl.Create(ctx, cm)).To(Succeed()) + if err := cl.Get(ctx, client.ObjectKeyFromObject(cm), got); err != nil { + return writeResult{}, err + } + got.Data["key"] = "updated" + if err := cl.Update(ctx, got); err != nil { + return writeResult{}, err + } + return writeResult{ + name: cm.Name, + data: map[string]string{"key": "updated"}, + }, nil + }), + Entry("patch", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { got := &corev1.ConfigMap{} - Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), got)).To(Succeed()) - - Expect(cl.Delete(ctx, cm)).To(Succeed()) - - list := &corev1.ConfigMapList{} - Expect(cl.List(ctx, list, client.InNamespace("default"))).To(Succeed()) - - for _, item := range list.Items { - Expect(item.Name).NotTo(Equal(cm.Name), "deleted ConfigMap should not appear in list") + if err := cl.Get(ctx, client.ObjectKeyFromObject(cm), got); err != nil { + return writeResult{}, err } - }) - }) + patch := client.MergeFrom(got.DeepCopy()) + got.Data["patched"] = "yes" + if err := cl.Patch(ctx, got, patch); err != nil { + return writeResult{}, err + } + return writeResult{ + name: cm.Name, + data: map[string]string{"key": "value", "patched": "yes"}, + }, nil + }), + + Entry("apply", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { + ac := corev1ac.ConfigMap(cm.Name, cm.Namespace). + WithData(map[string]string{"key": "applied"}) + if err := cl.Apply(ctx, ac, client.FieldOwner("consistency-test"), client.ForceOwnership); err != nil { + return writeResult{}, err + } + return writeResult{ + name: cm.Name, + data: map[string]string{"key": "applied"}, + }, nil + }), + + Entry("delete", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { + if err := cl.Delete(ctx, cm); err != nil { + return writeResult{}, err + } + return writeResult{ + name: cm.Name, + deleted: true, + }, nil + }), + ) Describe("Multiple sequential writes then Get", func() { It("should observe the final state after multiple updates", func() { From 24ade5b356023d216c6b6778ca658b20a57d3c76 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 10:01:36 +0100 Subject: [PATCH 13/25] Fix apply --- pkg/client/applyconfigurations.go | 16 ---------------- pkg/client/consistency.go | 6 ++++++ pkg/client/consistency_envtest_test.go | 2 +- pkg/client/typed_client.go | 1 - 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/pkg/client/applyconfigurations.go b/pkg/client/applyconfigurations.go index 3a1864847d..b788ad7f9d 100644 --- a/pkg/client/applyconfigurations.go +++ b/pkg/client/applyconfigurations.go @@ -19,26 +19,10 @@ package client import ( "fmt" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" ) -type unstructuredApplyConfiguration struct { - *unstructured.Unstructured -} - -func (u *unstructuredApplyConfiguration) IsApplyConfiguration() {} - -// ApplyConfigurationFromUnstructured creates a runtime.ApplyConfiguration from an *unstructured.Unstructured object. -// -// Do not use Unstructured objects here that were generated from API objects, as its impossible to tell -// if a zero value was explicitly set. -func ApplyConfigurationFromUnstructured(u *unstructured.Unstructured) runtime.ApplyConfiguration { - return &unstructuredApplyConfiguration{Unstructured: u} -} - func gvkFromApplyConfiguration(ac applyConfiguration) (schema.GroupVersionKind, error) { var gvk schema.GroupVersionKind gv, err := schema.ParseGroupVersion(ptr.Deref(ac.GetAPIVersion(), "")) diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index 92bcf8a07b..93d640b2d2 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/json" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) @@ -198,6 +199,11 @@ func (c *consistentClient) Apply(ctx context.Context, obj runtime.ApplyConfigura } func resourceVersionFromApplyConfiguration(obj applyConfiguration) (string, error) { + s, err := json.Marshal(obj) + if err != nil { + panic(err) + } + println(string(s)) v := reflect.ValueOf(obj) for v.Kind() == reflect.Ptr { v = v.Elem() diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index b90fca3788..29eef96088 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -166,7 +166,7 @@ var _ = Describe("ConsistentClient", func() { }, nil }), - Entry("apply", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { + FEntry("apply", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { ac := corev1ac.ConfigMap(cm.Name, cm.Namespace). WithData(map[string]string{"key": "applied"}) if err := cl.Apply(ctx, ac, client.FieldOwner("consistency-test"), client.ForceOwnership); err != nil { diff --git a/pkg/client/typed_client.go b/pkg/client/typed_client.go index 0840ac6998..fad3769622 100644 --- a/pkg/client/typed_client.go +++ b/pkg/client/typed_client.go @@ -23,7 +23,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/json" "k8s.io/client-go/util/apply" ) From d6d1d75d1031830e0b9f46c1e2116811f6f7a620 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 10:41:56 +0100 Subject: [PATCH 14/25] We need unstructured applyconfiguration --- pkg/client/applyconfigurations.go | 16 ++++++++++++++++ pkg/client/consistency_envtest_test.go | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/client/applyconfigurations.go b/pkg/client/applyconfigurations.go index b788ad7f9d..3a1864847d 100644 --- a/pkg/client/applyconfigurations.go +++ b/pkg/client/applyconfigurations.go @@ -19,10 +19,26 @@ package client import ( "fmt" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" ) +type unstructuredApplyConfiguration struct { + *unstructured.Unstructured +} + +func (u *unstructuredApplyConfiguration) IsApplyConfiguration() {} + +// ApplyConfigurationFromUnstructured creates a runtime.ApplyConfiguration from an *unstructured.Unstructured object. +// +// Do not use Unstructured objects here that were generated from API objects, as its impossible to tell +// if a zero value was explicitly set. +func ApplyConfigurationFromUnstructured(u *unstructured.Unstructured) runtime.ApplyConfiguration { + return &unstructuredApplyConfiguration{Unstructured: u} +} + func gvkFromApplyConfiguration(ac applyConfiguration) (schema.GroupVersionKind, error) { var gvk schema.GroupVersionKind gv, err := schema.ParseGroupVersion(ptr.Deref(ac.GetAPIVersion(), "")) diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index 29eef96088..b90fca3788 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -166,7 +166,7 @@ var _ = Describe("ConsistentClient", func() { }, nil }), - FEntry("apply", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { + Entry("apply", func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error) { ac := corev1ac.ConfigMap(cm.Name, cm.Namespace). WithData(map[string]string{"key": "applied"}) if err := cl.Apply(ctx, ac, client.FieldOwner("consistency-test"), client.ForceOwnership); err != nil { From bc0343a0396b008745293d9d3e25704450abbced Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 12:35:59 +0100 Subject: [PATCH 15/25] Remove debuging remainders --- pkg/client/consistency.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index 93d640b2d2..e90edebcb3 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -12,7 +12,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - "k8s.io/apimachinery/pkg/util/json" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) @@ -199,13 +198,8 @@ func (c *consistentClient) Apply(ctx context.Context, obj runtime.ApplyConfigura } func resourceVersionFromApplyConfiguration(obj applyConfiguration) (string, error) { - s, err := json.Marshal(obj) - if err != nil { - panic(err) - } - println(string(s)) v := reflect.ValueOf(obj) - for v.Kind() == reflect.Ptr { + for v.Kind() == reflect.Pointer { v = v.Elem() } if v.Kind() != reflect.Struct { From b9f172b6bd114da7d6f2b9c54c6c8705f2041fe9 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 12:36:11 +0100 Subject: [PATCH 16/25] Update design --- designs/read_your_own_write_client.md | 106 +++++++++++++++++--------- 1 file changed, 70 insertions(+), 36 deletions(-) diff --git a/designs/read_your_own_write_client.md b/designs/read_your_own_write_client.md index f8a381a087..418664956d 100644 --- a/designs/read_your_own_write_client.md +++ b/designs/read_your_own_write_client.md @@ -1,12 +1,18 @@ Read your own write client ========================== +## Background + +Controller-Runtimes default client writes to the api and reads from an informercache. As a result, performing a read +right after a write tends to not return that write since it hasn't made it back to the informer cache yet. This +leads to bugs and workarounds like replacing the read portion of the client to read from the API instead. The goal of +this proposal is to provide the same consistency in the cache-reading client a live client would with regards to +writes performed through the same client. + ## Goals -* Any read from the client (`Get` or `List`) that starts after a write started will observe that write. The - reason we consider the start and not the end time of the write to be the cutoff where reads must observe - it is that we don't know when exactly the server applied it, as there is delay between the write being - applied and the writer getting to know its write got applied. +* Any read from the client (`Get` or `List`) contains all writes performed through the same client that were + committed by the server at the time the read happened * Reads do not get blocked on writes that started after they started, as otherwise they may end up getting blocked indefinitely if many writes are happening. * Writes themselves do not get blocked waiting for them to be observable by reads because: @@ -19,42 +25,70 @@ Read your own write client ## Non-Goals -* Any sort of consistency with writes originating from a different client - The only way to do this would be +* Any sort of consistency with writes originating from a client in another binary - The only way to do this would be to do a get or list against the api at which point it doesn't make sense to have a cache-backed client at all -* Client reads observe writes that succeeded, but where the information if the write succeeded didn't make it - back to the client, because for example the connection was interrupted. This is primarily for practical +* Consistency between multiple clients constructed using the same informercache +* Consistency for writes that succeeded, but where the information if the write succeeded didn't make it + back to the client for any reason like the connection breaking. This is primarily for practical purposes, if the write doesn't get a response indicating success or failure back, it is impossible to tell if the write did or did not succeed -* Dealing with configurations where the cache doesn't contain all objects that are written, for example due to - label or field selectors: This is essentially a misconfiguration and non-trivial to detect. We may or may not - look into detecting this in the future +* Automatically dealing with configurations where the cache doesn't contain all objects that are written, for + example because its label or field selector doesn't match them. We will provide an option for this case and + may or may not try to detect this correctly in the future +* Support `DeleteAllOf` - The apiservers response to DeleteAllOf is insufficient to implement this correctly. DeleteAllOf + will error and instruct the user to use `List` and `Delete` +* Fail writes that use optimistic locking clientside - This could be done in the future, but is initially out of scope ## Implementation -The implementation is gated behind a new and default-off client option `ConsistentReads bool`. If set, mutating -operations will: -* get sequentialized by gvk+key: This is purely to simplify implementation, we may or may not change this in the future -* block following get calls for the same gvk and object key and list calls for the gvk - -Once the operation returned, it will: -1. Update the cache through a new internal-only `SetMinimumRVForGVKAndKey` method, which instructs it to block - all subsequent Get calls for the given GVK and key and all list calls for the given gvk until the passed RV was observed -2. Unblock reads - -A special challenge are Delete calls. There, the response is either the object including new RV if the object was -not deleted from storage or a metav1.Status without RV if the object was deleted from storage. To deal with this, -we will start deserializing the delete response into an unstructured and test which of the two it is. If it is the -object, we follow the same RV-waiting approach as for the other operations. If it is a success status, we will: -1. Update the cache through a new internal-only `AddRequiredDeleteForGVKKeyAndUID` method, which will make the cache - append the UID to a per-gvk and key set. Whenever a delete event for a gvk and key is observed, the uid will be - removed from the set. The cache then blocks gets on the gvk+key set being empty and lists on the gvk set being empty -2. Unblock reads - -*Note:* The above only fulfills the goal `Reads do not get blocked on writes that started after they started` by assuming -that reads read the RV/deleted object set in the cache before any subsequent write calls `SetMinimumRVForGVKAndKey` or -`AddRequiredDeleteForGVKKeyAndUID`. The underlying information in the cache will be protected by a mutex and copied for -reads. While golang mutexes do not guarantee any acquisition order, writes entail a networking roundtrip, so we assume -that the read will always acquire the mutex before subsequent writes do. In the extremely unlikely case that they do not, -the result would only be a performance degradation (we wait for additional writes), not a correctness issue which is deemed -acceptable. +The basic idea of the implementation is to make writes block concurrent reads to the same GVK+Key for Get and GVK for List +before the request is sent. If the request succeeds, the client provide the cache with either the returned resourceVersion +or the gvk+objectkey+uid of an object if it was deleted from storage, then unblock the reads. The cache will then copy this +RV/deleted object within the get/list and block the request until it observed it or the requests context times out. +It is important that we block before executing the write request and not after, because we can not know when exactly the server +commits it, only when it tells us having committed it. + +The implementation is gated behind a `ReadYourOwnWrite *bool` `client.Options.Cache` setting. It will initially be disabled +by default, the goal is to enable it by default once we are confident in the implementation. + +### Changes to the Cache + +* Add an internal-only `SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64)` method. Once + called, all `Get` requests to the GVK+key and all `List` requests to the GVK will be blocked until the cache observed the + passed rv or the request times out. The rv is copied before waiting to avoid subsequent calls to `SetMinimumRVForGVKAndKey` + blocking the request +* Add an internal-only `AddRequiredDeleteForObject(obj client.Object) error` method. Once set, all `Get` requests to the GVK+key + of object and all `List` requests to the GVK of object will be blocked until a delete event for GVK+UID of object was + observed OR the requests context times our OR `RemoveRequiredDeleteForObject` is called for the same object. The object(s) + whose deletion are awaited are copied before waiting to avoid subsequent `AddRequiredDeleteForObject` calls to block existing + `Get`/`List` calls further. It will error if there is no existing started and synced informer for the passed object, as + otherwise we can not observe the Delete event which will end up blocking all subsequent reads +* Add an internal-only `RemoveRequiredDeleteForObject(obj client.Object) error` which makes it not block reads for the passed + objects GVK+UID anymore. This is required because it is possible that a delete event arrives in the cache before the Delete + call finishes. This forces us to call `AddRequiredDeleteForObject` before executing the Delete call and hence we need to + reverse that again if the call fails or the Object had a finalizer + +### Changes to the Client + +Add a new `readYourOwnWriteClient` wrapper to the client, which will wrap any new client that is constructed with +`options.Cache.ReadYourOwnWrite: new(true)`. This wrapper maintains a map of locks for gvk+key. It will wrap all +mutating operations and in their beginning acquire the lock for the requests object. If the request succeeds, it +will call the caches `SetMinimumRVForGVKAndKey` before returning. For `Delete`, it will additionally call +`AddRequiredDeleteForObject` before executing the call and if the call fails or the response contains an object, it +will then call `RemoveRequiredDeleteForObject`. +The wrapper also wraps all reading operations and block them until the current lock holder for the GVK+key in the +case of `Get` or all current lock holders for GVK in the case of `List` release their lock or the requests context +expires. + +Add a new `DisableReadYourOwnWriteConsistency` option that can be used for either `Get` or `List` and disables the +above mentioned checks. This allows to disable the functionality for objects that are not cached. + + +## Open questions + +How exactly do we implement internal-only methods? Options include: +1. Implement them on the type by do not add them to the published interface. This allows anyone to use these methods, + but they need to dig for that and will probably aware that that usage is not supported +2. Move the implementations under `./internal` with the full interface visible there and put a small shim at + the existing location that only provides the methods we want to be externally usable From 3fc71b9254b7ba8f70ac2e53e9e029b548db4abc Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 12:38:39 +0100 Subject: [PATCH 17/25] Remove design --- designs/read_your_own_write_client.md | 94 --------------------------- 1 file changed, 94 deletions(-) delete mode 100644 designs/read_your_own_write_client.md diff --git a/designs/read_your_own_write_client.md b/designs/read_your_own_write_client.md deleted file mode 100644 index 418664956d..0000000000 --- a/designs/read_your_own_write_client.md +++ /dev/null @@ -1,94 +0,0 @@ -Read your own write client -========================== - -## Background - -Controller-Runtimes default client writes to the api and reads from an informercache. As a result, performing a read -right after a write tends to not return that write since it hasn't made it back to the informer cache yet. This -leads to bugs and workarounds like replacing the read portion of the client to read from the API instead. The goal of -this proposal is to provide the same consistency in the cache-reading client a live client would with regards to -writes performed through the same client. - -## Goals - -* Any read from the client (`Get` or `List`) contains all writes performed through the same client that were - committed by the server at the time the read happened -* Reads do not get blocked on writes that started after they started, as otherwise they may end up getting - blocked indefinitely if many writes are happening. -* Writes themselves do not get blocked waiting for them to be observable by reads because: - * This may lead to successful writes returning an error. While that should not result in incorrect behavior - of a controller as controllers are expected to be idempotent, it would lead to performance degradation, as - the error will typical lead to a retry with backoff and any work done before encountering it has to be - re-done - * Waiting for the read to make it back into the cache may entail setting up an informer, which takes - significant time - -## Non-Goals - -* Any sort of consistency with writes originating from a client in another binary - The only way to do this would be - to do a get or list against the api at which point it doesn't make sense to have a cache-backed client at - all -* Consistency between multiple clients constructed using the same informercache -* Consistency for writes that succeeded, but where the information if the write succeeded didn't make it - back to the client for any reason like the connection breaking. This is primarily for practical - purposes, if the write doesn't get a response indicating success or failure back, it is impossible to tell if - the write did or did not succeed -* Automatically dealing with configurations where the cache doesn't contain all objects that are written, for - example because its label or field selector doesn't match them. We will provide an option for this case and - may or may not try to detect this correctly in the future -* Support `DeleteAllOf` - The apiservers response to DeleteAllOf is insufficient to implement this correctly. DeleteAllOf - will error and instruct the user to use `List` and `Delete` -* Fail writes that use optimistic locking clientside - This could be done in the future, but is initially out of scope - -## Implementation - -The basic idea of the implementation is to make writes block concurrent reads to the same GVK+Key for Get and GVK for List -before the request is sent. If the request succeeds, the client provide the cache with either the returned resourceVersion -or the gvk+objectkey+uid of an object if it was deleted from storage, then unblock the reads. The cache will then copy this -RV/deleted object within the get/list and block the request until it observed it or the requests context times out. -It is important that we block before executing the write request and not after, because we can not know when exactly the server -commits it, only when it tells us having committed it. - -The implementation is gated behind a `ReadYourOwnWrite *bool` `client.Options.Cache` setting. It will initially be disabled -by default, the goal is to enable it by default once we are confident in the implementation. - -### Changes to the Cache - -* Add an internal-only `SetMinimumRVForGVKAndKey(gvk schema.GroupVersionKind, key client.ObjectKey, rv int64)` method. Once - called, all `Get` requests to the GVK+key and all `List` requests to the GVK will be blocked until the cache observed the - passed rv or the request times out. The rv is copied before waiting to avoid subsequent calls to `SetMinimumRVForGVKAndKey` - blocking the request -* Add an internal-only `AddRequiredDeleteForObject(obj client.Object) error` method. Once set, all `Get` requests to the GVK+key - of object and all `List` requests to the GVK of object will be blocked until a delete event for GVK+UID of object was - observed OR the requests context times our OR `RemoveRequiredDeleteForObject` is called for the same object. The object(s) - whose deletion are awaited are copied before waiting to avoid subsequent `AddRequiredDeleteForObject` calls to block existing - `Get`/`List` calls further. It will error if there is no existing started and synced informer for the passed object, as - otherwise we can not observe the Delete event which will end up blocking all subsequent reads -* Add an internal-only `RemoveRequiredDeleteForObject(obj client.Object) error` which makes it not block reads for the passed - objects GVK+UID anymore. This is required because it is possible that a delete event arrives in the cache before the Delete - call finishes. This forces us to call `AddRequiredDeleteForObject` before executing the Delete call and hence we need to - reverse that again if the call fails or the Object had a finalizer - -### Changes to the Client - -Add a new `readYourOwnWriteClient` wrapper to the client, which will wrap any new client that is constructed with -`options.Cache.ReadYourOwnWrite: new(true)`. This wrapper maintains a map of locks for gvk+key. It will wrap all -mutating operations and in their beginning acquire the lock for the requests object. If the request succeeds, it -will call the caches `SetMinimumRVForGVKAndKey` before returning. For `Delete`, it will additionally call -`AddRequiredDeleteForObject` before executing the call and if the call fails or the response contains an object, it -will then call `RemoveRequiredDeleteForObject`. -The wrapper also wraps all reading operations and block them until the current lock holder for the GVK+key in the -case of `Get` or all current lock holders for GVK in the case of `List` release their lock or the requests context -expires. - -Add a new `DisableReadYourOwnWriteConsistency` option that can be used for either `Get` or `List` and disables the -above mentioned checks. This allows to disable the functionality for objects that are not cached. - - -## Open questions - -How exactly do we implement internal-only methods? Options include: -1. Implement them on the type by do not add them to the published interface. This allows anyone to use these methods, - but they need to dig for that and will probably aware that that usage is not supported -2. Move the implementations under `./internal` with the full interface visible there and put a small shim at - the existing location that only provides the methods we want to be externally usable From 690f37126875c83f9021a6d970ca2a88d355fbd1 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 8 Mar 2026 22:44:24 +0100 Subject: [PATCH 18/25] Fix lint issues --- pkg/cache/delegating_by_gvk_cache.go | 4 +-- pkg/cache/informertest/fake_cache.go | 2 +- pkg/cache/multi_namespace_cache.go | 2 +- pkg/client/consistency.go | 42 +++++++++++++------------- pkg/client/consistency_envtest_test.go | 4 +-- 5 files changed, 26 insertions(+), 28 deletions(-) diff --git a/pkg/cache/delegating_by_gvk_cache.go b/pkg/cache/delegating_by_gvk_cache.go index da0e1a2aca..67e912f2d9 100644 --- a/pkg/cache/delegating_by_gvk_cache.go +++ b/pkg/cache/delegating_by_gvk_cache.go @@ -87,9 +87,7 @@ func (dbt *delegatingByGVKCache) RemoveRequiredDeleteForObject(obj client.Object if err != nil { return fmt.Errorf("getting cache for object: %w", err) } - cache.RemoveRequiredDeleteForObject(obj) - - return nil + return cache.RemoveRequiredDeleteForObject(obj) } func (dbt *delegatingByGVKCache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, opts ...InformerGetOption) (Informer, error) { diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index 329dde1b2f..d760d31d75 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -135,7 +135,7 @@ func (c *FakeInformers) IndexField(ctx context.Context, obj client.Object, field func (c *FakeInformers) SetMinimumRVForGVKAndKey(_ schema.GroupVersionKind, _ client.ObjectKey, _ int64) { } -// AddRequiredDeleteForGVKKeyAndUID implements Cache. +// AddRequiredDeleteForObject implements Cache. func (c *FakeInformers) AddRequiredDeleteForObject(client.Object) error { return nil } diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go index 1d41f9d281..3cdc2a6f13 100644 --- a/pkg/cache/multi_namespace_cache.go +++ b/pkg/cache/multi_namespace_cache.go @@ -240,7 +240,7 @@ func (c *multiNamespaceCache) AddRequiredDeleteForObject(obj client.Object) erro if ns := obj.GetNamespace(); ns == "" && c.clusterCache != nil { return c.clusterCache.AddRequiredDeleteForObject(obj) } else if cache, ok := c.namespaceToCache[ns]; ok { - cache.AddRequiredDeleteForObject(obj) + return cache.AddRequiredDeleteForObject(obj) } return nil diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index e90edebcb3..e3c7a564a0 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -34,7 +34,7 @@ type consistentClient struct { func (c *consistentClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error { gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { - return fmt.Errorf("failed to get GVK for object %T: %v", obj, err) + return fmt.Errorf("failed to get GVK for object %T: %w", obj, err) } keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(key) @@ -48,7 +48,7 @@ func (c *consistentClient) Get(ctx context.Context, key ObjectKey, obj Object, o func (c *consistentClient) List(ctx context.Context, list ObjectList, opts ...ListOption) error { gvk, err := apiutil.GVKForObject(list, c.upstream.Scheme()) if err != nil { - return fmt.Errorf("failed to get GVK for list %T: %v", list, err) + return fmt.Errorf("failed to get GVK for list %T: %w", list, err) } keys := c.lockedKeysByGVK.getOrCreate(gvk).allValues() @@ -64,14 +64,14 @@ func (c *consistentClient) List(ctx context.Context, list ObjectList, opts ...Li func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...CreateOption) error { gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { - return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) } defer keyLock.unlock() @@ -82,7 +82,7 @@ func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...Creat rvRaw := obj.GetResourceVersion() rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { - return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) @@ -92,14 +92,14 @@ func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...Creat func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error { gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { - return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) } defer keyLock.unlock() @@ -110,7 +110,7 @@ func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...Updat rvRaw := obj.GetResourceVersion() rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { - return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) @@ -120,14 +120,14 @@ func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...Updat func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error { gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { - return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) } defer keyLock.unlock() @@ -138,7 +138,7 @@ func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, o rvRaw := obj.GetResourceVersion() rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { - return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) @@ -160,7 +160,7 @@ func (c *consistentClient) Apply(ctx context.Context, obj runtime.ApplyConfigura case applyConfiguration: gv, err := schema.ParseGroupVersion(*t.GetAPIVersion()) if err != nil { - return fmt.Errorf("failed to parse group version %s: %v", *t.GetAPIVersion(), err) + return fmt.Errorf("failed to parse group version %s: %w", *t.GetAPIVersion(), err) } gvk.Group = gv.Group gvk.Version = gv.Version @@ -176,7 +176,7 @@ func (c *consistentClient) Apply(ctx context.Context, obj runtime.ApplyConfigura keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) } defer keyLock.unlock() @@ -186,11 +186,11 @@ func (c *consistentClient) Apply(ctx context.Context, obj runtime.ApplyConfigura rvRaw, err := getResourceVersion() if err != nil { - return fmt.Errorf("failed to get resource version from apply configuration: %v", err) + return fmt.Errorf("failed to get resource version from apply configuration: %w", err) } rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { - return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) @@ -221,38 +221,38 @@ func resourceVersionFromApplyConfiguration(obj applyConfiguration) (string, erro func (c *consistentClient) Delete(ctx context.Context, obj Object, opts ...DeleteOption) error { gvk, err := apiutil.GVKForObject(obj, c.upstream.Scheme()) if err != nil { - return fmt.Errorf("failed to get GVK for object %v: %v", obj, err) + return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %v", namespacedName.Namespace, namespacedName.Name, err) + return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) } defer keyLock.unlock() // Register the delete before we execute it, otherwise it may be in the cache // before we register it, causing a deadlock. if err := c.cache.AddRequiredDeleteForObject(obj); err != nil { - return fmt.Errorf("failed to add required delete for object: %v", err) + return fmt.Errorf("failed to add required delete for object: %w", err) } response, err := c.upstream.delete(ctx, obj, opts...) if err != nil { if removeErr := c.cache.RemoveRequiredDeleteForObject(obj); removeErr != nil { - return errors.Join(err, fmt.Errorf("failed to remove required delete for object after delete error: %v", removeErr)) + return errors.Join(err, fmt.Errorf("failed to remove required delete for object after delete error: %w", removeErr)) } return err } if rvRaw := response.GetResourceVersion(); rvRaw != "" { if err := c.cache.RemoveRequiredDeleteForObject(obj); err != nil { - return fmt.Errorf("failed to remove required delete for object after successful delete: %v", err) + return fmt.Errorf("failed to remove required delete for object after successful delete: %w", err) } rv, err := strconv.ParseInt(rvRaw, 10, 64) if err != nil { - return fmt.Errorf("failed to parse resource version %s: %v", rvRaw, err) + return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) } c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) } diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index b90fca3788..17ffc0f851 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -25,8 +25,8 @@ var _ = Describe("ConsistentClient", func() { counter uint64 ) - BeforeEach(func() { - ctx, cancel = context.WithCancel(context.Background()) + BeforeEach(func(specCtx context.Context) { + ctx, cancel = context.WithCancel(specCtx) c, err := cache.New(cfg, cache.Options{Scheme: kscheme.Scheme}) Expect(err).NotTo(HaveOccurred()) From ef86becd886233e29f1bde2345518673a417ebe6 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 12 Jul 2026 13:12:49 -0400 Subject: [PATCH 19/25] Add intenralCache to avoid leaking methods required for consistent reads into the public interface --- pkg/cache/cache.go | 15 ++++++++++----- pkg/cache/delegating_by_gvk_cache.go | 8 ++++---- pkg/cache/multi_namespace_cache.go | 12 ++++++------ 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 377519316d..41f21abe1c 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -68,7 +68,12 @@ type Cache interface { // Informers loads informers and adds field indices. Informers +} +// internalCache is implemented by all cache implementations in controller-runtime. It +// adds the requirement methods to be able to work for a consistent client. +type internalCache interface { + Cache // SetMinimumRVForGVKAndKey causes subsequent Get requests for the given // GVK and key to block until the informer for that GVK has observed a // resource version >= rv (or the context times out). For List requests @@ -463,7 +468,7 @@ func New(cfg *rest.Config, opts Options) (Cache, error) { newCacheFunc := newCache(cfg, opts) - var defaultCache Cache + var defaultCache internalCache if len(opts.DefaultNamespaces) > 0 { defaultConfig := optionDefaultsToConfig(&opts) defaultCache = newMultiNamespaceCache(newCacheFunc, opts.Scheme, opts.Mapper, opts.DefaultNamespaces, &defaultConfig) @@ -477,7 +482,7 @@ func New(cfg *rest.Config, opts Options) (Cache, error) { delegating := &delegatingByGVKCache{ scheme: opts.Scheme, - caches: make(map[schema.GroupVersionKind]Cache, len(opts.ByObject)), + caches: make(map[schema.GroupVersionKind]internalCache, len(opts.ByObject)), defaultCache: defaultCache, } @@ -486,7 +491,7 @@ func New(cfg *rest.Config, opts Options) (Cache, error) { if err != nil { return nil, fmt.Errorf("failed to get GVK for type %T: %w", obj, err) } - var cache Cache + var cache internalCache if len(config.Namespaces) > 0 { cache = newMultiNamespaceCache(newCacheFunc, opts.Scheme, opts.Mapper, config.Namespaces, nil) } else { @@ -534,10 +539,10 @@ func byObjectToConfig(byObject ByObject) Config { } } -type newCacheFunc func(config Config, namespace string) Cache +type newCacheFunc func(config Config, namespace string) internalCache func newCache(restConfig *rest.Config, opts Options) newCacheFunc { - return func(config Config, namespace string) Cache { + return func(config Config, namespace string) internalCache { return &informerCache{ scheme: opts.Scheme, Informers: internal.NewInformers(restConfig, &internal.InformersOpts{ diff --git a/pkg/cache/delegating_by_gvk_cache.go b/pkg/cache/delegating_by_gvk_cache.go index 67e912f2d9..cf036c5ad2 100644 --- a/pkg/cache/delegating_by_gvk_cache.go +++ b/pkg/cache/delegating_by_gvk_cache.go @@ -34,8 +34,8 @@ import ( // and uses the defaultCache otherwise. type delegatingByGVKCache struct { scheme *runtime.Scheme - caches map[schema.GroupVersionKind]Cache - defaultCache Cache + caches map[schema.GroupVersionKind]internalCache + defaultCache internalCache } func (dbt *delegatingByGVKCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { @@ -137,7 +137,7 @@ func (dbt *delegatingByGVKCache) IndexField(ctx context.Context, obj client.Obje return cache.IndexField(ctx, obj, field, extractValue) } -func (dbt *delegatingByGVKCache) cacheForObject(o runtime.Object) (Cache, error) { +func (dbt *delegatingByGVKCache) cacheForObject(o runtime.Object) (internalCache, error) { gvk, err := apiutil.GVKForObject(o, dbt.scheme) if err != nil { return nil, err @@ -146,7 +146,7 @@ func (dbt *delegatingByGVKCache) cacheForObject(o runtime.Object) (Cache, error) return dbt.cacheForGVK(gvk), nil } -func (dbt *delegatingByGVKCache) cacheForGVK(gvk schema.GroupVersionKind) Cache { +func (dbt *delegatingByGVKCache) cacheForGVK(gvk schema.GroupVersionKind) internalCache { if specific, hasSpecific := dbt.caches[gvk]; hasSpecific { return specific } diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go index 3cdc2a6f13..546f0f0d19 100644 --- a/pkg/cache/multi_namespace_cache.go +++ b/pkg/cache/multi_namespace_cache.go @@ -42,15 +42,15 @@ func newMultiNamespaceCache( restMapper apimeta.RESTMapper, namespaces map[string]Config, globalConfig *Config, // may be nil in which case no cache for cluster-scoped objects will be created -) Cache { +) internalCache { // Create every namespace cache. - caches := map[string]Cache{} + caches := map[string]internalCache{} for namespace, config := range namespaces { caches[namespace] = newCache(config, namespace) } // Create a cache for cluster scoped resources if requested - var clusterCache Cache + var clusterCache internalCache if globalConfig != nil { clusterCache = newCache(*globalConfig, corev1.NamespaceAll) } @@ -70,11 +70,11 @@ func newMultiNamespaceCache( type multiNamespaceCache struct { Scheme *runtime.Scheme RESTMapper apimeta.RESTMapper - namespaceToCache map[string]Cache - clusterCache Cache + namespaceToCache map[string]internalCache + clusterCache internalCache } -var _ Cache = &multiNamespaceCache{} +var _ internalCache = &multiNamespaceCache{} // Methods for multiNamespaceCache to conform to the Informers interface. From 79e513f9854bbf99c0d44a3bd1af6fc70a182e3f Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 12 Jul 2026 14:06:50 -0400 Subject: [PATCH 20/25] Avoid goroutine leak and failing on already-canceled context --- .../readerconsistency/readerconsistency.go | 142 +++++++++++------- 1 file changed, 85 insertions(+), 57 deletions(-) diff --git a/pkg/cache/internal/readerconsistency/readerconsistency.go b/pkg/cache/internal/readerconsistency/readerconsistency.go index 4f85241b9c..1bb1ac60e5 100644 --- a/pkg/cache/internal/readerconsistency/readerconsistency.go +++ b/pkg/cache/internal/readerconsistency/readerconsistency.go @@ -22,6 +22,7 @@ import ( "maps" "strconv" "sync" + "sync/atomic" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" @@ -31,27 +32,29 @@ import ( func NewHandler(rvCleanup func(int64)) *ConsistencyHandler { return &ConsistencyHandler{ - rvCond: sync.NewCond(&sync.Mutex{}), - pendingDeletesCond: sync.NewCond(&sync.Mutex{}), + rvCond: &broadcaster{}, + pendingDeletesCond: &broadcaster{}, + pendingDeletesLock: sync.RWMutex{}, pendingDeletes: make(map[client.ObjectKey]sets.Set[types.UID]), rvCleanup: rvCleanup, } } type ConsistencyHandler struct { - rvCond *sync.Cond - observedRV int64 + rvCond *broadcaster + observedRV atomic.Int64 - pendingDeletesCond *sync.Cond - // pendingDeletes holds pending deletes. Must only be acquired when holding pendingDeletesCond.L + pendingDeletesCond *broadcaster + pendingDeletesLock sync.RWMutex + // pendingDeletes holds pending deletes. Must only be acquired when holding pendingDeletesLock pendingDeletes map[client.ObjectKey]sets.Set[types.UID] rvCleanup func(int64) } func (h *ConsistencyHandler) AddPendingDelete(key client.ObjectKey, uid types.UID) { - h.pendingDeletesCond.L.Lock() - defer h.pendingDeletesCond.L.Unlock() + h.pendingDeletesLock.Lock() + defer h.pendingDeletesLock.Unlock() if h.pendingDeletes[key] == nil { h.pendingDeletes[key] = sets.New(uid) @@ -61,15 +64,15 @@ func (h *ConsistencyHandler) AddPendingDelete(key client.ObjectKey, uid types.UI } func (h *ConsistencyHandler) RemovePendingDelete(key client.ObjectKey, uid types.UID) { - h.pendingDeletesCond.L.Lock() - defer h.pendingDeletesCond.L.Unlock() + h.pendingDeletesLock.Lock() + defer h.pendingDeletesLock.Unlock() if h.pendingDeletes[key] != nil { h.pendingDeletes[key].Delete(uid) if len(h.pendingDeletes[key]) == 0 { delete(h.pendingDeletes, key) } - h.pendingDeletesCond.Broadcast() + h.pendingDeletesCond.broadcast() } } @@ -91,21 +94,21 @@ func (h *ConsistencyHandler) WaitForGet(ctx context.Context, key client.ObjectKe // waitDeletesForKey blocks until all pending deletes at the time of calling it were observed or context times out func (h *ConsistencyHandler) waitDeletesForKey(ctx context.Context, key client.ObjectKey) error { - h.pendingDeletesCond.L.Lock() + h.pendingDeletesLock.RLock() pendingDeletes := maps.Clone(h.pendingDeletes[key]) - h.pendingDeletesCond.L.Unlock() + h.pendingDeletesLock.RUnlock() return h.waitDeletes(ctx, pendingDeletes) } // waitDeletesForGVK blocks until all pending deletes at the time of calling it were observed or context times out func (h *ConsistencyHandler) waitAllDeletes(ctx context.Context) error { - h.pendingDeletesCond.L.Lock() + h.pendingDeletesLock.RLock() pendingDeletes := sets.Set[types.UID]{} for _, uids := range h.pendingDeletes { maps.Copy(pendingDeletes, uids) } - h.pendingDeletesCond.L.Unlock() + h.pendingDeletesLock.RUnlock() return h.waitDeletes(ctx, pendingDeletes) } @@ -115,24 +118,23 @@ func (h *ConsistencyHandler) waitDeletes(ctx context.Context, uids sets.Set[type return nil } - allDeleted := make(chan struct{}) - go func() { - h.pendingDeletesCond.L.Lock() - for !h.allDeletedLocked(uids) { - if ctx.Err() != nil { - break - } - h.pendingDeletesCond.Wait() + for { + // must store the chan before checking the deletes to guarantee that even if the deletes + // get updated after our check and before the select, we still get an event. + updatedChan := h.pendingDeletesCond.wait() + h.pendingDeletesLock.RLock() + done := h.allDeletedLocked(uids) + h.pendingDeletesLock.RUnlock() + if done { + return nil + } + + select { + case <-updatedChan: + continue + case <-ctx.Done(): + return ctx.Err() } - h.pendingDeletesCond.L.Unlock() - close(allDeleted) - }() - - select { - case <-ctx.Done(): - return ctx.Err() - case <-allDeleted: - return nil } } @@ -150,12 +152,12 @@ func (h *ConsistencyHandler) allDeletedLocked(uids sets.Set[types.UID]) bool { func (h *ConsistencyHandler) observeDeletion(obj client.Object) { key := client.ObjectKey{Namespace: obj.GetNamespace(), Name: obj.GetName()} - h.pendingDeletesCond.L.Lock() - defer h.pendingDeletesCond.L.Unlock() + h.pendingDeletesLock.Lock() + defer h.pendingDeletesLock.Unlock() if h.pendingDeletes[key].Has(obj.GetUID()) { h.pendingDeletes[key].Delete(obj.GetUID()) - h.pendingDeletesCond.Broadcast() + h.pendingDeletesCond.broadcast() } if len(h.pendingDeletes[key]) == 0 { delete(h.pendingDeletes, key) @@ -163,24 +165,19 @@ func (h *ConsistencyHandler) observeDeletion(obj client.Object) { } func (h *ConsistencyHandler) waitForRV(ctx context.Context, rv int64) error { - observed := make(chan struct{}) - go func() { - h.rvCond.L.Lock() - for h.observedRV < rv { - if ctx.Err() != nil { - break - } - h.rvCond.Wait() + for { + // must store the chan before checking the RV to guarantee that even if the RV + // gets updated after our check and before the select, we still get an event. + updatedChan := h.rvCond.wait() + if h.observedRV.Load() >= rv { + return nil + } + select { + case <-updatedChan: + continue + case <-ctx.Done(): + return ctx.Err() } - h.rvCond.L.Unlock() - close(observed) - }() - - select { - case <-ctx.Done(): - return ctx.Err() - case <-observed: - return nil } } @@ -191,14 +188,19 @@ func (h *ConsistencyHandler) observeResourceVersion(rv string) { return } - h.rvCond.L.Lock() - defer h.rvCond.L.Unlock() + for { + current := h.observedRV.Load() + if parsed <= current { + return + } - if parsed > h.observedRV { - h.observedRV = parsed - h.rvCond.Broadcast() + if h.observedRV.CompareAndSwap(current, parsed) { + break + } } + h.rvCond.broadcast() + go h.rvCleanup(parsed) } @@ -234,3 +236,29 @@ func (h *ConsistencyHandler) OnDelete(raw any) { go func() { h.observeResourceVersion(obj.GetResourceVersion()) }() go func() { h.observeDeletion(obj) }() } + +type broadcaster struct { + lock sync.Mutex + ch chan struct{} +} + +func (b *broadcaster) broadcast() { + b.lock.Lock() + defer b.lock.Unlock() + // Lazily create the channel in wait to avoid creating a channel per event. + if b.ch != nil { + close(b.ch) + b.ch = nil + } +} + +func (b *broadcaster) wait() <-chan struct{} { + b.lock.Lock() + defer b.lock.Unlock() + + if b.ch == nil { + b.ch = make(chan struct{}) + } + + return b.ch +} From 7f70c87639086a352246df8e68d9779dd21cf0af Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 12 Jul 2026 18:44:09 -0400 Subject: [PATCH 21/25] Address lints --- pkg/client/consistency.go | 56 +++++++------------------- pkg/client/consistency_envtest_test.go | 8 ++-- 2 files changed, 18 insertions(+), 46 deletions(-) diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index e3c7a564a0..32f46f529f 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -67,26 +67,9 @@ func (c *consistentClient) Create(ctx context.Context, obj Object, opts ...Creat return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } - namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} - - keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) - if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) - } - defer keyLock.unlock() - - if err := c.upstream.Create(ctx, obj, opts...); err != nil { - return err - } - - rvRaw := obj.GetResourceVersion() - rv, err := strconv.ParseInt(rvRaw, 10, 64) - if err != nil { - return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) - } - c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) - - return nil + return c.writeAndRecordRV(ctx, gvk, obj, func() error { + return c.upstream.Create(ctx, obj, opts...) + }) } func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...UpdateOption) error { @@ -95,26 +78,9 @@ func (c *consistentClient) Update(ctx context.Context, obj Object, opts ...Updat return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } - namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} - - keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) - if err := keyLock.lock(ctx); err != nil { - return fmt.Errorf("failed to acquire lock for %s/%s: %w", namespacedName.Namespace, namespacedName.Name, err) - } - defer keyLock.unlock() - - if err := c.upstream.Update(ctx, obj, opts...); err != nil { - return err - } - - rvRaw := obj.GetResourceVersion() - rv, err := strconv.ParseInt(rvRaw, 10, 64) - if err != nil { - return fmt.Errorf("failed to parse resource version %s: %w", rvRaw, err) - } - c.cache.SetMinimumRVForGVKAndKey(gvk, namespacedName, rv) - - return nil + return c.writeAndRecordRV(ctx, gvk, obj, func() error { + return c.upstream.Update(ctx, obj, opts...) + }) } func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error { @@ -123,6 +89,12 @@ func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, o return fmt.Errorf("failed to get GVK for object %v: %w", obj, err) } + return c.writeAndRecordRV(ctx, gvk, obj, func() error { + return c.upstream.Patch(ctx, obj, patch, opts...) + }) +} + +func (c *consistentClient) writeAndRecordRV(ctx context.Context, gvk schema.GroupVersionKind, obj Object, write func() error) error { namespacedName := types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()} keyLock := c.lockedKeysByGVK.getOrCreate(gvk).getOrCreate(namespacedName) @@ -131,7 +103,7 @@ func (c *consistentClient) Patch(ctx context.Context, obj Object, patch Patch, o } defer keyLock.unlock() - if err := c.upstream.Patch(ctx, obj, patch, opts...); err != nil { + if err := write(); err != nil { return err } @@ -209,7 +181,7 @@ func resourceVersionFromApplyConfiguration(obj applyConfiguration) (string, erro if !rv.IsValid() { return "", fmt.Errorf("type %T has no ResourceVersion field", obj) } - if rv.Kind() != reflect.Ptr || rv.Type().Elem().Kind() != reflect.String { + if rv.Kind() != reflect.Pointer || rv.Type().Elem().Kind() != reflect.String { return "", fmt.Errorf("ResourceVersion field in %T is not *string", obj) } if rv.IsNil() { diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index 17ffc0f851..bd7c89b223 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -22,7 +22,7 @@ var _ = Describe("ConsistentClient", func() { cl client.Client ctx context.Context cancel context.CancelFunc - counter uint64 + counter atomic.Uint64 ) BeforeEach(func(specCtx context.Context) { @@ -58,7 +58,7 @@ var _ = Describe("ConsistentClient", func() { }) newConfigMap := func(ns string) *corev1.ConfigMap { - n := atomic.AddUint64(&counter, 1) + n := counter.Add(1) return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("consistency-test-%d", n), @@ -76,7 +76,7 @@ var _ = Describe("ConsistentClient", func() { DescribeTable("write then read", func(ctx context.Context, write func(ctx context.Context, cl client.Client, cm *corev1.ConfigMap) (writeResult, error)) { - ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("consistency-wtr-%d", atomic.AddUint64(&counter, 1))}} + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("consistency-wtr-%d", counter.Add(1))}} Expect(cl.Create(ctx, ns)).To(Succeed()) DeferCleanup(func(ctx context.Context) { Expect(client.IgnoreNotFound(cl.Delete(ctx, ns))).To(Succeed()) @@ -212,7 +212,7 @@ var _ = Describe("ConsistentClient", func() { Describe("Create with namespace-scoped object", func() { It("should work across different namespaces", func() { - ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("consistency-ns-%d", atomic.AddUint64(&counter, 1))}} + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("consistency-ns-%d", counter.Add(1))}} Expect(cl.Create(ctx, ns)).To(Succeed()) DeferCleanup(func(ctx context.Context) { Expect(client.IgnoreNotFound(cl.Delete(ctx, ns))).To(Succeed()) From 1b473ee1852adcc0f21ea30970d8606109ddcc84 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 12 Jul 2026 18:50:04 -0400 Subject: [PATCH 22/25] Missing boilerplate header --- pkg/client/consistency_envtest_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index bd7c89b223..7da43dfa31 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -1,3 +1,19 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package client_test import ( From bf248e0233a86fede64ccc4ad846b9076cedf4c1 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 12 Jul 2026 18:50:59 -0400 Subject: [PATCH 23/25] Fakeinformers don't need to deal with consistency --- pkg/cache/informertest/fake_cache.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index d760d31d75..3ca4705e57 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -131,20 +131,6 @@ func (c *FakeInformers) IndexField(ctx context.Context, obj client.Object, field return nil } -// SetMinimumRVForGVKAndKey implements Cache. -func (c *FakeInformers) SetMinimumRVForGVKAndKey(_ schema.GroupVersionKind, _ client.ObjectKey, _ int64) { -} - -// AddRequiredDeleteForObject implements Cache. -func (c *FakeInformers) AddRequiredDeleteForObject(client.Object) error { - return nil -} - -// RemoveRequiredDeleteForObject implements Cache. -func (c *FakeInformers) RemoveRequiredDeleteForObject(client.Object) error { - return nil -} - // Get implements Cache. func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { return nil From ae1bb9c5c3b089ed5064400079fef95db4c3dd75 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 12 Jul 2026 19:21:00 -0400 Subject: [PATCH 24/25] More missing boilerplate headers --- pkg/client/consistency.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/client/consistency.go b/pkg/client/consistency.go index 32f46f529f..5630b24f5f 100644 --- a/pkg/client/consistency.go +++ b/pkg/client/consistency.go @@ -1,3 +1,19 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package client import ( From ac94867c593637a27d568bfc0eca7dac223af417 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Mon, 13 Jul 2026 22:12:35 -0400 Subject: [PATCH 25/25] Fix test using incorrect context --- pkg/client/consistency_envtest_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/client/consistency_envtest_test.go b/pkg/client/consistency_envtest_test.go index 7da43dfa31..a29f7f0e0c 100644 --- a/pkg/client/consistency_envtest_test.go +++ b/pkg/client/consistency_envtest_test.go @@ -41,8 +41,10 @@ var _ = Describe("ConsistentClient", func() { counter atomic.Uint64 ) - BeforeEach(func(specCtx context.Context) { - ctx, cancel = context.WithCancel(specCtx) + BeforeEach(func() { + // NB: Don't derive from the BeforeEach's context, Ginkgo cancels it when the + // node returns and it thus would not outlive it, stopping the cache's watches. + ctx, cancel = context.WithCancel(context.Background()) c, err := cache.New(cfg, cache.Options{Scheme: kscheme.Scheme}) Expect(err).NotTo(HaveOccurred())