I recently ran into this “fun” problem where the Compactor was stuck compacting the same blocks repeatedly. Only after lots of trial and error, I have found out that sharding using hashmod on some labels that are also dedup labels in vertical compaction was the culprit. Vertical compaction is like horizontal compaction but vertical compaction compacts blocks that have overlaps in time. And also vertical compaction removes the specified replica labels. Apparently, Thanos Compactor depends on having a “global” view of all blocks to filter out blocks that had been compacted into higher compaction level blocks. The interesting part is this filter: https://github.com/thanos-io/thanos/blob/f7ba14066fc35095e13ca3f675915c509310f476/cmd/thanos/compact.go#L235. It filters out blocks that can be formed from two or more overlapping blocks that fully matches the source blocks of the older blocks. It uses data in the meta.json file of each block to understand what are the sources of each block.
Example of a buggy configuration:
--- - action: hashmod source_labels: - foo modulus: 2 target_label: shard - action: keep source_labels: - shard regex: 0
And --deduplication.replica-label="foo" would trigger this issue. So, my suggestion would be to stop adding any of the replica labels into the source_labels part of hashmod in Thanos Compactor selector relabel configs. I don’t think there’s a use case for that. We could even add a check for this.
There is one other label that doesn’t make sense for Thanos Compactor. It is __block_id. We have the whole mark no downsample/compact functionality for that – for removing blocks from downsampling & compaction respectively. It really only fits in Thanos Store but then it is still iffy. Thanos Store also kind of needs to see a global view of blocks because it has this functionality where it does not download data from non-downsampled blocks if max_source_resolution allows this.
I gathered from talking with other people that the majority of them opt to use time-based sharding which seems to work well. Perhaps __block_id will be fixed at some point in the future by having a “global” storage of blocks metadata – this way we won’t have to depend on each fetcher having the full view. I remember some people talked about this in the past but I cannot find the issue anymore, sorry.