From 922bf9f45a47b2a0da028f9e5a9a7e9c88675365 Mon Sep 17 00:00:00 2001
From: Richard Liaw <rliaw@berkeley.edu>
Date: Thu, 27 Aug 2020 17:59:44 -0700
Subject: [PATCH] [cli] improve error handling, don't swallow errors (#10370)

---
 python/ray/autoscaler/command_runner.py       | 19 ++++++++++++-------
 .../ray/autoscaler/subprocess_output_util.py  |  5 ++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/python/ray/autoscaler/command_runner.py b/python/ray/autoscaler/command_runner.py
index cd646be1f..240317572 100644
--- a/python/ray/autoscaler/command_runner.py
+++ b/python/ray/autoscaler/command_runner.py
@@ -16,8 +16,8 @@ from ray.autoscaler.docker import check_docker_running_cmd, \
                                   with_docker_exec
 from ray.autoscaler.log_timer import LogTimer
 
-from ray.autoscaler.subprocess_output_util import (run_cmd_redirected,
-                                                   ProcessRunnerError)
+from ray.autoscaler.subprocess_output_util import (
+    run_cmd_redirected, ProcessRunnerError, is_output_redirected)
 
 from ray.autoscaler.cli_logger import cli_logger
 import colorful as cf
@@ -434,6 +434,11 @@ class SSHCommandRunner(CommandRunnerInterface):
             exit_on_fail (bool):
                 If `exit_on_fail` is `True`, the process will exit
                 if the command fails (exits with a code other than 0).
+
+        Raises:
+            ProcessRunnerError if using new log style and disabled
+                login shells.
+            click.ClickException if using login shells.
         """
         try:
             # For now, if the output is needed we just skip the new logic.
@@ -459,12 +464,12 @@ class SSHCommandRunner(CommandRunnerInterface):
 
             if exit_on_fail:
                 raise click.ClickException(
-                    "Command failed: \n\n  {}\n".format(quoted_cmd)) \
-                    from None
+                    "Command failed:\n\n  {}\n".format(quoted_cmd)) from None
             else:
-                raise click.ClickException(
-                    "SSH command Failed. See above for the output from the"
-                    " failure.") from None
+                fail_msg = "SSH command failed."
+                if is_output_redirected():
+                    fail_msg += " See above for the output from the failure."
+                raise click.ClickException(fail_msg) from None
 
     def run(
             self,
diff --git a/python/ray/autoscaler/subprocess_output_util.py b/python/ray/autoscaler/subprocess_output_util.py
index 47bd1c8f9..df3617b3d 100644
--- a/python/ray/autoscaler/subprocess_output_util.py
+++ b/python/ray/autoscaler/subprocess_output_util.py
@@ -223,7 +223,6 @@ def _run_and_process_output(cmd,
     The code is thus 100% thread-safe as long as the stream readers
     are read-only except for return values and possible exceptions.
     """
-
     stdin_overwrite = subprocess.PIPE
     # This already should be validated in a higher place of the stack.
     assert not (does_allow_interactive() and is_output_redirected()), (
@@ -235,9 +234,9 @@ def _run_and_process_output(cmd,
 
     if use_login_shells:
         if stdout_file is None:
-            stdout_file = subprocess.DEVNULL
+            stdout_file = sys.stdout
         if stderr_file is None:
-            stderr_file = subprocess.DEVNULL
+            stderr_file = sys.stderr
         return subprocess.check_call(
             cmd,
             # See implementation note #2