Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mark enable-taint-manager CLI flag as deprecated #111411

Merged
merged 1 commit into from Aug 1, 2022
Merged

Mark enable-taint-manager CLI flag as deprecated #111411

merged 1 commit into from Aug 1, 2022

Conversation

alculquicondor
Copy link
Member

What type of PR is this?

/kind cleanup
/kind api-change

What this PR does / why we need it:

The taint manager supports the feature TaintBasedEvictions, which is GA since 1.18. We don't want to continue supporting the older path.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The command line flag `enable-taint-manager` for kube-controller-manager is deprecated and will be removed in 1.26.
The feature that it supports, taint based eviction, is enabled by default and will continue to be implicitly enabled when the flag is removed.

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

- [Usage]: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/#taint-based-evictions

@alculquicondor
Mark enable-taint-manager CLI flag as deprecated
cdb0b6b 
Change-Id: Ib0247e034def837f973b69c55ed6bc9486bd364e
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 25, 2022
@alculquicondor
Copy link
Member Author

/assign @lavalamp

lavalamp
lavalamp reviewed Jul 25, 2022
View reviewed changes
cmd/kube-controller-manager/app/options/nodelifecyclecontroller.go
@@ -44,7 +44,8 @@ func (o *NodeLifecycleControllerOptions) AddFlags(fs *pflag.FlagSet) {
fs.Float32Var(&o.SecondaryNodeEvictionRate, "secondary-node-eviction-rate", 0.01, "Number of nodes per second on which pods are deleted in case of node failure when a zone is unhealthy (see --unhealthy-zone-threshold for definition of healthy/unhealthy). Zone refers to entire cluster in non-multizone clusters. This value is implicitly overridden to 0 if the cluster size is smaller than --large-cluster-size-threshold.")
fs.Int32Var(&o.LargeClusterSizeThreshold, "large-cluster-size-threshold", 50, "Number of nodes from which NodeController treats the cluster as large for the eviction logic purposes. --secondary-node-eviction-rate is implicitly overridden to 0 for clusters this size or smaller.")
fs.Float32Var(&o.UnhealthyZoneThreshold, "unhealthy-zone-threshold", 0.55, "Fraction of Nodes in a zone which needs to be not Ready (minimum 3) for zone to be treated as unhealthy. ")
fs.BoolVar(&o.EnableTaintManager, "enable-taint-manager", o.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(&o.EnableTaintManager, "enable-taint-manager", o.EnableTaintManager, "If set to true enables NoExecute Taints and will evict all not-tolerating Pod running on Nodes tainted with this kind of Taints.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this flag functional today? What if someone doesn't want to run this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is functional. If you set it to false, the older codepath runs. But we can't keep maintaining that path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damemi I think you graduated this to GA. Was this CLI flag left on purpose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alculquicondor no, I think it was just an oversight, didn't realize there was a command-line flag along with the feature gates so that's probably why it got overlooked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question as @lavalamp, this patch is based on an assumption that all people need to run taint manager, is that true?

If you set it to false, the older codepath runs. But we can't keep maintaining that path.

In addition, I'm thinking what's the benefit we can get from the deprecation work. Seems like it helps reduce the maintenance effort, right?
Can you give a link to the older codepath?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this patch is based on an assumption that all people need to run taint manager

The assumption is that the newer codepath does everything that the older codepath does, so there is no need to keep maintaining both.

In addition, I'm thinking what's the benefit we can get from the deprecation work. Seems like it helps reduce the maintenance effort, right?

Yes. This deprecation is motivated by #110959, where we were wondering if we should add new logic to this old codepath. And it doesn't really make sense to do so.

Can you give a link to the older codepath?

It's hard to visualize, but it's embedded in https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/nodelifecycle/node_lifecycle_controller.go
when runTaintManager=false

@alculquicondor
Copy link
Member Author

/retest

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@mimowo mimowo mentioned this pull request Jul 26, 2022
Merged
@leilajal
Copy link
Contributor

/cc @deads2k
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 26, 2022
@mimowo mimowo mentioned this pull request Jul 28, 2022
Merged
@alculquicondor
Copy link
Member Author

alculquicondor commented Jul 28, 2022 •
edited

Can we have this PR approved this week and keep on hold in case someone objects before the codefreeze?

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2022
@lavalamp
Copy link
Member

Sure.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, lavalamp

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2022
@alculquicondor alculquicondor mentioned this pull request Jul 29, 2022
Closed
@alculquicondor
Copy link
Member Author

/hold cancel

there was no push back

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2022
@k8s-ci-robot k8s-ci-robot merged commit d46742b into kubernetes:master Aug 1, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 1, 2022
@kerthcet kerthcet mentioned this pull request Sep 9, 2022
Closed
@kerthcet kerthcet mentioned this pull request Nov 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@damemi damemi damemi left review comments

@RainbowMango RainbowMango RainbowMango left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@mikedanese mikedanese Awaiting requested review from mikedanese

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@lavalamp lavalamp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@alculquicondor @k8s-triage-robot @leilajal @lavalamp @k8s-ci-robot @damemi @RainbowMango