From 584e690a9a9de6e4d83a530a3c3b07629150fcf2 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Thu, 4 May 2023 10:13:10 +0200 Subject: [PATCH] Fix inconsistent error for series limits in Store API (#6330) * store: fix inconsistent error for series limits Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> * update changelog Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> * Update pkg/store/bucket.go Co-authored-by: Saswata Mukherjee Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> * Update pkg/store/bucket.go Co-authored-by: Saswata Mukherjee Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> * rename labelValues serires liimiter test function Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --------- Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Co-authored-by: Saswata Mukherjee --- CHANGELOG.md | 1 + pkg/store/bucket.go | 12 +++- pkg/store/bucket_e2e_test.go | 110 +++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a95be6c8805..3126c1bf6a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re - [#6222](https://github.com/thanos-io/thanos/pull/6222) mixin(Receive): Fix tenant series received charts. - [#6218](https://github.com/thanos-io/thanos/pull/6218) mixin(Store): handle ResourceExhausted as a non-server error. As a consequence, this error won't contribute to Store's grpc errors alerts. - [#6271](https://github.com/thanos-io/thanos/pull/6271) Receive: Fix segfault in `LabelValues` during head compaction. +- [#6330](https://github.com/thanos-io/thanos/pull/6330) Store: Fix inconsistent error for series limits. ### Changed - [#6168](https://github.com/thanos-io/thanos/pull/6168) Receiver: Make ketama hashring fail early when configured with number of nodes lower than the replication factor. diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index ac4e439ab22..876f8220c6d 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1574,7 +1574,11 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq s.mtx.RUnlock() if err := g.Wait(); err != nil { - return nil, status.Error(codes.Internal, err.Error()) + code := codes.Internal + if s, ok := status.FromError(errors.Cause(err)); ok { + code = s.Code() + } + return nil, status.Error(code, err.Error()) } anyHints, err := types.MarshalAny(resHints) @@ -1762,7 +1766,11 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR s.mtx.RUnlock() if err := g.Wait(); err != nil { - return nil, status.Error(codes.Aborted, err.Error()) + code := codes.Internal + if s, ok := status.FromError(errors.Cause(err)); ok { + code = s.Code() + } + return nil, status.Error(code, err.Error()) } anyHints, err := types.MarshalAny(resHints) diff --git a/pkg/store/bucket_e2e_test.go b/pkg/store/bucket_e2e_test.go index 3d30ee171ce..1847e336b70 100644 --- a/pkg/store/bucket_e2e_test.go +++ b/pkg/store/bucket_e2e_test.go @@ -6,6 +6,7 @@ package store import ( "context" "fmt" + "math" "os" "path/filepath" "strings" @@ -760,6 +761,57 @@ func TestBucketStore_LabelNames_e2e(t *testing.T) { }) } +func TestBucketStore_LabelNames_SeriesLimiter_e2e(t *testing.T) { + cases := map[string]struct { + maxSeriesLimit uint64 + expectedErr string + code codes.Code + }{ + "should succeed if the max series limit is not exceeded": { + maxSeriesLimit: math.MaxUint64, + }, + "should fail if the max series limit is exceeded - ResourceExhausted": { + expectedErr: "exceeded series limit", + maxSeriesLimit: 1, + code: codes.ResourceExhausted, + }, + } + + for testName, testData := range cases { + t.Run(testName, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + bkt := objstore.NewInMemBucket() + dir := t.TempDir() + s := prepareStoreWithTestBlocks(t, dir, bkt, false, NewChunksLimiterFactory(0), NewSeriesLimiterFactory(testData.maxSeriesLimit), NewBytesLimiterFactory(0), emptyRelabelConfig, allowAllFilterConf) + testutil.Ok(t, s.store.SyncBlocks(ctx)) + req := &storepb.LabelNamesRequest{ + Matchers: []storepb.LabelMatcher{ + {Type: storepb.LabelMatcher_EQ, Name: "a", Value: "1"}, + }, + Start: minTimeDuration.PrometheusTimestamp(), + End: maxTimeDuration.PrometheusTimestamp(), + } + + s.cache.SwapWith(noopCache{}) + + _, err := s.store.LabelNames(context.Background(), req) + + if testData.expectedErr == "" { + testutil.Ok(t, err) + } else { + testutil.NotOk(t, err) + testutil.Assert(t, strings.Contains(err.Error(), testData.expectedErr)) + + status, ok := status.FromError(err) + testutil.Equals(t, true, ok) + testutil.Equals(t, testData.code, status.Code()) + } + }) + } +} + func TestBucketStore_LabelValues_e2e(t *testing.T) { objtesting.ForeachStore(t, func(t *testing.T, bkt objstore.Bucket) { ctx, cancel := context.WithCancel(context.Background()) @@ -867,6 +919,64 @@ func TestBucketStore_LabelValues_e2e(t *testing.T) { }) } +func TestBucketStore_LabelValues_SeriesLimiter_e2e(t *testing.T) { + cases := map[string]struct { + maxSeriesLimit uint64 + expectedErr string + code codes.Code + }{ + "should succeed if the max chunks limit is not exceeded": { + maxSeriesLimit: math.MaxUint64, + }, + "should fail if the max series limit is exceeded - ResourceExhausted": { + expectedErr: "exceeded series limit", + maxSeriesLimit: 1, + code: codes.ResourceExhausted, + }, + } + + for testName, testData := range cases { + t.Run(testName, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + bkt := objstore.NewInMemBucket() + + dir := t.TempDir() + + s := prepareStoreWithTestBlocks(t, dir, bkt, false, NewChunksLimiterFactory(0), NewSeriesLimiterFactory(testData.maxSeriesLimit), NewBytesLimiterFactory(0), emptyRelabelConfig, allowAllFilterConf) + testutil.Ok(t, s.store.SyncBlocks(ctx)) + + req := &storepb.LabelValuesRequest{ + Label: "a", + Start: minTimeDuration.PrometheusTimestamp(), + End: maxTimeDuration.PrometheusTimestamp(), + Matchers: []storepb.LabelMatcher{ + { + Type: storepb.LabelMatcher_EQ, + Name: "a", + Value: "1", + }, + }, + } + + s.cache.SwapWith(noopCache{}) + + _, err := s.store.LabelValues(context.Background(), req) + + if testData.expectedErr == "" { + testutil.Ok(t, err) + } else { + testutil.NotOk(t, err) + testutil.Assert(t, strings.Contains(err.Error(), testData.expectedErr)) + + status, ok := status.FromError(err) + testutil.Equals(t, true, ok) + testutil.Equals(t, testData.code, status.Code()) + } + }) + } +} + func emptyToNil(values []string) []string { if len(values) == 0 { return nil