Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

outlier detection: remove env var protection #31251

Merged
merged 2 commits into from Oct 6, 2022
Merged

outlier detection: remove env var protection #31251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
25085d3
outlier detection: remove env var protection
markdroth Oct 5, 2022
a38e860
fix sanity
markdroth Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 0 additions & 2 deletions BUILD
View file
Open in desktop
Expand Up @@ -4998,7 +4998,6 @@ grpc_cc_library(
"grpc_lb_policy_ring_hash",
"grpc_lb_xds_channel_args",
"grpc_lb_xds_common",
"grpc_outlier_detection_header",
"grpc_public_hdrs",
"grpc_resolver",
"grpc_resolver_fake",
Expand Down Expand Up @@ -5294,7 +5293,6 @@ grpc_cc_library(
"closure",
"config",
"debug_location",
"env",
"gpr",
"grpc_base",
"grpc_client_channel",
Expand Down
4 changes: 1 addition & 3 deletions build_autogenerated.yaml
View file
Open in desktop

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

17 changes: 2 additions & 15 deletions src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc
View file
Open in desktop
Expand Up @@ -46,9 +46,7 @@
#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/config/core_configuration.h"
#include "src/core/lib/debug/trace.h"
#include "src/core/lib/gpr/string.h"
#include "src/core/lib/gprpp/debug_location.h"
#include "src/core/lib/gprpp/env.h"
#include "src/core/lib/gprpp/orphanable.h"
#include "src/core/lib/gprpp/ref_counted.h"
#include "src/core/lib/gprpp/ref_counted_ptr.h"
Expand All @@ -71,15 +69,6 @@ namespace grpc_core {

TraceFlag grpc_outlier_detection_lb_trace(false, "outlier_detection_lb");

// TODO(donnadionne): Remove once outlier detection is no longer experimental
bool XdsOutlierDetectionEnabled() {
auto value = GetEnv("GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION");
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 {

constexpr absl::string_view kOutlierDetection =
Expand Down Expand Up @@ -1150,10 +1139,8 @@ void OutlierDetectionConfig::JsonPostLoad(const Json& json, const JsonArgs&,
//

void RegisterOutlierDetectionLbPolicy(CoreConfiguration::Builder* builder) {
if (XdsOutlierDetectionEnabled()) {
builder->lb_policy_registry()->RegisterLoadBalancingPolicyFactory(
std::make_unique<OutlierDetectionLbFactory>());
}
builder->lb_policy_registry()->RegisterLoadBalancingPolicyFactory(
std::make_unique<OutlierDetectionLbFactory>());
}

} // namespace grpc_core
2 changes: 0 additions & 2 deletions src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h
View file
Open in desktop
Expand Up @@ -31,8 +31,6 @@

namespace grpc_core {

bool XdsOutlierDetectionEnabled();

struct OutlierDetectionConfig {
Duration interval = Duration::Seconds(10);
Duration base_ejection_time = Duration::Milliseconds(30000);
Expand Down
44 changes: 13 additions & 31 deletions src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc
View file
Open in desktop
Expand Up @@ -40,7 +40,6 @@

#include "src/core/ext/filters/client_channel/lb_policy/address_filtering.h"
#include "src/core/ext/filters/client_channel/lb_policy/child_policy_handler.h"
#include "src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h"
#include "src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h"
#include "src/core/ext/filters/client_channel/lb_policy/xds/xds.h"
#include "src/core/ext/filters/client_channel/lb_policy/xds/xds_channel_args.h"
Expand Down Expand Up @@ -951,27 +950,18 @@ XdsClusterResolverLb::CreateChildPolicyConfigLocked() {
xds_cluster_impl_config["lrsLoadReportingServer"] =
discovery_config.lrs_load_reporting_server->ToJson();
}
Json locality_picking_policy;
if (XdsOutlierDetectionEnabled()) {
Json::Object outlier_detection_config;
if (discovery_entry.config().outlier_detection_lb_config.has_value()) {
outlier_detection_config =
discovery_entry.config().outlier_detection_lb_config.value();
}
outlier_detection_config["childPolicy"] = Json::Array{Json::Object{
{"xds_cluster_impl_experimental",
std::move(xds_cluster_impl_config)},
}};
locality_picking_policy = Json::Array{Json::Object{
{"outlier_detection_experimental",
std::move(outlier_detection_config)},
}};
} else {
locality_picking_policy = Json::Array{Json::Object{
{"xds_cluster_impl_experimental",
std::move(xds_cluster_impl_config)},
}};
Json::Object outlier_detection_config;
if (discovery_entry.config().outlier_detection_lb_config.has_value()) {
outlier_detection_config =
discovery_entry.config().outlier_detection_lb_config.value();
}
outlier_detection_config["childPolicy"] = Json::Array{Json::Object{
{"xds_cluster_impl_experimental", std::move(xds_cluster_impl_config)},
}};
Json locality_picking_policy = Json::Array{Json::Object{
{"outlier_detection_experimental",
std::move(outlier_detection_config)},
}};
// Add priority entry, with the appropriate child name.
std::string child_name = discovery_entry.GetChildPolicyName(priority);
priority_priorities.emplace_back(child_name);
Expand Down Expand Up @@ -1089,8 +1079,7 @@ XdsClusterResolverLbConfig::DiscoveryMechanism::JsonLoader(const JsonArgs&) {
.OptionalField("max_concurrent_requests",
&DiscoveryMechanism::max_concurrent_requests)
.OptionalField("outlierDetection",
&DiscoveryMechanism::outlier_detection_lb_config,
"outlier_detection")
&DiscoveryMechanism::outlier_detection_lb_config)
.Finish();
return loader;
}
Expand Down Expand Up @@ -1216,15 +1205,8 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory {
"requires configuration. "
"Please use loadBalancingConfig field of service config instead.");
}
class XdsJsonArgs : public JsonArgs {
public:
bool IsEnabled(absl::string_view key) const override {
if (key == "outlier_detection") return XdsOutlierDetectionEnabled();
return true;
}
};
return LoadRefCountedFromJson<XdsClusterResolverLbConfig>(
json, XdsJsonArgs(),
json, JsonArgs(),
"errors validating xds_cluster_resolver LB policy config");
}

Expand Down
9 changes: 2 additions & 7 deletions src/core/ext/xds/xds_cluster.cc
View file
Open in desktop
Expand Up @@ -422,13 +422,8 @@ absl::StatusOr<XdsClusterResource> CdsResourceParse(
}
}
}
// As long as outlier detection field is present in the cluster update,
// we will end up with a outlier detection in the cluster resource which will
// lead to the creation of outlier detection in discovery mechanism. Values
// for outlier detection will be based on fields received and
// default values.
if (XdsOutlierDetectionEnabled() &&
envoy_config_cluster_v3_Cluster_has_outlier_detection(cluster)) {
// Outlier detection config.
if (envoy_config_cluster_v3_Cluster_has_outlier_detection(cluster)) {
ValidationErrors::ScopedField field(&errors, ".outlier_detection");
OutlierDetectionConfig outlier_detection_update;
const envoy_config_cluster_v3_OutlierDetection* outlier_detection =
Expand Down
6 changes: 0 additions & 6 deletions test/core/client_channel/outlier_detection_lb_config_parser_test.cc
View file
Open in desktop
Expand Up @@ -23,7 +23,6 @@

#include "src/core/lib/channel/channel_args.h"
#include "src/core/lib/service_config/service_config_impl.h"
#include "test/core/util/scoped_env_var.h"
#include "test/core/util/test_config.h"

namespace grpc_core {
Expand All @@ -32,14 +31,9 @@ namespace {

class OutlierDetectionConfigParsingTest : public ::testing::Test {
public:
OutlierDetectionConfigParsingTest()
: env_var_("GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION") {}

static void SetUpTestSuite() { grpc_init(); }

static void TearDownTestSuite() { grpc_shutdown_blocking(); }

ScopedExperimentalEnvVar env_var_;
};

TEST_F(OutlierDetectionConfigParsingTest, ValidConfig) {
Expand Down
1 change: 0 additions & 1 deletion test/core/xds/BUILD
View file
Open in desktop
Expand Up @@ -186,7 +186,6 @@ grpc_cc_test(
"//src/proto/grpc/testing/xds/v3:cluster_proto",
"//src/proto/grpc/testing/xds/v3:tls_proto",
"//test/core/util:grpc_test_util",
"//test/core/util:scoped_env_var",
],
)

Expand Down
24 changes: 1 addition & 23 deletions test/core/xds/xds_cluster_resource_type_test.cc
View file
Open in desktop
Expand Up @@ -52,7 +52,6 @@
#include "src/proto/grpc/testing/xds/v3/endpoint.pb.h"
#include "src/proto/grpc/testing/xds/v3/outlier_detection.pb.h"
#include "src/proto/grpc/testing/xds/v3/tls.pb.h"
#include "test/core/util/scoped_env_var.h"
#include "test/core/util/test_config.h"

using envoy::config::cluster::v3::Cluster;
Expand Down Expand Up @@ -1014,8 +1013,7 @@ TEST_F(CircuitBreakingTest, DefaultThresholdWithMaxRequestsUnset) {

using OutlierDetectionTest = XdsClusterTest;

TEST_F(OutlierDetectionTest, EnabledWithDefaults) {
ScopedExperimentalEnvVar env("GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION");
TEST_F(OutlierDetectionTest, DefaultValues) {
Cluster cluster;
cluster.set_name("foo");
cluster.set_type(cluster.EDS);
Expand All @@ -1034,26 +1032,7 @@ TEST_F(OutlierDetectionTest, EnabledWithDefaults) {
EXPECT_EQ(*resource.outlier_detection, OutlierDetectionConfig());
}

TEST_F(OutlierDetectionTest, NotEnabledWithEnvVarUnset) {
Cluster cluster;
cluster.set_name("foo");
cluster.set_type(cluster.EDS);
cluster.mutable_eds_cluster_config()->mutable_eds_config()->mutable_self();
cluster.mutable_outlier_detection();
std::string serialized_resource;
ASSERT_TRUE(cluster.SerializeToString(&serialized_resource));
auto* resource_type = XdsClusterResourceType::Get();
auto decode_result = resource_type->Decode(
decode_context_, serialized_resource, /*is_v2=*/false);
ASSERT_TRUE(decode_result.resource.ok()) << decode_result.resource.status();
ASSERT_TRUE(decode_result.name.has_value());
EXPECT_EQ(*decode_result.name, "foo");
auto& resource = static_cast<XdsClusterResource&>(**decode_result.resource);
ASSERT_FALSE(resource.outlier_detection.has_value());
}

TEST_F(OutlierDetectionTest, AllFieldsSet) {
ScopedExperimentalEnvVar env("GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION");
Cluster cluster;
cluster.set_name("foo");
cluster.set_type(cluster.EDS);
Expand Down Expand Up @@ -1105,7 +1084,6 @@ TEST_F(OutlierDetectionTest, AllFieldsSet) {
}

TEST_F(OutlierDetectionTest, InvalidValues) {
ScopedExperimentalEnvVar env("GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION");
Cluster cluster;
cluster.set_name("foo");
cluster.set_type(cluster.EDS);
Expand Down
1 change: 0 additions & 1 deletion test/cpp/end2end/xds/BUILD
View file
Open in desktop
Expand Up @@ -295,7 +295,6 @@ grpc_cc_test(
"//src/proto/grpc/testing/xds/v3:fault_proto",
"//src/proto/grpc/testing/xds/v3:router_proto",
"//test/core/util:grpc_test_util",
"//test/core/util:scoped_env_var",
],
)

Expand Down