SSH Runner: Fix several issues and refactor code #17357

perseoGI · 2024-11-21T09:14:37Z

Changelog: Feature: Introducing SSH runner
Docs: conan-io/docs#3972

Changes:

Fix creation of virtual environment
Retrieve current conan client version and install remotely if it does not exist
Error checks
Create a custom SSH Logger based on ConanOutput
Properly manage build and host profiles, copying to the runner machine if needed and keeping the jinja2 templates intact to allow remote runner to render them locally
General code refactor: some improvements like typing and simplifying code flow
Fix cache inconsistency when a package have been created remotely and already restored the cache locally. If the package is removed locally, the runner was never bringing the package back to local. This happens because it only compress the build recipes. Solved by always exporting all the packages and forcing a local cache refresh
Correctly manage special characters like & and * during ssh remote invoques
Add different runner types detection on profile composition

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

conan/api/subapi/cache.py

conan/internal/runner/ssh.py

conan/api/subapi/cache.py

paramiko/paramiko#2474 (comment)

memsharded · 2025-02-19T12:56:44Z

test/functional/command/runner_test.py

    assert "Restore: pkg/0.2:8631cf963dbbb4d7a378a64a6fd1dc57558bc2fe" in client.out
    assert "Restore: pkg/0.2:8631cf963dbbb4d7a378a64a6fd1dc57558bc2fe metadata" in client.out
    assert "Removing container" in client.out
+


Please enable pep-8 style checks in your IDE.

conan/internal/runner/ssh.py

test/functional/command/runner_test.py

jcar87 · 2025-04-23T12:59:02Z

test/functional/command/runner_test.py

 from conan.test.assets.sources import gen_function_h, gen_function_cpp


+def docker_from_env():


feels like tests related to the Docker runner belong in a separate PR - if changes are needed for this PR, maybe open a new one that can be independently reviewed and merged ahead of this?

jcar87 · 2025-04-23T13:01:24Z

conan/internal/runner/ssh.py

+        hostname = self.host_profile.runner.get("host")
+        if not hostname:
+            raise ConanException("Host not specified in runner.ssh configuration")
+        configfile = self.host_profile.runner.get("configfile", False)


this is being changed - I like that we can accept a path to a file, rather than assume the user, but the name config variable seems odd - I would be more explicit - both openssh and paramiko call it "ssh_config"

jcar87 · 2025-04-23T13:05:08Z

conan/internal/runner/ssh.py

+            if ssh_config and ssh_config.lookup(hostname):
+                hostname = ssh_config.lookup(hostname)['hostname']
        self.client = SSHClient()
+        self.client.set_missing_host_key_policy(AutoAddPolicy())  # Auto accept unknown keys


For the sake of security, drop this line. Assume the user has previously explicitly added the host key - this needs to be documented and done outside of Conan's realm

jcar87 · 2025-04-23T13:06:58Z

conan/internal/runner/ssh.py

-                python_command = remote_folder + "/venv" + "/Scripts" + "/python.exe"
-            else:
-                python_command = remote_folder + "/venv" + "/bin" + "/python"
+        if not self.remote_conn.run_command(f"{conan_cmd} remote update conancenter --url='https://center2.conan.io'", "Updating conancenter remote").success:


Outside of the scope of the SSH runner - given that the runner will create a new conan home on the fly (and that this feature was never publicly released) - users will already have the correct value for Conan Center, so this is unnecessary.

jcar87 · 2025-04-23T13:07:38Z

conan/internal/runner/ssh.py

+    def _install_conan_remotely(self, python_command: str, version: str):
+        self.output.verbose(f"Installing conan version: {version}")
+        # Note: this may fail on windows
+        result = self.remote_conn.run_command(f"{python_command} -m pip install conan{f'=={version}' if version != 'dev' else ' --upgrade'}", "Installing conan")


why not --force-reinstall if we are currently assuming the venv only has a single version?

jcar87 · 2025-04-23T14:08:32Z

conan/internal/runner/ssh.py

+        result = self.remote_conn.run_command(f'{self.remote_python_command} -c "import os; print(os.name)"', "Checking remote OS type")
+        if not result.success:
+            self.output.error("Unable to determine remote OS type")
+        self.is_remote_windows = result.stdout == "nt"


Please keep self.remote_is_windows

jcar87 · 2025-04-23T14:24:58Z

conan/internal/runner/ssh.py

+        self.client: SSHClient = client
+        self.runner_output = runner_output
+
+    def put(self, src: str, dst: str) -> None:


put, get and mkdir are specific FTP words - so they make sense for sftp.put(), but not so much for a RemoteConnection.put() - I would suggest calling them sftp_put() and spft_get() etc, if we are so inclined to have these in this class

jcar87 · 2025-04-23T14:29:29Z

conan/internal/runner/ssh.py

+        if sys.stdout.isatty():
+            width, height = os.get_terminal_size()
+        else:
+            width, height = 80, 24


we should fallback to paramiko's defaults, and not hardcode a new set without additional context

jcar87 · 2025-04-23T14:34:54Z

conan/internal/runner/ssh.py

+            # This is the easiest and better working approach but not testable
+            # sys.stdout.buffer.write(line)
+            # sys.stdout.buffer.flush()
+            line = remove_cursor_movements(line.replace(b'\r', b'').decode(errors='ignore').strip())


