cmd: compact: clean partial / marked blocks concurrently by GiedriusS · Pull Request #3115 · thanos-io/thanos

GiedriusS · 2020-09-02T13:49:12Z

Changes

Clean partially uploaded and blocks marked for deletion concurrently
with the whole compaction/downsampling process. One iteration could
potentially take a few days so it should be nice to periodically clean
unneeded blocks in the background. Without this, there are huge spikes
in block storage usage. The spike's size depends on how long it takes to
complete one iteration.

The implementation of this is simple - factored out the deletion part
into a separate function. It is called at the end of an iteration +
concurrently if --wait has been specified. Add a mutex to protect from
concurrent runs.

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Verification

Updated e2e tests.

Clean partially uploaded and blocks marked for deletion concurrently with the whole compaction/downsampling process. One iteration could potentially take a few days so it should be nice to periodically clean unneeded blocks in the background. Without this, there are huge spikes in block storage usage. The spike's size depends on how long it takes to complete one iteration. The implementation of this is simple - factored out the deletion part into a separate function. It is called at the end of an iteration + concurrently if `--wait` has been specified. Add a mutex to protect from concurrent runs. Delete blocks from the deletion mark map so that we wouldn't try to delete same blocks twice or more. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

cmd/thanos/compact.go

yeya24

Overall it looks very good! Just one small nit, thanks!

yeya24 · 2020-09-11T13:23:42Z

cmd/thanos/compact.go

+		// No need to resync before partial uploads and delete marked blocks. Last sync should be valid.
+		compact.BestEffortCleanAbortedPartialUploads(ctx, logger, sy.Partial(), bkt, partialUploadDeleteAttempts, blocksCleaned, blockCleanupFailures)
+		if err := blocksCleaner.DeleteMarkedBlocks(ctx); err != nil {
+			return errors.Wrap(err, "error cleaning marked blocks")


One nit, is it better to be cleaning marked blocks? error seems redundant.

Yes, let's do this. Still valid comment 👍

bwplotka

Awesome, thanks!

Couple of comments but overall LGTM 💪

bwplotka · 2020-09-15T21:03:14Z

cmd/thanos/compact.go

+		// No need to resync before partial uploads and delete marked blocks. Last sync should be valid.
+		compact.BestEffortCleanAbortedPartialUploads(ctx, logger, sy.Partial(), bkt, partialUploadDeleteAttempts, blocksCleaned, blockCleanupFailures)
+		if err := blocksCleaner.DeleteMarkedBlocks(ctx); err != nil {
+			return errors.Wrap(err, "error cleaning marked blocks")


Yes, let's do this. Still valid comment 👍

cmd/thanos/compact.go

bwplotka · 2020-09-15T21:09:55Z

cmd/thanos/compact.go

+		// since one iteration potentially could take a long time.
+		if conf.cleanupBlocksInterval > 0 {
+			g.Add(func() error {
+				// Wait the whole period at the beginning because we've executed this on boot.


So... why not just removing this and removing boot time execution? (: Same stuff right?

EDIT: actually gave it a second thought. We need to explicitly run it at boot time to make sure that we don't have flaky tests because we depend there on a failure happening and a cleanup. It's not guaranteed to happen if we do everything concurrently.

It sounds wrong that we do more complex code only because we don't want to change tests 🤔

The tests would be much more complex and probably out of the scope of this PR. Actually, it's not just that, I think it's nice that we do this at least once. Imagine where someone doesn't use --wait and the whole Thanos Compact process ended before the clean-up has happened. Space usage would never go down in the remote object storage even though it could. And the user then could be charged more as a result of this not happening.

cmd/thanos/compact.go

onprem

Nice, LGTM!

bwplotka · 2020-09-23T17:51:42Z

Where we are with this? I want to cut 0.16.0-rc.0 tomorrow 🤗

…d_periodically Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

Remove "error" from the `error` and just directly call the function. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

bwplotka

LGTM for now, but I think we could improve a bit in future (: But not a blocker, LGTM!

Thanks 👍

bwplotka · 2020-10-21T10:58:35Z

cmd/thanos/compact.go

+		// since one iteration potentially could take a long time.
+		if conf.cleanupBlocksInterval > 0 {
+			g.Add(func() error {
+				// Wait the whole period at the beginning because we've executed this on boot.


It sounds wrong that we do more complex code only because we don't want to change tests 🤔

…d_periodically Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

Forgot to remove this part while solving conflicts. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

GiedriusS · 2020-10-22T11:56:44Z

I guess since the approvals are there and I have cleaned up the CHANGELOG.md, I'll merge this. I also ran this for a bit locally with --wait-interval=1s --wait --compact.cleanup-interval=1s locally and it seems to work (tm). Will probably deploy this into production quite quickly because the saw-tooth pattern is becoming really spiky due to the fact that one iteration is taking a long time :( I hope that's ok with everyone! Thanks for the reviews ❤️

) * cmd: compact: clean partial / marked blocks concurrently Clean partially uploaded and blocks marked for deletion concurrently with the whole compaction/downsampling process. One iteration could potentially take a few days so it should be nice to periodically clean unneeded blocks in the background. Without this, there are huge spikes in block storage usage. The spike's size depends on how long it takes to complete one iteration. The implementation of this is simple - factored out the deletion part into a separate function. It is called at the end of an iteration + concurrently if `--wait` has been specified. Add a mutex to protect from concurrent runs. Delete blocks from the deletion mark map so that we wouldn't try to delete same blocks twice or more. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> * *: update changelog, e2e tests Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> * cmd: compact: fix according to comments Remove "error" from the `error` and just directly call the function. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> * CHANGELOG: cleanups Forgot to remove this part while solving conflicts. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> * CHANGELOG: update Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> * CHANGELOG: clean whitespace Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com> Signed-off-by: Chans321 <tsschand@gmail.com>

GiedriusS force-pushed the clean_partial_marked_periodically branch 4 times, most recently from 53072a4 to 1fd6a5f Compare September 3, 2020 17:41

*: update changelog, e2e tests

09d60b5

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

GiedriusS force-pushed the clean_partial_marked_periodically branch from 1fd6a5f to 09d60b5 Compare September 3, 2020 19:23

GiedriusS marked this pull request as ready for review September 4, 2020 12:27

yeya24 reviewed Sep 8, 2020

View reviewed changes

cmd/thanos/compact.go Show resolved Hide resolved

yeya24 approved these changes Sep 11, 2020

View reviewed changes

bwplotka approved these changes Sep 15, 2020

View reviewed changes

onprem approved these changes Sep 16, 2020

View reviewed changes

GiedriusS added 2 commits October 16, 2020 17:48

Merge remote-tracking branch 'origin/master' into clean_partial_marke…

8c14462

…d_periodically Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

cmd: compact: fix according to comments

2e26808

Remove "error" from the `error` and just directly call the function. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

GiedriusS requested a review from bwplotka October 16, 2020 15:40

bwplotka approved these changes Oct 21, 2020

View reviewed changes

GiedriusS added 4 commits October 22, 2020 13:11

Merge remote-tracking branch 'origin/master' into clean_partial_marke…

73c2b49

…d_periodically Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

CHANGELOG: cleanups

8218d0c

Forgot to remove this part while solving conflicts. Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

CHANGELOG: update

d62aead

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

CHANGELOG: clean whitespace

f97130f

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

GiedriusS merged commit 57076a5 into thanos-io:master Oct 22, 2020

Sayrus mentioned this pull request Oct 29, 2020

mixin: Add block deletion panels for compactor dashboard #3380

Merged

2 tasks

zygiss mentioned this pull request Nov 3, 2020

Compactor: HTTP socket is only opened after initial cleanup of compacted blocks is complete #3395

Closed

GiedriusS mentioned this pull request Nov 8, 2020

compact: could "blocks marked for deletions" be deleted at the beginning? #2605

Closed

Shadi mentioned this pull request Nov 24, 2020

compact does not delete expired blocks in s3 #3436

Closed

wiardvanrij mentioned this pull request Jun 6, 2021

Tips for slow compact on a large bucket with large blocks #4310

Closed

Conversation

GiedriusS commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification

Uh oh!

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

yeya24 Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

bwplotka Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bwplotka Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

GiedriusS Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

GiedriusS Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

onprem left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka commented Sep 23, 2020

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

bwplotka Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

GiedriusS commented Oct 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GiedriusS commented Sep 2, 2020 •

edited

Loading

GiedriusS Oct 16, 2020 •

edited

Loading