From 1200c89d0c34cea04b50913890fdc2dab5c67462 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Tue, 28 Nov 2023 14:43:35 +0100 Subject: [PATCH] Fix tsdb.stripeSeries.gc so it handles conflicts properly (#13195) * Fix tsdb.stripeSeries.gc so it handles conflicts properly tsdb.stripeSeries.gc needs to prune seriesHashmap.conflicts first, otherwise seriesHashmap replaces the unique field with the first among the conflicts. Also add regression test. Signed-off-by: Arve Knudsen * TestStripeSeries_gc: Support stringlabels, don't use internals Signed-off-by: Arve Knudsen --------- Signed-off-by: Arve Knudsen --- tsdb/head.go | 8 +++++--- tsdb/head_test.go | 33 ++++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/tsdb/head.go b/tsdb/head.go index 25209cd01..9d9ada5cc 100644 --- a/tsdb/head.go +++ b/tsdb/head.go @@ -1886,14 +1886,16 @@ func (s *stripeSeries) gc(mint int64, minOOOMmapRef chunks.ChunkDiskMapperRef) ( deletedForCallback := make(map[chunks.HeadSeriesRef]labels.Labels, deletedFromPrevStripe) s.locks[i].Lock() - for hash, series := range s.hashes[i].unique { - check(i, hash, series, deletedForCallback) - } + // Delete conflicts first so seriesHashmap.del doesn't move them to the `unique` field, + // after deleting `unique`. for hash, all := range s.hashes[i].conflicts { for _, series := range all { check(i, hash, series, deletedForCallback) } } + for hash, series := range s.hashes[i].unique { + check(i, hash, series, deletedForCallback) + } s.locks[i].Unlock() diff --git a/tsdb/head_test.go b/tsdb/head_test.go index 7adfab1c9..d1e0b0d19 100644 --- a/tsdb/head_test.go +++ b/tsdb/head_test.go @@ -5563,7 +5563,10 @@ func labelsWithHashCollision() (labels.Labels, labels.Labels) { return ls1, ls2 } -func TestStripeSeries_getOrSet(t *testing.T) { +// stripeSeriesWithCollidingSeries returns a stripeSeries with two memSeries having the same, colliding, hash. +func stripeSeriesWithCollidingSeries(t *testing.T) (*stripeSeries, *memSeries, *memSeries) { + t.Helper() + lbls1, lbls2 := labelsWithHashCollision() ms1 := memSeries{ lset: lbls1, @@ -5589,9 +5592,29 @@ func TestStripeSeries_getOrSet(t *testing.T) { require.True(t, created) require.Same(t, &ms2, got) + return s, &ms1, &ms2 +} + +func TestStripeSeries_getOrSet(t *testing.T) { + s, ms1, ms2 := stripeSeriesWithCollidingSeries(t) + hash := ms1.lset.Hash() + // Verify that we can get both of the series despite the hash collision - got = s.getByHash(hash, lbls1) - require.Same(t, &ms1, got) - got = s.getByHash(hash, lbls2) - require.Same(t, &ms2, got) + got := s.getByHash(hash, ms1.lset) + require.Same(t, ms1, got) + got = s.getByHash(hash, ms2.lset) + require.Same(t, ms2, got) +} + +func TestStripeSeries_gc(t *testing.T) { + s, ms1, ms2 := stripeSeriesWithCollidingSeries(t) + hash := ms1.lset.Hash() + + s.gc(0, 0) + + // Verify that we can get neither ms1 nor ms2 after gc-ing corresponding series + got := s.getByHash(hash, ms1.lset) + require.Nil(t, got) + got = s.getByHash(hash, ms2.lset) + require.Nil(t, got) }