From 58ae5a78f997e2c8a291eecf4d9fb57b7d26beb5 Mon Sep 17 00:00:00 2001 From: Joel Smith Date: Wed, 6 Sep 2017 13:30:23 -0600 Subject: [PATCH] Clean up kublet secret and configmap unit test * Expected value comes before actual value in assert.Equal() * Use assert.Equal() instead of assert.True() when possible * Add a unit test that verifies no-op pod updates to the secret_manager and the configmap_manager * Add a clarifying comment about why it's good to seemingly delete a secret on updates. * Fix (for now, non-buggy) variable shadowing issue --- pkg/kubelet/configmap/configmap_manager.go | 5 +++ .../configmap/configmap_manager_test.go | 38 ++++++++++------ pkg/kubelet/secret/secret_manager.go | 5 +++ pkg/kubelet/secret/secret_manager_test.go | 43 ++++++++++++------- 4 files changed, 62 insertions(+), 29 deletions(-) diff --git a/pkg/kubelet/configmap/configmap_manager.go b/pkg/kubelet/configmap/configmap_manager.go index d66f0bc2386..cf5d263bdf0 100644 --- a/pkg/kubelet/configmap/configmap_manager.go +++ b/pkg/kubelet/configmap/configmap_manager.go @@ -282,6 +282,11 @@ func (c *cachingConfigMapManager) RegisterPod(pod *v1.Pod) { c.registeredPods[key] = pod if prev != nil { for name := range getConfigMapNames(prev) { + // On an update, the .Add() call above will have re-incremented the + // ref count of any existing items, so any configmaps that are in both + // names and prev need to have their ref counts decremented. Any that + // are only in prev need to be completely removed. This unconditional + // call takes care of both cases. c.configMapStore.Delete(prev.Namespace, name) } } diff --git a/pkg/kubelet/configmap/configmap_manager_test.go b/pkg/kubelet/configmap/configmap_manager_test.go index ec2a74257a7..d79a80b5012 100644 --- a/pkg/kubelet/configmap/configmap_manager_test.go +++ b/pkg/kubelet/configmap/configmap_manager_test.go @@ -299,11 +299,11 @@ type configMapsToAttach struct { volumes []string } -func podWithConfigMaps(ns, name string, toAttach configMapsToAttach) *v1.Pod { +func podWithConfigMaps(ns, podName string, toAttach configMapsToAttach) *v1.Pod { pod := &v1.Pod{ ObjectMeta: metav1.ObjectMeta{ Namespace: ns, - Name: name, + Name: podName, }, Spec: v1.PodSpec{}, } @@ -454,6 +454,17 @@ func TestCacheRefcounts(t *testing.T) { manager.RegisterPod(podWithConfigMaps("ns1", "other-name", s2)) manager.UnregisterPod(podWithConfigMaps("ns1", "other-name", s2)) + s5 := configMapsToAttach{ + containerEnvConfigMaps: []envConfigMaps{ + {envVarNames: []string{"s7"}}, + {envFromNames: []string{"s70"}}, + }, + } + + // Check the no-op update scenario + manager.RegisterPod(podWithConfigMaps("ns1", "noop-pod", s5)) + manager.RegisterPod(podWithConfigMaps("ns1", "noop-pod", s5)) + refs := func(ns, name string) int { store.lock.Lock() defer store.lock.Unlock() @@ -463,17 +474,18 @@ func TestCacheRefcounts(t *testing.T) { } return item.refCount } - assert.Equal(t, refs("ns1", "s1"), 1) - assert.Equal(t, refs("ns1", "s10"), 1) - assert.Equal(t, refs("ns1", "s2"), 1) - assert.Equal(t, refs("ns1", "s3"), 3) - assert.Equal(t, refs("ns1", "s30"), 2) - assert.Equal(t, refs("ns1", "s4"), 2) - assert.Equal(t, refs("ns1", "s5"), 4) - assert.Equal(t, refs("ns1", "s50"), 2) - assert.Equal(t, refs("ns1", "s6"), 0) - assert.Equal(t, refs("ns1", "s60"), 0) - assert.Equal(t, refs("ns1", "s7"), 0) + assert.Equal(t, 1, refs("ns1", "s1")) + assert.Equal(t, 1, refs("ns1", "s10")) + assert.Equal(t, 1, refs("ns1", "s2")) + assert.Equal(t, 3, refs("ns1", "s3")) + assert.Equal(t, 2, refs("ns1", "s30")) + assert.Equal(t, 2, refs("ns1", "s4")) + assert.Equal(t, 4, refs("ns1", "s5")) + assert.Equal(t, 2, refs("ns1", "s50")) + assert.Equal(t, 0, refs("ns1", "s6")) + assert.Equal(t, 0, refs("ns1", "s60")) + assert.Equal(t, 1, refs("ns1", "s7")) + assert.Equal(t, 1, refs("ns1", "s70")) } func TestCachingConfigMapManager(t *testing.T) { diff --git a/pkg/kubelet/secret/secret_manager.go b/pkg/kubelet/secret/secret_manager.go index 3b7356f8aeb..7fd8008784e 100644 --- a/pkg/kubelet/secret/secret_manager.go +++ b/pkg/kubelet/secret/secret_manager.go @@ -282,6 +282,11 @@ func (c *cachingSecretManager) RegisterPod(pod *v1.Pod) { c.registeredPods[key] = pod if prev != nil { for name := range getSecretNames(prev) { + // On an update, the .Add() call above will have re-incremented the + // ref count of any existing secrets, so any secrets that are in both + // names and prev need to have their ref counts decremented. Any that + // are only in prev need to be completely removed. This unconditional + // call takes care of both cases. c.secretStore.Delete(prev.Namespace, name) } } diff --git a/pkg/kubelet/secret/secret_manager_test.go b/pkg/kubelet/secret/secret_manager_test.go index 701a1c4b49c..36380e514cc 100644 --- a/pkg/kubelet/secret/secret_manager_test.go +++ b/pkg/kubelet/secret/secret_manager_test.go @@ -299,11 +299,11 @@ type secretsToAttach struct { containerEnvSecrets []envSecrets } -func podWithSecrets(ns, name string, toAttach secretsToAttach) *v1.Pod { +func podWithSecrets(ns, podName string, toAttach secretsToAttach) *v1.Pod { pod := &v1.Pod{ ObjectMeta: metav1.ObjectMeta{ Namespace: ns, - Name: name, + Name: podName, }, Spec: v1.PodSpec{}, } @@ -453,27 +453,38 @@ func TestCacheRefcounts(t *testing.T) { manager.RegisterPod(podWithSecrets("ns1", "other-name", s2)) manager.UnregisterPod(podWithSecrets("ns1", "other-name", s2)) + s5 := secretsToAttach{ + containerEnvSecrets: []envSecrets{ + {envVarNames: []string{"s7"}}, + {envFromNames: []string{"s70"}}, + }, + } + // Check the no-op update scenario + manager.RegisterPod(podWithSecrets("ns1", "noop-pod", s5)) + manager.RegisterPod(podWithSecrets("ns1", "noop-pod", s5)) + // Now we have: 3 pods with s1, 2 pods with s2 and 2 pods with s3, 0 pods with s4. - verify := func(ns, name string, count int) bool { + refs := func(ns, name string) int { store.lock.Lock() defer store.lock.Unlock() item, ok := store.items[objectKey{ns, name}] if !ok { - return count == 0 + return 0 } - return item.refCount == count + return item.refCount } - assert.True(t, verify("ns1", "s1", 3)) - assert.True(t, verify("ns1", "s10", 1)) - assert.True(t, verify("ns1", "s2", 3)) - assert.True(t, verify("ns1", "s3", 3)) - assert.True(t, verify("ns1", "s30", 2)) - assert.True(t, verify("ns1", "s4", 2)) - assert.True(t, verify("ns1", "s5", 4)) - assert.True(t, verify("ns1", "s50", 2)) - assert.True(t, verify("ns1", "s6", 0)) - assert.True(t, verify("ns1", "s60", 0)) - assert.True(t, verify("ns1", "s7", 0)) + assert.Equal(t, 3, refs("ns1", "s1")) + assert.Equal(t, 1, refs("ns1", "s10")) + assert.Equal(t, 3, refs("ns1", "s2")) + assert.Equal(t, 3, refs("ns1", "s3")) + assert.Equal(t, 2, refs("ns1", "s30")) + assert.Equal(t, 2, refs("ns1", "s4")) + assert.Equal(t, 4, refs("ns1", "s5")) + assert.Equal(t, 2, refs("ns1", "s50")) + assert.Equal(t, 0, refs("ns1", "s6")) + assert.Equal(t, 0, refs("ns1", "s60")) + assert.Equal(t, 1, refs("ns1", "s7")) + assert.Equal(t, 1, refs("ns1", "s70")) } func TestCachingSecretManager(t *testing.T) {