Compact: Fixed issue with no-compact-mark.json being ignored #6229

xBazilio · 2023-03-20T19:01:12Z

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

Changes

Verification

xBazilio · 2023-03-20T19:04:04Z

Possible fix for #5603

yeya24 · 2023-03-20T19:04:53Z

Thanks @xBazilio. Can you please elaborate more, how this fixed that issue?

xBazilio · 2023-03-20T19:17:55Z

Sometimes blocks are overlapping. The reason doesn't matter now. For compaction not to halt we mark those block with no-compact-mark.json

But this mark is being ignored by compactor and compactor keeps halting.

I described how to reproduce the problem in comment to #5603

There is a method areBlocksOverlapping, which has 2 arguments. It is used later in compact with result of planner.Plan as second argument.

When planner is created it reseived a method to exclude marked blocks.

However there, in the beginning of compact method, areBlocksOverlapping is used without argument, so it would always report about overlapping blocks and halt compaction, regardless of blocks being marked for no compaction.

xBazilio · 2023-03-21T07:58:23Z

I'll look into it more, because it's not very clear what areBlocksOverlapping is actualy doing with its 2 arguments.

xBazilio · 2023-03-24T17:46:58Z

Some comments to what I did

According to #3409, blocks marked with no-compaction-mark.json should not be compacted. But in fact they still meddle in the proccess and even cause compaction to halt.

I have added checks to Groupper and ApplyRetention so that they skip marked blocks.
This allows compactor to continue to work as intended, marked blocks won't be accidentally deleted by ApplyRetention.

I had to add one more test block to compact_e2e_test.go, without it there wasn't enough blocks to compact with excluded marked blocks.

xBazilio · 2023-03-28T15:41:20Z

@yeya24 hello! Would you be so kind as to look into my PR again, please, or tag anyone who can?

I struggled with e2e tests, fixed one that directly releated to compactor, but there are other failing tests.

xBazilio · 2023-04-10T12:14:39Z

All tests passed 🥹

GiedriusS · 2023-04-12T09:04:49Z

cmd/thanos/compact.go

+			sy.Metas(),
+			retentionByResolution,
+			compactMetrics.blocksMarked.WithLabelValues(metadata.DeletionMarkFilename, ""),
+			noCompactMarkerFilter); err != nil {


I understand the grouper change but why we should ignore no compact blocks in here? We still want to delete old blocks even if they have that mark, right? Otherwise, the block storage usage will grow inexorably.

My point is: overlap is somthing out of ordinary. One block can be small enough that it will never be downsampled. Thus, if we delete it with retention policy, we would get a gap in metrics.
I suppose that we won't be marking blocks in huge numbers, so storage usage shouldn't grow uncontrollably.

But no-compact-mark.json can appear even without the user knowing: https://github.com/thanos-io/thanos/blob/main/pkg/compact/planner.go#L282-L287. I have some blocks with no-compact-mark.json too due to this. So, with your change they would never get deleted because they would be filtered out by the retention apply function, right?

Right. Just today I stumbled on this case.
It's tricky. Maybe there should be a flag to control wether to prevent retention policy from deleting marked blocks.

Maybe retention policy alhorithm should check if there is a block with lower resolution before deleting block. We prefer to store 1h resolutoin forever, while having 0s for 12 days and 5m for 60 days. Also we had compaction halted for several months, before we noticed. If we would allow retention to remove blocks it would just delete all raw data that wasn't compacted due to halting. We solved it for now by turning off retention policy.

What do you think? Can I make a flag, or should I revert changes to retention?

Applying retention could definitely be improved as you've suggested however retention only gets applied after compaction/downsampling so it shouldn't ever delete data that hasn't been compacted.

All in all, could you please remove the changes to the retention part? I believe this kind of change should be discussed and then done separately. Then we could merge your PR.

Done. Tests passed too.

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

GiedriusS

Why is it not enough to exclude marked blocks from the planner? 🤔

xBazilio · 2023-04-18T11:23:37Z

Why is it not enough to exclude marked blocks from the planner? thinking

https://github.com/thanos-io/thanos/blob/main/pkg/compact/compact.go#L983

There is a check before the planning is done. I gues compact group should already contain non overlapping blocks.

GiedriusS · 2023-04-26T09:51:51Z

I believe that the correct fix is somewhere in the planner or in the filters? This sounds like a workaround. Groups should just give us the groups, without any filtering. It's the planner's responsibility to do the right thing. Digging a bit into this more and one potential fix is here: #6319

GiedriusS · 2023-04-26T09:52:20Z

I will try your #5603 (comment) instructions here to see what's happening because I believe I'm hitting this too.

stale · 2023-06-18T07:10:42Z

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

pull-request-size bot added the size/S label Mar 20, 2023

xBazilio mentioned this pull request Mar 21, 2023

compact: no-compact-mark.json ignored #5603

Closed

xBazilio force-pushed the fix_ignored_no_compact_mark branch from 99cd0fe to 4f5ab9b Compare March 24, 2023 17:32

pull-request-size bot added size/M and removed size/S labels Mar 24, 2023

xBazilio force-pushed the fix_ignored_no_compact_mark branch from 70fa331 to 7a91e9a Compare March 27, 2023 07:48

xBazilio force-pushed the fix_ignored_no_compact_mark branch from bd019a2 to b771377 Compare April 10, 2023 11:44

GiedriusS reviewed Apr 12, 2023

View reviewed changes

xBazilio added 6 commits April 13, 2023 11:28

ignore tmp

0b11c97

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

make compactor skip blocks marked for no compaction

6801cf9

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

changelog entry

d8eecaa

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

fix failing e2e tests on main branch

1b1fb1c

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

fix e2e test

e6448f7

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

revert changes to retention

dd203bd

Signed-off-by: Vasiliy Rumyantsev <4119114+xBazilio@users.noreply.github.com>

xBazilio force-pushed the fix_ignored_no_compact_mark branch from b771377 to dd203bd Compare April 13, 2023 08:42

GiedriusS reviewed Apr 18, 2023

View reviewed changes

stale bot added the stale label Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compact: Fixed issue with no-compact-mark.json being ignored #6229

Compact: Fixed issue with no-compact-mark.json being ignored #6229

xBazilio commented Mar 20, 2023 •

edited

Loading

xBazilio commented Mar 20, 2023

yeya24 commented Mar 20, 2023

xBazilio commented Mar 20, 2023 •

edited

Loading

xBazilio commented Mar 21, 2023

xBazilio commented Mar 24, 2023

xBazilio commented Mar 28, 2023

xBazilio commented Apr 10, 2023

GiedriusS Apr 12, 2023

xBazilio Apr 12, 2023

GiedriusS Apr 12, 2023

xBazilio Apr 12, 2023

GiedriusS Apr 13, 2023 •

edited

Loading

xBazilio Apr 13, 2023

GiedriusS left a comment

xBazilio commented Apr 18, 2023

GiedriusS commented Apr 26, 2023

GiedriusS commented Apr 26, 2023

stale bot commented Jun 18, 2023

Compact: Fixed issue with no-compact-mark.json being ignored #6229

Are you sure you want to change the base?

Compact: Fixed issue with no-compact-mark.json being ignored #6229

Conversation

xBazilio commented Mar 20, 2023 • edited Loading

Changes

Verification

xBazilio commented Mar 20, 2023

yeya24 commented Mar 20, 2023

xBazilio commented Mar 20, 2023 • edited Loading

xBazilio commented Mar 21, 2023

xBazilio commented Mar 24, 2023

xBazilio commented Mar 28, 2023

xBazilio commented Apr 10, 2023

GiedriusS Apr 12, 2023

Choose a reason for hiding this comment

xBazilio Apr 12, 2023

Choose a reason for hiding this comment

GiedriusS Apr 12, 2023

Choose a reason for hiding this comment

xBazilio Apr 12, 2023

Choose a reason for hiding this comment

GiedriusS Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

xBazilio Apr 13, 2023

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

xBazilio commented Apr 18, 2023

GiedriusS commented Apr 26, 2023

GiedriusS commented Apr 26, 2023

stale bot commented Jun 18, 2023

xBazilio commented Mar 20, 2023 •

edited

Loading

xBazilio commented Mar 20, 2023 •

edited

Loading

GiedriusS Apr 13, 2023 •

edited

Loading