Merge pull request #42647 from gmarek/nc-flag

Automatic merge from submit-queue (batch tested with PRs 42705, 42647)

Add taint-based evictions to feature-gates

This is not a PR, it's a repro, so please treat is as an issue:)

I was trying to move from dedicated flag to feature flag for Taint eviction feature and failed, as I'm unable to pass feature flag in the commandline. PR I tried is "included", and the results are following:
```
I0307 14:55:14.288374       5 feature_gate.go:174] feature gates: map[TaintBasedEvictions:true AffinityInAnnotations:true]
I0307 14:55:14.288479       5 feature_gate.go:174] feature gates: map[ExperimentalCriticalPodAnnotation:true]
...
I0307 14:55:37.358358       5 controllermanager.go:445] TaintBasedEvictions feature enabled: false
I0307 14:55:37.358368       5 controllermanager.go:446] AffinityInAnnotations feature enabled: false
```

As it can be seen unlike all other "feature gate" tests, I didn't write dedicated unit test that manipulates gates by hand, but added `--feature-gates` flag to the controller-manager command line.

I added 'AffinityInAnnotations' as a sanity check and it failed as well. So it seems to me that something is not working.

cc @sttts @jlowdermilk @timstclair @davidopp @thockin

cmd/kube-controller-manager/app/controllermanager.go

@@ -36,7 +36,10 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
+
 	"k8s.io/apiserver/pkg/server/healthz"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+
 	"k8s.io/client-go/discovery"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
 	clientv1 "k8s.io/client-go/pkg/api/v1"
@@ -44,6 +47,7 @@ import (
 	"k8s.io/client-go/tools/clientcmd"
 	"k8s.io/client-go/tools/record"
 	certutil "k8s.io/client-go/util/cert"
+
 	"k8s.io/kubernetes/cmd/kube-controller-manager/app/options"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
@@ -58,6 +62,7 @@ import (
 	serviceaccountcontroller "k8s.io/kubernetes/pkg/controller/serviceaccount"
 	"k8s.io/kubernetes/pkg/controller/volume/attachdetach"
 	persistentvolumecontroller "k8s.io/kubernetes/pkg/controller/volume/persistentvolume"
+	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/serviceaccount"
 	"k8s.io/kubernetes/pkg/util/configz"
 
@@ -466,14 +471,13 @@ func StartControllers(controllers map[string]InitFunc, s *options.CMServer, root
 			int(s.NodeCIDRMaskSize),
 			s.AllocateNodeCIDRs,
 			s.EnableTaintManager,
-			s.UseTaintBasedEvictions,
+			utilfeature.DefaultFeatureGate.Enabled(features.TaintBasedEvictions),
 		)
 		if err != nil {
 			return fmt.Errorf("failed to initialize nodecontroller: %v", err)
 		}
 		nodeController.Run()
 		time.Sleep(wait.Jitter(s.ControllerStartInterval.Duration, ControllerStartJitter))
-
 	} else {
 		glog.Warningf("%q is disabled", nodeControllerName)
 	}

cmd/kube-controller-manager/app/options/options.go

@@ -105,7 +105,6 @@ func NewCMServer() *CMServer {
 			ClusterSigningKeyFile:                 "/etc/kubernetes/ca/ca.key",
 			ReconcilerSyncLoopPeriod:              metav1.Duration{Duration: 60 * time.Second},
 			EnableTaintManager:                    true,
-			UseTaintBasedEvictions:                false,
 			HorizontalPodAutoscalerUseRESTClients: false,
 		},
 	}
@@ -203,7 +202,6 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.BoolVar(&s.DisableAttachDetachReconcilerSync, "disable-attach-detach-reconcile-sync", false, "Disable volume attach detach reconciler sync. Disabling this may cause volumes to be mismatched with pods. Use wisely.")
 	fs.DurationVar(&s.ReconcilerSyncLoopPeriod.Duration, "attach-detach-reconcile-sync-period", s.ReconcilerSyncLoopPeriod.Duration, "The reconciler sync wait time between volume attach detach. This duration must be larger than one second, and increasing this value from the default may allow for volumes to be mismatched with pods.")
 	fs.BoolVar(&s.EnableTaintManager, "enable-taint-manager", s.EnableTaintManager, "WARNING: Beta feature. If set to true enables NoExecute Taints and will evict all not-tolerating Pod running on Nodes tainted with this kind of Taints.")
