From d3c410212d13e93a43cb9e56bed84cb04d9ec0cc Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Tue, 27 Feb 2024 12:15:06 -0800 Subject: [PATCH 1/4] metric bucket should return nil when error is expected Signed-off-by: Ben Ye --- objstore.go | 29 +++++++++++++----- objstore_test.go | 78 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 97 insertions(+), 10 deletions(-) diff --git a/objstore.go b/objstore.go index 86ecfa2..1d3f17c 100644 --- a/objstore.go +++ b/objstore.go @@ -633,9 +633,12 @@ func (b *metricBucket) Iter(ctx context.Context, dir string, f func(string) erro err := b.bkt.Iter(ctx, dir, f, options...) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } + return err } return err } @@ -668,7 +671,9 @@ func (b *metricBucket) Attributes(ctx context.Context, name string) (ObjectAttri start := time.Now() attrs, err := b.bkt.Attributes(ctx, name) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } return attrs, err @@ -685,7 +690,9 @@ func (b *metricBucket) Get(ctx context.Context, name string) (io.ReadCloser, err rc, err := b.bkt.Get(ctx, name) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } b.metrics.opsDuration.WithLabelValues(op).Observe(time.Since(start).Seconds()) @@ -712,7 +719,9 @@ func (b *metricBucket) GetRange(ctx context.Context, name string, off, length in rc, err := b.bkt.GetRange(ctx, name, off, length) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } b.metrics.opsDuration.WithLabelValues(op).Observe(time.Since(start).Seconds()) @@ -738,7 +747,9 @@ func (b *metricBucket) Exists(ctx context.Context, name string) (bool, error) { start := time.Now() ok, err := b.bkt.Exists(ctx, name) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } return false, err @@ -767,7 +778,9 @@ func (b *metricBucket) Upload(ctx context.Context, name string, r io.Reader) err defer trc.Close() err := b.bkt.Upload(ctx, name, trc) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } return err @@ -783,7 +796,9 @@ func (b *metricBucket) Delete(ctx context.Context, name string) error { start := time.Now() if err := b.bkt.Delete(ctx, name); err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } return err diff --git a/objstore_test.go b/objstore_test.go index b1f8292..9a0d28b 100644 --- a/objstore_test.go +++ b/objstore_test.go @@ -537,6 +537,54 @@ func TestDownloadDir_CleanUp(t *testing.T) { testutil.Assert(t, os.IsNotExist(err)) } +func TestBucketExpectedErrNoReturnError(t *testing.T) { + expectedErr := errors.New("test error") + + bucket := WrapWithMetrics(&mockBucket{ + get: func(_ context.Context, _ string) (io.ReadCloser, error) { + return nil, expectedErr + }, + getRange: func(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) { + return nil, expectedErr + }, + upload: func(ctx context.Context, name string, r io.Reader) error { + return expectedErr + }, + iter: func(ctx context.Context, dir string, f func(string) error, options ...IterOption) error { + return expectedErr + }, + attributes: func(ctx context.Context, name string) (ObjectAttributes, error) { + return ObjectAttributes{}, expectedErr + }, + exists: func(ctx context.Context, name string) (bool, error) { + return false, expectedErr + }, + }, nil, "").WithExpectedErrs(func(err error) bool { + return errors.Is(err, expectedErr) + }) + + // Expect no error to be returned since the error is expected. + _, err := bucket.Get(context.Background(), "") + testutil.Ok(t, err) + + _, err = bucket.GetRange(context.Background(), "", 1, 2) + testutil.Ok(t, err) + + err = bucket.Upload(context.Background(), "", nil) + testutil.Ok(t, err) + + err = bucket.Iter(context.Background(), "", func(s string) error { + return nil + }) + testutil.Ok(t, err) + + _, err = bucket.Exists(context.Background(), "") + testutil.Ok(t, err) + + _, err = bucket.Attributes(context.Background(), "") + testutil.Ok(t, err) +} + // unreliableBucket implements Bucket and returns an error on every n-th Get. type unreliableBucket struct { Bucket @@ -570,9 +618,12 @@ func (r *mockReader) Close() error { type mockBucket struct { Bucket - upload func(ctx context.Context, name string, r io.Reader) error - get func(ctx context.Context, name string) (io.ReadCloser, error) - getRange func(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) + upload func(ctx context.Context, name string, r io.Reader) error + get func(ctx context.Context, name string) (io.ReadCloser, error) + getRange func(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) + iter func(ctx context.Context, dir string, f func(string) error, options ...IterOption) error + attributes func(ctx context.Context, name string) (ObjectAttributes, error) + exists func(ctx context.Context, name string) (bool, error) } func (b *mockBucket) Upload(ctx context.Context, name string, r io.Reader) error { @@ -596,6 +647,27 @@ func (b *mockBucket) GetRange(ctx context.Context, name string, off, length int6 return nil, errors.New("GetRange has not been mocked") } +func (b *mockBucket) Iter(ctx context.Context, dir string, f func(string) error, options ...IterOption) error { + if b.iter != nil { + return b.iter(ctx, dir, f, options...) + } + return errors.New("Iter has not been mocked") +} + +func (b *mockBucket) Exists(ctx context.Context, name string) (bool, error) { + if b.exists != nil { + return b.exists(ctx, name) + } + return false, errors.New("Exists has not been mocked") +} + +func (b *mockBucket) Attributes(ctx context.Context, name string) (ObjectAttributes, error) { + if b.attributes != nil { + return b.attributes(ctx, name) + } + return ObjectAttributes{}, errors.New("Attributes has not been mocked") +} + func Test_TryToGetSizeLimitedReader(t *testing.T) { b := &bytes.Buffer{} r := io.LimitReader(b, 1024) From 383edf7cb17ee09359b704439ca4cec400e72f3c Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Tue, 27 Feb 2024 13:30:38 -0800 Subject: [PATCH 2/4] fix acceptance test Signed-off-by: Ben Ye --- objstore_test.go | 4 ++-- objtesting/acceptance_e2e_test.go | 2 +- prefixed_bucket_test.go | 2 +- testing.go | 18 +++++++++++++----- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/objstore_test.go b/objstore_test.go index 9a0d28b..79c94ee 100644 --- a/objstore_test.go +++ b/objstore_test.go @@ -28,7 +28,7 @@ func TestMetricBucket_Close(t *testing.T) { testutil.Equals(t, 7, promtest.CollectAndCount(bkt.metrics.opsFailures)) testutil.Equals(t, 7, promtest.CollectAndCount(bkt.metrics.opsDuration)) - AcceptanceTest(t, bkt.WithExpectedErrs(bkt.IsObjNotFoundErr)) + AcceptanceTest(t, bkt.WithExpectedErrs(bkt.IsObjNotFoundErr), true) testutil.Equals(t, float64(9), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpIter))) testutil.Equals(t, float64(2), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpAttributes))) testutil.Equals(t, float64(3), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpGet))) @@ -51,7 +51,7 @@ func TestMetricBucket_Close(t *testing.T) { // Clear bucket, but don't clear metrics to ensure we use same. bkt.bkt = NewInMemBucket() - AcceptanceTest(t, bkt) + AcceptanceTestWithoutNotFoundErr(t, bkt) testutil.Equals(t, float64(18), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpIter))) testutil.Equals(t, float64(4), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpAttributes))) testutil.Equals(t, float64(6), promtest.ToFloat64(bkt.metrics.ops.WithLabelValues(OpGet))) diff --git a/objtesting/acceptance_e2e_test.go b/objtesting/acceptance_e2e_test.go index e162cee..611dd38 100644 --- a/objtesting/acceptance_e2e_test.go +++ b/objtesting/acceptance_e2e_test.go @@ -14,5 +14,5 @@ import ( // NOTE: This test assumes strong consistency, but in the same way it does not guarantee that if it passes, the // used object store is strongly consistent. func TestObjStore_AcceptanceTest_e2e(t *testing.T) { - ForeachStore(t, objstore.AcceptanceTest) + ForeachStore(t, objstore.AcceptanceTestWithoutNotFoundErr) } diff --git a/prefixed_bucket_test.go b/prefixed_bucket_test.go index 6252e05..230b54c 100644 --- a/prefixed_bucket_test.go +++ b/prefixed_bucket_test.go @@ -23,7 +23,7 @@ func TestPrefixedBucket_Acceptance(t *testing.T) { "someprefix"} for _, prefix := range prefixes { - AcceptanceTest(t, NewPrefixedBucket(NewInMemBucket(), prefix)) + AcceptanceTestWithoutNotFoundErr(t, NewPrefixedBucket(NewInMemBucket(), prefix)) UsesPrefixTest(t, NewInMemBucket(), prefix) } } diff --git a/testing.go b/testing.go index 80f1e19..bd6fe85 100644 --- a/testing.go +++ b/testing.go @@ -80,7 +80,11 @@ func (b noopInstrumentedBucket) ReaderWithExpectedErrs(IsOpFailureExpectedFunc) return b } -func AcceptanceTest(t *testing.T, bkt Bucket) { +func AcceptanceTestWithoutNotFoundErr(t *testing.T, bkt Bucket) { + AcceptanceTest(t, bkt, false) +} + +func AcceptanceTest(t *testing.T, bkt Bucket, expectedNotFoundErr bool) { ctx := context.Background() _, err := bkt.Get(ctx, "") @@ -88,16 +92,20 @@ func AcceptanceTest(t *testing.T, bkt Bucket) { testutil.Assert(t, !bkt.IsObjNotFoundErr(err), "expected user error got not found %s", err) _, err = bkt.Get(ctx, "id1/obj_1.some") - testutil.NotOk(t, err) - testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error got %s", err) + if !expectedNotFoundErr { + testutil.NotOk(t, err) + testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error got %s", err) + } ok, err := bkt.Exists(ctx, "id1/obj_1.some") testutil.Ok(t, err) testutil.Assert(t, !ok, "expected not exits") _, err = bkt.Attributes(ctx, "id1/obj_1.some") - testutil.NotOk(t, err) - testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error but got %s", err) + if !expectedNotFoundErr { + testutil.NotOk(t, err) + testutil.Assert(t, bkt.IsObjNotFoundErr(err), "expected not found error got %s", err) + } // Upload first object. testutil.Ok(t, bkt.Upload(ctx, "id1/obj_1.some", strings.NewReader("@test-data@"))) From 0c01d64a8313d844d14a93ce48622af87f0ad88c Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Wed, 11 Dec 2024 21:03:30 -0800 Subject: [PATCH 3/4] address comment and handle new IterWithAttributes method Signed-off-by: Ben Ye --- objstore.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/objstore.go b/objstore.go index 1d3f17c..0bdd3f4 100644 --- a/objstore.go +++ b/objstore.go @@ -638,7 +638,6 @@ func (b *metricBucket) Iter(ctx context.Context, dir string, f func(string) erro } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } - return err } return err } @@ -652,7 +651,9 @@ func (b *metricBucket) IterWithAttributes(ctx context.Context, dir string, f fun err := b.bkt.IterWithAttributes(ctx, dir, f, options...) if err != nil { - if !b.metrics.isOpFailureExpected(err) && ctx.Err() != context.Canceled { + if b.metrics.isOpFailureExpected(err) { + err = nil + } else if ctx.Err() != context.Canceled { b.metrics.opsFailures.WithLabelValues(op).Inc() } } From 238812f5b3f299718ebb8b0b722f6e3c1baafbf3 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Thu, 12 Dec 2024 13:32:07 -0800 Subject: [PATCH 4/4] update changelog Signed-off-by: Ben Ye --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0779d6..c068dbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,4 +68,6 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re - [#71](https://github.com/thanos-io/objstore/pull/71) Replace method `IsCustomerManagedKeyError` for a more generic `IsAccessDeniedErr` on the bucket interface. - [#89](https://github.com/thanos-io/objstore/pull/89) GCS: Upgrade cloud.google.com/go/storage version to `v1.35.1`. - [#123](https://github.com/thanos-io/objstore/pull/123) *: Upgrade minio-go version to `v7.0.71`. +- [#103](https://github.com/thanos-io/objstore/pull/103) *: Don't return error in metric bucket if the error is expected. + ### Removed