Add shuffle-sharding for the compactor

[ac1214]

What this PR does:

Adds shuffle-sharding for the plans generated by the grouper. Depends on #4432. Once #4432 is merged the diff for this PR will be reduced.

Implements Proposal Parallel Compaction by Time Interval

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[bboreham]

Seems plausible. Will review again after the preceding PRs are merged.

[edma2]

@ac1214 Any plans to resume progress on this amazing work?
We observed great improvement to compaction speed after running this branch. Our biggest tenant is ~ 40-50M time series and we were able to fully compact it down to 24h blocks in a few days.

[ac1214]

@ac1214 Any plans to resume progress on this amazing work? We observed great improvement to compaction speed after running this branch. Our biggest tenant is ~ 40-50M time series and we were able to fully compact it down to 24h blocks in a few days.

Thanks for checking/testing out these changes @edma2!

I haven't had much time to work on these changes, but I believe that @roystchiang might be taking over this work.

[edma2]

Every Compactor now "owns" all the users, even if it doesn't actually compact any blocks for most users. One particular impact I saw is a huge growth of metadata syncs. Instead, maybe we can compute ownUser based on the shuffle shard of a tenant.

[edma2]

Quick fix that seems to work: b0c2c2a

[roystchiang]

hey @edma2 thanks for taking a look at the compactor PR. We are actively testing this branch in our beta environment, and I'm glad to hear that it is working for you.

We have a similar fix to your commit, and I can confirm that we also saw an improvement in the metadata syncs.

However, on the tenant clean up side, we were running into issues where the deleted tenant directory was left dangling. While compactor-A is deleting the deletion markers, compactor-B is also trying to sync the data, and re-uploads the block index. We currently provide an override for the tenant shard size on the clean-up path so that only 1 compactor owns the cleanup for a given tenant.

[edma2]

thanks for the tip on the cleanup side. Makes sense that you don't want multiple compactors repeating the same work and potentially conflicting.

[edma2]

@roystchiang with this PR have you started seeing any errors that look like overlapping sources detected for plan? I usually delete one of the culprit blocks to resolve the issue, but I was wondering if this is something you also noticed on your end, and if so would support the theory that this is related to parallel compaction.

[roystchiang]

@roystchiang with this PR have you started seeing any errors that look like overlapping sources detected for plan? I usually delete one of the culprit blocks to resolve the issue, but I was wondering if this is something you also noticed on your end, and if so would support the theory that this is related to parallel compaction.

We've been running this change for a bigger workspace, but we have not seen this error yet

Are you able to provide more detail? Is it a level-1 -> level-2 compaction? and what level blocks are these?

[edma2]

Currently, my theory is that this happens in the following situation:

For a vertical compaction group e.g. (10:00 - 12:00), one Ingester has not yet uploaded its block (for some reason).
L1->L2 compaction proceeds anyway and compactor A outputs A1 as a L2 block.
Compactor B then starts L2 -> L3 compaction (e.g. 00:00 - 12:00) which includes block A1.
The late Ingester uploads the missing block M (for 10:00 - 12:00).
Compactor A does compaction again because there are > 1 block for that (10:00 - 12:00) compaction group: [A1, M].
Compactor B finishes compaction and outputs block B1 (00:00 - 12:00), which includes A1 as a source.
Compactor A finishes compaction and outputs block A2 (10:00 - 12:00), which includes A1 as a source.

Compactor B tries to compact [B1, A2] within the (00:00 - 12:00) compaction group and fails since both include A1 as the source. I don't think this issue would happen if only one compactor was running for a tenant.

[wilfriedroset]

I've been testing successfully this PR on my compactor on a cluster with a single tenant having 200M actives series.

I'm running 6 compactors with the following configuration.

compactor:
  block_deletion_marks_migration_enabled: false
  block_sync_concurrency: 10
  compaction_interval: 30m0s
  data_dir: /var/lib/cortex/data
  meta_sync_concurrency: 10
  sharding_enabled: true
  sharding_ring:
    instance_interface_names:
    - eth1
    kvstore:
      prefix: cortex/collectors/
  sharding_strategy: shuffle-sharding
limits:
  compactor_tenant_shard_size: 6
target: compactor

Here are a couple of message from the logs which can raise concern.
The first one has been discussed above

• Dec 13 13:51:02 cortex-compactor-6 cortex[8188]: ts=2021-12-13T13:51:02.855219667Z caller=log.go:168 component=compactor level=info msg="Found overlapping blocks during compaction"
  I'm not sure how bad this one is as the retry seems to be working
• Dec 13 13:53:38 cortex-compactor-6 cortex[8188]: level=debug ts=2021-12-13T13:53:38.781769674Z caller=client.go:191 msg="error CASing, trying again" key=cortex/collectors/compactor index=20677798

The last one is more concerning, seen it 20 time, only on 1 of my 6 compactors
Dec 11 04:53:42 cortex-compactor-6 cortex[8188]: level=warn ts=2021-12-11T04:53:42.954993545Z caller=block.go:191 component=compactor org_id=fake groupKey=29059490200@5679675083797525161 rangeStart="2021-11-27 10:00:00 +0000 UTC" rangeEnd="2021-11-27 12:00:00 +0000 UTC" externalLabels="{__org_id__=\"fake\"}" downsampleResolution=0 msg="requested to mark for deletion, but file already exists; this should not happen; investigate" err="file 01FNGQS0QF8RXE7YK043ZNPH3J/deletion-mark.json already exists in bucket"

[roystchiang]

@edma2, what you suggest makes sense, and it can definitely happen. How often does this issue happen to do you?

Let me see if there's a way to avoid this. With a coordinator for compactors, we can block the compaction of A2 until B1 is done. However, we don't have this right now. Thanos played around with this idea in thanos-io/thanos#4458, and it would be nice to unify this logic for both Thanos and Cortex.

@wilfriedroset, for requested to mark for deletion, but file already exists; this should not happen; investigate" err="file 01FNGQS0QF8RXE7YK043ZNPH3J/deletion-mark.json already exists in bucket, is this a part of the user retention period cleanup? or regular compaction?

[wilfriedroset]

The error message I got was during a regular compaction

[edma2]

what you suggest makes sense, and it can definitely happen. How often does this issue happen to do you?

not very often, I'd say a corrupt block is created less than once a week

[alanprot]

Compactor B then starts L2 -> L3 compaction (e.g. 00:00 - 12:00) which includes block A1.

Could we lock the block A1 at this point? So it would not be used as source to any other compaction?
If we do that we will not have the following problem anymore right?

Compactor B tries to compact [B1, A2] within the (00:00 - 12:00) compaction group and fails since both include A1 as the source. I don't think this issue would happen if only one compactor was running for a tenant.

The only problem with locks ofc is what to do if the compactor dies, we need to have a lock timeout and some kinda and compactor would need to keep updating the validUntil to now+10m or something like that?

@edma2 Could you prove that that's what happened on your case? Maybe looking at the timestamps of when the source blocks got updated on s3?

[edma2]

My original theory was based on what I saw in the compactor logs. I'll look at this again and see if I can find any recent compactor logs that tell this story.

[nschad]

We tested a few blocks starting from level 1 compaction (~1TB) on a new bucket. To test it it, I essentially ran the same config as @wilfriedroset . Couldn't find any major issues, seem to have compacted just fine.

Extra: Took this pr and rebased it against current master and then build the image.

Update: Compacted 10TiB this way, zero Issues

[alanprot]

I've been running this for some time and we did not see the problem happening. We could skip the problematic block using #4707

We could also propose a change on Thanos to skip those blocks similarly what was introduced thanos-io/thanos#4469