Implement locality/priority based load balancing #5610

jrhee17 · 2024-04-15T07:14:31Z

Motivation:

This pull request attempts to implement the default EdfLoadBalancerBase which is the default load balancer used in envoy. Most load balancer implementations in envoy derive from this base class.

The basic load balancer supports locality weighted load balancing and priority levels.

Note that not all functionality (namely zone aware load balancing, etc..) have been implemented for simplicity.
Additionally, SubsetLoadBalancer has not been implemented in this PR for easier reviewing. The previous SubsetLoadBalancer will be replaced in the next (and final) pull request.

This pull request mostly focuses on updating the LoadBalancer state when a ClusterSnapshot is updated.
The flow is as follows:

ClusterEntry#accept is called, which indicates a ClusterSnapshot has been updated
A PriorityStateManager computes endpoints per priority and locality.

https://github.com/envoyproxy/envoy/blob/bdff856f461c2261305b8303285e83d6f2cec627/source/common/upstream/upstream_impl.cc#L2075

A PrioritySet is created. This PrioritySet contains a map of Integer -> HostSet where a HostSet contains host information for each health/locality.

A DefaultLbStateFactory is created which creates a LbState for the DefaultLoadBalancer. A LbState is a convenience object used to avoid potential race conditions. The LbState is replaced atomically from the perspective of DefaultLoadBalancer.

In terms of envoy source, each load balancer registers a callback which listens for HostSet updates. Once a HostSet is updated, the load balancer state is updated.
https://github.com/envoyproxy/envoy/blob/bdff856f461c2261305b8303285e83d6f2cec627/source/common/upstream/load_balancer_impl.cc#L149-L154
Essentially, a range from 1~100 is assigned to each priority where healthy endpoints are prioritized.

Lastly, the load balancer chooses a HostSource, and subsequently a host from the selected HostSet

ref: #5450

Modifications:

ClusterEntry now creates a PrioritySet for each ClusterSnapshot update
- In order to create a PrioritySet, utility classes PriorityStateManager, PriorityState, HostSet, UpdateHostsParam have been created.
- Utility classes EndpointUtil, EndpointGroupUtil have been created
DefaultLoadBalancer has been introduced. The functionality of this LoadBalancer is equivalent to EdfLoadBalancer.
DefaultLbStateFactory has been introduced to facilitate creating a state for DefaultLoadBalancer. The LbState created by DefaultLbStateFactory is intended to be updated atomically from the perspective of DefaultLoadBalancer.

Result:

Priority and locality based load balancing is now available for XdsEndpointGroup

github-actions · 2024-04-15T08:17:41Z

🔍 Build Scan® (commit: `5330c5a`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/drzlq2em4jojk
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/hp5lfcztnj5ym
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/3wozgpztujdws
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/ewd4l3ytrovs6
build-self-hosted-unsafe-jdk-17-min-java-11	❌ (failure)	https://ge.armeria.dev/s/dv664vonxaalg
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/f2bj4pscpzldi
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/dhpdnvrrz36k4
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/tzwje7m344h46

ikhoon

All good! 👍👍

ikhoon · 2024-05-28T12:14:39Z

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLoadBalancer.java

+            return null;
+        }
+        if (!prioritySet.hostSets().containsKey(hostsSource.priority)) {
+            return null;


Question) With the current implementation, does hostsSource.priority always exist so we can't reach here?

Right, let me just throw an exception here to make this clearer 👍

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/EndpointGroupUtil.java

ikhoon · 2024-05-30T05:38:35Z

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/EndpointUtil.java

