Fix to support for dividing an upstream cluster into subsets using metadata in XdsEndpointGroup. #5461

minwoox · 2024-02-14T01:26:10Z

Motivation:
Envoy allows configuring subsets for an upstream cluster using metadata. Subsets are defined based on metadata associated with each endpoint, and configurations within lb_subset_config and metadata_match in the Cluster and RouteConfiguration respectively. This enhancement ensures compatibility with Envoy's subset configuration capabilities, facilitating more granular load balancing. Load Balancer Subsets

Modifications:

Introduce functionality in XdsEndpointGroup to divide an upstream cluster into subsets based on metadata.
Add XdsBootstrap.listenerRoot(Listener) to create a listener root with a static listener resource.

Result:

You can now divide an upstream cluster into subsets effectively within XdsEndpointGroup, enabling more sophisticated load balancing configurations.

…tadata in XdsEndpointGroup. Motivation: Envoy allows configuring subsets for an upstream cluster using metadata. Subsets are defined based on metadata associated with each endpoint, and configurations within `lb_subset_config` and `metadata_match` in the `Cluster` and `RouteConfiguration` respectively. This enhancement ensures compatibility with Envoy's subset configuration capabilities, facilitating more granular load balancing. [Load Balancer Subsets](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/subsets) Modifications: - Introduce functionality in `XdsEndpointGroup` to divide an upstream cluster into subsets based on metadata. - Add `XdsBootstrap.listenerRoot(Listener)` to create a listener root with a static listener resource. Result: - You can now divide an upstream cluster into subsets effectively within `XdsEndpointGroup`, enabling more sophisticated load balancing configurations.

github-actions · 2024-02-14T02:33:38Z

🔍 Build Scan® (commit: `7bba312`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/cxsnjhdlc24rs
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/a63jb7t4vedde
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/elmv6i73ee2aw
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/cd4hacaqpindw
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/deucq2nndpyqi
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/3yazlsbutk442
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ga6xvfpnp3tvq
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/b72435zatflkq

jrhee17

Left two minor questions but overall looks good 👍

xds/src/main/java/com/linecorp/armeria/xds/XdsBootstrap.java

xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java

jrhee17 · 2024-02-14T05:36:58Z

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

+                return;
+            }
+            final LbSubsetFallbackPolicy fallbackPolicy = lbSubsetConfig.getFallbackPolicy();
+            if (fallbackPolicy != LbSubsetFallbackPolicy.ANY_ENDPOINT) {


Note: I guess no need to handle this in this PR, but this setting can technically be overridden by LbSubsetSelector#getFallbackPolicy which can make this a false-positive warning.

Yeah, let's handle it in another PR. I didn't implement the feature fully. 😉

xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java

jrhee17 · 2024-02-14T05:50:04Z

One more question (although not directly related to this PR)

I understood that certain projects may need to customize metadata_match and lb_subset_config. I'm curious how we will allow users to configure their own projects (since CD based xDS doesn't support modifying xDS configurations by end-users yet)

jrhee17 · 2024-02-14T06:57:37Z

xds/src/main/java/com/linecorp/armeria/xds/XdsBootstrap.java

