Skip to content

CFE-4147: Added better logging for ssh connections #137

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 22, 2025
Merged

CFE-4147: Added better logging for ssh connections #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion cf_remote/aramid.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import subprocess
import time
from urllib.parse import urlparse
from cf_remote import log
Copy link
Member

@olehermanse olehermanse May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, aramid was an independent thing which didn't rely on anything from cf_remote. Now you've introduced a dependency on something from cf_remote, which means aramid can no longer be imported on its own as a separate module / library.

I think it's okay, but we could reverse this by:

Switching to only using python standard logging (https://docs.python.org/3/library/logging.html)

Or

Sending in a log object as a parameter when using aramid functions.


DEFAULT_SSH_ARGS = [
"-oLogLevel=ERROR",
Expand Down Expand Up @@ -126,6 +127,7 @@ def communicate(self, timeout=1, ignore_failed=False):
try:
out, err = self.proc.communicate(timeout=timeout)
except subprocess.TimeoutExpired:
log.debug("Connection timed out")
return False
except Exception as e:
raise _TaskError("Failed to communicate with the process") from e
Expand Down Expand Up @@ -245,7 +247,16 @@ def _hosts_to_host_specs(hosts):
def _wait_for_tasks(hosts, tasks, ignore_failed, echo, echo_action, out_flag=""):
while not all(task.done for task in tasks):
for task in (t for t in tasks if not t.done):
# TODO: add logging here

if task.proc.args[0] == "scp":
log.debug(
f"Copying '{task.action}' to {task.host.user}@{task.host.host_name} over scp"
)
else:
log.debug(
f"Running '{task.action}' on {task.host.user}@{task.host.host_name} over {task.proc.args[0]}"
)

try:
task.communicate(ignore_failed=ignore_failed)
except _TaskError as e:
Expand Down
9 changes: 4 additions & 5 deletions cf_remote/ssh.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,22 +183,22 @@ def scp(file, remote, connection=None, rename=None, hide=False):
with connect(remote) as connection:
scp(file, remote, connection, rename, hide=hide)
else:
print_function = log.debug if hide else print
print_function("Copying: '%s' to '%s'" % (file, remote))
if not hide:
print("Copying: '%s' to '%s'" % (file, remote))
Comment on lines +186 to +187
Copy link
Member

@olehermanse olehermanse May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but you have changed the logging in a not-so-obvious way: Before it was only logged to one place: either print, or log.debug. Now, after your change, it's printed twice (always to log.debug, and sometimes to print).

connection.put(file, hide=hide)
if rename:
file = os.path.basename(file)
if file == rename:
return 0
print_function("Renaming '%s' -> '%s' on '%s'" % (file, rename, remote))
if not hide:
print("Renaming '%s' -> '%s' on '%s'" % (file, rename, remote))
ssh_cmd(connection, "mv %s %s" % (file, rename), hide=hide)
return 0


def ssh_cmd(connection, cmd, errors=False):
assert connection

log.debug("Running over SSH: '%s'" % cmd)
result = connection.run(cmd, hide=True)
if result.retcode == 0:
output = result.stdout.replace("\r\n", "\n").strip("\n")
Expand All @@ -220,7 +220,6 @@ def ssh_cmd(connection, cmd, errors=False):
def ssh_sudo(connection, cmd, errors=False):
assert connection

log.debug("Running(sudo) over SSH: '%s'" % cmd)
if connection.needs_sudo:
escaped = cmd.replace('"', r"\"")
sudo_cmd = 'sudo bash -c "%s"' % escaped
Expand Down