Skip to content

Commit

Permalink
Fixes an issue in the index chunks/series intersect code. (grafana#2796)
Browse files Browse the repository at this point in the history
* Fixes an issue in the index chunks/series intersect code.

This was introduce in grafana#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>
  • Loading branch information
cyriltovena authored and bboreham committed Jun 26, 2020
1 parent 12b8b2f commit 107010f
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 2 deletions.
4 changes: 3 additions & 1 deletion chunk_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,11 +424,13 @@ func (c *store) lookupChunksByMetricName(ctx context.Context, userID string, fro
// Receive chunkSets from all matchers
var chunkIDs []string
var lastErr error
var initialized bool
for i := 0; i < len(matchers); i++ {
select {
case incoming := <-incomingChunkIDs:
if chunkIDs == nil {
if !initialized {
chunkIDs = incoming
initialized = true
} else {
chunkIDs = intersectStrings(chunkIDs, incoming)
}
Expand Down
4 changes: 4 additions & 0 deletions chunk_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,10 @@ func TestChunkStore_Get(t *testing.T) {
query: `foo{toms="code", bar="baz"}`,
expect: []Chunk{fooChunk1},
},
{
query: `foo{a="b", bar="baz"}`,
expect: nil,
},
{
query: `{__name__=~"foo"}`,
err: "query must contain metric name",
Expand Down
4 changes: 3 additions & 1 deletion series_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,12 +305,14 @@ func (c *seriesStore) lookupSeriesByMetricNameMatchers(ctx context.Context, from
var lastErr error
var cardinalityExceededErrors int
var cardinalityExceededError CardinalityExceededError
var initialized bool
for i := 0; i < len(matchers); i++ {
select {
case incoming := <-incomingIDs:
preIntersectionCount += len(incoming)
if ids == nil {
if !initialized {
ids = incoming
initialized = true
} else {
ids = intersectStrings(ids, incoming)
}
Expand Down

0 comments on commit 107010f

Please sign in to comment.