Make load-balancing extensible #600
Conversation
src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/LoadBalancingConstants.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Service/LoadBalancing/PowerOfTwoChoicesLoadBalancingPolicy.cs
Outdated
Show resolved
Hide resolved
test/ReverseProxy.Tests/Middleware/LoadBalancerMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
| /// A unique identifier for this load balancing policy. This will be referenced from config. | ||
| /// </summary> | ||
| public string Name { get; } | ||
|
|
||
| /// <summary> | ||
| /// Picks a destination to send traffic to. | ||
| /// </summary> | ||
| // TODO: How to ensure retries pick a different destination when available? | ||
| DestinationInfo PickDestination( |
There was a problem hiding this comment.
Thinking out loud: I think we'll want to pass more state here like the route and cluster data, though I'm not sure which type to pass in. Algorithms with options will likely pull those from the cluster metadata. That would mean passing ClusterConfig or IReverseProxyFeature. IReverseProxyFeature might be best since I think that's where we want to expose the route data in the future. Yes they could pull the IReverseProxyFeature from the HttpContext, but that's not discoverable.
What do you think?
There was a problem hiding this comment.
We should definitely keep the availableDestinations parameter.
If we're planning on adding more info onto IReverseProxyFeature, then that should be what we include for discoverability.
Do you think pointing out the GetRequiredProxyFeature extension in load balancing docs would help with discoverability enough to stick with HttpContext's "everything"?
It doesn't seem like we can reason on if/what info custom policies will rely on.
None of the built-in do (except RoundRobin, but that's our choice for state storage), and none of the ones in #107 do.
As you mention, policies with options would likely need that info.
There was a problem hiding this comment.
That's a good point that none of the proposed protocols have this requirement. We can wait to see if it comes up.
src/ReverseProxy/Abstractions/ClusterDiscovery/Contract/LoadBalancingPolicies.cs
Outdated
Show resolved
Hide resolved
Can you complete the docs as part of this PR? |
There shouldn't be any big conflicting code changes coming in to block a merge, so I will push the docs to this PR. |
|
Also check the samples and docs: reverse-proxy/samples/ReverseProxy.Config.Sample/appsettings.json Lines 23 to 25 in e4426b9 |
|
@Tratcher can you PTAL at the added docs? Thanks! |
Fixes #106
Fixes #229
Replace the internal
LoadBalancingMode,ILoadBalancer,LoadBalancerwith a publicILoadBalancingPolicyinterface.Following the pattern used with session affinity,
LoadBalancingMiddlewareresolves available policies from DI and select the current one per-invoke based on cluser config.LoadBalancingOptions(a struct wrapping theLoadBalancingModeenum) is replaced by a stringLoadBalancingPolicy.The 5 existing load balancing strategies are implemented as separate
ILoadBalancingPolicytypes. Power of two choices remains the default.ToDo: docs on load balancing