Improve xDS health check #5785

jrhee17 · 2024-06-26T03:00:21Z

Motivation:

It is recommended to review #5802 prior to this PR

This changeset attempts to solve several problems:

Custom filter logic

xDS considers all endpoints when computing whether a PrioritySet is in panic state. For instance, if the percentage of unhealthy endpoints exceeds a preconfigured panic threshold, the endpoint selection includes all endpoints regardless of the degraded status. While armeria supports an HealthCheckedEndpointGroup out of the box, it filters out healthy endpoints automatically.

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

Lines 99 to 105 in 72ebfe1

    
           if (lbState.perPriorityPanic().get(priority)) { 
        
               if (prioritySet.failTrafficOnPanic()) { 
        
                   return null; 
        
               } else { 
        
                   return new HostsSource(priority, SourceType.ALL_HOSTS); 
        
               } 
        
           }

ref: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/panic_threshold

In order to resolve this, I propose that a AbstractHealthCheckedEndpointGroupBuilder#healthCheckedEndpointPredicate API is added

Per-cluster health check configuration

Per-cluster member health check is difficult with the current API since a single parameter set is statically defined for an entire health checked endpoint group.

ref: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/health_checking#per-cluster-member-health-check-config

We already have an abstraction AbstractHealthCheckedEndpointGroupBuilder#newCheckerFactory. I propose that this API be used for the purpose of xDS. In order to support the parameters xDS allows configuring, I propose that parameters are passed to HttpHealthChecker via the constructor instead of the HealthCheckerContext. In order to support this change, HttpHealthChecker has been moved to an internal package so the xds module can also access it.

Modifications:

Added APIs AbstractHealthCheckedEndpointGroupBuilder#healthCheckedEndpointPredicate and modified HealthCheckedEndpointGroup to filter endpoints based on the predicate.
- Health checked Endpoints now have attributes HEALTHY and DEGRADED set.
- HealthCheckedEndpointGroup#setEndpoints is now called on every invocation of updateHealth, not just when a health status changes.
xDS sometimes needs to override the health checked Endpoint, so modified to receive health checked parameters from the HttpHealthChecker constructor instead of HealthCheckerContext.
- In the process, HttpHealthChecker has been moved to an internal package
- HealthCheckerContext#originalEndpoint has been added to retrieve the original endpoint. The other APIs haven't been deprecated since it is potentially useful information that other HealthCheckers may be using.
Introduced a XdsHealthCheckedEndpointGroupBuilder which implements its own checkerFactory.
EndpointUtil#coarseHealth has been updated to consider HEALTHY, DEGRADED attributes when determining health.

Result:

The behavior of XdsEndpointGroup is more aligned with envoy in terms of health checking
Preparation for zone-aware load balancing

github-actions · 2024-06-26T03:43:01Z

🔍 Build Scan® (commit: `0ae3789`)

Job name	Status	Build Scan®
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/vtgtvqk2ryxde
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/lsxzwaxry5gek
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/6y6j2wenyqrum
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/eecrpuk5naj4c
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/hye3e2lgci4qq
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/dns2eeungr3xy
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/qlrls3iw3xhyu

trustin

Just some nits 😄

trustin · 2024-06-26T06:48:08Z

core/src/main/java/com/linecorp/armeria/client/Endpoint.java

+     * in {@param newAttributes} has higher precedence.
+     */
+    @SuppressWarnings("unchecked")
+    public Endpoint mergeAttrs(Attributes newAttributes) {


Maybe unrelated, but I noticed that the current withAttrs behavior is somewhat confusing. withAttr(AttributeKey, ...) doesn't replace the entire attributes but withAttrs() does, and yet they both use with prefix. I feel like we need this change first:

Change the semantics of withAttrs() so it doesn't replace all attributes.

Introduce replaceAttrs() that provides the original behavior.

Addressed this, but note that this PR will act as an umbrella PR to let reviewers know the overall direction of the health check changes.
This PR will be split into multiple PRs for more thorough/readable changes as usual

core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckerParams.java

trustin · 2024-06-26T06:54:14Z

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

+     * A dummy implementation to avoid breaking changes in {@link AbstractHealthCheckedEndpointGroupBuilder}.
+     */
+    Function<Endpoint, HealthCheckerParams> paramsFactory() {
+        return endpoint -> new HealthCheckerParamsAdapter() {


What do you think about just always using the same HealthCheckerParams implementation class (or factory method) so we don't generate multiple classes? It looks like a simple value class.

Sure, did a cast instead of the previous method chaining.

Motivation: It seems inconsistent that `Endpoint#withAttr` merges the attribute to the existing attributes, whereas `Endpoint#withAttrs` simply replaces the previous attributes. I propose that we modify the behavior of `Endpoint#withAttrs` to be aligned with `Endpoint#withAttr`. Additionally, in order to support the previous behavior, I propose a new API `Endpoint#replaceAttrs` be added. I've confirmed that there are no uses of this API at least internally. ref: #5785 (comment) Modifications: - Modified the behavior of `Endpoint#withAttrs` to merge attributes - Added a new API `Endpoint#replaceAttrs` to support the previous behavior Result: - More consistent APIs - Preparation for #5785

ikhoon · 2024-08-02T03:48:38Z

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

+     * }</pre>
+     */
+    @UnstableApi
+    public SELF healthCheckedEndpointPredicate(Predicate<Endpoint> healthCheckedEndpointPredicate) {


If this API is used in xDS now, how about adding it after #5741 is merged?
Because I think the health status needs to be exposed explicitly to users to implement this healthCheckedEndpointPredicate.

public SELF healthCheckedEndpointPredicate(BiPredicate<Endpoint, HealthStatus> healthCheckedEndpointPredicate) { ... }

By the way, sorry for the late review for #5741 😅

If this API is used in xDS now, how about adding it after #5741 is merged?

I didn't think #5741 is related to this PR since I didn't think DEGRADED is not really used internally at the moment. I thought the more important benefit of this PR was that UNHEALTHY endpoints are now considered for priority calculation.

Because I think the health status needs to be exposed explicitly to users to implement this healthCheckedEndpointPredicate.

I see. Am I understanding correctly that you prefer this API is not exposed publicly unless HealthCheckedEndpoint is introduced?

I thought the more important benefit of this PR was that UNHEALTHY endpoints are now considered for priority calculation.

I looked over quicky your PR. Now I understood your purpose.

you prefer this API is not exposed publicly unless HealthCheckedEndpoint is introduced?

I thought at least an utility API should be provided to check the health status of an Endpoint if it takes some time to design HealthCheckedEndpoint.

It seems like no one will be using this API right now, so it is okay to add it. But changes for HealthCheckedEndpoint would be needed in a f/u work to make this API useful to normal users.

ikhoon · 2024-08-02T03:52:35Z

.../main/java/com/linecorp/armeria/client/endpoint/healthcheck/DefaultHealthCheckerContext.java

@@ -89,6 +94,8 @@ final class DefaultHealthCheckerContext
        this.clientOptions = clientOptions;
        this.retryBackoff = retryBackoff;
        this.onUpdateHealth = onUpdateHealth;
+        endpointAttributes = Attributes.of(HEALTHY_ATTR, false,
+                                           DEGRADED_ATTR, false);


Should we cache 4 Attributes.of(HEALTHY_ATTR, false|true, DEGRADED_ATTR, false|true) in a static map and reuse it?

ikhoon · 2024-08-02T05:18:53Z

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

+     */
+    @UnstableApi
+    public SELF initialStateResolver(
+            Function<? super List<Endpoint>, ? extends List<Endpoint>> initialStateResolver) {


Optional) What do you think of changing this method to take an Endpoint at once? I think this method resembles a combination of map + filter in a role.

initialStateResolver(endpoint -> endpoint) initialStateResolver(endpoint -> null)

I see, since the xDS implementation needs the health check attribute to be retained from filtered initialStateResolver, an AttributeRetainingEndpointGroup has been introduced

Spent way too long debugging an issue related to this. I realized that initialStateResolver can have a race condition with the first update of healthy candidates.

I've just removed initialStateResolver.

ikhoon

Thanks, @jrhee17 🙇‍♂️🙇‍♂️

ikhoon · 2024-08-05T05:31:20Z

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

+     * }</pre>
+     */
+    @UnstableApi
+    public SELF healthCheckedEndpointPredicate(Predicate<Endpoint> healthCheckedEndpointPredicate) {


I thought the more important benefit of this PR was that UNHEALTHY endpoints are now considered for priority calculation.

I looked over quicky your PR. Now I understood your purpose.

you prefer this API is not exposed publicly unless HealthCheckedEndpoint is introduced?

I thought at least an utility API should be provided to check the health status of an Endpoint if it takes some time to design HealthCheckedEndpoint.

It seems like no one will be using this API right now, so it is okay to add it. But changes for HealthCheckedEndpoint would be needed in a f/u work to make this API useful to normal users.

jrhee17 · 2024-08-07T01:35:23Z

This PR is now back to reliably passing the CI

minwoox

Looks fantastic. 👍 👍 👍

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

.../src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckContextGroup.java

...c/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java

core/src/main/java/com/linecorp/armeria/internal/client/endpoint/EndpointAttributeKeys.java

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

...main/java/com/linecorp/armeria/xds/client/endpoint/XdsHealthCheckedEndpointGroupBuilder.java

minwoox

👍 👍 👍 👍 👍

trustin reviewed Jun 26, 2024

View reviewed changes

jrhee17 mentioned this pull request Jul 8, 2024

Modify behavior of Endpoint#withAttrs to merge attributes #5802

Merged

minimal impl

c2b2403

jrhee17 force-pushed the feature/xds-health-check branch from 0ba8906 to c2b2403 Compare July 8, 2024 09:50

jrhee17 added 4 commits July 8, 2024 19:16

ci failure

9d26d71

cache health check information

2c315af

support more xds related hc configs

aacde7c

save progress

e49b406

jrhee17 added the new feature label Jul 9, 2024

jrhee17 added this to the 1.30.0 milestone Jul 9, 2024

jrhee17 added 2 commits July 9, 2024 13:20

minor fixes

a2aaf2e

nit

370e1eb

jrhee17 marked this pull request as ready for review July 9, 2024 05:44

jrhee17 requested review from ikhoon and minwoox as code owners July 9, 2024 05:44

jrhee17 added 2 commits July 10, 2024 09:59

revert unnecessary changes

f12f4dd

minor fixes

d9bfffd

ikhoon reviewed Aug 2, 2024

View reviewed changes

jrhee17 added 4 commits August 5, 2024 10:40

Merge remote-tracking branch 'origin/main' into feature/xds-health-check

78eadf1

fix merge failure

2e02285

cache attributes

9a83aef

add attribute retaining layer

9798317

ikhoon approved these changes Aug 5, 2024

View reviewed changes

jrhee17 force-pushed the feature/xds-health-check branch 2 times, most recently from 78d9d71 to ee7512c Compare August 6, 2024 06:00

remove initial state resolver

c1b45e5

jrhee17 force-pushed the feature/xds-health-check branch from 1bfc0f4 to c1b45e5 Compare August 6, 2024 09:00

only compare health related attributes

a05b1bb

minwoox reviewed Aug 7, 2024

View reviewed changes

address comments by @minwoox

0ae3789

minwoox approved these changes Aug 7, 2024

View reviewed changes

jrhee17 merged commit c7aca10 into line:main Aug 8, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve xDS health check #5785

Improve xDS health check #5785

jrhee17 commented Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024 •

edited

Loading

trustin left a comment

trustin Jun 26, 2024

jrhee17 Jun 26, 2024

trustin Jun 26, 2024

jrhee17 Jun 26, 2024

ikhoon Aug 2, 2024

jrhee17 Aug 2, 2024

ikhoon Aug 5, 2024

ikhoon Aug 2, 2024

jrhee17 Aug 5, 2024 •

edited

Loading

ikhoon Aug 2, 2024

jrhee17 Aug 5, 2024

jrhee17 Aug 5, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Aug 5, 2024

jrhee17 commented Aug 7, 2024

minwoox left a comment

minwoox left a comment

	if (lbState.perPriorityPanic().get(priority)) {
	if (prioritySet.failTrafficOnPanic()) {
	return null;
	} else {
	return new HostsSource(priority, SourceType.ALL_HOSTS);
	}
	}

Improve xDS health check #5785

Improve xDS health check #5785

Conversation

jrhee17 commented Jun 26, 2024 • edited Loading

Custom filter logic

Per-cluster health check configuration

github-actions bot commented Jun 26, 2024 • edited Loading

🔍 Build Scan® (commit: 0ae3789)

trustin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 commented Aug 7, 2024

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 26, 2024 •

edited

Loading

github-actions bot commented Jun 26, 2024 •

edited

Loading

🔍 Build Scan® (commit: `0ae3789`)

jrhee17 Aug 5, 2024 •

edited

Loading

jrhee17 Aug 5, 2024 •

edited

Loading