From e57ce8c6d25c3fce22592b18ef2207b658499802 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Tue, 2 May 2023 19:12:33 +0200 Subject: [PATCH 1/5] store: fix inconsistent error for series limits Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- pkg/store/bucket.go | 12 ++++ pkg/store/bucket_e2e_test.go | 110 +++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index ac4e439ab2..15084d4791 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1574,6 +1574,12 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq s.mtx.RUnlock() if err := g.Wait(); err != nil { + if statusErr, ok := status.FromError(err); ok { + if statusErr.Code() == codes.ResourceExhausted { + return nil, status.Error(codes.ResourceExhausted, err.Error()) + } + } + return nil, status.Error(codes.Internal, err.Error()) } @@ -1762,6 +1768,12 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR s.mtx.RUnlock() if err := g.Wait(); err != nil { + if statusErr, ok := status.FromError(err); ok { + if statusErr.Code() == codes.ResourceExhausted { + return nil, status.Error(codes.ResourceExhausted, err.Error()) + } + } + return nil, status.Error(codes.Aborted, err.Error()) } diff --git a/pkg/store/bucket_e2e_test.go b/pkg/store/bucket_e2e_test.go index 3d30ee171c..a229d89600 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_ChunksLimiter_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 From 31e49d59e64768fa84a871525df0b9d49de30c4c Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Wed, 3 May 2023 10:39:55 +0200 Subject: [PATCH 2/5] update changelog Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a08c755f21..d1ebe4769f 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. From 5d8a0bdd13585f6c2dc7fd37bbb8354ee81dd620 Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Thu, 4 May 2023 09:10:45 +0200 Subject: [PATCH 3/5] Update pkg/store/bucket.go Co-authored-by: Saswata Mukherjee Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- pkg/store/bucket.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index 15084d4791..bba434727a 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1574,13 +1574,11 @@ func (s *BucketStore) LabelNames(ctx context.Context, req *storepb.LabelNamesReq s.mtx.RUnlock() if err := g.Wait(); err != nil { - if statusErr, ok := status.FromError(err); ok { - if statusErr.Code() == codes.ResourceExhausted { - return nil, status.Error(codes.ResourceExhausted, err.Error()) - } + code := codes.Internal + if s, ok := status.FromError(errors.Cause(err)); ok { + code = s.Code() } - - return nil, status.Error(codes.Internal, err.Error()) + return nil, status.Error(code, err.Error()) } anyHints, err := types.MarshalAny(resHints) From d437316f25e9f96da49996582c6e2d10d6f2815e Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Thu, 4 May 2023 09:10:55 +0200 Subject: [PATCH 4/5] Update pkg/store/bucket.go Co-authored-by: Saswata Mukherjee Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- pkg/store/bucket.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/store/bucket.go b/pkg/store/bucket.go index bba434727a..876f8220c6 100644 --- a/pkg/store/bucket.go +++ b/pkg/store/bucket.go @@ -1766,13 +1766,11 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR s.mtx.RUnlock() if err := g.Wait(); err != nil { - if statusErr, ok := status.FromError(err); ok { - if statusErr.Code() == codes.ResourceExhausted { - return nil, status.Error(codes.ResourceExhausted, err.Error()) - } + code := codes.Internal + if s, ok := status.FromError(errors.Cause(err)); ok { + code = s.Code() } - - return nil, status.Error(codes.Aborted, err.Error()) + return nil, status.Error(code, err.Error()) } anyHints, err := types.MarshalAny(resHints) From 39492d8dfd4f56fcb9e12bf89983587f8167c21d Mon Sep 17 00:00:00 2001 From: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> Date: Thu, 4 May 2023 09:19:24 +0200 Subject: [PATCH 5/5] rename labelValues serires liimiter test function Signed-off-by: Thibault Mange <22740367+thibaultmg@users.noreply.github.com> --- pkg/store/bucket_e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/store/bucket_e2e_test.go b/pkg/store/bucket_e2e_test.go index a229d89600..1847e336b7 100644 --- a/pkg/store/bucket_e2e_test.go +++ b/pkg/store/bucket_e2e_test.go @@ -919,7 +919,7 @@ func TestBucketStore_LabelValues_e2e(t *testing.T) { }) } -func TestBucketStore_LabelValues_ChunksLimiter_e2e(t *testing.T) { +func TestBucketStore_LabelValues_SeriesLimiter_e2e(t *testing.T) { cases := map[string]struct { maxSeriesLimit uint64 expectedErr string