Support `STRICT_DNS`, `HealthCheck` for `XdsEndpointGroup` (xDS-endpoint pt 3) #5503

jrhee17 · 2024-03-13T15:28:36Z

This PR attempts to support DNS, Health Checked clusters. This is done by converting ClusterSnapshot to an EndpointGroup via XdsEndpointUtil#convertEndpoints. The rest of the changes are simply refactoring.

Motivation:

There are three major points of change:

XdsEndpointUtil#convertEndpointGroup has been introduced which converts a ClusterSnapshot into an EndpointGroup.

This method essentially converts a ClusterSnapshot into a DnsAddressEndpointGroup or a HealthCheckedEndpointGroup. Note that only basic functionality is supported at this stage.

During this implementation, I found it convenient to refer to a LbEndpoint or LocalityLbEndpoints for a particular Endpoint. For this reason, XdsAttributeKeys, XdsAttributeAssigningEndpointGroup has been introduced so that xDS resource information can be retrieved easily. This is useful later as well, in order to fetch the locality, priority, health, etc. of a particular endpoint.

New data structures have been introduced to match that of envoy's.
Some include:

The nuances are slightly different due to a difference in existing APIs, difference in threading model and lifecycles. However, an effort has been made to keep the implementation relatively similarly looking.

Note that the logic inside SubsetLoadBalancer is changed minimally to keep the changeset small.

The way updates are propagated has changed for XdsEndpointGroup.

ClusterManager now receives updates on the ListenerSnapshot by listening to the provided ListenerRoot. On each snapshot update, a ClusterEntry is created for each ClusterSnapshot. ClusterEntry constructs the EndpointGroup based on the provided ClusterSnapshot and endpoint updates (dns/health checked EndpointGroups may update multiple times).

Whenever 1) A ClusterEntry#endpointGroup's endpoints are updated or 2) ListenerSnapshot is updated, then ClusterManager#notifyListeners calls XdsEndpointGroup#setEndpoints. This will eventually trigger XdsEndpointSelector#selectNow. This will subsequently trigger ClusterManager#selectNow and LoadBalancer#selectNow.

Miscellaneous changes

XdsEndpointGroup implements EndpointGroup directly since custom comparison logic is required for XdsEndpointGroup#updateState
I think we will eventually need to turn Endpoint into an interface, and supported various types of endpoints (e.g. HealthCheckedEndpoint, XdsEndpoint, etc..). For now, I propose to use attributes for simplicity.
Because ClusterManager#notifyListeners is called for any update (i.e. update to the snapshot, DnsAddressEndpointGroup updates, etc..) there is a good chance that empty endpoints will be set. This isn't ideal based on the normal use case where users usually wait for initial endpoints via EndpointGroup#whenComplete. I've set allowEmptyEndpoints = false to be the default.

POC: #5450

Modifications:

Introduce XdsEndpointUtil#convertEndpointGroup, which creates an EndpointGroup based on the ClusterSnapshot. DNS, HealthCheck capabilities are partially supported.
LoadBalancer, ClusterManager, ClusterEntry, PrioritySet, SubsetLoadBalancer are newly introduced.
ClusterManager listens for any updates regarding Endpoints or Snapshots and notifies EndpointGroup if there are any changes. Every time this occurs, LoadBalancer#selectNow is called.

Result:

XdsEndpointGroup now partially supports DNS and health check.

github-actions · 2024-03-13T15:39:46Z

🔍 Build Scan® (commit: `a504ec9`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/chpzppdq5gn2a
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/yqle3lnyr6zlc
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/34tg6srwkjxuc
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/cfnkfasw6mc4k
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/h27zbcqbvknoe
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/neelraqhcgbkg
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/lm46msbfmw6ho
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/3c6asa6xlmkke

jrhee17 · 2024-03-25T03:39:44Z

Ready for review 😄

ikhoon

Overall looks good. Left minor comments.

ikhoon · 2024-03-25T05:54:59Z

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

+            endpoint = Endpoint.of(hostname)
+                               .withIpAddr(socketAddress.getAddress())
+                               .withAttr(XdsAttributesKeys.LB_ENDPOINT_KEY, lbEndpoint)
+                               .withAttr(XdsAttributesKeys.LOCALITY_LB_ENDPOINTS_KEY, localityLbEndpoints);


Q) Should we also set weight for this Endpoint?

ikhoon · 2024-03-25T07:21:14Z

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

