Fixes #34344 - Add support for halting #669
Conversation
| task.state = :stopped | ||
| task.save! | ||
| def force_unlock(task = find_dynflow_task) | ||
| task.halt |
There was a problem hiding this comment.
After description in Dynflow/dynflow#406 I understand that this means:
for this task do halt the execution plan but please do finish any running steps.
Is this correct? If so, I still don't quite understand how the whole task will be marked as stopped since we remove here the task.state = :stopped? Won't it be pending even after the running steps are done?
Also, we kinda change here behaviour we had previously. Wouldn't it confuse the users even more?
UPD: Seems like the cops are after us again. Needs rebase as well.
There was a problem hiding this comment.
Is this correct?
In general, yes, but if I were to be pedantic, I'd have to say almost. There might be different interpretations of "finish any running steps" with slight nuances, but yes. If an action's #run is being executed while the execution plan is halted, the execution plan moves over to stopped, but the action's #run will continue to run until it returns. This doesn't necessarily mean the step itself will be finished.
I still don't quite understand how the whole task will be marked as stopped
There is a mechanism, which makes tasks to be updated based on the state of its corresponding execution plan as the execution plan changes states. Ideally, tasks should be read-only and the only changes should be done to them by this mechanism. Once we flip the execution plan over to stopped, this hook will trigger and update the task to stopped.
we kinda change here behaviour we had previously. Wouldn't it confuse the users even more?
The previous behaviour was seriously broken. Dynflow doesn't care about tasks' attributes at all, so if we went in and updated the state of a task, it would not really do anything. The task would appear as stopped for some time, but if the underlying execution plan was still running, it would overwrite task's status again. It worked for things like locks which relied on "is a resource locked with a locked by a task which is not stopped", but we reworked that eventually.
There was a problem hiding this comment.
Well, by reading all of your comments and the code, everything LGTM and makes sense now. Not sure if I'm missing any side effects and not sure how to test this all :/ Otherwise ACK from me.
It would be perfect if we somehow ensured we don't brake anything by that. Can our tests do that? If so, let's merge both PRs (after the conflicts are solved of course).
|
Ohh, I knew I was missing something. The new way does nothing when used against a paused execution plan. Either we can:
|
I guess this is the safest way. We could start with this, let users to try this out. But if there will be a request to merge this feature into the current behaviour then we would still need to refactor this again :/
This sounds just OK if there wasn't "somehow" :D Also, I'm kinda afraid to poke Dynflow if we can avoid that.
What benefits it could bring instead of the first approach? |
|
Went with the second option, the changes in dynflow should now work reasonably well in all possible scenarios. Now further changes should be needed here |
Requires Dynflow/dynflow#406