Skip to content

[autoscaler] Strip login-shell profile output from captured SSH command output #67392

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

Open
lonexreb wants to merge 2 commits into
base: master
Choose a base branch
from
+76 −1
Open

[autoscaler] Strip login-shell profile output from captured SSH command output #67392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6730484
[autoscaler] Strip login-shell profile output from captured SSH comma…
lonexreb Oct 25, 2026
3a42e3f
Sort imports in test_command_runner
lonexreb Oct 28, 2026
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
34 changes: 33 additions & 1 deletion python/ray/autoscaler/_private/command_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,34 @@ def _with_environment_variables(cmd: str, environment_variables: Dict[str, objec
return all_vars + cmd


_OUTPUT_START_MARKER = "__RAY_COMMAND_OUTPUT_START__"
_OUTPUT_END_MARKER = "__RAY_COMMAND_OUTPUT_END__"

@rueian rueian Oct 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @lonexreb, instead of using the marker, why don't we just make autoscaler work with --use-normal-shells?

@lonexreb lonexreb Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rueian, that's a fair alternative. Here's why I went with markers, and I'm happy to switch if you prefer the other route.

  • Today the flag doesn't reach the autoscaler. --use-normal-shells only calls set_using_login_shells(False) / set_allow_interactive(False) in the local ray up process (commands.create_or_update_cluster). The monitor on the head keeps the module defaults (use_login_shells: True), which is why the reporter had to patch command_runner.py and subprocess_output_util.py in setup_commands.
  • Markers fix the default path. Most users never pass the flag, and login shells are the default because many images only set up PATH/conda in profile files. Markers make the captured output of docker inspect, uptime and similar checks robust without changing which shell runs.
  • Normal shells alone may not be enough. The reporter needed --use-normal-shells "possibly in conjunction with .hushlogin". Non-interactive SSH sessions can still emit output, for example bash sourcing ~/.bashrc under sshd or ForceCommand and PAM banners.

If you'd rather go the flag route, I'd do it like this. ray up --use-normal-shells records an internal field in the bootstrapped config it uploads to the head, and the monitor applies set_using_login_shells and set_allow_interactive from that field at startup. That keeps the behavior opt-in and leaves default users unchanged. I can also do both: markers for robustness, plus propagation so the flag means the same thing on the head. Which would you like?



def _with_output_markers(cmd: str) -> str:
"""Bracket the command's stdout with markers.

Login shells run profile scripts that may print a MOTD, quota warnings or
similar banners before (and after) the command runs. Callers that parse the
captured output, e.g. ``docker inspect`` JSON, break on that extra text.
The markers let :func:`_strip_output_markers` recover just the command's
own output while preserving its exit code.
"""
return (
f"echo {_OUTPUT_START_MARKER}; ({cmd}); __ray_status=$?; "
f"echo; echo {_OUTPUT_END_MARKER}; exit $__ray_status"
)


def _strip_output_markers(output: bytes) -> bytes:
"""Return the part of ``output`` between the markers, if both are present."""
start = output.find(_OUTPUT_START_MARKER.encode())
end = output.rfind(_OUTPUT_END_MARKER.encode())
if start == -1 or end == -1 or end < start:
return output
return output[start + len(_OUTPUT_START_MARKER) : end].strip(b"\r\n")
Comment on lines +127 to +133

@gemini-code-assist gemini-code-assist Bot Oct 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In a real SSH session with a pseudo-terminal (ssh -tt), the shell echoes the executed command back to stdout. Since the wrapped command contains echo __RAY_COMMAND_OUTPUT_START__ and echo __RAY_COMMAND_OUTPUT_END__, these markers will appear in the echoed command at the very beginning of the output.

Using simple .find() and .rfind() will match the markers inside the echoed command rather than the actual printed markers. This results in the echoed command and the markers themselves being included in the returned output, defeating the purpose of stripping them.

To fix this, we should find the occurrences of the markers that are not preceded by echo .

def _strip_output_markers(output: bytes) -> bytes:
    """Return the part of ``output`` between the markers, if both are present."""
    start_marker = _OUTPUT_START_MARKER.encode()
    end_marker = _OUTPUT_END_MARKER.encode()

    start = -1
    idx = output.find(start_marker)
    while idx != -1:
        if idx < 5 or output[idx - 5 : idx] != b"echo ":
            start = idx
            break
        idx = output.find(start_marker, idx + 1)

    end = -1
    idx = output.rfind(end_marker)
    while idx != -1:
        if idx < 5 or output[idx - 5 : idx] != b"echo ":
            end = idx
            break
        idx = output.rfind(end_marker, 0, idx)

    if start == -1 or end == -1 or end < start:
        return output
    return output[start + len(start_marker) : end].strip(b"\\r\\n")

@lonexreb lonexreb Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With ssh -tt a pseudo-terminal echoes only what is typed into its stdin. Here the wrapped command is passed as an ssh argument and run through bash --login -c -i '...', so it is never written to the tty and never echoed. The markers that come back are only the ones the remote echo prints, which is what find/rfind match. Keeping the test as is, since a mock with an echoed command line would model a case that doesn't occur.



def _with_interactive(cmd):
force_interactive = (
f"source ~/.bashrc; "
Expand Down Expand Up @@ -304,7 +332,9 @@ def _run_helper(
use_login_shells=is_using_login_shells(),
)
else:
return self.process_runner.check_output(final_cmd)
return _strip_output_markers(
self.process_runner.check_output(final_cmd)
)
except subprocess.CalledProcessError as e:
joined_cmd = " ".join(final_cmd)
if not is_using_login_shells():
Expand Down Expand Up @@ -387,6 +417,8 @@ def run(
if cmd:
if environment_variables:
cmd = _with_environment_variables(cmd, environment_variables)
if with_output:
cmd = _with_output_markers(cmd)
if is_using_login_shells():
final_cmd += _with_interactive(cmd)
else:
Expand Down
43 changes: 43 additions & 0 deletions python/ray/tests/test_command_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@
import pytest

from ray.autoscaler._private.command_runner import (
_OUTPUT_END_MARKER,
_OUTPUT_START_MARKER,
DockerCommandRunner,
SSHCommandRunner,
_strip_output_markers,
_with_environment_variables,
)
from ray.autoscaler.command_runner import CommandRunnerInterface
Expand Down Expand Up @@ -218,6 +221,46 @@ def fail_if_ip_is_requested(_):
assert f"ray@{node_id}:/remote/source" in download_command


def test_ssh_command_runner_strips_profile_output():
"""Regression test for https://github.com/ray-project/ray/issues/62045.

Login shells may print a MOTD or other banners around the command's own
output. Captured output must contain only what the command printed.
"""
process_runner = MockProcessRunner()
provider = MockProvider()
provider.create_node({}, {}, 1)
cmd_runner = SSHCommandRunner(
log_prefix="prefix",
node_id="0",
provider=provider,
auth_config=auth_config,
cluster_name="cluster",
process_runner=process_runner,
use_internal_ip=False,
)
process_runner.respond_to_call(
"echo hello",
[
"Welcome to the node, disk quota is 95% full\r\n"
f"{_OUTPUT_START_MARKER}\r\nhello\r\n\r\n{_OUTPUT_END_MARKER}\r\n"
"logout banner\r\n"
],
)
Comment on lines +242 to +249

@gemini-code-assist gemini-code-assist Bot Oct 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Update the mock response to include the echoed command. This makes the test much more realistic, as ssh -tt sessions allocate a pseudo-terminal which echoes the executed command back to stdout by default.

Suggested change
process_runner.respond_to_call(
"echo hello",
[
"Welcome to the node, disk quota is 95% full\r\n"
f"{_OUTPUT_START_MARKER}\r\nhello\r\n\r\n{_OUTPUT_END_MARKER}\r\n"
"logout banner\r\n"
],
)
process_runner.respond_to_call(
"echo hello",
[
"Welcome to the node, disk quota is 95% full\\r\\n"
f"echo {_OUTPUT_START_MARKER}; (echo hello); __ray_status=$?; echo; echo {_OUTPUT_END_MARKER}; exit $__ray_status\\r\\n"
f"{_OUTPUT_START_MARKER}\\r\\nhello\\r\\n\\r\\n{_OUTPUT_END_MARKER}\\r\\n"
"logout banner\\r\\n"
],
)

@lonexreb lonexreb Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With ssh -tt a pseudo-terminal echoes only what is typed into its stdin. Here the wrapped command is passed as an ssh argument and run through bash --login -c -i '...', so it is never written to the tty and never echoed. The markers that come back are only the ones the remote echo prints, which is what find/rfind match. Keeping the test as is, since a mock with an echoed command line would model a case that doesn't occur.


assert cmd_runner.run("echo hello", with_output=True) == b"hello"
process_runner.assert_has_call("1.2.3.4", pattern=_OUTPUT_START_MARKER)
process_runner.assert_has_call("1.2.3.4", pattern="exit \\$__ray_status")


@pytest.mark.parametrize(
"output",
[b"plain output with no markers", b"only " + _OUTPUT_START_MARKER.encode()],
)
def test_strip_output_markers_passthrough(output):
assert _strip_output_markers(output) == output


def test_docker_command_runner():
process_runner = MockProcessRunner()
provider = MockProvider()
Expand Down
Loading