Delete deletion-mark.json at last when deleting a block

[pracucci]

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

Changes

In Cortex, we have an extra protection to delete partial blocks only when the deletion-mark.json exists (reason is that we don't expect any partial block unless an issue). However, if the block deletion fails, we may end up in a situation where meta.json as been deleted (correct), deletion-mark.json deleted but few other files still left in the bucket (eg. index).

In this PR I'm proposing to delete deletion-mark.json at last. The only downside is that we're adding an extra HEAD object call to check for existence, but I'm planning to improve in a follow up PR (I would like to propose to change objstore clients Delete() contract to be idempotent, but that's a separate discussion #3662).

Verification

Unit tests.

[bwplotka]

Makes sense.

I wonder how you don't see partial blocks in worst cases in Cortex? What if Compactor crashes in middle of upload? What if ingester crashes in middle of upload (this one will resumed I supposed). What if it is being decommissioned in the middle of upload? (scale down)

[GiedriusS]

Makes sense.

I wonder how you don't see partial blocks in worst cases in Cortex? What if Compactor crashes in middle of upload? What if ingester crashes in middle of upload (this one will resumed I supposed). What if it is being decommissioned in the middle of upload? (scale down)

I have the same question too. This change sounds more like a workaround for Cortex that doesn't (?) do that cleanup 🤔

[pracucci]

I wonder how you don't see partial blocks in worst cases in Cortex? What if Compactor crashes in middle of upload? What if ingester crashes in middle of upload (this one will resumed I supposed). What if it is being decommissioned in the middle of upload? (scale down)

Good question.

The thought behind it was that there's no easy way to know if a partial block is safe to be deleted or not. If it comes from a Cortex ingester, then it's probably not safe, while if it comes from compactor it is, but without a meta.json we don't know where it comes from. Because of this, the conservative approach we took was to only delete a partial block if had a deletion mark, delegating the operator to manually investigate it and take an informed decision (at least while we learn all failure scenarios), and we now have a metric to view/alert on number of partial blocks per tenant.

More thoughts about possible cases. There are two services uploading blocks:

• Ingester
  • Graceful shutdown: should wait until upload is done
  • Abruptly termination: should reupload it at next startup (if, for any reason, it doesn't, then we would potentially have a data loss, if the same issue happens on 2+ ingesters)
• Compactor
  • It's ok if leaves a partial block

Thoughts?

[pracucci]

@bwplotka @GiedriusS I would like to follow up on this discussion. What do you think about my last message? I would be glad to either bring this change in or discard it (abd in that case we'll have to figure out how to deal with that in Cortex).

[bwplotka]

Thanks for explaining.

I still think you miss valid case of partial upload during ingester scaledown 🤔

Anyway, this does not mean we can't have this invariant in our deletion process - I like this determinsm anyway. SGTM!

Sorry for lag!

[pracucci]

I still think you miss valid case of partial upload during ingester scaledown

If all blocks are not successfully uploaded on ingesters scaledown, then you will have a data loss (well, you do if this happens on 2+ ingesters). I'm not sure it's a legit case having partial blocks on ingesters scaledown.