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

Remove node.alpha.kubernetes.io/ismaster taint from kube-up.sh #43537

Merged
merged 1 commit into from
Mar 23, 2017
Merged

Remove node.alpha.kubernetes.io/ismaster taint from kube-up.sh #43537

merged 1 commit into from
Mar 23, 2017

Conversation

Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented Mar 22, 2017 •
edited

This PR changed master NoSchedule taint to opt-in.

As is discussed with @bgrant0607 @janetkuo, NoSchedule master taint breaks existing user workload, we should not enable it by default.

Previously, NPD required the taint because it can only support one OS distro with a specific configuration. If master and node are using different OS distros, NPD will not work either on master or node. However, we've already fixed this in #40206, so for NPD it's fine to disable the taint.

This should work, but I'll still try it in my cluster to confirm.

@kubernetes/sig-scheduling-misc @dchen1107 @mikedanese

@k8s-reviewable
Copy link

This change is Reviewable

@Random-Liu Random-Liu added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Mar 22, 2017
@Random-Liu Random-Liu added this to the v1.6 milestone Mar 22, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Mar 22, 2017
@Random-Liu Random-Liu force-pushed the disable-master-taint-by-default branch from 214069a to 26e8f00 Compare March 22, 2017 19:57
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2017
@Random-Liu Random-Liu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 22, 2017
@janetkuo
Copy link
Member

This fixes a breaking change, so mark as release blocking.

mikedanese
mikedanese reviewed Mar 22, 2017
View reviewed changes
cluster/gce/configure-vm.sh Outdated
@@ -447,6 +447,7 @@ initial_etcd_cluster_state: '$(echo "${INITIAL_ETCD_CLUSTER_STATE:-}" | sed -e "
ca_cert_bundle_path: '$(echo "${CA_CERT_BUNDLE_PATH:-}" | sed -e "s/'/''/g")'
hostname: $(hostname -s)
enable_default_storage_class: '$(echo "$ENABLE_DEFAULT_STORAGE_CLASS" | sed -e "s/'/''/g")'
enable_master_noschedule_taint: '$(echo "$ENABLE_MASTER_NOSCHEDULE_TAINT" | sed -e "s/'/''/g")'
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary, nodes don't need to know about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK, will remove this.

@mikedanese
Copy link
Member

I'd say just remove the flag, don't add a configuration option.

