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

Added unschedulable taint #61161

Merged
merged 1 commit into from Mar 16, 2018

Conversation

@k82cn
Member

k82cn commented Mar 14, 2018

Signed-off-by: Da K. Ma klaus1982.cn@gmail.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of #59194; fixes #61050

Release note:

When `TaintNodesByCondition` enabled, added `node.kubernetes.io/unschedulable:NoSchedule`
 taint to the node if `spec.Unschedulable` is true.

When `ScheduleDaemonSetPods` enabled, `node.kubernetes.io/unschedulable:NoSchedule` 
toleration is added automatically to DaemonSet Pods; so the `unschedulable` field of 
a node is not respected by the DaemonSet controller.
@k82cn

This comment has been minimized.

Member

k82cn commented Mar 14, 2018

@k8s-ci-robot k8s-ci-robot requested review from bsalamat and janetkuo Mar 14, 2018

@@ -438,7 +438,20 @@ func (nc *Controller) doNoScheduleTaintingPass(node *v1.Node) error {
}
}
}
if node.Spec.Unschedulable {
// If unscheduable, append related taint.

This comment has been minimized.

@liggitt

liggitt Mar 14, 2018

Member

what removes this taint when the node becomes schedulable again?

This comment has been minimized.

@liggitt

liggitt Mar 14, 2018

Member

definitely needs a test demonstrating this lifecycle works properly

This comment has been minimized.

@janetkuo

janetkuo Mar 14, 2018

Member

Good point. This might break kubectl cordon/uncordon too.

This comment has been minimized.

@yastij

yastij Mar 14, 2018

Member

@janetkuo +1 as we need to ensure how the taint is removed when kubectl uncordon

This comment has been minimized.

@k82cn

k82cn Mar 15, 2018

Member

The removal is handled here: https://github.com/kubernetes/kubernetes/pull/61161/files#diff-43035107687eb30550696751ac1066e4R458 . we'll get target taints according to the conditions, and the exist taints on the node; and then get taintsToAdd and taintsToDel by TaintSetDiff. SwapNodeControllerTaint will update the taints accordingly.

re a test: definitely :)

This comment has been minimized.

@k82cn

k82cn Mar 15, 2018

Member

added integration test for unschedulable; tested removal manually.

@bsalamat

When Unschedulable field is cleared, we need to remove the "NoSchedule" taint.

// AppendUnscheduableTaintIfNotExist appends unscheduable toleration to `.spec` if not exist; otherwise,
// no changes to `.spec.tolerations`.
func AppendUnscheduableTaintIfNotExist(tolerations []v1.Toleration) []v1.Toleration {

This comment has been minimized.

@bsalamat

bsalamat Mar 14, 2018

Contributor

Please rename to AppendNoScheduleTolerationIfNotExist.

func AppendUnscheduableTaintIfNotExist(tolerations []v1.Toleration) []v1.Toleration {
unscheduableTaintExist := false
for _, t := range tolerations {
if t.Key == algorithm.TaintNodeUnscheduable {

This comment has been minimized.

@bsalamat

bsalamat Mar 14, 2018

Contributor

Just the key being equal to the taint key does not necessarily mean that the toleration is the right one that we need. It could have an operator or a different effect. We should check if the key is the same, operator is "Exists" and effect is "NoSchedule".

This comment has been minimized.

@yastij

yastij Mar 14, 2018

Member

@bsalamat - we should create a function under util/toleration that checks if a toleration exists in a slice of tolerations, IIRC we’re doing the same for taints.

if !unscheduableTaintExist {
tolerations = append(tolerations, v1.Toleration{
Key: algorithm.TaintNodeUnscheduable,

This comment has been minimized.

@bsalamat

bsalamat Mar 14, 2018

Contributor

I think we need to set operator to "Exists" as well.

// TaintNodeUnscheduable will be added when node becomes unscheduable
// and feature-gate for TaintNodesByCondition flag is enabled,
// and removed when node becomes scheduable.
TaintNodeUnscheduable = "node.kubernetes.io/unscheduable"

This comment has been minimized.

@bsalamat

bsalamat Mar 14, 2018

Contributor

s/TaintNodeUnscheduable/TaintNodeUnschedulable

s/unscheduable/unschedulable

This comment has been minimized.

@janetkuo

janetkuo Mar 14, 2018

Member

good catch!

@krzyzacy

This comment has been minimized.

Member

krzyzacy commented Mar 14, 2018

/milestone v1.10

@jdumars

This comment has been minimized.

Member

jdumars commented Mar 14, 2018

/sig node

// AppendUnscheduableTaintIfNotExist appends unscheduable toleration to `.spec` if not exist; otherwise,
// no changes to `.spec.tolerations`.
func AppendUnscheduableTaintIfNotExist(tolerations []v1.Toleration) []v1.Toleration {
unscheduableTaintExist := false

This comment has been minimized.

@dims

dims Mar 14, 2018

Member

typo please rename tounschedulableTaintExist

@dims

This comment has been minimized.

Member

dims commented Mar 14, 2018

@k82cn is this related to #60763 ?

@jdumars

This comment has been minimized.

Member

jdumars commented Mar 14, 2018

@dims see comment from @janetkuo in #61050

@janetkuo

This comment has been minimized.

Member

janetkuo commented Mar 14, 2018

is this related to #60763?

@dims it's not. #60763 is not caused by DaemonSet, see #60763 (comment)

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 14, 2018

@krzyzacy is this release blocking? it looks like this is just changing function behind alpha gates, and is still WIP. Should it move out of the milestone?

edit: I see it is fixing alpha function for the alpha feature CI job

@k82cn k82cn changed the title from WIP: Added unscheduable taint to WIP: Added unschedulable taint Mar 15, 2018

@dims

This comment has been minimized.

Member

dims commented Mar 16, 2018

@mikedanese ping for pkg/controller/daemon/ approval
@gmarek @bowei ping for pkg/controller/nodelifecycle approval

@k82cn

This comment has been minimized.

Member

k82cn commented Mar 16, 2018

/assign @gmarek

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 16, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@bsalamat @gmarek @janetkuo @k82cn

Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help
@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Mar 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 16, 2018

@gmarek

This comment has been minimized.

Member

gmarek commented Mar 16, 2018

LGTM for NodeController.

/approve

@jberkus

This comment has been minimized.

jberkus commented Mar 16, 2018

@mikedanese @janetkuo can one of you approve this PR please?

@dims

This comment has been minimized.

Member

dims commented Mar 16, 2018

@kow3ns does this look good to you too?

@janetkuo

This comment has been minimized.

Member

janetkuo commented Mar 16, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, gmarek, janetkuo, k82cn

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 16, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 16, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit ca02c11 into kubernetes:master Mar 16, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation k82cn authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@bsalamat bsalamat referenced this pull request Mar 16, 2018

Closed

Move Priority and Preemption to Beta #57471

12 of 13 tasks complete

@jberkus jberkus referenced this pull request Mar 16, 2018

Closed

Schedule DaemonSet Pods by default scheduler #59194

7 of 7 tasks complete

@k82cn k82cn deleted the k82cn:k8s_59194_4 branch Mar 16, 2018

@kow3ns kow3ns added this to Backlog in Workloads via automation Mar 19, 2018

@kow3ns kow3ns moved this from Backlog to In Progress in Workloads Mar 19, 2018

@kow3ns kow3ns moved this from In Progress to Done in Workloads Mar 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment