Skip to content

[rayapp] detect remote command failures on Kubernetes clouds - #496

Open
elliot-barn wants to merge 1 commit into
mainfrom
rayapp-detect-remote-command-failure
Open

[rayapp] detect remote command failures on Kubernetes clouds#496
elliot-barn wants to merge 1 commit into
mainfrom
rayapp-detect-remote-command-failure

Conversation

@elliot-barn

Copy link
Copy Markdown
Collaborator

Problem

anyscale workspace_v2 run_command discards the remote exit status and always exits 0. The only failure signal runAnyscaleCLI has is the exec failed with exit code N line added in #490 — and that line comes from the exec shim on VM clusters. Kubernetes clouds (AKS/EKS/GKE) ssh straight into the pod, nothing emits it, and a failing test command is reported as a pass.

Measured on both stacks with the same template and binary:

AWS (VM, us-west-2) Azure (K8S, westus2)
test command exits non-zero level=fatal msg="exec failed with exit code 3"Success: false (nothing)Success: true

The Azure run that prompted this reported Success: true after unzip: cannot find or open workspace-intro.zip and bash: tests.sh: No such file or directory — the template's tests never executed.

Fix

Append an echo of the exit status to every remote command and check it. Works on both stacks, independent of the shim.

  • The command runs in a subshell, so a script ending in exit N reports N instead of killing the shell before the echo. (A real-shell test caught this; without the subshell, exit 3 produced no tag at all.)
  • A missing tag is a failure — the remote shell died early, which is exactly the silent-pass case being guarded.

Testing

  • go test ./rayapp/ passes; package coverage 87.3% (gate 80%).
  • New TestWithRemoteExitStatusInShell runs the wrapped command through a real bash across the shapes the test runner builds: plain, exit N, timeout … bash -c '…' (passing and failing), nested single quotes, and command-not-found (127).
  • Unit tests cover non-zero status with a CLI success, and the missing-tag case.
  • The fake CLI now echoes the tag, matching real behavior.

Not addressed here

Two more Kubernetes-only gaps, neither fixable inside rayapp today — flagging so they aren't mistaken for this bug:

  1. Working directory. run_command starts in $HOME; push writes to the workspace working directory, so pushed files aren't visible to the test command. On VM clusters both are the working directory. A fix needs the default dir name, which the CLI only exposes SDK-internally (get_default_dir_name), not as a command.
  2. Shell environment. bash -c doesn't source ~/.bashrc, where Kubernetes workspaces declare PATH and the ANYSCALE_* vars below the non-interactive guard. On VM clusters the test command sees them without help. Arguably a dataplane-side difference rather than a rayapp one.

With all three worked around by hand, workspace-intro runs its full notebook test on an AKS cloud (24 cells, exit 0), so the k8s path is otherwise sound.

🤖 Generated with Claude Code

`anyscale workspace_v2 run_command` discards the remote exit status and
always exits 0, so runAnyscaleCLI's only failure signal is the
`exec failed with exit code N` line added in #490. That line comes from
the exec shim on VM clusters. Kubernetes clouds (AKS/EKS/GKE) ssh
straight into the pod, nothing emits it, and a failing command reads as
a pass.

Measured on an Azure AKS cloud: a test command that unzipped nothing,
then ran `bash: tests.sh: No such file or directory`, finished with
`Success: true`. The same template with a deliberately failing command
on an AWS VM cloud logs `level=fatal msg="exec failed with exit code 3"`
and is correctly reported as a failure.

Append an echo of the status to every remote command and check it, which
works on both stacks and does not depend on the shim. The command runs
in a subshell so a script ending in `exit N` reports N rather than
killing the shell before the echo. A missing tag is treated as a
failure: the remote shell died early, which is the silent-pass case this
is meant to catch.

Two related Kubernetes gaps are not addressed here, since neither is
fixable inside rayapp today:

- `run_command` starts in $HOME while `push` writes to the workspace
  working directory, so pushed files are not visible to the test
  command. On VM clusters both are the working directory. Resolving it
  needs the default dir name, which the CLI only exposes SDK-internally
  (`get_default_dir_name`), not as a command.
- `bash -c` does not source ~/.bashrc, where Kubernetes workspaces
  declare PATH and the ANYSCALE_* vars below the non-interactive guard.
  On VM clusters the test command sees them without help.

Signed-off-by: Elliot Barnwell <elliot.barnwell@anyscale.com>

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces a mechanism to capture and verify the exit status of remote commands run in Anyscale workspaces, which is especially critical for Kubernetes-based clouds where the CLI otherwise silently discards non-zero exit codes. It wraps commands to echo their exit status, parses the output for this status, and updates the corresponding tests. The reviewer suggested a robust improvement to the status parsing logic to prevent false-positive failures caused by trailing logs or SSH teardown messages being incorrectly included in the parsed exit status.

Comment on lines +151 to +155
status := strings.TrimSpace(output[idx+len(remoteExitPrefix):])
if status == "0" {
return nil
}
return fmt.Errorf("remote command exited with status %s:\n%s", status, output)

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

If there is any trailing output from SSH or container teardown (e.g., connection closed messages, background daemon logs, or environment cleanup warnings) after the command exits, strings.TrimSpace on the entire remaining string will include that trailing text in status. This will cause status == "0" to evaluate to false, leading to false-positive test failures.

Isolating only the first line of the remaining output before trimming ensures we only parse the exit status code and ignore any subsequent trailing logs.

	remaining := output[idx+len(remoteExitPrefix):]
	status := remaining
	if nl := strings.IndexByte(remaining, '\n'); nl >= 0 {
		status = remaining[:nl]
	}
	status = strings.TrimSpace(status)
	if status == "0" {
		return nil
	}
	return fmt.Errorf("remote command exited with status %s:\n%s", status, output)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant