Follow up for #3895 - returning 422 (instead of 500) when query hits `max_chunks_per_query` limit with block storage #3937

alanprot · 2021-03-11T00:31:49Z

What this PR does:
Following up PR #3895

There were still some cases where Cortex was returning 5XX (instead of 422) when a user sends queries that hit max_chunks_per_query limit.

Specifically, this limit is also checked inside Thanos and returned as an generic error here

This PR updates Thanos as it needs this change to work.

Basically a custom limiter was created to return an error with a specific GRPC code when those limits are hit, and thanos will take care of propagating this error code.

Its important to note that when updating thanos we brought this change that added a parameter on some methods.
The value metadata.NoneFunc as used on this extra parameter. (I don't know if cortex want to enable this hash check in the future)

Ex: b5b572c#diff-adc81f6ffba52bc1871b0dac7c88237aa1cdea40e7691e35e834b40129ab723fR56

Which issue(s) this PR fixes:
Fixes #3895

Checklist

Tests updated
[NA] Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

Thanks for working on this! The changes LGTM, including Thanos, but we should investigate why the compactor cleanup fails.

CHANGELOG.md

pracucci · 2021-03-22T16:29:19Z

pkg/compactor/compactor_test.go

@@ -1019,7 +1019,7 @@ func removeIgnoredLogs(input []string) []string {

 	for i := 0; i < len(input); i++ {
 		log := input[i]
-		if strings.Contains(log, "block.MetaFetcher") || strings.Contains(log, "block.BaseFetcher") {
+		if strings.Contains(log, "block.MetaFetcher") || strings.Contains(log, "block.BaseFetcher") || strings.Contains(log, "failed deleting non-compaction group directories/files, some disk space usage might have leaked") {


This doesn't sound correct. We shouldn't filter out error logs. If there's an issue we should fix it. Could you investigate, please?

Suggestion: try to upgrade to Thanos master again. The compactor local filesystem cleanup is something still under work.

Hi @pracucci.

I did upgrade Thanos and the problem persists.

Investigating this issue seems that the problem is cause during the first compaction the compactDir is not yet created and then we get an error - no such file or directory when trying to delete on this line.

We can see that the folder is immediately created when the first compaction run from here, here and here

I think its fine to ignore this new log line there or we should try to do a change on thanos to not log that warn in case of a ENOENT. Thoughts?

BTW, the log level of this message is Warn not Error.

This looks harmless to me. Changing thanos code to not generate error if dir doesn't exist would be another way of fixing this.

I disagree on not fixing it (and ignoring the log) because of a couple of reasons:

If a Cortex user sees this warning, it may be "warned" even if it's a non issue

If in the future other issues cause this deletion to fail, we'll not notice it if we ignore such log

I agree it's "not a big deal" right now, but given the fix looks trivial we could just go ahead and fix it:
thanos-io/thanos#3974

Thanks!!! And thanks for following up with thanos! :)

thanos-io/thanos#3974 has been merged so if you upgrade Thanos again (which would be required anyway because of fixes done in the meanwhile) you can then remove the change from removeIgnoredLogs(). Thanks!

pracucci · 2021-03-22T16:31:13Z

pkg/storegateway/bucket_stores.go

+type ChunkLimiter struct {
+	limiter *store.Limiter
+}
+
+func (c *ChunkLimiter) Reserve(num uint64) error {
+	err := c.limiter.Reserve(num)
+	if err != nil {
+		return httpgrpc.Errorf(http.StatusUnprocessableEntity, err.Error())
+	}
+
+	return nil
+}


A couple of nits:

I would move it at the end of the file, to keep NewBucketStores() close to its struct definition

I think it's not required to export ChunkLimiter so I would rename it to chunkLimiter

@alanprot could you address this comment, please?

Done! :D Thanks again!! :D

pstibrany · 2021-03-25T09:28:20Z

Overall, this looks good to me (after fixing Marco's suggestions). Please fix DCO and rebase to fix CHANGELOG conflict before we can merge.

pracucci · 2021-04-01T08:44:36Z

We need to upgrade Thanos again, because of some fixes done in the meanwhile (see thanos-io/thanos#3978). Upgrading to current main should be fine (I just checked it).

Please also rebase master before upgrading Thanos cause we did some changes to go.mod which may have an impact to the updated dependencies.

pracucci · 2021-04-01T15:41:49Z

Please also rebase master before upgrading Thanos cause we did some changes to go.mod which may have an impact to the updated dependencies.

@alanprot to keep the Thanos review easier, could you update Thanos at the commit of when thanos-io/thanos#3974 has been merged? Other PRs have been merged after that and I would prefer reviewing them in a separate PR.

Signed-off-by: Alan Protasio <[email protected]>

alanprot · 2021-04-05T05:57:03Z

Thanks @pracucci ..
I just updated the PR

pracucci

Very good, thanks! Sorry for my very late reply :( I double checked Thanos changes and LGTM as well.

pull-request-size bot added the size/XL label Mar 11, 2021

alanprot force-pushed the feature/limits-err-code-2 branch from d483c4d to 906e883 Compare March 11, 2021 00:37

tomwilkie requested a review from pracucci March 18, 2021 16:16

pracucci reviewed Mar 22, 2021

View reviewed changes

alanprot force-pushed the feature/limits-err-code-2 branch 4 times, most recently from 8979f8b to b9035fa Compare March 25, 2021 21:45

alanprot force-pushed the feature/limits-err-code-2 branch from 0b8cba1 to c313cb9 Compare April 5, 2021 04:41

pull-request-size bot added size/XXL and removed size/XL labels Apr 5, 2021

alanprot force-pushed the feature/limits-err-code-2 branch 2 times, most recently from 18f8fef to c038351 Compare April 5, 2021 05:11

alanprot added 4 commits April 4, 2021 22:19

Updating thanos

b76d084

Signed-off-by: Alan Protasio <[email protected]>

Returning 422 when chunk limits are reached

5d7142c

Signed-off-by: Alan Protasio <[email protected]>

Addresing PR comments

5d5d0c9

Signed-off-by: Alan Protasio <[email protected]>

Updating Changelog

20fefb0

Signed-off-by: Alan Protasio <[email protected]>

alanprot force-pushed the feature/limits-err-code-2 branch from c038351 to 20fefb0 Compare April 5, 2021 05:19

pracucci approved these changes Apr 8, 2021

View reviewed changes

pstibrany approved these changes Apr 8, 2021

View reviewed changes

pracucci merged commit 02bcf27 into cortexproject:master Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up for #3895 - returning 422 (instead of 500) when query hits `max_chunks_per_query` limit with block storage #3937

Follow up for #3895 - returning 422 (instead of 500) when query hits `max_chunks_per_query` limit with block storage #3937

alanprot commented Mar 11, 2021 •

edited

Loading

pracucci left a comment

pracucci Mar 22, 2021

alanprot Mar 23, 2021 •

edited

Loading

pstibrany Mar 25, 2021

pracucci Mar 25, 2021

alanprot Mar 25, 2021

pracucci Apr 1, 2021

pracucci Mar 22, 2021

pracucci Mar 25, 2021

alanprot Mar 25, 2021

pstibrany commented Mar 25, 2021

pracucci commented Apr 1, 2021

pracucci commented Apr 1, 2021

alanprot commented Apr 5, 2021

pracucci left a comment

Follow up for #3895 - returning 422 (instead of 500) when query hits max_chunks_per_query limit with block storage #3937

Follow up for #3895 - returning 422 (instead of 500) when query hits max_chunks_per_query limit with block storage #3937

Conversation

alanprot commented Mar 11, 2021 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanprot Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany commented Mar 25, 2021

pracucci commented Apr 1, 2021

pracucci commented Apr 1, 2021

alanprot commented Apr 5, 2021

pracucci left a comment

Choose a reason for hiding this comment

Follow up for #3895 - returning 422 (instead of 500) when query hits `max_chunks_per_query` limit with block storage #3937

Follow up for #3895 - returning 422 (instead of 500) when query hits `max_chunks_per_query` limit with block storage #3937

alanprot commented Mar 11, 2021 •

edited

Loading

alanprot Mar 23, 2021 •

edited

Loading