Merge pull request #1225 from rphillips/backports/108756

Bug 1999325: Backport 107821 and 107831

pkg/kubelet/kubelet.go

@@ -1677,6 +1677,16 @@ func (kl *Kubelet) syncPod(ctx context.Context, updateType kubetypes.SyncPodType
 		return false, fmt.Errorf("%s: %v", NetworkNotReadyErrorMsg, err)
 	}
 
+	// ensure the kubelet knows about referenced secrets or configmaps used by the pod
+	if !kl.podWorkers.IsPodTerminationRequested(pod.UID) {
+		if kl.secretManager != nil {
+			kl.secretManager.RegisterPod(pod)
+		}
+		if kl.configMapManager != nil {
+			kl.configMapManager.RegisterPod(pod)
+		}
+	}
+
 	// Create Cgroups for the pod and apply resource parameters
 	// to them if cgroups-per-qos flag is enabled.
 	pcm := kl.containerManager.NewPodContainerManager()
@@ -1895,6 +1905,14 @@ func (kl *Kubelet) syncTerminatedPod(ctx context.Context, pod *v1.Pod, podStatus
 	}
 	klog.V(4).InfoS("Pod termination unmounted volumes", "pod", klog.KObj(pod), "podUID", pod.UID)
 
+	// After volume unmount is complete, let the secret and configmap managers know we're done with this pod
+	if kl.secretManager != nil {
+		kl.secretManager.UnregisterPod(pod)
+	}
+	if kl.configMapManager != nil {
+		kl.configMapManager.UnregisterPod(pod)
+	}
+
 	// Note: we leave pod containers to be reclaimed in the background since dockershim requires the
 	// container for retrieving logs and we want to make sure logs are available until the pod is
 	// physically deleted.

pkg/kubelet/pod/pod_manager.go

@@ -159,10 +159,6 @@ func (pm *basicManager) UpdatePod(pod *v1.Pod) {
 	pm.updatePodsInternal(pod)
 }
 
