Add support for global size-based retention policy #628
Conversation
| case b := <-f.evictCh: | ||
| b.evicted, b.err = f.blockQuerier.evict(b.blockID) | ||
| close(b.done) |
There was a problem hiding this comment.
Not related to the PR, but: to me it's not entirely clear whether the head block is visible to queries in-between Flush and runBlockQuerierSync that should pick the new block:
case <-f.Head().flushCh:
if err := f.Flush(ctx); err != nil {
level.Error(f.logger).Log("msg", "flushing head block failed", "err", err)
continue
}
f.runBlockQuerierSync(ctx)
kolesnikovae
force-pushed
the
feat/size-based-retention-policy-enforcement
branch
from
d0dc191 to
0d8b636
Compare
| defaultMinFreeDisk = 10 * 1024 * 1024 * 1024 // 10Gi | ||
| defaultMinDiskAvailablePercentage = 0.05 | ||
| defaultRetentionPolicyEnforcementInterval = 5 * time.Minute |
There was a problem hiding this comment.
I think it's worth making these parameters configurable
There was a problem hiding this comment.
I tend to agree, it will always be a balance between flexibility and making Pyroscope complex to operate, but being able to change that or disable deletion all together is more than a nice to have. That said I wouldn't mind if you do this in here or in a follow up PR.
| // cleanup there to avoid deleting all blocks when disk usage reporting | ||
| // is delayed. | ||
| if volumeStatsPrev != nil && volumeStatsPrev.BytesAvailable >= volumeStatsCurrent.BytesAvailable { | ||
| level.Warn(e.logger).Log("msg", "disk utilization is not lowered by deletion of a block, pausing until next cycle") |
There was a problem hiding this comment.
defaultRetentionPolicyEnforcementInterval = 5 * time.Minute
Just a note here, for my own Laptop, the filesystem btrfs is fairly slow to reflect the free disk change (or doesn't at all because of snapshots). So that basically means we would delete a block every 5 minutes. Which I think is ok and better than deleting all blocks because the free disk space doesn't go down.
There was a problem hiding this comment.
Oh, that's a great point, which worth mentioning in docs. As a small optimization, I'll implement batch removal that relies on the actual block size on disk.
|
Thank you so much for the review and help with testing @simonswine! |
kolesnikovae
changed the title
* feat: support for global size-based retention policy * feat: use manager for ingester subservices * add querier block eviction test * decouple retention enforcer and ingester
Resolves #612.
One of the drawbacks is that mount points and symlinks are not honoured: if one uses a separate file system for a specific tenant (or a group), this fact is completely ignored. An ultimate solution to this would be tracking each file system independently. As a workaround, we could make it so that a user can exclude tenants from this global policy.
This change also allows implementing per-tenant retention policies in an easy way.