Skip to content

Conversation

alexqyle
Copy link
Contributor

@alexqyle alexqyle commented Feb 5, 2025

What this PR does:

If there is bucket index file but no blocks in bucket store for one user, cleaner would keep running cleaning logic and update empty bucket index for such user. With this PR, cleaner would delete bucket index and its sync status file if there is no block in bucket store to prevent such dangling user exists in bucket store.

Which issue(s) this PR fixes:

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@dosubot dosubot bot added component/compactor storage/blocks Blocks storage engine labels Feb 5, 2025
Signed-off-by: Alex Le <[email protected]>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 5, 2025
Copy link
Contributor

@danielblando danielblando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return err
if idx.IsEmpty() && len(partials) == 0 {
level.Info(userLogger).Log("msg", "deleting bucket index since it is empty")
if err := bucketindex.DeleteIndex(ctx, c.bucketClient, userID, c.cfgProvider); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR. Do we need to handle AccessDeniederror in bucketindex.DeleteIndex and bucketindex.DeleteIndexSyncStatus?

I see they only handle object not found

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cortex itself cannot handle AccessDenied. Once it happens, cleaner should fail the process and enduser should take some action on it.

begin = time.Now()
if err := bucketindex.WriteIndex(ctx, c.bucketClient, userID, c.cfgProvider, idx); err != nil {
return err
if idx.IsEmpty() && len(partials) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the bucket index is empty but we have partial blocks, we are going to still write the index. Are we waiting for cleaner to clean up partial blocks and eventually we will delete the empty bucket index?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. If there are partial blocks, we need cleaner to delete those first. Bucket index file should be deleted if it is the only file in bucket store for a tenant.

Signed-off-by: Alex Le <[email protected]>
@yeya24 yeya24 merged commit d8cb7f1 into cortexproject:master Feb 11, 2025
17 checks passed
@alexqyle alexqyle deleted the cleaner-bucket-index branch February 13, 2025 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants