Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

atosatto · 2023-07-10T16:42:08Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is implementing KEP-3902 by introducing a feature-flag, SeparateTaintManager, allowing to de-couple taint-manager from node-lifecycle-controller.

Which issue(s) this PR fixes:

kubernetes/enhancements#3902

Special notes for your reviewer:

Unlike the KEP proposal, the flag used to disable/enable the controller in KCM has been named taint-eviction-controller in order to preserve the existing convention of having all of the controllers names ending with -controller. This has been preferred to the taint-manager-controller and/or taint-manager alternatives. For compatibility with the KEP a taint-manager legacy flag has been provided in the implementation.

Does this PR introduce a user-facing change?

Decouple TaintManager from NodeLifeCycleController (KEP-3902)

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-07-10T16:42:18Z

Hi @atosatto. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Huang-Wei · 2023-07-10T16:43:08Z

/ok-to-test

/sig scheduling
/sig node
/sig apps

Huang-Wei · 2023-07-10T16:43:21Z

/milestone v1.28

pkg/controller/taintmanager/OWNERS

atosatto · 2023-07-10T19:02:42Z

/test pull-kubernetes-e2e-gce-cos-alpha-features

Huang-Wei

Thanks for the PR. The logic looks good. Could you add some integration tests to cover that:

if the feature gate is enabled, we are able to disable taintmanager from controller manager
if the feature gate is disable, there is no way to disable taintmanager (taintmanger functions by default)

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

pkg/controller/taintmanager/taint_manager.go

pkg/features/kube_features.go

pkg/controller/taintmanager/OWNERS

pkg/controller/nodelifecycle/node_lifecycle_controller.go

zelenushechka · 2023-07-11T08:23:55Z

Hey, I'm Anhelina from the Bug Triage Release team 1.28 👋. I wanted to reach out to discuss its progress and ensure it's on track for the v1.28 release, considering the Code Freeze is coming up on 01:00 UTC Wednesday 19th July 2023 / 18:00 PDT Tuesday 18th July 2023. Could you please provide an update on the current status of the issue? Is it planned for the v1.28 release? We are currently in Week 9, and Code Freeze is just over a week away. If there's anything I can do, please let me know:)

k8s-ci-robot

@atosatto: 1 invalid OWNERS file

In response to this:

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is implementing KEP-3902 by introducing a feature-flag, SeparateTaintManager, allowing to de-couple taint-manager from node-lifecycle-controller.

Which issue(s) this PR fixes:

kubernetes/enhancements#3902

Special notes for your reviewer:

Unlike the KEP proposal, the flag used to disable/enable the controller in KCM has been named taint-eviction-controller in order to preserve the existing convention of having all of the controllers names ending with -controller. This has been preferred to the taint-manager-controller and/or taint-manager alternatives. For compatibility with the KEP a taint-manager legacy flag has been provided in the implementation.

Does this PR introduce a user-facing change?
Decouple TaintManager from NodeLifeCycleController (KEP-3902)
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP - KEP-3902: Decouple TaintManager from NodeLifeCycleController enhancements#3901

Docs - [WIP] Docs for Decoupled TaintManager from NodeLifeCycleController (KEP-3902) website#41970

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/controller/taintmanager/OWNERS

atosatto · 2023-07-11T12:50:08Z

Thanks for the PR. The logic looks good. Could you add some integration tests to cover that:

if the feature gate is enabled, we are able to disable taintmanager from controller manager

if the feature gate is disable, there is no way to disable taintmanager (taintmanger functions by default)

I've added some specific tests covering the controller registration in e9b28ea8923 which should cover this from the point of view of the kube-controller-manager registered controllers. Is this the scenario you had in mind? If not I am happy to add more tests.

Huang-Wei · 2023-10-26T16:30:34Z

/retest

yuanchen8911

/lgtm

k8s-ci-robot · 2023-10-26T16:33:51Z

LGTM label has been added.

Git tree hash: f4a6fbd596ab190095e7a47e7a630f1b0e03b491

k8s-ci-robot · 2023-10-29T21:36:03Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…roller (KEP-3902)

atiratree

Sorry about the last minute changes to the controllers registration in #120371.

Feature gating approach has changed a needs to be updated. Otherwise LGTM

atiratree · 2023-10-30T12:55:51Z

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

