-
Notifications
You must be signed in to change notification settings - Fork 230
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
Add support for DLO automatic cleanup #278
Conversation
|
This is WIP. Let me know if you're interested in this and I will add tests + README.md updates. |
Gemfile
Outdated
| gem 'puppetlabs_spec_helper', :require => false | ||
| # Pinning due to bug in newer rspec with Ruby 1.8.7 | ||
| gem 'rspec-core', '3.1.7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I run into some issues while running the tests locally. For debugging I removed the pinning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to support such an old Ruby version anymore, so this should be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased against the release branch to get all this fixed. I hope that was okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still discussing the details around the customization options for cleanup frequency. I'll get back to this PR as soon as I have more information. Thanks for your contributions!
manifests/server.pp
Outdated
| -> systemd::unit_file{'puppetdb-cleanup.timer': | ||
| content => epp("${module_name}/puppetdb-DLO-cleanup.timer.epp"), | ||
| } | ||
| -> service{'puppetdb-cleanup.timer': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a ~> so that the service will be restarted if the puppetdb-cleanup.timer file changes.
As an alternative, it looks like the ~> service block can be omitted entirely with something like this (which I pulled from the README)
systemd::unit_file { 'foo.service':
source => "puppet:///modules/${module_name}/foo.service",
enable => true,
active => true,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After months of work, this feature is finally released. It wasn't available when when I created this PR. updated the code.
manifests/server.pp
Outdated
| if $facts['systemd'] { | ||
| # deploy a systemd timer + service to cleanup old reports | ||
| # https://puppet.com/docs/puppetdb/5.2/maintain_and_tune.html#clean-up-the-dead-letter-office | ||
| systemd::unit_file{'puppetdb-cleanup.service': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename this to puppetdb-dlo-cleanup.service (and the matching timer one below) in case we want to add another cleanup service in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, done
manifests/server.pp
Outdated
| } | ||
| -> service{'puppetdb-cleanup.timer': | ||
| ensure => 'running', | ||
| enable => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the enable option is necessary here, but I could be mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be enabled. Otherwise the timer is off after the next reboot and the upcoming puppet run would start it.
|
|
||
| [Timer] | ||
| # Daily at 8am | ||
| OnCalendar=*-*-* 08:0:00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should choose the time of day via fqdn_rand. If there are multiple PuppetDB VMs running on the same physical machine this will prevent them from hammering the disk at the same time.
Also the comment will no longer be accurate, so you can remove it or replace it with a general comment about running once a day with a link to documentation on the systemd timer syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could select the hour with fqdn_rand(24) and the minutes with fqdn_rand(60).
| @@ -297,6 +298,23 @@ | |||
| } | |||
| } | |||
|
|
|||
| if $automatic_dlo_cleanup { | |||
| if $facts['systemd'] { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If systemd does not exist, lets try setting up a cron job to clean this up instead. For example Redhat 6 has cron but not systemd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
@bastelfreak These comments are done and ready for you to take a look at. |
|
|
||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/usr/bin/find /opt/puppetlabs/server/data/puppetdb/stockpile/discard/ -type f -mtime +90 -delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, one final note. For security reasons, this service should run as the PuppetDB user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added user and group
cd9f333
to
1923209
Compare
|
Personal reminder:
|
|
Awesome! This looks good. If you need any help writing tests just let us know and we can help out. I would say these would be nice.
And if you add DLO age as variable, a test that that can be customized. |
8b60736
to
7a2bc12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments with respect to the travis and sh bits. Thanks.
.travis.yml
Outdated
| - rm -f Gemfile.lock | ||
| - gem update --system | ||
| - gem --version | ||
| - bundle -v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest either moving these to a helper script,
before_install: some/where/prep-testsor using a different approach, i.e. say with one of the yaml multi-line sytaxes.
For example, using ">":
before_install: >
set -ex
&& bundle -v
&& rm -f Gemfile.lock
&& ...or using "|"
before_install: |
set -ex
bundle -v
rm -f Gemfile.lock
...(Note that syntax is just approximate -- haven't checked it.)
Otherwise, travis will just keep going iirc. That is, it runs all the bullet points, even if one of them fails, only stopping if there was a failure after it has tried them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.travis.yml
Outdated
| - gem --version | ||
| - bundle -v | ||
| script: | ||
| - 'bundle exec rake $CHECK' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do we need the single quotes, and regardless, wonder if we should quote the CHECK expansion as "$CHECK", just to avoid having to worry about the content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@bastelfreak, sorry but would you be able to rebase this on master again? We ended up making some changes/rebasing the existing pr we had up before it merged. Thanks for all the help! |
|
@austb can you review this again please? |
There is also a small change to a string that needed to be double quoted.
|
Thanks @bastelfreak for the PR! |
No description provided.