Skip to content
This repository was archived by the owner on Jul 28, 2026. It is now read-only.

Clean up abandoned WAL directories - #304

Merged
56quarters merged 6 commits into
masterfrom
walgc
Jan 8, 2021
Merged

Clean up abandoned WAL directories#304
56quarters merged 6 commits into
masterfrom
walgc

Conversation

@56quarters

@56quarters 56quarters commented Dec 30, 2020

Copy link
Copy Markdown
Contributor

Remove WAL directories that are no longer associated with a ManagedInstance
that this agent is responsible for. The storage directories used by active
instances are compared to all subdirectories under the wal_directory root.
Any that are not associated with an instance and that haven't been modified
in over a configured amount of time are removed.

How often the check is run and the cutoff for how recently a WAL must have
been modified are controlled by the wal_cleanup_period and
wal_cleanup_age settings respectively, under the top-level Prometheus
configuration. When omitted, they default to a period of 30 minutes and
recently modified threshold of 12 hours.

Fixes #132

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@CLAassistant

CLAassistant commented Dec 30, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@56quarters
56quarters marked this pull request as draft December 30, 2020 19:38
@56quarters

Copy link
Copy Markdown
Contributor Author

@rfratto I'm not really sure about the best way to integrate this into the rest of the agent. My initial thought was that a Cleaner instance would be a member of the Agent struct. The goroutine to run it periodically would be started on construction of the Agent struct. Your thoughts?

@rfratto

rfratto commented Dec 30, 2020

Copy link
Copy Markdown
Contributor

Thanks for working on this!

I think creating it from the Agent struct is fine, but I think I'd prefer the goroutine to be launched by NewCleaner with a Stop method that signals the goroutine to exit. I'd like to keep the Agent acting as just the glue for other pieces of logic and not have any real logic itself. We do something similar in newManager, where it launches its own goroutine and has a Stop method to close it.

@56quarters
56quarters force-pushed the walgc branch 5 times, most recently from 07fbabe to a249f81 Compare January 5, 2021 20:58
@56quarters 56quarters changed the title [WIP] Clean up abandoned WAL directories Clean up abandoned WAL directories Jan 5, 2021
@56quarters
56quarters force-pushed the walgc branch 2 times, most recently from 159a2f7 to 809042a Compare January 5, 2021 22:47
@56quarters
56quarters marked this pull request as ready for review January 5, 2021 22:57
@56quarters
56quarters requested a review from rfratto January 5, 2021 22:57
Comment thread pkg/prom/wal/util.go Outdated

@rfratto rfratto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this, this will be great for people using the scraping service! 🎉 This is pretty close to a mergeable state, though I think there's some work that we can do to make the code a bit smaller.

Aside from my smaller pieces of feedback, I have one larger one. Some linters require function and type comments to be commented in the "godoc" style which are strings optionally starting with A/An/The and the name of the thing being commented:

// The Cleaner is responsible for ...
type Cleaner struct {}

// WithCleanerMinAge overrides the default age after which ....
func WithCleanerMinAge(d time.Duration) WALCleanerOpts 

We don't have that linting rule enabled (apparently, though I thought we did) and it only applies the restriction to exported functions/types, but I personally like doing it consistently for all types and functions. Can you make sure that at least all exported types and functions you added are written in this style?

Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go
Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go
Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go Outdated
Comment thread pkg/prom/cleaner.go Outdated

@rfratto rfratto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thank you! Once you rebase and fix the merge conflict, feel free to merge.

Remove WAL directories that are no longer associated with a `ManagedInstance`
that this agent is responsible for. The storage directories used by active
instances are compared to all subdirectories under the `wal_directory` root.
Any that are not associated with an instance and that haven't been modified
in over a configured amount of time are removed.

How often the check is run and the cutoff for how recently a WAL must have
been modified are controlled by the `wal_cleanup_period` and
`wal_cleanup_age` settings respectively, under the top-level Prometheus
configuration. When omitted, they default to a period of 30 minutes and
recently modified threshold of 12 hours.

Fixes #132
@56quarters
56quarters merged commit 6cf3d9e into master Jan 8, 2021
@56quarters
56quarters deleted the walgc branch January 8, 2021 16:33
@mattdurham mattdurham mentioned this pull request Sep 7, 2021
3 tasks
mattdurham pushed a commit that referenced this pull request Nov 11, 2021
Remove WAL directories that are no longer associated with a `ManagedInstance`
that this agent is responsible for. The storage directories used by active
instances are compared to all subdirectories under the `wal_directory` root.
Any that are not associated with an instance and that haven't been modified
in over a configured amount of time are removed.

How often the check is run and the cutoff for how recently a WAL must have
been modified are controlled by the `wal_cleanup_period` and
`wal_cleanup_age` settings respectively, under the top-level Prometheus
configuration. When omitted, they default to a period of 30 minutes and
recently modified threshold of 12 hours.

Fixes #132
@github-actions github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 22, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scraping service: WAL management

3 participants