[lonexreb]

Description

The autoscaler runs remote commands through a login shell (bash --login -i over ssh -tt). Profile scripts on many images print a MOTD, quota warnings or similar banners, and that text ends up in the captured stdout of every run(..., with_output=True) call. Callers that parse the output, most visibly the docker inspect JSON used during node setup, then fail with confusing errors. #62045 lists four earlier reports of the same breakage going back to #13433.

This change brackets captured commands with start/end markers (echo START; (cmd); status=$?; echo; echo END; exit $status) and returns only the output between them, preserving the command's exit code. Commands that do not capture output are untouched, and output without both markers is returned as is.

Related issues

Fixes #62045

@rueian asked on the issue for a formal PR. #64130 by @enginarslan1 proposed the same marker approach but was closed by the stale bot on Jul 23 with no author activity since; this is a smaller version of it (fixed markers, one helper each way). gh pr list --state open --search "62045 in:body" and a search for command_runner.py return nothing open.

Additional information

New tests:

• test_ssh_command_runner_strips_profile_output feeds a banner before and after the marked output (with the \r\n line endings ssh -tt produces) and asserts only the command's own output is returned, and that the wrapped command carries the markers and the exit-code preservation.
• test_strip_output_markers_passthrough covers output with no or only one marker.

Test commands run locally (Ray 3.0.0.dev0 nightly wheel + setup-dev.py, macOS arm64):

pytest python/ray/tests/test_command_runner.py                                   # 15 passed
pytest python/ray/tests/test_autoscaler.py -k "docker or ssh or init"           # passed
black 22.10.0 / ruff 0.8.4 clean on the two changed files

AI assistance (Claude Code) was used to draft this change; every line was reviewed and tested by the submitter.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces output markers (_OUTPUT_START_MARKER and _OUTPUT_END_MARKER) to bracket and strip out MOTD, quota warnings, or other login shell banners from captured command outputs in the Ray autoscaler command runner. The review feedback highlights a potential issue where pseudo-terminal SSH sessions (ssh -tt) echo the executed command back to stdout, causing the marker extraction logic to match the echoed command instead of the actual output markers. To address this, the reviewer suggests refining _strip_output_markers to ignore markers preceded by echo and updating the corresponding unit tests to simulate this realistic scenario.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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]

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.

[rueian]

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

[lonexreb]

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?