-
Notifications
You must be signed in to change notification settings - Fork 93
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
Intelligent Host Select. #4184
Intelligent Host Select. #4184
Conversation
tests/functional/platforms/08-intelligent-host-selection/flow.cylc
Outdated
Show resolved
Hide resolved
Copied from a comment for @oliver-sanders Intelligent Host Selection - Job Submission The idea is Cylc tries to submit the job to one host, if that fails 255 it tries the next one, if it runs out of hosts it puts the job into a submit-failed state. This means the job submission system has to remember the hosts it has previously tried. At the moment on Tim's branch this state is centralised which makes sense. This way Cylc doesn't try to use hosts which is knows are down. This is good for efficiency and also prevents flooding the system with SSH commands that are sitting around before they hit their timeout. Problems: We would have to wipe this global state at some point otherwise hosts which have come back will not be considered for any operation. Best solution I can come up with is: Reset this state periodically (use cylc.flow.main_loop.periodical) |
I've tagged multiple people in this review because I am aware that the half term hols are coming up. Feel free to poke about with this. |
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.
11/31 files viewed
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.
21/32 files viewed
tests/functional/intelligent-host-selection/01-periodic-clear-badhosts/flow.cylc
Outdated
Show resolved
Hide resolved
tests/functional/intelligent-host-selection/01-periodic-clear-badhosts.t
Show resolved
Hide resolved
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.
33/33 files viewed
tests/functional/platforms/08-intelligent-host-selection/flow.cylc
Outdated
Show resolved
Hide resolved
* 'ihs.new-go' of github.com:wxtim/cylc: (29 commits) fixed a test scan: workflow_params fix -new-window → --new-window removed stray todos actions: revert dns hack Remove old "cylc search" command. response to review Update cylc/flow/cfgspec/workflow.py Update cylc/flow/cfgspec/globalcfg.py Update cylc/flow/cfgspec/globalcfg.py Update change log. Fix func test. Remove old "cylc edit" command. Change error type. startcp and starttask exclusive; extend unit test. Update cylc/flow/scheduler_cli.py Update cylc/flow/scheduler_cli.py Update cylc/flow/scheduler_cli.py Update cylc/flow/scheduler_cli.py Fix bug in runahead release with --start-task.' ...
I don't think this message provides us with any new information? We should log:
No need to log the bad hosts with every single host-selection. |
Sorry, I've fixed this now - I honestly thought that I had already... |
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.
Couple of small comments on the rsync fallback:
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Still testing, no new issues. |
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Processes touched
|
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.
Still testing, made it most the way through ... 😓.
There is a potential bug in the rsync:fail => ssh
logic if platform
is not defined. I guess we could fallback to a plain ssh
command although that makes it easy for the platform logic to be silently ignored. Here's a diff anyway:
diff --git a/cylc/flow/subprocpool.py b/cylc/flow/subprocpool.py
index 9c533dc3a..31e925f41 100644
--- a/cylc/flow/subprocpool.py
+++ b/cylc/flow/subprocpool.py
@@ -542,15 +542,21 @@ class SubProcPool:
may cause different rsync failures depending on version, and some of
the failures may be caused by other problems.
"""
- if platform is not None:
- ssh_cmd = platform['ssh command']
-
rsync_255_fail = False
if (
ctx.cmd[0] == 'rsync'
and ctx.ret_code not in [0, 255]
and is_remote_host(ctx.host)
):
+ ssh_cmd = (
+ platform['ssh command']
+ if platform is not None
+ else 'ssh'
+ )
+ ssh_test_cmd = shlex.split(f'{ssh_cmd} {ctx.host} true')
+ LOG.info(
+ f'testing connectivity for {ctx.host} using {ssh_test_cmd}'
+ )
ssh_test = run(
shlex.split(f'{ssh_cmd} {ctx.host} true'),
capture_output=True
Save the one small issue above and the one small conflict, all good 🎉! |
- Oliver's suggested fallback for testing rsync fails - Warning to docstring of construct_rsync_over_ssh_cmd to avoid breaking subprocpool.rsync_255_fail
@oliver-sanders I think that I've responded with fff1c7b. Sorry about the close/reopen. I've going to restart my VM. It's very laggy. :( |
Going in! This was a tricky one, good work! |
These changes partially address #3827
@datamel should be credited somewhere since we pair programmed the Remote Init and Filinstall logic.
Summary of logic:
badhosts: set
toget_host_from_platform
. This is then subtracted from the platforms list of hosts and the selection is carried out on the leftover hosts.TaskRemoteMgr.badhosts = set()
, allowing us to store a list of bad hosts.get_host_from_platform
inremote.py
to usebadhosts
if they can.Caveats
WIP Checklist
TaskRemoteMgr.badhosts
) in which to store hosts which aren't accessible.TaskRemoteMgr.badhosts
from time to time.Standard Checklist
CONTRIBUTING.md
and added my name as a Code Contributor.