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

Exclude master from LoadBalancer / NodePort #44745

Merged
merged 1 commit into from
Apr 24, 2017
Merged

Exclude master from LoadBalancer / NodePort #44745

merged 1 commit into from
Apr 24, 2017

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Apr 21, 2017 •
edited by enisoc
Loading

The servicecontroller documents that the master is excluded from the
LoadBalancer / NodePort, but this is broken for clusters where we are
using taints for the master (as introduced in 1.6), instead of marking
the master as unschedulable.

This restores the desired documented behaviour, by excluding nodes that
are labeled as masters with the new 1.6 labels, even if they use the new
1.6 taints.

Fix #33884

Exclude nodes labeled as master from LoadBalancer / NodePort; restores documented behaviour.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 21, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 21, 2017
@justinsb
Exclude master from LoadBalancer / NodePort
82d600b 
The servicecontroller documents that the master is excluded from the
LoadBalancer / NodePort, but this is broken for clusters where we are
using taints for the master (as introduced in 1.6), instead of marking
the master as unschedulable.

This restores the desired documented behaviour, by excluding nodes that
are labeled as masters with the new 1.6 labels, even if they use the new
1.6 taints.

Fix kubernetes#33884
@justinsb justinsb force-pushed the lb_recognize_16_unschedulable branch from 0aa7b45 to 82d600b Compare April 21, 2017 02:19
@MrHohn
Copy link
Member

MrHohn commented Apr 21, 2017

/lgtm

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

eghobo commented Apr 21, 2017

@justinsb: are you planning back port it to 1.6 release?

@justinsb
Copy link
Member Author

Surprisingly difficult to see the failure in that test run - I think it is a statefulset, so going to rerun

@k8s-bot cvm gce e2e test this

@justinsb
Copy link
Member Author

@thockin do you think you could have a look and make sure you agree that this is a regression, and if so /approve?

@thockin
Copy link
Member

thockin commented Apr 23, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, justinsb, thockin

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 Apr 23, 2017
@justinsb
Copy link
Member Author

@k8s-bot unit test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 56ea95f into kubernetes:master Apr 24, 2017
@justinsb justinsb added this to the v1.6 milestone Apr 25, 2017
@justinsb
Copy link
Member Author

Going to propose for cherry-pick (cc @eghobo) - it's a nuisance on AWS, but it's actually a serious problem on GCE, where we only open the firewall rule to the nodes and we don't have a health check.... but the master is added. Not sure how this got through e2e.

@justinsb justinsb mentioned this pull request Apr 25, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Apr 25, 2017
Merge pull request #44926 from justinsb/automated-cherry-pick-of-#447…
09614e9 
…45-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #44745

Cherry pick of #44745 on release-1.6.

#44745: Exclude master from LoadBalancer / NodePort
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@jkemp101 jkemp101 mentioned this pull request May 2, 2017
Closed
@Sprinkle
Copy link

Sprinkle commented May 13, 2017 •
edited
Loading

Tried this in 1.6.3 and master node is still added to ELB on AWS.

@justinsb Could it be in servicecontroller.go, createLoadBalancer is calling includeNodeFromNodeList which is still only doing return !node.Spec.Unschedulable?

@justinsb
Copy link
Member Author

@Sprinkle you're absolutely right. Going to open another issue to track this - I have no idea why we have two functions (which is why I want to give it its own issue, in case there is a reason!).

@justinsb justinsb mentioned this pull request May 13, 2017
Closed
@justinsb
Copy link
Member Author

Issue #45772

@jgmize jgmize mentioned this pull request May 15, 2017
Closed
@justinsb justinsb mentioned this pull request Jun 12, 2017
Closed
@jbeda jbeda mentioned this pull request Jun 30, 2017
Closed
@tomfotherby
Copy link

Tried this in 1.7.0 and master nodes are no longer added to ELB on AWS. (Kubernetes installer: https://github.com/kz8s/tack) 👍

@FengyunPan FengyunPan mentioned this pull request Jul 30, 2017
Closed
@MrHohn MrHohn mentioned this pull request Sep 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrHohn MrHohn MrHohn approved these changes

Assignees

@MrHohn MrHohn

@bprashanth bprashanth

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.