From 0f4b8a70de2d6abeb9b24eac6a6b581869618ba2 Mon Sep 17 00:00:00 2001 From: Marco Nenciarini Date: Tue, 2 Sep 2025 19:15:30 +0200 Subject: [PATCH] fix: improve object cache reliability * make sure objects expire after DefaultTTLSeconds * make cached objects have GKV information * fix cache retrieval and removal logic Closes #502 Signed-off-by: Marco Nenciarini --- .../cnpgi/instance/internal/client/client.go | 43 ++++++++++- .../instance/internal/client/client_test.go | 77 ++++++++++++++----- 2 files changed, 96 insertions(+), 24 deletions(-) diff --git a/internal/cnpgi/instance/internal/client/client.go b/internal/cnpgi/instance/internal/client/client.go index e634532..5beb487 100644 --- a/internal/cnpgi/instance/internal/client/client.go +++ b/internal/cnpgi/instance/internal/client/client.go @@ -21,11 +21,11 @@ const DefaultTTLSeconds = 10 type cachedEntry struct { entry client.Object fetchUnixTime int64 - ttl time.Duration + ttlSeconds int64 } func (e *cachedEntry) isExpired() bool { - return time.Now().Unix()-e.fetchUnixTime > int64(e.ttl) + return time.Now().Unix()-e.fetchUnixTime > e.ttlSeconds } // ExtendedClient is an extended client that is capable of caching multiple secrets without relying on informers @@ -71,6 +71,28 @@ func (e *ExtendedClient) Get( return e.Client.Get(ctx, key, obj, opts...) } +// addTypeInformationToObject adds TypeMeta information to a client.Object based upon the client Scheme +// inspired by: https://github.com/kubernetes/cli-runtime/blob/v0.19.2/pkg/printers/typesetter.go#L41 +func (e *ExtendedClient) addTypeInformationToObject(obj client.Object) error { + gvks, _, err := e.Client.Scheme().ObjectKinds(obj) + if err != nil { + return fmt.Errorf("missing apiVersion or kind and cannot assign it; %w", err) + } + + for _, gvk := range gvks { + if len(gvk.Kind) == 0 { + continue + } + if len(gvk.Version) == 0 || gvk.Version == runtime.APIVersionInternal { + continue + } + obj.GetObjectKind().SetGroupVersionKind(gvk) + break + } + + return nil +} + func (e *ExtendedClient) getCachedObject( ctx context.Context, key client.ObjectKey, @@ -81,6 +103,12 @@ func (e *ExtendedClient) getCachedObject( WithName("extended_client"). WithValues("name", key.Name, "namespace", key.Namespace) + // Make sure the object has GVK information + // This is needed to compare the object type with the cached one + if err := e.addTypeInformationToObject(obj); err != nil { + return fmt.Errorf("cannot add type metadata to object of type %T: %w", obj, err) + } + contextLogger.Trace("locking the cache") e.mux.Lock() defer e.mux.Unlock() @@ -120,9 +148,16 @@ func (e *ExtendedClient) getCachedObject( return err } + // Populate the GKV information again, as the client.Get() may have + // returned an object without this information set + if err := e.addTypeInformationToObject(obj); err != nil { + return fmt.Errorf("cannot add type metadata to object of type %T: %w", obj, err) + } + cs := cachedEntry{ - entry: obj.(runtime.Object).DeepCopyObject().(client.Object), + entry: obj.DeepCopyObject().(client.Object), fetchUnixTime: time.Now().Unix(), + ttlSeconds: DefaultTTLSeconds, } contextLogger.Debug("setting object in the cache") @@ -143,7 +178,7 @@ func (e *ExtendedClient) removeObject(object client.Object) { for i, cache := range e.cachedObjects { if cache.entry.GetNamespace() == object.GetNamespace() && cache.entry.GetName() == object.GetName() && - cache.entry.GetObjectKind().GroupVersionKind() != object.GetObjectKind().GroupVersionKind() { + cache.entry.GetObjectKind().GroupVersionKind() == object.GetObjectKind().GroupVersionKind() { e.cachedObjects = append(e.cachedObjects[:i], e.cachedObjects[i+1:]...) return } diff --git a/internal/cnpgi/instance/internal/client/client_test.go b/internal/cnpgi/instance/internal/client/client_test.go index 70568f4..729f64a 100644 --- a/internal/cnpgi/instance/internal/client/client_test.go +++ b/internal/cnpgi/instance/internal/client/client_test.go @@ -9,7 +9,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - v1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" + barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -20,16 +20,28 @@ var scheme = buildScheme() func buildScheme() *runtime.Scheme { scheme := runtime.NewScheme() _ = corev1.AddToScheme(scheme) - _ = v1.AddToScheme(scheme) + _ = barmancloudv1.AddToScheme(scheme) return scheme } +func addToCache(c *ExtendedClient, obj client.Object, fetchUnixTime int64) { + ce := cachedEntry{ + entry: obj.DeepCopyObject().(client.Object), + fetchUnixTime: fetchUnixTime, + ttlSeconds: DefaultTTLSeconds, + } + ce.entry.SetResourceVersion("from cache") + err := c.addTypeInformationToObject(ce.entry) + Expect(err).ToNot(HaveOccurred()) + c.cachedObjects = append(c.cachedObjects, ce) +} + var _ = Describe("ExtendedClient Get", func() { var ( extendedClient *ExtendedClient secretInClient *corev1.Secret - objectStore *v1.ObjectStore + objectStore *barmancloudv1.ObjectStore ) BeforeEach(func() { @@ -39,12 +51,12 @@ var _ = Describe("ExtendedClient Get", func() { Name: "test-secret", }, } - objectStore = &v1.ObjectStore{ + objectStore = &barmancloudv1.ObjectStore{ ObjectMeta: metav1.ObjectMeta{ Namespace: "default", Name: "test-object-store", }, - Spec: v1.ObjectStoreSpec{}, + Spec: barmancloudv1.ObjectStoreSpec{}, } baseClient := fake.NewClientBuilder(). @@ -61,35 +73,34 @@ var _ = Describe("ExtendedClient Get", func() { }, } - // manually add the secret to the cache, this is not present in the fake client so we are sure it is from the - // cache - extendedClient.cachedObjects = []cachedEntry{ - { - entry: secretNotInClient, - fetchUnixTime: time.Now().Unix(), - }, - } + // manually add the secret to the cache, this is not present in the fake client, + // so we are sure it is from the cache + addToCache(extendedClient, secretNotInClient, time.Now().Unix()) err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretNotInClient), secretInClient) Expect(err).NotTo(HaveOccurred()) - Expect(secretNotInClient).To(Equal(extendedClient.cachedObjects[0].entry)) + Expect(secretInClient).To(Equal(extendedClient.cachedObjects[0].entry)) + Expect(secretInClient.GetResourceVersion()).To(Equal("from cache")) }) It("fetches secret from base client if cache is expired", func(ctx SpecContext) { - extendedClient.cachedObjects = []cachedEntry{ - { - entry: secretInClient.DeepCopy(), - fetchUnixTime: time.Now().Add(-2 * time.Minute).Unix(), - }, - } + addToCache(extendedClient, secretInClient, time.Now().Add(-2*time.Minute).Unix()) err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretInClient), secretInClient) Expect(err).NotTo(HaveOccurred()) + Expect(secretInClient.GetResourceVersion()).NotTo(Equal("from cache")) + + // the cache is updated with the new value + Expect(extendedClient.cachedObjects).To(HaveLen(1)) + Expect(extendedClient.cachedObjects[0].entry.GetResourceVersion()).NotTo(Equal("from cache")) }) It("fetches secret from base client if not in cache", func(ctx SpecContext) { err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretInClient), secretInClient) Expect(err).NotTo(HaveOccurred()) + + // the cache is updated with the new value + Expect(extendedClient.cachedObjects).To(HaveLen(1)) }) It("does not cache non-secret objects", func(ctx SpecContext) { @@ -106,4 +117,30 @@ var _ = Describe("ExtendedClient Get", func() { Expect(err).NotTo(HaveOccurred()) Expect(extendedClient.cachedObjects).To(BeEmpty()) }) + + It("returns the correct object from cache when multiple objects with the same object key are cached", + func(ctx SpecContext) { + secretNotInClient := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "common-name", + }, + } + objectStoreNotInClient := &barmancloudv1.ObjectStore{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "common-name", + }, + } + + // manually add the objects to the cache, these are not present in the fake client, + // so we are sure they are from the cache + addToCache(extendedClient, secretNotInClient, time.Now().Unix()) + addToCache(extendedClient, objectStoreNotInClient, time.Now().Unix()) + + err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretNotInClient), secretInClient) + Expect(err).NotTo(HaveOccurred()) + err = extendedClient.Get(ctx, client.ObjectKeyFromObject(objectStoreNotInClient), objectStore) + Expect(err).NotTo(HaveOccurred()) + }) })