Skip to content

Commit

Permalink
This is an automated cherry-pick of pingcap#43022
Browse files Browse the repository at this point in the history
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
  • Loading branch information
Leavrth authored and ti-chi-bot committed May 8, 2023
1 parent 3fa4fb9 commit 6491990
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 3 deletions.
6 changes: 6 additions & 0 deletions br/pkg/storage/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ go_library(
"@com_github_klauspost_compress//snappy",
"@com_github_klauspost_compress//zstd",
"@com_github_pingcap_errors//:errors",
"@com_github_pingcap_failpoint//:failpoint",
"@com_github_pingcap_kvproto//pkg/brpb",
"@com_github_pingcap_log//:log",
"@com_github_spf13_pflag//:pflag",
Expand Down Expand Up @@ -70,6 +71,10 @@ go_test(
],
embed = [":storage"],
flaky = True,
<<<<<<< HEAD
=======
shard_count = 43,
>>>>>>> 2d259bdcb67 (br: add more retryable error for retryer (#43022))
deps = [
"//br/pkg/mock",
"@com_github_aws_aws_sdk_go//aws",
Expand All @@ -80,6 +85,7 @@ go_test(
"@com_github_fsouza_fake_gcs_server//fakestorage",
"@com_github_golang_mock//gomock",
"@com_github_pingcap_errors//:errors",
"@com_github_pingcap_failpoint//:failpoint",
"@com_github_pingcap_kvproto//pkg/brpb",
"@com_github_stretchr_testify//require",
],
Expand Down
15 changes: 15 additions & 0 deletions br/pkg/storage/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/aws/aws-sdk-go/service/s3/s3iface"
"github.com/aws/aws-sdk-go/service/s3/s3manager"
"github.com/pingcap/errors"
"github.com/pingcap/failpoint"
backuppb "github.com/pingcap/kvproto/pkg/brpb"
"github.com/pingcap/log"
berrors "github.com/pingcap/tidb/br/pkg/errors"
Expand Down Expand Up @@ -943,7 +944,21 @@ func isDeadlineExceedError(err error) bool {
return strings.Contains(err.Error(), "context deadline exceeded")
}

func isConnectionResetError(err error) bool {
return strings.Contains(err.Error(), "read: connection reset")
}

func (rl retryerWithLog) ShouldRetry(r *request.Request) bool {
// for unit test
failpoint.Inject("replace-error-to-connection-reset-by-peer", func(_ failpoint.Value) {
log.Info("original error", zap.Error(r.Error))
if r.Error != nil {
r.Error = errors.New("read tcp *.*.*.*:*->*.*.*.*:*: read: connection reset by peer")
}
})
if isConnectionResetError(r.Error) {
return true
}
if isDeadlineExceedError(r.Error) && r.HTTPRequest.URL.Host == ec2MetaAddress {
// fast fail for unreachable linklocal address in EC2 containers.
log.Warn("failed to get EC2 metadata. skipping.", logutil.ShortError(r.Error))
Expand Down
123 changes: 123 additions & 0 deletions br/pkg/storage/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"net/http"
"net/http/httptest"
"os"
"sync"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand All @@ -20,6 +21,7 @@ import (
"github.com/aws/aws-sdk-go/service/s3"
"github.com/golang/mock/gomock"
"github.com/pingcap/errors"
"github.com/pingcap/failpoint"
backuppb "github.com/pingcap/kvproto/pkg/brpb"
"github.com/pingcap/tidb/br/pkg/mock"
. "github.com/pingcap/tidb/br/pkg/storage"
Expand Down Expand Up @@ -1193,3 +1195,124 @@ func TestObjectLock(t *testing.T) {
)
require.Equal(t, true, s.storage.IsObjectLockEnabled())
}
<<<<<<< HEAD
=======

func TestS3StorageBucketRegion(t *testing.T) {
type testcase struct {
name string
expectRegion string
s3 *backuppb.S3
}

require.NoError(t, os.Setenv("AWS_ACCESS_KEY_ID", "ab"))
require.NoError(t, os.Setenv("AWS_SECRET_ACCESS_KEY", "cd"))
require.NoError(t, os.Setenv("AWS_SESSION_TOKEN", "ef"))

cases := []testcase{
{
"empty region from aws",
"us-east-1",
&backuppb.S3{
Region: "",
Bucket: "bucket",
Prefix: "prefix",
Provider: "aws",
},
},
{
"region from different provider",
"sdg",
&backuppb.S3{
Region: "sdg",
Bucket: "bucket",
Prefix: "prefix",
Provider: "ovh",
},
},
{
"empty region from different provider",
"",
&backuppb.S3{
Region: "",
Bucket: "bucket",
Prefix: "prefix",
Provider: "ovh",
},
},
{
"region from aws",
"us-west-2",
&backuppb.S3{
Region: "us-west-2",
Bucket: "bucket",
Prefix: "prefix",
Provider: "aws",
},
},
}
for _, ca := range cases {
func(name string, region string, s3 *backuppb.S3) {
s := createGetBucketRegionServer(region, 200, true)
defer s.Close()
s3.ForcePathStyle = true
s3.Endpoint = s.URL

t.Log(name)
es, err := New(context.Background(),
&backuppb.StorageBackend{Backend: &backuppb.StorageBackend_S3{S3: s3}},
&ExternalStorageOptions{})
require.NoError(t, err)
ss, ok := es.(*S3Storage)
require.True(t, ok)
require.Equal(t, region, ss.GetOptions().Region)
}(ca.name, ca.expectRegion, ca.s3)
}
}

func TestRetryError(t *testing.T) {
var count int32 = 0
var errString string = "read tcp *.*.*.*:*->*.*.*.*:*: read: connection reset by peer"
var lock sync.Mutex
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method == "PUT" {
var curCnt int32
t.Log(r.URL)
lock.Lock()
count += 1
curCnt = count
lock.Unlock()
if curCnt < 2 {
// write an cannot-retry error, but we modify the error to specific error, so client would retry.
w.WriteHeader(403)
return
}
}

w.WriteHeader(200)
}))

defer server.Close()
t.Log(server.URL)

require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/br/pkg/storage/replace-error-to-connection-reset-by-peer", "return(true)"))
defer func() {
failpoint.Disable("github.com/pingcap/tidb/br/pkg/storage/replace-error-to-connection-reset-by-peer")
}()

ctx := context.Background()
s, err := NewS3Storage(ctx, &backuppb.S3{
Endpoint: server.URL,
Bucket: "test",
Prefix: "retry",
AccessKey: "none",
SecretAccessKey: "none",
Provider: "skip check region",
ForcePathStyle: true,
}, &ExternalStorageOptions{})
require.NoError(t, err)
err = s.WriteFile(ctx, "reset", []byte(errString))
require.NoError(t, err)
require.Equal(t, count, int32(2))
}
>>>>>>> 2d259bdcb67 (br: add more retryable error for retryer (#43022))
4 changes: 2 additions & 2 deletions br/pkg/utils/backoff.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,11 @@ func (bo *pdReqBackoffer) NextBackoff(err error) time.Duration {
// bo.attempt--
e := errors.Cause(err)
switch e { // nolint:errorlint
case nil, context.Canceled, context.DeadlineExceeded, io.EOF, sql.ErrNoRows:
case nil, context.Canceled, context.DeadlineExceeded, sql.ErrNoRows:
// Excepted error, finish the operation
bo.delayTime = 0
bo.attempt = 0
case berrors.ErrRestoreTotalKVMismatch:
case berrors.ErrRestoreTotalKVMismatch, io.EOF:
bo.delayTime = 2 * bo.delayTime
bo.attempt--
default:
Expand Down
6 changes: 5 additions & 1 deletion br/pkg/utils/backoff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package utils_test

import (
"context"
"io"
"testing"
"time"

Expand Down Expand Up @@ -101,13 +102,16 @@ func TestPdBackoffWithRetryableError(t *testing.T) {
gRPCError := status.Error(codes.Unavailable, "transport is closing")
err := utils.WithRetry(context.Background(), func() error {
defer func() { counter++ }()
if counter == 2 {
return io.EOF
}
return gRPCError
}, backoffer)
require.Equal(t, 16, counter)
require.Equal(t, []error{
gRPCError,
gRPCError,
gRPCError,
io.EOF,
gRPCError,
gRPCError,
gRPCError,
Expand Down

0 comments on commit 6491990

Please sign in to comment.