Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete the index for empty tenants #2678

Merged
merged 4 commits into from
Jul 20, 2023

Conversation

zalegrala
Copy link
Contributor

@zalegrala zalegrala commented Jul 19, 2023

What this PR does:

When a tenant stops writing to tempo, the blocks eventually get cleaned up by the retention, but the tenant index is still left and so the tenant is still left in the list when the poller determines which indexes to pull. This can create additional load for no benefit long term if the tenant doesn't return with traffic.

Here we call the recently implemented Delete on the RawWriter to remove the tenant indexes from the backend to complete the deletion of all tenant objects when no blocks or compacted blocks have been found to exist.

Which issue(s) this PR fixes:
Fixes #2520

Checklist

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

Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

Nice cleanup. I think all of the real object stores will "delete" the tenant folder when the last item is removed (i.e. index.json.gz). Will this be a problem for the local backend? It might be used in some cases for non-production setups. The local-blocks processor and the ingester also use a local backend but not with polling so they should be ok.

@zalegrala
Copy link
Contributor Author

The local backend would leave the directories around, good eyes. I suspect this isn't an issue due to the speed of local disk in comparison to network calls. Since the use cases you mention don't keep an index, I'm okay with this as is. We can revisit if it comes up again.

@zalegrala zalegrala merged commit c869872 into grafana:main Jul 20, 2023
14 checks passed
@zalegrala zalegrala deleted the tempoIndexDeletion branch July 20, 2023 20:02
zalegrala added a commit to zalegrala/tempo that referenced this pull request Apr 10, 2024
The tenant index deletion was originally put in as TCO win, but did not
have the desired effect and surfaced other issues in the system.

Related grafana#2678
Related grafana#2754
Related grafana#2781
Related grafana#2878
Related grafana#3115
Related grafana#3223

Due to the number of issues here, and causing considerable noise on the
pager, perhaps the right thing to do is back out the tenant deletion.

Raising here for discussion.
@zalegrala zalegrala mentioned this pull request Apr 10, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tempo leaves tenant indexes on tenants with no blocks
2 participants