From f1338dce8be0d73d8031521eea1c652617b55c4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Fri, 13 Aug 2021 09:58:21 +0200 Subject: [PATCH] Don't try to compact blocks marked for deletion. (#4328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Don't try to compact blocks marked for deletion. * Update CHANGELOG.md Signed-off-by: Peter Štibraný Signed-off-by: Alvin Lin --- CHANGELOG.md | 1 + pkg/compactor/compactor.go | 4 ++-- pkg/compactor/compactor_test.go | 12 ++++-------- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c67fb4785d..9af8a36153 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,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 diff --git a/pkg/compactor/compactor.go b/pkg/compactor/compactor.go index 3e65d6cd12..1b1715473b 100644 --- a/pkg/compactor/compactor.go +++ b/pkg/compactor/compactor.go @@ -644,11 +644,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. + // 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( diff --git a/pkg/compactor/compactor_test.go b/pkg/compactor/compactor_test.go index 911c59aa26..7bb39a7867 100644 --- a/pkg/compactor/compactor_test.go +++ b/pkg/compactor/compactor_test.go @@ -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) @@ -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. @@ -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"`,