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
Fixes #8144 - cleanup mechanism for tasks #100
Conversation
Depends on Dynflow/dynflow#141 |
@current, @total = 0, tasks.size | ||
puts "#{@current}/#{@total}" | ||
end | ||
|
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.
extra whitespace
In the discussion on the foreman channel, @elobato suggested to add that also as a cron task with the installation by default. As from the audit value, I would like to leave on the user decision this. However, there are some tasks (as FactsImport) that don't have much value even for the auditing. Therefore, as @stbenjam suggested, the actions themselves can give a hints, if they have auditing value or not. In that case, the actions that should be cleaned automatically would look something like this: class ImportFacts < Dynflow::Action
def self.audited
false
end
end |
+1 for the |
filter_parts = [initial_filter] | ||
filter_parts << %{started_at < "#{before.ago.to_s(:db)}"} if before > 0 | ||
filter_parts << %{(state = "paused" OR state = "stopped")} | ||
filter_parts.select(&:present?).join(' AND ') |
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.
Could Arel
be used instead of the SQL?
I've extreacted the fix for duplicate rows into separate PR, to not block that fix on this one #128 |
@ares I've updated the PR, see the commit message and readme update for more details |
else | ||
[:after, :states].each do |invalid_option| | ||
if options.key?(invalid_option) | ||
raise "The option #{invalid_option} is not valid unless the filter specified" |
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.
maybe worth of README note?
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.
Updated
end | ||
|
||
desc 'Show the current configuration for auto-cleanup' | ||
task :config => 'environment' do |
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.
would you mind mentioning this in README as well?
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.
updated
5dd384d
to
77d0868
Compare
end | ||
|
||
def parse_time_interval(string) | ||
matched_string = string.gsub(' ','').match(/\A(\d+)(\w)\Z/) |
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.
probably missing string.to_s
, when I run as
foreman_tasks:cleanup FILTER='Actions::Foreman::Host::ImportFacts' AFTER=1000d NOOP=true
I get
rake aborted!
NoMethodError: undefined method `gsub' for 1000:Fixnum
/home/ares/z/foreman-tasks/lib/foreman_tasks/cleaner.rb:132:in `parse_time_interval'
/home/ares/z/foreman-tasks/lib/foreman_tasks/cleaner.rb:64:in `initialize'
/home/ares/z/foreman-tasks/lib/foreman_tasks/cleaner.rb:7:in `new'
/home/ares/z/foreman-tasks/lib/foreman_tasks/cleaner.rb:7:in `run'
/home/ares/z/foreman-tasks/lib/foreman_tasks/tasks/cleanup.rake:33:in `block (3 levels) in <top (required)>'
Tasks: TOP => foreman_tasks:cleanup => foreman_tasks:cleanup:run
EDIT:
hm, even with this, d
is stripped so get ArgumentError: Unexpected time unit in 1000, expected one of [s,h,d,y]
, not sure where it's converted to integer
EDIT2:
and now I see it :-)
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.
Should be fixed
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.
sry for force-push
This adds a rake task for cleaning tasks based on filter and age. Example: # delete all the tasks of rake foreman_tasks:cleanup FILTER='label = "Actions::Foreman::Host::ImportFacts"' # delete all tasks older than 30 days rake foreman_tasks:cleanup AFTER=30d # delete all tasks that have the auto-cleanup specified in code or settings rake foreman_tasks:cleanup By default, it only touches tasks in stopped state, this behaviour can be customized by setting the `STATES` parameter. To see the current configuration for the auto-cleanup, one can see that with: rake foreman_tasks:cleanup:config
Works very nice! ACK and 👏 tests seems to be green on most workers already. |
Fixes #8144 - cleanup mechanism for tasks
This adds a rake task for cleaning tasks based on filter and age.
Example:
delete all the tasks of
rake foreman_tasks:cleanup FILTER='label = "Actions::Foreman::Host::ImportFacts"'
delete all tasks older than 30 days
rake foreman_tasks:cleanup AFTER=30d
delete all tasks that have the auto-cleanup specified in code or settings
rake foreman_tasks:cleanup
By default, it only touches tasks in stopped state, this behaviour can be
customized by setting the
STATES
parameter.To see the current configuration for the auto-cleanup, one can see
that with:
rake foreman_tasks:cleanup:config