SSH runner: created RemoteConnection and copy conan config to remote #18599
Conversation
conan/internal/runner/ssh.py
Outdated
| self.client.load_system_host_keys() | ||
| self.client.connect(hostname) | ||
| self.runner_output = RunnerOutput(hostname) | ||
| self.remote_conn = RemoteConnection(self.client, self.runner_output) |
There was a problem hiding this comment.
This is confusing to have both a self.client that is a SSHClient, and to have another self.remote_conn, that is an object that contains another SSHClient
There was a problem hiding this comment.
I understand your concern and agree with you.
self.client will be removed in a future PR. I'm trying to split changes from #17357 and to do the migration in an incremental way, some duplications have to coexist.
There was a problem hiding this comment.
I am fine with it, maybe just add a couple of comments:
# this self.client manages the main ssh connection
self.client = SSHClient()
# This client manages just the sftp transfers
# TODO: Integrate both client calls in one
self.remote_conn = RemoteConnection
There was a problem hiding this comment.
I understand your concern and agree with you.
self.client will be removed in a future PR. I'm trying to split changes from #17357 and to do the migration in an incremental way, some duplications have to coexist.
I'm not entirely sure it's justified to have a whole class just to wrap simple calls - however, I would argue that these are not duplicates, just have very confusing names.
self.client = SSHClient() this is very clear
self.remote_conn = RemoteConnection this not so much - this is only wrapping around paramiko's SFTPClient - which is a different class because it's a different connection. Arguably, the RemoteConnection class does not need to get a new one for each operation.
Could be two attributes, self.ssh_client and self.sftp_client - persist the latter throughout the entire execution, rather than open a new connection for each operation (see my comments elsewhere)
There was a problem hiding this comment.
Okey, I've updated the code renaming the class properties
conan/internal/runner/ssh.py
Outdated
| def _sync_conan_config(self): | ||
| # Transfer conan config to remote | ||
| self.remote_conn.put_dir( | ||
| self.conan_api.config.home(), |
There was a problem hiding this comment.
What folder is this sending, exactly? I would probably have something more explicit rather than anything by exclusion -
We could stand simple and just sync the profiles folder (maybe the remotes and maybe global.conf)
we should probably think ahead - there's a chance we'll have to be more selective (any configs that have paths that are relevant to the "local" machine will not work on the remote runner!)
There was a problem hiding this comment.
That is a good approach. I've updated the method to only copy:
- profiles folder
- settings.yml
- settings_user.yml
- global.conf
- remotes.json
I've added a TODO note to check for local path references on each file before copying. If you agree, I can implement a basic version of that functionality in this PR, or I can leave it for a future (and isolated) PR.
conan/internal/runner/ssh.py
Outdated
| continue | ||
| if item.is_file(): | ||
| self.runner_output.verbose(f"Copying file {item.as_posix()} to {dest_item}") | ||
| self.put(item.as_posix(), dest_item) |
There was a problem hiding this comment.
we are opening a new sftp connection for each file, which doesn't feel right, i would open/close a new connection for every single file operation.
I would rename this class sftpClient or something to that effect, keep the paramiko object as an attribute (passed to the constructor), and persist it instead of calling open_sftp() from this class
There was a problem hiding this comment.
That is a good improve in performance, thank you!
conan/internal/runner/ssh.py
Outdated
| try: | ||
| sftp.mkdir(folder) | ||
| except IOError: | ||
| if ignore_existing: |
There was a problem hiding this comment.
is it possible for the IOError exception to be raised by something else other than the directory existing??
There was a problem hiding this comment.
Not in the context of mkdir
There was a problem hiding this comment.
Is the error type different if you don't have permission to create the folder? The question is if you can hide a permissions error if you have ignore_existing=True.
There was a problem hiding this comment.
I've updated the code with a more exhaustive exception check, thanks!
| self.conan_api.cache.restore(local_cache_tgz) | ||
|
|
||
|
|
||
| class RemoteConnection: |
There was a problem hiding this comment.
do we want to allow Python annotations in this class?
There was a problem hiding this comment.
I cannot answer this to you, IMHO I prefer annotations
5 participants
Changelog: omit
Docs: omit
Adapt changes from #17357
Partially migrated plain
sfptdirect usages and copy the full conan config (without the cache packages) to the remote machine instead of just copying the build/host profiles.