Move of unreachable taint key out of alpha #54208

resouer · 2017-10-19T07:23:29Z

What this PR does / why we need it:

Move of unreachable taint key out of alpha, which already happened in community doc.

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

Special notes for your reviewer:
Please see #54198 for the context of this inconsistency.

Release note:

Move unreachable taint key out of alpha. 
Please note the existing pods with the alpha toleration should be updated by user himself to tolerate the GA taint.

bsalamat · 2017-10-19T21:54:12Z

/lgtm

I wait for Klaus to take a look as well before approving, just in case.

k82cn · 2017-10-20T01:47:00Z

plugin/pkg/scheduler/algorithm/well_known_labels.go

@@ -30,7 +30,7 @@ const (
 	// TaintNodeUnreachable would be automatically added by node controller
 	// when node becomes unreachable (corresponding to NodeReady status ConditionUnknown)
 	// and removed when node becomes reachable (NodeReady status ConditionTrue).
-	TaintNodeUnreachable = "node.alpha.kubernetes.io/unreachable"
+	TaintNodeUnreachable = "node.kubernetes.io/unreachable"


Why not do this similar with not-ready? Add an new label, named DeprecateTaintNodeUnreachable.

@k82cn Not very sure about this actually, as unreachable seems to be internal used only.

@liggitt Do you think we need to address compatibility issue here, just like what we did for not-ready taint?

@liggitt , any suggestion :).

similar issue to the other taint name change... the node controller currently adds this taint, so I'd expect it to remove the alpha taint, otherwise you can end up with a node with a permanent taint added by a kubernetes component

also, the DefaultTolerationSeconds admission plugin currently adds tolerations for the alpha taint (and isn't documented to be alpha or subject to change). What's the plan for avoiding disrupting pods admitted with tolerations by that plugin?

The approach in #51266 should be able to fix the first concern, i.e. auto-replace old taint to new taint by controller.

Can we do the same operation for DefaultTolerationSeconds? let the admission plugin replace old toleration with new one. @liggitt WDYT?

let the admission plugin replace old toleration with new one. @liggitt WDYT?

that doesn't update pods already in the system

So maybe add a admit plugin to fix pods tolerations? @liggitt

I'm not sure the best course of action... @davidopp how did you envision updating system-added tolerations on existing pods when the system-added taint changes names.

kindly ping @davidopp , we are discussing the strategy to upgrade alpha taint keys without breaking existing user, and your advice would be highly valued here.

k82cn · 2017-10-20T01:47:33Z

/approve cancel

k82cn · 2017-10-20T01:48:00Z

/cc @liggitt

k82cn · 2017-10-20T01:49:18Z

/lgtm cancel

k82cn · 2017-10-20T01:50:30Z

Just to stop merge progress before we get agreement on the PR :).

BenTheElder · 2017-10-20T03:16:49Z

/retest

BenTheElder · 2017-10-20T04:17:10Z

/retest

davidopp · 2017-10-27T06:26:30Z

@gmarek

davidopp · 2017-10-27T06:27:33Z

I'll take a look at this in the next few days, but if @gmarek has a comment it would be useful too.

resouer · 2017-10-27T06:33:54Z

Thanks David! @davidopp

@gmarek As @liggitt pointed out, the DefaultTolerationSeconds admission plugin currently adds tolerations for the alpha taint. So the pods already in the system are tend be suffered from this update.

One possible approach I am thinking is adding a admit to fix pods toleration. Do you think it's reasonable?

bsalamat · 2017-10-27T17:40:03Z

@resouer Why don't you take a similar approach to notReady? Leave the alpha in place and mark it deprecated and add a new taint for non-alpha version.

resouer · 2017-10-30T14:43:06Z

@bsalamat The special problem here is the DefaultTolerationSeconds admission plugin currently adds tolerations for the alpha taint to Pods.

So I just proposed to adding a admit to fix pods tolerations. Does it sound good to you?

