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.

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

Hierarchical EDS weighted load balancing #1285

Closed
htuch opened this issue Jul 19, 2017 · 10 comments
Closed

Hierarchical EDS weighted load balancing #1285

htuch opened this issue Jul 19, 2017 · 10 comments
Assignees
Labels

Comments

@htuch
Copy link
Member

htuch commented Jul 19, 2017

EDS now expresses weights at both the endpoint and locality level, these need to be combined to produce an effective LB weighting. The issue tracks the implementation work on this.

@htuch htuch added the api/v2 label Jul 19, 2017
@rshriram
Copy link
Member

rshriram commented Sep 3, 2017

Define locality: data center or az ?
Or weighted clusters? If latter, the labels are already chosen in route block.

@alyssawilk
Copy link
Contributor

This includes proper support for all of the load balancers: fixing the 'stickiness' for LeastRequestLoadBalancer, proper weighted round robin for, ring hash, etc.

Once this is done we can remove the suggested load balancing weight limits in the eds config.

@mattklein123 mattklein123 added this to the 1.6.0 milestone Nov 22, 2017
@alyssawilk
Copy link
Contributor

Given #1929 took longer than expected I'm probably not going to be able to pick this one in up in the near future.

@alyssawilk alyssawilk removed their assignment Dec 21, 2017
@mattklein123 mattklein123 added the help wanted Needs help! label Jan 23, 2018
@mattklein123 mattklein123 removed this from the 1.6.0 milestone Jan 23, 2018
@htuch htuch removed the help wanted Needs help! label Mar 19, 2018
@htuch htuch self-assigned this Mar 19, 2018
@mpuncel
Copy link
Contributor

mpuncel commented Apr 12, 2018

is this a duplicate of #2725 ?

@alyssawilk
Copy link
Contributor

Not quite? For example I don't think we want to remove suggested weight limits for eds until/unless we refactor/replace LeastRequestLoadBalancer

@htuch
Copy link
Member Author

htuch commented Apr 12, 2018

I don't think we're going to be removing the weight limits, given how the hierarchical Maglev balancer works today. I think we should close this one out, as the work is complete, feel free to open additional tracking issues for point features.

@htuch htuch closed this as completed Apr 12, 2018
@ZackButcher
Copy link

Should we update the comment in the API about this issue given it's been closed?

// .. attention::
//
// The limit of 128 is somewhat arbitrary, but is applied due to performance
// concerns with the current implementation and can be removed when
// `this issue <https://github.com/envoyproxy/envoy/issues/1285>`_ is fixed.
google.protobuf.UInt32Value load_balancing_weight = 3

@htuch
Copy link
Member Author

htuch commented Jul 31, 2018

Agree, PRs welcome :)

@ZackButcher
Copy link

The resolution is not clear to me from this issue given it was closed with no changes. Based on current LB impl, the limit of 128 sticks around due to perf concerns and the reference to this issue should be removed, or have the underlying perf concerns been addressed?

@mattklein123
Copy link
Member

There aren't any perf concerns anymore w/ weighting for both RR and LR. Both have been fixed. I don't recall exactly how Maglev works. TBH I would probably just update the comment to remove the attention link until if/when someone complains that the weight max needs to be larger than 128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants