From b53465c1061c9fd31e168046a1a0ac26c13da130 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 8 May 2019 14:27:58 -0700 Subject: [PATCH] Add test for not special casing grpclb if loadbalancingconfig is used --- .../filters/client_channel/client_channel.cc | 118 ++++++++++-------- .../lb_policy/subchannel_list.h | 4 +- test/cpp/end2end/grpclb_end2end_test.cc | 32 +++++ 3 files changed, 103 insertions(+), 51 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 787fe2f4750..1dc6ac48d57 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1156,49 +1156,47 @@ void ChannelData::ProcessLbPolicy( UniquePtr* lb_policy_name, RefCountedPtr* lb_policy_config) { // Prefer the LB policy name found in the service config. - if (parsed_object != nullptr) { - if (parsed_object->parsed_lb_config() != nullptr) { - lb_policy_name->reset( - gpr_strdup(parsed_object->parsed_lb_config()->name())); - *lb_policy_config = parsed_object->parsed_lb_config(); - return; - } else if (parsed_object->parsed_deprecated_lb_policy() != nullptr) { - lb_policy_name->reset( - gpr_strdup(parsed_object->parsed_deprecated_lb_policy())); - } - } - // Otherwise, find the LB policy name set by the client API. - if (*lb_policy_name == nullptr) { - const grpc_arg* channel_arg = - grpc_channel_args_find(resolver_result.args, GRPC_ARG_LB_POLICY_NAME); - lb_policy_name->reset(gpr_strdup(grpc_channel_arg_get_string(channel_arg))); - } - // Special case: If at least one balancer address is present, we use - // the grpclb policy, regardless of what the resolver has returned. - // TODO(yashkt) : Test that we do not use this special case if we have set - // the lb policy from the loadBalancingConfig field - bool found_balancer_address = false; - for (size_t i = 0; i < resolver_result.addresses.size(); ++i) { - const ServerAddress& address = resolver_result.addresses[i]; - if (address.IsBalancer()) { - found_balancer_address = true; - break; + if (parsed_object != nullptr && + parsed_object->parsed_lb_config() != nullptr) { + lb_policy_name->reset( + gpr_strdup(parsed_object->parsed_lb_config()->name())); + *lb_policy_config = parsed_object->parsed_lb_config(); + } else { + const char* local_policy_name = nullptr; + if (parsed_object != nullptr && + parsed_object->parsed_deprecated_lb_policy() != nullptr) { + local_policy_name = parsed_object->parsed_deprecated_lb_policy(); + } else { + const grpc_arg* channel_arg = + grpc_channel_args_find(resolver_result.args, GRPC_ARG_LB_POLICY_NAME); + local_policy_name = grpc_channel_arg_get_string(channel_arg); + } + // Special case: If at least one balancer address is present, we use + // the grpclb policy, regardless of what the resolver has returned. + bool found_balancer_address = false; + for (size_t i = 0; i < resolver_result.addresses.size(); ++i) { + const ServerAddress& address = resolver_result.addresses[i]; + if (address.IsBalancer()) { + found_balancer_address = true; + break; + } } - } - if (found_balancer_address) { - if (*lb_policy_name != nullptr && - strcmp(lb_policy_name->get(), "grpclb") != 0) { - gpr_log(GPR_INFO, - "resolver requested LB policy %s but provided at least one " - "balancer address -- forcing use of grpclb LB policy", - lb_policy_name->get()); + if (found_balancer_address) { + if (local_policy_name != nullptr && + strcmp(local_policy_name, "grpclb") != 0) { + gpr_log(GPR_INFO, + "resolver requested LB policy %s but provided at least one " + "balancer address -- forcing use of grpclb LB policy", + local_policy_name); + } + local_policy_name = "grpclb"; } - lb_policy_name->reset(gpr_strdup("grpclb")); - } - // Use pick_first if nothing was specified and we didn't select grpclb - // above. - if (*lb_policy_name == nullptr) { - lb_policy_name->reset(gpr_strdup("pick_first")); + // Use pick_first if nothing was specified and we didn't select grpclb + // above. + if (local_policy_name == nullptr) { + local_policy_name = "pick_first"; + } + lb_policy_name->reset(gpr_strdup(local_policy_name)); } } @@ -1218,11 +1216,28 @@ bool ChannelData::ProcessResolverResultLocked( // API if (chand->saved_service_config_ != nullptr) { service_config = chand->saved_service_config_; + if (grpc_client_channel_routing_trace.enabled()) { + gpr_log(GPR_INFO, + "chand=%p: resolver returned invalid service config. " + "Continuing to use previous service config.", + chand); + } } else if (chand->default_service_config_ != nullptr) { + if (grpc_client_channel_routing_trace.enabled()) { + gpr_log(GPR_INFO, + "chand=%p: resolver returned invalid service config. Using " + "default service config provided by client API.", + chand); + } service_config = chand->default_service_config_; } } else if (result.service_config == nullptr) { - // We got no service config. Fallback to default service config + if (grpc_client_channel_routing_trace.enabled()) { + gpr_log(GPR_INFO, + "chand=%p: resolver returned no service config. Using default " + "service config provided by client API.", + chand); + } if (chand->default_service_config_ != nullptr) { service_config = chand->default_service_config_; } @@ -1234,7 +1249,7 @@ bool ChannelData::ProcessResolverResultLocked( result.service_config_error != GRPC_ERROR_NONE) { return false; } - char* service_config_json = nullptr; + UniquePtr service_config_json = nullptr; // Process service config. const internal::ClientChannelGlobalParsedObject* parsed_object = nullptr; if (service_config != nullptr) { @@ -1242,15 +1257,20 @@ bool ChannelData::ProcessResolverResultLocked( static_cast( service_config->GetParsedGlobalServiceConfigObject( internal::ClientChannelServiceConfigParser::ParserIndex())); - service_config_json = gpr_strdup(service_config->service_config_json()); } - bool service_config_changed = + const bool service_config_changed = ((service_config == nullptr) != (chand->saved_service_config_ == nullptr)) || (service_config != nullptr && strcmp(service_config->service_config_json(), chand->saved_service_config_->service_config_json()) != 0); if (service_config_changed) { + service_config_json.reset( + gpr_strdup(service_config->service_config_json())); + if (grpc_client_channel_routing_trace.enabled()) { + gpr_log(GPR_INFO, "chand=%p: resolver returned service config: \"%s\"", + chand, service_config_json.get()); + } chand->saved_service_config_ = std::move(service_config); if (parsed_object != nullptr) { chand->health_check_service_name_.reset( @@ -1274,15 +1294,13 @@ bool ChannelData::ProcessResolverResultLocked( UniquePtr processed_lb_policy_name; chand->ProcessLbPolicy(result, parsed_object, &processed_lb_policy_name, lb_policy_config); - if (grpc_client_channel_routing_trace.enabled()) { - gpr_log(GPR_INFO, "chand=%p: resolver returned service config: \"%s\"", - chand, service_config_json); - } // Swap out the data used by GetChannelInfo(). { MutexLock lock(&chand->info_mu_); chand->info_lb_policy_name_ = std::move(processed_lb_policy_name); - chand->info_service_config_json_.reset(service_config_json); + if (service_config_json != nullptr) { + chand->info_service_config_json_ = std::move(service_config_json); + } } // Return results. *lb_policy_name = chand->info_lb_policy_name_.get(); diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h index 004ee04459b..c222985cf18 100644 --- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h +++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h @@ -506,7 +506,9 @@ SubchannelList::SubchannelList( GRPC_ARG_INHIBIT_HEALTH_CHECKING}; // Create a subchannel for each address. for (size_t i = 0; i < addresses.size(); i++) { - GPR_ASSERT(!addresses[i].IsBalancer()); + if (addresses[i].IsBalancer()) { + continue; + } InlinedVector args_to_add; const size_t subchannel_address_arg_index = args_to_add.size(); args_to_add.emplace_back( diff --git a/test/cpp/end2end/grpclb_end2end_test.cc b/test/cpp/end2end/grpclb_end2end_test.cc index 5e0faf0265a..3ac28ec7e43 100644 --- a/test/cpp/end2end/grpclb_end2end_test.cc +++ b/test/cpp/end2end/grpclb_end2end_test.cc @@ -737,6 +737,38 @@ TEST_F(SingleBalancerTest, SelectGrpclbWithMigrationServiceConfig) { EXPECT_EQ("grpclb", channel_->GetLoadBalancingPolicyName()); } +TEST_F(SingleBalancerTest, + DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTest1) { + const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor(); + ResetStub(kFallbackTimeoutMs); + SetNextResolution({AddressData{backends_[0]->port_, false, ""}, + AddressData{balancers_[0]->port_, true, ""}}, + "{\n" + " \"loadBalancingConfig\":[\n" + " {\"pick_first\":{} }\n" + " ]\n" + "}"); + CheckRpcSendOk(); + // Check LB policy name for the channel. + EXPECT_EQ("pick_first", channel_->GetLoadBalancingPolicyName()); +} + +TEST_F(SingleBalancerTest, + DoNotSpecialCaseUseGrpclbWithLoadBalancingConfigTest2) { + const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor(); + ResetStub(kFallbackTimeoutMs); + SetNextResolution({AddressData{balancers_[0]->port_, true, ""}}, + "{\n" + " \"loadBalancingConfig\":[\n" + " {\"pick_first\":{} }\n" + " ]\n" + "}"); + // This should fail since we do not have a non-balancer backend + CheckRpcSendFailure(); + // Check LB policy name for the channel. + EXPECT_EQ("pick_first", channel_->GetLoadBalancingPolicyName()); +} + TEST_F(SingleBalancerTest, SelectGrpclbWithMigrationServiceConfigAndNoAddresses) { const int kFallbackTimeoutMs = 200 * grpc_test_slowdown_factor();