bsalamat · 2017-10-30T18:11:44Z

@resouer Your description is a bit brief. So, I am not sure if your solution will handle the following scenario correctly:

Assume that an existing replicaset is creating pods from an existing template which has toleration for the alpha taint.
Nodes get the GA taint.
A new pod for the replicaset is created with the old taint.

Will the new pod be scheduled?

resouer · 2017-10-31T15:40:20Z

@bsalamat I think that approach may solve the problem:

User replaces Kubernetes binary to upgrade.
The node controller replaces alpha taint to GA taint on node (like we did before).
Then we write an admission plugin, it will check if the pod has old taint key, and replace it with new one (also, update the pod template of its controllerRef).

So the system should work.

Am I missed something?

liggitt · 2017-10-31T15:48:22Z

we write an admission plugin, when it sees new pod comes in, it will check if the pod has old taint key, and replace it with new one

I don't think this is necessary... I'd only expect us to work to smoothly transition objects tainted/tolerated by in-tree components:

switch the existing admission plugin to write the non-alpha toleration
handle updating existing pods to swap the alpha toleration with a non-alpha toleration (or add the non-alpha toleration... I don't think existing tolerations can be removed)
update nodes to remove the alpha taint and add the non-alpha taint

I think out of tree components are responsible for stopping their own usage of alpha tolerations

bsalamat · 2017-10-31T18:30:01Z

What @liggitt said makes sense. Given that this was an alpha taint, we don't need to avoid breaking external components that use the alpha taint in the future.

resouer · 2017-11-02T14:25:33Z

OK, so the scope is much clear now. I've already update the patch and amending unit tests now. Thank you for advice.

bsalamat · 2017-11-15T18:37:04Z

@resouer Can you please update the release note and mention that existing pods with the alpha toleration should be updated to tolerate the GA taint as well?

resouer · 2017-11-16T02:28:51Z

@bsalamat Updated. Will also mention this in google group if it's fine.

bsalamat · 2017-11-16T18:17:17Z

/lgtm

bsalamat · 2017-11-16T18:20:51Z

I think this PR requires "action required" label.

dims · 2017-11-16T21:06:53Z

/assign @deads2k

k8s-github-robot · 2017-11-16T21:07:22Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@bsalamat @deads2k @k82cn @resouer

Pull Request Labels

sig/scheduling: Pull Request will be escalated to these SIGs if needed.
priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
kind/cleanup: Adding tests, refactoring, fixing old bugs.

Help

resouer · 2017-11-17T14:09:54Z

@bsalamat I just updated the order of test cases to fix test failure after rebase, could you please apply label again?

I think this PR requires "action required" label.

@dims Do you know how to do that?

resouer · 2017-11-17T14:41:41Z

/test pull-kubernetes-unit

bsalamat · 2017-11-17T19:30:29Z

/lgtm

dims · 2017-11-18T15:16:32Z

/retest

deads2k · 2017-11-20T13:28:00Z

You probably want to address the migration concern here: #54208 (comment)

/approve

k8s-github-robot · 2017-11-20T13:28:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, deads2k, resouer

Associated issue: 54198

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

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

~~pkg/controller/node/OWNERS~~ [deads2k]
~~plugin/pkg/admission/OWNERS~~ [deads2k]
~~plugin/pkg/scheduler/OWNERS~~ [bsalamat]

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

fejta-bot · 2017-11-20T16:25:14Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-11-20T18:05:05Z

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

k8s-github-robot · 2017-11-20T18:50:16Z

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

resouer · 2017-11-21T04:27:56Z

@deads2k Yes, the current plan is Release Note + Announcement in kubernetes-dev

resouer added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Oct 19, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 19, 2017

k8s-github-robot assigned k82cn and bsalamat Oct 19, 2017

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

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

k82cn reviewed Oct 20, 2017

View reviewed changes

k8s-ci-robot requested a review from liggitt October 20, 2017 01:48

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

resouer force-pushed the rm-alpha branch from acd5002 to 7e0342f Compare October 20, 2017 04:14

resouer force-pushed the rm-alpha branch from 7e0342f to 5fc89fb Compare November 2, 2017 14:59

resouer added status/approved-for-milestone and removed milestone/needs-approval labels Nov 15, 2017

resouer mentioned this pull request Nov 16, 2017

Tracking work to deprecate alpha taint key and camelCase taint key #53236

Closed

12 tasks

resouer changed the title ~~[RFC] Move of unreachable taint key out of alpha~~ Move of unreachable taint key out of alpha Nov 16, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2017

k8s-ci-robot assigned deads2k Nov 16, 2017

Harry Zhang added 2 commits November 17, 2017 15:35

Replace node's alpha taint key with GA

df8c92a

Add GA toleration key and leave alpha ones untouched

e380c21

resouer force-pushed the rm-alpha branch from d23d54d to e380c21 Compare November 17, 2017 14:07

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2017

k8s-github-robot merged commit d0301aa into kubernetes:master Nov 20, 2017

resouer deleted the rm-alpha branch November 21, 2017 04:28

Move of unreachable taint key out of alpha #54208

Move of unreachable taint key out of alpha #54208

Conversation

resouer commented Oct 19, 2017 • edited

bsalamat commented Oct 19, 2017

k82cn Oct 20, 2017

Choose a reason for hiding this comment

resouer Oct 20, 2017 • edited

Choose a reason for hiding this comment

k82cn Oct 23, 2017

Choose a reason for hiding this comment

liggitt Oct 23, 2017 • edited

Choose a reason for hiding this comment

resouer Oct 24, 2017 • edited

Choose a reason for hiding this comment

liggitt Oct 24, 2017

Choose a reason for hiding this comment

resouer Oct 24, 2017 • edited

Choose a reason for hiding this comment

liggitt Oct 24, 2017

Choose a reason for hiding this comment

resouer Oct 27, 2017

Choose a reason for hiding this comment

k82cn commented Oct 20, 2017

k82cn commented Oct 20, 2017

k82cn commented Oct 20, 2017

k82cn commented Oct 20, 2017

BenTheElder commented Oct 20, 2017

BenTheElder commented Oct 20, 2017

davidopp commented Oct 27, 2017

davidopp commented Oct 27, 2017

resouer commented Oct 27, 2017 • edited

bsalamat commented Oct 27, 2017

resouer commented Oct 30, 2017 • edited

bsalamat commented Oct 30, 2017

resouer commented Oct 31, 2017 • edited

liggitt commented Oct 31, 2017 • edited

bsalamat commented Oct 31, 2017

resouer commented Nov 2, 2017

bsalamat commented Nov 15, 2017 • edited

resouer commented Nov 16, 2017

bsalamat commented Nov 16, 2017

bsalamat commented Nov 16, 2017

dims commented Nov 16, 2017

k8s-github-robot commented Nov 16, 2017

resouer commented Nov 17, 2017

resouer commented Nov 17, 2017

bsalamat commented Nov 17, 2017

dims commented Nov 18, 2017

deads2k commented Nov 20, 2017

k8s-github-robot commented Nov 20, 2017

fejta-bot commented Nov 20, 2017

k8s-github-robot commented Nov 20, 2017

k8s-github-robot commented Nov 20, 2017

resouer commented Nov 21, 2017

resouer commented Oct 19, 2017 •

edited

resouer Oct 20, 2017 •

edited

liggitt Oct 23, 2017 •

edited

resouer Oct 24, 2017 •

edited

resouer Oct 24, 2017 •

edited

resouer commented Oct 27, 2017 •

edited

resouer commented Oct 30, 2017 •

edited

resouer commented Oct 31, 2017 •

edited

liggitt commented Oct 31, 2017 •

edited

bsalamat commented Nov 15, 2017 •

edited