Skip to content

Commit 10d6002

Browse files
ejona86ejona86
committed
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.
1 parent d034a56 commit 10d6002

File tree

4 files changed

+56
-45
lines changed

4 files changed

+56
-45
lines changed

‎xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancer.java

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@
2323
import com.google.common.base.MoreObjects;
2424
import io.grpc.ConnectivityState;
2525
import io.grpc.InternalLogId;
26-
import io.grpc.LoadBalancerProvider;
26+
import io.grpc.LoadBalancer;
2727
import io.grpc.Status;
2828
import io.grpc.SynchronizationContext;
2929
import io.grpc.SynchronizationContext.ScheduledHandle;
30-
import io.grpc.internal.ServiceConfigUtil.PolicySelection;
30+
import io.grpc.util.GracefulSwitchLoadBalancer;
3131
import io.grpc.util.MultiChildLoadBalancer;
3232
import io.grpc.xds.ClusterManagerLoadBalancerProvider.ClusterManagerConfig;
3333
import io.grpc.xds.client.XdsLogger;
@@ -71,7 +71,10 @@ class ClusterManagerLoadBalancer extends MultiChildLoadBalancer {
7171

7272
@Override
7373
protected ResolvedAddresses getChildAddresses(Object key, ResolvedAddresses resolvedAddresses,
74-
Object childConfig) {
74+
Object unusedChildConfig) {
75+
ClusterManagerConfig config = (ClusterManagerConfig)
76+
resolvedAddresses.getLoadBalancingPolicyConfig();
77+
Object childConfig = config.childPolicies.get(key);
7578
return resolvedAddresses.toBuilder().setLoadBalancingPolicyConfig(childConfig).build();
7679
}
7780

@@ -81,13 +84,12 @@ protected Map<Object, ChildLbState> createChildLbMap(ResolvedAddresses resolvedA
8184
resolvedAddresses.getLoadBalancingPolicyConfig();
8285
Map<Object, ChildLbState> newChildPolicies = new HashMap<>();
8386
if (config != null) {
84-
for (Entry<String, PolicySelection> entry : config.childPolicies.entrySet()) {
85-
ChildLbState child = getChildLbState(entry.getKey());
87+
for (String key : config.childPolicies.keySet()) {
88+
ChildLbState child = getChildLbState(key);
8689
if (child == null) {
87-
child = new ClusterManagerLbState(entry.getKey(),
88-
entry.getValue().getProvider(), entry.getValue().getConfig());
90+
child = new ClusterManagerLbState(key, GracefulSwitchLoadBalancerFactory.INSTANCE, null);
8991
}
90-
newChildPolicies.put(entry.getKey(), child);
92+
newChildPolicies.put(key, child);
9193
}
9294
}
9395
logger.log(
@@ -108,8 +110,8 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
108110
resolvedAddresses.getLoadBalancingPolicyConfig();
109111
ClusterManagerConfig lastConfig = (ClusterManagerConfig)
110112
lastResolvedAddresses.getLoadBalancingPolicyConfig();
111-
Map<String, PolicySelection> adjChildPolicies = new HashMap<>(config.childPolicies);
112-
for (Entry<String, PolicySelection> entry : lastConfig.childPolicies.entrySet()) {
113+
Map<String, Object> adjChildPolicies = new HashMap<>(config.childPolicies);
114+
for (Entry<String, Object> entry : lastConfig.childPolicies.entrySet()) {
113115
ClusterManagerLbState state = (ClusterManagerLbState) getChildLbState(entry.getKey());
114116
if (adjChildPolicies.containsKey(entry.getKey())) {
115117
if (state.deletionTimer != null) {
@@ -202,9 +204,9 @@ private class ClusterManagerLbState extends ChildLbState {
202204
@Nullable
203205
ScheduledHandle deletionTimer;
204206

205-
public ClusterManagerLbState(Object key, LoadBalancerProvider policyProvider,
207+
public ClusterManagerLbState(Object key, LoadBalancer.Factory policyFactory,
206208
Object childConfig) {
207-
super(key, policyProvider, childConfig);
209+
super(key, policyFactory, childConfig);
208210
}
209211

210212
@Override
@@ -237,8 +239,8 @@ class DeletionTask implements Runnable {
237239
public void run() {
238240
ClusterManagerConfig config = (ClusterManagerConfig)
239241
lastResolvedAddresses.getLoadBalancingPolicyConfig();
240-
Map<String, PolicySelection> childPolicies = new HashMap<>(config.childPolicies);
241-
PolicySelection removed = childPolicies.remove(getKey());
242+
Map<String, Object> childPolicies = new HashMap<>(config.childPolicies);
243+
Object removed = childPolicies.remove(getKey());
242244
assert removed != null;
243245
config = new ClusterManagerConfig(childPolicies);
244246
lastResolvedAddresses =
@@ -276,4 +278,13 @@ public void updateBalancingState(final ConnectivityState newState,
276278
}
277279
}
278280
}
281+
282+
static final class GracefulSwitchLoadBalancerFactory extends LoadBalancer.Factory {
283+
static final LoadBalancer.Factory INSTANCE = new GracefulSwitchLoadBalancerFactory();
284+
285+
@Override
286+
public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) {
287+
return new GracefulSwitchLoadBalancer(helper);
288+
}
289+
}
279290
}

‎xds/src/main/java/io/grpc/xds/ClusterManagerLoadBalancerProvider.java

Lines changed: 10 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,9 @@
2626
import io.grpc.NameResolver.ConfigOrError;
2727
import io.grpc.Status;
2828
import io.grpc.internal.JsonUtil;
29-
import io.grpc.internal.ServiceConfigUtil;
30-
import io.grpc.internal.ServiceConfigUtil.LbConfig;
31-
import io.grpc.internal.ServiceConfigUtil.PolicySelection;
29+
import io.grpc.util.GracefulSwitchLoadBalancer;
3230
import java.util.Collections;
3331
import java.util.LinkedHashMap;
34-
import java.util.List;
3532
import java.util.Map;
3633
import java.util.Objects;
3734
import javax.annotation.Nullable;
@@ -73,7 +70,7 @@ public String getPolicyName() {
7370

7471
@Override
7572
public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
76-
Map<String, PolicySelection> parsedChildPolicies = new LinkedHashMap<>();
73+
Map<String, Object> parsedChildPolicies = new LinkedHashMap<>();
7774
try {
7875
Map<String, ?> childPolicies = JsonUtil.getObject(rawConfig, "childPolicy");
7976
if (childPolicies == null || childPolicies.isEmpty()) {
@@ -86,27 +83,19 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map<String, ?> rawConfig) {
8683
return ConfigOrError.fromError(Status.INTERNAL.withDescription(
8784
"No config for child " + name + " in cluster_manager LB policy: " + rawConfig));
8885
}
89-
List<LbConfig> childConfigCandidates =
90-
ServiceConfigUtil.unwrapLoadBalancingConfigList(
91-
JsonUtil.getListOfObjects(childPolicy, "lbPolicy"));
92-
if (childConfigCandidates == null || childConfigCandidates.isEmpty()) {
93-
return ConfigOrError.fromError(Status.INTERNAL.withDescription(
94-
"No config specified for child " + name + " in cluster_manager Lb policy: "
95-
+ rawConfig));
96-
}
9786
LoadBalancerRegistry registry =
9887
lbRegistry != null ? lbRegistry : LoadBalancerRegistry.getDefaultRegistry();
99-
ConfigOrError selectedConfig =
100-
ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, registry);
101-
if (selectedConfig.getError() != null) {
102-
Status error = selectedConfig.getError();
88+
ConfigOrError childConfig = GracefulSwitchLoadBalancer.parseLoadBalancingPolicyConfig(
89+
JsonUtil.getListOfObjects(childPolicy, "lbPolicy"), registry);
90+
if (childConfig.getError() != null) {
91+
Status error = childConfig.getError();
10392
return ConfigOrError.fromError(
10493
Status.INTERNAL
10594
.withCause(error.getCause())
10695
.withDescription(error.getDescription())
107-
.augmentDescription("Failed to select config for child " + name));
96+
.augmentDescription("Failed to parse config for child " + name));
10897
}
109-
parsedChildPolicies.put(name, (PolicySelection) selectedConfig.getConfig());
98+
parsedChildPolicies.put(name, childConfig.getConfig());
11099
}
111100
} catch (RuntimeException e) {
112101
return ConfigOrError.fromError(
@@ -122,9 +111,9 @@ public LoadBalancer newLoadBalancer(Helper helper) {
122111
}
123112

124113
static class ClusterManagerConfig {
125-
final Map<String, PolicySelection> childPolicies;
114+
final Map<String, Object> childPolicies;
126115

127-
ClusterManagerConfig(Map<String, PolicySelection> childPolicies) {
116+
ClusterManagerConfig(Map<String, Object> childPolicies) {
128117
this.childPolicies = Collections.unmodifiableMap(childPolicies);
129118
}
130119

‎xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerProviderTest.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
import io.grpc.Status;
2727
import io.grpc.Status.Code;
2828
import io.grpc.internal.JsonParser;
29-
import io.grpc.internal.ServiceConfigUtil.PolicySelection;
29+
import io.grpc.util.GracefulSwitchLoadBalancer;
3030
import io.grpc.xds.ClusterManagerLoadBalancerProvider.ClusterManagerConfig;
3131
import java.io.IOException;
3232
import java.util.Map;
@@ -133,10 +133,9 @@ public ConfigOrError parseLoadBalancingPolicyConfig(
133133
assertThat(config.childPolicies)
134134
.containsExactly(
135135
"child1",
136-
new PolicySelection(
137-
lbProviderFoo, fooConfig),
136+
GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(lbProviderFoo, fooConfig),
138137
"child2",
139-
new PolicySelection(lbProviderBar, barConfig));
138+
GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(lbProviderBar, barConfig));
140139
}
141140

142141
@Test

‎xds/src/test/java/io/grpc/xds/ClusterManagerLoadBalancerTest.java

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,11 @@
5252
import io.grpc.SynchronizationContext;
5353
import io.grpc.internal.FakeClock;
5454
import io.grpc.internal.PickSubchannelArgsImpl;
55-
import io.grpc.internal.ServiceConfigUtil.PolicySelection;
5655
import io.grpc.testing.TestMethodDescriptors;
56+
import io.grpc.util.GracefulSwitchLoadBalancer;
5757
import io.grpc.xds.ClusterManagerLoadBalancerProvider.ClusterManagerConfig;
5858
import java.util.ArrayList;
59+
import java.util.Arrays;
5960
import java.util.Collections;
6061
import java.util.HashMap;
6162
import java.util.LinkedHashMap;
@@ -288,16 +289,27 @@ private void deliverResolvedAddresses(final Map<String, String> childPolicies, b
288289
.build());
289290
}
290291

292+
// Prevent ClusterManagerLB from detecting different providers even when the configuration is the
293+
// same.
294+
private Map<List<Object>, FakeLoadBalancerProvider> fakeLoadBalancerProviderCache
295+
= new HashMap<>();
296+
291297
private ClusterManagerConfig buildConfig(Map<String, String> childPolicies, boolean failing) {
292-
Map<String, PolicySelection> childPolicySelections = new LinkedHashMap<>();
298+
Map<String, Object> childConfigs = new LinkedHashMap<>();
293299
for (String name : childPolicies.keySet()) {
294300
String childPolicyName = childPolicies.get(name);
295301
Object childConfig = lbConfigInventory.get(name);
296-
PolicySelection policy =
297-
new PolicySelection(new FakeLoadBalancerProvider(childPolicyName, failing), childConfig);
298-
childPolicySelections.put(name, policy);
302+
FakeLoadBalancerProvider lbProvider =
303+
fakeLoadBalancerProviderCache.get(Arrays.asList(childPolicyName, failing));
304+
if (lbProvider == null) {
305+
lbProvider = new FakeLoadBalancerProvider(childPolicyName, failing);
306+
fakeLoadBalancerProviderCache.put(Arrays.asList(childPolicyName, failing), lbProvider);
307+
}
308+
Object policy =
309+
GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(lbProvider, childConfig);
310+
childConfigs.put(name, policy);
299311
}
300-
return new ClusterManagerConfig(childPolicySelections);
312+
return new ClusterManagerConfig(childConfigs);
301313
}
302314

303315
private static PickResult pickSubchannel(SubchannelPicker picker, String clusterName) {

0 commit comments

Comments
 (0)