if I'm getting this right.. are we doing a line.replace() on every line (or rather, every chunk of read bytes, as they're not necessarily lines), and operating on each line with the remove_cursos_movements ?

what is the aim of this? it was working fine before (despite the slightly off logic for windows hosts) - we're dealing with potentially tens of megabytes of log data - should it not be streamed to the terminal as quickly as possible, minimising overhead to the fullest extent?

jcar87 · 2025-04-23T14:42:17Z

conan/internal/runner/ssh.py

+        remote_tmp_dir = _Path(self.remote_create_dir)
+
+        # Check if recipe defines a root folder
+        app = ConanApp(self.conan_api)


This should probably be better explained in the PR description and I would suggest handling this in a separate PR for a proper discussion

With the proposed logic, the conanfile would be loaded 3 times:

during the first export locally

here

remotely, when invoking Conan create

We may as well simplify everything - we know that the recipe was already exported in the local cache - thus, all paths relative to the recipe were already resolved in order for that to succeed. We can export the recipe as it is in the cache, or use the cona cache save/restore for this - and just restore it remotely (we may need to switch a conan install + conan test instead) - but as I said, best discussed in a separate PR altogether

perseoGI marked this pull request as ready for review November 22, 2024 13:15

perseoGI force-pushed the pgi/ssh-runner branch from cf0893c to 6c7b1b8 Compare November 25, 2024 10:04

perseoGI modified the milestones: 2.10.0, 2.11 Nov 29, 2024

memsharded reviewed Dec 18, 2024

View reviewed changes

conan/api/subapi/cache.py Outdated Show resolved Hide resolved

conan/api/subapi/cache.py Outdated Show resolved Hide resolved

conan/internal/runner/ssh.py Show resolved Hide resolved

memsharded modified the milestones: 2.11.0, 2.12.0 Dec 18, 2024

perseoGI commented Dec 18, 2024

View reviewed changes

conan/api/subapi/cache.py Outdated Show resolved Hide resolved

conan/api/subapi/cache.py Outdated Show resolved Hide resolved

perseoGI force-pushed the pgi/ssh-runner branch from ff37095 to 5032b3c Compare December 27, 2024 10:26

memsharded assigned jcar87 Jan 21, 2025

czoido modified the milestones: 2.12.0, 2.13.0 Jan 27, 2025

perseoGI force-pushed the pgi/ssh-runner branch 11 times, most recently from e0ce817 to 83c905d Compare January 30, 2025 15:19

perseoGI added 6 commits January 30, 2025 18:06

Fix several issues on SSH runner and refactor code

fb4bda4

Log errors blocking and some improvements

bf5e761

Fixed '&' bug and more refactors

e6ee256

Private import

16f746a

Follow issue reference

777db28

paramiko/paramiko#2474 (comment)

Protect possible exception

25ba67f

Merge and fix develop2 conflicts

de318b1

memsharded modified the milestones: 2.13.0, 2.14.0 Feb 24, 2025

memsharded modified the milestones: 2.14.0, 2.15.0 Mar 10, 2025

perseoGI added 2 commits March 27, 2025 17:44

Merged with develop2 and fixed conflicts

fdff80d

Use system service

ba11f7c

czoido modified the milestones: 2.15.0, 2.16.0 Mar 31, 2025

davidsanfal requested review from jcar87 and memsharded April 14, 2025 10:34

davidsanfal approved these changes Apr 14, 2025

View reviewed changes

memsharded reviewed Apr 14, 2025

View reviewed changes

perseoGI added 2 commits April 15, 2025 10:13

Resolved threads

b577c67

Merge branch 'develop2' into pgi/ssh-runner

a025ba9

perseoGI force-pushed the pgi/ssh-runner branch from f359099 to a025ba9 Compare April 15, 2025 08:15

davidsanfal added 5 commits April 21, 2025 14:30

delete blank line

1d664ad

ssh runner_output fixed

7ab864f

runners test fixed

c54f3f1

style formatting

4149efb

style formatting

cc258fb

AbrilRBS modified the milestones: 2.16.0, 2.X Apr 24, 2025

Merge branch 'develop2' into pgi/ssh-runner

6ad129e

jcar87 reviewed Jun 25, 2025

View reviewed changes

This was referenced Jun 25, 2025

Runners: allow profile composition and renamed GitHub job #18534

Merged

SSH runner: User friendly paramiko import protection #18566

Merged

perseoGI added a commit to perseoGI/conan that referenced this pull request Jul 7, 2025

ssh runner: partially migrated RemoteConnection from conan-io#17357

8643736

perseoGI added a commit to perseoGI/conan that referenced this pull request Jul 7, 2025

ssh runner: partially migrated RemoteConnection from conan-io#17357

c0e2849

perseoGI mentioned this pull request Jul 7, 2025

SSH runner: created RemoteConnection and copy conan config to remote #18599

Open

		from conan.test.assets.sources import gen_function_h, gen_function_cpp


		def docker_from_env():

SSH Runner: Fix several issues and refactor code #17357

Are you sure you want to change the base?

SSH Runner: Fix several issues and refactor code #17357

Uh oh!

Conversation

perseoGI commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

perseoGI commented Nov 21, 2024 •

edited

Loading