+
+    static int hash(ClientRequestContext ctx) {
+        if (ctx.hasAttr(XdsAttributesKeys.SELECTION_HASH)) {
+            return ctx.attr(XdsAttributesKeys.SELECTION_HASH);


Question)

Could we ensure that the value set with XdsAttributesKeys.SELECTION_HASH is a non-negative number?

Where does the value of SELECTION_HASH come from? I checked that it is used for testing in this PR. Will it be fetched from a control plane later?

Where does the value of SELECTION_HASH come from? I checked that it is used for testing in this PR. Will it be fetched from a control plane later?

I believe this is only used from tests in upstream as well. I think it's fine to remove this attribute if it is too confusing. Let me know what you think

minwoox

This looks excellent. Thanks a lot!
I left a few suggestions, mostly nits and style issues.

I also noticed that there are no logs at all. If everything goes well, it shouldn't be a problem. However, if something goes wrong, troubleshooting will be very difficult. Could you add trace or debug logs where you think they are necessary?

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/ClusterEntry.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/UpdateHostsParam.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/EndpointUtil.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/HostSet.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLbStateFactory.java

minwoox

This looks excellent. Thanks a lot!
I left a few suggestions, mostly nits and style issues.

I also noticed that there are no logs at all. If everything goes well, it shouldn't be a problem. However, if something goes wrong, troubleshooting will be very difficult. Could you add trace or debug logs where you think they are necessary?

minwoox

This looks excellent. Thanks a lot!
I left a few suggestions, mostly nits and style issues.

I also noticed that there are no logs at all. If everything goes well, it shouldn't be a problem. However, if something goes wrong, troubleshooting will be very difficult. Could you add trace or debug logs where you think they are necessary?

jrhee17 · 2024-06-04T03:17:31Z

Could you add trace or debug logs where you think they are necessary?

Good point. Added logs in the PrioritySet update path, and added more information to the exceptions being thrown when selecting an endpoint.

trustin

Just some minor comments. Looks good!

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLbStateFactory.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/DefaultLoadBalancer.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/HostSet.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/PriorityStateManager.java

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/XdsAttributesKeys.java

trustin

Just some minor comments. Looks good!

trustin

Way to go, @jrhee17! 👍

minwoox

Great work, @jrhee17! Thanks a lot. 👍 👍 👍

minwoox · 2024-06-10T09:06:03Z

👍 👍 👍 👍 👍

jrhee17 added 2 commits April 15, 2024 12:32

minimal impl

d476443

rename to default lb

3ea1678

github-actions bot requested review from ikhoon, minwoox and trustin April 15, 2024 07:14

jrhee17 added the new feature label Apr 15, 2024

jrhee17 added this to the 1.29.0 milestone Apr 15, 2024

jrhee17 marked this pull request as ready for review April 15, 2024 07:39

Merge branch 'main' into feature/default-lb

27d2ed4

ikhoon approved these changes May 30, 2024

View reviewed changes

minwoox reviewed May 30, 2024

View reviewed changes

jrhee17 added 4 commits May 31, 2024 14:29

Merge branch 'main' into feature/default-lb

b727e00

address comments by @ikhoon and @minwoox

e451538

address comment by @ikhoon

597fa27

address comments by @minwoox

bfef1fc

jrhee17 added 3 commits June 5, 2024 17:10

Merge remote-tracking branch 'origin/main' into feature/default-lb

1c5256d

post-merge fix

6bdb731

minor cleanups

741c26d

trustin reviewed Jun 6, 2024

View reviewed changes

trustin mentioned this pull request Jun 6, 2024

Add support for xDS based EndpointGroup #5450

Draft

jrhee17 added 2 commits June 7, 2024 12:13

address comments by @trustin

6e68be2

nit

1969a16

trustin approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'main' into feature/default-lb

5330c5a

minwoox approved these changes Jun 10, 2024

View reviewed changes

minwoox merged commit b47d6c9 into line:main Jun 10, 2024
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement locality/priority based load balancing #5610

Implement locality/priority based load balancing #5610

jrhee17 commented Apr 15, 2024 •

edited

Loading

github-actions bot commented Apr 15, 2024 •

edited

Loading

ikhoon left a comment

ikhoon May 28, 2024

jrhee17 Jun 3, 2024

ikhoon May 30, 2024

jrhee17 Jun 3, 2024

minwoox left a comment

minwoox left a comment

minwoox left a comment

jrhee17 commented Jun 4, 2024

trustin left a comment

trustin left a comment

trustin left a comment

minwoox left a comment

minwoox commented Jun 10, 2024

Implement locality/priority based load balancing #5610

Implement locality/priority based load balancing #5610

Conversation

jrhee17 commented Apr 15, 2024 • edited Loading

github-actions bot commented Apr 15, 2024 • edited Loading

🔍 Build Scan® (commit: 5330c5a)

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon May 28, 2024

Choose a reason for hiding this comment

jrhee17 Jun 3, 2024

Choose a reason for hiding this comment

ikhoon May 30, 2024

Choose a reason for hiding this comment

jrhee17 Jun 3, 2024

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 commented Jun 4, 2024

trustin left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox commented Jun 10, 2024

jrhee17 commented Apr 15, 2024 •

edited

Loading

github-actions bot commented Apr 15, 2024 •

edited

Loading

🔍 Build Scan® (commit: `5330c5a`)