Repository navigation
Conversation
e000cf4 to
0355207
Compare
0355207 to
8f727a8
Compare
|
Rebased onto master to resolve the The earlier |
8f727a8 to
77650d5
Compare
|
Rebased onto master again to resolve the |
sandy2008
left a comment
sandy2008
left a comment
There was a problem hiding this comment.
The empty-location guard looks safe given the ordering of the writers (the global mark is written before the per-block one, and block.Delete removes deletion-mark.json last), and the new test fails without the fix. One problem with how the iteration is stopped early, inline.
| hasObjects := false | ||
| err := w.bkt.Iter(ctx, id.String()+objstore.DirDelim, func(string) error { | ||
| hasObjects = true | ||
| return errStopIteration |
There was a problem hiding this comment.
Returning an error to stop early doesn't play well with S3. The objstore S3 Iter returns straight away and leaves minio's ListObjects channel undrained, and its producer blocks on the send (even after ctx cancel), so with three or more objects under the prefix a goroutine leaks per call. Cortex's BucketWithRetries also treats the sentinel as a failure and retries it 5 times with 5s→1m backoff, and the metrics wrapper counts it in thanos_objstore_bucket_operation_failures_total{operation="iter"}. This path recurs every cleanup cycle for a block whose per-block mark never got uploaded. A block directory only has a handful of objects, so setting hasObjects and returning nil (letting the listing finish) avoids all three.
There was a problem hiding this comment.
Good catch, thanks. In 4fe9f99 the callback now sets hasObjects and returns nil, so the listing always finishes, and I removed errStopIteration. The bucketindex tests pass.
When a block no longer exists but its deletion mark is still in the tenant's markers/ location, the bucket index updater skipped the mark at every run and never removed it, so the tenant location stayed in the bucket forever. Delete such a mark when the block location is empty. Fixes cortexproject#7885 Signed-off-by: Srinivas Chippagiri <cvas22@gmail.com>
Signed-off-by: Srinivas Chippagiri <cvas22@gmail.com>
Returning an error from the Iter callback leaves the S3 listing undrained, and BucketWithRetries and the metrics wrapper treat it as a failed operation. A block location holds only a few objects, so list them all. Signed-off-by: Srinivas Chippagiri <cvas22@gmail.com>
4fe9f99 to
e67337a
Compare
|
I rebased this on |
What this PR does:
When a block is gone from the bucket but its deletion mark is still in the tenant's
markers/location, the bucket index updater logsskipped missing block deletion markon every run and never removes the mark. The tenant location then stays in the bucket forever, even with no blocks and no bucket index.This change deletes such a mark from the updater. To avoid racing with a block that is being marked for deletion (the global mark is uploaded before the per-block one), the mark is only removed when nothing exists under the block's location. If listing or deleting fails, it is logged and the update continues.
Only block deletion marks are handled. Orphaned no-compact marks are not touched.
Tests: new
TestUpdater_UpdateIndex_ShouldDeleteOrphanedGlobalDeletionMarks(fails without the change). Ran thebucketindexandcompactorblocks cleaner tests on Linux.Which issue(s) this PR fixes:
Fixes #7885
Checklist
Documentation added(no user-facing config or documented behavior changes)CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]docs/configuration/v1-guarantees.mdupdated if this PR introduces experimental flags