Merge pull request #83325 from yutedz/static-mirror-pod

Check whether mirror pod is ciritical in managerImpl#evictPod

pkg/kubelet/eviction/eviction_manager.go

@@ -547,19 +547,9 @@ func (m *managerImpl) evictPod(pod *v1.Pod, gracePeriodOverride int64, evictMsg
 	// If the pod is marked as critical and static, and support for critical pod annotations is enabled,
 	// do not evict such pods. Static pods are not re-admitted after evictions.
 	// https://github.com/kubernetes/kubernetes/issues/40573 has more details.
-	if kubelettypes.IsStaticPod(pod) {
+	if kubelettypes.IsCriticalPod(pod) {
-		// need mirrorPod to check its "priority" value; static pod doesn't carry it
+		klog.Errorf("eviction manager: cannot evict a critical pod %s", format.Pod(pod))
-		if mirrorPod, ok := m.mirrorPodFunc(pod); ok && mirrorPod != nil {
+		return false
-			// skip only when it's a static and critical pod
-			if kubelettypes.IsCriticalPod(mirrorPod) {
-				klog.Errorf("eviction manager: cannot evict a critical static pod %s", format.Pod(pod))
-				return false
-			}
-		} else {
-			// we should never hit this
-			klog.Errorf("eviction manager: cannot get mirror pod from static pod %s, so cannot evict it", format.Pod(pod))
-			return false
-		}
 	}
 	status := v1.PodStatus{
 		Phase:   v1.PodFailed,

pkg/kubelet/eviction/eviction_manager_test.go

@@ -1267,11 +1267,6 @@ func TestStaticCriticalPodsAreNotEvicted(t *testing.T) {
 	if !manager.IsUnderMemoryPressure() {
 		t.Errorf("Manager should report memory pressure")
 	}
-
-	// check the right pod was killed
-	if podKiller.pod != podToEvict {
-		t.Errorf("Manager chose to kill pod: %v, but should have chosen %v", podKiller.pod.Name, podToEvict.Name)
-	}
 }
 
 // TestAllocatableMemoryPressure

pkg/kubelet/kubelet.go

@@ -2052,7 +2052,7 @@ func (kl *Kubelet) HandlePodAdditions(pods []*v1.Pod) {
 		// the apiserver and no action (other than cleanup) is required.
 		kl.podManager.AddPod(pod)
 
-		if kubepod.IsMirrorPod(pod) {
+		if kubetypes.IsMirrorPod(pod) {
 			kl.handleMirrorPod(pod, start)
 			continue
 		}
@@ -2087,7 +2087,7 @@ func (kl *Kubelet) HandlePodUpdates(pods []*v1.Pod) {
 	}
 	for _, pod := range pods {
 		kl.podManager.UpdatePod(pod)
-		if kubepod.IsMirrorPod(pod) {
+		if kubetypes.IsMirrorPod(pod) {
 			kl.handleMirrorPod(pod, start)
 			continue
 		}
@@ -2104,7 +2104,7 @@ func (kl *Kubelet) HandlePodRemoves(pods []*v1.Pod) {
 	start := kl.clock.Now()
 	for _, pod := range pods {
 		kl.podManager.DeletePod(pod)
-		if kubepod.IsMirrorPod(pod) {
+		if kubetypes.IsMirrorPod(pod) {
 			kl.handleMirrorPod(pod, start)
 			continue
 		}

pkg/kubelet/pod/mirror_client.go

@@ -110,12 +110,6 @@ func IsStaticPod(pod *v1.Pod) bool {
 	return err == nil && source != kubetypes.ApiserverSource
 }
 
-// IsMirrorPod returns true if the passed Pod is a Mirror Pod.
-func IsMirrorPod(pod *v1.Pod) bool {
-	_, ok := pod.Annotations[kubetypes.ConfigMirrorAnnotationKey]
-	return ok
-}
-
 func getHashFromMirrorPod(pod *v1.Pod) (string, bool) {
 	hash, ok := pod.Annotations[kubetypes.ConfigMirrorAnnotationKey]
 	return hash, ok

pkg/kubelet/pod/pod_manager.go

@@ -206,7 +206,7 @@ func (pm *basicManager) updatePodsInternal(pods ...*v1.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.
-		if IsMirrorPod(pod) {
+		if kubetypes.IsMirrorPod(pod) {
 			mirrorPodUID := kubetypes.MirrorPodUID(pod.UID)
 			pm.mirrorPodByUID[mirrorPodUID] = pod
 			pm.mirrorPodByFullName[podFullName] = pod
@@ -235,7 +235,7 @@ func (pm *basicManager) DeletePod(pod *v1.Pod) {
 	}
 	podFullName := kubecontainer.GetPodFullName(pod)
 	// It is safe to type convert here due to the IsMirrorPod guard.
-	if IsMirrorPod(pod) {
+	if kubetypes.IsMirrorPod(pod) {
 		mirrorPodUID := kubetypes.MirrorPodUID(pod.UID)
 		delete(pm.mirrorPodByUID, mirrorPodUID)
 		delete(pm.mirrorPodByFullName, podFullName)

pkg/kubelet/status/status_manager.go

@@ -581,7 +581,7 @@ func (m *manager) needsUpdate(uid types.UID, status versionedPodStatus) bool {
 }
 
 func (m *manager) canBeDeleted(pod *v1.Pod, status v1.PodStatus) bool {
-	if pod.DeletionTimestamp == nil || kubepod.IsMirrorPod(pod) {
+	if pod.DeletionTimestamp == nil || kubetypes.IsMirrorPod(pod) {
 		return false
 	}
 	return m.podDeletionSafety.PodResourcesAreReclaimed(pod, status)

pkg/kubelet/status/status_manager_test.go

@@ -537,7 +537,7 @@ func TestStaticPod(t *testing.T) {
 
 	t.Logf("Create the mirror pod")
 	m.podManager.AddPod(mirrorPod)
-	assert.True(t, kubepod.IsMirrorPod(mirrorPod), "SetUp error: mirrorPod")
+	assert.True(t, kubetypes.IsMirrorPod(mirrorPod), "SetUp error: mirrorPod")
 	assert.Equal(t, m.podManager.TranslatePodUID(mirrorPod.UID), kubetypes.ResolvedPodUID(staticPod.UID))
 
 	t.Logf("Should be able to get the mirror pod status from status manager")
@@ -890,7 +890,7 @@ func TestDoNotDeleteMirrorPods(t *testing.T) {
 	m.podManager.AddPod(mirrorPod)
 	t.Logf("Verify setup.")
 	assert.True(t, kubetypes.IsStaticPod(staticPod), "SetUp error: staticPod")
-	assert.True(t, kubepod.IsMirrorPod(mirrorPod), "SetUp error: mirrorPod")
+	assert.True(t, kubetypes.IsMirrorPod(mirrorPod), "SetUp error: mirrorPod")
 	assert.Equal(t, m.podManager.TranslatePodUID(mirrorPod.UID), kubetypes.ResolvedPodUID(staticPod.UID))
 
 	status := getRandomPodStatus()

pkg/kubelet/types/pod_update.go

@@ -137,11 +137,26 @@ func (sp SyncPodType) String() string {
 	}
 }
 
+// IsMirrorPod returns true if the passed Pod is a Mirror Pod.
+func IsMirrorPod(pod *v1.Pod) bool {
+	_, ok := pod.Annotations[ConfigMirrorAnnotationKey]
+	return ok
+}
+
+// IsStaticPod returns true if the pod is a static pod.
+func IsStaticPod(pod *v1.Pod) bool {
+	source, err := GetPodSource(pod)
+	return err == nil && source != ApiserverSource
+}
+
 // IsCriticalPod returns true if pod's priority is greater than or equal to SystemCriticalPriority.
 func IsCriticalPod(pod *v1.Pod) bool {
 	if IsStaticPod(pod) {
 		return true
 	}
+	if IsMirrorPod(pod) {
+		return true
+	}
 	if pod.Spec.Priority != nil && IsCriticalPodBasedOnPriority(*pod.Spec.Priority) {
 		return true
 	}
@@ -166,9 +181,3 @@ func Preemptable(preemptor, preemptee *v1.Pod) bool {
 func IsCriticalPodBasedOnPriority(priority int32) bool {
 	return priority >= scheduling.SystemCriticalPriority
 }
-
-// IsStaticPod returns true if the pod is a static pod.
-func IsStaticPod(pod *v1.Pod) bool {
-	source, err := GetPodSource(pod)
-	return err == nil && source != ApiserverSource
-}

test/e2e/framework/pod/BUILD

@@ -16,7 +16,7 @@ go_library(
         "//pkg/api/v1/pod:go_default_library",
         "//pkg/client/conditions:go_default_library",
         "//pkg/controller:go_default_library",
-        "//pkg/kubelet/pod:go_default_library",
+        "//pkg/kubelet/types:go_default_library",
         "//pkg/kubelet/util/format:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",

test/e2e/framework/pod/resource.go

@@ -35,7 +35,7 @@ import (
 	clientset "k8s.io/client-go/kubernetes"
 	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 	"k8s.io/kubernetes/pkg/client/conditions"
-	kubepod "k8s.io/kubernetes/pkg/kubelet/pod"
+	kubetypes "k8s.io/kubernetes/pkg/kubelet/types"
 	e2elog "k8s.io/kubernetes/test/e2e/framework/log"
 	testutils "k8s.io/kubernetes/test/utils"
 	imageutils "k8s.io/kubernetes/test/utils/image"
@@ -434,7 +434,7 @@ func FilterNonRestartablePods(pods []*v1.Pod) []*v1.Pod {
 }
 
 func isNotRestartAlwaysMirrorPod(p *v1.Pod) bool {
-	if !kubepod.IsMirrorPod(p) {
+	if !kubetypes.IsMirrorPod(p) {
 		return false
 	}
 	return p.Spec.RestartPolicy != v1.RestartPolicyAlways