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

Refactor node taint conditions #51266

Merged
merged 2 commits into from
Oct 4, 2017
Merged

Conversation

resouer
Copy link
Contributor

@resouer resouer commented Aug 24, 2017

What this PR does / why we need it:
We should use not-ready etc as node condition taint key.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):
fixes #51246

Special notes for your reviewer:

Release note:

Use `not-ready` to replace `notReady` in node condition taint keys.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 24, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 24, 2017
@resouer resouer requested a review from k82cn August 24, 2017 12:03
@resouer
Copy link
Contributor Author

resouer commented Aug 24, 2017

I would prefer @k82cn to review this.

@resouer resouer assigned k82cn and unassigned lavalamp and janetkuo Aug 24, 2017
@resouer
Copy link
Contributor Author

resouer commented Aug 24, 2017

/assign @lavalamp

@k82cn
Copy link
Member

k82cn commented Aug 24, 2017

/lgtm

I think we need to update doc accordingly, both design doc and guidance.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2017
@resouer
Copy link
Contributor Author

resouer commented Aug 24, 2017

Right, I will try a global search and fix across the org.

@luxas
Copy link
Member

luxas commented Aug 24, 2017

Thanks @resouer for this PR!
cc @thockin ^

@resouer
Copy link
Contributor Author

resouer commented Aug 24, 2017

@luxas One concern is how we can avoid affecting existing user labels?

@xiangpengzhao
Copy link
Contributor

I think we should consider backward compatibility. This change might break some user applications (or PaaS project based on Kubernetes).

@resouer
Copy link
Contributor Author

resouer commented Aug 25, 2017

The affected users in my mind:

  1. Admins tainted their node with these keys manually. notReady taint may be the most common case here.

  2. Systems read these taints to learn about the node information, this may happen to users who build there own system based on k8s, which are tend to be hard coded.

Some ideas in my mind:

  1. We need to announce this change in kubernetes-users & kubernetes-dev mailing list at least.
  2. Need a release note
  3. Any other ideas?

cc @davidopp

@k82cn
Copy link
Member

k82cn commented Aug 25, 2017

@resouer , I think we need to send an announcement to dev@ and user@ list. If the new name is right, it's a good time to correct it in alpha :).

@lavalamp
Copy link
Member

Are these currently in use? This seems very unlikely to be backwards compatible.

@resouer
Copy link
Contributor Author

resouer commented Aug 31, 2017

@lavalamp In most cases they are internal used, but I think there can be some external use cases (though not best practice), see: #51266 (comment)

The most common case will be not-ready, but it is in alpha stage, that's why we agree it's a good time to fix it. Make sense?

@resouer
Copy link
Contributor Author

resouer commented Sep 5, 2017

kindly ping @lavalamp back to this thread 💯

@k82cn
Copy link
Member

k82cn commented Sep 7, 2017

ping for approve :).

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 15, 2017
@resouer resouer removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 16, 2017
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Looks like a well-tested, backwards-compatible change to me.

@resouer
Copy link
Contributor Author

resouer commented Sep 29, 2017

Thanks @jaypipes

@liggitt Feel free to do a new round of review whenever you have time.

@luxas
Copy link
Member

luxas commented Sep 29, 2017

Yay, let's get this into v1.9 ASAP

@@ -404,6 +404,17 @@ func NewNodeController(
})
}

// NOTE(resouer): nodeInformer to substitute deprecated taint key (notReady -> not-ready).
Copy link
Contributor

Choose a reason for hiding this comment

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

s/NOTE/TODO/

@gmarek
Copy link
Contributor

gmarek commented Oct 3, 2017

LGTM. Ping @liggitt for final lgtm.

@liggitt
Copy link
Member

liggitt commented Oct 3, 2017

/lgtm

needs release-note-action-required with a description of the full non-alpha taint name so people can update their tolerations prior to upgrade to avoid disruption

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017
@gmarek
Copy link
Contributor

gmarek commented Oct 4, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmarek, k82cn, liggitt, resouer

Associated issue: 51246

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 731f421 into kubernetes:master Oct 4, 2017
@resouer resouer deleted the not-ready branch October 4, 2017 14:04
k8s-github-robot pushed a commit to kubernetes/community that referenced this pull request Nov 21, 2017
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
justaugustus pushed a commit to justaugustus/enhancements that referenced this pull request Sep 3, 2018
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
MadhavJivrajani pushed a commit to kubernetes/design-proposals-archive that referenced this pull request Nov 30, 2021
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
MadhavJivrajani pushed a commit to MadhavJivrajani/design-proposals that referenced this pull request Dec 1, 2021
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
MadhavJivrajani pushed a commit to MadhavJivrajani/design-proposals that referenced this pull request Dec 1, 2021
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
MadhavJivrajani pushed a commit to MadhavJivrajani/design-proposals that referenced this pull request Dec 1, 2021
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
MadhavJivrajani pushed a commit to kubernetes/design-proposals-archive that referenced this pull request Dec 1, 2021
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
MadhavJivrajani pushed a commit to kubernetes/design-proposals-archive that referenced this pull request Dec 1, 2021
Automatic merge from submit-queue.

Use new taint key in design doc

Part of: kubernetes/kubernetes#51246

~~**DO NOT MERGE** until kubernetes/kubernetes#51266 is in!~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"notReady" toleration should be "not-ready"