From 0939f9010381fb5ce56ee0543109c2554681dacb Mon Sep 17 00:00:00 2001 From: Ted Yu Date: Tue, 1 Oct 2019 11:12:18 -0700 Subject: [PATCH] Check whether mirror pod is ciritical in managerImpl#evictPod --- pkg/kubelet/eviction/eviction_manager.go | 16 +++----------- pkg/kubelet/eviction/eviction_manager_test.go | 5 ----- pkg/kubelet/kubelet.go | 6 +++--- pkg/kubelet/pod/mirror_client.go | 6 ------ pkg/kubelet/pod/pod_manager.go | 4 ++-- pkg/kubelet/status/status_manager.go | 2 +- pkg/kubelet/status/status_manager_test.go | 4 ++-- pkg/kubelet/types/pod_update.go | 21 +++++++++++++------ test/e2e/framework/pod/BUILD | 2 +- test/e2e/framework/pod/resource.go | 4 ++-- 10 files changed, 29 insertions(+), 41 deletions(-) diff --git a/pkg/kubelet/eviction/eviction_manager.go b/pkg/kubelet/eviction/eviction_manager.go index 4ef2a89dce6..2a7ee0c88bd 100644 --- a/pkg/kubelet/eviction/eviction_manager.go +++ b/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) { - // need mirrorPod to check its "priority" value; static pod doesn't carry it - if mirrorPod, ok := m.mirrorPodFunc(pod); ok && mirrorPod != nil { - // 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 - } + if kubelettypes.IsCriticalPod(pod) { + klog.Errorf("eviction manager: cannot evict a critical pod %s", format.Pod(pod)) + return false } status := v1.PodStatus{ Phase: v1.PodFailed, diff --git a/pkg/kubelet/eviction/eviction_manager_test.go b/pkg/kubelet/eviction/eviction_manager_test.go index 38c2cba983d..fa407d8679a 100644 --- a/pkg/kubelet/eviction/eviction_manager_test.go +++ b/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 diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 994e502b794..875860c2156 100644 --- a/pkg/kubelet/kubelet.go +++ b/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 } diff --git a/pkg/kubelet/pod/mirror_client.go b/pkg/kubelet/pod/mirror_client.go index 5078bc808db..fcc2e1dbbbd 100644 --- a/pkg/kubelet/pod/mirror_client.go +++ b/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 diff --git a/pkg/kubelet/pod/pod_manager.go b/pkg/kubelet/pod/pod_manager.go index 7e3ef5edd4a..23f4e8258b3 100644 --- a/pkg/kubelet/pod/pod_manager.go +++ b/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) diff --git a/pkg/kubelet/status/status_manager.go b/pkg/kubelet/status/status_manager.go index 92b24ec3f52..96252cd4230 100644 --- a/pkg/kubelet/status/status_manager.go +++ b/pkg/kubelet/status/status_manager.go @@ -580,7 +580,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) diff --git a/pkg/kubelet/status/status_manager_test.go b/pkg/kubelet/status/status_manager_test.go index a616b1368e2..27468352981 100644 --- a/pkg/kubelet/status/status_manager_test.go +++ b/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() diff --git a/pkg/kubelet/types/pod_update.go b/pkg/kubelet/types/pod_update.go index 9c3d676cd09..d3c06c8e755 100644 --- a/pkg/kubelet/types/pod_update.go +++ b/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 -} diff --git a/test/e2e/framework/pod/BUILD b/test/e2e/framework/pod/BUILD index 156ae8c3fcf..7995c90907a 100644 --- a/test/e2e/framework/pod/BUILD +++ b/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", diff --git a/test/e2e/framework/pod/resource.go b/test/e2e/framework/pod/resource.go index 805ae3568ae..0fb4d8eb5f5 100644 --- a/test/e2e/framework/pod/resource.go +++ b/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