From 020e9b4dd6ca2a9bd9942b266507fb6858712b70 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 26 Apr 2023 09:40:22 -0700 Subject: [PATCH] [WRR] Remove env var guard for WRR policy (#32936) - remove the `_experimental` suffix from the gRPC policy name - remove the env var guard for the xDS policy config --- build_autogenerated.yaml | 1 - doc/grpc_xds_features.md | 1 + .../weighted_round_robin.cc | 3 +-- src/core/ext/xds/xds_lb_policy_registry.cc | 26 +++---------------- .../weighted_round_robin_config_test.cc | 6 ++--- .../lb_policy/weighted_round_robin_test.cc | 5 ++-- test/core/xds/BUILD | 1 - test/core/xds/xds_lb_policy_registry_test.cc | 22 ++-------------- test/cpp/end2end/client_lb_end2end_test.cc | 10 +++---- 9 files changed, 17 insertions(+), 58 deletions(-) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 7c0a370b73e..fc988cee0e8 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -13484,7 +13484,6 @@ targets: build: test language: c++ headers: - - test/core/util/scoped_env_var.h - test/cpp/util/cli_call.h - test/cpp/util/cli_credentials.h - test/cpp/util/config_grpc_cli.h diff --git a/doc/grpc_xds_features.md b/doc/grpc_xds_features.md index 82809070d20..389bd16b132 100644 --- a/doc/grpc_xds_features.md +++ b/doc/grpc_xds_features.md @@ -70,3 +70,4 @@ Support for [xDS v2 APIs](https://www.envoyproxy.io/docs/envoy/latest/api/api_su [Outlier Detection](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/outlier):
Only the following detection types are supported: | [A50](https://github.com/grpc/proposal/blob/master/A50-xds-outlier-detection.md) | v1.51.0 | v1.49.0 | v1.50.0 | v1.7.0 | | [Custom Load Balancer Configuration](https://github.com/envoyproxy/envoy/blob/57be3189ffa3372b34e9480d1f02b2d165e49077/api/envoy/config/cluster/v3/cluster.proto#L1208) | [A52](https://github.com/grpc/proposal/blob/master/A52-xds-custom-lb-policies.md) | v1.55.0 | v1.47.0 | | | [xDS Federation](https://github.com/cncf/xds/blob/main/proposals/TP1-xds-transport-next.md) | [A47](https://github.com/grpc/proposal/blob/master/A47-xds-federation.md) | v1.55.0 | | | | +[Client-Side Weighted Round Robin LB Policy](https://github.com/envoyproxy/envoy/blob/a6d46b6ac4750720eec9a49abe701f0df9bf8e0a/api/envoy/extensions/load_balancing_policies/client_side_weighted_round_robin/v3/client_side_weighted_round_robin.proto#L36) | [A58](https://github.com/grpc/proposal/blob/master/A58-client-side-weighted-round-robin-lb-policy.md) | v1.55.0 | | | | diff --git a/src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc index b89a4f35660..95260a6281d 100644 --- a/src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc @@ -77,8 +77,7 @@ TraceFlag grpc_lb_wrr_trace(false, "weighted_round_robin_lb"); namespace { -constexpr absl::string_view kWeightedRoundRobin = - "weighted_round_robin_experimental"; +constexpr absl::string_view kWeightedRoundRobin = "weighted_round_robin"; // Config for WRR policy. class WeightedRoundRobinConfig : public LoadBalancingPolicy::Config { diff --git a/src/core/ext/xds/xds_lb_policy_registry.cc b/src/core/ext/xds/xds_lb_policy_registry.cc index 0422aa3e38a..036ce70b722 100644 --- a/src/core/ext/xds/xds_lb_policy_registry.cc +++ b/src/core/ext/xds/xds_lb_policy_registry.cc @@ -35,8 +35,6 @@ #include "src/core/ext/xds/xds_common_types.h" #include "src/core/lib/config/core_configuration.h" -#include "src/core/lib/gpr/string.h" -#include "src/core/lib/gprpp/env.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/load_balancing/lb_policy_registry.h" @@ -137,8 +135,7 @@ class ClientSideWeightedRoundRobinLbPolicyConfigFactory } config["errorUtilizationPenalty"] = value; } - return Json::Object{ - {"weighted_round_robin_experimental", std::move(config)}}; + return Json::Object{{"weighted_round_robin", std::move(config)}}; } absl::string_view type() override { return Type(); } @@ -258,19 +255,6 @@ class WrrLocalityLbPolicyConfigFactory // XdsLbPolicyRegistry // -namespace { - -// TODO(roth): Remove this when interop tests pass. -bool XdsWrrLbEnabled() { - auto value = GetEnv("GRPC_EXPERIMENTAL_XDS_WRR_LB"); - if (!value.has_value()) return false; - bool parsed_value; - bool parse_succeeded = gpr_parse_bool_value(value->c_str(), &parsed_value); - return parse_succeeded && parsed_value; -} - -} // namespace - XdsLbPolicyRegistry::XdsLbPolicyRegistry() { policy_config_factories_.emplace( RingHashLbPolicyConfigFactory::Type(), @@ -278,11 +262,9 @@ XdsLbPolicyRegistry::XdsLbPolicyRegistry() { policy_config_factories_.emplace( RoundRobinLbPolicyConfigFactory::Type(), std::make_unique()); - if (XdsWrrLbEnabled()) { - policy_config_factories_.emplace( - ClientSideWeightedRoundRobinLbPolicyConfigFactory::Type(), - std::make_unique()); - } + policy_config_factories_.emplace( + ClientSideWeightedRoundRobinLbPolicyConfigFactory::Type(), + std::make_unique()); policy_config_factories_.emplace( WrrLocalityLbPolicyConfigFactory::Type(), std::make_unique()); diff --git a/test/core/client_channel/lb_policy/weighted_round_robin_config_test.cc b/test/core/client_channel/lb_policy/weighted_round_robin_config_test.cc index bb286282861..d3312d9661c 100644 --- a/test/core/client_channel/lb_policy/weighted_round_robin_config_test.cc +++ b/test/core/client_channel/lb_policy/weighted_round_robin_config_test.cc @@ -34,7 +34,7 @@ TEST(WeightedRoundRobinConfigTest, EmptyConfig) { const char* service_config_json = "{\n" " \"loadBalancingConfig\":[{\n" - " \"weighted_round_robin_experimental\":{\n" + " \"weighted_round_robin\":{\n" " }\n" " }]\n" "}\n"; @@ -48,7 +48,7 @@ TEST(WeightedRoundRobinConfigTest, InvalidTypes) { const char* service_config_json = "{\n" " \"loadBalancingConfig\":[{\n" - " \"weighted_round_robin_experimental\":{\n" + " \"weighted_round_robin\":{\n" " \"enableOobLoadReport\": 5,\n" " \"oobReportingPeriod\": true,\n" " \"blackoutPeriod\": [],\n" @@ -78,7 +78,7 @@ TEST(WeightedRoundRobinConfigTest, InvalidValues) { const char* service_config_json = "{\n" " \"loadBalancingConfig\":[{\n" - " \"weighted_round_robin_experimental\":{\n" + " \"weighted_round_robin\":{\n" " \"errorUtilizationPenalty\": -1.0\n" " }\n" " }]\n" diff --git a/test/core/client_channel/lb_policy/weighted_round_robin_test.cc b/test/core/client_channel/lb_policy/weighted_round_robin_test.cc index 9bb9894886d..0aa03b43c51 100644 --- a/test/core/client_channel/lb_policy/weighted_round_robin_test.cc +++ b/test/core/client_channel/lb_policy/weighted_round_robin_test.cc @@ -106,8 +106,7 @@ class WeightedRoundRobinTest : public LoadBalancingPolicyTest { } RefCountedPtr Build() { - Json config = Json::Array{ - Json::Object{{"weighted_round_robin_experimental", json_}}}; + Json config = Json::Array{Json::Object{{"weighted_round_robin", json_}}}; gpr_log(GPR_INFO, "CONFIG: %s", JsonDump(config).c_str()); return MakeConfig(config); } @@ -155,7 +154,7 @@ class WeightedRoundRobinTest : public LoadBalancingPolicyTest { return true; }; ON_CALL(*mock_ee_, Cancel(::testing::_)).WillByDefault(cancel); - lb_policy_ = MakeLbPolicy("weighted_round_robin_experimental"); + lb_policy_ = MakeLbPolicy("weighted_round_robin"); } ~WeightedRoundRobinTest() override { diff --git a/test/core/xds/BUILD b/test/core/xds/BUILD index f96890737c3..b8fb0d9561a 100644 --- a/test/core/xds/BUILD +++ b/test/core/xds/BUILD @@ -122,7 +122,6 @@ grpc_cc_test( "//src/proto/grpc/testing/xds/v3:udpa_typed_struct_proto", "//src/proto/grpc/testing/xds/v3:wrr_locality_proto", "//test/core/util:grpc_test_util", - "//test/core/util:scoped_env_var", "//test/cpp/util:grpc_cli_utils", ], ) diff --git a/test/core/xds/xds_lb_policy_registry_test.cc b/test/core/xds/xds_lb_policy_registry_test.cc index 48554f8937b..43d63dd40c6 100644 --- a/test/core/xds/xds_lb_policy_registry_test.cc +++ b/test/core/xds/xds_lb_policy_registry_test.cc @@ -52,7 +52,6 @@ #include "src/proto/grpc/testing/xds/v3/round_robin.pb.h" #include "src/proto/grpc/testing/xds/v3/typed_struct.pb.h" #include "src/proto/grpc/testing/xds/v3/wrr_locality.pb.h" -#include "test/core/util/scoped_env_var.h" #include "test/core/util/test_config.h" namespace grpc_core { @@ -131,7 +130,6 @@ TEST(RoundRobin, Basic) { // TEST(ClientSideWeightedRoundRobinTest, DefaultConfig) { - ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_WRR_LB"); LoadBalancingPolicyProto policy; policy.add_policies() ->mutable_typed_extension_config() @@ -139,11 +137,10 @@ TEST(ClientSideWeightedRoundRobinTest, DefaultConfig) { ->PackFrom(ClientSideWeightedRoundRobin()); auto result = ConvertXdsPolicy(policy); ASSERT_TRUE(result.ok()) << result.status(); - EXPECT_EQ(*result, "{\"weighted_round_robin_experimental\":{}}"); + EXPECT_EQ(*result, "{\"weighted_round_robin\":{}}"); } TEST(ClientSideWeightedRoundRobinTest, FieldsExplicitlySet) { - ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_WRR_LB"); ClientSideWeightedRoundRobin wrr; wrr.mutable_enable_oob_load_report()->set_value(true); wrr.mutable_oob_reporting_period()->set_seconds(1); @@ -159,7 +156,7 @@ TEST(ClientSideWeightedRoundRobinTest, FieldsExplicitlySet) { auto result = ConvertXdsPolicy(policy); ASSERT_TRUE(result.ok()) << result.status(); EXPECT_EQ(*result, - "{\"weighted_round_robin_experimental\":{" + "{\"weighted_round_robin\":{" "\"blackoutPeriod\":\"2.000000000s\"," "\"enableOobLoadReport\":true," "\"errorUtilizationPenalty\":5.000000," @@ -170,7 +167,6 @@ TEST(ClientSideWeightedRoundRobinTest, FieldsExplicitlySet) { } TEST(ClientSideWeightedRoundRobinTest, InvalidValues) { - ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_WRR_LB"); ClientSideWeightedRoundRobin wrr; wrr.mutable_oob_reporting_period()->set_seconds(-1); wrr.mutable_blackout_period()->set_seconds(-2); @@ -213,20 +209,6 @@ TEST(ClientSideWeightedRoundRobinTest, InvalidValues) { << result.status(); } -TEST(ClientSideWeightedRoundRobinTest, EnvVarNotEnabled) { - LoadBalancingPolicyProto policy; - policy.add_policies() - ->mutable_typed_extension_config() - ->mutable_typed_config() - ->PackFrom(ClientSideWeightedRoundRobin()); - auto result = ConvertXdsPolicy(policy); - EXPECT_EQ(result.status().code(), absl::StatusCode::kInvalidArgument); - EXPECT_EQ(result.status().message(), - "validation errors: [field:load_balancing_policy " - "error:no supported load balancing policy config found]") - << result.status(); -} - // // RingHash // diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index d20942bfe78..a783c453551 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -3060,7 +3060,7 @@ TEST_F(ControlPlaneStatusRewritingTest, RewritesFromConfigSelector) { const char kServiceConfigPerCall[] = "{\n" " \"loadBalancingConfig\": [\n" - " {\"weighted_round_robin_experimental\": {\n" + " {\"weighted_round_robin\": {\n" " \"blackoutPeriod\": \"0s\",\n" " \"weightUpdatePeriod\": \"0.1s\"\n" " }}\n" @@ -3070,7 +3070,7 @@ const char kServiceConfigPerCall[] = const char kServiceConfigOob[] = "{\n" " \"loadBalancingConfig\": [\n" - " {\"weighted_round_robin_experimental\": {\n" + " {\"weighted_round_robin\": {\n" " \"blackoutPeriod\": \"0s\",\n" " \"weightUpdatePeriod\": \"0.1s\",\n" " \"enableOobLoadReport\": true\n" @@ -3155,8 +3155,7 @@ TEST_F(WeightedRoundRobinTest, CallAndServerMetric) { ExpectWeightedRoundRobinPicks(DEBUG_LOCATION, stub, /*expected_weights=*/{1, 2, 4}); // Check LB policy name for the channel. - EXPECT_EQ("weighted_round_robin_experimental", - channel->GetLoadBalancingPolicyName()); + EXPECT_EQ("weighted_round_robin", channel->GetLoadBalancingPolicyName()); } class WeightedRoundRobinParamTest @@ -3191,8 +3190,7 @@ TEST_P(WeightedRoundRobinParamTest, Basic) { ExpectWeightedRoundRobinPicks(DEBUG_LOCATION, stub, /*expected_weights=*/{1, 2, 4}); // Check LB policy name for the channel. - EXPECT_EQ("weighted_round_robin_experimental", - channel->GetLoadBalancingPolicyName()); + EXPECT_EQ("weighted_round_robin", channel->GetLoadBalancingPolicyName()); } } // namespace