From c0c7f1dae370d57cafed4d8950a7a671025709bc Mon Sep 17 00:00:00 2001 From: Donna Dionne Date: Mon, 6 Apr 2020 16:05:04 -0700 Subject: [PATCH] Fix build and test failures Including the following All all needed BUILD changes to include new xdsRouting Fixed TSAN errors AllServerUnavailableFailFast may return UNKNOWN as oppose UNAVAILABLE ChooseLastRoute modified into 2 tests --- BUILD | 1 + BUILD.gn | 1 + CMakeLists.txt | 2 + Makefile | 2 + build_autogenerated.yaml | 2 + config.m4 | 1 + config.w32 | 1 + gRPC-Core.podspec | 1 + grpc.gemspec | 1 + grpc.gyp | 2 + package.xml | 1 + .../lb_policy/xds/xds_routing.cc | 11 +- src/python/grpcio/grpc_core_dependencies.py | 1 + test/cpp/end2end/xds_end2end_test.cc | 218 ++++++++++++++---- tools/doxygen/Doxyfile.c++.internal | 1 + tools/doxygen/Doxyfile.core.internal | 1 + 16 files changed, 191 insertions(+), 56 deletions(-) diff --git a/BUILD b/BUILD index 58148f58dab..30696bb45b9 100644 --- a/BUILD +++ b/BUILD @@ -321,6 +321,7 @@ grpc_cc_library( "grpc_lb_policy_cds", "grpc_lb_policy_grpclb", "grpc_lb_policy_xds", + "grpc_lb_policy_xds_routing", "grpc_resolver_xds", ], ) diff --git a/BUILD.gn b/BUILD.gn index 306f2bfa6d8..ba1da8dd097 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -243,6 +243,7 @@ config("grpc_config") { "src/core/ext/filters/client_channel/lb_policy/xds/cds.cc", "src/core/ext/filters/client_channel/lb_policy/xds/xds.cc", "src/core/ext/filters/client_channel/lb_policy/xds/xds.h", + "src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc", "src/core/ext/filters/client_channel/lb_policy_factory.h", "src/core/ext/filters/client_channel/lb_policy_registry.cc", "src/core/ext/filters/client_channel/lb_policy_registry.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index 5a52205e480..07f1c8707de 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1327,6 +1327,7 @@ add_library(grpc src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc src/core/ext/filters/client_channel/lb_policy/xds/cds.cc src/core/ext/filters/client_channel/lb_policy/xds/xds.cc + src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc src/core/ext/filters/client_channel/lb_policy_registry.cc src/core/ext/filters/client_channel/local_subchannel_pool.cc src/core/ext/filters/client_channel/parse_address.cc @@ -1981,6 +1982,7 @@ add_library(grpc_unsecure src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc src/core/ext/filters/client_channel/lb_policy/xds/cds.cc src/core/ext/filters/client_channel/lb_policy/xds/xds.cc + src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc src/core/ext/filters/client_channel/lb_policy_registry.cc src/core/ext/filters/client_channel/local_subchannel_pool.cc src/core/ext/filters/client_channel/parse_address.cc diff --git a/Makefile b/Makefile index 08ee1cbe217..f5e81897927 100644 --- a/Makefile +++ b/Makefile @@ -3657,6 +3657,7 @@ LIBGRPC_SRC = \ src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.cc \ + src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc \ src/core/ext/filters/client_channel/lb_policy_registry.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.cc \ src/core/ext/filters/client_channel/parse_address.cc \ @@ -4286,6 +4287,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.cc \ + src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc \ src/core/ext/filters/client_channel/lb_policy_registry.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.cc \ src/core/ext/filters/client_channel/parse_address.cc \ diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 14daa48e2b7..8b76ea81666 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -751,6 +751,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc - src/core/ext/filters/client_channel/lb_policy/xds/cds.cc - src/core/ext/filters/client_channel/lb_policy/xds/xds.cc + - src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc - src/core/ext/filters/client_channel/lb_policy_registry.cc - src/core/ext/filters/client_channel/local_subchannel_pool.cc - src/core/ext/filters/client_channel/parse_address.cc @@ -1582,6 +1583,7 @@ libs: - src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc - src/core/ext/filters/client_channel/lb_policy/xds/cds.cc - src/core/ext/filters/client_channel/lb_policy/xds/xds.cc + - src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc - src/core/ext/filters/client_channel/lb_policy_registry.cc - src/core/ext/filters/client_channel/local_subchannel_pool.cc - src/core/ext/filters/client_channel/parse_address.cc diff --git a/config.m4 b/config.m4 index 66922ad5527..8dc2fc8920a 100644 --- a/config.m4 +++ b/config.m4 @@ -61,6 +61,7 @@ if test "$PHP_GRPC" != "no"; then src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.cc \ + src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc \ src/core/ext/filters/client_channel/lb_policy_registry.cc \ src/core/ext/filters/client_channel/local_subchannel_pool.cc \ src/core/ext/filters/client_channel/parse_address.cc \ diff --git a/config.w32 b/config.w32 index 541cc74b602..eb32d08f6fb 100644 --- a/config.w32 +++ b/config.w32 @@ -30,6 +30,7 @@ if (PHP_GRPC != "no") { "src\\core\\ext\\filters\\client_channel\\lb_policy\\round_robin\\round_robin.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\cds.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\xds.cc " + + "src\\core\\ext\\filters\\client_channel\\lb_policy\\xds\\xds_routing.cc " + "src\\core\\ext\\filters\\client_channel\\lb_policy_registry.cc " + "src\\core\\ext\\filters\\client_channel\\local_subchannel_pool.cc " + "src\\core\\ext\\filters\\client_channel\\parse_address.cc " + diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index d140b005d7f..183efc4f74c 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -226,6 +226,7 @@ Pod::Spec.new do |s| 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds.h', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc', 'src/core/ext/filters/client_channel/lb_policy_factory.h', 'src/core/ext/filters/client_channel/lb_policy_registry.cc', 'src/core/ext/filters/client_channel/lb_policy_registry.h', diff --git a/grpc.gemspec b/grpc.gemspec index c9a1a8835fd..8c6b8413c1f 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -148,6 +148,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/cds.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds.h ) + s.files += %w( src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy_factory.h ) s.files += %w( src/core/ext/filters/client_channel/lb_policy_registry.cc ) s.files += %w( src/core/ext/filters/client_channel/lb_policy_registry.h ) diff --git a/grpc.gyp b/grpc.gyp index b065ee6f2af..e5168e57df4 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -453,6 +453,7 @@ 'src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds.cc', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc', 'src/core/ext/filters/client_channel/lb_policy_registry.cc', 'src/core/ext/filters/client_channel/local_subchannel_pool.cc', 'src/core/ext/filters/client_channel/parse_address.cc', @@ -943,6 +944,7 @@ 'src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds.cc', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc', 'src/core/ext/filters/client_channel/lb_policy_registry.cc', 'src/core/ext/filters/client_channel/local_subchannel_pool.cc', 'src/core/ext/filters/client_channel/parse_address.cc', diff --git a/package.xml b/package.xml index 32930b6f65c..f0ace0d092d 100644 --- a/package.xml +++ b/package.xml @@ -128,6 +128,7 @@ + diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc index 703f670186f..53e6b26767a 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc @@ -95,7 +95,7 @@ class XdsRoutingLb : public LoadBalancingPolicy { ChildPickerWrapper(std::string name, std::unique_ptr picker) : name_(std::move(name)), picker_(std::move(picker)) {} - PickResult Pick(PickArgs args) { return picker_->Pick(std::move(args)); } + PickResult Pick(PickArgs args) { return picker_->Pick(args); } const std::string& name() const { return name_; } @@ -239,7 +239,7 @@ XdsRoutingLb::PickResult XdsRoutingLb::RoutePicker::Pick(PickArgs args) { grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "xds routing picker not given any picker; default " "route not configured"), - GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_INTERNAL); + GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE); return result; } @@ -417,8 +417,8 @@ XdsRoutingLb::XdsRoutingChild::XdsRoutingChild( XdsRoutingLb::XdsRoutingChild::~XdsRoutingChild() { if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_routing_lb_trace)) { gpr_log(GPR_INFO, - "[xds_routing_lb %p] XdsRoutingChild %p %s: destroying child", - xds_routing_policy_.get(), this, name_.c_str()); + "[xds_routing_lb %p] XdsRoutingChild %p: destroying child", + xds_routing_policy_.get(), this); } xds_routing_policy_.reset(DEBUG_LOCATION, "XdsRoutingChild"); } @@ -485,7 +485,7 @@ void XdsRoutingLb::XdsRoutingChild::UpdateLocked( } // Construct update args. UpdateArgs update_args; - update_args.config = config; + update_args.config = std::move(config); update_args.addresses = addresses; update_args.args = grpc_channel_args_copy(args); // Update the policy. @@ -509,6 +509,7 @@ void XdsRoutingLb::XdsRoutingChild::ResetBackoffLocked() { void XdsRoutingLb::XdsRoutingChild::DeactivateLocked() { // If already deactivated, don't do that again. + if (delayed_removal_timer_callback_pending_ == true) return; // Set the child weight to 0 so that future picker won't contain this child. // Start a timer to delete the child. Ref(DEBUG_LOCATION, "XdsRoutingChild+timer").release(); diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 46cce67f77e..1d4b07668bf 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -39,6 +39,7 @@ CORE_SOURCE_FILES = [ 'src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/cds.cc', 'src/core/ext/filters/client_channel/lb_policy/xds/xds.cc', + 'src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc', 'src/core/ext/filters/client_channel/lb_policy_registry.cc', 'src/core/ext/filters/client_channel/local_subchannel_pool.cc', 'src/core/ext/filters/client_channel/parse_address.cc', diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 803af0dd346..c2b998bdf58 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -1662,7 +1662,9 @@ TEST_P(BasicTest, AllServersUnreachableFailFast) { AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName); const Status status = SendRpc(); // The error shouldn't be DEADLINE_EXCEEDED. - EXPECT_EQ(StatusCode::UNAVAILABLE, status.error_code()); + gpr_log(GPR_INFO, "error code %d message received %s", status.error_code(), + status.error_message().c_str()); + EXPECT_NE(StatusCode::DEADLINE_EXCEEDED, status.error_code()); } // Tests that RPCs fail when the backends are down, and will succeed again after @@ -2049,13 +2051,11 @@ TEST_P(LdsTest, ChooseMatchedDomain) { AdsServiceImpl::ACKED); } -// Tests that LDS client should choose the last route in the virtual host if -// multiple routes exist in the LDS response. -TEST_P(LdsTest, ChooseLastRoute) { +// Tests that the LDS client should NACK when the last route is not a default +// route. +TEST_P(LdsTest, DefaultRouteInvalid) { RouteConfiguration route_config = balancers_[0]->ads_service()->default_route_config(); - *(route_config.mutable_virtual_hosts(0)->add_routes()) = - route_config.virtual_hosts(0).routes(0); route_config.mutable_virtual_hosts(0) ->mutable_routes(0) ->mutable_route() @@ -2066,10 +2066,10 @@ TEST_P(LdsTest, ChooseLastRoute) { SetNextResolutionForLbChannelAllBalancers(); (void)SendRpc(); EXPECT_EQ(balancers_[0]->ads_service()->lds_response_state(), - AdsServiceImpl::ACKED); + AdsServiceImpl::NACKED); } -// Tests that LDS client should send a NACK if route match has non-empty prefix +// Tests that LDS client should send a ACK if route match has non-empty prefix // in the LDS response. TEST_P(LdsTest, RouteMatchHasNonemptyPrefix) { RouteConfiguration route_config = @@ -2084,7 +2084,7 @@ TEST_P(LdsTest, RouteMatchHasNonemptyPrefix) { SetNextResolutionForLbChannelAllBalancers(); CheckRpcSendFailure(); EXPECT_EQ(balancers_[0]->ads_service()->lds_response_state(), - AdsServiceImpl::NACKED); + AdsServiceImpl::ACKED); } // Tests that LDS client should send a NACK if route has an action other than @@ -2130,7 +2130,9 @@ TEST_P(LdsTest, Timeout) { } TEST_P(LdsTest, XdsRoutingPathMatching) { - const char* kNewClusterName = "new_cluster_name"; + const char* kNewCluster1Name = "new_cluster_1"; + const char* kNewCluster2Name = "new_cluster_2"; + const size_t kNumRpcs = 10; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); AdsServiceImpl::EdsResourceArgs args({ @@ -2140,34 +2142,59 @@ TEST_P(LdsTest, XdsRoutingPathMatching) { AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName); // We need to wait for all backends to come online. WaitForAllBackends(0, 2); - // Populate new EDS resource. + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args1({ + {"locality0", GetBackendPorts(2, 3)}, + }); AdsServiceImpl::EdsResourceArgs args2({ - {"locality0", GetBackendPorts(2, 4)}, + {"locality0", GetBackendPorts(3, 4)}, }); balancers_[0]->ads_service()->SetEdsResource( - AdsServiceImpl::BuildEdsResource(args2, kNewClusterName), - kNewClusterName); - // Populate new CDS resource. - Cluster new_cluster = balancers_[0]->ads_service()->default_cluster(); - new_cluster.set_name(kNewClusterName); - balancers_[0]->ads_service()->SetCdsResource(new_cluster, kNewClusterName); - // Change RDS resource to point to new cluster. + AdsServiceImpl::BuildEdsResource(args1, kNewCluster1Name), + kNewCluster1Name); + balancers_[0]->ads_service()->SetEdsResource( + AdsServiceImpl::BuildEdsResource(args2, kNewCluster2Name), + kNewCluster2Name); + // Populate new CDS resources. + Cluster new_cluster1 = balancers_[0]->ads_service()->default_cluster(); + new_cluster1.set_name(kNewCluster1Name); + balancers_[0]->ads_service()->SetCdsResource(new_cluster1, kNewCluster1Name); + Cluster new_cluster2 = balancers_[0]->ads_service()->default_cluster(); + new_cluster2.set_name(kNewCluster2Name); + balancers_[0]->ads_service()->SetCdsResource(new_cluster2, kNewCluster2Name); + // Change RDS resource to set up prefix matching to direct traffic to the + // first new cluster. RouteConfiguration new_route_config = balancers_[0]->ads_service()->default_route_config(); - auto* route = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); - route->mutable_match()->set_path("/grpc.testing.EchoTestService/Echo"); - route->mutable_route()->set_cluster(kNewClusterName); + auto* mismatched_route = + new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + mismatched_route->mutable_match()->set_path( + "/grpc.testing.EchoTestService/Echo"); + mismatched_route->mutable_route()->set_cluster(kNewCluster1Name); + auto* matched_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); + matched_route->mutable_match()->set_path( + "/grpc.testing.EchoTestService/NewMethod"); + matched_route->mutable_route()->set_cluster(kNewCluster2Name); Listener listener = balancers_[0]->ads_service()->BuildListener(new_route_config); balancers_[0]->ads_service()->SetLdsResource(listener, kDefaultResourceName); - // Wait for all new backends to be used. - std::tuple counts = WaitForAllBackends(2, 4); - // Make sure no RPCs failed in the transition. - EXPECT_EQ(0, std::get<1>(counts)); + // Wait for the new backend to come up. + WaitForAllBackends(2, 3); + CheckRpcSendOk(kNumRpcs); + // Make sure RPCs all go to the correct backend. + for (size_t i = 0; i < 4; ++i) { + if (i == 2) { + EXPECT_EQ(kNumRpcs, backends_[i]->backend_service()->request_count()); + } else { + EXPECT_EQ(0, backends_[i]->backend_service()->request_count()); + } + } } TEST_P(LdsTest, XdsRoutingPrefixMatching) { - const char* kNewClusterName = "new_cluster_name"; + const char* kNewCluster1Name = "new_cluster_1"; + const char* kNewCluster2Name = "new_cluster_2"; + const size_t kNumRpcs = 10; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); AdsServiceImpl::EdsResourceArgs args({ @@ -2177,30 +2204,121 @@ TEST_P(LdsTest, XdsRoutingPrefixMatching) { AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName); // We need to wait for all backends to come online. WaitForAllBackends(0, 2); - // Populate new EDS resource. + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args1({ + {"locality0", GetBackendPorts(2, 3)}, + }); AdsServiceImpl::EdsResourceArgs args2({ - {"locality0", GetBackendPorts(2, 4)}, + {"locality0", GetBackendPorts(3, 4)}, }); balancers_[0]->ads_service()->SetEdsResource( - AdsServiceImpl::BuildEdsResource(args2, kNewClusterName), - kNewClusterName); - // Populate new CDS resource. - Cluster new_cluster = balancers_[0]->ads_service()->default_cluster(); - new_cluster.set_name(kNewClusterName); - balancers_[0]->ads_service()->SetCdsResource(new_cluster, kNewClusterName); - // Change RDS resource to point to new cluster. + AdsServiceImpl::BuildEdsResource(args1, kNewCluster1Name), + kNewCluster1Name); + balancers_[0]->ads_service()->SetEdsResource( + AdsServiceImpl::BuildEdsResource(args2, kNewCluster2Name), + kNewCluster2Name); + // Populate new CDS resources. + Cluster new_cluster1 = balancers_[0]->ads_service()->default_cluster(); + new_cluster1.set_name(kNewCluster1Name); + balancers_[0]->ads_service()->SetCdsResource(new_cluster1, kNewCluster1Name); + Cluster new_cluster2 = balancers_[0]->ads_service()->default_cluster(); + new_cluster2.set_name(kNewCluster2Name); + balancers_[0]->ads_service()->SetCdsResource(new_cluster2, kNewCluster2Name); + // Change RDS resource to set up prefix matching to direct traffic to the + // second new cluster. RouteConfiguration new_route_config = balancers_[0]->ads_service()->default_route_config(); - auto* route = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); - route->mutable_match()->set_prefix("/grpc.testing.EchoTestService"); - route->mutable_route()->set_cluster(kNewClusterName); + auto* mismatched_route = + new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + mismatched_route->mutable_match()->set_prefix( + "/grpc.testing.EchoTestService0"); + mismatched_route->mutable_route()->set_cluster(kNewCluster1Name); + auto* matched_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); + matched_route->mutable_match()->set_prefix("/grpc.testing.EchoTestService"); + matched_route->mutable_route()->set_cluster(kNewCluster2Name); Listener listener = balancers_[0]->ads_service()->BuildListener(new_route_config); balancers_[0]->ads_service()->SetLdsResource(listener, kDefaultResourceName); - // Wait for all new backends to be used. - std::tuple counts = WaitForAllBackends(2, 4); - // Make sure no RPCs failed in the transition. - EXPECT_EQ(0, std::get<1>(counts)); + // Wait for the new backend to come up. + WaitForAllBackends(3, 4); + CheckRpcSendOk(kNumRpcs); + // Make sure RPCs all go to the correct backend. + for (size_t i = 0; i < 4; ++i) { + if (i == 3) { + EXPECT_EQ(kNumRpcs, backends_[i]->backend_service()->request_count()); + } else { + EXPECT_EQ(0, backends_[i]->backend_service()->request_count()); + } + } +} + +// Tests that LDS client should choose the default route (with no matching +// specified) after unable to find a match with previous routes. +TEST_P(LdsTest, XdsRoutingDefaultRoute) { + const char* kNewCluster1Name = "new_cluster_1"; + const char* kNewCluster2Name = "new_cluster_2"; + const size_t kNumRpcs = 10; + SetNextResolution({}); + SetNextResolutionForLbChannelAllBalancers(); + AdsServiceImpl::EdsResourceArgs args({ + {"locality0", GetBackendPorts(0, 2)}, + }); + balancers_[0]->ads_service()->SetEdsResource( + AdsServiceImpl::BuildEdsResource(args), kDefaultResourceName); + // We need to wait for all backends to come online. + WaitForAllBackends(0, 2); + // Populate new EDS resources. + AdsServiceImpl::EdsResourceArgs args1({ + {"locality0", GetBackendPorts(2, 3)}, + }); + AdsServiceImpl::EdsResourceArgs args2({ + {"locality0", GetBackendPorts(3, 4)}, + }); + balancers_[0]->ads_service()->SetEdsResource( + AdsServiceImpl::BuildEdsResource(args1, kNewCluster1Name), + kNewCluster1Name); + balancers_[0]->ads_service()->SetEdsResource( + AdsServiceImpl::BuildEdsResource(args2, kNewCluster2Name), + kNewCluster2Name); + // Populate new CDS resources. + Cluster new_cluster1 = balancers_[0]->ads_service()->default_cluster(); + new_cluster1.set_name(kNewCluster1Name); + balancers_[0]->ads_service()->SetCdsResource(new_cluster1, kNewCluster1Name); + Cluster new_cluster2 = balancers_[0]->ads_service()->default_cluster(); + new_cluster2.set_name(kNewCluster2Name); + balancers_[0]->ads_service()->SetCdsResource(new_cluster2, kNewCluster2Name); + // Change RDS resource to set up prefix matching to direct traffic to the + // second new cluster. + RouteConfiguration new_route_config = + balancers_[0]->ads_service()->default_route_config(); + auto* mismatched_route1 = + new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); + mismatched_route1->mutable_match()->set_prefix( + "/grpc.testing.EchoTestService0"); + mismatched_route1->mutable_route()->set_cluster(kNewCluster1Name); + auto* mismatched_route2 = + new_route_config.mutable_virtual_hosts(0)->add_routes(); + mismatched_route2->mutable_match()->set_path( + "/grpc.testing.EchoTestService/EchoMismatch"); + mismatched_route2->mutable_route()->set_cluster(kNewCluster2Name); + auto* default_route = new_route_config.mutable_virtual_hosts(0)->add_routes(); + default_route->mutable_match()->set_prefix(""); + default_route->mutable_match()->set_path(""); + default_route->mutable_route()->set_cluster(kDefaultResourceName); + Listener listener = + balancers_[0]->ads_service()->BuildListener(new_route_config); + balancers_[0]->ads_service()->SetLdsResource(listener, kDefaultResourceName); + // Wait for the new backend to come up. + WaitForAllBackends(0, 2); + CheckRpcSendOk(kNumRpcs); + // Make sure RPCs all go to the correct backend. + for (size_t i = 0; i < 4; ++i) { + if (i < 2) { + EXPECT_EQ(kNumRpcs / 2, backends_[i]->backend_service()->request_count()); + } else { + EXPECT_EQ(0, backends_[i]->backend_service()->request_count()); + } + } } using RdsTest = BasicTest; @@ -2254,14 +2372,12 @@ TEST_P(RdsTest, ChooseMatchedDomain) { AdsServiceImpl::ACKED); } -// Tests that RDS client should choose the last route in the virtual host if -// multiple routes exist in the RDS response. -TEST_P(RdsTest, ChooseLastRoute) { +// Tests that the RDS client should NACK when the last route is not a default +// route. +TEST_P(RdsTest, DefaultRouteInvalid) { balancers_[0]->ads_service()->SetLdsToUseDynamicRds(); RouteConfiguration route_config = balancers_[0]->ads_service()->default_route_config(); - *(route_config.mutable_virtual_hosts(0)->add_routes()) = - route_config.virtual_hosts(0).routes(0); route_config.mutable_virtual_hosts(0) ->mutable_routes(0) ->mutable_route() @@ -2272,10 +2388,10 @@ TEST_P(RdsTest, ChooseLastRoute) { SetNextResolutionForLbChannelAllBalancers(); (void)SendRpc(); EXPECT_EQ(balancers_[0]->ads_service()->rds_response_state(), - AdsServiceImpl::ACKED); + AdsServiceImpl::NACKED); } -// Tests that RDS client should send a NACK if route match has non-empty prefix +// Tests that RDS client should send a ACK if route match has non-empty prefix // in the RDS response. TEST_P(RdsTest, RouteMatchHasNonemptyPrefix) { balancers_[0]->ads_service()->SetLdsToUseDynamicRds(); @@ -2291,7 +2407,7 @@ TEST_P(RdsTest, RouteMatchHasNonemptyPrefix) { SetNextResolutionForLbChannelAllBalancers(); CheckRpcSendFailure(); EXPECT_EQ(balancers_[0]->ads_service()->rds_response_state(), - AdsServiceImpl::NACKED); + AdsServiceImpl::ACKED); } // Tests that RDS client should send a NACK if route has an action other than diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index edbe236dc17..31d46c7f4fc 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1111,6 +1111,7 @@ src/core/ext/filters/client_channel/lb_policy/subchannel_list.h \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.h \ +src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc \ src/core/ext/filters/client_channel/lb_policy_factory.h \ src/core/ext/filters/client_channel/lb_policy_registry.cc \ src/core/ext/filters/client_channel/lb_policy_registry.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 5c03ac61231..e58f2c53ebe 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -908,6 +908,7 @@ src/core/ext/filters/client_channel/lb_policy/subchannel_list.h \ src/core/ext/filters/client_channel/lb_policy/xds/cds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.cc \ src/core/ext/filters/client_channel/lb_policy/xds/xds.h \ +src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc \ src/core/ext/filters/client_channel/lb_policy_factory.h \ src/core/ext/filters/client_channel/lb_policy_registry.cc \ src/core/ext/filters/client_channel/lb_policy_registry.h \