From c25a52b76917819833e06c1181536c449a506631 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Tue, 9 Nov 2021 13:06:38 -0800 Subject: [PATCH] config: remove retry disable via environment variable (#4922) --- dialoptions.go | 2 -- internal/envconfig/envconfig.go | 6 ---- internal/xds/env/env.go | 4 --- test/retry_test.go | 11 -------- .../test/xds_client_integration_test.go | 6 ---- .../xdsclient/xdsresource/unmarshal_rds.go | 2 +- .../xdsresource/unmarshal_rds_test.go | 28 ++++++------------- 7 files changed, 10 insertions(+), 49 deletions(-) diff --git a/dialoptions.go b/dialoptions.go index 280df92e74b..22d626f183f 100644 --- a/dialoptions.go +++ b/dialoptions.go @@ -29,7 +29,6 @@ import ( "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal" internalbackoff "google.golang.org/grpc/internal/backoff" - "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/transport" "google.golang.org/grpc/keepalive" "google.golang.org/grpc/resolver" @@ -576,7 +575,6 @@ func withHealthCheckFunc(f internal.HealthChecker) DialOption { func defaultDialOptions() dialOptions { return dialOptions{ - disableRetry: !envconfig.Retry, healthCheckFunc: internal.HealthCheckFunc, copts: transport.ConnectOptions{ WriteBufferSize: defaultWriteBufSize, diff --git a/internal/envconfig/envconfig.go b/internal/envconfig/envconfig.go index 9f25a67fc6b..6f027254311 100644 --- a/internal/envconfig/envconfig.go +++ b/internal/envconfig/envconfig.go @@ -22,20 +22,14 @@ package envconfig import ( "os" "strings" - - xdsenv "google.golang.org/grpc/internal/xds/env" ) const ( prefix = "GRPC_GO_" - retryStr = prefix + "RETRY" txtErrIgnoreStr = prefix + "IGNORE_TXT_ERRORS" ) var ( - // Retry is enabled unless explicitly disabled via "GRPC_GO_RETRY=off" or - // if XDS retry support is explicitly disabled. - Retry = !strings.EqualFold(os.Getenv(retryStr), "off") && xdsenv.RetrySupport // TXTErrIgnore is set if TXT errors should be ignored ("GRPC_GO_IGNORE_TXT_ERRORS" is not "false"). TXTErrIgnore = !strings.EqualFold(os.Getenv(txtErrIgnoreStr), "false") ) diff --git a/internal/xds/env/env.go b/internal/xds/env/env.go index 2c85e7804ba..75bdc6efb19 100644 --- a/internal/xds/env/env.go +++ b/internal/xds/env/env.go @@ -42,7 +42,6 @@ const ( ringHashSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH" clientSideSecuritySupportEnv = "GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT" aggregateAndDNSSupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_AGGREGATE_AND_LOGICAL_DNS_CLUSTER" - retrySupportEnv = "GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY" rbacSupportEnv = "GRPC_XDS_EXPERIMENTAL_RBAC" federationEnv = "GRPC_EXPERIMENTAL_XDS_FEDERATION" @@ -81,9 +80,6 @@ var ( // "true". AggregateAndDNSSupportEnv = strings.EqualFold(os.Getenv(aggregateAndDNSSupportEnv), "true") - // RetrySupport indicates whether xDS retry is enabled. - RetrySupport = !strings.EqualFold(os.Getenv(retrySupportEnv), "false") - // RBACSupport indicates whether xDS configured RBAC HTTP Filter is enabled, // which can be disabled by setting the environment variable // "GRPC_XDS_EXPERIMENTAL_RBAC" to "false". diff --git a/test/retry_test.go b/test/retry_test.go index 7f068d79f44..1bd866add60 100644 --- a/test/retry_test.go +++ b/test/retry_test.go @@ -33,7 +33,6 @@ import ( "github.com/golang/protobuf/proto" "google.golang.org/grpc" "google.golang.org/grpc/codes" - "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/metadata" "google.golang.org/grpc/stats" @@ -41,14 +40,7 @@ import ( testpb "google.golang.org/grpc/test/grpc_testing" ) -func enableRetry() func() { - old := envconfig.Retry - envconfig.Retry = true - return func() { envconfig.Retry = old } -} - func (s) TestRetryUnary(t *testing.T) { - defer enableRetry()() i := -1 ss := &stubserver.StubServer{ EmptyCallF: func(context.Context, *testpb.Empty) (*testpb.Empty, error) { @@ -116,7 +108,6 @@ func (s) TestRetryUnary(t *testing.T) { } func (s) TestRetryThrottling(t *testing.T) { - defer enableRetry()() i := -1 ss := &stubserver.StubServer{ EmptyCallF: func(context.Context, *testpb.Empty) (*testpb.Empty, error) { @@ -192,7 +183,6 @@ func (s) TestRetryThrottling(t *testing.T) { } func (s) TestRetryStreaming(t *testing.T) { - defer enableRetry()() req := func(b byte) *testpb.StreamingOutputCallRequest { return &testpb.StreamingOutputCallRequest{Payload: &testpb.Payload{Body: []byte{b}}} } @@ -510,7 +500,6 @@ func (*retryStatsHandler) TagConn(ctx context.Context, _ *stats.ConnTagInfo) con func (*retryStatsHandler) HandleConn(context.Context, stats.ConnStats) {} func (s) TestRetryStats(t *testing.T) { - defer enableRetry()() lis, err := net.Listen("tcp", "localhost:0") if err != nil { t.Fatalf("Failed to listen. Err: %v", err) diff --git a/xds/internal/test/xds_client_integration_test.go b/xds/internal/test/xds_client_integration_test.go index e26e3e08f4c..6a9a8c9688f 100644 --- a/xds/internal/test/xds_client_integration_test.go +++ b/xds/internal/test/xds_client_integration_test.go @@ -32,7 +32,6 @@ import ( "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/internal/stubserver" "google.golang.org/grpc/internal/testutils" - "google.golang.org/grpc/internal/xds/env" "google.golang.org/grpc/status" "google.golang.org/grpc/xds/internal/testutils/e2e" @@ -105,11 +104,6 @@ func (s) TestClientSideXDS(t *testing.T) { } func (s) TestClientSideRetry(t *testing.T) { - if !env.RetrySupport { - // Skip this test if retry is not enabled. - return - } - ctr := 0 errs := []codes.Code{codes.ResourceExhausted} ss := &stubserver.StubServer{ diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_rds.go b/xds/internal/xdsclient/xdsresource/unmarshal_rds.go index 0642500f303..bcaf1315b9d 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_rds.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_rds.go @@ -109,7 +109,7 @@ func generateRDSUpdateFromRouteConfiguration(rc *v3routepb.RouteConfiguration, l } func generateRetryConfig(rp *v3routepb.RetryPolicy) (*RetryConfig, error) { - if !env.RetrySupport || rp == nil { + if rp == nil { return nil, nil } diff --git a/xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go b/xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go index 38a7e99a9ed..955e08d4b37 100644 --- a/xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go +++ b/xds/internal/xdsclient/xdsresource/unmarshal_rds_test.go @@ -99,10 +99,6 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) { } } goodUpdateWithRetryPolicy = func(vhrc *RetryConfig, rrc *RetryConfig) RouteConfigUpdate { - if !env.RetrySupport { - vhrc = nil - rrc = nil - } return RouteConfigUpdate{ VirtualHosts: []*VirtualHost{{ Domains: []string{ldsTarget}, @@ -116,13 +112,7 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) { }}, } } - defaultRetryBackoff = RetryBackoff{BaseInterval: 25 * time.Millisecond, MaxInterval: 250 * time.Millisecond} - goodUpdateIfRetryDisabled = func() RouteConfigUpdate { - if env.RetrySupport { - return RouteConfigUpdate{} - } - return goodUpdateWithRetryPolicy(nil, nil) - } + defaultRetryBackoff = RetryBackoff{BaseInterval: 25 * time.Millisecond, MaxInterval: 250 * time.Millisecond} ) tests := []struct { @@ -554,26 +544,26 @@ func (s) TestRDSGenerateRDSUpdateFromRouteConfiguration(t *testing.T) { { name: "bad-retry-policy-0-retries", rc: goodRouteConfigWithRetryPolicy(&v3routepb.RetryPolicy{RetryOn: "cancelled", NumRetries: &wrapperspb.UInt32Value{Value: 0}}, nil), - wantUpdate: goodUpdateIfRetryDisabled(), - wantError: env.RetrySupport, + wantUpdate: RouteConfigUpdate{}, + wantError: true, }, { name: "bad-retry-policy-0-base-interval", rc: goodRouteConfigWithRetryPolicy(&v3routepb.RetryPolicy{RetryOn: "cancelled", RetryBackOff: &v3routepb.RetryPolicy_RetryBackOff{BaseInterval: durationpb.New(0)}}, nil), - wantUpdate: goodUpdateIfRetryDisabled(), - wantError: env.RetrySupport, + wantUpdate: RouteConfigUpdate{}, + wantError: true, }, { name: "bad-retry-policy-negative-max-interval", rc: goodRouteConfigWithRetryPolicy(&v3routepb.RetryPolicy{RetryOn: "cancelled", RetryBackOff: &v3routepb.RetryPolicy_RetryBackOff{MaxInterval: durationpb.New(-time.Second)}}, nil), - wantUpdate: goodUpdateIfRetryDisabled(), - wantError: env.RetrySupport, + wantUpdate: RouteConfigUpdate{}, + wantError: true, }, { name: "bad-retry-policy-negative-max-interval-no-known-retry-on", rc: goodRouteConfigWithRetryPolicy(&v3routepb.RetryPolicy{RetryOn: "something", RetryBackOff: &v3routepb.RetryPolicy_RetryBackOff{MaxInterval: durationpb.New(-time.Second)}}, nil), - wantUpdate: goodUpdateIfRetryDisabled(), - wantError: env.RetrySupport, + wantUpdate: RouteConfigUpdate{}, + wantError: true, }, } for _, test := range tests {