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.

Sign up for GitHub

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

Taint based eviction promoted to GA in 1.18 #19302

Merged
merged 1 commit into from
Mar 12, 2020
Merged

Taint based eviction promoted to GA in 1.18 #19302

merged 1 commit into from
Mar 12, 2020

Conversation

ingvagabund
Copy link
Contributor

@ingvagabund ingvagabund commented Feb 25, 2020 •
edited
Loading

Taint based eviction as described in https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/#taint-based-evictions is about to be GA'ed in 1.18. Updating the docs appropriately.

Fixes: kubernetes/kubernetes#87429

Will cherry-pick to 1.18 until this one is reviewed or before Friday.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 25, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 25, 2020
This was referenced Feb 25, 2020
Closed
Closed
@damemi
Copy link
Contributor

damemi commented Feb 25, 2020 •
edited
Loading

/hold
until the other PRs go in to functionally make this GA, see kubernetes/kubernetes#87161

@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 Feb 25, 2020
@netlify
Copy link

netlify bot commented Feb 25, 2020 •
edited
Loading

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 10378d0

https://deploy-preview-19302--kubernetes-io-master-staging.netlify.com

@sftim
Copy link
Contributor

sftim commented Feb 25, 2020

Hi @ingvagabund

This repository has a live master branch (and does not usually cherry-pick). I think you should edit this PR to target the branch dev-1.18.

/sig scheduling

@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Feb 25, 2020
sftim
sftim reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Remember to update https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

@ingvagabund ingvagabund force-pushed the taint-based-eviction-promoted-to-ga-in-1.18 branch from 4888745 to 10378d0 Compare February 25, 2020 16:12
@ingvagabund ingvagabund changed the base branch from master to dev-1.18 February 25, 2020 16:15
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 25, 2020
@ingvagabund ingvagabund force-pushed the taint-based-eviction-promoted-to-ga-in-1.18 branch from 10378d0 to 0de472c Compare February 25, 2020 16:16
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Feb 25, 2020 •
edited
Loading

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit 37912a1

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e663b5e9120a50009b04841

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 25, 2020
@ingvagabund
Copy link
Contributor Author

@sftim thanks!!!

@ingvagabund ingvagabund force-pushed the taint-based-eviction-promoted-to-ga-in-1.18 branch from 0de472c to 51ed2bd Compare February 26, 2020 09:33
@ingvagabund ingvagabund mentioned this pull request Feb 26, 2020
Closed
@damemi damemi mentioned this pull request Mar 3, 2020
Merged
damemi
damemi approved these changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

/lgtm
This is just waiting for kubernetes/kubernetes#88152 and kubernetes/kubernetes#87487 to merge

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 3, 2020
@damemi damemi mentioned this pull request Mar 4, 2020
Merged
@ingvagabund ingvagabund force-pushed the taint-based-eviction-promoted-to-ga-in-1.18 branch from 51ed2bd to 9c6af49 Compare March 5, 2020 10:25
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2020
@damemi
Copy link
Contributor

damemi commented Mar 5, 2020

/hold cancel

@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 Mar 5, 2020
@Huang-Wei
Copy link
Member

@ingvagabund A reminder Re:

Will cherry-pick to 1.18 until this one is reviewed or before Friday.

The 1.18 release team will cover it, so you don't need to manually cherry-pick it.

@ahg-g
Copy link
Member

ahg-g commented Mar 5, 2020

/milestone v1.18

@k8s-ci-robot
Copy link
Contributor

@ahg-g: You must be a member of the kubernetes/website-milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Website milestone maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.18

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.

@VineethReddy02
Copy link
Contributor

👋 Please rebase this PR on dev-1.18 in order to avoid merge conflicts.

Feel free to /hold cancel after you've rebased.

/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 Mar 7, 2020
sftim
sftim reviewed Mar 8, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Some more feedback on the text @ingvagabund

content/en/docs/concepts/configuration/taint-and-toleration.md Outdated Show resolved Hide resolved
content/en/docs/concepts/configuration/taint-and-toleration.md Outdated
Comment on lines 234 to 235
The taints are automatically added by the NodeController (or kubelet) and the normal
logic for evicting pods from nodes based on the Ready NodeCondition is disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this also accurate

Suggested change
The taints are automatically added by the NodeController (or kubelet) and the normal
logic for evicting pods from nodes based on the Ready NodeCondition is disabled.
When the node controller or the kubelet observe problems on a node, they add taints to the
relevant Node object. If the fault condition returns to normal the kubelet or node
controller can remove the relevant taint(s). The kubelet reports problems that affect
its ability to run Pods whereas the node controller monitors whether the kubelet
itself is healthy.

?

I wouldn't mention “the normal logic for evicting pods from nodes based on the Ready NodeCondition is disabled” if there's no way to make use of that logic in v1.18. If there is a way to make use of that formerly-normal logic, tell readers how to learn about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to be careful here to properly distinguish when a taint has NoSchedule effect and when it has NoExecute. I have integrated the first two sentences. The last one expresses a general statement saying "a node is tainted based on a condition" which can have both effects interchangeable.

@ingvagabund ingvagabund force-pushed the taint-based-eviction-promoted-to-ga-in-1.18 branch from 9c6af49 to 37912a1 Compare March 9, 2020 12:49
@ingvagabund
Copy link
Contributor Author

Please rebase this PR on dev-1.18 in order to avoid merge conflicts.

Done

Feel free to /hold cancel after you've rebased.

/hold cancel

@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 Mar 9, 2020
@ingvagabund
Copy link
Contributor Author

@sftim updated, PTAL

damemi
damemi approved these changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

Bumping, is there any more feedback for this?
/lgtm
from the scheduling side

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2020
sftim
sftim reviewed Mar 12, 2020
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Markdown & page style looks good to me

@damemi
Copy link
Contributor

damemi commented Mar 12, 2020

@sftim thanks, can you /approve this?

@sftim
Copy link
Contributor

sftim commented Mar 12, 2020

@damemi I'm not on the docs release team
@VineethReddy02 would you have time to take a look?

@VineethReddy02
Copy link
Contributor

/lgtm

@sftim
Copy link
Contributor

sftim commented Mar 12, 2020

/approve
following #19302 (comment)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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 Mar 12, 2020
@k8s-ci-robot k8s-ci-robot merged commit 10bd735 into kubernetes:dev-1.18 Mar 12, 2020
@ingvagabund ingvagabund deleted the taint-based-eviction-promoted-to-ga-in-1.18 branch March 12, 2020 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@damemi damemi damemi approved these changes

@davidopp davidopp Awaiting requested review from davidopp

@kevin-wangzefeng kevin-wangzefeng Awaiting requested review from kevin-wangzefeng

Assignees

@damemi damemi

@VineethReddy02 VineethReddy02

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. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TaintBasedEvictions] Update docs to reflect GA status
8 participants
@ingvagabund @damemi @sftim @k8sio-netlify-preview-bot @Huang-Wei @ahg-g @k8s-ci-robot @VineethReddy02