Conversation
Introduce the concept of locality weighted LB (as distinct from zone aware LB) in the docs and a new field in Cluster, locality_weighted_lb, for configuring this behavior. DO NOT SUBMIT until we have the corresponding implementation side merged (1.6.0 release freeze week). Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
In general LGTM. Mainly would like to clarify relation of this and priority failover per our offline convo.
| and adjust the management server assigned weights based on this. For example, | ||
| if locality X is weighted 60%, locality Y at 20% and locality Z at 20%, but | ||
| only half of locality Z's endpoints are healthy, we would weight X/Y/Z as | ||
| 67%/22%/11%. |
There was a problem hiding this comment.
It's a bit unclear to me how health comes into weighting. Can you spell out the math for the reader? I think that would help.
Also, per our offline convo with @alyssawilk, how can we potentially have this same algorithm be used for priority failover selection? I think it would be really nice to have the same heuristics/algorithms used (possibly tunable in the future).
There was a problem hiding this comment.
P.S., I would model the docs after @alyssawilk amazing docs on priority failover: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/load_balancing#priority-levels (and really again related because IMO the algorithm should basically be the same, at least logically, possibly with different knobs).
There was a problem hiding this comment.
Your call, but I found on the loadbalancing PRs that doing the code and docs in parallel made sense, as I coded things up as a combination of how I wanted them and how Envoy affected our internal LB, and it resulted in much less doc thrashing :-P
envoy/api/v2/cds.proto
Outdated
| // <arch_overview_load_balancing_locality_weighted_lb>` is used. Locality weight at | ||
| // a given priority level is computed from a combination of EDS assigned | ||
| // locality weight and known health status of the endpoints within a | ||
| // locality. For example, if locality X is weighted 60%, locality Y at 20% |
There was a problem hiding this comment.
nit: probably not needed to repeat the math example, specially if the arch overview gets longer / more detailed. Up to you. Same below.
envoy/api/v2/cds.proto
Outdated
| // locality. For example, if locality X is weighted 60%, locality Y at 20% | ||
| // and locality Z at 20%, but only half of locality Z's endpoints are | ||
| // healthy, we would weight X/Y/Z as 67%/22%/11%. | ||
| bool locality_weighted_lb = 2; |
There was a problem hiding this comment.
I think this needs to be enum. There could be different locality weighted algorithms that would require to reinterpret weights differently.
There was a problem hiding this comment.
I might even just make this an empty message within a oneof to give us complete config flexibility later (I could imagine the knobs for tuning percentage multipliers, etc. could be tweaked).
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
LGTM. Can you also have @amb67 or @alyssawilk take a look?
Risk Level: Low (only enabled when explicitly configured). Testing: Unit tests for EDS, LoadBalancerImpl and UpstreamImpl. The load stats integration test provides end-to-end validation. Docs Changes: envoyproxy/data-plane-api#565 Release Notes: Signed-off-by: Harvey Tuch <htuch@google.com>
Underlying issue: #2725 Risk Level: Low (only enabled when explicitly configured). Testing: Unit tests for EDS, LoadBalancerImpl and UpstreamImpl. The load stats integration test provides end-to-end validation. Docs Changes: envoyproxy/data-plane-api#565 Release Notes: Signed-off-by: Harvey Tuch <htuch@google.com>
Introduce the concept of locality weighted LB (as distinct from zone
aware LB) in the docs and a new field in Cluster, locality_weighted_lb,
for configuring this behavior.
Signed-off-by: Harvey Tuch htuch@google.com