Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xds: Allow child of cluster_impl LB to change #10091

Merged
merged 3 commits into from
Apr 24, 2023
Merged

xds: Allow child of cluster_impl LB to change #10091

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d50d989
xds: Allow child of cluster_impl LB to change
temawi Apr 21, 2023
e0da90d
Don't check for a change in child policy
temawi Apr 21, 2023
058f0f5
Remove unused field
temawi Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 12 additions & 7 deletions xds/src/main/java/io/grpc/xds/ClusterImplLoadBalancer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import io.grpc.internal.ObjectPool;
import io.grpc.util.ForwardingLoadBalancerHelper;
import io.grpc.util.ForwardingSubchannel;
import io.grpc.util.GracefulSwitchLoadBalancer;
import io.grpc.xds.Bootstrapper.ServerInfo;
import io.grpc.xds.ClusterImplLoadBalancerProvider.ClusterImplConfig;
import io.grpc.xds.Endpoints.DropOverload;
Expand Down Expand Up @@ -87,7 +88,9 @@ final class ClusterImplLoadBalancer extends LoadBalancer {
private CallCounterProvider callCounterProvider;
private ClusterDropStats dropStats;
private ClusterImplLbHelper childLbHelper;
private LoadBalancer childLb;
private GracefulSwitchLoadBalancer childSwitchLb;
@Nullable String childPolicy;
temawi marked this conversation as resolved.
Show resolved Hide resolved


ClusterImplLoadBalancer(Helper helper) {
this(helper, ThreadSafeRandomImpl.instance);
Expand Down Expand Up @@ -120,7 +123,7 @@ public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
childLbHelper = new ClusterImplLbHelper(
callCounterProvider.getOrCreate(config.cluster, config.edsServiceName),
config.lrsServerInfo);
childLb = config.childPolicy.getProvider().newLoadBalancer(childLbHelper);
childSwitchLb = new GracefulSwitchLoadBalancer(childLbHelper);
// Assume load report server does not change throughout cluster lifetime.
if (config.lrsServerInfo != null) {
dropStats = xdsClient.addClusterDropStats(config.lrsServerInfo, cluster, edsServiceName);
Expand All @@ -129,7 +132,9 @@ public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
childLbHelper.updateDropPolicies(config.dropCategories);
childLbHelper.updateMaxConcurrentRequests(config.maxConcurrentRequests);
childLbHelper.updateSslContextProviderSupplier(config.tlsContext);
childLb.handleResolvedAddresses(

childSwitchLb.switchTo(config.childPolicy.getProvider());
childSwitchLb.handleResolvedAddresses(
resolvedAddresses.toBuilder()
.setAttributes(attributes)
.setLoadBalancingPolicyConfig(config.childPolicy.getConfig())
Expand All @@ -139,8 +144,8 @@ public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {

@Override
public void handleNameResolutionError(Status error) {
if (childLb != null) {
childLb.handleNameResolutionError(error);
if (childSwitchLb != null) {
childSwitchLb.handleNameResolutionError(error);
} else {
helper.updateBalancingState(ConnectivityState.TRANSIENT_FAILURE, new ErrorPicker(error));
}
Expand All @@ -151,8 +156,8 @@ public void shutdown() {
if (dropStats != null) {
dropStats.release();
}
if (childLb != null) {
childLb.shutdown();
if (childSwitchLb != null) {
childSwitchLb.shutdown();
if (childLbHelper != null) {
childLbHelper.updateSslContextProviderSupplier(null);
childLbHelper = null;
Expand Down
35 changes: 35 additions & 0 deletions xds/src/test/java/io/grpc/xds/ClusterImplLoadBalancerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,41 @@ public void handleResolvedAddresses_propagateToChildPolicy() {
.isSameInstanceAs(xdsClientPool);
}

/**
* If the control plane switches from using the legacy lb_policy field in the xDS Cluster proto
* to the newer load_balancing_policy then the child policy can switch from weighted_target to
* xds_wrr_locality (this could happen the opposite way as well). This test assures that this
* results in the child LB changing if this were to happen. If this is not done correctly the new
* configuration would be given to the old LB implementation which would cause a channel panic.
*/
@Test
public void handleResolvedAddresses_childPolicyChanges() {
FakeLoadBalancerProvider weightedTargetProvider =
new FakeLoadBalancerProvider(XdsLbPolicies.WEIGHTED_TARGET_POLICY_NAME);
Object weightedTargetConfig = new Object();
ClusterImplConfig configWithWeightedTarget = new ClusterImplConfig(CLUSTER, EDS_SERVICE_NAME,
LRS_SERVER_INFO,
null, Collections.<DropOverload>emptyList(),
new PolicySelection(weightedTargetProvider, weightedTargetConfig), null);
EquivalentAddressGroup endpoint = makeAddress("endpoint-addr", locality);
deliverAddressesAndConfig(Collections.singletonList(endpoint), configWithWeightedTarget);
FakeLoadBalancer childBalancer = Iterables.getOnlyElement(downstreamBalancers);
assertThat(childBalancer.name).isEqualTo(XdsLbPolicies.WEIGHTED_TARGET_POLICY_NAME);
assertThat(childBalancer.config).isSameInstanceAs(weightedTargetConfig);

FakeLoadBalancerProvider wrrLocalityProvider =
new FakeLoadBalancerProvider(XdsLbPolicies.WRR_LOCALITY_POLICY_NAME);
Object wrrLocalityConfig = new Object();
ClusterImplConfig configWithWrrLocality = new ClusterImplConfig(CLUSTER, EDS_SERVICE_NAME,
LRS_SERVER_INFO,
null, Collections.<DropOverload>emptyList(),
new PolicySelection(wrrLocalityProvider, wrrLocalityConfig), null);
deliverAddressesAndConfig(Collections.singletonList(endpoint), configWithWrrLocality);
childBalancer = Iterables.getOnlyElement(downstreamBalancers);
assertThat(childBalancer.name).isEqualTo(XdsLbPolicies.WRR_LOCALITY_POLICY_NAME);
assertThat(childBalancer.config).isSameInstanceAs(wrrLocalityConfig);
}

@Test
public void nameResolutionError_beforeChildPolicyInstantiated_returnErrorPickerToUpstream() {
loadBalancer.handleNameResolutionError(Status.UNIMPLEMENTED.withDescription("not found"));
Expand Down