Skip to content

Commit

Permalink
xds: Remove WRR and PF experimental flags
Browse files Browse the repository at this point in the history 
They have been on by default for a good while, and seem stable. This
also removes some grpc-isms from XdsResourceType.
  • Loading branch information
@ejona86
ejona86 committed Mar 5, 2024
1 parent 2782446 commit 85e52cd
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 71 deletions.
28 changes: 10 additions & 18 deletions xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,14 @@ class LoadBalancerConfigFactory {
*
* @throws ResourceInvalidException If the {@link Cluster} has an invalid LB configuration.
*/
static ImmutableMap<String, ?> newConfig(Cluster cluster, boolean enableLeastRequest,
boolean enableWrr, boolean enablePickFirst)
static ImmutableMap<String, ?> newConfig(Cluster cluster, boolean enableLeastRequest)
throws ResourceInvalidException {
// The new load_balancing_policy will always be used if it is set, but for backward
// compatibility we will fall back to using the old lb_policy field if the new field is not set.
if (cluster.hasLoadBalancingPolicy()) {
try {
return LoadBalancingPolicyConverter.convertToServiceConfig(cluster.getLoadBalancingPolicy(),
0, enableWrr, enablePickFirst);
0);
} catch (MaxRecursionReachedException e) {
throw new ResourceInvalidException("Maximum LB config recursion depth reached", e);
}
Expand Down Expand Up @@ -213,8 +212,7 @@ static class LoadBalancingPolicyConverter {
* Converts a {@link LoadBalancingPolicy} object to a service config JSON object.
*/
private static ImmutableMap<String, ?> convertToServiceConfig(
LoadBalancingPolicy loadBalancingPolicy, int recursionDepth, boolean enableWrr,
boolean enablePickFirst)
LoadBalancingPolicy loadBalancingPolicy, int recursionDepth)
throws ResourceInvalidException, MaxRecursionReachedException {
if (recursionDepth > MAX_RECURSION) {
throw new MaxRecursionReachedException();
Expand All @@ -228,20 +226,16 @@ static class LoadBalancingPolicyConverter {
serviceConfig = convertRingHashConfig(typedConfig.unpack(RingHash.class));
} else if (typedConfig.is(WrrLocality.class)) {
serviceConfig = convertWrrLocalityConfig(typedConfig.unpack(WrrLocality.class),
recursionDepth, enableWrr, enablePickFirst);
recursionDepth);
} else if (typedConfig.is(RoundRobin.class)) {
serviceConfig = convertRoundRobinConfig();
} else if (typedConfig.is(LeastRequest.class)) {
serviceConfig = convertLeastRequestConfig(typedConfig.unpack(LeastRequest.class));
} else if (typedConfig.is(ClientSideWeightedRoundRobin.class)) {
if (enableWrr) {
serviceConfig = convertWeightedRoundRobinConfig(
typedConfig.unpack(ClientSideWeightedRoundRobin.class));
}
serviceConfig = convertWeightedRoundRobinConfig(
typedConfig.unpack(ClientSideWeightedRoundRobin.class));
} else if (typedConfig.is(PickFirst.class)) {
if (enablePickFirst) {
serviceConfig = convertPickFirstConfig(typedConfig.unpack(PickFirst.class));
}
serviceConfig = convertPickFirstConfig(typedConfig.unpack(PickFirst.class));
} else if (typedConfig.is(com.github.xds.type.v3.TypedStruct.class)) {
serviceConfig = convertCustomConfig(
typedConfig.unpack(com.github.xds.type.v3.TypedStruct.class));
Expand Down Expand Up @@ -310,12 +304,10 @@ static class LoadBalancingPolicyConverter {
* Converts a wrr_locality {@link Any} configuration to service config format.
*/
private static ImmutableMap<String, ?> convertWrrLocalityConfig(WrrLocality wrrLocality,
int recursionDepth, boolean enableWrr, boolean enablePickFirst)
throws ResourceInvalidException,
MaxRecursionReachedException {
int recursionDepth)
throws ResourceInvalidException, MaxRecursionReachedException {
return buildWrrLocalityConfig(
convertToServiceConfig(wrrLocality.getEndpointPickingPolicy(),
recursionDepth + 1, enableWrr, enablePickFirst));
convertToServiceConfig(wrrLocality.getEndpointPickingPolicy(), recursionDepth + 1));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion xds/src/main/java/io/grpc/xds/XdsClusterResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ static CdsUpdate processCluster(Cluster cluster,
CdsUpdate.Builder updateBuilder = structOrError.getStruct();

ImmutableMap<String, ?> lbPolicyConfig = LoadBalancerConfigFactory.newConfig(cluster,
enableLeastRequest, enableWrr, enablePickFirst);
enableLeastRequest);

// Validate the LB config by trying to parse it with the corresponding LB provider.
LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(lbPolicyConfig);
Expand Down
6 changes: 0 additions & 6 deletions xds/src/main/java/io/grpc/xds/client/XdsResourceType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,6 @@ public abstract class XdsResourceType<T extends ResourceUpdate> {
? Boolean.parseBoolean(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST"))
: Boolean.parseBoolean(System.getProperty("io.grpc.xds.experimentalEnableLeastRequest"));

@VisibleForTesting
public static boolean enableWrr = getFlag("GRPC_EXPERIMENTAL_XDS_WRR_LB", true);

@VisibleForTesting
protected static boolean enablePickFirst = getFlag("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG", true);

protected static final String TYPE_URL_CLUSTER_CONFIG =
"type.googleapis.com/envoy.extensions.clusters.aggregate.v3.ClusterConfig";
protected static final String TYPE_URL_TYPED_STRUCT_UDPA =
Expand Down
4 changes: 0 additions & 4 deletions xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,22 +170,18 @@ public class GrpcXdsClientImplDataTest {
private final FilterRegistry filterRegistry = FilterRegistry.getDefaultRegistry();
private boolean originalEnableRouteLookup;
private boolean originalEnableLeastRequest;
private boolean originalEnableWrr;

@Before
public void setUp() {
originalEnableRouteLookup = XdsResourceType.enableRouteLookup;
originalEnableLeastRequest = XdsResourceType.enableLeastRequest;
assertThat(originalEnableLeastRequest).isFalse();
originalEnableWrr = XdsResourceType.enableWrr;
assertThat(originalEnableWrr).isTrue();
}

@After
public void tearDown() {
XdsResourceType.enableRouteLookup = originalEnableRouteLookup;
XdsResourceType.enableLeastRequest = originalEnableLeastRequest;
XdsResourceType.enableWrr = originalEnableWrr;
}

@Test
Expand Down
65 changes: 23 additions & 42 deletions xds/src/test/java/io/grpc/xds/LoadBalancerConfigFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,14 @@ public void deregisterCustomProvider() {
public void roundRobin() throws ResourceInvalidException {
Cluster cluster = newCluster(buildWrrPolicy(ROUND_ROBIN_POLICY));

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
}

@Test
public void weightedRoundRobin() throws ResourceInvalidException {
Cluster cluster = newCluster(buildWrrPolicy(WRR_POLICY));

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_WRR_CONFIG);
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_WRR_CONFIG);
}

@Test
Expand All @@ -179,22 +179,15 @@ public void weightedRoundRobin_invalid() throws ResourceInvalidException {
.build())
.build()));

assertResourceInvalidExceptionThrown(cluster, true, true, true,
assertResourceInvalidExceptionThrown(cluster, true,
"Invalid duration in weighted round robin config");
}

@Test
public void weightedRoundRobin_fallback_roundrobin() throws ResourceInvalidException {
Cluster cluster = newCluster(buildWrrPolicy(WRR_POLICY, ROUND_ROBIN_POLICY));

assertThat(newLbConfig(cluster, true, false, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
}

@Test
public void roundRobin_legacy() throws ResourceInvalidException {
Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.ROUND_ROBIN).build();

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
}

@Test
Expand All @@ -203,7 +196,7 @@ public void ringHash() throws ResourceInvalidException {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(RING_HASH_POLICY))
.build();

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_RING_HASH_CONFIG);
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_RING_HASH_CONFIG);
}

@Test
Expand All @@ -213,7 +206,7 @@ public void ringHash_legacy() throws ResourceInvalidException {
.setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE))
.setHashFunction(HashFunction.XX_HASH)).build();

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_RING_HASH_CONFIG);
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_RING_HASH_CONFIG);
}

@Test
Expand All @@ -225,15 +218,15 @@ public void ringHash_invalidHash() {
.setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE))
.setHashFunction(RingHash.HashFunction.MURMUR_HASH_2).build()))).build());

assertResourceInvalidExceptionThrown(cluster, true, true, true, "Invalid ring hash function");
assertResourceInvalidExceptionThrown(cluster, true, "Invalid ring hash function");
}

@Test
public void ringHash_invalidHash_legacy() {
Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.RING_HASH).setRingHashLbConfig(
RingHashLbConfig.newBuilder().setHashFunction(HashFunction.MURMUR_HASH_2)).build();

assertResourceInvalidExceptionThrown(cluster, true, true, true, "invalid ring hash function");
assertResourceInvalidExceptionThrown(cluster, true, "invalid ring hash function");
}

@Test
Expand All @@ -242,7 +235,7 @@ public void leastRequest() throws ResourceInvalidException {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(LEAST_REQUEST_POLICY))
.build();

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_LEAST_REQUEST_CONFIG);
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_LEAST_REQUEST_CONFIG);
}

@Test
Expand All @@ -254,7 +247,7 @@ public void leastRequest_legacy() throws ResourceInvalidException {
LeastRequestLbConfig.newBuilder()
.setChoiceCount(UInt32Value.of(LEAST_REQUEST_CHOICE_COUNT))).build();

LbConfig lbConfig = newLbConfig(cluster, true, true, true);
LbConfig lbConfig = newLbConfig(cluster, true);
assertThat(lbConfig.getPolicyName()).isEqualTo("wrr_locality_experimental");

List<LbConfig> childConfigs = ServiceConfigUtil.unwrapLoadBalancingConfigList(
Expand All @@ -269,7 +262,7 @@ public void leastRequest_legacy() throws ResourceInvalidException {
public void leastRequest_notEnabled() {
Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.LEAST_REQUEST).build();

assertResourceInvalidExceptionThrown(cluster, false, true, true, "unsupported lb policy");
assertResourceInvalidExceptionThrown(cluster, false, "unsupported lb policy");
}

@Test
Expand All @@ -278,24 +271,14 @@ public void pickFirst() throws ResourceInvalidException {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(PICK_FIRST_POLICY))
.build();

assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_PICK_FIRST_CONFIG);
}

@Test
public void pickFirst_notEnabled() throws ResourceInvalidException {
Cluster cluster = Cluster.newBuilder()
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(PICK_FIRST_POLICY))
.build();

assertResourceInvalidExceptionThrown(cluster, true, true, false, "Invalid LoadBalancingPolicy");
assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_PICK_FIRST_CONFIG);
}

@Test
public void customRootLb_providerRegistered() throws ResourceInvalidException {
LoadBalancerRegistry.getDefaultRegistry().register(CUSTOM_POLICY_PROVIDER);

assertThat(newLbConfig(newCluster(CUSTOM_POLICY), false,
true, true)).isEqualTo(VALID_CUSTOM_CONFIG);
assertThat(newLbConfig(newCluster(CUSTOM_POLICY), false)).isEqualTo(VALID_CUSTOM_CONFIG);
}

@Test
Expand All @@ -304,7 +287,7 @@ public void customRootLb_providerNotRegistered() throws ResourceInvalidException
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(CUSTOM_POLICY))
.build();

assertResourceInvalidExceptionThrown(cluster, false, true, true, "Invalid LoadBalancingPolicy");
assertResourceInvalidExceptionThrown(cluster, false, "Invalid LoadBalancingPolicy");
}

// When a provider for the endpoint picking custom policy is available, the configuration should
Expand All @@ -316,7 +299,7 @@ public void customLbInWrr_providerRegistered() throws ResourceInvalidException {
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder()
.addPolicies(buildWrrPolicy(CUSTOM_POLICY, ROUND_ROBIN_POLICY))).build();

assertThat(newLbConfig(cluster, false, true, true)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR);
assertThat(newLbConfig(cluster, false)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR);
}

// When a provider for the endpoint picking custom policy is available, the configuration should
Expand All @@ -328,7 +311,7 @@ public void customLbInWrr_providerRegistered_udpa() throws ResourceInvalidExcept
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder()
.addPolicies(buildWrrPolicy(CUSTOM_POLICY_UDPA, ROUND_ROBIN_POLICY))).build();

assertThat(newLbConfig(cluster, false, true, true)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR);
assertThat(newLbConfig(cluster, false)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR);
}

// When a provider for the custom wrr_locality child policy is NOT available, we should fall back
Expand All @@ -338,7 +321,7 @@ public void customLbInWrr_providerNotRegistered() throws ResourceInvalidExceptio
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder()
.addPolicies(buildWrrPolicy(CUSTOM_POLICY, ROUND_ROBIN_POLICY))).build();

assertThat(newLbConfig(cluster, false, true, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
assertThat(newLbConfig(cluster, false)).isEqualTo(VALID_ROUND_ROBIN_CONFIG);
}

// When a provider for the custom wrr_locality child policy is NOT available and no alternative
Expand All @@ -348,7 +331,7 @@ public void customLbInWrr_providerNotRegistered_noFallback() throws ResourceInva
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(
LoadBalancingPolicy.newBuilder().addPolicies(buildWrrPolicy(CUSTOM_POLICY))).build();

assertResourceInvalidExceptionThrown(cluster, false, true, true, "Invalid LoadBalancingPolicy");
assertResourceInvalidExceptionThrown(cluster, false, "Invalid LoadBalancingPolicy");
}

@Test
Expand All @@ -375,7 +358,7 @@ public void maxRecursion() {
buildWrrPolicy(
ROUND_ROBIN_POLICY))))))))))))))))))).build();

assertResourceInvalidExceptionThrown(cluster, false, true, true,
assertResourceInvalidExceptionThrown(cluster, false,
"Maximum LB config recursion depth reached");
}

Expand All @@ -391,18 +374,16 @@ private static Policy buildWrrPolicy(Policy... childPolicy) {
.build()))).build();
}

private LbConfig newLbConfig(Cluster cluster, boolean enableLeastRequest, boolean enableWrr,
boolean enablePickFirst)
private LbConfig newLbConfig(Cluster cluster, boolean enableLeastRequest)
throws ResourceInvalidException {
return ServiceConfigUtil.unwrapLoadBalancingConfig(
LoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest,
enableWrr, enablePickFirst));
LoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest));
}

private void assertResourceInvalidExceptionThrown(Cluster cluster, boolean enableLeastRequest,
boolean enableWrr, boolean enablePickFirst, String expectedMessage) {
String expectedMessage) {
try {
newLbConfig(cluster, enableLeastRequest, enableWrr, enablePickFirst);
newLbConfig(cluster, enableLeastRequest);
} catch (ResourceInvalidException e) {
assertThat(e).hasMessageThat().contains(expectedMessage);
return;
Expand Down

0 comments on commit 85e52cd

Please sign in to comment.