From fed11c0793f9103123b441841688f3ccf983bf4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Mon, 30 Oct 2023 23:49:21 +0100 Subject: [PATCH 1/2] remove unnecessary gating of taint-eviction-controller descriptor --- .../app/controllermanager.go | 6 +- .../app/controllermanager_test.go | 60 ++++++++++++++++--- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/cmd/kube-controller-manager/app/controllermanager.go b/cmd/kube-controller-manager/app/controllermanager.go index cdcc1776762..bc0121d4149 100644 --- a/cmd/kube-controller-manager/app/controllermanager.go +++ b/cmd/kube-controller-manager/app/controllermanager.go @@ -75,7 +75,6 @@ import ( "k8s.io/kubernetes/cmd/kube-controller-manager/names" kubectrlmgrconfig "k8s.io/kubernetes/pkg/controller/apis/config" serviceaccountcontroller "k8s.io/kubernetes/pkg/controller/serviceaccount" - "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/serviceaccount" ) @@ -557,10 +556,7 @@ func NewControllerDescriptors() map[string]*ControllerDescriptor { register(newResourceClaimControllerDescriptor()) register(newLegacyServiceAccountTokenCleanerControllerDescriptor()) register(newValidatingAdmissionPolicyStatusControllerDescriptor()) - if utilfeature.DefaultFeatureGate.Enabled(features.SeparateTaintEvictionController) { - // register the flag only if the SeparateTaintEvictionController flag is enabled - register(newTaintEvictionControllerDescriptor()) - } + register(newTaintEvictionControllerDescriptor()) for _, alias := range aliases.UnsortedList() { if _, ok := controllers[alias]; ok { diff --git a/cmd/kube-controller-manager/app/controllermanager_test.go b/cmd/kube-controller-manager/app/controllermanager_test.go index ceea94ae592..d3adbebb88d 100644 --- a/cmd/kube-controller-manager/app/controllermanager_test.go +++ b/cmd/kube-controller-manager/app/controllermanager_test.go @@ -17,6 +17,7 @@ limitations under the License. package app import ( + "context" "regexp" "strings" "testing" @@ -28,9 +29,10 @@ import ( cpnames "k8s.io/cloud-provider/names" "k8s.io/component-base/featuregate" featuregatetesting "k8s.io/component-base/featuregate/testing" + controllermanagercontroller "k8s.io/controller-manager/controller" + "k8s.io/klog/v2/ktesting" "k8s.io/kubernetes/cmd/kube-controller-manager/names" "k8s.io/kubernetes/pkg/features" - "k8s.io/utils/strings/slices" ) func TestControllerNamesConsistency(t *testing.T) { @@ -159,16 +161,56 @@ func TestFeatureGatedControllersShouldNotDefineAliases(t *testing.T) { } } -// TestTaintEvictionControllerDeclaration ensures that it is possible to run taint-manager as a separated controller +// TestTaintEvictionControllerGating ensures that it is possible to run taint-manager as a separated controller // only when the SeparateTaintEvictionController feature is enabled -func TestTaintEvictionControllerDeclaration(t *testing.T) { - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SeparateTaintEvictionController, true)() - if !slices.Contains(KnownControllers(), names.TaintEvictionController) { - t.Errorf("TaintEvictionController should be a registered controller when the SeparateTaintEvictionController feature is enabled") +func TestTaintEvictionControllerGating(t *testing.T) { + tests := []struct { + name string + enableFeatureGate bool + expectInitFuncCall bool + }{ + { + name: "standalone taint-eviction-controller should run when SeparateTaintEvictionController feature gate is enabled", + enableFeatureGate: true, + expectInitFuncCall: true, + }, + { + name: "standalone taint-eviction-controller should not run when SeparateTaintEvictionController feature gate is not enabled", + enableFeatureGate: false, + expectInitFuncCall: false, + }, } - defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SeparateTaintEvictionController, false)() - if slices.Contains(KnownControllers(), names.TaintEvictionController) { - t.Errorf("TaintEvictionController should not be a registered controller when the SeparateTaintEvictionController feature is disabled") + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SeparateTaintEvictionController, test.enableFeatureGate)() + _, ctx := ktesting.NewTestContext(t) + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + controllerCtx := ControllerContext{} + controllerCtx.ComponentConfig.Generic.Controllers = []string{names.TaintEvictionController} + + initFuncCalled := false + + taintEvictionControllerDescriptor := NewControllerDescriptors()[names.TaintEvictionController] + taintEvictionControllerDescriptor.initFunc = func(ctx context.Context, controllerContext ControllerContext, controllerName string) (controller controllermanagercontroller.Interface, enabled bool, err error) { + initFuncCalled = true + return nil, true, nil + } + + healthCheck, err := StartController(ctx, controllerCtx, taintEvictionControllerDescriptor, nil) + if err != nil { + t.Errorf("starting a TaintEvictionController controller should not return an error") + } + if test.expectInitFuncCall != initFuncCalled { + t.Errorf("TaintEvictionController init call check failed: expected=%v, got=%v", test.expectInitFuncCall, initFuncCalled) + } + hasHealthCheck := healthCheck != nil + expectHealthCheck := test.expectInitFuncCall + if expectHealthCheck != hasHealthCheck { + t.Errorf("TaintEvictionController healthCheck check failed: expected=%v, got=%v", expectHealthCheck, hasHealthCheck) + } + }) } } From 1daf1b0705fd2dabba59f3c24fe0ee363bb5c6f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Mon, 30 Oct 2023 14:35:02 +0100 Subject: [PATCH 2/2] test that controller descriptors should not be feature gated controllers enabled by default should define feature gates in ControllerDescriptor.requiredFeatureGates and not during a descriptor registration in NewControllerDescriptors --- cmd/kube-controller-manager/app/controllermanager_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/kube-controller-manager/app/controllermanager_test.go b/cmd/kube-controller-manager/app/controllermanager_test.go index d3adbebb88d..efde618dc1c 100644 --- a/cmd/kube-controller-manager/app/controllermanager_test.go +++ b/cmd/kube-controller-manager/app/controllermanager_test.go @@ -107,6 +107,9 @@ func TestNewControllerDescriptorsShouldNotPanic(t *testing.T) { } func TestNewControllerDescriptorsAlwaysReturnsDescriptorsForAllControllers(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, "AllAlpha", false)() + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, "AllBeta", false)() + controllersWithoutFeatureGates := KnownControllers() defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, "AllAlpha", true)()