Compactor: remove malformed blocks after delay

[mjd95]

Changes

Introduce a consistency delay in the compactor. If the compactor sees a block that does not have a meta.json in its initial sync, and the block is older than consistencyDelay, then the compactor removes it.

Related issues #621 #976

Verification

Manually deleted the meta.json files from some blocks in our testing bucket, ran this compactor against them and checked that it deleted the blocks in its inital sync then continued with the rest of the compaction.

[bwplotka]

This is great and exactly what we need, but we need to improve it with 2 things:

• Reuse with syncDelay we already have.
• Add support for the same thing in store gateway! As partial blocks affects store as well.

Thanks for this @mjd95

[mjd95]

Have changed around to re-use syncDelay. It has a default value of 30 minutes which I think is good. The only danger I guess would be if people have been running the compactor with a very low value? I wonder if we should still keep a safety check in removeIfMalformed anyway?

Discussed offline and won't add anything to the store gateway right now, as it's good to keep the store as read only.

[bwplotka]

Mostly allright, some suggestions though.

[bwplotka]

Have changed around to re-use syncDelay. It has a default value of 30 minutes which I think is good. The only danger I guess would be if people have been running the compactor with a very low value? I wonder if we should still keep a safety check in removeIfMalformed anyway?

Good point. I think we need to define some minimumDelay, Let's imagine you run strongly consistent objstorage like GCS. Even in this case consistencyDelay=0s is wrong as sidecars can upload blocks slowly, so it may take some time where the block is partially uploaded and we don't want compactor to remove that, right?

[mjd95]

Yeah @bwplotka that's the sort of thing I was worried about. Now that the --sync-delay flag is also controlling whether the block is old enough to be deleted or not, we need to be more careful if the flag is set very low.

I think we should probably have some sort of extra safety check in removeIfMetaMalformed. Either we could force the --sync-delay flag value to be >10m, or we could just add in an additional check removeIfMetaMalformed for some delay Thanos itself sets. I think I prefer the latter.

(I changed the flag name BTW. If we land it like this, it's annoying that the name change is a breaking changed, but it does help to show that the behaviour of the flag has changed.)

[bwplotka]

Yes, I think the flag name here is reasonable and it's not heavy used I think. Not sure for the the constraint. I think syncDelay delay minimum value makes sense more

[bwplotka]

Okey, let's not fit this into release although I am super sad ): I think this is critial piece. If you have 0 consistency delay it means you will kill your sidecar uploads ): Not fun. No way to separate those from bucket perspecitve as all is in meta. So this means some sort of consistency delay is must have, even for tests.

Alternatives:

• Push meta.json first and have another special file for locking (we would rather have native block support)
• Separate blocks to tell which ones are from compactor? Maybe compacted block should have some indication in dir name? or special file?
• Maybe we should have consistency Delay being configured but have special removeIfMalformed after hardcoded 30 min?

[bwplotka]

Well technically you can do just if c.removeIfMetaMalformed(workCtx, id) here (:

[bwplotka]

Let's ship it! LGTM.

[xjewer]

why it removes data by default, can we add a flag to explicitly do removal?

[midnightconman]

why it removes data by default, can we add a flag to explicitly do removal?

Like a debug or dry-run flag?

[xjewer]

Like a debug or dry-run flag?

the opposite, the flag which explicitly tells to remove these blocks, like --compact.remove-malformed-blocks

[bwplotka]

Very good point @xjewer, we could do that for safety, but we cannot see any current case when we remove important block, not duplicated in other places (even with eventually consistent storage), so why would you not remove the paritaly uploaded blocks (no meta) automatically? If you are worried I am happy to add such flag.

[xjewer]

while it's happening #318 (comment), i'd prefer removing manually instead of losing data, e.g. if I a meta.json is missing, I can get it at least from debug directory

[bwplotka]

I cannot see the use case but I am fine with manual option. I would argue if manual should be default though.

Even the issue you mentioned - I doubt it is causing partial uploads as we remove the partial uploaded one immdiately after failure (see: https://github.com/improbable-eng/thanos/blob/e6d5b49bf2d2281f8f2f2d0e853158174a2e541f/pkg/block/block.go#L113) and we do not proceed with removal of old compacted blocks.

[xjewer]

I doubt it is causing partial uploads as we remove the partial uploaded one immdiately after failure

for some reason, there's a 400 error on proxy, but minio (seems to be multipart upload ¯\_(ツ)_/¯) doesn't treat it as an error, thus it uploads block with no error and as a consequence there's no cleanUp.

we do not proceed with removal of old compacted blocks.

wait, I see the opposite

Malformed blocks older than the maximum of consistency-delay and %s will be removed

https://github.com/improbable-eng/thanos/pull/1053/files#diff-921a1164068d3a04217b2d85714b16adR298

[bwplotka]

I meant "we do not proceed with removal of old compacted blocks." if the upload was failed (:

[bwplotka]

@xjewer let's jump on slack I am confused (:

for some reason, there's a 400 error on proxy, but minio (seems to be multipart upload ¯_(ツ)_/¯) doesn't treat it as an error, thus it uploads block with no error and as a consequence there's no cleanUp.

You mean file was NOT uploaded but we get 200 and success from minio Upload request?