-
Notifications
You must be signed in to change notification settings - Fork 94
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
File installation via rsync and install target installation added #3796
File installation via rsync and install target installation added #3796
Conversation
Note, I will do a read through/double check doc strings and fix a broken functional test that I have written (tests/f/remote/03-install-target.t) |
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.
Looks good, some RST formatting comments...
3790acf
to
ac029d0
Compare
3f57dce
to
45e3263
Compare
45e3263
to
a0f0d1b
Compare
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.
Finished code review, some minor comments:
3e42882
to
6f73a25
Compare
The new logging is working fine, logs roll over when the suite restarts. One small niggle is that the log files are created a few seconds apart so their timestamps don't match but we can address that later on.
|
dc05f4c
to
c3e47ba
Compare
Yes, that is a bit of a niggle. I guess it would be nicer for the suite log and the file-installation-log timestamps to match. I can put an issue up and assign myself so it doesn't hold this up if you like? |
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.
Tests pass for me (with the cylc version
diff I sent you), looks good, works as expected.
Few more comments which could be addressed on this PR if time permits but which needn't delay merge.
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.
A bunch of minor comments and suggestions, but generally looking great 🎉
The conflicting files
have been moved to |
Thanks @wxtim! |
c279af4
to
cb56b54
Compare
Fix log level in ssh and rename key methods
Fix reporting of rsync logs
1e805fc
to
71d47ea
Compare
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.
🎉 nice work @datamel
Install by target
Remote installation is now on a per
install target
rather than a perplatform
basis. Targets can be configured in the global.cylc e.g.REMOTE_INIT_FAILED
where a key for a different target is found on the remote.Existing keys are identified by filename (where the install target is now added to it). The initial implementation was through the key metadata. However, it turns out that metadata goes in the secret key only and not the public key.
There is now no mechanism for checking for a shared file system because uuid file has been removed. This now has to be configured in global.cylc.
Notes on call to task_remote_mgr.remote_init(...)
Previously, platform name was being sent to
remote_init
and then a skeleton platform dictionary was generated from that. We're now sending a complete dictionary rather than regenerating.In
restart_remote_init
we were using aset
to pick distinct platforms. As platform is a dictionary and not an object (where we could add a hash), the method is reworked to create a list of platforms filtered as distinct install targetsFile install via Rsync
File installation is now via rsync. Files to be installed are configurable in the flow.cylc file (translated to rsync include and filter rules).
File install is still a push but happens after remote initialisation in the callback because we need to know whether or not the file install is required before we push files across.
Add files to the rsunc in the flow.cylc:
This currently supports top level directories and files - I have also added validation for this feature.
Rsync Logging
New RSYNC_LOG log which will be created in the suite log directory. Any errors from the file installation will appear in the suite log.
These changes close #3687
CONTRIBUTING.md
and added my name as a Code Contributor.