+
+    static int endpointWeight(LbEndpoint lbEndpoint) {
+        return lbEndpoint.hasLoadBalancingWeight() ?
+               Math.max(1, lbEndpoint.getLoadBalancingWeight().getValue()) : 1;


Envoy uses 1 as the default weight if unspecified.
https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/endpoint/v3/endpoint_components.proto

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

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

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

ikhoon · 2024-03-25T08:19:06Z

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

+        this.endpoints = ImmutableList.copyOf(endpoints);
+    }
+
+    ClusterLoadAssignment clusterLoadAssignment() {


The code is unused. Did you add it for the future usage?

I eventually will, but I'm committed to splitting PRs so let me remove it 😄

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

codecov · 2024-03-26T13:01:34Z

Codecov Report

Attention: Patch coverage is 71.92982% with 96 lines in your changes are missing coverage. Please review.

Project coverage is 74.07%. Comparing base (7f30250) to head (cdb5d24).
Report is 6 commits behind head on main.

❗ Current head cdb5d24 differs from pull request most recent head a504ec9. Consider uploading reports for the commit a504ec9 to get more accurate results

Files	Patch %	Lines
...rp/armeria/xds/client/endpoint/ClusterManager.java	68.42%	25 Missing and 11 partials ⚠️
...p/armeria/xds/client/endpoint/XdsEndpointUtil.java	61.11%	20 Missing and 15 partials ⚠️
.../armeria/xds/client/endpoint/XdsEndpointGroup.java	79.54%	5 Missing and 4 partials ⚠️
...rmeria/xds/client/endpoint/SubsetLoadBalancer.java	70.37%	4 Missing and 4 partials ⚠️
...corp/armeria/xds/client/endpoint/MetadataUtil.java	76.19%	3 Missing and 2 partials ⚠️
...corp/armeria/xds/client/endpoint/ClusterEntry.java	84.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5503      +/-   ##
============================================
+ Coverage     74.05%   74.07%   +0.01%     
- Complexity    20854    20923      +69     
============================================
  Files          1807     1815       +8     
  Lines         76757    77010     +253     
  Branches       9792     9823      +31     
============================================
+ Hits          56842    57044     +202     
- Misses        15292    15319      +27     
- Partials       4623     4647      +24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ikhoon

Nice work! 🚀🚀

ikhoon · 2024-03-27T07:39:22Z

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

+        endpointGroup.removeListener(this);
+        endpointGroup.removeListener(clusterManager);
+        return endpointGroup.closeAsync();
+    }
+
+    @Override
+    public void close() {
+        endpointGroup.close();


Q) Is endpointGroup.removeListener(...) unnecessary for close()?

ikhoon · 2024-03-27T07:49:42Z

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

