Include active broken-connection/irresponsive-workflow checks - #851
Include active broken-connection/irresponsive-workflow checks#851dwsutherland wants to merge 6 commits into
Conversation
|
Will sort tests in morning .. I could just change socket testing from testing PUB to REQ/RES server |
74d55e0 to
a71b2b5
Compare
|
Pushed a related fix .. The workflow manager wasn't disconnecting the workflow and cleaning it up properly when the workflow stopped, only the data-store manager was disconnecting properly. This effected a fix I had in.. |
7f01878 to
0b87d67
Compare
|
I've amended the description, and added tasks (see above). These are reduced to:
And I've removed the socket testing (as that's covered by the REQ/RES/PUB/SUB test) and the direct contact file deletion.. But am yet to add actual integration tests.. Will work on that this afternoon. |
| } | ||
| # ensure workflows get properly dealt with by the workflow manger | ||
| active_before |= w_stops | ||
| inactive_before -= w_stops |
There was a problem hiding this comment.
This is to ensure that the workflow manager actually calls it's _disconnect method, and cleans up the workflow record on it's side.. (the _disconnect wasn't been called at all, so the self.workflows[w_id].get('req_client') was not being set to None)
There was a problem hiding this comment.
I don't understand this change, stopped workflows should already be getting detected.
Could you give me an example where this is a problem to help me understand and review?
| """ | ||
| self._disconnect_workflow(w_id, update_contact) | ||
|
|
||
| def _disconnect_workflow(self, w_id, update_contact=True): |
There was a problem hiding this comment.
_disconnect_workflow can be called from multiple places, so only wanted @log_call to activate once (when the workflow manager cleans up it's records and calls it's own _disconnect)
fac21a4 to
b424334
Compare
|
Hi, I raised the question of proactive crashed workflow detection at our end...
This is a very different issue to #845, and would require a cylc-flow interface, so we should split the #845 bugfix (target 1.9.x) from the proactive crash detection (target 1.10.0). |
|
I have extracted the part of this PR which addresses #845 into PR #856 so we can get it into 1.9.3 quickly - will try to get that released today as there are Cylc Review fixes we need. I've chalked up a new issue to cover the proactive detection of crashed workflows that the remainder of the PR addresses - #856 There are some vague ideas around this, but we haven't managed to give it much time yet. One idea outlined was to get cylc-flow to The UIS might want to do something more networky if there are scenarios where disconnecting/reconnecting to the same scheduler instance might be advantageous. E.g, if there is some unrecoverable ZMQ or networking issue and the connection needs to be reset? |
I already removed this part from this PR (and the socket test).
This is the primary purpose of the ping/pong, to detect and refresh/reconnect after possible networking/ZMQ issues.. Not to detect crashed workflows, perhaps that can be another effort. If the workflow doesn't respond after two ping/pong attempts, then it is reset.. (set to
Given my above responses, can we not just get this in now.
Also, the ping/pong alone would address #845 , because the restarted workflow means the existing-UIS/old connections don't work anymore... So it would reestablish the workflow at the UIS. Another thing, it's important for a (above mentioned) problem found at ESNZ, so also want this in ASAP. |
|
This ping solution requires a cylc-flow release so we can't get it in and released today. I think there are some crossed wires here. The issue in #845 is about the contact file changing between scans, the ping-pong mechanism cannot detect this (it's checking connection timeout not scanning the contact file) but your other change to the workflows mgr does. So I've cherry-picked this change for quick merge and release.
I don't think this issue requires anything more than #856 to fix, here's my test to reproduce:
Example GUI log: The detects the removal of the contact file and disconnects the workflow as expected. We can prove that it's working correctly by restarting the workflow and requesting a UIS scan - the UIS will then successfully reconnect. (note this is a subtly different scenario to #845 as the workflow is detected as stopped in one scan and restarted in another)
I've not yet heard of issues with ZMQ connections, have you got any details on this or approaches to reproduce or methods to test? I've opened a stub issue for this problem, please add any details you can: #857 A ping-pong seems like a reasonable solution to this problem. Out of interest, why did you go down the route of implementing a bespoke low-level interface for this rather than using the basic GraphQL request interface used by |
There was a problem hiding this comment.
The /active state transition will cause:
- disconnect
- unregister
- register
- connect
We want the disconnect/reconnect, but we don't want the unregister/register (this yields pruned/added deltas). The workflow hasn't been deleted (unregistered), we're just re-attempting the connection (which may well fail).
Note that if the connection does fail, it will activate the detect_old_contact_file logic via the timeout_handler, so this is equivalent to automatically running detect_old_contact_file in a loop 🤦.
b424334 to
fa76c05
Compare
fa76c05 to
facae3e
Compare
facae3e to
36d9a74
Compare
36d9a74 to
13046b8
Compare



closes #857
There were two problems:
DB restarts don't change theCYLC_WORKFLOW_UUID, and this was the criteria for reestablishing the UIS workflow.old contact files of inactive workflows left an old subscription/workflow in the UIS.These are addressed here by using PID as an alternate criteria,and testing the PUB tcp listener of workflows (which will help clear old contact files and/or reestablish subscriptions)Update:
I have an idea for another check, however, it would need some back compat added.. It would essentially check both the REQ/RES PUB/SUB are still connected/working.
I would create a new endpoint on the
cylc-flowside that would take a REQ, and pass it back via the PUB. We would then pick that up via the existing UIS SUB, and record the timestamp. This would happen periodically (say every ~5min?), and any workflow with a last recorded time older than some threshold (say 11min) would be disconnected/unregistered/purged, and re-registered on the next scan (if available).Why? Because it's good to check the existing connections (not just create new ones), and also another problem I've seen:
Although I could investigate whether ZeroMQ has some kind of default inactivity timeout on connections, I think this will act as a check for any other connection disruptions.
I will crack on with it.
Tasks:
Add criteria checks (PID,HOST) in addition toUUID.Sibling PR cylc/cylc-flow#7355
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.