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

xds: enable ringhash and retry by default #4776

Merged
merged 4 commits into from
Sep 16, 2021
Merged

xds: enable ringhash and retry by default #4776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
845fc55
[affinity_env_var_on] xds: enable ringhash and retry by default
menghanl Sep 15, 2021
39abf29
[affinity_env_var_on] change how its done for security
menghanl Sep 15, 2021
7677f87
[affinity_env_var_on] test yml delete retry env
menghanl Sep 16, 2021
a1f9b37
[affinity_env_var_on] delete TestRetryDisabledByDefault
menghanl Sep 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .github/workflows/testing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,6 @@ jobs:
goversion: 1.17
testflags: -race

- type: tests
goversion: 1.17
grpcenv: GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY=true

- type: extras
goversion: 1.17

Expand Down
22 changes: 5 additions & 17 deletions internal/xds/env/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,16 @@ var (
// When both bootstrap FileName and FileContent are set, FileName is used.
BootstrapFileContent = os.Getenv(BootstrapFileContentEnv)
// RingHashSupport indicates whether ring hash support is enabled, which can
// be enabled by setting the environment variable
// "GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH" to "true".
RingHashSupport = strings.EqualFold(os.Getenv(ringHashSupportEnv), "true")
// be disabled by setting the environment variable
// "GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH" to "false".
RingHashSupport = !strings.EqualFold(os.Getenv(ringHashSupportEnv), "false")
// ClientSideSecuritySupport is used to control processing of security
// configuration on the client-side.
//
// Note that there is no env var protection for the server-side because we
// have a brand new API on the server-side and users explicitly need to use
// the new API to get security integration on the server.
ClientSideSecuritySupport = strings.EqualFold(os.Getenv(clientSideSecuritySupportEnv), "true")
ClientSideSecuritySupport = !strings.EqualFold(os.Getenv(clientSideSecuritySupportEnv), "false")
// AggregateAndDNSSupportEnv indicates whether processing of aggregated
// cluster and DNS cluster is enabled, which can be enabled by setting the
// environment variable
Expand All @@ -80,7 +80,7 @@ var (
AggregateAndDNSSupportEnv = strings.EqualFold(os.Getenv(aggregateAndDNSSupportEnv), "true")

// RetrySupport indicates whether xDS retry is enabled.
RetrySupport = strings.EqualFold(os.Getenv(retrySupportEnv), "true")
RetrySupport = !strings.EqualFold(os.Getenv(retrySupportEnv), "false")
Copy link

Choose a reason for hiding this comment

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

Was this intentional? It effectively enables Retry by default (not just with xDS). IE: We no longer need to set GRPC_GO_RETRY=on.

In envconfig.go:

Retry = strings.EqualFold(os.Getenv(retryStr), "on") || xdsenv.RetrySupport

envconfig.Retry will allways be true as long as GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY is undefined.

Copy link

Choose a reason for hiding this comment

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

If this was intentional, then GRPC_GO_RETRY should probably be retired, as it does not have the same semantics anymore (and this is confusing).

Copy link
Member

Choose a reason for hiding this comment

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

This was intentional, though it wasn't intentional that it overrides GRPC_GO_RETRY=off. Maybe we should fix that before the release (make GRPC_GO_RETRY default to on, and disable retry if either GRPC_GO_RETRY=off or GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY=false).

Also this should be release-noted.

Copy link
Member

Choose a reason for hiding this comment

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

#4899 fixes GRPC_GO_RETRY=off to disable retry as one would expect.

Copy link

Choose a reason for hiding this comment

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

OK, this is good for me that I can rely on retry being enabled by default from v1.41.0 onward.

Copy link
Member

Choose a reason for hiding this comment

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

Note that we will remove this as a configurable setting entirely in the future (hopefully v1.42). It is & will always be controllable per-ClientConn via the WithDisableRetry dial option. Aaand I'll send a PR to fix the docstring on that method next.


// C2PResolverSupport indicates whether support for C2P resolver is enabled.
// This can be enabled by setting the environment variable
Expand All @@ -89,15 +89,3 @@ var (
// C2PResolverTestOnlyTrafficDirectorURI is the TD URI for testing.
C2PResolverTestOnlyTrafficDirectorURI = os.Getenv(c2pResolverTestOnlyTrafficDirectorURIEnv)
)

func init() {
// Set the env var used to control processing of security configuration on
// the client-side to true by default.
// TODO(easwars): Remove this env var completely in 1.42.x release.
//
// If the env var is set explicitly, honor it.
ClientSideSecuritySupport = true
if val, ok := os.LookupEnv(clientSideSecuritySupportEnv); ok {
ClientSideSecuritySupport = strings.EqualFold(val, "true")
}
}
64 changes: 0 additions & 64 deletions test/retry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"fmt"
"io"
"net"
"os"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -116,69 +115,6 @@ func (s) TestRetryUnary(t *testing.T) {
}
}

func (s) TestRetryDisabledByDefault(t *testing.T) {
if strings.EqualFold(os.Getenv("GRPC_GO_RETRY"), "on") ||
strings.EqualFold(os.Getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RETRY"), "true") {
return
}
i := -1
ss := &stubserver.StubServer{
EmptyCallF: func(context.Context, *testpb.Empty) (*testpb.Empty, error) {
i++
switch i {
case 0:
return nil, status.New(codes.AlreadyExists, "retryable error").Err()
}
return &testpb.Empty{}, nil
},
}
if err := ss.Start([]grpc.ServerOption{}); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()
ss.NewServiceConfig(`{
"methodConfig": [{
"name": [{"service": "grpc.testing.TestService"}],
"waitForReady": true,
"retryPolicy": {
"MaxAttempts": 4,
"InitialBackoff": ".01s",
"MaxBackoff": ".01s",
"BackoffMultiplier": 1.0,
"RetryableStatusCodes": [ "ALREADY_EXISTS" ]
}
}]}`)
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
for {
if ctx.Err() != nil {
t.Fatalf("Timed out waiting for service config update")
}
if ss.CC.GetMethodConfig("/grpc.testing.TestService/EmptyCall").WaitForReady != nil {
break
}
time.Sleep(time.Millisecond)
}
cancel()

testCases := []struct {
code codes.Code
count int
}{
{codes.AlreadyExists, 0},
}
for _, tc := range testCases {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
_, err := ss.Client.EmptyCall(ctx, &testpb.Empty{})
cancel()
if status.Code(err) != tc.code {
t.Fatalf("EmptyCall(_, _) = _, %v; want _, <Code() = %v>", err, tc.code)
}
if i != tc.count {
t.Fatalf("i = %v; want %v", i, tc.count)
}
}
}

func (s) TestRetryThrottling(t *testing.T) {
defer enableRetry()()
i := -1
Expand Down