Fix issue where queries can fail or omit OOO samples if OOO head compaction occurs between creating a querier and reading chunks

[charleskorn]

This PR fixes two related issues:

• queries can fail with an cannot populate chunk XXX from block 2ZBXFNYVVFDXFPGSB1CHFNYQTZ: not found error if OOO head compaction occurs while the query is being evaluated
• queries can omit data from the OOO head if OOO head compaction occurs while the query is being evaluated

cannot populate chunk XXX from block 2ZBXFNYVVFDXFPGSB1CHFNYQTZ: not found error

This issue occurs if events happen in this sequence:

1. Call DB.ChunksQuerier(), which returns a querier q
2. Call q.Select(), which returns a series set ss
3. Retrieve an iterator it for a series with samples in the OOO head block (ie. call ss.At().Iterator(...))
4. Run OOO head compaction
5. Use it to read the chunks for the series

Eventually, it.Next() will return false, and it.Err() will report a cannot populate chunk XXX from block 2ZBXFNYVVFDXFPGSB1CHFNYQTZ: not found error.

2ZBXFNYVVFDXFPGSB1CHFNYQTZ is the ULID of the OOO head block, defined here.

This happens because the garbage collection that occurs during OOO head compaction does not wait for pending reads to complete, and so the iterator created in step 3 has a reference to a chunk that is removed during compaction and can't be read in step 5.

Omitting data

This issue occurs if events happen in this sequence:

1. Call DB.ChunksQuerier(), which returns a querier q
2. Call q.Select(), which returns a series set ss
3. Run OOO head compaction
4. Retrieve an iterator it for a series with samples that were in the OOO head before compaction (ie. call ss.At().Iterator(...))
5. Use it to read the chunks for the series, none of which will include the samples that were in the OOO head before compaction

It also happens if steps 2 and 3 happen in the reverse order (ie. head compaction happens before the call to q.Select()).

In this case, the data that was in the OOO head before compaction is omitted, and no error is returned, so query results are incorrect.

This happens because the list of blocks on disk is captured in step 1, but by the time we get to step 4, which evaluates the list of chunks for the series, the chunk that was in the OOO head has been written to disk and garbage collected from the head, so it isn't returned from either source (disk or head).

Root cause

I believe the reason for this is that OOO compaction doesn't check for pending reads that overlap with the chunks to be garbage collected, unlike what is done for in-order head compaction here.

Under normal circumstances (ie. when DB.Compact() is used), OOO head compaction only occurs after in-order head compaction. Because of this, most of the time, this issue won't happen in practice because the time range of a query that includes OOO samples will also include time range of the in-order head and therefore be protected by the pending read check done before compacting the in-order head, or be protected by another query running at the same time that includes the in-order head. However, queries that are only for the time range of blocks on disk and OOO head samples (ie. do not overlap with the time range of the in-order head) would be susceptible to this issue.

---

I'd suggest reviewing each commit individually, the key commits are:

• 8898a56 and 07c443f add tests that demonstrates the issue
• cc23822 adds a naive solution that uses the query range and chunk time range to block OOO garbage collection running while queries touching chunks that will be GCed are in progress, but this runs the risk of never allowing garbage collection to proceed
• e3d12ed improves on the previous commit by using the last compacted OOO chunk reference instead to block garbage collection

I'd suggest reviewing this carefully: while this fix makes the test pass, I suspect there are some edge cases I haven't considered.

[jesusvazquez]

2ZBXFNYVVFDXFPGSB1CHFNYQTZ is the ULID of the OOO head block, defined here.

A minor comment, recently @colega has made the head ULIDs readable so they are easy to differentiate form actual blocks #13100 This should bring a bit of clarity in the future if more issues of this kind arise 🙏

[jesusvazquez]

Thank you for your contribution @charleskorn

I clarified in a comment above that ULIDs for the head blocks should now be more readable, hopefully providing a better UX to the administrator when this kind of bugs come up. I bet it was hard to find this...

I've given a first pass of feedback with some small nits, I'm fairly confident that this will work since your tests are comprehensive and nothing else broke.

Unfortunately our prombench does not test the OOO head so we wont be able to catch any possible issues on CI, we won't know if something breaks until this runs in prod for some time, this scares me a bit. I'll make sure whomever is crafting the next release is aware about this PR in case issues comes up we know what to revert.

I've asked @fionaliao to also review the changes since she is working on OOO compaction and her feedback might be useful.

While you rebase + address my comments I'll read a bit more on isolation just to make sure I did not miss anything.

[jesusvazquez]

/prombench main

[prombot]

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-13115 and main

After successful deployment, the benchmarking results can be viewed at:

• Prometheus Meta
• Prombench Dashboard
• Grafana Explorer, Loki logs
• Parca profiles (e.g. in-use memory)

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

[jesusvazquez]

@charleskorn there is a notable difference in memory usage in the benchmark

But I have seen the same behavior in a benchmark I'm running for another PR.

Could you please rebase your branch with the latest changes in main so I can run a benchmark that is more up to date? We need to check for regressions because this PR is changing a few things.

[jesusvazquez]

/prombench cancel

[prombot]

Benchmark cancel is in progress.

[charleskorn]

@charleskorn there is a notable difference in memory usage in the benchmark

...

But I have seen the same behavior in a benchmark I'm running for another PR.

Could you please rebase your branch with the latest changes in main so I can run a benchmark that is more up to date? We need to check for regressions because this PR is changing a few things.

Done, just rebased it now.

[charleskorn]

Looks like the CI failure is due to linting issues affecting unrelated files. #13178 will fix the linting issues.

[jesusvazquez]

/prombench main

[prombot]

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-13115 and main

After successful deployment, the benchmarking results can be viewed at:

• Prometheus Meta
• Prombench Dashboard
• Grafana Explorer, Loki logs
• Parca profiles (e.g. in-use memory)

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

[jesusvazquez]

/prombench cancel

[prombot]

Benchmark cancel is in progress.

[jesusvazquez]

Benchmarks results looking good 🎉

[jesusvazquez]

LGTM, thanks @charleskorn for addressing this issue and improving the code in between.

[bboreham]

Do we need a similar Close in blockChunkQuerierForRange? (Line 2030 in this PR)

[charleskorn]

Yes we do, good catch. Fixed in #14341.