+    private State state() {
+        final Map<ClusterSnapshot, ClusterEntry> clusterEntries = this.clusterEntries;
+        if (clusterEntries.isEmpty()) {
+            return new State(listenerSnapshot, Collections.emptyList());


To avoid additional object allocations. EmptyList in JDK creates an empty array when .toArray() is called. In addition, the call path inside ImmutableList.copyOf() is shorter.

Suggested change

return new State(listenerSnapshot, Collections.emptyList());

return new State(listenerSnapshot, ImmutableList.of());

minwoox

Left only minor suggestion.
Looks great!

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

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

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

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

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

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

minwoox

Looks great! 💯 💯 💯

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

…sterManager.java Co-authored-by: minux <[email protected]>

minwoox · 2024-05-30T05:06:23Z

...c/main/java/com/linecorp/armeria/xds/client/endpoint/XdsAttributeAssigningEndpointGroup.java

+    public void accept(List<Endpoint> endpoints) {
+        final List<Endpoint> mappedEndpoints =
+                endpoints.stream()
+                         .map(endpoint -> endpoint.withAttr(LB_ENDPOINT_KEY, lbEndpoint)
+                                                  .withAttr(LOCALITY_LB_ENDPOINTS_KEY, localityLbEndpoints)
+                                                  .withWeight(XdsEndpointUtil.endpointWeight(lbEndpoint)))
+                         .collect(Collectors.toList());
+        setEndpoints(mappedEndpoints);
+    }


@jrhee17 Is this method necessary?

I believe so.

The objective of XdsAttributeAssigningEndpointGroup is to add attributes to Endpoints from the delegate.

For this reason, the delegate is listened to:

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

Line 43 in 01f2c6f

delegate.addListener(this, true);

and then setEndpoints is called.

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

Line 54 in 01f2c6f

setEndpoints(mappedEndpoints);

Having said this, if you believe this method is unnecessary you're more than welcome to try to remove it and upload a patch

Ah, it's just that I couldn't find where the accept is called. 😓 Thanks for the explanation.

github-actions bot requested review from ikhoon, minwoox and trustin March 13, 2024 15:28

jrhee17 force-pushed the feature/xds-endpoints-3 branch from c980e73 to 9ff1c74 Compare March 13, 2024 15:37

jrhee17 changed the title ~~Support STRICT_DNS, HealthCheck for XdsEndpointGroup ((xDS-endpoint pt 3)~~ Support STRICT_DNS, HealthCheck for XdsEndpointGroup (xDS-endpoint pt 3) Mar 14, 2024

jrhee17 force-pushed the feature/xds-endpoints-3 branch 4 times, most recently from 07f880a to 874016a Compare March 14, 2024 14:14

jrhee17 added the new feature label Mar 14, 2024

jrhee17 added this to the 1.28.0 milestone Mar 14, 2024

jrhee17 marked this pull request as ready for review March 14, 2024 15:11

jrhee17 force-pushed the feature/xds-endpoints-3 branch from 874016a to 2ff20b5 Compare March 15, 2024 01:48

minwoox mentioned this pull request Mar 22, 2024

Migrate XdsEndpointGroup to client.endpoint (xDS-endpoint pt 2) #5502

Merged

introduce ClusterManager

d39ec69

jrhee17 force-pushed the feature/xds-endpoints-3 branch from 2ff20b5 to d39ec69 Compare March 25, 2024 03:09

ikhoon reviewed Mar 25, 2024

View reviewed changes

address comments by @ikhoon

ef485bc

ikhoon approved these changes Mar 27, 2024

View reviewed changes

jrhee17 added 2 commits March 28, 2024 07:58

Merge branch 'main' into feature/xds-endpoints-3

bb7cc5c

address comments by @ikhoon

cdb5d24

minwoox reviewed Mar 28, 2024

View reviewed changes

jrhee17 added 2 commits April 1, 2024 18:41

address comments by @minwoox

4159627

address comment by @minwoox

f0c5084

minwoox approved these changes Apr 1, 2024

View reviewed changes

xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/ClusterManager.java Outdated Show resolved Hide resolved

Update xds/src/main/java/com/linecorp/armeria/xds/client/endpoint/Clu…

a504ec9

…sterManager.java Co-authored-by: minux <[email protected]>

jrhee17 merged commit 2cc50a1 into line:main Apr 2, 2024
16 checks passed

minwoox reviewed May 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `STRICT_DNS`, `HealthCheck` for `XdsEndpointGroup` (xDS-endpoint pt 3) #5503

Support `STRICT_DNS`, `HealthCheck` for `XdsEndpointGroup` (xDS-endpoint pt 3) #5503

jrhee17 commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

jrhee17 commented Mar 25, 2024

ikhoon left a comment

ikhoon Mar 25, 2024

jrhee17 Mar 26, 2024

ikhoon Mar 25, 2024

ikhoon Mar 25, 2024

jrhee17 Mar 26, 2024

codecov bot commented Mar 26, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Mar 27, 2024

ikhoon Mar 27, 2024

minwoox left a comment

minwoox left a comment

minwoox May 30, 2024

jrhee17 May 30, 2024

jrhee17 May 30, 2024

minwoox May 30, 2024

	return new State(listenerSnapshot, Collections.emptyList());
	return new State(listenerSnapshot, ImmutableList.of());

Support STRICT_DNS, HealthCheck for XdsEndpointGroup (xDS-endpoint pt 3) #5503

Support STRICT_DNS, HealthCheck for XdsEndpointGroup (xDS-endpoint pt 3) #5503

Conversation

jrhee17 commented Mar 13, 2024 • edited Loading

github-actions bot commented Mar 13, 2024 • edited Loading

🔍 Build Scan® (commit: a504ec9)

jrhee17 commented Mar 25, 2024

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2024 • edited Loading

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support `STRICT_DNS`, `HealthCheck` for `XdsEndpointGroup` (xDS-endpoint pt 3) #5503

Support `STRICT_DNS`, `HealthCheck` for `XdsEndpointGroup` (xDS-endpoint pt 3) #5503

jrhee17 commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

🔍 Build Scan® (commit: `a504ec9`)

codecov bot commented Mar 26, 2024 •

edited

Loading