Fixes #21802 - Make recurring tasks more resilient #311
Conversation
| @@ -29,6 +29,15 @@ def cancellable? | |||
| end | |||
|
|
|||
| def cancel | |||
| if execution_plan && execution_plan.state == :scheduled | |||
There was a problem hiding this comment.
What would happen if, for some reason the cancelling was not successful? Let's consider that the currently planned task would be actually finishing the run and until we would get to the execution_plan!.cancel.any? method, it would already finish, including planning the next round of the action? It might be a corner case, but we should probably count on the worst case scenario here.
It seems that what we are missing in dynflow is ability to hook into the lifecycle of the action to perform some additional logic. I'm thinking of something like
class MyAction < Dynflow::Action
execution_plan_hooks.on_stop do
# do something when the execution plan stops
end
execution_plan_hooks.on_pause do
# do something when the execution plan got paused (regardless of what step caused the pause)
end
execution_plan_hooks.on_success do
# do something when the execution plan finished successfully
end
execution_plan_hooks.on_fail do
# do something when the execution plan got stopped with failed state (:error, :warning or :cancel)
end
endIn the first phase, we would probably not need to make this extendable by action, but at least a way to hook into the lifecycle of the execution plan to things like this would be quite beneficial and would allow to do things like this easier that they are right now, where we need to use things like finalize for it, which is not always the best place to put the logic, especially when dealing with handling exceptional state.
adamruzicka
force-pushed
the
fix/21802
branch
from
6c67841
to
e00db0c
Compare
app/lib/hooks/trigger_repeat.rb
Outdated
| @@ -0,0 +1,34 @@ | |||
| module Hooks | |||
| class TriggerRepeat < ::Dynflow::ExecutionPlan::Hooks::Abstract | |||
There was a problem hiding this comment.
I forgot to open a PR for it... adamruzicka/foreman_remote_execution@3bb154d
EDIT: here it is theforeman/foreman_remote_execution#325
app/lib/actions/recurring_action.rb
Outdated
| # Hook to be called when a repetition needs to be triggered. This either happens when the plan goes into planned state | ||
| # or when it fails. | ||
| def trigger_repeat(kind, execution_plan) | ||
| if (kind == :planned || execution_plan.result == :error) && execution_plan.delay_record && recurring_logic_task_group |
There was a problem hiding this comment.
What is the situation that (kind == :planned || execution_plan.result == :error) would not be true?
There was a problem hiding this comment.
Right, now that I'm thinking about it it should not happen, unless someone explicitly adds another execution_plan_hooks.use :trigger_repeat line to the action
|
Tests are failing now. Also, I guess the the bump on dynflow version requirement to No other comments code-wise. @adamruzicka could you work with @jhutar testing/validation of the behavior? |
Before this fix cancelling a task belonging to a recurring logic would make the recurring logic be active forever without triggering another repetition.
| self.started_at = string_to_time(utc_zone, data[:started_at]) unless data[:started_at].nil? | ||
| self.ended_at = string_to_time(utc_zone, data[:ended_at]) unless data[:ended_at].nil? | ||
| self.start_at = string_to_time(utc_zone, data[:start_at]) if data[:start_at] | ||
| self.start_before = string_to_time(utc_zone, data[:start_before]) if data[:start_before] |
There was a problem hiding this comment.
These things are needed since the dynflow schema normalization went it, maybe it would warrant a separate PR?
|
@jhutar feel free to ping me whenever you're ready to take a look at this |
|
While testing, I've found one issue: When cancelling a job, the next job is scheduled, but the 'next occurrence' field still stays at the previous value, compare:
Otherwise looks good |
|
Tests running at http://ci.theforeman.org/job/test_plugin_pull_request/4829/ |
|
Tested and works well, waiting for tests to finish |
|
Tests are passing. No need to wait more. Thanks @adamruzicka |
|
Direct link to the issue: https://projects.theforeman.org/issues/21802 |
Requires