From c88feeffe833b38e4f0a15b9e6073074f02fd951 Mon Sep 17 00:00:00 2001 From: Chengyuan Zhang Date: Tue, 8 Sep 2020 18:25:28 -0700 Subject: [PATCH] xds: always generate xds_routing LB policy as the top-level LB policy in legacy xDS resolver (to be deprecated) (#7401) Now that routing is enabled by default, we should always generate a load balancing config that uses xds_routing as the top-level LB policy. This change is made to the legacy xDS resolver, which will soon be replaced by XdsNameResolver2. But this change allows us to merge split codepath for CDS LB policy. --- .../java/io/grpc/xds/XdsNameResolver.java | 36 +----- .../xds/XdsNameResolverIntegrationTest.java | 109 +++++------------- 2 files changed, 33 insertions(+), 112 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index e0c03e5f90..a94360848f 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -23,7 +23,6 @@ import com.google.common.base.Stopwatch; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.gson.Gson; import com.google.re2j.Pattern; import io.grpc.Attributes; @@ -155,35 +154,12 @@ final class XdsNameResolver extends NameResolver { @Override public void onConfigChanged(ConfigUpdate update) { - Map rawLbConfig; - if (update.getRoutes().size() > 1) { - logger.log( - XdsLogLevel.INFO, - "Received config update with {0} routes from xDS client {1}", - update.getRoutes().size(), - xdsClient); - rawLbConfig = generateXdsRoutingRawConfig(update.getRoutes()); - } else { - Route defaultRoute = Iterables.getOnlyElement(update.getRoutes()); - RouteAction action = defaultRoute.getRouteAction(); - String clusterName = defaultRoute.getRouteAction().getCluster(); - if (action.getCluster() != null) { - logger.log( - XdsLogLevel.INFO, - "Received config update from xDS client {0}: cluster_name={1}", - xdsClient, - clusterName); - rawLbConfig = generateCdsRawConfig(clusterName); - } else { - logger.log( - XdsLogLevel.INFO, - "Received config update with one weighted cluster route from xDS client {0}", - xdsClient); - List clusterWeights = defaultRoute.getRouteAction().getWeightedCluster(); - rawLbConfig = generateWeightedTargetRawConfig(clusterWeights); - } - } - + logger.log( + XdsLogLevel.INFO, + "Received config update with {0} routes from xDS client {1}", + update.getRoutes().size(), + xdsClient); + Map rawLbConfig = generateXdsRoutingRawConfig(update.getRoutes()); Map serviceConfig = ImmutableMap.of("loadBalancingConfig", ImmutableList.of(rawLbConfig)); if (logger.isLoggable(XdsLogLevel.INFO)) { diff --git a/xds/src/test/java/io/grpc/xds/XdsNameResolverIntegrationTest.java b/xds/src/test/java/io/grpc/xds/XdsNameResolverIntegrationTest.java index d911b34ec0..e843fd14a4 100644 --- a/xds/src/test/java/io/grpc/xds/XdsNameResolverIntegrationTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsNameResolverIntegrationTest.java @@ -22,7 +22,6 @@ import static io.grpc.xds.XdsClientTestHelper.buildListenerV2; import static io.grpc.xds.XdsClientTestHelper.buildRouteConfigurationV2; import static io.grpc.xds.XdsClientTestHelper.buildVirtualHostV2; import static io.grpc.xds.XdsNameResolverTest.assertCdsPolicy; -import static io.grpc.xds.XdsNameResolverTest.assertWeightedTargetConfigClusterWeights; import static io.grpc.xds.XdsNameResolverTest.assertWeightedTargetPolicy; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -295,9 +294,15 @@ public class XdsNameResolverIntegrationTest { List> rawLbConfigs = (List>) serviceConfig.get("loadBalancingConfig"); Map lbConfig = Iterables.getOnlyElement(rawLbConfigs); - assertThat(lbConfig.keySet()).containsExactly("cds_experimental"); - Map rawConfigValues = (Map) lbConfig.get("cds_experimental"); - assertThat(rawConfigValues).containsExactly("cluster", "cluster-foo.googleapis.com"); + assertThat(lbConfig.keySet()).containsExactly("xds_routing_experimental"); + Map rawConfigValues = (Map) lbConfig.get("xds_routing_experimental"); + Map> actions = + (Map>) rawConfigValues.get("action"); + List> routes = (List>) rawConfigValues.get("route"); + Map route = Iterables.getOnlyElement(routes); + assertThat(route.keySet()).containsExactly("prefix", "action"); + assertThat((String) route.get("prefix")).isEqualTo(""); + assertCdsPolicy(actions.get(route.get("action")), "cluster-foo.googleapis.com"); // Simulate receiving another LDS response that tells client to do RDS. String routeConfigName = "route-foo.googleapis.com"; @@ -318,33 +323,14 @@ public class XdsNameResolverIntegrationTest { serviceConfig = (Map) result.getServiceConfig().getConfig(); rawLbConfigs = (List>) serviceConfig.get("loadBalancingConfig"); lbConfig = Iterables.getOnlyElement(rawLbConfigs); - assertThat(lbConfig.keySet()).containsExactly("cds_experimental"); - rawConfigValues = (Map) lbConfig.get("cds_experimental"); - assertThat(rawConfigValues).containsExactly("cluster", "cluster-blade.googleapis.com"); - } - - @SuppressWarnings("unchecked") - @Test - public void resolve_cdsLoadBalancing() { - xdsNameResolver.start(mockListener); - assertThat(responseObservers).hasSize(1); - StreamObserver responseObserver = responseObservers.poll(); - - // Simulate receiving an LDS response that contains cluster resolution directly in-line. - String clusterName = "cluster-foo.googleapis.com"; - responseObserver.onNext( - buildLdsResponseForCluster("0", AUTHORITY, clusterName, "0000")); - - verify(mockListener).onResult(resolutionResultCaptor.capture()); - ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); - Map serviceConfig = (Map) result.getServiceConfig().getConfig(); - List> rawLbConfigs = - (List>) serviceConfig.get("loadBalancingConfig"); - Map lbConfig = Iterables.getOnlyElement(rawLbConfigs); - assertThat(lbConfig.keySet()).containsExactly("cds_experimental"); - Map rawConfigValues = (Map) lbConfig.get("cds_experimental"); - assertThat(rawConfigValues).containsExactly("cluster", clusterName); + assertThat(lbConfig.keySet()).containsExactly("xds_routing_experimental"); + rawConfigValues = (Map) lbConfig.get("xds_routing_experimental"); + actions = (Map>) rawConfigValues.get("action"); + routes = (List>) rawConfigValues.get("route"); + route = Iterables.getOnlyElement(routes); + assertThat(route.keySet()).containsExactly("prefix", "action"); + assertThat((String) route.get("prefix")).isEqualTo(""); + assertCdsPolicy(actions.get(route.get("action")), "cluster-blade.googleapis.com"); } @Test @@ -447,54 +433,6 @@ public class XdsNameResolverIntegrationTest { assertCdsPolicy(actions.get(route4.get("action")), "cluster-hello.googleapis.com"); } - @SuppressWarnings("unchecked") - @Test - public void resolve_weightedTargetLoadBalancing() { - xdsNameResolver.start(mockListener); - assertThat(responseObservers).hasSize(1); - StreamObserver responseObserver = responseObservers.poll(); - - // Simulate receiving another LDS response that tells client to do RDS. - String routeConfigName = "route-foo.googleapis.com"; - responseObserver.onNext( - buildLdsResponseForRdsResource("1", AUTHORITY, routeConfigName, "0001")); - - // Client sent an RDS request for resource "route-foo.googleapis.com" (Omitted in this test). - - // Simulate receiving an RDS response that contains the resource "route-foo.googleapis.com" - // with a route resolution for a single weighted cluster route. - Route weightedClustersDefaultRoute = - Route.newBuilder() - .setMatch(RouteMatch.newBuilder().setPrefix("")) - .setRoute(buildWeightedClusterRoute( - ImmutableMap.of( - "cluster-foo.googleapis.com", 20, "cluster-bar.googleapis.com", 80))) - .build(); - List routeConfigs = ImmutableList.of( - Any.pack( - buildRouteConfigurationV2( - routeConfigName, - ImmutableList.of( - buildVirtualHostForRoutes( - AUTHORITY, ImmutableList.of(weightedClustersDefaultRoute)))))); - responseObserver.onNext( - buildDiscoveryResponseV2("0", routeConfigs, XdsClientImpl.ADS_TYPE_URL_RDS_V2, "0000")); - - verify(mockListener).onResult(resolutionResultCaptor.capture()); - ResolutionResult result = resolutionResultCaptor.getValue(); - assertThat(result.getAddresses()).isEmpty(); - Map serviceConfig = (Map) result.getServiceConfig().getConfig(); - List> rawLbConfigs = - (List>) serviceConfig.get("loadBalancingConfig"); - Map lbConfig = Iterables.getOnlyElement(rawLbConfigs); - assertThat(lbConfig.keySet()).containsExactly("weighted_target_experimental"); - Map rawConfigValues = (Map) lbConfig.get("weighted_target_experimental"); - assertWeightedTargetConfigClusterWeights( - rawConfigValues, - ImmutableMap.of( - "cluster-foo.googleapis.com", 20, "cluster-bar.googleapis.com", 80)); - } - @Test @SuppressWarnings("unchecked") public void resolve_resourceNewlyAdded() { @@ -511,6 +449,7 @@ public class XdsNameResolverIntegrationTest { verify(mockListener).onResult(resolutionResultCaptor.capture()); ResolutionResult result = resolutionResultCaptor.getValue(); assertThat(result.getAddresses()).isEmpty(); + assertThat((Map) result.getServiceConfig().getConfig()).isEmpty(); // Simulate receiving another LDS response that contains cluster resolution directly in-line. responseObserver.onNext( @@ -524,9 +463,15 @@ public class XdsNameResolverIntegrationTest { List> rawLbConfigs = (List>) serviceConfig.get("loadBalancingConfig"); Map lbConfig = Iterables.getOnlyElement(rawLbConfigs); - assertThat(lbConfig.keySet()).containsExactly("cds_experimental"); - Map rawConfigValues = (Map) lbConfig.get("cds_experimental"); - assertThat(rawConfigValues).containsExactly("cluster", "cluster-foo.googleapis.com"); + assertThat(lbConfig.keySet()).containsExactly("xds_routing_experimental"); + Map rawConfigValues = (Map) lbConfig.get("xds_routing_experimental"); + Map> actions = + (Map>) rawConfigValues.get("action"); + List> routes = (List>) rawConfigValues.get("route"); + Map route = Iterables.getOnlyElement(routes); + assertThat(route.keySet()).containsExactly("prefix", "action"); + assertThat((String) route.get("prefix")).isEqualTo(""); + assertCdsPolicy(actions.get(route.get("action")), "cluster-foo.googleapis.com"); } /**