Skip to content
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

Less verbose logging in ssh operator #24915

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

hsnprsd
Copy link
Contributor

@hsnprsd hsnprsd commented Jul 8, 2022

Current logs of SSHOperator are too verbose. SSHHook already logs stdout and stderr of the running command in task logs, so there is no need for including stderr in AirflowException message returned by run_ssh_client_command. Also there is no need to catch and then raise the same exception in execute method of SSHOperator.

Example of current logs:

**<combined stderr & stdout of command>**

Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.7/site-packages/airflow/providers/ssh/operators/ssh.py", line 173, in execute
    result = self.run_ssh_client_command(ssh_client, self.command)
  File "/home/airflow/.local/lib/python3.7/site-packages/airflow/providers/ssh/operators/ssh.py", line 160, in run_ssh_client_command
    self.raise_for_status(exit_status, agg_stderr)
  File "/home/airflow/.local/lib/python3.7/site-packages/airflow/providers/ssh/operators/ssh.py", line 153, in raise_for_status
    raise AirflowException(f"error running cmd: {self.command}, error: {error_msg}")
airflow.exceptions.AirflowException: error running cmd: **<ssh command>**, error: **<stderr of command>**

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.7/site-packages/divar_airflow/operators/spark_operator/ssh/spark_operator.py", line 199, in execute
    return super().execute(**context)
  File "/home/airflow/.local/lib/python3.7/site-packages/airflow/providers/ssh/operators/ssh.py", line 175, in execute
    raise AirflowException(f"SSH operator error: {str(e)}")
airflow.exceptions.AirflowException: SSH operator error: error running cmd: **<ssh command>**, error: error running cmd: **<ssh command>**, error: **<stderr of command>**

@hsnprsd hsnprsd force-pushed the non-verbose-logging-ssh-operator branch from a3e7032 to a639ae5 Compare July 8, 2022 10:15
@potiuk
Copy link
Member

potiuk commented Jul 12, 2022

Static checks are faiing.

@hsnprsd
Copy link
Contributor Author

hsnprsd commented Jul 12, 2022

@potiuk I will try to fix them tomorrow. btw, do you think the current change is valid?

@potiuk
Copy link
Member

potiuk commented Jul 12, 2022

Yep. if error message is already printed it makes no sense to print it again.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Changes make sense to me, should be ready after fixing static check errors.

@hsnprsd
Copy link
Contributor Author

hsnprsd commented Jul 13, 2022

Checks are successful now. @uranusjr

@potiuk potiuk merged commit ca99c23 into apache:main Jul 13, 2022
@potiuk
Copy link
Member

potiuk commented Jul 13, 2022

Just in time for the next provider's wave

@morooshka
Copy link
Contributor

morooshka commented Jan 2, 2023

Hi @potiuk, @hsnprsd! We need to see stderr in exception - it is viewable via error email reporting and significantly reduces the time to analyse the problem for supports. What about return stderr to exception? I can implement it. So ideally i want to see something like this:

Exception: SSH operator error: exit status = 1, errmsg = File not found

@potiuk
Copy link
Member

potiuk commented Jan 2, 2023

Yes. Might be a good idea @morooshka - feel free to implement it.

@dhananjays
Copy link

Sorry for commenting on an old PR.

Before this change, exceptions thrown by paramiko (eg. host not reachable, etc) were caught and raised forward as an AirflowException. After this change, that's not the case. Was that intentional @hsnprsd? If not, I can raise a separate issue if you agree this is not desired behaviour.

@potiuk
Copy link
Member

potiuk commented May 9, 2024

Before this change, exceptions thrown by paramiko (eg. host not reachable, etc) were caught and raised forward as an AirflowException. After this change, that's not the case. Was that intentional @hsnprsd? If not, I can raise a separate issue if you agree this is not desired behaviour.

Airlfow Exception is only relevant as base exception for some special behaviours (timeout etc.). There is no particular reason why you should expect provider to throw only Airflow Exception, at any point in time any unhandled exception can be raised so your code should be ready to handle any of those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants