xds: Remove WRR and PF experimental flags

They have been on by default for a good while, and seem stable. This
also removes some grpc-isms from XdsResourceType.
This commit is contained in:
Eric Anderson 2024-03-05 11:29:29 -08:00
parent 27824469bf
commit 85e52cd113
5 changed files with 34 additions and 71 deletions

View File

@ -98,15 +98,14 @@ class LoadBalancerConfigFactory {
* *
* @throws ResourceInvalidException If the {@link Cluster} has an invalid LB configuration. * @throws ResourceInvalidException If the {@link Cluster} has an invalid LB configuration.
*/ */
static ImmutableMap<String, ?> newConfig(Cluster cluster, boolean enableLeastRequest, static ImmutableMap<String, ?> newConfig(Cluster cluster, boolean enableLeastRequest)
boolean enableWrr, boolean enablePickFirst)
throws ResourceInvalidException { throws ResourceInvalidException {
// The new load_balancing_policy will always be used if it is set, but for backward // 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. // compatibility we will fall back to using the old lb_policy field if the new field is not set.
if (cluster.hasLoadBalancingPolicy()) { if (cluster.hasLoadBalancingPolicy()) {
try { try {
return LoadBalancingPolicyConverter.convertToServiceConfig(cluster.getLoadBalancingPolicy(), return LoadBalancingPolicyConverter.convertToServiceConfig(cluster.getLoadBalancingPolicy(),
0, enableWrr, enablePickFirst); 0);
} catch (MaxRecursionReachedException e) { } catch (MaxRecursionReachedException e) {
throw new ResourceInvalidException("Maximum LB config recursion depth reached", e); throw new ResourceInvalidException("Maximum LB config recursion depth reached", e);
} }
@ -213,8 +212,7 @@ class LoadBalancerConfigFactory {
* Converts a {@link LoadBalancingPolicy} object to a service config JSON object. * Converts a {@link LoadBalancingPolicy} object to a service config JSON object.
*/ */
private static ImmutableMap<String, ?> convertToServiceConfig( private static ImmutableMap<String, ?> convertToServiceConfig(
LoadBalancingPolicy loadBalancingPolicy, int recursionDepth, boolean enableWrr, LoadBalancingPolicy loadBalancingPolicy, int recursionDepth)
boolean enablePickFirst)
throws ResourceInvalidException, MaxRecursionReachedException { throws ResourceInvalidException, MaxRecursionReachedException {
if (recursionDepth > MAX_RECURSION) { if (recursionDepth > MAX_RECURSION) {
throw new MaxRecursionReachedException(); throw new MaxRecursionReachedException();
@ -228,20 +226,16 @@ class LoadBalancerConfigFactory {
serviceConfig = convertRingHashConfig(typedConfig.unpack(RingHash.class)); serviceConfig = convertRingHashConfig(typedConfig.unpack(RingHash.class));
} else if (typedConfig.is(WrrLocality.class)) { } else if (typedConfig.is(WrrLocality.class)) {
serviceConfig = convertWrrLocalityConfig(typedConfig.unpack(WrrLocality.class), serviceConfig = convertWrrLocalityConfig(typedConfig.unpack(WrrLocality.class),
recursionDepth, enableWrr, enablePickFirst); recursionDepth);
} else if (typedConfig.is(RoundRobin.class)) { } else if (typedConfig.is(RoundRobin.class)) {
serviceConfig = convertRoundRobinConfig(); serviceConfig = convertRoundRobinConfig();
} else if (typedConfig.is(LeastRequest.class)) { } else if (typedConfig.is(LeastRequest.class)) {
serviceConfig = convertLeastRequestConfig(typedConfig.unpack(LeastRequest.class)); serviceConfig = convertLeastRequestConfig(typedConfig.unpack(LeastRequest.class));
} else if (typedConfig.is(ClientSideWeightedRoundRobin.class)) { } else if (typedConfig.is(ClientSideWeightedRoundRobin.class)) {
if (enableWrr) { serviceConfig = convertWeightedRoundRobinConfig(
serviceConfig = convertWeightedRoundRobinConfig( typedConfig.unpack(ClientSideWeightedRoundRobin.class));
typedConfig.unpack(ClientSideWeightedRoundRobin.class));
}
} else if (typedConfig.is(PickFirst.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)) { } else if (typedConfig.is(com.github.xds.type.v3.TypedStruct.class)) {
serviceConfig = convertCustomConfig( serviceConfig = convertCustomConfig(
typedConfig.unpack(com.github.xds.type.v3.TypedStruct.class)); typedConfig.unpack(com.github.xds.type.v3.TypedStruct.class));
@ -310,12 +304,10 @@ class LoadBalancerConfigFactory {
* Converts a wrr_locality {@link Any} configuration to service config format. * Converts a wrr_locality {@link Any} configuration to service config format.
*/ */
private static ImmutableMap<String, ?> convertWrrLocalityConfig(WrrLocality wrrLocality, private static ImmutableMap<String, ?> convertWrrLocalityConfig(WrrLocality wrrLocality,
int recursionDepth, boolean enableWrr, boolean enablePickFirst) int recursionDepth)
throws ResourceInvalidException, throws ResourceInvalidException, MaxRecursionReachedException {
MaxRecursionReachedException {
return buildWrrLocalityConfig( return buildWrrLocalityConfig(
convertToServiceConfig(wrrLocality.getEndpointPickingPolicy(), convertToServiceConfig(wrrLocality.getEndpointPickingPolicy(), recursionDepth + 1));
recursionDepth + 1, enableWrr, enablePickFirst));
} }
/** /**

View File

@ -141,7 +141,7 @@ class XdsClusterResource extends XdsResourceType<CdsUpdate> {
CdsUpdate.Builder updateBuilder = structOrError.getStruct(); CdsUpdate.Builder updateBuilder = structOrError.getStruct();
ImmutableMap<String, ?> lbPolicyConfig = LoadBalancerConfigFactory.newConfig(cluster, ImmutableMap<String, ?> lbPolicyConfig = LoadBalancerConfigFactory.newConfig(cluster,
enableLeastRequest, enableWrr, enablePickFirst); enableLeastRequest);
// Validate the LB config by trying to parse it with the corresponding LB provider. // Validate the LB config by trying to parse it with the corresponding LB provider.
LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(lbPolicyConfig); LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(lbPolicyConfig);

View File

@ -52,12 +52,6 @@ public abstract class XdsResourceType<T extends ResourceUpdate> {
? Boolean.parseBoolean(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST")) ? Boolean.parseBoolean(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST"))
: Boolean.parseBoolean(System.getProperty("io.grpc.xds.experimentalEnableLeastRequest")); : 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 = protected static final String TYPE_URL_CLUSTER_CONFIG =
"type.googleapis.com/envoy.extensions.clusters.aggregate.v3.ClusterConfig"; "type.googleapis.com/envoy.extensions.clusters.aggregate.v3.ClusterConfig";
protected static final String TYPE_URL_TYPED_STRUCT_UDPA = protected static final String TYPE_URL_TYPED_STRUCT_UDPA =

View File

@ -170,22 +170,18 @@ public class GrpcXdsClientImplDataTest {
private final FilterRegistry filterRegistry = FilterRegistry.getDefaultRegistry(); private final FilterRegistry filterRegistry = FilterRegistry.getDefaultRegistry();
private boolean originalEnableRouteLookup; private boolean originalEnableRouteLookup;
private boolean originalEnableLeastRequest; private boolean originalEnableLeastRequest;
private boolean originalEnableWrr;
@Before @Before
public void setUp() { public void setUp() {
originalEnableRouteLookup = XdsResourceType.enableRouteLookup; originalEnableRouteLookup = XdsResourceType.enableRouteLookup;
originalEnableLeastRequest = XdsResourceType.enableLeastRequest; originalEnableLeastRequest = XdsResourceType.enableLeastRequest;
assertThat(originalEnableLeastRequest).isFalse(); assertThat(originalEnableLeastRequest).isFalse();
originalEnableWrr = XdsResourceType.enableWrr;
assertThat(originalEnableWrr).isTrue();
} }
@After @After
public void tearDown() { public void tearDown() {
XdsResourceType.enableRouteLookup = originalEnableRouteLookup; XdsResourceType.enableRouteLookup = originalEnableRouteLookup;
XdsResourceType.enableLeastRequest = originalEnableLeastRequest; XdsResourceType.enableLeastRequest = originalEnableLeastRequest;
XdsResourceType.enableWrr = originalEnableWrr;
} }
@Test @Test

View File

@ -155,14 +155,14 @@ public class LoadBalancerConfigFactoryTest {
public void roundRobin() throws ResourceInvalidException { public void roundRobin() throws ResourceInvalidException {
Cluster cluster = newCluster(buildWrrPolicy(ROUND_ROBIN_POLICY)); 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 @Test
public void weightedRoundRobin() throws ResourceInvalidException { public void weightedRoundRobin() throws ResourceInvalidException {
Cluster cluster = newCluster(buildWrrPolicy(WRR_POLICY)); 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 @Test
@ -179,22 +179,15 @@ public class LoadBalancerConfigFactoryTest {
.build()) .build())
.build())); .build()));
assertResourceInvalidExceptionThrown(cluster, true, true, true, assertResourceInvalidExceptionThrown(cluster, true,
"Invalid duration in weighted round robin config"); "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 @Test
public void roundRobin_legacy() throws ResourceInvalidException { public void roundRobin_legacy() throws ResourceInvalidException {
Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.ROUND_ROBIN).build(); 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 @Test
@ -203,7 +196,7 @@ public class LoadBalancerConfigFactoryTest {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(RING_HASH_POLICY)) .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(RING_HASH_POLICY))
.build(); .build();
assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_RING_HASH_CONFIG); assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_RING_HASH_CONFIG);
} }
@Test @Test
@ -213,7 +206,7 @@ public class LoadBalancerConfigFactoryTest {
.setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE)) .setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE))
.setHashFunction(HashFunction.XX_HASH)).build(); .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 @Test
@ -225,7 +218,7 @@ public class LoadBalancerConfigFactoryTest {
.setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE)) .setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE))
.setHashFunction(RingHash.HashFunction.MURMUR_HASH_2).build()))).build()); .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 @Test
@ -233,7 +226,7 @@ public class LoadBalancerConfigFactoryTest {
Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.RING_HASH).setRingHashLbConfig( Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.RING_HASH).setRingHashLbConfig(
RingHashLbConfig.newBuilder().setHashFunction(HashFunction.MURMUR_HASH_2)).build(); 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 @Test
@ -242,7 +235,7 @@ public class LoadBalancerConfigFactoryTest {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(LEAST_REQUEST_POLICY)) .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(LEAST_REQUEST_POLICY))
.build(); .build();
assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_LEAST_REQUEST_CONFIG); assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_LEAST_REQUEST_CONFIG);
} }
@Test @Test
@ -254,7 +247,7 @@ public class LoadBalancerConfigFactoryTest {
LeastRequestLbConfig.newBuilder() LeastRequestLbConfig.newBuilder()
.setChoiceCount(UInt32Value.of(LEAST_REQUEST_CHOICE_COUNT))).build(); .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"); assertThat(lbConfig.getPolicyName()).isEqualTo("wrr_locality_experimental");
List<LbConfig> childConfigs = ServiceConfigUtil.unwrapLoadBalancingConfigList( List<LbConfig> childConfigs = ServiceConfigUtil.unwrapLoadBalancingConfigList(
@ -269,7 +262,7 @@ public class LoadBalancerConfigFactoryTest {
public void leastRequest_notEnabled() { public void leastRequest_notEnabled() {
Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.LEAST_REQUEST).build(); Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.LEAST_REQUEST).build();
assertResourceInvalidExceptionThrown(cluster, false, true, true, "unsupported lb policy"); assertResourceInvalidExceptionThrown(cluster, false, "unsupported lb policy");
} }
@Test @Test
@ -278,24 +271,14 @@ public class LoadBalancerConfigFactoryTest {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(PICK_FIRST_POLICY)) .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(PICK_FIRST_POLICY))
.build(); .build();
assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_PICK_FIRST_CONFIG); assertThat(newLbConfig(cluster, 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");
} }
@Test @Test
public void customRootLb_providerRegistered() throws ResourceInvalidException { public void customRootLb_providerRegistered() throws ResourceInvalidException {
LoadBalancerRegistry.getDefaultRegistry().register(CUSTOM_POLICY_PROVIDER); LoadBalancerRegistry.getDefaultRegistry().register(CUSTOM_POLICY_PROVIDER);
assertThat(newLbConfig(newCluster(CUSTOM_POLICY), false, assertThat(newLbConfig(newCluster(CUSTOM_POLICY), false)).isEqualTo(VALID_CUSTOM_CONFIG);
true, true)).isEqualTo(VALID_CUSTOM_CONFIG);
} }
@Test @Test
@ -304,7 +287,7 @@ public class LoadBalancerConfigFactoryTest {
.setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(CUSTOM_POLICY)) .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(CUSTOM_POLICY))
.build(); .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 // When a provider for the endpoint picking custom policy is available, the configuration should
@ -316,7 +299,7 @@ public class LoadBalancerConfigFactoryTest {
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder() Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder()
.addPolicies(buildWrrPolicy(CUSTOM_POLICY, ROUND_ROBIN_POLICY))).build(); .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 // When a provider for the endpoint picking custom policy is available, the configuration should
@ -328,7 +311,7 @@ public class LoadBalancerConfigFactoryTest {
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder() Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder()
.addPolicies(buildWrrPolicy(CUSTOM_POLICY_UDPA, ROUND_ROBIN_POLICY))).build(); .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 // When a provider for the custom wrr_locality child policy is NOT available, we should fall back
@ -338,7 +321,7 @@ public class LoadBalancerConfigFactoryTest {
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder() Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder()
.addPolicies(buildWrrPolicy(CUSTOM_POLICY, ROUND_ROBIN_POLICY))).build(); .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 // When a provider for the custom wrr_locality child policy is NOT available and no alternative
@ -348,7 +331,7 @@ public class LoadBalancerConfigFactoryTest {
Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy( Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(
LoadBalancingPolicy.newBuilder().addPolicies(buildWrrPolicy(CUSTOM_POLICY))).build(); LoadBalancingPolicy.newBuilder().addPolicies(buildWrrPolicy(CUSTOM_POLICY))).build();
assertResourceInvalidExceptionThrown(cluster, false, true, true, "Invalid LoadBalancingPolicy"); assertResourceInvalidExceptionThrown(cluster, false, "Invalid LoadBalancingPolicy");
} }
@Test @Test
@ -375,7 +358,7 @@ public class LoadBalancerConfigFactoryTest {
buildWrrPolicy( buildWrrPolicy(
ROUND_ROBIN_POLICY))))))))))))))))))).build(); ROUND_ROBIN_POLICY))))))))))))))))))).build();
assertResourceInvalidExceptionThrown(cluster, false, true, true, assertResourceInvalidExceptionThrown(cluster, false,
"Maximum LB config recursion depth reached"); "Maximum LB config recursion depth reached");
} }
@ -391,18 +374,16 @@ public class LoadBalancerConfigFactoryTest {
.build()))).build(); .build()))).build();
} }
private LbConfig newLbConfig(Cluster cluster, boolean enableLeastRequest, boolean enableWrr, private LbConfig newLbConfig(Cluster cluster, boolean enableLeastRequest)
boolean enablePickFirst)
throws ResourceInvalidException { throws ResourceInvalidException {
return ServiceConfigUtil.unwrapLoadBalancingConfig( return ServiceConfigUtil.unwrapLoadBalancingConfig(
LoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest, LoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest));
enableWrr, enablePickFirst));
} }
private void assertResourceInvalidExceptionThrown(Cluster cluster, boolean enableLeastRequest, private void assertResourceInvalidExceptionThrown(Cluster cluster, boolean enableLeastRequest,
boolean enableWrr, boolean enablePickFirst, String expectedMessage) { String expectedMessage) {
try { try {
newLbConfig(cluster, enableLeastRequest, enableWrr, enablePickFirst); newLbConfig(cluster, enableLeastRequest);
} catch (ResourceInvalidException e) { } catch (ResourceInvalidException e) {
assertThat(e).hasMessageThat().contains(expectedMessage); assertThat(e).hasMessageThat().contains(expectedMessage);
return; return;