[chuangz0]

Dev Engineer Review

jenkins/L0_Test.groovy now lists top-level performance directories with remote find and filters for aggr and disagg locally. It also removes 2>/dev/null from the remote job-ID fallback read, avoiding csh redirection behavior. No test results or end-to-end verification are available.

QA Engineer Review

No test changes.

Per-File QA Perspective

• jenkins/L0_Test.groovy: Verify that the progress watcher discovers and copies the expected aggr and disagg folders on csh and bash login shells. Verify that a missing slurm_job_id.txt still reaches the intended typed error path.

Description

Symptom. The test-results tarballs of PerfSanity stages that run on oci-jhb or oci-aga (the GB300 stages) contain only results*.xml, passed_test_list.txt and unfinished_test.txt. The per-test aggr_upload-* / disagg_upload-* folders are missing, and with them the server logs and the cache_transceiver_precheck/{logs,status,progress,rendezvous,csv} data. Stages on oci-hsg (GB200) and nsc-svg (DGX_B200) still have the folders. This affects both aggregated and disaggregated PerfSanity stages, and it happens whether pytest passes or not. No error is printed.

Sampled successful PerfSanity tarballs, main L0_PostMerge #2993–#3018:

Cluster	svc_tensorrt login shell	sshd ForceCommand that runs the command with bash	Tarballs with a perf folder
oci-jhb	/bin/csh	no	0 / 15
oci-aga	/bin/csh	no	0 / 15
oci-hsg	/bin/csh	yes (exec bash -c "$SSH_ORIGINAL_COMMAND")	14 / 14
nsc-svg	/bin/bash	no	9 / 9

Cause. #14138 moved the perf-folder listing into sshListPerfCmd:

find '<ws>' -maxdepth 1 -type d \( -name 'aggr*' -o -name 'disagg*' \) -print 2>/dev/null || true

Both consumers paste this command between single quotes: SLURM_SSH_LIST_PERF_CMD='${sshListPerfCmd}' for progress_upload_watcher.sh, and eval '${sshListPerfCmd}' in the final snapshot. That strips the inner quotes, so the remote side receives -name aggr* -o -name disagg*. sshd then runs the command with svc_tensorrt's login shell.

• Under bash, it still works.
• Under csh (bsd-csh on these Ubuntu login nodes), the bare globs fail with No match. Even with the quotes intact, 2>/dev/null becomes an extra argument 2 plus a stdout redirect to /dev/null.

The error goes to stderr, which the local 2>/dev/null discards. The folder list comes back empty, so the scp loop never runs.

Fix.

• Run only find <ws> -mindepth 1 -maxdepth 1 -type d remotely, with no quotes, globs or fd redirects. Keep the aggr* / disagg* folders with a local grep -e /aggr -e /disagg. The workspace path only contains [a-z0-9._-], so it is safe unquoted in both shells.
• Drop 2>/dev/null from the slurm_job_id.txt fallback read. Under csh it always returned an empty job ID, for the same reason.

Test Coverage

No CI test covers the Jenkins-side copy logic. Manual verification:

• Generated-script layering. On an oci-hsg login node I reproduced both consumers' layering: VAR='<cmd>' + eval "$VAR", and eval '<cmd>'. The remote shell was /usr/bin/bsd-csh and then /bin/bash:
  • Old command: empty under csh in both paths; lists both folders under bash.
  • New command: lists exactly the aggr_* and disagg_* directories under both shells, skipping other directories and files.
• Job-ID fallback. cat <file> 2>/dev/null || true returns empty under bsd-csh; without the redirect it returns the file content.
• pre-commit passes.

To check end to end, run a GB300 Disagg-PerfSanity stage that lands on oci-jhb or oci-aga. Its results-<stage>.tar.gz should then contain the disagg_upload-* folder.

PR Checklist

Please review the following before submitting your PR:

• PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

• PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

• Test cases are provided for new code paths (see test instructions)

• If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

• Any new dependencies have been scanned for license and vulnerabilities

• CODEOWNERS updated if ownership changes

• Documentation updated as needed

• Update tava architecture diagram if there is a significant design change in PR.

• The reviewers assigned automatically/manually are appropriate for the PR.

• Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: NVIDIA/TensorRT-LLM
• Branch: main
• File: .coderabbit.yaml
• Commit: b78033a
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 13b1b3e7-5a41-40a1-b208-72819b053af9

📥 Commits

Reviewing files that changed from the base of the PR and between 5f48562 and 5828d4f.

📒 Files selected for processing: 1

• jenkins/L0_Test.groovy

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Walkthrough

The SLURM test runner changes fallback job-ID retrieval and progress-watcher commands. Redirects and filters now run on the local side of SSH.

Changes

SLURM sbatch test execution

Layer / File(s)	Summary
Read sbatch job ID jenkins/L0_Test.groovy	The fallback job-ID read no longer places 2>/dev/null in the remote command.
Discover performance directories jenkins/L0_Test.groovy	The progress-watcher commands move redirection and filtering to the local shell. Their remote commands avoid syntax rejected by csh login shells.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: emmaqiaoch

Merge Risk: ⚪ Minimal · up to 5828d

No concrete merge-blocking regression is established by the reviewed shell-handling changes.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly identifies the infrastructure fix: collecting performance result folders on Slurm clusters that use a csh login shell.
Description check		The description explains the symptom, cause, fix, test coverage, manual verification, and end-to-end validation steps. It also includes the required checklist and marks it as reviewed.
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}