xds: manage load stats for all clusters in XdsClient #7299

voidzcy · 2020-08-07T05:01:13Z

Mostly an extensive refactoring work, might be awful to review (sorry about that 😢 ).

Currently we are having an issue for xDS routing:

For example, if we have two routes with each of them routing some RPCs to the same cluster, say clusterA. Our current architecture will create two identical CDS policy instances for clusterA (as well as the whole LB subtree). Then each of these two CDS policies' child EDS policy will create its own stats object and pass it to the XdsClient for load reporting. However, this is not what our current implementation expects and causes problem (fails the precondition check, it is completely not the usage of the load report client API).

This really is an issue caused by the current design&implementation contradicts what we previously had. After the RouteAction design is implemented, no more than one CDS policy will be created for the same cluster.

Move the creation of LoadStatsStore (aka, the stats object) into XdsClient (as described in the design for refactoring xDS LB policies). The XdsClient is responsible for managing the lifetime of stats objects.
- This helps mitigate the issue of current LB hierarchy for routing and traffic splitting enabled: there could be multiple CDS/EDS policy instances created for the same cluster. They should use the same stats object for recording loads.
- Also this would be one step towards migrating to use XdsClient for stats object creation as mentioned in the xDS LB policy refactoring design.
Counters for recording loads per locality also need to be reference counted, as each EDS policy for the same cluster will receive endpoints for the same group of localities, they will use the same load counters for recording each locality's loads.

Tests for the consumer side of the stats object is added (see LoadReportClientTest). Tests for the producer side is very poorly written given that LocalityStoreTest and EdsLoadBalancerTest are totally a mess. Hopefully we can clean them up soon.

voidzcy

I added some notes to help understand the change.

xds/src/main/java/io/grpc/xds/LocalityStore.java

voidzcy · 2020-08-07T21:22:43Z

xds/src/main/java/io/grpc/xds/LoadStatsManager.java

+/**
+ * Manages all stats for client side load.
+ */
+final class LoadStatsManager {


Borrowed the idea from Envoy's implementation: https://github.com/envoyproxy/envoy/blob/50ef0945fa2c5da4bff7627c3abf41fdd3b7cffd/source/common/upstream/load_stats_reporter.cc#L17

voidzcy · 2020-08-07T21:25:55Z

xds/src/main/java/io/grpc/xds/LoadReportClient.java

@@ -386,27 +337,6 @@ private void cleanUp() {
    }
  }

