Fix DeleteAll() on non-existing directory

[pracucci]

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

Changes

We're upgrading Thanos in Cortex and we noticed the compactor logs "failed deleting non-compaction group directories/files, some disk space usage might have leaked". The reason is that, under some conditions, the directory doesn't exist at all. I suggest to consider that case as a non-error.

Verification

N/A

[GiedriusS]

There was an identical attempt here: #3869 (comment). Under what circumstances and what directory might not exist when calling this? (:

[pracucci]

There was an identical attempt here: #3869 (comment). Under what circumstances and what directory might not exist when calling this? (:

In Cortex the directory passed to Compactor is not pre-created so the first time you run it you would get the error. In other news, DeleteAll() is a generic function and I believe returning no error if the directory doesn't exist looks safer from its side.

[GiedriusS]

There was an identical attempt here: #3869 (comment). Under what circumstances and what directory might not exist when calling this? (:

In Cortex the directory passed to Compactor is not pre-created so the first time you run it you would get the error. In other news, DeleteAll() is a generic function and I believe returning no error if the directory doesn't exist looks safer from its side.

Playing devil's advocate: what stops from checking whether this directory exists before calling this function there? Tbf I don't mind this change at all, but let's see what @bwplotka thinks.

[pracucci]

what stops from checking whether this directory exists before calling this function there?

We can check if the directory exists in the Thanos compactor before calling DeleteAll() but I can't understand why we should do it there instead of fixing it at the root.

[bwplotka]

Thanks.

This is similar to object storage deletion and the idea to make it idempotent or not. I don't feel idempotency is good in those cases, as you want to know usually why you unnecessary spent cycles, especially if some code will change in the future.

However given we are a library for Cortex, we have to behave better on this.
I would be not opposed to merging this UNLESS #3869 (comment) is not solving your problem, we can port those changes to master anytime (happy to do this today evening).

[bwplotka]

We merged the leak causing the log line: Wonder if this helps #3978

If not, let's merge this for easy of use

[pracucci]

We merged the leak causing the log line: Wonder if this helps #3978

I just tried. The issue we have is still there.

This is similar to object storage deletion and the idea to make it idempotent or not. I don't feel idempotency is good in those cases, as you want to know usually why you unnecessary spent cycles, especially if some code will change in the future.

I see DeleteAll() similar to os.RemoveAll(), with the additional support for ignoreDirs. os.RemoveAll() is idempotent (returns no error if the provided dir doesn't exist), reason why I thought making DeleteAll() idempotent was not a terrible idea.

That being said, we need to move on in Cortex, so I would appreciate if we take a final decision here. I'm OK if this PR gets rejected (I will do a workaround in Cortex), but a decision is better than no decision 😉

[bwplotka]

You convinced me with std lib example (:

LGTM!