From 30d3c93cda49bb51e059e36bbf118c3feabd88f6 Mon Sep 17 00:00:00 2001 From: wei840222 Date: Tue, 24 Aug 2021 16:53:19 +0800 Subject: [PATCH 1/6] make s3 backend readError logic more robust --- tempodb/backend/s3/s3.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tempodb/backend/s3/s3.go b/tempodb/backend/s3/s3.go index 01e7a76d7d5..764e426ae63 100644 --- a/tempodb/backend/s3/s3.go +++ b/tempodb/backend/s3/s3.go @@ -26,8 +26,9 @@ import ( ) const ( - s3KeyDoesNotExist = "The specified key does not exist." - uptoHedgedRequests = 2 + s3KeyDoesNotExist = "The specified key does not exist." + s3KeyDoesNotExistCode = "NoSuchKey" + uptoHedgedRequests = 2 ) // readerWriter can read/write from an s3 backend @@ -363,5 +364,9 @@ func readError(err error) error { return backend.ErrDoesNotExist } + if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { + return backend.ErrDoesNotExist + } + return err } From ff0f95a8d3dddd409fba8499b919a55038d75dcc Mon Sep 17 00:00:00 2001 From: wei840222 Date: Tue, 24 Aug 2021 17:00:45 +0800 Subject: [PATCH 2/6] update change log --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66f75cc62f2..eb1f53e0dbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * [BUGFIX] Cortex upgrade to fix an issue where unhealthy compactors can't be forgotten [#878](https://github.com/grafana/tempo/pull/878) (@joe-elliott) * [ENHANCEMENT] Added "query blocks" cli option. [#876](https://github.com/grafana/tempo/pull/876) (@joe-elliott) * [ENHANCEMENT] Added traceid to `trace too large message`. [#888](https://github.com/grafana/tempo/pull/888) (@mritunjaysharma394) +* [ENHANCEMENT] Make s3 backend readError logic more robust [#905](https://github.com/grafana/tempo/pull/905) (@wei840222) ## v1.1.0-rc.0 / 2021-08-11 From b9f861b112e405b506733e441455b0a170c84d33 Mon Sep 17 00:00:00 2001 From: wei840222 Date: Tue, 24 Aug 2021 17:08:20 +0800 Subject: [PATCH 3/6] also add s3KeyDoesNotExistCode check on readAllWithObjInfo --- tempodb/backend/s3/s3.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tempodb/backend/s3/s3.go b/tempodb/backend/s3/s3.go index 764e426ae63..a384bd70972 100644 --- a/tempodb/backend/s3/s3.go +++ b/tempodb/backend/s3/s3.go @@ -265,6 +265,8 @@ func (rw *readerWriter) readAllWithObjInfo(ctx context.Context, name string) ([] reader, info, _, err := rw.hedgedCore.GetObject(ctx, rw.cfg.Bucket, name, minio.GetObjectOptions{}) if err != nil && err.Error() == s3KeyDoesNotExist { return nil, minio.ObjectInfo{}, backend.ErrDoesNotExist + } else if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { + return nil, minio.ObjectInfo{}, backend.ErrDoesNotExist } else if err != nil { return nil, minio.ObjectInfo{}, errors.Wrap(err, "error fetching object from s3 backend") } From 673108ea90532101211676078daadaa161973071 Mon Sep 17 00:00:00 2001 From: wei840222 Date: Tue, 24 Aug 2021 18:24:29 +0800 Subject: [PATCH 4/6] only check minio error code for object not exists --- tempodb/backend/s3/s3.go | 10 +--------- tempodb/backend/s3/s3_test.go | 5 ++++- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/tempodb/backend/s3/s3.go b/tempodb/backend/s3/s3.go index a384bd70972..2caf346dfd1 100644 --- a/tempodb/backend/s3/s3.go +++ b/tempodb/backend/s3/s3.go @@ -26,7 +26,6 @@ import ( ) const ( - s3KeyDoesNotExist = "The specified key does not exist." s3KeyDoesNotExistCode = "NoSuchKey" uptoHedgedRequests = 2 ) @@ -263,9 +262,7 @@ func (rw *readerWriter) readAll(ctx context.Context, name string) ([]byte, error func (rw *readerWriter) readAllWithObjInfo(ctx context.Context, name string) ([]byte, minio.ObjectInfo, error) { reader, info, _, err := rw.hedgedCore.GetObject(ctx, rw.cfg.Bucket, name, minio.GetObjectOptions{}) - if err != nil && err.Error() == s3KeyDoesNotExist { - return nil, minio.ObjectInfo{}, backend.ErrDoesNotExist - } else if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { + if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { return nil, minio.ObjectInfo{}, backend.ErrDoesNotExist } else if err != nil { return nil, minio.ObjectInfo{}, errors.Wrap(err, "error fetching object from s3 backend") @@ -362,13 +359,8 @@ func createCore(cfg *Config, hedge bool) (*minio.Core, error) { } func readError(err error) error { - if err != nil && err.Error() == s3KeyDoesNotExist { - return backend.ErrDoesNotExist - } - if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { return backend.ErrDoesNotExist } - return err } diff --git a/tempodb/backend/s3/s3_test.go b/tempodb/backend/s3/s3_test.go index eb1e7f2ee58..4045e26f906 100644 --- a/tempodb/backend/s3/s3_test.go +++ b/tempodb/backend/s3/s3_test.go @@ -12,6 +12,7 @@ import ( "github.com/cortexproject/cortex/pkg/util/flagext" "github.com/grafana/tempo/tempodb/backend" + "github.com/minio/minio-go/v7" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -103,7 +104,9 @@ func fakeServer(t *testing.T, returnIn time.Duration, counter *int32) *httptest. } func TestReadError(t *testing.T) { - errA := fmt.Errorf(s3KeyDoesNotExist) + errA := minio.ErrorResponse{ + Code: s3KeyDoesNotExistCode, + } errB := readError(errA) assert.Equal(t, backend.ErrDoesNotExist, errB) From e37db1853385bd5a80222cf4c6b17c976a4889e1 Mon Sep 17 00:00:00 2001 From: wei840222 Date: Tue, 24 Aug 2021 22:40:42 +0800 Subject: [PATCH 5/6] using aws sdk s3 error code --- tempodb/backend/s3/s3.go | 8 ++++---- tempodb/backend/s3/s3_test.go | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tempodb/backend/s3/s3.go b/tempodb/backend/s3/s3.go index 2caf346dfd1..c7b7b2f41af 100644 --- a/tempodb/backend/s3/s3.go +++ b/tempodb/backend/s3/s3.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/tempo/tempodb/backend/instrumentation" + "github.com/aws/aws-sdk-go/service/s3" log_util "github.com/cortexproject/cortex/pkg/util/log" "github.com/cristalhq/hedgedhttp" "github.com/go-kit/kit/log" @@ -26,8 +27,7 @@ import ( ) const ( - s3KeyDoesNotExistCode = "NoSuchKey" - uptoHedgedRequests = 2 + uptoHedgedRequests = 2 ) // readerWriter can read/write from an s3 backend @@ -262,7 +262,7 @@ func (rw *readerWriter) readAll(ctx context.Context, name string) ([]byte, error func (rw *readerWriter) readAllWithObjInfo(ctx context.Context, name string) ([]byte, minio.ObjectInfo, error) { reader, info, _, err := rw.hedgedCore.GetObject(ctx, rw.cfg.Bucket, name, minio.GetObjectOptions{}) - if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { + if err != nil && minio.ToErrorResponse(err).Code == s3.ErrCodeNoSuchKey { return nil, minio.ObjectInfo{}, backend.ErrDoesNotExist } else if err != nil { return nil, minio.ObjectInfo{}, errors.Wrap(err, "error fetching object from s3 backend") @@ -359,7 +359,7 @@ func createCore(cfg *Config, hedge bool) (*minio.Core, error) { } func readError(err error) error { - if err != nil && minio.ToErrorResponse(err).Code == s3KeyDoesNotExistCode { + if err != nil && minio.ToErrorResponse(err).Code == s3.ErrCodeNoSuchKey { return backend.ErrDoesNotExist } return err diff --git a/tempodb/backend/s3/s3_test.go b/tempodb/backend/s3/s3_test.go index 4045e26f906..957c7e6dcd4 100644 --- a/tempodb/backend/s3/s3_test.go +++ b/tempodb/backend/s3/s3_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go/service/s3" "github.com/cortexproject/cortex/pkg/util/flagext" "github.com/grafana/tempo/tempodb/backend" "github.com/minio/minio-go/v7" @@ -105,7 +106,7 @@ func fakeServer(t *testing.T, returnIn time.Duration, counter *int32) *httptest. func TestReadError(t *testing.T) { errA := minio.ErrorResponse{ - Code: s3KeyDoesNotExistCode, + Code: s3.ErrCodeNoSuchKey, } errB := readError(errA) assert.Equal(t, backend.ErrDoesNotExist, errB) From 88b0b8cf6f1f16168fa0b36cf14851cb7a6a9781 Mon Sep 17 00:00:00 2001 From: wei840222 Date: Wed, 25 Aug 2021 21:53:22 +0800 Subject: [PATCH 6/6] fix vendor error --- go.mod | 1 + vendor/modules.txt | 1 + 2 files changed, 2 insertions(+) diff --git a/go.mod b/go.mod index 77fd9dc6f25..c395e320755 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/Azure/azure-pipeline-go v0.2.2 github.com/Azure/azure-storage-blob-go v0.8.0 github.com/alecthomas/kong v0.2.11 + github.com/aws/aws-sdk-go v1.38.60 github.com/cespare/xxhash v1.1.0 github.com/cortexproject/cortex v1.10.1-0.20210816080356-090988c40f3e github.com/cristalhq/hedgedhttp v0.6.0 diff --git a/vendor/modules.txt b/vendor/modules.txt index f5a3fa7f39d..40d14725291 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -69,6 +69,7 @@ github.com/armon/go-metrics/prometheus # github.com/asaskevich/govalidator v0.0.0-20200907205600-7a23bdc65eef github.com/asaskevich/govalidator # github.com/aws/aws-sdk-go v1.38.60 +## explicit github.com/aws/aws-sdk-go/aws github.com/aws/aws-sdk-go/aws/arn github.com/aws/aws-sdk-go/aws/awserr