Skip to content

Commit

Permalink
[Backport-1.55] [WRR] Remove env var guard for WRR policy (#32946)
Browse files Browse the repository at this point in the history
Backport of #32936

Co-authored-by: Mark D. Roth <roth@google.com>
  • Loading branch information
veblush and markdroth committed Apr 26, 2023
1 parent 8c2a45c commit 76a2e13
Show file tree
Hide file tree
Showing 9 changed files with 17 additions and 58 deletions.
1 change: 0 additions & 1 deletion build_autogenerated.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions doc/grpc_xds_features.md
Original file line number Diff line number Diff line change
Expand Up @@ -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):<br>Only the following detection types are supported:<ul><li>Success Rate</li><li>Failure Percentage</li></ul> | [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 | | | |
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,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 {
Expand Down
26 changes: 4 additions & 22 deletions src/core/ext/xds/xds_lb_policy_registry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(); }
Expand Down Expand Up @@ -258,31 +255,16 @@ 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(),
std::make_unique<RingHashLbPolicyConfigFactory>());
policy_config_factories_.emplace(
RoundRobinLbPolicyConfigFactory::Type(),
std::make_unique<RoundRobinLbPolicyConfigFactory>());
if (XdsWrrLbEnabled()) {
policy_config_factories_.emplace(
ClientSideWeightedRoundRobinLbPolicyConfigFactory::Type(),
std::make_unique<ClientSideWeightedRoundRobinLbPolicyConfigFactory>());
}
policy_config_factories_.emplace(
ClientSideWeightedRoundRobinLbPolicyConfigFactory::Type(),
std::make_unique<ClientSideWeightedRoundRobinLbPolicyConfigFactory>());
policy_config_factories_.emplace(
WrrLocalityLbPolicyConfigFactory::Type(),
std::make_unique<WrrLocalityLbPolicyConfigFactory>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ class WeightedRoundRobinTest : public LoadBalancingPolicyTest {
}

RefCountedPtr<LoadBalancingPolicy::Config> 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);
}
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion test/core/xds/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
Expand Down
22 changes: 2 additions & 20 deletions test/core/xds/xds_lb_policy_registry_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -131,19 +130,17 @@ TEST(RoundRobin, Basic) {
//

TEST(ClientSideWeightedRoundRobinTest, DefaultConfig) {
ScopedExperimentalEnvVar env_var("GRPC_EXPERIMENTAL_XDS_WRR_LB");
LoadBalancingPolicyProto policy;
policy.add_policies()
->mutable_typed_extension_config()
->mutable_typed_config()
->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);
Expand All @@ -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,"
Expand All @@ -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);
Expand Down Expand Up @@ -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
//
Expand Down
10 changes: 4 additions & 6 deletions test/cpp/end2end/client_lb_end2end_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 76a2e13

Please sign in to comment.