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

xds: enable Outlier Detection by default #5673

Merged
merged 1 commit into from Sep 30, 2022
Merged

xds: enable Outlier Detection by default #5673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions internal/envconfig/xds.go
View file
Open in desktop
Expand Up @@ -84,9 +84,9 @@ var (
// "GRPC_XDS_EXPERIMENTAL_RBAC" to "false".
XDSRBAC = !strings.EqualFold(os.Getenv(rbacSupportEnv), "false")
// XDSOutlierDetection indicates whether outlier detection support is
// enabled, which can be enabled by setting the environment variable
// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "true".
XDSOutlierDetection = strings.EqualFold(os.Getenv(outlierDetectionSupportEnv), "true")
// enabled, which can be disabled by setting the environment variable
// "GRPC_EXPERIMENTAL_ENABLE_OUTLIER_DETECTION" to "false".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We haven't done this in the past, but at some point we discussed this. We should add the release that we are enabling it from, and add a TODO saying at what release we should completely get rid of the env var. Without this, we will have an ever growing list of env vars here and sprinkled everywhere else in the code.

XDSOutlierDetection = !strings.EqualFold(os.Getenv(outlierDetectionSupportEnv), "false")
// XDSFederation indicates whether federation support is enabled.
XDSFederation = strings.EqualFold(os.Getenv(federationEnv), "true")

Expand Down
16 changes: 0 additions & 16 deletions internal/internal.go
View file
Open in desktop
Expand Up @@ -120,22 +120,6 @@ var (
//
// TODO: Remove this function once the RBAC env var is removed.
UnregisterRBACHTTPFilterForTesting func()

// RegisterOutlierDetectionBalancerForTesting registers the Outlier
// Detection Balancer for testing purposes, regardless of the Outlier
// Detection environment variable.
//
// TODO: Remove this function once the Outlier Detection env var is removed.
RegisterOutlierDetectionBalancerForTesting func()

// UnregisterOutlierDetectionBalancerForTesting unregisters the Outlier
// Detection Balancer for testing purposes. This is needed because there is
// no way to unregister the Outlier Detection Balancer after registering it
// solely for testing purposes using
// RegisterOutlierDetectionBalancerForTesting().
//
// TODO: Remove this function once the Outlier Detection env var is removed.
UnregisterOutlierDetectionBalancerForTesting func()
)

// HealthChecker defines the signature of the client-side LB channel health checking function.
Expand Down
18 changes: 0 additions & 18 deletions test/xds/xds_client_outlier_detection_test.go
View file
Open in desktop
Expand Up @@ -32,8 +32,6 @@ import (
v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/internal/testutils/xds/e2e"
testgrpc "google.golang.org/grpc/test/grpc_testing"
Expand All @@ -49,14 +47,6 @@ import (
// Detection balancer. This test verifies that an RPC is able to proceed
// normally with this configuration.
func (s) TestOutlierDetection_NoopConfig(t *testing.T) {
oldOD := envconfig.XDSOutlierDetection
envconfig.XDSOutlierDetection = true
internal.RegisterOutlierDetectionBalancerForTesting()
defer func() {
envconfig.XDSOutlierDetection = oldOD
internal.UnregisterOutlierDetectionBalancerForTesting()
}()

managementServer, nodeID, _, resolver, cleanup1 := e2e.SetupManagementServer(t, nil)
defer cleanup1()

Expand Down Expand Up @@ -129,14 +119,6 @@ func clusterWithOutlierDetection(clusterName, edsServiceName string, secLevel e2
// Detection Balancer should eject the connection to the backend which
// constantly errors, and thus RPC's should mainly go to backend 1 and 2.
func (s) TestOutlierDetectionWithOutlier(t *testing.T) {
oldOD := envconfig.XDSOutlierDetection
envconfig.XDSOutlierDetection = true
internal.RegisterOutlierDetectionBalancerForTesting()
defer func() {
envconfig.XDSOutlierDetection = oldOD
internal.UnregisterOutlierDetectionBalancerForTesting()
}()

managementServer, nodeID, _, resolver, cleanup := e2e.SetupManagementServer(t, nil)
defer cleanup()

Expand Down
13 changes: 7 additions & 6 deletions xds/internal/balancer/balancer.go
View file
Open in desktop
Expand Up @@ -20,10 +20,11 @@
package balancer

import (
_ "google.golang.org/grpc/balancer/weightedtarget" // Register the weighted_target balancer
_ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer" // Register the CDS balancer
_ "google.golang.org/grpc/xds/internal/balancer/clusterimpl" // Register the xds_cluster_impl balancer
_ "google.golang.org/grpc/xds/internal/balancer/clustermanager" // Register the xds_cluster_manager balancer
_ "google.golang.org/grpc/xds/internal/balancer/clusterresolver" // Register the xds_cluster_resolver balancer
_ "google.golang.org/grpc/xds/internal/balancer/priority" // Register the priority balancer
_ "google.golang.org/grpc/balancer/weightedtarget" // Register the weighted_target balancer
_ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer" // Register the CDS balancer
_ "google.golang.org/grpc/xds/internal/balancer/clusterimpl" // Register the xds_cluster_impl balancer
_ "google.golang.org/grpc/xds/internal/balancer/clustermanager" // Register the xds_cluster_manager balancer
_ "google.golang.org/grpc/xds/internal/balancer/clusterresolver" // Register the xds_cluster_resolver balancer
_ "google.golang.org/grpc/xds/internal/balancer/outlierdetection" // Register the outlier_detection balancer
_ "google.golang.org/grpc/xds/internal/balancer/priority" // Register the priority balancer
)
15 changes: 7 additions & 8 deletions xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go
View file
Open in desktop
Expand Up @@ -34,7 +34,6 @@ import (
"google.golang.org/grpc/internal/testutils"
"google.golang.org/grpc/internal/xds/matcher"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/xds/internal/balancer/outlierdetection"
"google.golang.org/grpc/xds/internal/testutils/fakeclient"
"google.golang.org/grpc/xds/internal/xdsclient/bootstrap"
"google.golang.org/grpc/xds/internal/xdsclient/xdsresource"
Expand Down Expand Up @@ -251,7 +250,7 @@ func (s) TestSecurityConfigWithoutXDSCreds(t *testing.T) {
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -307,7 +306,7 @@ func (s) TestNoSecurityConfigWithXDSCreds(t *testing.T) {
// newChildBalancer function as part of test setup. No security config is
// passed to the CDS balancer as part of this update.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -463,7 +462,7 @@ func (s) TestSecurityConfigUpdate_BadToGood(t *testing.T) {
// create a new EDS balancer. The fake EDS balancer created above will be
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -497,7 +496,7 @@ func (s) TestGoodSecurityConfig(t *testing.T) {
// create a new EDS balancer. The fake EDS balancer created above will be
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -550,7 +549,7 @@ func (s) TestSecurityConfigUpdate_GoodToFallback(t *testing.T) {
// create a new EDS balancer. The fake EDS balancer created above will be
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -600,7 +599,7 @@ func (s) TestSecurityConfigUpdate_GoodToBad(t *testing.T) {
// create a new EDS balancer. The fake EDS balancer created above will be
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -678,7 +677,7 @@ func (s) TestSecurityConfigUpdate_GoodToGood(t *testing.T) {
SubjectAltNameMatchers: testSANMatchers,
},
}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down
16 changes: 6 additions & 10 deletions xds/internal/balancer/cdsbalancer/cdsbalancer_test.go
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ import (
"google.golang.org/grpc/balancer"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpctest"
internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
"google.golang.org/grpc/internal/testutils"
Expand Down Expand Up @@ -366,14 +365,11 @@ func (s) TestUpdateClientConnStateWithSameState(t *testing.T) {
// different updates and verifies that the expect ClientConnState is propagated
// to the edsBalancer.
func (s) TestHandleClusterUpdate(t *testing.T) {
oldOutlierDetection := envconfig.XDSOutlierDetection
envconfig.XDSOutlierDetection = true
xdsC, cdsB, edsB, _, cancel := setupWithWatch(t)
xdsC.SetBootstrapConfig(&bootstrap.Config{
XDSServer: defaultTestAuthorityServerConfig,
})
defer func() {
envconfig.XDSOutlierDetection = oldOutlierDetection
cancel()
cdsB.Close()
}()
Expand Down Expand Up @@ -506,7 +502,7 @@ func (s) TestHandleClusterUpdateError(t *testing.T) {
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -591,7 +587,7 @@ func (s) TestResolverError(t *testing.T) {
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -640,7 +636,7 @@ func (s) TestUpdateSubConnState(t *testing.T) {
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -675,7 +671,7 @@ func (s) TestCircuitBreaking(t *testing.T) {
// the service's counter with the new max requests.
var maxRequests uint32 = 1
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: clusterName, MaxRequests: &maxRequests}
wantCCS := edsCCS(clusterName, &maxRequests, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(clusterName, &maxRequests, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -708,7 +704,7 @@ func (s) TestClose(t *testing.T) {
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down Expand Up @@ -779,7 +775,7 @@ func (s) TestExitIdle(t *testing.T) {
// returned to the CDS balancer, because we have overridden the
// newChildBalancer function as part of test setup.
cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName}
wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{})
wantCCS := edsCCS(serviceName, nil, false, nil, noopODLBCfg)
ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer ctxCancel()
if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil {
Expand Down
9 changes: 0 additions & 9 deletions xds/internal/balancer/clusterresolver/clusterresolver_test.go
View file
Open in desktop
Expand Up @@ -30,8 +30,6 @@ import (
"google.golang.org/grpc/balancer/roundrobin"
"google.golang.org/grpc/balancer/weightedtarget"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpctest"
internalserviceconfig "google.golang.org/grpc/internal/serviceconfig"
"google.golang.org/grpc/internal/testutils"
Expand Down Expand Up @@ -533,13 +531,6 @@ func newLBConfigWithOneEDSAndOutlierDetection(edsServiceName string, odCfg outli
// Configuration sent downward should have a top level Outlier Detection Policy
// for each priority.
func (s) TestOutlierDetection(t *testing.T) {
oldOutlierDetection := envconfig.XDSOutlierDetection
envconfig.XDSOutlierDetection = true
internal.RegisterOutlierDetectionBalancerForTesting()
defer func() {
envconfig.XDSOutlierDetection = oldOutlierDetection
}()

edsLBCh := testutils.NewChannel()
xdsC, cleanup := setup(edsLBCh)
defer cleanup()
Expand Down