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

Remove tenant when last block is removed from local backend #1947

Merged
merged 8 commits into from
Dec 15, 2022

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Dec 13, 2022

What this PR does:
This fixes a long running issue where tenants were persisted even if they had a zero length wal. The issue was that when rediscovering local blocks a tenant was still found even if there were no blocks. This PR fixes the issue with two changes:

  • When rediscovering local blocks don't include tenants without blocks
  • When clearing a block in the local backend remove the tenant as well

Technically only the first of the two is needed and would be a safer change. Having the second is nice because it will clear up folders in our ingester local backends. Should we remove the second change and err towards safety? Especially with the 2.0 release coming up?

Checklist

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

Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Copy link
Member Author

joe-elliott commented Dec 13, 2022

Another note:

Currently when rediscovering local blocks this PR will nicely skip tenants that don't have blocks. However, it will leave those folders forever. Only when a block is cleared from the local backend do we make the check on whether or not to remove the tenant. This PR, as is, will not clean up already existing empty tenant folders in the local backend.

@mdisibio
Copy link
Contributor

Only when a block is cleared from the local backend do we make the check on whether or not to remove the tenant. This PR, as is, will not clean up already existing empty tenant folders in the local backend.

Does it make sense to do local folder cleanup at ingester shutdown, which could also handle this scenario?

@joe-elliott
Copy link
Member Author

joe-elliott commented Dec 13, 2022

Does it make sense to do local folder cleanup at ingester shutdown, which could also handle this scenario?

This is likely the better approach. Let me see if I can put something together 👍

@joe-elliott
Copy link
Member Author

@mdisibio i took your suggestion and implemented clean up on shutdown. let me know what you think

Signed-off-by: Joe Elliott <[email protected]>
@@ -161,9 +161,27 @@ func (rw *Backend) ReadRange(ctx context.Context, name string, keypath backend.K
return nil
}

// Shutdown implements backend.Reader
// Shutdown implements backend.Reader. It attempts to clear all tenants
// that do not have blocks.
func (rw *Backend) Shutdown() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot this existed. Excellent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. It's weird that Shutdown() on the Reader interface does the job of cleaning up, but meh. We should combine these interfaces anyway. I don't think much is gained from the split up.

(or just add Shutdown to the writer?)

@joe-elliott joe-elliott merged commit 93a264c into grafana:main Dec 15, 2022
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.

2 participants