Fixed partial delete issues on compactor; Added Upload/Delete tests.

[bwplotka]

Fixes #1331

Problem that we are fixing is explained in the linked issue.

Also the problem will be fully fixed once this is also done: #1394 This is because although major parts of Thanos for syncing blocks is resilient to partial uploads, there are some points when we don't handle it properly (crash e.g compactor)

cc @lx223 @metalmatze

Signed-off-by: Bartek Plotka bwplotka@gmail.com

[metalmatze]

Maybe it makes sense to put path.Join(id.String(), MetaFilename) into a variable, to avoid mismatches in the future with the second time below that you use this path.

[bwplotka]

👍
Fixed

[metalmatze]

One small improvement/nit. Otherwise this looks good! Nice fix!

[SuperQ]

Is there any race between the sidecar uploading (no meta.json yet) and the compactor deleting?

[bwplotka]

Is there any race between the sidecar uploading (no meta.json yet) and the compactor deleting?

@SuperQ Yea, detecting if the block is partially uploaded (malformed) vs upload is in progress is in current model quite tricky. We do that currently by just waiting for some time called consistencyDelay (configurable on compactor:

thanos/cmd/thanos/compact.go

Line 86 in c83332f

consistencyDelay := modelDuration(cmd.Flag("consistency-delay", fmt.Sprintf("Minimum age of fresh (non-compacted) blocks before they are being processed. Malformed blocks older than the maximum of consistency-delay and %s will be removed.", compact.MinimumAgeForRemoval)).

). For now the default is 30m.

This has proven to be ok for all users (as per: no known issue because of that). Once upload takes more than 30 minutes, you might want to increase such consistency delay. This delay is also useful for object storages that are eventually consistent.

Good question is: can we warn/document these details about uploads longer than 30 minutes ):

[SuperQ]

What about a simple locking method of renaming meta.json to meta.json.deleted, then cleanup the index/chunks, then finally delete the meta.json.deleted file.

This would also allow for deletions to continue in the event of a restart.

[bwplotka]

I merged already, but happy to discuss alternative ideas @SuperQ

What about a simple locking method of renaming meta.json to meta.json.deleted, then cleanup the index/chunks, then finally delete the meta.json.deleted file.

Do you mean for deleting process? Not sure how is that better (: We want to get rid meta.json ASAP. Once we remove meta.json, block is nicely handled as malformed even if deletion would be disrupted at any point (e.g by pod crash/restart). In fact we would be fine by just deleting meta.json and allow compactor to assume as malformed block in next iteration.

[bwplotka]

We also discussed some alternatives here: https://thanos.io/proposals/201901-read-write-operations-bucket.md/

[bwplotka]

There is still some slight chance to restart compactor in a certain moment that will cause overlaps (in the worst case). For that to work we need to implement action items for design doc: https://thanos.io/proposals/201901-read-write-operations-bucket.md/