@@ -556,6 +557,10 @@ func NewControllerDescriptors() map[string]*ControllerDescriptor {
 	register(newResourceClaimControllerDescriptor())
 	register(newLegacyServiceAccountTokenCleanerControllerDescriptor())
 	register(newValidatingAdmissionPolicyStatusControllerDescriptor())
+	if utilfeature.DefaultFeatureGate.Enabled(features.SeparateTaintEvictionController) {


with #120371 there is no need for defining the gating here anymore, since the gating should be automatic (taken from the descriptor)

@atiratree not quite, this controller is on by default, since it's beta, so this check ensures with the feature gate off it's not turned on. Not sure how to handle that with your controller initiation changes. So I think we can leave it as is, for now, and fix in a followup.

this check has been moved to

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

Lines 683 to 688 in b84e4a1

for _, featureGate := range controllerDescriptor.GetRequiredFeatureGates() {

if !utilfeature.DefaultFeatureGate.Enabled(featureGate) {

logger.Info("Controller is disabled by a feature gate", "controller", controllerName, "requiredFeatureGates", controllerDescriptor.GetRequiredFeatureGates())

return nil, nil

}

}

and it should work properly even when it is beta and on by default and then turned off by the user.

So it is redundant to have it in this place as well. But I don't mind merging this PR first and fixing this later.

fyi, I am trying to enforce this in #121611

right, but you remove a controller explicitly, but when you also separately have a feature gate, which allows disabling the split, what then?

requiredFeatureGates field is meant as a replacement of this logic and the other gated controllers were ported to it without any issue. It is possible to gate a controller with multiple feature gates as you can see for example in StorageVersionGarbageCollectorController.

I made the simplest implementation I could. The implementation can be changed in the future if more custom decision logic (not just AND) is required. Since this controller has just one, it should be okay in this situation as well.

The reason behind this is simple, we want to reason about controllers without running them and have the gating information available declaratively. For example we want to show available controllers in kube-controller-manager --help. We can then also show the gating information there.

I kind of warned this was going to happen #120371 (comment) 🙃

I have updated the #121611 PR and included a test to show that the gating works properly with this new controller. Please let me know which part of the test you think is insufficient. Or if you think it would be a good idea to implement a generic solution for future controllers that would require more complex gating.

atiratree · 2023-10-30T13:00:15Z

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

+
+// TestTaintEvictionControllerDeclaration 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) {


I think we can remove this test now, or just check the presence of descriptor.requiredFeatureGates if you really want to keep it.

Same thing as the other comment.

soltysh

/lgtm
/approve

soltysh · 2023-10-30T18:09:07Z

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

@@ -556,6 +557,10 @@ func NewControllerDescriptors() map[string]*ControllerDescriptor {
 	register(newResourceClaimControllerDescriptor())
 	register(newLegacyServiceAccountTokenCleanerControllerDescriptor())
 	register(newValidatingAdmissionPolicyStatusControllerDescriptor())
+	if utilfeature.DefaultFeatureGate.Enabled(features.SeparateTaintEvictionController) {


@atiratree not quite, this controller is on by default, since it's beta, so this check ensures with the feature gate off it's not turned on. Not sure how to handle that with your controller initiation changes. So I think we can leave it as is, for now, and fix in a followup.

soltysh · 2023-10-30T18:09:25Z

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

+
+// TestTaintEvictionControllerDeclaration 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) {


Same thing as the other comment.

k8s-ci-robot · 2023-10-30T18:12:36Z

LGTM label has been added.

Git tree hash: 17798ff8a66e3c744af8fdb388a29ff4bdc64c19

k8s-ci-robot · 2023-10-30T18:12:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atosatto, Huang-Wei, soltysh, yuanchen8911

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kube-controller-manager/OWNERS~~ [soltysh]
~~pkg/controller/OWNERS~~ [soltysh]
~~pkg/features/OWNERS~~ [soltysh]
~~test/integration/node/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2023-10-30T18:14:45Z

/milestone v1.29

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 10, 2023

k8s-ci-robot requested review from mortent and mrunalp July 10, 2023 16:43

k8s-ci-robot added this to the v1.28 milestone Jul 10, 2023

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 10, 2023

atosatto commented Jul 10, 2023

View reviewed changes

pkg/controller/taintmanager/OWNERS Outdated Show resolved Hide resolved

Huang-Wei reviewed Jul 10, 2023

View reviewed changes

atosatto force-pushed the separate-taint-manager branch from 4edec75 to 12dca6b Compare July 11, 2023 12:16

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Jul 11, 2023

k8s-ci-robot reviewed Jul 11, 2023

View reviewed changes

pkg/controller/taintmanager/OWNERS Outdated Show resolved Hide resolved

yuanchen8911 approved these changes Oct 26, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2023

atosatto force-pushed the separate-taint-manager branch from 1071a66 to 85347a2 Compare October 30, 2023 11:10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2023

k8s-ci-robot requested a review from yuanchen8911 October 30, 2023 11:10

atosatto force-pushed the separate-taint-manager branch 2 times, most recently from 2f323d4 to 62cde68 Compare October 30, 2023 12:16

kube-controller-manager: Decouple TaintManager from NodeLifeCycleCont…

ccda2d6

…roller (KEP-3902)

atosatto force-pushed the separate-taint-manager branch from 62cde68 to ccda2d6 Compare October 30, 2023 12:24

atiratree reviewed Oct 30, 2023

View reviewed changes

soltysh approved these changes Oct 30, 2023

View reviewed changes

k8s-ci-robot assigned soltysh Oct 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023

soltysh removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2023

k8s-ci-robot modified the milestones: next-candidate, v1.29 Oct 30, 2023

atiratree mentioned this pull request Oct 30, 2023

controller descriptors should not be feature gated #121611

Merged

k8s-ci-robot merged commit e421287 into kubernetes:master Oct 30, 2023
16 checks passed

SIG Node PR Triage automation moved this from Waiting on Author to Done Oct 30, 2023

liggitt mentioned this pull request Oct 31, 2023

failing-test: TestStoreDeleteCollectionWithContextCancellation #121627

Closed

atosatto deleted the separate-taint-manager branch October 31, 2023 16:59

atosatto mentioned this pull request Mar 7, 2024

REQUEST: New membership for atosatto kubernetes/org#4804

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

atosatto commented Jul 10, 2023 •

edited

k8s-ci-robot commented Jul 10, 2023

Huang-Wei commented Jul 10, 2023

Huang-Wei commented Jul 10, 2023

atosatto commented Jul 10, 2023 •

edited

Huang-Wei left a comment

zelenushechka commented Jul 11, 2023

k8s-ci-robot left a comment

atosatto commented Jul 11, 2023 •

edited

Huang-Wei commented Oct 26, 2023

yuanchen8911 left a comment

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 29, 2023

atiratree left a comment

atiratree Oct 30, 2023

soltysh Oct 30, 2023

atiratree Oct 30, 2023

soltysh Oct 30, 2023

atiratree Oct 30, 2023

aojea Oct 30, 2023

atiratree Oct 30, 2023 •

edited

atiratree Oct 30, 2023

soltysh Oct 30, 2023

soltysh left a comment

soltysh Oct 30, 2023

soltysh Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

soltysh commented Oct 30, 2023

	for _, featureGate := range controllerDescriptor.GetRequiredFeatureGates() {
	if !utilfeature.DefaultFeatureGate.Enabled(featureGate) {
	logger.Info("Controller is disabled by a feature gate", "controller", controllerName, "requiredFeatureGates", controllerDescriptor.GetRequiredFeatureGates())
	return nil, nil
	}
	}

Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

Decouple TaintManager from NodeLifeCycleController (KEP-3902) #119208

Conversation

atosatto commented Jul 10, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jul 10, 2023

Huang-Wei commented Jul 10, 2023

Huang-Wei commented Jul 10, 2023

atosatto commented Jul 10, 2023 • edited

Huang-Wei left a comment

Choose a reason for hiding this comment

zelenushechka commented Jul 11, 2023

k8s-ci-robot left a comment

Choose a reason for hiding this comment

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

atosatto commented Jul 11, 2023 • edited

Huang-Wei commented Oct 26, 2023

yuanchen8911 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 26, 2023

k8s-ci-robot commented Oct 29, 2023

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

soltysh commented Oct 30, 2023

atosatto commented Jul 10, 2023 •

edited

atosatto commented Jul 10, 2023 •

edited

atosatto commented Jul 11, 2023 •

edited

atiratree Oct 30, 2023 •

edited