From 73684020545d78a5d8419c8aa510d1a15848ea64 Mon Sep 17 00:00:00 2001 From: Jesus Vazquez Date: Wed, 10 Aug 2022 17:33:36 +0200 Subject: [PATCH] Fix OOO Head LabelValues and PostingsForMatchers OOOHeadIndexReader was using the headIndexReader PostingsForMatchers() and LabelValues() implementation which lead to a very subtle bug that led to wrong query results. headIndexReader LabelValues() implementation checks if the query timerange overlaps with the head maxt and mint and if it doesnt it returns an empty list of values. Since this code was also used by the ooo head it led to wrong results that we were not able to see in tests because our queries where always from MinInt64 to MaxInt64. This commit also adds a new test that performs multiple time range queries to make sure this never happens again. Signed-off-by: Jesus Vazquez --- tsdb/db_test.go | 44 +++++++++++++++++++++++++++++-------------- tsdb/ooo_head_read.go | 21 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/tsdb/db_test.go b/tsdb/db_test.go index 50b4a0e1e..f1bdc7d34 100644 --- a/tsdb/db_test.go +++ b/tsdb/db_test.go @@ -4196,7 +4196,7 @@ func TestOOOAppendAndQuery(t *testing.T) { s2 := labels.FromStrings("foo", "bar2") minutes := func(m int64) int64 { return m * time.Minute.Milliseconds() } - expSamples := make(map[string][]tsdbutil.Sample) + appendedSamples := make(map[string][]tsdbutil.Sample) totalSamples := 0 addSample := func(lbls labels.Labels, fromMins, toMins int64, faceError bool) { app := db.Appender(context.Background()) @@ -4209,7 +4209,7 @@ func TestOOOAppendAndQuery(t *testing.T) { require.Error(t, err) } else { require.NoError(t, err) - expSamples[key] = append(expSamples[key], sample{t: min, v: val}) + appendedSamples[key] = append(appendedSamples[key], sample{t: min, v: val}) totalSamples++ } } @@ -4220,17 +4220,30 @@ func TestOOOAppendAndQuery(t *testing.T) { } } - testQuery := func() { - querier, err := db.Querier(context.TODO(), math.MinInt64, math.MaxInt64) + testQuery := func(from, to int64) { + querier, err := db.Querier(context.TODO(), from, to) require.NoError(t, err) seriesSet := query(t, querier, labels.MustNewMatcher(labels.MatchRegexp, "foo", "bar.")) - for k, v := range expSamples { + for k, v := range appendedSamples { sort.Slice(v, func(i, j int) bool { return v[i].T() < v[j].T() }) - expSamples[k] = v + appendedSamples[k] = v + } + + expSamples := make(map[string][]tsdbutil.Sample) + for k, samples := range appendedSamples { + for _, s := range samples { + if s.T() < from { + continue + } + if s.T() > to { + continue + } + expSamples[k] = append(expSamples[k], s) + } } require.Equal(t, expSamples, seriesSet) require.Equal(t, float64(totalSamples-2), prom_testutil.ToFloat64(db.head.metrics.outOfOrderSamplesAppended), "number of ooo appended samples mismatch") @@ -4245,40 +4258,43 @@ func TestOOOAppendAndQuery(t *testing.T) { addSample(s1, 300, 300, false) addSample(s2, 290, 290, false) require.Equal(t, float64(2), prom_testutil.ToFloat64(db.head.metrics.chunksCreated)) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) // Some ooo samples. addSample(s1, 250, 260, false) addSample(s2, 255, 265, false) verifyOOOMinMaxTimes(250, 265) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) + testQuery(minutes(250), minutes(265)) // Test querying ono data time range + testQuery(minutes(290), minutes(300)) // Test querying in-order data time range + testQuery(minutes(250), minutes(300)) // Test querying the entire range // Out of time window. addSample(s1, 59, 59, true) addSample(s2, 49, 49, true) verifyOOOMinMaxTimes(250, 265) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) // At the edge of time window, also it would be "out of bound" without the ooo support. addSample(s1, 60, 65, false) verifyOOOMinMaxTimes(60, 265) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) // This sample is not within the time window w.r.t. the head's maxt, but it is within the window // w.r.t. the series' maxt. But we consider only head's maxt. addSample(s2, 59, 59, true) verifyOOOMinMaxTimes(60, 265) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) // Now the sample is within time window w.r.t. the head's maxt. addSample(s2, 60, 65, false) verifyOOOMinMaxTimes(60, 265) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) // Out of time window again. addSample(s1, 59, 59, true) addSample(s2, 49, 49, true) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) // Generating some m-map chunks. The m-map chunks here are in such a way // that when sorted w.r.t. mint, the last chunk's maxt is not the overall maxt @@ -4287,7 +4303,7 @@ func TestOOOAppendAndQuery(t *testing.T) { addSample(s1, 180, 249, false) require.Equal(t, float64(6), prom_testutil.ToFloat64(db.head.metrics.chunksCreated)) verifyOOOMinMaxTimes(60, 265) - testQuery() + testQuery(math.MinInt64, math.MaxInt64) } func TestOOODisabled(t *testing.T) { diff --git a/tsdb/ooo_head_read.go b/tsdb/ooo_head_read.go index 2261d5915..aa1293585 100644 --- a/tsdb/ooo_head_read.go +++ b/tsdb/ooo_head_read.go @@ -134,6 +134,27 @@ func (oh *OOOHeadIndexReader) series(ref storage.SeriesRef, lbls *labels.Labels, return nil } +// PostingsForMatchers needs to be overridden so that the right IndexReader +// implementation gets passed down to the PostingsForMatchers call. +func (oh *OOOHeadIndexReader) PostingsForMatchers(concurrent bool, ms ...*labels.Matcher) (index.Postings, error) { + return oh.head.pfmc.PostingsForMatchers(oh, concurrent, ms...) +} + +// LabelValues needs to be overridden from the headIndexReader implementation due +// to the check that happens at the beginning where we make sure that the query +// interval overlaps with the head minooot and maxooot. +func (oh *OOOHeadIndexReader) LabelValues(name string, matchers ...*labels.Matcher) ([]string, error) { + if oh.maxt < oh.head.MinOOOTime() || oh.mint > oh.head.MaxOOOTime() { + return []string{}, nil + } + + if len(matchers) == 0 { + return oh.head.postings.LabelValues(name), nil + } + + return labelValuesWithMatchers(oh, name, matchers...) +} + type chunkMetaAndChunkDiskMapperRef struct { meta chunks.Meta ref chunks.ChunkDiskMapperRef