-  private final class LoadStatsEntity {


Eliminated this wrapper. Move the interval recording into each LoadStatsStore's implementation. Since each LoadStatsStore represents a "living report", when taking a snapshot for the report, we could "stamp an interval" for the report. Since the report is queried every time for sending an LRS request, it totally makes sense to have the report already stamped with the interval.

xds/src/main/java/io/grpc/xds/LocalityStore.java

xds/src/main/java/io/grpc/xds/EdsLoadBalancer.java

xds/src/main/java/io/grpc/xds/LocalityStore.java

…ecording and reporting flow.

…ter:eds_service loads

xds/src/main/java/io/grpc/xds/EdsLoadBalancer.java

dapengzhang0 · 2020-08-10T18:46:11Z

xds/src/main/java/io/grpc/xds/EdsLoadBalancer.java

@@ -303,16 +302,17 @@ public boolean canHandleEmptyAddressListFromNameResolution() {

      @Override
      public void shutdown() {
+        localityStore.reset();
        if (isReportingLoad) {


Where is isReportingLoad set true? I know is hard to test load report in EdsLoadBalancer because EdsLoadBalancerTest uses a real XdsClientImpl with fake channel.

Oops, forgot to put that back. Basically, nothing has changed in terms of the control flow. Only added XdsClient APIs for creating stats objects.

dapengzhang0 · 2020-08-10T18:49:11Z

xds/src/main/java/io/grpc/xds/LoadStatsManager.java

+      loadStatsStores.put(cluster, new HashMap<String, RefCounted>());
+    }
+    Map<String, RefCounted> clusterLoadStatsStores = loadStatsStores.get(cluster);
+    if (!clusterLoadStatsStores.containsKey(clusterService)) {


I don't think it is necessary. Stats objects are always created, and it always succeeds to create. In terms of functionality, there is no difference between creating an stats object by the LB policy or by the XdsClient.

Not necessary for creating/removing stats object. But you have removed some logs, are they supposed to be moved somewhere else?

I deleted logs for "Start/Stop load reporting for cluster: ..., cluster_service: ..." in EDS policy. They are misleading and look awkward:

There is a single load reporting client for all clusters, it is either turned on or off, not for a specific cluster/cluster_service.

Load reporting is completely driven by the management server: which clusters to report load for are determined by the management server, not the gRPC client.

Each EDS policy's log for receiving it LB config includes the information for if this policy enables load reporting. The global load reporting is enabled as long as there is at least one EDS policy enabling it.

Instead, I put logs in XdsClient to indicate when the global load reporting is turned on or off.

xds/src/main/java/io/grpc/xds/EdsLoadBalancer.java

…ce counting.

voidzcy · 2020-08-11T05:13:49Z

Update: not only LoadStatsStore objects need to be reference counted in order for multiple CDS/EDS policies for the same cluster to create them. The counters used to actually record RPC states also need to be reference counted as multiple EDS policies may want to create the same counter for tracking loads sent to the same locality.

@dapengzhang0 PTAL.

voidzcy · 2020-08-11T05:17:00Z

xds/src/main/java/io/grpc/xds/ClientLoadCounter.java

@@ -54,27 +54,10 @@
  private final AtomicLong callsIssued = new AtomicLong();
  private final MetricRecorder[] metricRecorders = new MetricRecorder[THREAD_BALANCING_FACTOR];

-  // True if this counter continues to record stats after next snapshot. Otherwise, it will be
-  // discarded.
-  private boolean active;


As a reminder, active is true if this counter is currently in-use by some LB policy to track loads sent to a specific locality.

We do not mark internally if a counter is still in use by some LB policy, instead ref-count the usage of this counter externally by wrapping a counter with ReferenceCounted.

voidzcy · 2020-08-11T05:17:49Z

xds/src/main/java/io/grpc/xds/ClientLoadCounter.java

-   * Must only be used for testing.
-   */
-  @VisibleForTesting
-  ClientLoadCounter(long callsSucceeded, long callsInProgress, long callsFailed, long callsIssued) {


Replaced by individual setters as the way of testing has changed.

xds/src/main/java/io/grpc/xds/ReferenceCounted.java

dapengzhang0 · 2020-08-11T16:26:46Z

xds/src/main/java/io/grpc/xds/LoadStatsStoreImpl.java

  @Override
  public ClientLoadCounter getLocalityCounter(final Locality locality) {
-    return localityLoadCounters.get(locality);
+    ReferenceCounted<ClientLoadCounter> counter = localityLoadCounters.get(locality);
+    return counter == null || counter.getReferenceCount() == 0 ? null : counter.get();


Why changed this?

Following the same specification of the API as before, if the locality is is not under track, return null. We don't want to break in the middle as that makes this API fragile.

dapengzhang0 · 2020-08-11T16:36:02Z

xds/src/main/java/io/grpc/xds/LoadStatsStoreImpl.java

-    ClientLoadCounter counter = localityLoadCounters.get(locality);
-    checkState(counter == null || !counter.isActive(),
-        "An active counter for locality %s already exists", locality);
+    ReferenceCounted<ClientLoadCounter> counter = localityLoadCounters.get(locality);


What about ClientLoadCounter addLocality(Locality locality)
and remove ClientLoadCounter getLocalityCounter(Locality locality) ?

Yeah, that may make it better. Done.

Move the creation of LoadStatsStore (aka, the stats object) into XdsClient. The XdsClient is responsible for managing the lifetime of stats objects. Creations of LoadStatsStores are reference counted so that multiple EDS policies can retrieve the same stats object for load recording. Counters for recording loads per locality also need to be reference counted, as each EDS policy for the same cluster will receive endpoints for the same group of localities, they will use the same load counters for recording each locality's loads.

…t) (#7299) (#7317) Move the creation of LoadStatsStore (aka, the stats object) into XdsClient. The XdsClient is responsible for managing the lifetime of stats objects. Creations of LoadStatsStores are reference counted so that multiple EDS policies can retrieve the same stats object for load recording. Counters for recording loads per locality also need to be reference counted, as each EDS policy for the same cluster will receive endpoints for the same group of localities, they will use the same load counters for recording each locality's loads.

Move the creation of LoadStatsStore (aka, the stats object) into XdsClient. The XdsClient is responsible for managing the lifetime of stats objects. Creations of LoadStatsStores are reference counted so that multiple EDS policies can retrieve the same stats object for load recording. Counters for recording loads per locality also need to be reference counted, as each EDS policy for the same cluster will receive endpoints for the same group of localities, they will use the same load counters for recording each locality's loads.

voidzcy added 5 commits August 6, 2020 21:54

Introduce LoadStatsManager for managing stats for all clusters.

d2df5c1

Use LoadStatsManager as the source of getting stats for load reporting.

92f53bc

Change the XdsClient API for initiating load reporting.

1e1b00b

Clean up

e2955ea

Delete blank line.

26d39bc

voidzcy requested a review from dapengzhang0 August 7, 2020 09:35

voidzcy commented Aug 7, 2020

View reviewed changes

dapengzhang0 reviewed Aug 7, 2020

View reviewed changes

voidzcy added 6 commits August 7, 2020 17:05

Modify the XdsClient load reporting/recording APIs to separate load r…

887e00c

…ecording and reporting flow.

Revert LocalityStore changes.

58cb691

Elimite the unused LoadReportCallback.

84232bd

style issue

210a803

Delete a poorly written test.

754b69f

Use new XdsClient load reporting/recording APIs for managing per clus…

30d8c17

…ter:eds_service loads

voidzcy requested a review from dapengzhang0 August 10, 2020 17:09

dapengzhang0 reviewed Aug 10, 2020

View reviewed changes

Fixed missing toggling isReportingLoad.

4d9d25e

voidzcy force-pushed the refactor/manage_global_client_stats branch from 04a0fb4 to 4d9d25e Compare August 10, 2020 19:27

voidzcy commented Aug 10, 2020

View reviewed changes

xds/src/main/java/io/grpc/xds/EdsLoadBalancer.java Show resolved Hide resolved

Clean up logs for controling load reporting.

b035d3e

dapengzhang0 approved these changes Aug 10, 2020

View reviewed changes

voidzcy added 4 commits August 10, 2020 21:48

Add a reference counting wrapper for objects.

bd48b69

Eliminate counter's internal reference tracking, use external referen…

84e6579

…ce counting.

Reference count the localities added for load recording.

d35e66e

Use the shared class for reference count wrapper.

801b23d

voidzcy commented Aug 11, 2020

View reviewed changes

dapengzhang0 reviewed Aug 11, 2020

View reviewed changes

Combine add and get load counter method for LoadStatsStore.

6b4199d

dapengzhang0 approved these changes Aug 11, 2020

View reviewed changes

voidzcy merged commit f0a0e67 into grpc:master Aug 11, 2020

voidzcy mentioned this pull request Aug 11, 2020

xds: manage load stats for all clusters in XdsClient (v1.31.x backport) #7314

Closed

This was referenced Aug 12, 2020

xds: manage load stats for all clusters in XdsClient (backport 1.31.x) #7317

Merged

xds: replace verifyNoMoreInteractions with verifyNoInteractions #7320

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: manage load stats for all clusters in XdsClient #7299

xds: manage load stats for all clusters in XdsClient #7299

voidzcy commented Aug 7, 2020 •

edited

voidzcy left a comment

voidzcy Aug 7, 2020

voidzcy Aug 7, 2020

dapengzhang0 Aug 10, 2020

voidzcy Aug 10, 2020

dapengzhang0 Aug 10, 2020

voidzcy Aug 10, 2020

dapengzhang0 Aug 10, 2020

voidzcy Aug 10, 2020 •

edited

voidzcy commented Aug 11, 2020

voidzcy Aug 11, 2020

voidzcy Aug 11, 2020

dapengzhang0 Aug 11, 2020

voidzcy Aug 11, 2020

dapengzhang0 Aug 11, 2020

voidzcy Aug 11, 2020

xds: manage load stats for all clusters in XdsClient #7299

xds: manage load stats for all clusters in XdsClient #7299

Conversation

voidzcy commented Aug 7, 2020 • edited

voidzcy 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

voidzcy Aug 10, 2020 • edited

Choose a reason for hiding this comment

voidzcy commented Aug 11, 2020

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

voidzcy commented Aug 7, 2020 •

edited

voidzcy Aug 10, 2020 •

edited