diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc index b3790cfa7c1..55948fc436f 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc @@ -107,17 +107,6 @@ CircuitBreakerCallCounterMap::CallCounter::~CallCounter() { constexpr char kXdsClusterImpl[] = "xds_cluster_impl_experimental"; -// TODO (donnadionne): Check to see if circuit breaking is enabled, this will be -// removed once circuit breaking feature is fully integrated and enabled by -// default. -bool XdsCircuitBreakingEnabled() { - char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING"); - bool parsed_value; - bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); - gpr_free(value); - return parse_succeeded && parsed_value; -} - // Config for xDS Cluster Impl LB policy. class XdsClusterImplLbConfig : public LoadBalancingPolicy::Config { public: @@ -208,7 +197,6 @@ class XdsClusterImplLb : public LoadBalancingPolicy { private: RefCountedPtr call_counter_; - bool xds_circuit_breaking_enabled_; uint32_t max_concurrent_requests_; RefCountedPtr drop_config_; RefCountedPtr drop_stats_; @@ -277,7 +265,6 @@ class XdsClusterImplLb : public LoadBalancingPolicy { XdsClusterImplLb::Picker::Picker(XdsClusterImplLb* xds_cluster_impl_lb, RefCountedPtr picker) : call_counter_(xds_cluster_impl_lb->call_counter_), - xds_circuit_breaking_enabled_(XdsCircuitBreakingEnabled()), max_concurrent_requests_( xds_cluster_impl_lb->config_->max_concurrent_requests()), drop_config_(xds_cluster_impl_lb->config_->drop_config()), @@ -301,15 +288,13 @@ LoadBalancingPolicy::PickResult XdsClusterImplLb::Picker::Pick( } // Handle circuit breaking. uint32_t current = call_counter_->Increment(); - if (xds_circuit_breaking_enabled_) { - // Check and see if we exceeded the max concurrent requests count. - if (current >= max_concurrent_requests_) { - call_counter_->Decrement(); - if (drop_stats_ != nullptr) drop_stats_->AddUncategorizedDrops(); - PickResult result; - result.type = PickResult::PICK_COMPLETE; - return result; - } + // Check and see if we exceeded the max concurrent requests count. + if (current >= max_concurrent_requests_) { + call_counter_->Decrement(); + if (drop_stats_ != nullptr) drop_stats_->AddUncategorizedDrops(); + PickResult result; + result.type = PickResult::PICK_COMPLETE; + return result; } // If we're not dropping the call, we should always have a child picker. if (picker_ == nullptr) { // Should never happen. diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index 795fb7d3e99..84e165d202c 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -100,17 +100,6 @@ namespace grpc_core { -// TODO(donnadionne): Check to see if timeout is enabled, this will be -// removed once timeout feature is fully integration-tested and enabled by -// default. -bool XdsTimeoutEnabled() { - char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); - bool parsed_value; - bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); - gpr_free(value); - return parse_succeeded && parsed_value; -} - // TODO(donnadionne): Check to see if cluster types aggregate_cluster and // logical_dns are enabled, this will be // removed once the cluster types are fully integration-tested and enabled by @@ -1516,7 +1505,7 @@ grpc_error* RouteActionParse(const EncodingContext& context, // No cluster or weighted_clusters found in RouteAction, ignore this route. *ignore_route = true; } - if (XdsTimeoutEnabled() && !*ignore_route) { + if (!*ignore_route) { const envoy_config_route_v3_RouteAction_MaxStreamDuration* max_stream_duration = envoy_config_route_v3_RouteAction_max_stream_duration(route_action); @@ -1836,20 +1825,18 @@ grpc_error* HttpConnectionManagerParse( bool is_v2, XdsApi::LdsUpdate::HttpConnectionManager* http_connection_manager) { MaybeLogHttpConnectionManager(context, http_connection_manager_proto); - if (XdsTimeoutEnabled()) { - // Obtain max_stream_duration from Http Protocol Options. - const envoy_config_core_v3_HttpProtocolOptions* options = - envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_common_http_protocol_options( - http_connection_manager_proto); - if (options != nullptr) { - const google_protobuf_Duration* duration = - envoy_config_core_v3_HttpProtocolOptions_max_stream_duration(options); - if (duration != nullptr) { - http_connection_manager->http_max_stream_duration.seconds = - google_protobuf_Duration_seconds(duration); - http_connection_manager->http_max_stream_duration.nanos = - google_protobuf_Duration_nanos(duration); - } + // Obtain max_stream_duration from Http Protocol Options. + const envoy_config_core_v3_HttpProtocolOptions* options = + envoy_extensions_filters_network_http_connection_manager_v3_HttpConnectionManager_common_http_protocol_options( + http_connection_manager_proto); + if (options != nullptr) { + const google_protobuf_Duration* duration = + envoy_config_core_v3_HttpProtocolOptions_max_stream_duration(options); + if (duration != nullptr) { + http_connection_manager->http_max_stream_duration.seconds = + google_protobuf_Duration_seconds(duration); + http_connection_manager->http_max_stream_duration.nanos = + google_protobuf_Duration_nanos(duration); } } // Parse filters. diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index 46afc38db31..1aa45d4ff59 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -2864,7 +2864,6 @@ TEST_P(XdsResolverOnlyTest, DefaultRouteSpecifiesSlashPrefix) { } TEST_P(XdsResolverOnlyTest, CircuitBreaking) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING", "true"); constexpr size_t kMaxConcurrentRequests = 10; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -2906,11 +2905,9 @@ TEST_P(XdsResolverOnlyTest, CircuitBreaking) { // Make sure RPCs go to the correct backend: EXPECT_EQ(kMaxConcurrentRequests + 1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING"); } TEST_P(XdsResolverOnlyTest, CircuitBreakingMultipleChannelsShareCallCounter) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING", "true"); constexpr size_t kMaxConcurrentRequests = 10; // Populate new EDS resources. AdsServiceImpl::EdsResourceArgs args({ @@ -2962,45 +2959,6 @@ TEST_P(XdsResolverOnlyTest, CircuitBreakingMultipleChannelsShareCallCounter) { // Make sure RPCs go to the correct backend: EXPECT_EQ(kMaxConcurrentRequests + 1, backends_[0]->backend_service()->request_count()); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_CIRCUIT_BREAKING"); -} - -TEST_P(XdsResolverOnlyTest, CircuitBreakingDisabled) { - constexpr size_t kMaxConcurrentRequests = 10; - SetNextResolution({}); - SetNextResolutionForLbChannelAllBalancers(); - // Populate new EDS resources. - AdsServiceImpl::EdsResourceArgs args({ - {"locality0", GetBackendPorts(0, 1)}, - }); - balancers_[0]->ads_service()->SetEdsResource(BuildEdsResource(args)); - // Update CDS resource to set max concurrent request. - CircuitBreakers circuit_breaks; - Cluster cluster = default_cluster_; - auto* threshold = cluster.mutable_circuit_breakers()->add_thresholds(); - threshold->set_priority(RoutingPriority::DEFAULT); - threshold->mutable_max_requests()->set_value(kMaxConcurrentRequests); - balancers_[0]->ads_service()->SetCdsResource(cluster); - // Send exactly max_concurrent_requests long RPCs. - LongRunningRpc rpcs[kMaxConcurrentRequests]; - for (size_t i = 0; i < kMaxConcurrentRequests; ++i) { - rpcs[i].StartRpc(stub_.get()); - } - // Wait for all RPCs to be in flight. - while (backends_[0]->backend_service()->RpcsWaitingForClientCancel() < - kMaxConcurrentRequests) { - gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), - gpr_time_from_micros(1 * 1000, GPR_TIMESPAN))); - } - // Sending a RPC now should not fail as circuit breaking is disabled. - Status status = SendRpc(); - EXPECT_TRUE(status.ok()); - for (size_t i = 0; i < kMaxConcurrentRequests; ++i) { - rpcs[i].CancelRpc(); - } - // Make sure RPCs go to the correct backend: - EXPECT_EQ(kMaxConcurrentRequests + 1, - backends_[0]->backend_service()->request_count()); } TEST_P(XdsResolverOnlyTest, MultipleChannelsShareXdsClient) { @@ -5093,7 +5051,6 @@ TEST_P(LdsRdsTest, XdsRoutingClusterUpdateClustersWithPickingDelays) { } TEST_P(LdsRdsTest, XdsRoutingApplyXdsTimeout) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutMillis = 500; const int64_t kTimeoutNano = kTimeoutMillis * 1000000; const int64_t kTimeoutGrpcTimeoutHeaderMaxSecond = 1; @@ -5226,89 +5183,9 @@ TEST_P(LdsRdsTest, XdsRoutingApplyXdsTimeout) { t0 = NowFromCycleCounter(); EXPECT_GE(t0, t1); EXPECT_LT(t0, t2); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); -} - -TEST_P(LdsRdsTest, XdsRoutingXdsTimeoutDisabled) { - const int64_t kTimeoutMillis = 500; - const int64_t kTimeoutNano = kTimeoutMillis * 1000000; - const int64_t kTimeoutGrpcTimeoutHeaderMaxSecond = 1; - const int64_t kTimeoutApplicationSecond = 4; - SetNextResolution({}); - SetNextResolutionForLbChannelAllBalancers(); - // Populate new EDS resources. - AdsServiceImpl::EdsResourceArgs args({ - {"locality0", {grpc_pick_unused_port_or_die()}}, - }); - balancers_[0]->ads_service()->SetEdsResource(BuildEdsResource(args)); - RouteConfiguration new_route_config = default_route_config_; - // route 1: Set grpc_timeout_header_max of 1.5 - auto* route1 = new_route_config.mutable_virtual_hosts(0)->mutable_routes(0); - auto* max_stream_duration = - route1->mutable_route()->mutable_max_stream_duration(); - auto* duration = max_stream_duration->mutable_grpc_timeout_header_max(); - duration->set_seconds(kTimeoutGrpcTimeoutHeaderMaxSecond); - duration->set_nanos(kTimeoutNano); - SetRouteConfiguration(0, new_route_config); - // Test grpc_timeout_header_max of 1.5 seconds is not applied - gpr_timespec t0 = gpr_now(GPR_CLOCK_MONOTONIC); - gpr_timespec est_timeout_time = gpr_time_add( - t0, gpr_time_from_millis( - kTimeoutGrpcTimeoutHeaderMaxSecond * 1000 + kTimeoutMillis, - GPR_TIMESPAN)); - CheckRpcSendFailure(1, - RpcOptions() - .set_rpc_service(SERVICE_ECHO1) - .set_rpc_method(METHOD_ECHO1) - .set_wait_for_ready(true) - .set_timeout_ms(kTimeoutApplicationSecond * 1000), - StatusCode::DEADLINE_EXCEEDED); - gpr_timespec timeout_time = gpr_now(GPR_CLOCK_MONOTONIC); - EXPECT_GT(gpr_time_cmp(timeout_time, est_timeout_time), 0); -} - -TEST_P(LdsRdsTest, XdsRoutingHttpTimeoutDisabled) { - const int64_t kTimeoutMillis = 500; - const int64_t kTimeoutNano = kTimeoutMillis * 1000000; - const int64_t kTimeoutHttpMaxStreamDurationSecond = 3; - const int64_t kTimeoutApplicationSecond = 4; - SetNextResolution({}); - SetNextResolutionForLbChannelAllBalancers(); - // Populate new EDS resources. - AdsServiceImpl::EdsResourceArgs args({ - {"locality0", {grpc_pick_unused_port_or_die()}}, - }); - // Construct listener. - auto listener = default_listener_; - HttpConnectionManager http_connection_manager; - listener.mutable_api_listener()->mutable_api_listener()->UnpackTo( - &http_connection_manager); - // Set up HTTP max_stream_duration of 3.5 seconds - auto* duration = - http_connection_manager.mutable_common_http_protocol_options() - ->mutable_max_stream_duration(); - duration->set_seconds(kTimeoutHttpMaxStreamDurationSecond); - duration->set_nanos(kTimeoutNano); - listener.mutable_api_listener()->mutable_api_listener()->PackFrom( - http_connection_manager); - SetListenerAndRouteConfiguration(0, std::move(listener), - default_route_config_); - // Test http_stream_duration of 3.5 seconds is not applied - auto t0 = gpr_now(GPR_CLOCK_MONOTONIC); - auto est_timeout_time = gpr_time_add( - t0, gpr_time_from_millis( - kTimeoutHttpMaxStreamDurationSecond * 1000 + kTimeoutMillis, - GPR_TIMESPAN)); - CheckRpcSendFailure(1, - RpcOptions().set_wait_for_ready(true).set_timeout_ms( - kTimeoutApplicationSecond * 1000), - StatusCode::DEADLINE_EXCEEDED); - auto timeout_time = gpr_now(GPR_CLOCK_MONOTONIC); - EXPECT_GT(gpr_time_cmp(timeout_time, est_timeout_time), 0); } TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenXdsTimeoutExplicit0) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutNano = 500000000; const int64_t kTimeoutMaxStreamDurationSecond = 2; const int64_t kTimeoutHttpMaxStreamDurationSecond = 3; @@ -5410,11 +5287,9 @@ TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenXdsTimeoutExplicit0) { system_clock::now() - t0); EXPECT_GT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); } TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenHttpTimeoutExplicit0) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutApplicationSecond = 4; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -5449,13 +5324,11 @@ TEST_P(LdsRdsTest, XdsRoutingApplyApplicationTimeoutWhenHttpTimeoutExplicit0) { t0); EXPECT_GT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); } // Test to ensure application-specified deadline won't be affected when // the xDS config does not specify a timeout. TEST_P(LdsRdsTest, XdsRoutingWithOnlyApplicationTimeout) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT", "true"); const int64_t kTimeoutApplicationSecond = 4; SetNextResolution({}); SetNextResolutionForLbChannelAllBalancers(); @@ -5474,7 +5347,6 @@ TEST_P(LdsRdsTest, XdsRoutingWithOnlyApplicationTimeout) { t0); EXPECT_GT(ellapsed_nano_seconds.count(), kTimeoutApplicationSecond * 1000000000); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"); } TEST_P(LdsRdsTest, XdsRoutingHeadersMatching) {