Merge pull request #66082 from sjenning/fix-is-critical-checks

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

move feature gate checks inside IsCriticalPod

Currently `IsCriticalPod()` calls throughout the code are protected by `utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation)`.

However, with Pod Priority, this gate could be disabled which skips the priority check inside IsCriticalPod().

This PR moves the feature gate checking inside `IsCriticalPod()` and handles both situations properly.

@aveshagarwal @ravisantoshgudimetla @derekwaynecarr 
/sig node
/sig scheduling
/king bug

pkg/controller/daemon/daemon_controller.go

@@ -1283,8 +1283,7 @@ func (dsc *DaemonSetsController) simulate(newPod *v1.Pod, node *v1.Node, ds *app
 	})
 
 	// TODO(#48843) OutOfDisk taints will be removed in 1.10
-	if utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) &&
+	if kubelettypes.IsCriticalPod(newPod) {
-		kubelettypes.IsCriticalPod(newPod) {
 		v1helper.AddOrUpdateTolerationInPod(newPod, &v1.Toleration{
 			Key:      algorithm.TaintNodeOutOfDisk,
 			Operator: v1.TolerationOpExists,
@@ -1466,8 +1465,7 @@ func Predicates(pod *v1.Pod, nodeInfo *schedulercache.NodeInfo) (bool, []algorit
 		return len(predicateFails) == 0, predicateFails, nil
 	}
 
-	critical := utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) &&
+	critical := kubelettypes.IsCriticalPod(pod)
-		kubelettypes.IsCriticalPod(pod)
 
 	fit, reasons, err := predicates.PodToleratesNodeTaints(pod, nil, nodeInfo)
 	if err != nil {

pkg/kubelet/eviction/eviction_manager.go

@@ -131,7 +131,7 @@ func (m *managerImpl) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAd
 	}
 	// Admit Critical pods even under resource pressure since they are required for system stability.
 	// https://github.com/kubernetes/kubernetes/issues/40573 has more details.
-	if utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) && kubelettypes.IsCriticalPod(attrs.Pod) {
+	if kubelettypes.IsCriticalPod(attrs.Pod) {
 		return lifecycle.PodAdmitResult{Admit: true}
 	}
 	// the node has memory pressure, admit if not best-effort
@@ -544,8 +544,7 @@ 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 utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) &&
+	if kubelettypes.IsCriticalPod(pod) && kubepod.IsStaticPod(pod) {
-		kubelettypes.IsCriticalPod(pod) && kubepod.IsStaticPod(pod) {
 		glog.Errorf("eviction manager: cannot evict a critical static pod %s", format.Pod(pod))
 		return false
 	}

pkg/kubelet/preemption/BUILD

@@ -13,7 +13,6 @@ go_library(
     deps = [
         "//pkg/api/v1/resource:go_default_library",
         "//pkg/apis/core/v1/helper/qos:go_default_library",
-        "//pkg/features:go_default_library",
         "//pkg/kubelet/events:go_default_library",
         "//pkg/kubelet/eviction:go_default_library",
         "//pkg/kubelet/lifecycle:go_default_library",
@@ -22,7 +21,6 @@ go_library(
         "//pkg/scheduler/algorithm:go_default_library",
         "//pkg/scheduler/algorithm/predicates:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
-        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
         "//staging/src/k8s.io/client-go/tools/record:go_default_library",
         "//vendor/github.com/golang/glog:go_default_library",
     ],
@@ -51,6 +49,7 @@ go_test(
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
         "//staging/src/k8s.io/client-go/tools/record:go_default_library",
     ],
 )

pkg/kubelet/preemption/preemption.go

@@ -22,11 +22,9 @@ import (
 
 	"github.com/golang/glog"
 	"k8s.io/api/core/v1"
-	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/kubernetes/pkg/api/v1/resource"
 	v1qos "k8s.io/kubernetes/pkg/apis/core/v1/helper/qos"
-	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/kubelet/events"
 	"k8s.io/kubernetes/pkg/kubelet/eviction"
 	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
@@ -64,7 +62,7 @@ func NewCriticalPodAdmissionHandler(getPodsFunc eviction.ActivePodsFunc, killPod
 // HandleAdmissionFailure gracefully handles admission rejection, and, in some cases,
 // to allow admission of the pod despite its previous failure.
 func (c *CriticalPodAdmissionHandler) HandleAdmissionFailure(pod *v1.Pod, failureReasons []algorithm.PredicateFailureReason) (bool, []algorithm.PredicateFailureReason, error) {
-	if !kubetypes.IsCriticalPod(pod) || !utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) {
+	if !kubetypes.IsCriticalPod(pod) {
 		return false, failureReasons, nil
 	}
 	// InsufficientResourceError is not a reason to reject a critical pod.

pkg/kubelet/preemption/preemption_test.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/record"
 	kubeapi "k8s.io/kubernetes/pkg/apis/core"
 	kubetypes "k8s.io/kubernetes/pkg/kubelet/types"
@@ -85,6 +86,9 @@ func getTestCriticalPodAdmissionHandler(podProvider *fakePodProvider, podKiller
 }
 
 func TestEvictPodsToFreeRequests(t *testing.T) {
+	if err := utilfeature.DefaultFeatureGate.Set("ExperimentalCriticalPodAnnotation=true"); err != nil {
+		t.Errorf("failed to set ExperimentalCriticalPodAnnotation to true: %v", err)
+	}
 	type testRun struct {
 		testName              string
 		inputPods             []*v1.Pod

pkg/kubelet/types/BUILD

@@ -20,9 +20,11 @@ go_library(
     deps = [
         "//pkg/apis/core:go_default_library",
         "//pkg/apis/scheduling:go_default_library",
+        "//pkg/features:go_default_library",
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
     ],
 )
 
@@ -38,6 +40,7 @@ go_test(
     deps = [
         "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
         "//vendor/github.com/stretchr/testify/require:go_default_library",
     ],

pkg/kubelet/types/pod_update.go

@@ -21,8 +21,10 @@ import (
 
 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	kubeapi "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/scheduling"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 const (
@@ -144,7 +146,17 @@ func (sp SyncPodType) String() string {
 // or equal to SystemCriticalPriority. Both the rescheduler(deprecated in 1.10) and the kubelet use this function
 // to make admission and scheduling decisions.
 func IsCriticalPod(pod *v1.Pod) bool {
-	return IsCritical(pod.Namespace, pod.Annotations) || (pod.Spec.Priority != nil && IsCriticalPodBasedOnPriority(*pod.Spec.Priority))
+	if utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
+		if pod.Spec.Priority != nil && IsCriticalPodBasedOnPriority(*pod.Spec.Priority) {
+			return true
+		}
+	}
+	if utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) {
+		if IsCritical(pod.Namespace, pod.Annotations) {
+			return true
+		}
+	}
+	return false
 }
 
 // IsCritical returns true if parameters bear the critical pod annotation

pkg/kubelet/types/pod_update_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 )
 
 func TestGetValidatedSources(t *testing.T) {
@@ -115,6 +116,9 @@ func TestString(t *testing.T) {
 }
 
 func TestIsCriticalPod(t *testing.T) {
+	if err := utilfeature.DefaultFeatureGate.Set("ExperimentalCriticalPodAnnotation=true"); err != nil {
+		t.Errorf("failed to set ExperimentalCriticalPodAnnotation to true: %v", err)
+	}
 	cases := []struct {
 		pod      v1.Pod
 		expected bool