Browse Source

feat(cache): add atomic ContainsOrAdd (#6706)

Add a first-writer-wins cache helper that atomically inserts values when
the key is absent. Treat existing keys as present regardless of version
to avoid concurrent constructors overwriting cached clients.

Use this helper for Vault client, having the same behaviour:
caching and cover version mismatch, cleanup, eviction,
and concurrent insertion behavior.

This sets a precedent for more efficiency in other providers.

Fixes: #6627

Signed-off-by: Jean-Philippe Evrard <jean-philippe.evrard+rochepub@external.roche.com>
Jean-Philippe Evrard 1 week ago
parent
commit
cee28558b7
3 changed files with 82 additions and 3 deletions
  1. 2 2
      providers/v1/vault/provider.go
  2. 11 0
      runtime/cache/cache.go
  3. 69 1
      runtime/cache/cache_test.go

+ 2 - 2
providers/v1/vault/provider.go

@@ -263,8 +263,8 @@ func getVaultClient(p *Provider, store esv1.GenericStore, cfg *vault.Config, nam
 		return nil, fmt.Errorf(errVaultClient, err)
 	}
 
-	if useCache && !clientCache.Contains(key) {
-		clientCache.Add(store.GetObjectMeta().ResourceVersion, key, client)
+	if useCache {
+		clientCache.ContainsOrAdd(store.GetObjectMeta().ResourceVersion, key, client)
 	}
 	return client, nil
 }

+ 11 - 0
runtime/cache/cache.go

@@ -27,6 +27,7 @@ import (
 // lookup values using a key and a version.
 // By design, this cache allows access to only a single version of a given key.
 // A version mismatch is considered a cache miss and the key gets evicted if it exists.
+// ContainsOrAdd is first-writer-wins: any existing version counts as present.
 // When a key is evicted an optional cleanup function is called.
 type Cache[T any] struct {
 	lru         *lru.Cache
@@ -96,6 +97,16 @@ func (c *Cache[T]) Add(version string, key Key, client T) {
 	c.lru.Add(key, value[T]{Version: version, Client: client})
 }
 
+// ContainsOrAdd atomically checks whether the key exists and adds the value if
+// it does not. An existing key counts as present even when its version differs,
+// preventing a concurrent constructor from overwriting the cached value.
+// It returns true when the key already exists. Rejected values are not passed
+// to the cleanup function because they never become owned by the cache.
+func (c *Cache[T]) ContainsOrAdd(version string, key Key, client T) bool {
+	exists, _ := c.lru.ContainsOrAdd(key, value[T]{Version: version, Client: client})
+	return exists
+}
+
 // Contains returns true if a value with the given key exists.
 func (c *Cache[T]) Contains(key Key) bool {
 	return c.lru.Contains(key)

+ 69 - 1
runtime/cache/cache_test.go

@@ -17,12 +17,16 @@ limitations under the License.
 package cache
 
 import (
+	"sync"
+	"sync/atomic"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
-type client struct{}
+type client struct {
+	id int
+}
 
 var cacheKey = Key{Name: "foo"}
 
@@ -55,6 +59,70 @@ func TestCacheContains(t *testing.T) {
 	assert.Nil(t, err)
 }
 
+func TestCacheContainsOrAdd(t *testing.T) {
+	c := Must[client](1, nil)
+	first := client{id: 1}
+	second := client{id: 2}
+
+	assert.False(t, c.ContainsOrAdd("v1", cacheKey, first))
+	assert.True(t, c.ContainsOrAdd("v1", cacheKey, second))
+
+	cached, ok := c.Get("v1", cacheKey)
+	assert.True(t, ok)
+	assert.Equal(t, first, cached)
+}
+
+func TestCacheContainsOrAddVersionMismatch(t *testing.T) {
+	c := Must[client](1, nil)
+	first := client{id: 1}
+
+	c.Add("v1", cacheKey, first)
+	assert.True(t, c.ContainsOrAdd("v2", cacheKey, client{id: 2}))
+
+	cached, ok := c.Get("v1", cacheKey)
+	assert.True(t, ok)
+	assert.Equal(t, first, cached)
+}
+
+func TestCacheContainsOrAddDoesNotCleanUpRejectedValue(t *testing.T) {
+	var cleaned []client
+	c := Must(1, func(value client) {
+		cleaned = append(cleaned, value)
+	})
+	first := client{id: 1}
+
+	assert.False(t, c.ContainsOrAdd("v1", cacheKey, first))
+	assert.True(t, c.ContainsOrAdd("v1", cacheKey, client{id: 2}))
+	assert.Empty(t, cleaned)
+
+	assert.False(t, c.ContainsOrAdd("v1", Key{Name: "bar"}, client{id: 3}))
+	assert.Equal(t, []client{first}, cleaned)
+}
+
+func TestCacheContainsOrAddConcurrent(t *testing.T) {
+	const workers = 64
+	c := Must[client](workers, nil)
+	start := make(chan struct{})
+	var inserted atomic.Int32
+	var wg sync.WaitGroup
+
+	for i := range workers {
+		wg.Go(func() {
+			<-start
+			if !c.ContainsOrAdd("v1", cacheKey, client{id: i}) {
+				inserted.Add(1)
+			}
+		})
+	}
+
+	close(start)
+	wg.Wait()
+
+	assert.EqualValues(t, 1, inserted.Load())
+	_, ok := c.Get("v1", cacheKey)
+	assert.True(t, ok)
+}
+
 func TestCacheGet(t *testing.T) {
 	c, err := New[*client](1, nil)
 	if err != nil {