Fixes an issue in the index chunks/series intersect code. #2796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was introduce in cortexproject#2700, more specifically this line https://github.com/cortexproject/cortex/pull/2700/files#diff-10bca0f4f31a2ca1edc507d0289b143dR537 This causes any query with the first label matcher not matching anything to return all matches of all other labels. This is a nasty one since, the code was relying on empty slice, and so it would skip nil values instead of returning no matches. I've added a regression test proving this is fixed everywhere. I think in cortex it can probably affect performance (since you have to download all chunk not required) but not read integrity. I have found this with @slim-bean while deploying Loki, all queriers where OOMing because of this. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
pstibrany
approved these changes
Jun 26, 2020
pracucci
approved these changes
Jun 26, 2020
There was a problem hiding this comment.
Sorry @cyriltovena for the troubles my change has caused and thanks for the fix!
|
Let's merge this fix quicky and then @bboreham can cherry-pick it into the release-1.2. |
bboreham
pushed a commit
that referenced
this pull request
Jun 26, 2020
* Fixes an issue in the index chunks/series intersect code. This was introduce in #2700, more specifically this line https://github.com/cortexproject/cortex/pull/2700/files#diff-10bca0f4f31a2ca1edc507d0289b143dR537 This causes any query with the first label matcher not matching anything to return all matches of all other labels. This is a nasty one since, the code was relying on empty slice, and so it would skip nil values instead of returning no matches. I've added a regression test proving this is fixed everywhere. I think in cortex it can probably affect performance (since you have to download all chunk not required) but not read integrity. I have found this with @slim-bean while deploying Loki, all queriers where OOMing because of this. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com> * Update changelog. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
bboreham
added a commit
that referenced
this pull request
Jun 26, 2020
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was introduce in #2700, more specifically this line https://github.com/cortexproject/cortex/pull/2700/files#diff-10bca0f4f31a2ca1edc507d0289b143dR537
This causes any query with the first label matcher not matching anything to return all matches of all other labels.
This is a nasty one since, the code was relying on empty slice, and so it would skip nil values instead of returning no matches. I've added a regression test proving this is fixed everywhere. I think in Cortex it probably affect performance (since you have to download all chunks not required) but not read integrity.
I have found this with @slim-bean while deploying Loki, we fought a lot to pin down this exact issue while all queriers where OOMing.
/cc @gouthamve @pracucci @pstibrany We need to put on hold all current releases this is a dangerous bug.
/cc @rfratto @slim-bean Let's wait for this to be merged before applying the latest release of Loki.
Signed-off-by: Cyril Tovena cyril.tovena@gmail.com
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]