diff --git a/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java b/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java index 34c513fb6c..cb340f178f 100644 --- a/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java +++ b/xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java @@ -23,14 +23,10 @@ import static io.grpc.xds.XdsLbPolicies.CLUSTER_RESOLVER_POLICY_NAME; import com.google.common.annotations.VisibleForTesting; import io.grpc.InternalLogId; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.Status; import io.grpc.SynchronizationContext; import io.grpc.internal.ObjectPool; -import io.grpc.internal.ServiceConfigUtil; -import io.grpc.internal.ServiceConfigUtil.LbConfig; -import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.xds.CdsLoadBalancerProvider.CdsConfig; import io.grpc.xds.ClusterResolverLoadBalancerProvider.ClusterResolverConfig; import io.grpc.xds.ClusterResolverLoadBalancerProvider.ClusterResolverConfig.DiscoveryMechanism; @@ -185,17 +181,9 @@ final class CdsLoadBalancer2 extends LoadBalancer { return; } - // The LB policy config is provided in service_config.proto/JSON format. It is unwrapped - // to determine the name of the policy in the load balancer registry. - LbConfig unwrappedLbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig( - root.result.lbPolicyConfig()); - LoadBalancerProvider lbProvider = lbRegistry.getProvider(unwrappedLbConfig.getPolicyName()); - Object lbConfig = lbProvider.parseLoadBalancingPolicyConfig( - unwrappedLbConfig.getRawConfigValue()).getConfig(); - ClusterResolverConfig config = new ClusterResolverConfig( Collections.unmodifiableList(instances), - new PolicySelection(lbProvider, lbConfig)); + root.result.lbPolicySelection()); if (childLb == null) { childLb = lbRegistry.getProvider(CLUSTER_RESOLVER_POLICY_NAME).newLoadBalancer(helper); } diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java index 309b8f0d63..ff69656331 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -67,6 +67,7 @@ import io.grpc.Context; import io.grpc.EquivalentAddressGroup; import io.grpc.Grpc; import io.grpc.InternalLogId; +import io.grpc.LoadBalancerProvider; import io.grpc.LoadBalancerRegistry; import io.grpc.ManagedChannel; import io.grpc.NameResolver; @@ -77,6 +78,7 @@ import io.grpc.SynchronizationContext.ScheduledHandle; import io.grpc.internal.BackoffPolicy; import io.grpc.internal.ServiceConfigUtil; import io.grpc.internal.ServiceConfigUtil.LbConfig; +import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.internal.TimeProvider; import io.grpc.xds.AbstractXdsClient.ResourceType; import io.grpc.xds.Bootstrapper.AuthorityInfo; @@ -138,14 +140,6 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res static final int INITIAL_RESOURCE_FETCH_TIMEOUT_SEC = 15; private static final String TRANSPORT_SOCKET_NAME_TLS = "envoy.transport_sockets.tls"; @VisibleForTesting - static final long DEFAULT_RING_HASH_LB_POLICY_MIN_RING_SIZE = 1024L; - @VisibleForTesting - static final long DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE = 8 * 1024 * 1024L; - @VisibleForTesting - static final int DEFAULT_LEAST_REQUEST_CHOICE_COUNT = 2; - @VisibleForTesting - static final long MAX_RING_HASH_LB_POLICY_RING_SIZE = 8 * 1024 * 1024L; - @VisibleForTesting static final String AGGREGATE_CLUSTER_TYPE_NAME = "envoy.clusters.aggregate"; @VisibleForTesting static final String HASH_POLICY_FILTER_STATE_KEY = "io.grpc.channel_id"; @@ -1643,21 +1637,16 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res } CdsUpdate.Builder updateBuilder = structOrError.getStruct(); - // TODO: If load_balancing_policy is set in Cluster use it for LB config, otherwise fall back - // to using the legacy lb_policy field. - ImmutableMap lbPolicyConfig = LegacyLoadBalancerConfigBuilder.forCluster(cluster, - enableLeastRequest).build(); - - // Validate the LB config by trying to parse it with the corresponding LB provider. - LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(lbPolicyConfig); - NameResolver.ConfigOrError configOrError = loadBalancerRegistry.getProvider( - lbConfig.getPolicyName()).parseLoadBalancingPolicyConfig( - lbConfig.getRawConfigValue()); + LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig( + LegacyLoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest)); + LoadBalancerProvider lbProvider = loadBalancerRegistry.getProvider(lbConfig.getPolicyName()); + NameResolver.ConfigOrError configOrError = lbProvider + .parseLoadBalancingPolicyConfig(lbConfig.getRawConfigValue()); if (configOrError.getError() != null) { throw new ResourceInvalidException(structOrError.getErrorDetail()); } - updateBuilder.lbPolicyConfig(lbPolicyConfig); + updateBuilder.lbPolicySelection(new PolicySelection(lbProvider, configOrError.getConfig())); return updateBuilder.build(); } diff --git a/xds/src/main/java/io/grpc/xds/LegacyLoadBalancerConfigBuilder.java b/xds/src/main/java/io/grpc/xds/LegacyLoadBalancerConfigBuilder.java deleted file mode 100644 index 9ae63a3735..0000000000 --- a/xds/src/main/java/io/grpc/xds/LegacyLoadBalancerConfigBuilder.java +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright 2022 The gRPC Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package io.grpc.xds; - -import com.google.common.collect.ImmutableMap; -import io.envoyproxy.envoy.config.cluster.v3.Cluster; -import io.envoyproxy.envoy.config.cluster.v3.Cluster.LeastRequestLbConfig; -import io.envoyproxy.envoy.config.cluster.v3.Cluster.RingHashLbConfig; -import io.grpc.xds.ClientXdsClient.ResourceInvalidException; -import java.util.Map; - -/** - * Builds a JSON LB configuration based on the old style of using the xDS Cluster proto message. The - * lb_policy field is used to select the policy and configuration is extracted from various policy - * specific fields in Cluster. - */ -abstract class LegacyLoadBalancerConfigBuilder { - - /** - * Factory method for creating a new {link LoadBalancerConfigConverter} for a given xDS {@link - * Cluster}. - * - * @throws ResourceInvalidException If the {@link Cluster} has an invalid LB configuration. - */ - static LegacyLoadBalancerConfigBuilder forCluster(Cluster cluster, boolean enableLeastRequest) - throws ResourceInvalidException { - switch (cluster.getLbPolicy()) { - case ROUND_ROBIN: - return new RoundRobinLoadBalancerConfigBuilder(); - case RING_HASH: - return new RingHashLoadBalancerConfigBuilder(cluster); - case LEAST_REQUEST: - if (enableLeastRequest) { - return new LeastRequestLoadBalancerConfigBuilder(cluster); - } - break; - default: - } - throw new ResourceInvalidException( - "Cluster " + cluster.getName() + ": unsupported lb policy: " + cluster.getLbPolicy()); - } - - /** - * Builds the configuration {@link Map} based on the configuration data from the provider {@code - * Cluster}. - */ - abstract ImmutableMap build(); - - /** - * Round robin does not support any configuration, a null value will always be built. - */ - static class RoundRobinLoadBalancerConfigBuilder extends LegacyLoadBalancerConfigBuilder { - - static final String LB_NAME_FIELD = "round_robin"; - - @Override - ImmutableMap build() { - return ImmutableMap.of(LB_NAME_FIELD, ImmutableMap.of()); - } - } - - /** - * Builds a load balancer config for the {@link RingHashLoadBalancerProvider}. - */ - static class RingHashLoadBalancerConfigBuilder extends LegacyLoadBalancerConfigBuilder { - - static final long DEFAULT_RING_HASH_LB_POLICY_MIN_RING_SIZE = 1024L; - static final long DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE = 8 * 1024 * 1024L; - static final long MAX_RING_HASH_LB_POLICY_RING_SIZE = 8 * 1024 * 1024L; - - static final String LB_NAME_FIELD = "ring_hash_experimental"; - static final String MIN_RING_SIZE_FIELD_NAME = "minRingSize"; - static final String MAX_RING_SIZE_FIELD_NAME = "maxRingSize"; - - private final Long minRingSize; - private final Long maxRingSize; - - RingHashLoadBalancerConfigBuilder(Cluster cluster) throws ResourceInvalidException { - RingHashLbConfig lbConfig = cluster.getRingHashLbConfig(); - minRingSize = lbConfig.hasMinimumRingSize() ? lbConfig.getMinimumRingSize().getValue() - : DEFAULT_RING_HASH_LB_POLICY_MIN_RING_SIZE; - maxRingSize = lbConfig.hasMaximumRingSize() ? lbConfig.getMaximumRingSize().getValue() - : DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE; - if (lbConfig.getHashFunction() != RingHashLbConfig.HashFunction.XX_HASH - || minRingSize > maxRingSize || maxRingSize > MAX_RING_HASH_LB_POLICY_RING_SIZE) { - throw new ResourceInvalidException( - "Cluster " + cluster.getName() + ": invalid ring_hash_lb_config: " + lbConfig); - } - } - - @Override - ImmutableMap build() { - ImmutableMap.Builder configBuilder = ImmutableMap.builder(); - configBuilder.put(MIN_RING_SIZE_FIELD_NAME, minRingSize.doubleValue()); - configBuilder.put(MAX_RING_SIZE_FIELD_NAME, maxRingSize.doubleValue()); - return ImmutableMap.of(LB_NAME_FIELD, configBuilder.build()); - } - } - - static class LeastRequestLoadBalancerConfigBuilder extends LegacyLoadBalancerConfigBuilder { - - static final long DEFAULT_LEAST_REQUEST_CHOICE_COUNT = 2L; - - static final String LB_NAME_FIELD = "least_request_experimental"; - static final String CHOICE_COUNT_FIELD_NAME = "choiceCount"; - - private final Long choiceCount; - - LeastRequestLoadBalancerConfigBuilder(Cluster cluster) throws ResourceInvalidException { - LeastRequestLbConfig lbConfig = cluster.getLeastRequestLbConfig(); - choiceCount = lbConfig.hasChoiceCount() ? lbConfig.getChoiceCount().getValue() - : DEFAULT_LEAST_REQUEST_CHOICE_COUNT; - if (choiceCount < DEFAULT_LEAST_REQUEST_CHOICE_COUNT) { - throw new ResourceInvalidException( - "Cluster " + cluster.getName() + ": invalid least_request_lb_config: " + lbConfig); - } - } - - @Override - ImmutableMap build() { - ImmutableMap.Builder configBuilder = ImmutableMap.builder(); - configBuilder.put(CHOICE_COUNT_FIELD_NAME, choiceCount.doubleValue()); - return ImmutableMap.of(LB_NAME_FIELD, configBuilder.build()); - } - } -} diff --git a/xds/src/main/java/io/grpc/xds/LegacyLoadBalancerConfigFactory.java b/xds/src/main/java/io/grpc/xds/LegacyLoadBalancerConfigFactory.java new file mode 100644 index 0000000000..45a73b1e1f --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/LegacyLoadBalancerConfigFactory.java @@ -0,0 +1,105 @@ +/* + * Copyright 2022 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds; + +import com.google.common.collect.ImmutableMap; +import io.envoyproxy.envoy.config.cluster.v3.Cluster; +import io.envoyproxy.envoy.config.cluster.v3.Cluster.LeastRequestLbConfig; +import io.envoyproxy.envoy.config.cluster.v3.Cluster.RingHashLbConfig; +import io.grpc.xds.ClientXdsClient.ResourceInvalidException; + +/** + * Builds a JSON LB configuration based on the old style of using the xDS Cluster proto message. The + * lb_policy field is used to select the policy and configuration is extracted from various policy + * specific fields in Cluster. + */ +abstract class LegacyLoadBalancerConfigFactory { + + static final String ROUND_ROBIN_FIELD_NAME = "round_robin"; + + static final String RING_HASH_FIELD_NAME = "ring_hash_experimental"; + static final String MIN_RING_SIZE_FIELD_NAME = "minRingSize"; + static final String MAX_RING_SIZE_FIELD_NAME = "maxRingSize"; + + static final String LEAST_REQUEST_FIELD_NAME = "least_request_experimental"; + static final String CHOICE_COUNT_FIELD_NAME = "choiceCount"; + + /** + * Factory method for creating a new {link LoadBalancerConfigConverter} for a given xDS {@link + * Cluster}. + * + * @throws ResourceInvalidException If the {@link Cluster} has an invalid LB configuration. + */ + static ImmutableMap newConfig(Cluster cluster, boolean enableLeastRequest) + throws ResourceInvalidException { + switch (cluster.getLbPolicy()) { + case ROUND_ROBIN: + return newRoundRobinConfig(); + case RING_HASH: + return newRingHashConfig(cluster); + case LEAST_REQUEST: + if (enableLeastRequest) { + return newLeastRequestConfig(cluster); + } + break; + default: + } + throw new ResourceInvalidException( + "Cluster " + cluster.getName() + ": unsupported lb policy: " + cluster.getLbPolicy()); + } + + // Builds an empty configuration for round robin (it is not configurable). + private static ImmutableMap newRoundRobinConfig() { + return ImmutableMap.of(ROUND_ROBIN_FIELD_NAME, ImmutableMap.of()); + } + + // Builds a ring hash config and validates the hash function selection. + private static ImmutableMap newRingHashConfig(Cluster cluster) + throws ResourceInvalidException { + RingHashLbConfig lbConfig = cluster.getRingHashLbConfig(); + + // The hash function needs to be validated here as it is not exposed in the returned + // configuration for later validation. + if (lbConfig.getHashFunction() != RingHashLbConfig.HashFunction.XX_HASH) { + throw new ResourceInvalidException( + "Cluster " + cluster.getName() + ": invalid ring hash function: " + lbConfig); + } + + ImmutableMap.Builder configBuilder = ImmutableMap.builder(); + if (lbConfig.hasMinimumRingSize()) { + configBuilder.put(MIN_RING_SIZE_FIELD_NAME, + ((Long) lbConfig.getMinimumRingSize().getValue()).doubleValue()); + } + if (lbConfig.hasMaximumRingSize()) { + configBuilder.put(MAX_RING_SIZE_FIELD_NAME, + ((Long) lbConfig.getMaximumRingSize().getValue()).doubleValue()); + } + return ImmutableMap.of(RING_HASH_FIELD_NAME, configBuilder.build()); + } + + // Builds a new least request config. + private static ImmutableMap newLeastRequestConfig(Cluster cluster) { + LeastRequestLbConfig lbConfig = cluster.getLeastRequestLbConfig(); + + ImmutableMap.Builder configBuilder = ImmutableMap.builder(); + if (lbConfig.hasChoiceCount()) { + configBuilder.put(CHOICE_COUNT_FIELD_NAME, + ((Integer) lbConfig.getChoiceCount().getValue()).doubleValue()); + } + return ImmutableMap.of(LEAST_REQUEST_FIELD_NAME, configBuilder.build()); + } +} diff --git a/xds/src/main/java/io/grpc/xds/XdsClient.java b/xds/src/main/java/io/grpc/xds/XdsClient.java index bbc8a01b84..c03c98f50b 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClient.java +++ b/xds/src/main/java/io/grpc/xds/XdsClient.java @@ -24,19 +24,22 @@ import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.common.net.UrlEscapers; import com.google.common.util.concurrent.ListenableFuture; import com.google.protobuf.Any; +import io.grpc.LoadBalancerRegistry; import io.grpc.Status; +import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.xds.AbstractXdsClient.ResourceType; import io.grpc.xds.Bootstrapper.ServerInfo; import io.grpc.xds.Endpoints.DropOverload; import io.grpc.xds.Endpoints.LocalityLbEndpoints; import io.grpc.xds.EnvoyServerProtoData.Listener; import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext; +import io.grpc.xds.LeastRequestLoadBalancer.LeastRequestConfig; import io.grpc.xds.LoadStatsManager2.ClusterDropStats; import io.grpc.xds.LoadStatsManager2.ClusterLocalityStats; +import io.grpc.xds.RingHashLoadBalancer.RingHashConfig; import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; @@ -179,7 +182,7 @@ abstract class XdsClient { abstract ClusterType clusterType(); - abstract ImmutableMap lbPolicyConfig(); + abstract PolicySelection lbPolicySelection(); // Only valid if lbPolicy is "ring_hash_experimental". abstract long minRingSize(); @@ -276,7 +279,7 @@ abstract class XdsClient { return MoreObjects.toStringHelper(this) .add("clusterName", clusterName()) .add("clusterType", clusterType()) - .add("lbPolicyConfig", lbPolicyConfig()) + .add("lbPolicySelection", lbPolicySelection()) .add("minRingSize", minRingSize()) .add("maxRingSize", maxRingSize()) .add("choiceCount", choiceCount()) @@ -297,21 +300,23 @@ abstract class XdsClient { // Private, use one of the static factory methods instead. protected abstract Builder clusterType(ClusterType clusterType); - protected abstract Builder lbPolicyConfig(ImmutableMap lbPolicyConfig); + protected abstract Builder lbPolicySelection(PolicySelection lbPolicySelection); Builder roundRobinLbPolicy() { - return this.lbPolicyConfig(ImmutableMap.of("round_robin", ImmutableMap.of())); + return this.lbPolicySelection(new PolicySelection( + LoadBalancerRegistry.getDefaultRegistry().getProvider("round_robin"), null)); } Builder ringHashLbPolicy(Long minRingSize, Long maxRingSize) { - return this.lbPolicyConfig(ImmutableMap.of("ring_hash_experimental", - ImmutableMap.of("minRingSize", minRingSize.doubleValue(), "maxRingSize", - maxRingSize.doubleValue()))); + return this.lbPolicySelection(new PolicySelection( + LoadBalancerRegistry.getDefaultRegistry().getProvider("ring_hash_experimental"), + new RingHashConfig(minRingSize, maxRingSize))); } Builder leastRequestLbPolicy(Integer choiceCount) { - return this.lbPolicyConfig(ImmutableMap.of("least_request_experimental", - ImmutableMap.of("choiceCount", choiceCount.doubleValue()))); + return this.lbPolicySelection(new PolicySelection( + LoadBalancerRegistry.getDefaultRegistry().getProvider("least_request_experimental"), + new LeastRequestConfig(choiceCount))); } // Private, use leastRequestLbPolicy(int). diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java index 98bdd34b0d..eb45e10b02 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java @@ -30,7 +30,6 @@ import com.google.protobuf.Message; import com.google.protobuf.StringValue; import com.google.protobuf.Struct; import com.google.protobuf.UInt32Value; -import com.google.protobuf.UInt64Value; import com.google.protobuf.Value; import com.google.protobuf.util.Durations; import com.google.re2j.Pattern; @@ -38,9 +37,6 @@ import io.envoyproxy.envoy.config.cluster.v3.Cluster; import io.envoyproxy.envoy.config.cluster.v3.Cluster.DiscoveryType; import io.envoyproxy.envoy.config.cluster.v3.Cluster.EdsClusterConfig; import io.envoyproxy.envoy.config.cluster.v3.Cluster.LbPolicy; -import io.envoyproxy.envoy.config.cluster.v3.Cluster.LeastRequestLbConfig; -import io.envoyproxy.envoy.config.cluster.v3.Cluster.RingHashLbConfig; -import io.envoyproxy.envoy.config.cluster.v3.Cluster.RingHashLbConfig.HashFunction; import io.envoyproxy.envoy.config.core.v3.Address; import io.envoyproxy.envoy.config.core.v3.AggregatedConfigSource; import io.envoyproxy.envoy.config.core.v3.CidrRange; @@ -108,9 +104,6 @@ import io.grpc.InsecureChannelCredentials; import io.grpc.LoadBalancer; import io.grpc.LoadBalancerRegistry; import io.grpc.Status.Code; -import io.grpc.internal.JsonUtil; -import io.grpc.internal.ServiceConfigUtil; -import io.grpc.internal.ServiceConfigUtil.LbConfig; import io.grpc.lookup.v1.GrpcKeyBuilder; import io.grpc.lookup.v1.GrpcKeyBuilder.Name; import io.grpc.lookup.v1.NameMatcher; @@ -1740,12 +1733,8 @@ public class ClientXdsClientDataTest { CdsUpdate update = ClientXdsClient.processCluster( cluster, new HashSet(), null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); - LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(update.lbPolicyConfig()); - assertThat(lbConfig.getPolicyName()).isEqualTo("ring_hash_experimental"); - assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "minRingSize")) - .isEqualTo(ClientXdsClient.DEFAULT_RING_HASH_LB_POLICY_MIN_RING_SIZE); - assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "maxRingSize")) - .isEqualTo(ClientXdsClient.DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE); + assertThat(update.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "ring_hash_experimental"); } @Test @@ -1766,10 +1755,8 @@ public class ClientXdsClientDataTest { CdsUpdate update = ClientXdsClient.processCluster( cluster, new HashSet(), null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); - LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(update.lbPolicyConfig()); - assertThat(lbConfig.getPolicyName()).isEqualTo("least_request_experimental"); - assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "choiceCount")) - .isEqualTo(ClientXdsClient.DEFAULT_LEAST_REQUEST_CHOICE_COUNT); + assertThat(update.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "least_request_experimental"); } @Test @@ -1795,86 +1782,6 @@ public class ClientXdsClientDataTest { LoadBalancerRegistry.getDefaultRegistry()); } - @Test - public void parseCluster_ringHashLbPolicy_invalidRingSizeConfig_minGreaterThanMax() - throws ResourceInvalidException { - Cluster cluster = Cluster.newBuilder() - .setName("cluster-foo.googleapis.com") - .setType(DiscoveryType.EDS) - .setEdsClusterConfig( - EdsClusterConfig.newBuilder() - .setEdsConfig( - ConfigSource.newBuilder() - .setAds(AggregatedConfigSource.getDefaultInstance())) - .setServiceName("service-foo.googleapis.com")) - .setLbPolicy(LbPolicy.RING_HASH) - .setRingHashLbConfig( - RingHashLbConfig.newBuilder() - .setHashFunction(HashFunction.XX_HASH) - .setMinimumRingSize(UInt64Value.newBuilder().setValue(1000L)) - .setMaximumRingSize(UInt64Value.newBuilder().setValue(100L))) - .build(); - - thrown.expect(ResourceInvalidException.class); - thrown.expectMessage("Cluster cluster-foo.googleapis.com: invalid ring_hash_lb_config"); - ClientXdsClient.processCluster(cluster, new HashSet(), null, LRS_SERVER_INFO, - LoadBalancerRegistry.getDefaultRegistry()); - } - - @Test - public void parseCluster_ringHashLbPolicy_invalidRingSizeConfig_tooLargeRingSize() - throws ResourceInvalidException { - Cluster cluster = Cluster.newBuilder() - .setName("cluster-foo.googleapis.com") - .setType(DiscoveryType.EDS) - .setEdsClusterConfig( - EdsClusterConfig.newBuilder() - .setEdsConfig( - ConfigSource.newBuilder() - .setAds(AggregatedConfigSource.getDefaultInstance())) - .setServiceName("service-foo.googleapis.com")) - .setLbPolicy(LbPolicy.RING_HASH) - .setRingHashLbConfig( - RingHashLbConfig.newBuilder() - .setHashFunction(HashFunction.XX_HASH) - .setMinimumRingSize(UInt64Value.newBuilder().setValue(1000L)) - .setMaximumRingSize( - UInt64Value.newBuilder() - .setValue(ClientXdsClient.MAX_RING_HASH_LB_POLICY_RING_SIZE + 1))) - .build(); - - thrown.expect(ResourceInvalidException.class); - thrown.expectMessage("Cluster cluster-foo.googleapis.com: invalid ring_hash_lb_config"); - ClientXdsClient.processCluster(cluster, new HashSet(), null, LRS_SERVER_INFO, - LoadBalancerRegistry.getDefaultRegistry()); - } - - @Test - public void parseCluster_leastRequestLbPolicy_invalidChoiceCountConfig_tooSmallChoiceCount() - throws ResourceInvalidException { - ClientXdsClient.enableLeastRequest = true; - Cluster cluster = Cluster.newBuilder() - .setName("cluster-foo.googleapis.com") - .setType(DiscoveryType.EDS) - .setEdsClusterConfig( - EdsClusterConfig.newBuilder() - .setEdsConfig( - ConfigSource.newBuilder() - .setAds(AggregatedConfigSource.getDefaultInstance())) - .setServiceName("service-foo.googleapis.com")) - .setLbPolicy(LbPolicy.LEAST_REQUEST) - .setLeastRequestLbConfig( - LeastRequestLbConfig.newBuilder() - .setChoiceCount(UInt32Value.newBuilder().setValue(1)) - ) - .build(); - - thrown.expect(ResourceInvalidException.class); - thrown.expectMessage("Cluster cluster-foo.googleapis.com: invalid least_request_lb_config"); - ClientXdsClient.processCluster(cluster, new HashSet(), null, LRS_SERVER_INFO, - LoadBalancerRegistry.getDefaultRegistry()); - } - @Test public void parseCluster_validateEdsSourceConfig() throws ResourceInvalidException { Set retainedEdsResources = new HashSet<>(); diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java index 3bf3f17cd4..9959b252c8 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java @@ -56,9 +56,7 @@ import io.grpc.internal.BackoffPolicy; import io.grpc.internal.FakeClock; import io.grpc.internal.FakeClock.ScheduledTask; import io.grpc.internal.FakeClock.TaskFilter; -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.internal.TimeProvider; import io.grpc.testing.GrpcCleanupRule; import io.grpc.xds.AbstractXdsClient.ResourceType; @@ -72,7 +70,9 @@ import io.grpc.xds.Endpoints.LocalityLbEndpoints; import io.grpc.xds.EnvoyProtoData.Node; import io.grpc.xds.EnvoyServerProtoData.FilterChain; import io.grpc.xds.FaultConfig.FractionalPercent.DenominatorType; +import io.grpc.xds.LeastRequestLoadBalancer.LeastRequestConfig; import io.grpc.xds.LoadStatsManager2.ClusterDropStats; +import io.grpc.xds.RingHashLoadBalancer.RingHashConfig; import io.grpc.xds.XdsClient.CdsResourceWatcher; import io.grpc.xds.XdsClient.CdsUpdate; import io.grpc.xds.XdsClient.CdsUpdate.ClusterType; @@ -1617,8 +1617,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1640,8 +1640,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1668,9 +1668,10 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()); - assertThat(lbConfig.getPolicyName()).isEqualTo("least_request_experimental"); - assertThat(lbConfig.getRawConfigValue().get("choiceCount")).isEqualTo(3); + PolicySelection policySelection = cdsUpdate.lbPolicySelection(); + assertThat(policySelection.getProvider().getPolicyName()).isEqualTo( + "least_request_experimental"); + assertThat(((LeastRequestConfig) policySelection.getConfig()).choiceCount).isEqualTo(3); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1696,12 +1697,10 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()); - assertThat(lbConfig.getPolicyName()).isEqualTo("ring_hash_experimental"); - assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "minRingSize")).isEqualTo( - 10L); - assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "maxRingSize")).isEqualTo( - 100L); + PolicySelection policySelection = cdsUpdate.lbPolicySelection(); + assertThat(policySelection.getProvider().getPolicyName()).isEqualTo("ring_hash_experimental"); + assertThat(((RingHashConfig) policySelection.getConfig()).minRingSize).isEqualTo(10L); + assertThat(((RingHashConfig) policySelection.getConfig()).maxRingSize).isEqualTo(100L); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1725,8 +1724,8 @@ public abstract class ClientXdsClientTestBase { CdsUpdate cdsUpdate = cdsUpdateCaptor.getValue(); assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.AGGREGATE); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.prioritizedClusterNames()).containsExactlyElementsIn(candidates).inOrder(); verifyResourceMetadataAcked(CDS, CDS_RESOURCE, clusterAggregate, VERSION_1, TIME_INCREMENT); verifySubscribedResourcesMetadataSizes(0, 1, 0, 0); @@ -1747,8 +1746,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isEqualTo(200L); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1898,8 +1897,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1941,8 +1940,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.LOGICAL_DNS); assertThat(cdsUpdate.dnsHostName()).isEqualTo(dnsHostAddr + ":" + dnsHostPort); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1961,8 +1960,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isEqualTo(edsService); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isEqualTo(lrsServerInfo); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -1983,8 +1982,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isNull(); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -2036,8 +2035,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(CDS_RESOURCE); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.LOGICAL_DNS); assertThat(cdsUpdate.dnsHostName()).isEqualTo(dnsHostAddr + ":" + dnsHostPort); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isNull(); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -2046,8 +2045,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(cdsResourceTwo); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isEqualTo(edsService); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isEqualTo(lrsServerInfo); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); @@ -2056,8 +2055,8 @@ public abstract class ClientXdsClientTestBase { assertThat(cdsUpdate.clusterName()).isEqualTo(cdsResourceTwo); assertThat(cdsUpdate.clusterType()).isEqualTo(ClusterType.EDS); assertThat(cdsUpdate.edsServiceName()).isEqualTo(edsService); - assertThat(ServiceConfigUtil.unwrapLoadBalancingConfig(cdsUpdate.lbPolicyConfig()) - .getPolicyName()).isEqualTo("round_robin"); + assertThat(cdsUpdate.lbPolicySelection().getProvider().getPolicyName()).isEqualTo( + "round_robin"); assertThat(cdsUpdate.lrsServerInfo()).isEqualTo(lrsServerInfo); assertThat(cdsUpdate.maxConcurrentRequests()).isNull(); assertThat(cdsUpdate.upstreamTlsContext()).isNull(); diff --git a/xds/src/test/java/io/grpc/xds/LegacyLoadBalancerConfigBuilderTest.java b/xds/src/test/java/io/grpc/xds/LegacyLoadBalancerConfigFactoryTest.java similarity index 60% rename from xds/src/test/java/io/grpc/xds/LegacyLoadBalancerConfigBuilderTest.java rename to xds/src/test/java/io/grpc/xds/LegacyLoadBalancerConfigFactoryTest.java index 60460bf3f7..f90a181a18 100644 --- a/xds/src/test/java/io/grpc/xds/LegacyLoadBalancerConfigBuilderTest.java +++ b/xds/src/test/java/io/grpc/xds/LegacyLoadBalancerConfigFactoryTest.java @@ -35,17 +35,17 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; /** - * Unit test for {@link LegacyLoadBalancerConfigBuilder}. + * Unit test for {@link LegacyLoadBalancerConfigFactory}. */ @RunWith(JUnit4.class) -public class LegacyLoadBalancerConfigBuilderTest { +public class LegacyLoadBalancerConfigFactoryTest { @Test public void roundRobin() throws ResourceInvalidException { Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.ROUND_ROBIN).build(); LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); + LegacyLoadBalancerConfigFactory.newConfig(cluster, true)); assertThat(lbConfig.getPolicyName()).isEqualTo("round_robin"); assertThat(lbConfig.getRawConfigValue()).isEmpty(); @@ -59,7 +59,7 @@ public class LegacyLoadBalancerConfigBuilderTest { .setMaximumRingSize(UInt64Value.newBuilder().setValue(2).build()).build()).build(); LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); + LegacyLoadBalancerConfigFactory.newConfig(cluster, true)); assertThat(lbConfig.getPolicyName()).isEqualTo("ring_hash_experimental"); assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "minRingSize")).isEqualTo(1); @@ -73,42 +73,9 @@ public class LegacyLoadBalancerConfigBuilderTest { try { ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); + LegacyLoadBalancerConfigFactory.newConfig(cluster, true)); } catch (ResourceInvalidException e) { - assertThat(e).hasMessageThat().contains("invalid ring_hash_lb_config"); - return; - } - fail("ResourceInvalidException not thrown"); - } - - @Test - public void ringHash_minRingSizeLargerThanMax() throws ResourceInvalidException { - Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.RING_HASH).setRingHashLbConfig( - RingHashLbConfig.newBuilder() - .setMinimumRingSize(UInt64Value.newBuilder().setValue(2).build()) - .setMaximumRingSize(UInt64Value.newBuilder().setValue(1).build())).build(); - - try { - ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); - } catch (ResourceInvalidException e) { - assertThat(e).hasMessageThat().contains("invalid ring_hash_lb_config"); - return; - } - fail("ResourceInvalidException not thrown"); - } - - @Test - public void ringHash_maxRingSizeTooLarge() throws ResourceInvalidException { - Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.RING_HASH).setRingHashLbConfig( - RingHashLbConfig.newBuilder() - .setMaximumRingSize(UInt64Value.newBuilder().setValue(Long.MAX_VALUE).build())).build(); - - try { - ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); - } catch (ResourceInvalidException e) { - assertThat(e).hasMessageThat().contains("invalid ring_hash_lb_config"); + assertThat(e).hasMessageThat().contains("invalid ring hash function"); return; } fail("ResourceInvalidException not thrown"); @@ -123,29 +90,12 @@ public class LegacyLoadBalancerConfigBuilderTest { .setChoiceCount(UInt32Value.newBuilder().setValue(10).build())).build(); LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); + LegacyLoadBalancerConfigFactory.newConfig(cluster, true)); assertThat(lbConfig.getPolicyName()).isEqualTo("least_request_experimental"); assertThat(JsonUtil.getNumberAsLong(lbConfig.getRawConfigValue(), "choiceCount")).isEqualTo(10); } - @Test - public void leastRequest_choiceCountTooSmall() throws ResourceInvalidException { - System.setProperty("io.grpc.xds.experimentalEnableLeastRequest", "true"); - - Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.LEAST_REQUEST) - .setLeastRequestLbConfig(LeastRequestLbConfig.newBuilder() - .setChoiceCount(UInt32Value.newBuilder().setValue(1).build())).build(); - - try { - ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, true).build()); - } catch (ResourceInvalidException e) { - assertThat(e).hasMessageThat().contains("invalid least_request_lb_config"); - return; - } - fail("ResourceInvalidException not thrown"); - } @Test public void leastRequest_notEnabled() throws ResourceInvalidException { @@ -155,7 +105,7 @@ public class LegacyLoadBalancerConfigBuilderTest { try { ServiceConfigUtil.unwrapLoadBalancingConfig( - LegacyLoadBalancerConfigBuilder.forCluster(cluster, false).build()); + LegacyLoadBalancerConfigFactory.newConfig(cluster, false)); } catch (ResourceInvalidException e) { assertThat(e).hasMessageThat().contains("unsupported lb policy"); return;