Fix delete implementation for s3/gcs/azure backends to account for prefix - #6011
Conversation
|
|
|
Thanks for the fix! It seems like this test is failing: https://github.com/grafana/tempo/blob/main/integration/poller/poller_test.go#L214 Can you review? |
The backend delete function (which prepends the prefix) is being used in two places: first to delete indexes directly using the file path, and secondly, the results from the Find function are iterated over to delete the files for a tenant. If the result set from the Find function returns the full path (including the prefix), the delete function would prepend the prefix again and hence would not target the correct path. Hence all Find implementations are updated to return unprefixed key in the result set (we trim the prefix from key before passing over to the FindFunc arg). This is consistent with the List implementation.
|
Thanks @joe-elliott, I have pushed a fix for the test failure. It seems the test was failing due to the presence of prefix in the readWriter.Find function result set. The delete function is being called from two places: backend.writer.WriteTenantIndex and poller.deleteTenant. The first call passes a non prefixed path for deletion, while the second one passes the full path including the prefix. In the deleteTenant function, the result of readWriter.Find call is used to generate a list of files to be deleted for a tenant. The Find function was populating the prefixed key in backend.FindMatch (full path to the object including the prefix), and the key from this result was being passed to the Delete function which would again prepend the prefix, causing the test to fail. I have changed the readWriter.Find implementation for s3/gcs/azure backends to populate non prefixed key in the FindMatch resultset. This is in accordance with the List implementation for these backends, where the prefix is trimmed from the result. |
joe-elliott
left a comment
joe-elliott
left a comment
There was a problem hiding this comment.
Nice find and fix! Thank you for the contribution.
|
Has this not been part of a release yet? |
…efix (grafana#6011) * Fix delete impl for s3/gcs/azure backends to account for prefix * Update changelog * Omit prefix from backend.readerWriter.Find result set The backend delete function (which prepends the prefix) is being used in two places: first to delete indexes directly using the file path, and secondly, the results from the Find function are iterated over to delete the files for a tenant. If the result set from the Find function returns the full path (including the prefix), the delete function would prepend the prefix again and hence would not target the correct path. Hence all Find implementations are updated to return unprefixed key in the result set (we trim the prefix from key before passing over to the FindFunc arg). This is consistent with the List implementation. * Add prefix for keypath for versioned deletes * Remove bucket prefix in backend.Find instead of entire prefix
4 participants
What this PR does:
The PR updates the delete implementation for s3/gcs/azure backends to account for prefix if it is configured for the storage backend.
This fixes the issue where tenant indexes are never deleted if a prefix is used. When the poller attempts to write the tenant index for tenants with empty blocklist, the backend writer is unable to delete the index because of the missing prefix in index filepath.
Which issue(s) this PR fixes:
Fixes #5993
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]