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.

Sign up for GitHub

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

Don't try to compact blocks marked for deletion. #4328

Merged
merged 2 commits into from
Aug 13, 2021
Merged

Don't try to compact blocks marked for deletion. #4328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa87261
Don't try to compact blocks marked for deletion.
pstibrany Jun 30, 2021
58621b2
Update CHANGELOG.md
pstibrany Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* [CHANGE] Prevent path traversal attack from users able to control the HTTP header `X-Scope-OrgID`. #4375 (CVE-2021-36157)
* Users only have control of the HTTP header when Cortex is not frontend by an auth proxy validating the tenant IDs
* [CHANGE] Some files and directories created by Mimir components on local disk now have stricter permissions, and are only readable by owner, but not group or others. #4394
* [CHANGE] Compactor: compactor will no longer try to compact blocks that are already marked for deletion. Previously compactor would consider blocks marked for deletion within `-compactor.deletion-delay / 2` period as eligible for compaction. #4328
* [ENHANCEMENT] Add timeout for waiting on compactor to become ACTIVE in the ring. #4262
* [ENHANCEMENT] Reduce memory used by streaming queries, particularly in ruler. #4341
* [ENHANCEMENT] Ring: allow experimental configuration of disabling of heartbeat timeouts by setting the relevant configuration value to zero. Applies to the following: #4342
Expand Down
4 changes: 2 additions & 2 deletions pkg/compactor/compactor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,11 +603,11 @@ func (c *Compactor) compactUser(ctx context.Context, userID string) error {
deduplicateBlocksFilter := block.NewDeduplicateFilter()

// While fetching blocks, we filter out blocks that were marked for deletion by using IgnoreDeletionMarkFilter.
// The delay of deleteDelay/2 is added to ensure we fetch blocks that are meant to be deleted but do not have a replacement yet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm... this comment was actually explaining the reason why we did it. It was introduced here:
https://github.com/cortexproject/cortex/pull/2335/files#diff-adc81f6ffba52bc1871b0dac7c88237aa1cdea40e7691e35e834b40129ab723f

It was imported by Thanos (where the logic is still there):
https://github.com/thanos-io/thanos/blob/e53a1f70f947fe83d4d232c9e2887911d390d03c/cmd/thanos/compact.go#L224-L227

I think we should think a bit more about this change.

Copy link
Contributor Author

@pstibrany pstibrany Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only result of using the delay here is that compactor will get to see blocks that are already marked for deletion. It will then use such blocks for grouping and planning.

If such block marked for deletion (let's call it "B") is already "included" in some other block (as determined by compactor sources in meta.json), it will be ignored anyway.

If "B" is not yet included in any other block [*], and can be compacted together with other blocks, it will be. I think that is a mistake, because it would result in deleted block to be included in a new block, effectively resurrecting it.

There may be other blocks that are "source" blocks for our block "B". Presumably, those other blocks are also marked for deletion already, since "B" contains their data now. If compactor doesn't see blocks marked for deletion, it will not see those source blocks either.

If those source blocks are not marked for deletion yet (but "B" is) and compactor doesn't see "B" but sees source blocks, it would create new compacted block (basically "new B") from source blocks. This would be unfortunate, but this is a scenario which I don't see how it could happen.

[*] Either because of scenario I've described with backfill, or very inconsistent storage, which makes new blocks invisible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more thinking, I agree with you. I also tried to look at the Thanos history to see if I could find more information about why it was done like that in the compactor and looks it was picked by the Thanos bucket store, but I can't see how the two are correlated here.

// No delay is used -- all blocks with deletion marker are ignored, and not considered for compaction.
ignoreDeletionMarkFilter := block.NewIgnoreDeletionMarkFilter(
ulogger,
bucket,
time.Duration(c.compactorCfg.DeletionDelay.Seconds()/2)*time.Second,
0,
c.compactorCfg.MetaSyncConcurrency)

fetcher, err := block.NewMetaFetcher(
Expand Down
12 changes: 4 additions & 8 deletions pkg/compactor/compactor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,10 +581,12 @@ func TestCompactor_ShouldNotCompactBlocksMarkedForDeletion(t *testing.T) {
bucketClient.MockIter("user-1/", []string{"user-1/01DTVP434PA9VFXSW2JKB3392D", "user-1/01DTW0ZCPDDNV4BV83Q2SV4QAZ"}, nil)
bucketClient.MockExists(path.Join("user-1", cortex_tsdb.TenantDeletionMarkPath), false, nil)

// Block that has just been marked for deletion. It will not be deleted just yet, and it also will not be compacted.
bucketClient.MockGet("user-1/01DTVP434PA9VFXSW2JKB3392D/meta.json", mockBlockMetaJSON("01DTVP434PA9VFXSW2JKB3392D"), nil)
bucketClient.MockGet("user-1/01DTVP434PA9VFXSW2JKB3392D/deletion-mark.json", mockDeletionMarkJSON("01DTVP434PA9VFXSW2JKB3392D", time.Now()), nil)
bucketClient.MockGet("user-1/markers/01DTVP434PA9VFXSW2JKB3392D-deletion-mark.json", mockDeletionMarkJSON("01DTVP434PA9VFXSW2JKB3392D", time.Now()), nil)

// This block will be deleted by cleaner.
bucketClient.MockGet("user-1/01DTW0ZCPDDNV4BV83Q2SV4QAZ/meta.json", mockBlockMetaJSON("01DTW0ZCPDDNV4BV83Q2SV4QAZ"), nil)
bucketClient.MockGet("user-1/01DTW0ZCPDDNV4BV83Q2SV4QAZ/deletion-mark.json", mockDeletionMarkJSON("01DTW0ZCPDDNV4BV83Q2SV4QAZ", time.Now().Add(-cfg.DeletionDelay)), nil)
bucketClient.MockGet("user-1/markers/01DTW0ZCPDDNV4BV83Q2SV4QAZ-deletion-mark.json", mockDeletionMarkJSON("01DTW0ZCPDDNV4BV83Q2SV4QAZ", time.Now().Add(-cfg.DeletionDelay)), nil)
Expand All @@ -608,12 +610,6 @@ func TestCompactor_ShouldNotCompactBlocksMarkedForDeletion(t *testing.T) {

c, _, tsdbPlanner, logs, registry := prepare(t, cfg, bucketClient)

// Mock the planner as if there's no compaction to do,
// in order to simplify tests (all in all, we just want to
// test our logic and not TSDB compactor which we expect to
// be already tested).
tsdbPlanner.On("Plan", mock.Anything, mock.Anything).Return([]*metadata.Meta{}, nil)

require.NoError(t, services.StartAndAwaitRunning(context.Background(), c))

// Wait until a run has completed.
Expand All @@ -623,8 +619,8 @@ func TestCompactor_ShouldNotCompactBlocksMarkedForDeletion(t *testing.T) {

require.NoError(t, services.StopAndAwaitTerminated(context.Background(), c))

// Only one user's block is compacted.
tsdbPlanner.AssertNumberOfCalls(t, "Plan", 1)
// Since both blocks are marked for deletion, none of them are going to be compacted.
tsdbPlanner.AssertNumberOfCalls(t, "Plan", 0)

assert.ElementsMatch(t, []string{
`level=info component=cleaner msg="started blocks cleanup and maintenance"`,
Expand Down