xds: reimplement EDS LB policy with downstream LB config generations that migrate to hierarchical LB tree codepath #7391

voidzcy · 2020-09-03T17:16:06Z

No description provided.

…s that migrates to hierarchical LB tree codepath.

dapengzhang0 · 2020-09-10T23:45:16Z

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

+    @Nullable
+    private final String edsServiceName;
+    @Nullable
+    private final String lrsServerName;  // assume LRS server never change


The comment is not necessary as with edsServiceName

dapengzhang0 · 2020-09-10T23:56:41Z

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

+        if (lrsServerName != null) {
+          loadStatsStore = xdsClient.addClientStats(cluster, edsServiceName);
+          xdsClient.reportClientStats();
+          attributes =


The attribute should be updated each time handleResolvedAddresses().

Okay, I removed the separate caching of attributes. Instead, always populate from the freshest ResolvedAddresses.

dapengzhang0 · 2020-09-11T00:12:17Z

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

+        if (lb != null) {
+          lb.handleNameResolutionError(error);
+        } else {
+          helper.updateBalancingState(TRANSIENT_FAILURE, new ErrorPicker(error));


This is the wrong helper. The right helper should be the helper from the constructor ChildLbState(Helper helper) that the GracefulSwitchingLoadBalancer passes to it, which could be a pendingHelper that does not propagate TRANSIENT_FAILURE upstream. Even forget about the above technical details, the right helper should still be the wrapped helper in the direct layer, and allow that layer to intercept update.

Same for onResourceDoesNotExist() and onError().

Good catch. This is a subtle one. Fixed.

…hould be initialized before watching the endpoint info.

dapengzhang0 · 2020-09-14T23:05:52Z

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

@@ -105,7 +105,9 @@ public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
    if (cluster == null) {
      cluster = config.clusterName;
    }
-    switchingLoadBalancer.switchTo(new EdsLbState(config.edsServiceName, config.lrsServerName));
+    switchingLoadBalancer.switchTo(new EdsLbState(
+        config.edsServiceName, config.lrsServerName, config.localityPickingPolicy,


This looks more than a fix. You changed the definition of EdsLbState, why? But EdsLbState equals()andhashCode()` are not changed accordingly.

That's done purposely. EdsLbState is a LoadBalancer.Factory used for switching during the eds_service_name or lrs_server_name change (aka, replacing cluster). But we don't treat changing endpoint_picking_policy or locality_picking_policy as replacing cluster. So they are not part of equality of EdsLbState. These two fields are mutable and are moved to EdsLbState from ChildLbState to avoid the watcher reentrancy issue: ChildLbState is subscribing to some endpoint resource, while the addWatcher operation can invoke the callback inline (reentrant). So it is too late to set up ChildLbState's endpoint_picking_policy and locality_picking_policy in its handleResolvedAddresses.

I'm not so comfortable having these mutable fields in a Factory. It looks not elegant. Is it possible to move xdsClient.watchEndpointData() in handleResolvedAddresses() to avoid reentrancy issue?

I tried a couple of different approaches, none of them seems to be satisfactory. handleResolvedAddresses is called for multiple times while xdsClient.watchEndpointData() is only called once. Moving it into handleResolvedAddresses is cumbersome. From another aspect, EdsLbState is not just a LoadBalancer.Factory, it implements LoadBalancer.Factory because we have to (because of the usage of GracefulSwitchLoadBalancer API), but it's also representing the state of EdsLoadBalancer: a running instance for one EdsConfig (eds_service_name, lrs_server_name, locality_picking_policy, endpoint_picking_policy). From this perspective, it's not that weird to have endpoint_picking_policy and locality_picking_policy saved there.

Moved endpoint_picking_policy and locality_picking_policy into ChildLbState. Delaying the execution of xdsClient.watchEndpointData() outside ChildLbState's constructor to avoid reentrancy may be problematic, as ChildLbState.shutdown() (which cancels the watcher) may happen before current task in the syncContext returns. So it'd be better to keep watcher addition/cancellation in place. Instead, we can just let onEndpointChanged callback not invoking child policy's handleResolvedAddresses() if such a reentrancy happens. After this callback returns, the following invocation of ChildLbState's handleResolvedAddresses() will get the endpoint_picking_policy and locality_picking_policy and it will invoke child policy's handleResolvedAddresses().

…to_hierarchical_downstream_lb_tree

…hildLbState.

…lrs server for the same cluster (eds_service_name) does not change.

dapengzhang0 · 2020-09-16T23:54:42Z

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

+          loadStatsStore = xdsClient.addClientStats(cluster, edsServiceName);
+          xdsClient.reportClientStats();


Does the order matter?

Doesn't really matter. addClientStats creates the stats object for recording loads while reportClientStats creates the LRS RPC stream and performs load reporting. Since the LRS response handling is always serialized in the sync context, at the time sending the next load reporting request it always sees the newly created stats object for this cluster. So, the order of these two lines doesn't affect anything.

dapengzhang0 · 2020-09-16T23:56:59Z

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

+          PriorityLbConfig config = generatePriorityLbConfig(cluster, edsServiceName,
+              lrsServerName, localityPickingPolicy, endpointPickingPolicy, lbRegistry,
+              prioritizedLocalityWeights);
+          // TODO(chengyuanzhang): to be deleted after migrating to use XdsClient API.
+          Attributes attributes;
+          if (lrsServerName != null) {
+            attributes =
+                resolvedAddresses.getAttributes().toBuilder()
+                    .set(XdsAttributes.ATTR_CLUSTER_SERVICE_LOAD_STATS_STORE, loadStatsStore)
+                    .build();
+          } else {
+            attributes = resolvedAddresses.getAttributes();
+          }
+          lb.handleResolvedAddresses(
+              resolvedAddresses.toBuilder()
+                  .setAddresses(endpointAddresses)
+                  .setAttributes(attributes)
+                  .setLoadBalancingPolicyConfig(config)
+                  .build());
+        }


Is this duplicate code?

Kind of yes. Lines for putting LoadStatsStore into the attribute is temporary and will be deleted after the XdsClient has API to directly access per-locality stats object. Also, there are really only a few statement, but just involving the verbose builder pattern.

However, I also noticed that it would be better to make the latest ResolvedAddresses as a field in ChildLbState, as each ChildLbState instance operates according to a specific address update, so it should be bound with that ResolvedAddresses.

dapengzhang0 · 2020-09-17T00:02:16Z

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

+        if (lb != null) {
+          lb.handleNameResolutionError(error);
+        } else {
+          lbHelper.updateBalancingState(TRANSIENT_FAILURE, new ErrorPicker(error));


It shouldn't be this DropHandlingLbHelper lbHelper either, that's for lb with dropping logic, it's too downstream. The right one should be the helper in ChildLbState(Helper helper).

Yeah, I know. It doesn't matter though. DropHandlingLbHelper is just a the helper received in ChildLbState(Helper helper) with a thin layer wrapped to record drops, which doesn't affect its error propagation functionality.

From the API's perspective, your thought is legit. What about changing to lbHelper.delegate().updateBalancingState(...)? or lbHelper.helper.updateBalancingState(...)

dapengzhang0 · 2020-09-17T02:36:22Z

xds/src/test/java/io/grpc/xds/EdsLoadBalancer2Test.java

+
+    @Override
+    public Subchannel createSubchannel(CreateSubchannelArgs args) {
+      return new FakeSubchannel(args.getAddresses());


Can just return mock(Subchannel.class). The test does not depend on anything of Subchannel

Yeah, right. Fixed.

dapengzhang0 · 2020-09-17T16:54:21Z

xds/src/test/java/io/grpc/xds/EdsLoadBalancer2Test.java

+    LoadBalancerRegistry lbRegistry = LoadBalancerRegistry.getDefaultRegistry();
+    lbRegistry.deregister(lbRegistry.getProvider(fakeLeafPolicyProvider.getPolicyName()));
+    lbRegistry.register(fakeLeafPolicyProvider);
+    loadBalancer.shutdown();
+    loadBalancer = new EdsLoadBalancer2(helper, lbRegistry, mockRandom);
+    PolicySelection weightedTargetSelection =
+        new PolicySelection(lbRegistry.getProvider(WEIGHTED_TARGET_POLICY_NAME), null, null);


An alternative is: deregister all the fake providers in the setUp(), and re-register those policies with real providers. Then the benefit is you don't need to shutdown the balancer and create a new balancer.

registry.deregister(registry.getProvider(PRIORITY_POLICY_NAME)); registry.register(new PriorityLoadBalancerProvider()); registry.deregister(registry.getProvider(LRS_POLICY_NAME)); registry.register(new LrsLoadBalancerProvider()); registry.register(fakeLeafPolicyProvider); PolicySelection weightedTargetSelection = new PolicySelection(new WeightedTargetLoadBalancerProvider(), null, null);

If using real registry, you need clean up the real registry at the end of test.

Fixed to reuse the existing registry with replaced LB providers.

Hmm... It turns out even without

lbRegistry.register(fakeLeafPolicyProvider);

the test could still pass.

Because the leaf policy provider (aka endpoint_picking_policy) is not obtained from the registry, it is obtained from the LB config. So the one in the registry is not used.

dapengzhang0

LGTM

voidzcy · 2020-09-17T20:29:11Z

I am gonna test it before merging.

…d turn into TF is no priority can be used.

…y endpoints.

voidzcy · 2020-09-18T00:16:38Z

The EDS policy is responsible to sieve usable priorities/localities.

I added some missing special case handlings:

Localities with no healthy endpoints should be thrown away and not propagate to downstream policies.
Priorities with no usable localities should be thrown away and not propagate to downstream policies.
Should turn to TF if no usable priority is found.

PTAL.

dapengzhang0 · 2020-09-18T17:08:32Z

LGTM

voidzcy · 2020-09-18T23:06:21Z

The header matching interop test fails consistently as the gRPC client keeps receiving empty EDS responses.

Initially the client receives valid LDS/RDS/CDS responses, but empty EDS responses. After a while, it receives empty CDS responses, and later all LDS/RDS/CDS/EDS responses become empty.

This happens at the initialization stage (aka, setting up the two backend services and waiting endpoints in both backend services to receive RPCs) and the test driver hasn't even apply any header matching specific configurations to the URL map. Interestingly, tests with the same initialization steps (e.g., path matching test) run successfully.

This shouldn't be related to anything done in this change (or even gRPC's implementation). I am merging this and move investigations to the next PR that tries to promote changes in this PR.

…that migrate to hierarchical LB tree codepath (grpc#7391) Implemented the new EDS LB policy, which generates a LB config for instantiating a hierarchical load balancing subtree. The subtree includes downstream LB policies: - priority LB policy, which load balances individual priorities separately - weighted-target LB policy, which load balances individual localities within the same priority separately - lrs LB policy, which applies load recording that aggregates load stats for each locality - leaf LB policy (round_robin) The EDS LB policy is the place that receives endpoint information from traffic director and organizes the data into a model for hierarchical load balancing for the cluster.

voidzcy added 4 commits September 3, 2020 10:11

Add locality_pickinig_policy field to EdsConfig.

db94708

Clean up EdsLoadBalancerProvider test.

309a779

Add missing LB policies for service loading.

79b65a6

Reimplement EDS LB policy with downstream LB policy config generation…

94feec5

…s that migrates to hierarchical LB tree codepath.

voidzcy requested a review from dapengzhang0 September 3, 2020 17:17

voidzcy force-pushed the impl/migrate_to_hierarchical_downstream_lb_tree branch from 2db7335 to ae61247 Compare September 3, 2020 19:01

Fix child balancer's helper not wrapped by GracefulSwitchLoadBalancer.

55606fc

voidzcy force-pushed the impl/migrate_to_hierarchical_downstream_lb_tree branch 2 times, most recently from a6b6206 to 1977109 Compare September 3, 2020 19:25

voidzcy marked this pull request as ready for review September 3, 2020 19:25

Add EDS LB policy tests.

7286d5c

voidzcy force-pushed the impl/migrate_to_hierarchical_downstream_lb_tree branch from 1977109 to 7286d5c Compare September 3, 2020 19:32

voidzcy mentioned this pull request Sep 8, 2020

xds: fail to create xDS channel if no server with supported channel creds found #7400

Merged

dapengzhang0 reviewed Sep 11, 2020

View reviewed changes

voidzcy added 3 commits September 10, 2020 17:51

Delete unnecessary comment.

ef67f6a

Fix to use a single reference of attributes and downstream policies s…

c02bf06

…hould be initialized before watching the endpoint info.

Fix using the wrong helper.

fe8372f

voidzcy force-pushed the impl/migrate_to_hierarchical_downstream_lb_tree branch from 380af00 to fe8372f Compare September 11, 2020 02:05

dapengzhang0 reviewed Sep 14, 2020

View reviewed changes

voidzcy added 4 commits September 16, 2020 13:05

Merge branch 'master' of github.com:grpc/grpc-java into impl/migrate_…

59da20b

…to_hierarchical_downstream_lb_tree

Save current endpoint_picking_policy and locality_picking_policy in C…

ef13175

…hildLbState.

Switch EDS instance only when eds_service_name changes, assuming the …

79b0c92

…lrs server for the same cluster (eds_service_name) does not change.

Remove unnecessary field.

f191f85

voidzcy added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Sep 16, 2020

grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Sep 16, 2020

voidzcy closed this Sep 16, 2020

voidzcy reopened this Sep 16, 2020

dapengzhang0 reviewed Sep 17, 2020

View reviewed changes

voidzcy added 2 commits September 17, 2020 01:41

Use the right Helper precisely.

8444882

Move ResolvedAddresses into each ChildLbState component.

fb32bbe

dapengzhang0 reviewed Sep 17, 2020

View reviewed changes

voidzcy added 2 commits September 17, 2020 10:34

Removed unused FakeSubchannel.

c2dae0f

Reuse existing LoadBalancer registry with real LB policies.

11197c1

dapengzhang0 approved these changes Sep 17, 2020

View reviewed changes

voidzcy added 2 commits September 17, 2020 17:12

Should throw away localities/priorities with no healthy endpoints, an…

8dc07fb

…d turn into TF is no priority can be used.

Add tests to cover throwing away localities/priorities with no health…

c574c5d

…y endpoints.

voidzcy merged commit e6b61ea into grpc:master Sep 18, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: reimplement EDS LB policy with downstream LB config generations that migrate to hierarchical LB tree codepath #7391

xds: reimplement EDS LB policy with downstream LB config generations that migrate to hierarchical LB tree codepath #7391

voidzcy commented Sep 3, 2020 •

edited

dapengzhang0 Sep 10, 2020

voidzcy Sep 11, 2020

dapengzhang0 Sep 10, 2020

voidzcy Sep 11, 2020

dapengzhang0 Sep 11, 2020

voidzcy Sep 11, 2020

dapengzhang0 Sep 14, 2020

voidzcy Sep 14, 2020 •

edited

dapengzhang0 Sep 14, 2020

voidzcy Sep 15, 2020

voidzcy Sep 16, 2020

dapengzhang0 Sep 16, 2020

voidzcy Sep 17, 2020

dapengzhang0 Sep 16, 2020

voidzcy Sep 17, 2020

dapengzhang0 Sep 17, 2020

voidzcy Sep 17, 2020

voidzcy Sep 17, 2020

dapengzhang0 Sep 17, 2020

voidzcy Sep 17, 2020

dapengzhang0 Sep 17, 2020

dapengzhang0 Sep 17, 2020

voidzcy Sep 17, 2020

dapengzhang0 Sep 17, 2020

voidzcy Sep 17, 2020 •

edited

dapengzhang0 left a comment

voidzcy commented Sep 17, 2020

voidzcy commented Sep 18, 2020

dapengzhang0 commented Sep 18, 2020

voidzcy commented Sep 18, 2020

		loadStatsStore = xdsClient.addClientStats(cluster, edsServiceName);
		xdsClient.reportClientStats();

xds: reimplement EDS LB policy with downstream LB config generations that migrate to hierarchical LB tree codepath #7391

xds: reimplement EDS LB policy with downstream LB config generations that migrate to hierarchical LB tree codepath #7391

Conversation

voidzcy commented Sep 3, 2020 • edited

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 Sep 14, 2020 • edited

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

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 Sep 17, 2020 • edited

Choose a reason for hiding this comment

dapengzhang0 left a comment

Choose a reason for hiding this comment

voidzcy commented Sep 17, 2020

voidzcy commented Sep 18, 2020

dapengzhang0 commented Sep 18, 2020

voidzcy commented Sep 18, 2020

voidzcy commented Sep 3, 2020 •

edited

voidzcy Sep 14, 2020 •

edited

voidzcy Sep 17, 2020 •

edited