Fixes #20819 - Allow turning task cleanup cron on and off #582

adamruzicka · 2017-08-31T11:11:33Z

to allow easy toggle of the cleanup, so far opt-in. For discussion see theforeman/foreman-tasks#247

theforeman-bot · 2017-08-31T11:11:35Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-08-31T11:27:36Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ekohl

It might make sense to revisit #484 and determine the automatic_cleanup on the config value cleanup_after.

ekohl · 2017-08-31T11:38:53Z

manifests/plugin/tasks.pp

@@ -20,4 +23,22 @@
    enable => true,
    name   => $service,
  }
+  $cron_content = @(END)


I think it's cleaner to move this to files/tasks.cron and use content => file('foreman/tasks.cron').

ekohl · 2017-08-31T11:40:07Z

manifests/plugin/tasks.pp

+             45 19 * * *     foreman    /usr/sbin/foreman-rake foreman_tasks:cleanup >>/var/log/foreman/cron.log 2>&1
+             | END
+  file { 'foreman-tasks-cleanup-cron':
+    ensure  => if $automatic_cleanup {


The inline conditional is not that readable so I'd prefer a separate variable.

ekohl · 2017-08-31T11:42:02Z

manifests/plugin/tasks.pp

 class foreman::plugin::tasks(
-  String $package = $::foreman::plugin::tasks::params::package,
-  String $service = $::foreman::plugin::tasks::params::service,
+  String  $package = $::foreman::plugin::tasks::params::package,


We don't align parameters anymore. Too much changes, huge diffs for little benefit.

theforeman-bot · 2017-08-31T11:46:51Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-08-31T11:46:53Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-08-31T13:27:28Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

adamruzicka · 2017-08-31T13:29:47Z

I'm not against revisiting the other foreman-tasks PR, but we would probably still keep this on/off option for the cleaner, because of changes in the cleaner itself.

ekohl

I'm not against a parameter.

ekohl · 2017-08-31T13:43:17Z

manifests/plugin/tasks.pp

@@ -20,4 +23,15 @@
    enable => true,
    name   => $service,
  }
+  $cron_state = if $automatic_cleanup {


The following syntax is a bit more common:

$cron_state = $automatic_cleanup ? { true => 'present', default => 'absent', }

ekohl · 2017-08-31T13:45:39Z

manifests/plugin/tasks.pp

+  file { 'foreman-tasks-cleanup-cron':
+    ensure  => $cron_state,
+    owner   => 'root',
+    path    => '/etc/cron.d/foreman-tasks-cleanup',


Would foreman-tasks be more future proof? There could be another cronjob. We also often put the path in the file resource: file { '/etc/cron.d/foreman-tasks': }

ekohl · 2017-08-31T13:49:10Z

manifests/plugin/tasks.pp

+  }
+  file { 'foreman-tasks-cleanup-cron':
+    ensure  => $cron_state,
+    owner   => 'root',


Could you add the group => 'root' as well? Otherwise Puppet will try to derive it based on the file owner of the manifest which could be different. An explicit mode might be a good idea as well.

theforeman-bot · 2017-08-31T13:49:17Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-08-31T13:49:19Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2017-08-31T13:57:37Z

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #16922: cronjob to clear old tasks is the correct one
move ticket #16922 from foreman-tasks to the Installer project
or file a new ticket in the Installer project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

ekohl · 2017-08-31T14:13:01Z

manifests/plugin/tasks.pp

+    ensure  => $cron_state,
+    owner   => 'root',
+    group   => 'root',
+    mode    => 'u=rw,go=r',


Usually we write 0644 but this works too I believe.

ekohl · 2017-08-31T14:13:16Z

manifests/plugin/tasks.pp

+    owner   => 'root',
+    group   => 'root',
+    mode    => 'u=rw,go=r',
+    path    => '/etc/cron.d/foreman-tasks',


This is now redundant since you have it in the title.

ekohl · 2017-08-31T14:15:11Z

files/tasks.cron

+FOREMAN_HOME=/usr/share/foreman
+
+# Clean up expired tasks from the database
+45 19 * * *     foreman    /usr/sbin/foreman-rake foreman_tasks:cleanup >>/var/log/foreman/cron.log 2>&1


Usually we have line endings at the end of the file, this results in smaller diffs if you add another line.

ekohl

Is this still pending the tasks pr or is this already something that is useful?

ekohl · 2017-09-04T11:46:45Z

files/tasks.cron

+FOREMAN_HOME=/usr/share/foreman
+
+# Clean up expired tasks from the database
+45 19 * * *     foreman    /usr/sbin/foreman-rake foreman_tasks:cleanup >>/var/log/foreman/cron.log 2>&1


adamruzicka · 2017-09-05T07:30:16Z

Still pending the tasks PR

ekohl · 2017-09-06T09:38:26Z

Ok, added the label Waiting on contributor for now.

ehelms · 2017-10-13T16:59:16Z

I anticipate we are going to get requests to be able to customize the cron timestamp. While users could hand edit it, the moment they run the installer again this would get wiped out. I'd recommend solving this now rather than later by introducing a variable and templatizing the cron.

adamruzicka · 2017-10-16T09:02:39Z

Converted the cron file to a template, hope having the entire cronline as a single string is enough.

ekohl · 2017-10-16T13:09:44Z

templates/tasks.cron.erb

+FOREMAN_HOME=/usr/share/foreman
+
+# Clean up expired tasks from the database
+<% cronline = scope.lookupvar("::foreman::plugin::tasks::cron_line") %>


cronline is in the local scope so @cronline also works.

~~Chmm, the tests don't seem to like this change~~
pebcak...

mmoll · 2017-10-16T18:33:42Z

manifests/plugin/tasks/params.pp

@@ -1,5 +1,7 @@
 # Data for the foreman-tasks plugin
 class foreman::plugin::tasks::params {
+  $automatic_cleanup = false
+  $cron_line = "45 19 * * *"


manifests/plugin/tasks/params.pp:4:WARNING: double quoted string containing no variables

ekohl · 2017-10-18T11:40:58Z

templates/tasks.cron.erb

+FOREMAN_HOME=/usr/share/foreman
+
+# Clean up expired tasks from the database
+<%= @cron_line %>    foreman    /usr/sbin/foreman-rake foreman_tasks:cleanup >>/var/log/foreman/cron.log 2>&1


The foreman username is also available as the variable foreman::user.

ehelms · 2017-10-19T12:57:08Z

Templating and configuration aspect look good, thanks @adamruzicka for making that change

mmoll · 2017-10-19T17:16:20Z

merged, díky @adamruzicka!

adamruzicka · 2017-10-19T17:19:50Z

Thanks everybody who helped me pull this one through

theforeman-bot added the Waiting on contributor label Aug 31, 2017

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 31, 2017

ekohl requested changes Aug 31, 2017

View reviewed changes

theforeman-bot added Waiting on contributor labels Aug 31, 2017

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Aug 31, 2017

ekohl requested changes Aug 31, 2017

View reviewed changes

theforeman-bot added the Waiting on contributor label Aug 31, 2017

theforeman-bot added Waiting on contributor labels Aug 31, 2017

theforeman-bot removed the Waiting on contributor label Aug 31, 2017

adamruzicka added 4 commits August 31, 2017 16:00

Fixes #20819 - Allow turning task cleanup cron on and off

81ad59c

Fixes #20819 - Make puppet lint happy

c0c45c6

Fixes #20819 - Address comments

6f2d432

Fixes #20819 - Address comments, vol. 2

9d08f3a

adamruzicka force-pushed the cron-tasks-cleanup branch from 4e09b6f to 9d08f3a Compare August 31, 2017 14:01

adamruzicka changed the title ~~Fixes #16922 - Allow turning task cleanup cron on and off~~ Fixes # - Allow turning task cleanup cron on and off Aug 31, 2017

adamruzicka changed the title ~~Fixes # - Allow turning task cleanup cron on and off~~ Fixes #20819 - Allow turning task cleanup cron on and off Aug 31, 2017

ekohl reviewed Aug 31, 2017

View reviewed changes

ekohl added the Waiting on contributor label Aug 31, 2017

Fixes #20819 - Address comments, vol. 3

2051b44

theforeman-bot removed the Waiting on contributor label Sep 4, 2017

ekohl approved these changes Sep 4, 2017

View reviewed changes

ekohl added the Waiting on contributor label Sep 6, 2017

This was referenced Oct 8, 2017

Refs 16922 - Add cronjob to clear old tasks [deb] theforeman/foreman-packaging#1618

Closed

Refs #16922 - Add cronjob to clear old tasks [rpm] theforeman/foreman-packaging#1616

Closed

Fixes #20819 - Template the cron job

f1da250

theforeman-bot removed the Waiting on contributor label Oct 16, 2017

ekohl reviewed Oct 16, 2017

View reviewed changes

adamruzicka added 2 commits October 16, 2017 15:24

Fixes #20819 - Cleaner template

c5435f5

Fixes #20819 - Cleaner template, vol. 2

cfe2f1e

ekohl approved these changes Oct 16, 2017

View reviewed changes

mmoll suggested changes Oct 16, 2017

View reviewed changes

theforeman-bot added the Waiting on contributor label Oct 16, 2017

Fixes #20819 - Double to single quotes where not needed

ecf3a5e

theforeman-bot removed the Waiting on contributor label Oct 17, 2017

ekohl reviewed Oct 18, 2017

View reviewed changes

Fixes #20819 - User foreman::user variable

c23ec4a

mmoll approved these changes Oct 19, 2017

View reviewed changes

mmoll merged commit 56d196b into theforeman:master Oct 19, 2017

adamruzicka deleted the cron-tasks-cleanup branch October 19, 2017 17:19

ekohl added the Enhancement label Jan 25, 2018

Fixes #20819 - Allow turning task cleanup cron on and off #582

Fixes #20819 - Allow turning task cleanup cron on and off #582

Conversation

adamruzicka commented Aug 31, 2017

theforeman-bot commented Aug 31, 2017

theforeman-bot commented Aug 31, 2017

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Aug 31, 2017

theforeman-bot commented Aug 31, 2017

theforeman-bot commented Aug 31, 2017

adamruzicka commented Aug 31, 2017

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Aug 31, 2017

theforeman-bot commented Aug 31, 2017

theforeman-bot commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka commented Sep 5, 2017

ekohl commented Sep 6, 2017

ehelms commented Oct 13, 2017

adamruzicka commented Oct 16, 2017

Choose a reason for hiding this comment

adamruzicka Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Oct 19, 2017

mmoll commented Oct 19, 2017

adamruzicka commented Oct 19, 2017

adamruzicka Oct 16, 2017 •

edited