Retrying remote-init on failure from restart - #7379
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves restart behavior for remote initialization by retrying remote-init / file installation when the remote step returns an SSH 255 condition, and adds an integration test to cover the new retry behavior.
Changes:
- Update
Scheduler.manage_remote_initto retryremote-init/ file install on 255, instead of dropping the install target from restart tracking. - Add an integration test that exercises the retry and cleanup behavior across the relevant status transitions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cylc/flow/scheduler.py |
Adjusts restart-time remote init management to retry on 255 and only remove targets on completion/fatal failure. |
tests/integration/test_scheduler.py |
Adds coverage for the retry-on-255 restart behavior and expected map cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif status == REMOTE_INIT_255: | ||
| # Remote init failed due to unreachable host, retry. | ||
| del remote_mgr.remote_init_map[install_target] | ||
| remote_mgr.remote_init(platform) | ||
| elif status == REMOTE_FILE_INSTALL_255: | ||
| # File install failed due to unreachable host, retry. | ||
| del remote_mgr.remote_init_map[install_target] | ||
| remote_mgr.file_install(platform) |
There was a problem hiding this comment.
Have gotta feeling that deleting the remote_init_map entry deletes the state of all prior progress.
I.e, For REMOTE_FILE_INSTALL_255 that would delete the state which indicated successful remote-init.
Do we need to delete the remote_init_map state to force retry, or is it enough to just call the relevant method?
There was a problem hiding this comment.
Not sure, ill have a play.
There was a problem hiding this comment.
Both remote_init() and file_install() set the entry in the map almost immediately, so I don't think the del is needed before either
There was a problem hiding this comment.
I think you are correct. When I tested this I briefly saw some odd behaviour but that might be another bug. It seems to work, but when I clicked on a running task in the GUI, and asked it to show the log, it just gave me an error that it couldn't connect to bad1.
^^ Different issue. we can remove the del lines.
|
Is there an issue linked to this? If not it would be useful to include some more details on the original post explaining the problem statement |
Yep sorry I forgot to link it. It's #7286, now added to the PR |
|
Note GH doesn't link PRs that are against non-master branch to the issue in the sidebar and it has to be done manually (and in this case the issue wouldn't come up in the search for some reason, so had to go to the issue and link the PR instead) 
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
MetRonnie
left a comment
MetRonnie
left a comment
There was a problem hiding this comment.
Thanks, I understand how to repro now and have confirmed this fixes it. The del statements ought to be removed as not needed (and I think the test will need adjusting): https://github.com/cylc/cylc-flow/pull/7379/changes#r3644924502
…atus. Removed the matching assert lines in the relevant integration test.
Closes #7286
Added some re-try logic when remote-init failures are returned following a restart.
Ran isort
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.