@mikedanese mikedanese closed this Mar 22, 2017
@mikedanese mikedanese reopened this Mar 22, 2017
davidopp
davidopp reviewed Mar 22, 2017
View reviewed changes
cluster/gce/container-linux/configure-helper.sh Outdated
@@ -516,7 +516,9 @@ function start-kubelet {
if [[ "${REGISTER_MASTER_KUBELET:-false}" == "true" ]]; then
flags+=" --api-servers=https://${KUBELET_APISERVER}"
flags+=" --register-schedulable=false"
flags+=" --register-with-taints=node.alpha.kubernetes.io/ismaster=:NoSchedule"
if [[ "${ENABLE_MASTER_NOSCHEDULE_TAINT:-false}" == "true" ]]; then
flags+=" --register-with-taints=node.alpha.kubernetes.io/ismaster=:NoSchedule"
Copy link
Member

Choose a reason for hiding this comment

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

why was this alpha in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

You reviewed and lgtm'd this taint key :) why should it not be?

Copy link
Member

Choose a reason for hiding this comment

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

Does Kubelet support the beta taint fields in the implementation of this flag?

Copy link
Member

Choose a reason for hiding this comment

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

Does Kubelet support the beta taint fields in the implementation of this flag?

Yes, Kubelet in 1.6 uses field, not annotation.

Copy link
Member

Choose a reason for hiding this comment

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

You reviewed and lgtm'd this taint key

I guess I was thinking this was an experimental feature because we still have Unschedulable. Maybe.

Copy link
Member

Choose a reason for hiding this comment

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

Kubelet uses .node.spec.taints. The alpha indicates that the name of the key is alpha, like we do with label and annotation keys.

Copy link
Member

Choose a reason for hiding this comment

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

If reverting this is necessary, it will be impossible for us to migrate to taints from unscheduable because that would be an unacceptable breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

If reverting this is necessary to revert, it will be impossible for us to migrate to taints from unscheduable because that would be an unacceptable breaking change.

That's why we should make this opt-in, and if needed, we should explain this somewhere to recommend user to enable the taint and gradually add tolerance on their pods, so that in the future we could enable this by default someday.

@Random-Liu
Copy link
Member Author

I'd say just remove the flag, don't add a configuration option.

I think we should eventually enable this, so it would be better to make this opt-in.

@mikedanese
Copy link
Member

What is the roadmap for enabling this permanently? xref #39112

@Random-Liu Random-Liu force-pushed the disable-master-taint-by-default branch from 26e8f00 to 8d864fc Compare March 22, 2017 21:03
@mikedanese
Copy link
Member

mikedanese commented Mar 22, 2017 •
edited

I'm weary of adding permanent disabled configuration bits to turnup without an actually timeline for enabling them. If there's a chance we might want this in the vague hand wavy future, I would prefer we just remove it for now. Can someone make a plan on how to turn this on and have @bgrant0607 sign off?

@calebamiles calebamiles mentioned this pull request Mar 22, 2017
Closed
6 tasks
@calebamiles
Copy link
Contributor

@k8s-bot gci gce e2e test this

@Random-Liu
Copy link
Member Author

Random-Liu commented Mar 22, 2017 •
edited

If there's a chance we might want this in the vague hand wavy future, I would prefer we just remove it for now.

@mikedanese I'm totally fine with completely removing this, but may need to know whether it is ok for @kubernetes/sig-scheduling-misc @kubernetes/sig-cluster-lifecycle-misc

Should we remove the flag completely?

@mikedanese
Copy link
Member

mikedanese commented Mar 22, 2017 •
edited

To be clear: not remove the flag completely (it's used by kubeadm) but don't pass the flag in kube-up and don't add a configuration option to pass the flag.

@davidopp
Copy link
Member

My view is that the scheduling SIG is responsible for the implementation of taints and tolerations, but not how they are used. Usage is the domain of @kubernetes/sig-cluster-lifecycle-misc

My suggestion here is to just make sure @mikedanese and @justinsb buy into whatever you decide.

@Random-Liu Random-Liu force-pushed the disable-master-taint-by-default branch from 8d864fc to 3ab5feb Compare March 22, 2017 22:18
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 22, 2017
@Random-Liu
Copy link
Member Author

Random-Liu commented Mar 22, 2017 •
edited

To be clear: not remove the flag completely (it's used by kubeadm) but don't pass the flag in kube-up and don't add a configuration option to pass the flag.

Sorry, I just mean that. :)

I removed the line setting NoSchedule master taint in kubeup script.

@enisoc
Copy link
Member

enisoc commented Mar 22, 2017

What about in salt?

kubernetes/cluster/saltbase/salt/kubelet/default

Line 36 in a8a8120

{% set master_kubelet_args = master_kubelet_args + "--register-schedulable=false --register-with-taints=node.alpha.kubernetes.io/ismaster=:NoSchedule" -%}

@Random-Liu Random-Liu force-pushed the disable-master-taint-by-default branch from 3ab5feb to 965c262 Compare March 22, 2017 22:35
@Random-Liu
Copy link
Member Author

@enisoc Thanks for pointing out! Missed that, I thought we only enable it on gci/coreos.

Updated the PR.

@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Random-Liu, mikedanese

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 Mar 22, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit c415325 into kubernetes:master Mar 23, 2017
@Random-Liu Random-Liu deleted the disable-master-taint-by-default branch March 23, 2017 01:22
@bgrant0607
Copy link
Member

@Random-Liu Thanks. Please cherrypick this.

@bgrant0607
Copy link
Member

@Random-Liu Please update the PR title, which is no longer correct

@bgrant0607
Copy link
Member

Dan said there is no need to cherrypick yet, because the release branch will be advanced to pick it up.

@bgrant0607 bgrant0607 changed the title Add an env KUBE_ENABLE_MASTER_NOSCHEDULE_TAINT and disable it by default Remove node.alpha.kubernetes.io/ismaster taint from kube-up.sh Mar 23, 2017
@bgrant0607 bgrant0607 mentioned this pull request Mar 31, 2017
Closed
@mikedanese mikedanese mentioned this pull request May 31, 2017
Closed
@MrHohn MrHohn mentioned this pull request Aug 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikedanese mikedanese mikedanese left review comments

@bgrant0607 bgrant0607 bgrant0607 left review comments

@davidopp davidopp davidopp left review comments

Assignees

@mikedanese mikedanese

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-blocker release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@Random-Liu @k8s-reviewable @janetkuo @mikedanese @calebamiles @davidopp @enisoc @k8s-github-robot @bgrant0607 @k8s-ci-robot