+     * in the tree are updated. The {@link HttpConnectionManager} in the {@link Listener} must have either,
+     * {@link HttpConnectionManager#hasRds()} or {@link HttpConnectionManager#hasRouteConfig()}.


Suggested change

* in the tree are updated. The {@link HttpConnectionManager} in the {@link Listener} must have either,

* {@link HttpConnectionManager#hasRds()} or {@link HttpConnectionManager#hasRouteConfig()}.

* in the tree are updated.

Technically, it is perfectly valid to have a Listener without a Route attached to it

Technically, it is perfectly valid to have a Listener without a Route attached to it

Yes, but we cannot use the listener without a Route attached to it and
it raises an exception if it doesn't have it.

armeria/xds/src/main/java/com/linecorp/armeria/xds/ListenerXdsResource.java

Line 56 in e494846

checkArgument(connectionManager.hasRds() || connectionManager.hasRouteConfig(),

xds/src/main/java/com/linecorp/armeria/xds/XdsBootstrap.java

jrhee17

Left a minor question but looks good to me! 👍 Thanks @minwoox 🙇 👍 🙇

jrhee17 · 2024-02-14T09:44:20Z

xds/src/main/java/com/linecorp/armeria/xds/BootstrapListeners.java

+            final StaticResources staticResources = bootstrap.getStaticResources();
+            for (Listener listener: staticResources.getListenersList()) {
+                final ListenerXdsResource listenerResource = ListenerResourceParser.INSTANCE.parse(listener);
+                builder.put(listener.getName(), listenerResource);


note: envoy seems to generate a random name if not present.
I don't think this is an issue for now, but might be something to consider in case anyone files an issue.

ref: https://github.com/envoyproxy/envoy/blob/45b3f9af9d3889f23b4aded7416d92687cc72de0/source/common/listener_manager/listener_manager_impl.cc#L458

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

xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java

ikhoon

Looks good!

xds/src/main/java/com/linecorp/armeria/xds/BootstrapListeners.java

xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java

minwoox · 2024-02-16T02:11:31Z

Let me merge this and send another PR for Mostly static with dynamic EDS.
Thanks a lot for reviewing. 🙇

…tadata in XdsEndpointGroup. (line#5461) Motivation: Envoy allows configuring subsets for an upstream cluster using metadata. Subsets are defined based on metadata associated with each endpoint, and configurations within `lb_subset_config` and `metadata_match` in the `Cluster` and `RouteConfiguration` respectively. This enhancement ensures compatibility with Envoy's subset configuration capabilities, facilitating more granular load balancing. [Load Balancer Subsets](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/subsets) Modifications: - Introduce functionality in `XdsEndpointGroup` to divide an upstream cluster into subsets based on metadata. - Add `XdsBootstrap.listenerRoot(Listener)` to create a listener root with a static listener resource. Result: - You can now divide an upstream cluster into subsets effectively within `XdsEndpointGroup`, enabling more sophisticated load balancing configurations.

minwoox added the defect label Feb 14, 2024

minwoox requested review from ikhoon, jrhee17 and trustin as code owners February 14, 2024 01:26

minwoox added this to the 1.27.2 milestone Feb 14, 2024

jrhee17 reviewed Feb 14, 2024

View reviewed changes

minwoox added 3 commits February 14, 2024 16:49

Address comments from @jrhee17

fe0e8a5

Use static resource

66aa248

remove unused import

0662f68

jrhee17 approved these changes Feb 14, 2024

View reviewed changes

ikhoon approved these changes Feb 15, 2024

View reviewed changes

xds/src/main/java/com/linecorp/armeria/xds/BootstrapListeners.java Outdated Show resolved Hide resolved

xds/src/main/java/com/linecorp/armeria/xds/XdsConverterUtil.java Outdated Show resolved Hide resolved

minwoox added 2 commits February 15, 2024 22:59

Address comments from @jrhee17

584e37a

Address comments from @ikhoon

7bba312

minwoox merged commit 7d2ab64 into line:main Feb 16, 2024
15 of 16 checks passed

minwoox deleted the add_subsetting branch February 16, 2024 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to support for dividing an upstream cluster into subsets using metadata in XdsEndpointGroup. #5461

Fix to support for dividing an upstream cluster into subsets using metadata in XdsEndpointGroup. #5461

minwoox commented Feb 14, 2024

github-actions bot commented Feb 14, 2024 •

edited

jrhee17 left a comment

jrhee17 Feb 14, 2024

minwoox Feb 14, 2024

jrhee17 commented Feb 14, 2024

jrhee17 Feb 14, 2024

minwoox Feb 14, 2024

jrhee17 left a comment

jrhee17 Feb 14, 2024 •

edited

ikhoon left a comment

minwoox commented Feb 16, 2024

		* in the tree are updated. The {@link HttpConnectionManager} in the {@link Listener} must have either,
		* {@link HttpConnectionManager#hasRds()} or {@link HttpConnectionManager#hasRouteConfig()}.

	* in the tree are updated. The {@link HttpConnectionManager} in the {@link Listener} must have either,
	* {@link HttpConnectionManager#hasRds()} or {@link HttpConnectionManager#hasRouteConfig()}.
	* in the tree are updated.

Fix to support for dividing an upstream cluster into subsets using metadata in XdsEndpointGroup. #5461

Fix to support for dividing an upstream cluster into subsets using metadata in XdsEndpointGroup. #5461

Conversation

minwoox commented Feb 14, 2024

github-actions bot commented Feb 14, 2024 • edited

🔍 Build Scan® (commit: 7bba312)

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Feb 14, 2024

Choose a reason for hiding this comment

minwoox Feb 14, 2024

Choose a reason for hiding this comment

jrhee17 commented Feb 14, 2024

jrhee17 Feb 14, 2024

Choose a reason for hiding this comment

minwoox Feb 14, 2024

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Feb 14, 2024 • edited

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Feb 16, 2024

github-actions bot commented Feb 14, 2024 •

edited

🔍 Build Scan® (commit: `7bba312`)

jrhee17 Feb 14, 2024 •

edited