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

servicecontroller: use consistent node criteria #45773

Merged
merged 1 commit into from
Jun 12, 2017
Merged

servicecontroller: use consistent node criteria #45773

merged 1 commit into from
Jun 12, 2017

Conversation

justinsb
Copy link
Member

We have two node selection functions: includeNodeFromNodeList and
getNodeConditionPredicate, and the logic is different.

The logic should be the same, so remove includeNodeFromNodeList and just
use getNodeConditionPredicate everywhere.

Fix #45772

servicecontroller: Fix node selection logic on initial LB creation

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2017
@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 May 13, 2017
@justinsb justinsb added area/controller-manager and removed 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. labels May 13, 2017
@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 May 13, 2017
@justinsb
servicecontroller: use consistent node criteria
976e046 
We have two node selection functions: includeNodeFromNodeList and
getNodeConditionPredicate, and the logic is different.

The logic should be the same, so remove includeNodeFromNodeList and just
use getNodeConditionPredicate everywhere.

Fix kubernetes#45772
@justinsb justinsb force-pushed the servicecontroller_harmonize branch from b4e2e6d to 976e046 Compare May 13, 2017 18:07
bowei
bowei reviewed May 16, 2017
View reviewed changes
Copy link
Member

@bowei bowei left a comment

Choose a reason for hiding this comment

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

Look ok to me cc: @freehan

@freehan
Copy link
Contributor

freehan commented May 17, 2017

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2017
@freehan
Copy link
Contributor

freehan commented May 22, 2017

@k8s-bot kops aws e2e test this

@thockin for approval.

@justinsb
Copy link
Member Author

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@justinsb
Copy link
Member Author

ping @thockin - this is pretty nasty ... we have two node selection functions, so LBs are created with a different set of nodes than is selected on updates (IIUC)

@thockin
Copy link
Member

thockin commented May 24, 2017 via email

@bowei
Copy link
Member

bowei commented May 25, 2017

@mikedanese BUILD file change in pkg/controller needs approval

@mikedanese
Copy link
Member

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, justinsb, 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 May 25, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 25, 2017 •
edited
Loading

@justinsb: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
Jenkins kops AWS e2e 976e046 link @k8s-bot kops aws e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wojtek-t
Copy link
Member

@k8s-bot node-e2e test this

@justinsb
Copy link
Member Author

@k8s-bot kops aws e2e test this

@k8s-bot node-e2e test this

(node-e2e I can't even see a failure; aws-e2e looks unrelated but I'm looking into it)

@justinsb
Copy link
Member Author

justinsb commented Jun 1, 2017

@k8s-bot node-e2e test this

@justinsb justinsb added this to the v1.7 milestone Jun 1, 2017
@bowei
Copy link
Member

bowei commented Jun 2, 2017

@k8s-bot node-e2e test this

@justinsb
Copy link
Member Author

justinsb commented Jun 3, 2017

@k8s-bot pull-kubernetes-node-e2e test this

@marun marun added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Jun 8, 2017
@justinsb
Copy link
Member Author

@k8s-bot pull-kubernetes-node-e2e test this

@justinsb justinsb mentioned this pull request Jun 12, 2017
Closed
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b84567a into kubernetes:master Jun 12, 2017
@fejta fejta mentioned this pull request Jun 12, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei bowei left review comments

Assignees

@bowei bowei

@freehan freehan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/controller-manager 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. sig/network Categorizes an issue or PR as relevant to SIG Network. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
10 participants
@justinsb @freehan @thockin @bowei @mikedanese @k8s-github-robot @k8s-ci-robot @wojtek-t @marun @bprashanth