querier: Dedup series is now replica label agnostic and simpler. Fixe…

…d panic seen when using larger number of replicas and small series. Fixes #2645 Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
thanos-io · Jun 10, 2020 · 3daea58 · 3daea58
1 parent 6f2c3b1
commit 3daea58
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 595 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 - [#2637](https://github.com/thanos-io/thanos/pull/2637) Compact: detect retryable errors that are inside of a wrapped `tsdb.MultiError`
 - [#2648](https://github.com/thanos-io/thanos/pull/2648) Store: allow index cache and caching bucket to be configured at the same time.
 - [#2705](https://github.com/thanos-io/thanos/pull/2705) minio-go: Added support for `af-south-1` and `eu-south-1` regions.
+- [#2728](https://github.com/thanos-io/thanos/pull/2728) Query: Fixed panics when using larger number of replica labels with short series label sets.
 
 ### Changed
 

diff --git a/pkg/query/iter.go b/pkg/query/iter.go
@@ -381,8 +381,12 @@ func (s *dedupSeriesSet) peekLset() labels.Labels {
 	}
 	// Check how many replica labels are present so that these are removed.
 	var totalToRemove int
-	for index := 0; index < len(s.replicaLabels); index++ {
-		if _, ok := s.replicaLabels[lset[len(lset)-index-1].Name]; ok {
+	for i := 0; i < len(s.replicaLabels); i++ {
+		if len(lset)-i == 0 {
+			break
+		}
+
+		if _, ok := s.replicaLabels[lset[len(lset)-i-1].Name]; ok {
 			totalToRemove++
 		}
 	}

diff --git a/pkg/query/querier.go b/pkg/query/querier.go
@@ -214,20 +214,17 @@ func (q *querier) Select(_ bool, hints *storage.SelectHints, ms ...*labels.Match
 		}, warns, nil
 	}
 
-	// TODO(fabxc): this could potentially pushed further down into the store API
-	// to make true streaming possible.
+	// TODO(fabxc): this could potentially pushed further down into the store API to make true streaming possible.
 	sortDedupLabels(resp.seriesSet, q.replicaLabels)
-
 	set := &promSeriesSet{
 		mint:  q.mint,
 		maxt:  q.maxt,
 		set:   newStoreSeriesSet(resp.seriesSet),
 		aggrs: aggrs,
 	}
 
-	// The merged series set assembles all potentially-overlapping time ranges
-	// of the same series into a single one. The series are ordered so that equal series
-	// from different replicas are sequential. We can now deduplicate those.
+	// The merged series set assembles all potentially-overlapping time ranges of the same series into a single one.
+	// TODO(bwplotka): We could potentially dedup on chunk level, use chunk iterator for that when available.
 	return newDedupSeriesSet(set, q.replicaLabels, len(aggrs) == 1 && aggrs[0] == storepb.Aggr_COUNTER), warns, nil
 }
 

diff --git a/pkg/query/querier_test.go b/pkg/query/querier_test.go
@@ -800,123 +800,36 @@ func TestSortReplicaLabel(t *testing.T) {
 		// 0 Single deduplication label.
 		{
 			input: []storepb.Series{
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "c", Value: "3"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "c", Value: "3"},
-					{Name: "d", Value: "4"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "c", Value: "4"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-2"},
-					{Name: "c", Value: "3"},
-				}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "3"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "c", Value: "4"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "c", Value: "3"}}},
 			},
 			exp: []storepb.Series{
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "b", Value: "replica-1"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "b", Value: "replica-2"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "d", Value: "4"},
-					{Name: "b", Value: "replica-1"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "4"},
-					{Name: "b", Value: "replica-1"},
-				}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-1"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}, {Name: "b", Value: "replica-1"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "4"}, {Name: "b", Value: "replica-1"}}},
 			},
 			dedupLabels: map[string]struct{}{"b": {}},
 		},
 		// 1 Multi deduplication labels.
 		{
 			input: []storepb.Series{
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "b1", Value: "replica-1"},
-					{Name: "c", Value: "3"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "b1", Value: "replica-1"},
-					{Name: "c", Value: "3"},
-					{Name: "d", Value: "4"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "b1", Value: "replica-1"},
-					{Name: "c", Value: "4"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-2"},
-					{Name: "b1", Value: "replica-2"},
-					{Name: "c", Value: "3"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "b", Value: "replica-2"},
-					{Name: "c", Value: "3"},
-				}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "3"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}, {Name: "c", Value: "4"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "b1", Value: "replica-2"}, {Name: "c", Value: "3"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "b", Value: "replica-2"}, {Name: "c", Value: "3"}}},
 			},
 			exp: []storepb.Series{
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "b1", Value: "replica-1"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "b", Value: "replica-2"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "b", Value: "replica-2"},
-					{Name: "b1", Value: "replica-2"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "3"},
-					{Name: "d", Value: "4"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "b1", Value: "replica-1"},
-				}},
-				{Labels: []storepb.Label{
-					{Name: "a", Value: "1"},
-					{Name: "c", Value: "4"},
-					{Name: "b", Value: "replica-1"},
-					{Name: "b1", Value: "replica-1"},
-				}},
-			},
-			dedupLabels: map[string]struct{}{
-				"b":  {},
-				"b1": {},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "b", Value: "replica-2"}, {Name: "b1", Value: "replica-2"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
+				{Labels: []storepb.Label{{Name: "a", Value: "1"}, {Name: "c", Value: "4"}, {Name: "b", Value: "replica-1"}, {Name: "b1", Value: "replica-1"}}},
 			},
