Skip to content

New balancer implementation: consistent hash subset #3396

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
Merged
merged 1 commit into from
Jan 4, 2019
Merged

New balancer implementation: consistent hash subset #3396

merged 1 commit into from
Jan 4, 2019

Conversation

diegows
Copy link
Contributor

@diegows diegows commented Nov 11, 2018

What this PR does / why we need it:
Add Consistent hash balancing to a subset of nodes. It provides the benefits of consistent hashing, but also providing some load balance between some nodes.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 11, 2018
@diegows
Copy link
Contributor Author

diegows commented Nov 11, 2018

/retest

@ElvinEfendi
Copy link
Member

@diegows any specific reason you're using different packages per annotation? Why do you not use existing internal/ingress/annotations/upstreamhashby/ to parse the two new annotations you're introducing?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 27, 2018
diegows
diegows commented Nov 27, 2018
View reviewed changes
Copy link
Contributor Author

@diegows diegows left a comment

Choose a reason for hiding this comment

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

@diegows any specific reason you're using different packages per annotation? Why do you not use existing internal/ingress/annotations/upstreamhashby/ to parse the two new annotations you're introducing?

No reason, fixed following the coding standards.

@diegows
Copy link
Contributor Author

diegows commented Nov 27, 2018

Thanks for your detailed review @ElvinEfendi. Addressed some of the issues, I hope I can finish everything before the end of the week.

@aledbf
Copy link
Member

aledbf commented Nov 30, 2018

@diegows please squash the commits and rebase from master (we fix several e2e flakiness)

@diegows
Copy link
Contributor Author

diegows commented Dec 9, 2018

@aledbf @ElvinEfendi Squashed and rebased. I've applied all the feedback you suggested, improved the code, added e2e test. Let me know how it looks now. Thanks!

@ElvinEfendi
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 22, 2018
@ElvinEfendi
Copy link
Member

ElvinEfendi commented Jan 2, 2019 •
edited
Loading

@diegows please squash your commits into one, we can then merge this.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2019
@diegows diegows force-pushed the master branch 2 times, most recently from 0765feb to c68cae3 Compare January 3, 2019 04:10
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 3, 2019
@diegows
Consistent hashing to a subset of nodes. It works like consistent hash,
60b9835 
but instead of mapping to a single node, we map to a subset of nodes.
@ElvinEfendi
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: diegows, ElvinEfendi

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-ci-robot k8s-ci-robot merged commit 2c3ce07 into kubernetes:master Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElvinEfendi ElvinEfendi ElvinEfendi left review comments

@aledbf aledbf Awaiting requested review from aledbf

Assignees

@ElvinEfendi ElvinEfendi

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@diegows @ElvinEfendi @aledbf @k8s-ci-robot