From 10d6002cbda0345d2f3fd1b1d91ce5b36319fb16 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 25 Jul 2024 18:07:11 -0700 Subject: [PATCH] xds: ClusterManagerLB must update child configuration While child LB policies are unlikey to change for each cluster name (RLS returns regular cluster names, so should be unique), and the configuration for CDS policies won't change, RLS configuration can definitely change. --- .../grpc/xds/ClusterManagerLoadBalancer.java | 39 ++++++++++++------- .../ClusterManagerLoadBalancerProvider.java | 31 +++++---------- ...lusterManagerLoadBalancerProviderTest.java | 7 ++-- .../xds/ClusterManagerLoadBalancerTest.java | 24 +++++++++--- 4 files changed, 56 insertions(+), 45 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancer.java b/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancer.java index 6b6d2b81352..6b155545b12 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancer.java +++ b/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancer.java @@ -23,11 +23,11 @@ import com.google.common.base.MoreObjects; import io.grpc.ConnectivityState; import io.grpc.InternalLogId; -import io.grpc.LoadBalancerProvider; +import io.grpc.LoadBalancer; import io.grpc.Status; import io.grpc.SynchronizationContext; import io.grpc.SynchronizationContext.ScheduledHandle; -import io.grpc.internal.ServiceConfigUtil.PolicySelection; +import io.grpc.util.GracefulSwitchLoadBalancer; import io.grpc.util.MultiChildLoadBalancer; import io.grpc.xds.ClusterManagerLoadBalancerProvider.ClusterManagerConfig; import io.grpc.xds.client.XdsLogger; @@ -71,7 +71,10 @@ class ClusterManagerLoadBalancer extends MultiChildLoadBalancer { @Override protected ResolvedAddresses getChildAddresses(Object key, ResolvedAddresses resolvedAddresses, - Object childConfig) { + Object unusedChildConfig) { + ClusterManagerConfig config = (ClusterManagerConfig) + resolvedAddresses.getLoadBalancingPolicyConfig(); + Object childConfig = config.childPolicies.get(key); return resolvedAddresses.toBuilder().setLoadBalancingPolicyConfig(childConfig).build(); } @@ -81,13 +84,12 @@ protected Map createChildLbMap(ResolvedAddresses resolvedA resolvedAddresses.getLoadBalancingPolicyConfig(); Map newChildPolicies = new HashMap<>(); if (config != null) { - for (Entry entry : config.childPolicies.entrySet()) { - ChildLbState child = getChildLbState(entry.getKey()); + for (String key : config.childPolicies.keySet()) { + ChildLbState child = getChildLbState(key); if (child == null) { - child = new ClusterManagerLbState(entry.getKey(), - entry.getValue().getProvider(), entry.getValue().getConfig()); + child = new ClusterManagerLbState(key, GracefulSwitchLoadBalancerFactory.INSTANCE, null); } - newChildPolicies.put(entry.getKey(), child); + newChildPolicies.put(key, child); } } logger.log( @@ -108,8 +110,8 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { resolvedAddresses.getLoadBalancingPolicyConfig(); ClusterManagerConfig lastConfig = (ClusterManagerConfig) lastResolvedAddresses.getLoadBalancingPolicyConfig(); - Map adjChildPolicies = new HashMap<>(config.childPolicies); - for (Entry entry : lastConfig.childPolicies.entrySet()) { + Map adjChildPolicies = new HashMap<>(config.childPolicies); + for (Entry entry : lastConfig.childPolicies.entrySet()) { ClusterManagerLbState state = (ClusterManagerLbState) getChildLbState(entry.getKey()); if (adjChildPolicies.containsKey(entry.getKey())) { if (state.deletionTimer != null) { @@ -202,9 +204,9 @@ private class ClusterManagerLbState extends ChildLbState { @Nullable ScheduledHandle deletionTimer; - public ClusterManagerLbState(Object key, LoadBalancerProvider policyProvider, + public ClusterManagerLbState(Object key, LoadBalancer.Factory policyFactory, Object childConfig) { - super(key, policyProvider, childConfig); + super(key, policyFactory, childConfig); } @Override @@ -237,8 +239,8 @@ class DeletionTask implements Runnable { public void run() { ClusterManagerConfig config = (ClusterManagerConfig) lastResolvedAddresses.getLoadBalancingPolicyConfig(); - Map childPolicies = new HashMap<>(config.childPolicies); - PolicySelection removed = childPolicies.remove(getKey()); + Map childPolicies = new HashMap<>(config.childPolicies); + Object removed = childPolicies.remove(getKey()); assert removed != null; config = new ClusterManagerConfig(childPolicies); lastResolvedAddresses = @@ -276,4 +278,13 @@ public void updateBalancingState(final ConnectivityState newState, } } } + + static final class GracefulSwitchLoadBalancerFactory extends LoadBalancer.Factory { + static final LoadBalancer.Factory INSTANCE = new GracefulSwitchLoadBalancerFactory(); + + @Override + public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { + return new GracefulSwitchLoadBalancer(helper); + } + } } diff --git a/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancerProvider.java index 9c97d3fe966..7a7e16286f8 100644 --- a/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancerProvider.java @@ -26,12 +26,9 @@ import io.grpc.NameResolver.ConfigOrError; import io.grpc.Status; import io.grpc.internal.JsonUtil; -import io.grpc.internal.ServiceConfigUtil; -import io.grpc.internal.ServiceConfigUtil.LbConfig; -import io.grpc.internal.ServiceConfigUtil.PolicySelection; +import io.grpc.util.GracefulSwitchLoadBalancer; import java.util.Collections; import java.util.LinkedHashMap; -import java.util.List; import java.util.Map; import java.util.Objects; import javax.annotation.Nullable; @@ -73,7 +70,7 @@ public String getPolicyName() { @Override public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { - Map parsedChildPolicies = new LinkedHashMap<>(); + Map parsedChildPolicies = new LinkedHashMap<>(); try { Map childPolicies = JsonUtil.getObject(rawConfig, "childPolicy"); if (childPolicies == null || childPolicies.isEmpty()) { @@ -86,27 +83,19 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { return ConfigOrError.fromError(Status.INTERNAL.withDescription( "No config for child " + name + " in cluster_manager LB policy: " + rawConfig)); } - List childConfigCandidates = - ServiceConfigUtil.unwrapLoadBalancingConfigList( - JsonUtil.getListOfObjects(childPolicy, "lbPolicy")); - if (childConfigCandidates == null || childConfigCandidates.isEmpty()) { - return ConfigOrError.fromError(Status.INTERNAL.withDescription( - "No config specified for child " + name + " in cluster_manager Lb policy: " - + rawConfig)); - } LoadBalancerRegistry registry = lbRegistry != null ? lbRegistry : LoadBalancerRegistry.getDefaultRegistry(); - ConfigOrError selectedConfig = - ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, registry); - if (selectedConfig.getError() != null) { - Status error = selectedConfig.getError(); + ConfigOrError childConfig = GracefulSwitchLoadBalancer.parseLoadBalancingPolicyConfig( + JsonUtil.getListOfObjects(childPolicy, "lbPolicy"), registry); + if (childConfig.getError() != null) { + Status error = childConfig.getError(); return ConfigOrError.fromError( Status.INTERNAL .withCause(error.getCause()) .withDescription(error.getDescription()) - .augmentDescription("Failed to select config for child " + name)); + .augmentDescription("Failed to parse config for child " + name)); } - parsedChildPolicies.put(name, (PolicySelection) selectedConfig.getConfig()); + parsedChildPolicies.put(name, childConfig.getConfig()); } } catch (RuntimeException e) { return ConfigOrError.fromError( @@ -122,9 +111,9 @@ public LoadBalancer newLoadBalancer(Helper helper) { } static class ClusterManagerConfig { - final Map childPolicies; + final Map childPolicies; - ClusterManagerConfig(Map childPolicies) { + ClusterManagerConfig(Map childPolicies) { this.childPolicies = Collections.unmodifiableMap(childPolicies); } diff --git a/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerProviderTest.java index 515f6fef3ef..40943658520 100644 --- a/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerProviderTest.java @@ -26,7 +26,7 @@ import io.grpc.Status; import io.grpc.Status.Code; import io.grpc.internal.JsonParser; -import io.grpc.internal.ServiceConfigUtil.PolicySelection; +import io.grpc.util.GracefulSwitchLoadBalancer; import io.grpc.xds.ClusterManagerLoadBalancerProvider.ClusterManagerConfig; import java.io.IOException; import java.util.Map; @@ -133,10 +133,9 @@ public ConfigOrError parseLoadBalancingPolicyConfig( assertThat(config.childPolicies) .containsExactly( "child1", - new PolicySelection( - lbProviderFoo, fooConfig), + GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(lbProviderFoo, fooConfig), "child2", - new PolicySelection(lbProviderBar, barConfig)); + GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(lbProviderBar, barConfig)); } @Test diff --git a/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerTest.java b/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerTest.java index f55b0d73f79..aa0e205dd8f 100644 --- a/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerTest.java +++ b/xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerTest.java @@ -52,10 +52,11 @@ import io.grpc.SynchronizationContext; import io.grpc.internal.FakeClock; import io.grpc.internal.PickSubchannelArgsImpl; -import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.testing.TestMethodDescriptors; +import io.grpc.util.GracefulSwitchLoadBalancer; import io.grpc.xds.ClusterManagerLoadBalancerProvider.ClusterManagerConfig; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; @@ -288,16 +289,27 @@ private void deliverResolvedAddresses(final Map childPolicies, b .build()); } + // Prevent ClusterManagerLB from detecting different providers even when the configuration is the + // same. + private Map, FakeLoadBalancerProvider> fakeLoadBalancerProviderCache + = new HashMap<>(); + private ClusterManagerConfig buildConfig(Map childPolicies, boolean failing) { - Map childPolicySelections = new LinkedHashMap<>(); + Map childConfigs = new LinkedHashMap<>(); for (String name : childPolicies.keySet()) { String childPolicyName = childPolicies.get(name); Object childConfig = lbConfigInventory.get(name); - PolicySelection policy = - new PolicySelection(new FakeLoadBalancerProvider(childPolicyName, failing), childConfig); - childPolicySelections.put(name, policy); + FakeLoadBalancerProvider lbProvider = + fakeLoadBalancerProviderCache.get(Arrays.asList(childPolicyName, failing)); + if (lbProvider == null) { + lbProvider = new FakeLoadBalancerProvider(childPolicyName, failing); + fakeLoadBalancerProviderCache.put(Arrays.asList(childPolicyName, failing), lbProvider); + } + Object policy = + GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(lbProvider, childConfig); + childConfigs.put(name, policy); } - return new ClusterManagerConfig(childPolicySelections); + return new ClusterManagerConfig(childConfigs); } private static PickResult pickSubchannel(SubchannelPicker picker, String clusterName) {