+			dedupLabels: map[string]struct{}{"b": {}, "b1": {}},
 		},
 	}
 	for _, test := range tests {
@@ -1007,6 +920,36 @@ func TestDedupSeriesSet(t *testing.T) {
 				"replica": {},
 			},
 		},
+		{
+			// Regression tests against: https://github.com/thanos-io/thanos/issues/2645
+			// We were panicking on requests with more replica labels than overall labels in any series.
+			input: []series{
+				{
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-1"}},
+					samples: []sample{{10000, 1}, {20000, 2}},
+				}, {
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-2"}},
+					samples: []sample{{60000, 3}, {70000, 4}},
+				}, {
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "replica", Value: "replica-3"}},
+					samples: []sample{{100000, 10}, {150000, 20}},
+				}, {
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}},
+					samples: []sample{{10000, 1}, {20000, 2}},
+				},
+			},
+			exp: []series{
+				{
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}},
+					samples: []sample{{10000, 1}, {20000, 2}, {60000, 3}, {70000, 4}, {100000, 10}, {150000, 20}},
+				},
+				{
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}, {Name: "d", Value: "4"}},
+					samples: []sample{{10000, 1}, {20000, 2}},
+				},
+			},
+			dedupLabels: map[string]struct{}{"replica": {}, "replica2": {}, "replica3": {}, "replica4": {}, "replica5": {}, "replica6": {}, "replica7": {}},
+		},
 		{
 			// Multi dedup label.
 			input: []series{
@@ -1131,7 +1074,7 @@ func TestDedupSeriesSet(t *testing.T) {
 			},
 		},
 		{
-			// Same thing but not for counter should not adjust antything.
+			// Same thing but not for counter should not adjust anything.
 			isCounter: false,
 			input: []series{
 				{
@@ -1223,6 +1166,27 @@ func TestDedupSeriesSet(t *testing.T) {
 			},
 			dedupLabels: map[string]struct{}{"replica": {}},
 		},
+		{
+			// Regression tests against: https://github.com/thanos-io/thanos/issues/2645
+			input: []series{
+				{
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}},
+					samples: []sample{{10000, 1}, {20000, 2}},
+				}, {
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}},
+					samples: []sample{{60000, 3}, {70000, 4}},
+				},
+			},
+			exp: []series{
+				{
+					lset:    labels.Labels{{Name: "a", Value: "1"}, {Name: "c", Value: "3"}},
+					samples: []sample{{10000, 1}, {20000, 2}, {60000, 3}, {70000, 4}},
+				},
+			},
+			dedupLabels: map[string]struct{}{
+				"replica": {},
+			},
+		},
 	}
 
 	for _, tcase := range tests {