diff --git a/pkg/kubelet/util/manager/manager.go b/pkg/kubelet/util/manager/manager.go index d4a755f20d7..7c5c908a824 100644 --- a/pkg/kubelet/util/manager/manager.go +++ b/pkg/kubelet/util/manager/manager.go @@ -47,15 +47,15 @@ type Manager interface { // Store is the interface for a object cache that // can be used by cacheBasedManager. type Store interface { - // AddReference adds a reference to the object to the store. + // AddReference adds a reference of referencedFrom to the object to the store. // Note that multiple additions to the store has to be allowed // in the implementations and effectively treated as refcounted. - AddReference(namespace, name string, podUID types.UID) - // DeleteReference deletes reference to the object from the store. + AddReference(namespace, name string, referencedFrom types.UID) + // DeleteReference deletes a reference of referencedFrom to the object from the store. // Note that object should be deleted only when there was a // corresponding Delete call for each of Add calls (effectively - // when refcount was reduced to zero). - DeleteReference(namespace, name string, podUID types.UID) + // when refcount of every referenceFrom was reduced to zero). + DeleteReference(namespace, name string, referencedFrom types.UID) // Get an object from a store. Get(namespace, name string) (runtime.Object, error) } diff --git a/pkg/kubelet/util/manager/watch_based_manager.go b/pkg/kubelet/util/manager/watch_based_manager.go index c8d7d339026..f04891fce53 100644 --- a/pkg/kubelet/util/manager/watch_based_manager.go +++ b/pkg/kubelet/util/manager/watch_based_manager.go @@ -246,7 +246,7 @@ func (c *objectCache) newReflectorLocked(namespace, name string) *objectCacheIte return item } -func (c *objectCache) AddReference(namespace, name string, podUID types.UID) { +func (c *objectCache) AddReference(namespace, name string, referencedFrom types.UID) { key := objectKey{namespace: namespace, name: name} // AddReference is called from RegisterPod thus it needs to be efficient. @@ -261,18 +261,18 @@ func (c *objectCache) AddReference(namespace, name string, podUID types.UID) { item = c.newReflectorLocked(namespace, name) c.items[key] = item } - item.refMap[podUID]++ + item.refMap[referencedFrom]++ } -func (c *objectCache) DeleteReference(namespace, name string, podUID types.UID) { +func (c *objectCache) DeleteReference(namespace, name string, referencedFrom types.UID) { key := objectKey{namespace: namespace, name: name} c.lock.Lock() defer c.lock.Unlock() if item, ok := c.items[key]; ok { - item.refMap[podUID]-- - if item.refMap[podUID] == 0 { - delete(item.refMap, podUID) + item.refMap[referencedFrom]-- + if item.refMap[referencedFrom] == 0 { + delete(item.refMap, referencedFrom) } if len(item.refMap) == 0 { // Stop the underlying reflector. diff --git a/pkg/kubelet/util/manager/watch_based_manager_test.go b/pkg/kubelet/util/manager/watch_based_manager_test.go index 990a7e0919b..64d33c6f53c 100644 --- a/pkg/kubelet/util/manager/watch_based_manager_test.go +++ b/pkg/kubelet/util/manager/watch_based_manager_test.go @@ -499,3 +499,123 @@ func TestReflectorNotStoppedOnSlowInitialization(t *testing.T) { obj, _ := store.Get("ns", "name") assert.True(t, apiequality.Semantic.DeepEqual(secret, obj)) } + +func TestRefMapHandlesReferencesCorrectly(t *testing.T) { + secret1 := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret1", + Namespace: "ns1", + }, + } + type step struct { + action string + ns string + name string + referencedFrom types.UID + } + type expect struct { + ns string + name string + referencedFrom types.UID + expectCount int + } + tests := []struct { + desc string + steps []step + expects []expect + }{ + { + desc: "adding and deleting should works", + steps: []step{ + {"add", "ns1", "secret1", "pod1"}, + {"add", "ns1", "secret1", "pod1"}, + {"delete", "ns1", "secret1", "pod1"}, + {"delete", "ns1", "secret1", "pod1"}, + }, + expects: []expect{ + {"ns1", "secret1", "pod1", 1}, + {"ns1", "secret1", "pod1", 2}, + {"ns1", "secret1", "pod1", 1}, + {"ns1", "secret1", "pod1", 0}, + }, + }, + { + desc: "deleting a non-existent reference should have no effect", + steps: []step{ + {"delete", "ns1", "secret1", "pod1"}, + }, + expects: []expect{ + {"ns1", "secret1", "pod1", 0}, + }, + }, + { + desc: "deleting more than adding should not lead to negative refcount", + steps: []step{ + {"add", "ns1", "secret1", "pod1"}, + {"delete", "ns1", "secret1", "pod1"}, + {"delete", "ns1", "secret1", "pod1"}, + }, + expects: []expect{ + {"ns1", "secret1", "pod1", 1}, + {"ns1", "secret1", "pod1", 0}, + {"ns1", "secret1", "pod1", 0}, + }, + }, + { + desc: "deleting should not affect refcount of other objects or referencedFrom", + steps: []step{ + {"add", "ns1", "secret1", "pod1"}, + {"delete", "ns1", "secret1", "pod2"}, + {"delete", "ns1", "secret2", "pod1"}, + {"delete", "ns2", "secret1", "pod1"}, + }, + expects: []expect{ + {"ns1", "secret1", "pod1", 1}, + {"ns1", "secret1", "pod1", 1}, + {"ns1", "secret1", "pod1", 1}, + {"ns1", "secret1", "pod1", 1}, + }, + }, + } + for _, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + fakeClient := &fake.Clientset{} + listReactor := func(a core.Action) (bool, runtime.Object, error) { + result := &v1.SecretList{ + ListMeta: metav1.ListMeta{ + ResourceVersion: "200", + }, + Items: []v1.Secret{*secret1}, + } + return true, result, nil + } + fakeClient.AddReactor("list", "secrets", listReactor) + fakeWatch := watch.NewFake() + fakeClient.AddWatchReactor("secrets", core.DefaultWatchReactor(fakeWatch, nil)) + fakeClock := testingclock.NewFakeClock(time.Now()) + store := newSecretCache(fakeClient, fakeClock, time.Minute) + + for i, step := range tc.steps { + expect := tc.expects[i] + switch step.action { + case "add": + store.AddReference(step.ns, step.name, step.referencedFrom) + case "delete": + store.DeleteReference(step.ns, step.name, step.referencedFrom) + default: + t.Errorf("unrecognized action of testcase %v", tc.desc) + } + + key := objectKey{namespace: expect.ns, name: expect.name} + item, exists := store.items[key] + if !exists { + if tc.expects[i].expectCount != 0 { + t.Errorf("reference to %v/%v from %v should exists", expect.ns, expect.name, expect.referencedFrom) + } + } else if item.refMap[expect.referencedFrom] != expect.expectCount { + t.Errorf("expects %v but got %v", expect.expectCount, item.refMap[expect.referencedFrom]) + } + } + }) + } +}