-func isPodInTerminatedState(pod *v1.Pod) bool {
-	return pod.Status.Phase == v1.PodFailed || pod.Status.Phase == v1.PodSucceeded
-}
-
 // updateMetrics updates the metrics surfaced by the pod manager.
 // oldPod or newPod may be nil to signify creation or deletion.
 func updateMetrics(oldPod, newPod *v1.Pod) {
@@ -187,32 +183,6 @@ func updateMetrics(oldPod, newPod *v1.Pod) {
 // lock.
 func (pm *basicManager) updatePodsInternal(pods ...*v1.Pod) {
 	for _, pod := range pods {
-		if pm.secretManager != nil {
-			if isPodInTerminatedState(pod) {
-				// Pods that are in terminated state and no longer running can be
-				// ignored as they no longer require access to secrets.
-				// It is especially important in watch-based manager, to avoid
-				// unnecessary watches for terminated pods waiting for GC.
-				pm.secretManager.UnregisterPod(pod)
-			} else {
-				// TODO: Consider detecting only status update and in such case do
-				// not register pod, as it doesn't really matter.
-				pm.secretManager.RegisterPod(pod)
-			}
-		}
-		if pm.configMapManager != nil {
-			if isPodInTerminatedState(pod) {
-				// Pods that are in terminated state and no longer running can be
-				// ignored as they no longer require access to configmaps.
-				// It is especially important in watch-based manager, to avoid
-				// unnecessary watches for terminated pods waiting for GC.
-				pm.configMapManager.UnregisterPod(pod)
-			} else {
-				// TODO: Consider detecting only status update and in such case do
-				// not register pod, as it doesn't really matter.
-				pm.configMapManager.RegisterPod(pod)
-			}
-		}
 		podFullName := kubecontainer.GetPodFullName(pod)
 		// This logic relies on a static pod and its mirror to have the same name.
 		// It is safe to type convert here due to the IsMirrorPod guard.
@@ -239,12 +209,6 @@ func (pm *basicManager) DeletePod(pod *v1.Pod) {
 	updateMetrics(pod, nil)
 	pm.lock.Lock()
 	defer pm.lock.Unlock()
-	if pm.secretManager != nil {
-		pm.secretManager.UnregisterPod(pod)
-	}
-	if pm.configMapManager != nil {
-		pm.configMapManager.UnregisterPod(pod)
-	}
 	podFullName := kubecontainer.GetPodFullName(pod)
 	// It is safe to type convert here due to the IsMirrorPod guard.
 	if kubetypes.IsMirrorPod(pod) {

pkg/kubelet/util/manager/cache_based_manager.go

@@ -29,6 +29,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/utils/clock"
 )
@@ -42,6 +43,7 @@ type GetObjectFunc func(string, string, metav1.GetOptions) (runtime.Object, erro
 type objectKey struct {
 	namespace string
 	name      string
+	uid       types.UID
 }
 
 // objectStoreItems is a single item stored in objectStore.
@@ -226,7 +228,7 @@ func (c *cacheBasedManager) RegisterPod(pod *v1.Pod) {
 		c.objectStore.AddReference(pod.Namespace, name)
 	}
 	var prev *v1.Pod
-	key := objectKey{namespace: pod.Namespace, name: pod.Name}
+	key := objectKey{namespace: pod.Namespace, name: pod.Name, uid: pod.UID}
 	prev = c.registeredPods[key]
 	c.registeredPods[key] = pod
 	if prev != nil {
@@ -243,7 +245,7 @@ func (c *cacheBasedManager) RegisterPod(pod *v1.Pod) {
 
 func (c *cacheBasedManager) UnregisterPod(pod *v1.Pod) {
 	var prev *v1.Pod
-	key := objectKey{namespace: pod.Namespace, name: pod.Name}
+	key := objectKey{namespace: pod.Namespace, name: pod.Name, uid: pod.UID}
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	prev = c.registeredPods[key]

pkg/kubelet/util/manager/cache_based_manager_test.go

@@ -30,6 +30,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	clientset "k8s.io/client-go/kubernetes"
@@ -335,10 +336,15 @@ type secretsToAttach struct {
 }
 
 func podWithSecrets(ns, podName string, toAttach secretsToAttach) *v1.Pod {
+	return podWithSecretsAndUID(ns, podName, "", toAttach)
+}
+
+func podWithSecretsAndUID(ns, podName, podUID string, toAttach secretsToAttach) *v1.Pod {
 	pod := &v1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: ns,
 			Name:      podName,
+			UID:       types.UID(podUID),
 		},
 		Spec: v1.PodSpec{},
 	}
@@ -444,7 +450,7 @@ func TestRegisterIdempotence(t *testing.T) {
 	refs := func(ns, name string) int {
 		store.lock.Lock()
 		defer store.lock.Unlock()
-		item, ok := store.items[objectKey{ns, name}]
+		item, ok := store.items[objectKey{namespace: ns, name: name}]
 		if !ok {
 			return 0
 		}
@@ -531,7 +537,7 @@ func TestCacheRefcounts(t *testing.T) {
 	refs := func(ns, name string) int {
 		store.lock.Lock()
 		defer store.lock.Unlock()
-		item, ok := store.items[objectKey{ns, name}]
+		item, ok := store.items[objectKey{namespace: ns, name: name}]
 		if !ok {
 			return 0
 		}
@@ -549,6 +555,42 @@ func TestCacheRefcounts(t *testing.T) {
 	assert.Equal(t, 0, refs("ns1", "s60"))
 	assert.Equal(t, 1, refs("ns1", "s7"))
 	assert.Equal(t, 1, refs("ns1", "s70"))
+
+	// Check the interleaved registerpod/unregisterpod with identical names and different uids scenario
+	secret1 := secretsToAttach{
+		containerEnvSecrets: []envSecrets{
+			{envVarNames: []string{"secret1"}},
+		},
+	}
+	secret2 := secretsToAttach{
+		containerEnvSecrets: []envSecrets{
+			{envVarNames: []string{"secret2"}},
+		},
+	}
+
+	// precondition: no references
+	assert.Equal(t, 0, refs("nsinterleaved", "secret1"))
+	assert.Equal(t, 0, refs("nsinterleaved", "secret2"))
+
+	// add first pod that references secret1 only
+	manager.RegisterPod(podWithSecretsAndUID("nsinterleaved", "pod", "poduid1", secret1))
+	assert.Equal(t, 1, refs("nsinterleaved", "secret1"))
+	assert.Equal(t, 0, refs("nsinterleaved", "secret2"))
+
+	// add second pod that references secret2 only, retain references to secret1
+	manager.RegisterPod(podWithSecretsAndUID("nsinterleaved", "pod", "poduid2", secret2))
+	assert.Equal(t, 1, refs("nsinterleaved", "secret1"))
+	assert.Equal(t, 1, refs("nsinterleaved", "secret2"))
+
+	// remove first pod that references secret1, retain references to secret2
+	manager.UnregisterPod(podWithSecretsAndUID("nsinterleaved", "pod", "poduid1", secretsToAttach{}))
+	assert.Equal(t, 0, refs("nsinterleaved", "secret1"))
+	assert.Equal(t, 1, refs("nsinterleaved", "secret2"))
+
+	// remove second pod that references secret2
+	manager.UnregisterPod(podWithSecretsAndUID("nsinterleaved", "pod", "poduid2", secretsToAttach{}))
+	assert.Equal(t, 0, refs("nsinterleaved", "secret1"))
+	assert.Equal(t, 0, refs("nsinterleaved", "secret2"))
 }
 
 func TestCacheBasedSecretManager(t *testing.T) {