-	fs.BoolVar(&s.UseTaintBasedEvictions, "use-taint-based-evictions", s.UseTaintBasedEvictions, "WARNING: Alpha feature. If set to true NodeController will use taints to evict Pods from notReady and unreachable Nodes.")
 	fs.BoolVar(&s.HorizontalPodAutoscalerUseRESTClients, "horizontal-pod-autoscaler-use-rest-clients", s.HorizontalPodAutoscalerUseRESTClients, "WARNING: alpha feature.  If set to true, causes the horizontal pod autoscaler controller to use REST clients through the kube-aggregator, instead of using the legacy metrics client through the API server proxy.  This is required for custom metrics support in the horizonal pod autoscaler.")
 
 	leaderelection.BindFlags(&s.LeaderElection, fs)

pkg/apis/componentconfig/types.go

@@ -817,8 +817,6 @@ type KubeControllerManagerConfiguration struct {
 	// If set to true enables NoExecute Taints and will evict all not-tolerating
 	// Pod running on Nodes tainted with this kind of Taints.
 	EnableTaintManager bool
-	// If set to true NodeController will use taints to evict Pods from notReady and unreachable Nodes.
-	UseTaintBasedEvictions bool
 	// HorizontalPodAutoscalerUseRESTClients causes the HPA controller to use REST clients
 	// through the kube-aggregator when enabled, instead of using the legacy metrics client
 	// through the API server proxy.

pkg/controller/node/nodecontroller.go

@@ -23,7 +23,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/golang/glog"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -32,11 +31,13 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
+
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
 	clientv1 "k8s.io/client-go/pkg/api/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/flowcontrol"
+
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
@@ -50,6 +51,8 @@ import (
 	utilnode "k8s.io/kubernetes/pkg/util/node"
 	"k8s.io/kubernetes/pkg/util/system"
 	utilversion "k8s.io/kubernetes/pkg/util/version"
+
+	"github.com/golang/glog"
 )
 
 func init() {
@@ -261,6 +264,9 @@ func NewNodeController(
 		runTaintManager:                 runTaintManager,
 		useTaintBasedEvictions:          useTaintBasedEvictions && runTaintManager,
 	}
+	if useTaintBasedEvictions {
+		glog.Infof("NodeController is using taint based evictions.")
+	}
 	nc.enterPartialDisruptionFunc = nc.ReducedQPSFunc
 	nc.enterFullDisruptionFunc = nc.HealthyQPSFunc
 	nc.computeZoneStateFunc = nc.ComputeZoneState

pkg/features/kube_features.go

@@ -81,6 +81,13 @@ const (
 	// Only Nvidia GPUs are supported as of v1.6.
 	// Works only with Docker Container Runtime.
 	Accelerators utilfeature.Feature = "Accelerators"
+
+	// owner: @gmarek
+	// alpha: v1.6
+	//
+	// Changes the logic behind evicting Pods from not ready Nodes
+	// to take advantage of NoExecute Taints and Tolerations.
+	TaintBasedEvictions utilfeature.Feature = "TaintBasedEvictions"
 )
 
 func init() {
@@ -99,6 +106,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
 	ExperimentalCriticalPodAnnotation:           {Default: false, PreRelease: utilfeature.Alpha},
 	AffinityInAnnotations:                       {Default: false, PreRelease: utilfeature.Alpha},
 	Accelerators:                                {Default: false, PreRelease: utilfeature.Alpha},
+	TaintBasedEvictions:                         {Default: false, PreRelease: utilfeature.Alpha},
 
 	// inherited features from generic apiserver, relisted here to get a conflict if it is changed
 	// unintentionally on either side: