Task 0: Added node taints labels and feature flags #49547

k82cn · 2017-07-25T07:26:01Z

What this PR does / why we need it:
Added node taint const for node condition.

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

Release note:

None

k82cn · 2017-07-25T07:26:23Z

/cc @liggitt

k82cn · 2017-07-25T08:12:54Z

/retest

liggitt · 2017-07-25T13:06:37Z

plugin/pkg/scheduler/algorithm/well_known_labels.go

+	// When feature-gate for TaintBasedEvictions=true flag is enabled,
+	// TaintNodeOutOfDisk would be automatically added by node controller
+	// when node becomes out of disk, and removed when node has enough disk.
+	TaintNodeOutOfDisk = "node.alpha.kubernetes.io/outOfDisk"


What is the rationale behind including alpha in the taint name? Is there concern a taint named node.kubernetes.io/outOfDisk would want to be used in some other way in the future? If not, I'd prefer taking the approach recommended for API fields:

Carefully choose names

Feature gate the setting of the data while in alpha

On graduation to beta, the feature gate can be enabled by default

That means that if the feature survives, clusters that enabled and used the taints in alpha state can simply continue to do so.

cc @kubernetes/sig-api-machinery-pr-reviews

Seems node.kubernetes.io/outOfDisk is better to keep the backward compatibility. @gmarek , WDYT?

+1 for node.kubernetes.io/outOfDisk

Great! Updated to node.kubernetes.io/xxxx. Will handle notReady and unreachable in beta graduation PR.

k82cn · 2017-07-25T13:22:59Z

/cc @gmarek

k82cn · 2017-07-25T13:51:08Z

/cc @kubernetes/sig-scheduling-api-reviews

k82cn · 2017-07-31T11:33:31Z

/unassign @jayunit100 @timothysc
/assign @gmarek

k82cn · 2017-07-31T23:44:24Z

/retest

timothysc · 2017-08-01T03:18:23Z

plugin/pkg/scheduler/algorithm/well_known_labels.go

+	// When feature-gate for TaintBasedEvictions=true flag is enabled,
+	// TaintNodeOutOfDisk would be automatically added by node controller
+	// when node becomes out of disk, and removed when node has enough disk.
+	TaintNodeOutOfDisk = "node.kubernetes.io/outOfDisk"


these should all have .alpha.

see the discussion on including alpha in the name in kubernetes/community#819 (comment)

tl;dr: if you include alpha in the name, there is no reasonable way to migrate people to the beta and ga versions. feature-gating the alpha function is sufficient to keep it from affecting anything until it graduates to beta or an admin opts in to using it in alpha mode.

Update the link to open comments directly (https://github.com/kubernetes/community/pull/819/files/1bb113172319e32a8bde63f996c0b382e2fa511a#r128887276)

@timothysc , is that OK for your?

We should update other taints then, and have a pattern for upgrade.
/cc @davidopp

I'd like to update other taint when graduating from alpha to beta in separate PR.

these look OK to me, per @LiGgit's comment

k82cn · 2017-08-02T01:17:58Z

if not comments on the name of label, can we get this PR merged firstly?

timothysc · 2017-08-02T16:51:46Z

pkg/features/kube_features.go

+	//
+	// Taint nodes based on their condition status for 'NetworkUnavailable',
+	// 'MemoryPressure', 'OutOfDisk' and 'DiskPressure'.
+	TaintNodesByCondition utilfeature.Feature = "TaintNodesByCondition"


Don't feature gates need to be plumbed through api-machinery to hit all component configs?

Nop; AFAIK, feature gates are local map in each component for now :).

timothysc · 2017-08-02T16:53:13Z

plugin/pkg/scheduler/algorithm/well_known_labels.go

+	// When feature-gate for TaintBasedEvictions=true flag is enabled,
+	// TaintNodeOutOfDisk would be automatically added by node controller
+	// when node becomes out of disk, and removed when node has enough disk.
+	TaintNodeOutOfDisk = "node.kubernetes.io/outOfDisk"


We should update other taints then, and have a pattern for upgrade.
/cc @davidopp

k82cn · 2017-08-03T23:27:31Z

@timothysc @liggitt @gmarek , can we get this merged?

liggitt · 2017-08-04T01:40:31Z

@timothysc @liggitt @gmarek , can we get this merged?

My main interest was the taint name and plans for progressing to beta and stable, so no objections from me

k82cn · 2017-08-04T05:46:55Z

@davidopp , do you have any comments for the name of taint labels? If not, I think we can merge this :).

gmarek · 2017-08-04T07:51:41Z

/lgtm

wojtek-t · 2017-08-04T08:20:21Z

/approve

k8s-github-robot · 2017-08-04T08:20:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmarek, k82cn, wojtek-t

Associated issue: 42001

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/OWNERS~~ [wojtek-t]
~~plugin/OWNERS~~ [wojtek-t]

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-08-04T15:21:39Z

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

Review the full test history for this PR.

k82cn · 2017-08-04T16:15:10Z

/retest

k8s-github-robot · 2017-08-04T21:29:40Z

Automatic merge from submit-queue (batch tested with PRs 50119, 48366, 47181, 41611, 49547)

Added node taints labels.

c85e4dc

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 25, 2017

k8s-github-robot assigned timothysc and jayunit100 Jul 25, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Jul 25, 2017

k8s-ci-robot requested a review from liggitt July 25, 2017 07:26

k82cn mentioned this pull request Jul 25, 2017

Task 1: Tainted node by condition. #49257

Merged

liggitt reviewed Jul 25, 2017

View reviewed changes

k8s-ci-robot requested a review from gmarek July 25, 2017 13:22

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jul 25, 2017

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2017

k82cn changed the title ~~Task 0: Added node taints labels.~~ Task 0: Added node taints labels and feature flags Jul 27, 2017

k82cn mentioned this pull request Jul 30, 2017

NodeController should add NoSchedule taints and we should get rid of getNodeConditionPredicate() #42001

Closed

7 tasks

Added taints node by condition feature flag.

ec4aa19

k82cn force-pushed the k8s_42001_0 branch from bad7387 to ec4aa19 Compare July 31, 2017 11:30

k8s-ci-robot assigned gmarek and unassigned timothysc and jayunit100 Jul 31, 2017

timothysc reviewed Aug 1, 2017

View reviewed changes

timothysc reviewed Aug 2, 2017

View reviewed changes

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

gmarek assigned wojtek-t Aug 4, 2017

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

timothysc added this to the v1.8 milestone Aug 4, 2017

k8s-github-robot merged commit 90a45b2 into kubernetes:master Aug 4, 2017

davidopp mentioned this pull request Sep 22, 2017

delete pods API call latencies shot up on large cluster tests #51899

Closed

k82cn deleted the k8s_42001_0 branch May 16, 2022 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task 0: Added node taints labels and feature flags #49547

Task 0: Added node taints labels and feature flags #49547

k82cn commented Jul 25, 2017 •

edited

Loading

k82cn commented Jul 25, 2017

k82cn commented Jul 25, 2017

liggitt Jul 25, 2017

k82cn Jul 27, 2017

wanghaoran1988 Jul 28, 2017

gmarek Jul 31, 2017

k82cn Jul 31, 2017

k82cn commented Jul 25, 2017

k82cn commented Jul 25, 2017

k82cn commented Jul 31, 2017

k82cn commented Jul 31, 2017

timothysc Aug 1, 2017

liggitt Aug 1, 2017

k82cn Aug 1, 2017

k82cn Aug 2, 2017

timothysc Aug 2, 2017

k82cn Aug 3, 2017

davidopp Aug 4, 2017

k82cn commented Aug 2, 2017

timothysc Aug 2, 2017

k82cn Aug 3, 2017

timothysc Aug 2, 2017

k82cn commented Aug 3, 2017

liggitt commented Aug 4, 2017

k82cn commented Aug 4, 2017 •

edited

Loading

gmarek commented Aug 4, 2017

wojtek-t commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

fejta-bot commented Aug 4, 2017

k82cn commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

Task 0: Added node taints labels and feature flags #49547

Task 0: Added node taints labels and feature flags #49547

Conversation

k82cn commented Jul 25, 2017 • edited Loading

k82cn commented Jul 25, 2017

k82cn commented Jul 25, 2017

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

k82cn commented Jul 25, 2017

k82cn commented Jul 25, 2017

k82cn commented Jul 31, 2017

k82cn commented Jul 31, 2017

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

k82cn commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Aug 3, 2017

liggitt commented Aug 4, 2017

k82cn commented Aug 4, 2017 • edited Loading

gmarek commented Aug 4, 2017

wojtek-t commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

fejta-bot commented Aug 4, 2017

k82cn commented Aug 4, 2017

k8s-github-robot commented Aug 4, 2017

k82cn commented Jul 25, 2017 •

edited

Loading

k82cn commented Aug 4, 2017 •

edited

Loading