Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expanded Postings Cache can cache results without the nearly created series under high load. #6417

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

alanprot
Copy link
Member

@alanprot alanprot commented Dec 11, 2024

What this PR does:

#6296 introduced an experimental flag to cache expanded postings, including postings from the head blocks. In that PR, the TSDB PostCreation hook is used to "expire" cache keys when new metrics are added for a specific metric name.

The issue happens under scenarios of high concurrency, where a key may be expired before the new series are available for querying, leading to cached results that exclude the new series.

The root cause is that the PostCreation hook is invoked after the series is created but before it is added to memPostings:

Relevant code:

Moving the hook invocation to occur after line 1732 resolves the issue.

This PR currently only includes a test that demonstrates the problem.

This PR implements a workaround to expire the series after the "commit". This workaround can be removed if the the PR on prometheus is accepted.

cc @GiedriusS

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@alanprot alanprot changed the title Head ExpandedPostingsCache: Querying and adding series concurrently can ca… Expanded Postings Cache can cache results without the nearly created series under high load. Dec 11, 2024
@alanprot
Copy link
Member Author

alanprot commented Dec 11, 2024

Seems that now the test is failing due: prometheus/prometheus#15141

WARNING: DATA RACE
Write at 0x00c01c8b0dc0 by goroutine 41866:
  github.com/prometheus/prometheus/tsdb/index.(*MemPostings).addFor()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/index/postings.go:368 +0x30a
  github.com/prometheus/prometheus/tsdb/index.(*MemPostings).Add.func1()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/index/postings.go:341 +0xfb
  github.com/prometheus/prometheus/model/labels.Labels.Range()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/model/labels/labels.go:332 +0xec
  github.com/prometheus/prometheus/tsdb/index.(*MemPostings).Add()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/index/postings.go:340 +0x3f
  github.com/prometheus/prometheus/tsdb.(*Head).getOrCreateWithID()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/head.go:1721 +0x204
  github.com/prometheus/prometheus/tsdb.(*Head).getOrCreate()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/head.go:1699 +0xe4
  github.com/prometheus/prometheus/tsdb.(*headAppender).getOrCreate()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/head_append.go:438 +0x4ab
  github.com/prometheus/prometheus/tsdb.(*headAppender).Append()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/head_append.go:334 +0x13b
  github.com/prometheus/prometheus/tsdb.(*dbAppender).Append()
      <autogenerated>:1 +0xa1
  github.com/cortexproject/cortex/pkg/ingester.(*Ingester).Push()
      /__w/cortex/cortex/pkg/ingester/ingester.go:1237 +0x350c
  github.com/cortexproject/cortex/pkg/ingester.TestExpandedCachePostings_Race.func2()
      /__w/cortex/cortex/pkg/ingester/ingester_test.go:5304 +0x305

Previous read at 0x00c01c8b0dc0 by goroutine 418[65](https://github.com/cortexproject/cortex/actions/runs/12284077424/job/34279028516?pr=6417#step:6:66):
  github.com/prometheus/prometheus/tsdb/index.(*ListPostings).Next()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/index/postings.go:740 +0x70
  github.com/prometheus/prometheus/tsdb/index.ExpandPostings()
      /__w/cortex/cortex/vendor/github.com/prometheus/prometheus/tsdb/index/postings.go:441 +0x89
  github.com/cortexproject/cortex/pkg/storage/tsdb.(*blocksPostingsForMatchersCache).fetchPostings.func3()
      /__w/cortex/cortex/pkg/storage/tsdb/expanded_postings_cache.go:204 +0xf1
  github.com/cortexproject/cortex/pkg/storage/tsdb.(*fifoCache[go.shape.[]github.com/prometheus/prometheus/storage.SeriesRef]).getPromiseForKey()
      /__w/cortex/cortex/pkg/storage/tsdb/expanded_postings_cache.go:376 +0x2ae
  github.com/cortexproject/cortex/pkg/storage/tsdb.(*blocksPostingsForMatchersCache).fetchPostings()
      /__w/cortex/cortex/pkg/storage/tsdb/expanded_postings_cache.go:212 +0x9d0
  github.com/cortexproject/cortex/pkg/storage/tsdb.(*blocksPostingsForMatchersCache).PostingsForMatchers()
      /__w/cortex/cortex/pkg/storage/tsdb/expanded_postings_cache.go:165 +0x99
  github.com/cortexproject/cortex/pkg/storage/tsdb.selectChunkSeriesSet()
      /__w/cortex/cortex/pkg/storage/tsdb/cached_chunks_querier.go:117 +0x198
  github.com/cortexproject/cortex/pkg/storage/tsdb.(*cachedBlockChunkQuerier).Select()
      /__w/cortex/cortex/pkg/storage/tsdb/cached_chunks_querier.go:102 +0x2f0
  github.com/cortexproject/cortex/pkg/ingester.(*Ingester).queryStreamChunks()
      /__w/cortex/cortex/pkg/ingester/ingester.go:2039 +0x2cd
  github.com/cortexproject/cortex/pkg/ingester.(*Ingester).QueryStream()
      /__w/cortex/cortex/pkg/ingester/ingester.go:1990 +0x3b3
  github.com/cortexproject/cortex/pkg/ingester.TestExpandedCachePostings_Race.func3()
      /__w/cortex/cortex/pkg/ingester/ingester_test.go:5312 +0x2d7

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 11, 2024
@alanprot alanprot force-pushed the expanded-postings-race branch from b167b32 to 59579cf Compare December 11, 2024 21:04
…che wrong results

Signed-off-by: alanprot <alanprot@gmail.com>
Signed-off-by: alanprot <alanprot@gmail.com>
@alanprot alanprot force-pushed the expanded-postings-race branch from 59579cf to 0606da3 Compare December 11, 2024 21:52
@alanprot
Copy link
Member Author

I created a new GH workflow steps to run the test without the -race flag and to run the TestExpandedCachePostings_Race test without it.

I needed to do that as there is a known race on prometheus that this test were also catching.

Signed-off-by: alanprot <alanprot@gmail.com>
@alanprot alanprot force-pushed the expanded-postings-race branch from 0606da3 to 1cbcc8b Compare December 11, 2024 22:04
@alanprot alanprot requested a review from yeya24 December 11, 2024 22:22
@alanprot alanprot merged commit 75525a9 into cortexproject:master Dec 12, 2024
17 checks passed
@alanprot alanprot deleted the expanded-postings-race branch December 12, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants