Skip to content

fix: drop literal quotes from scontrol Reason/Comment argv (fixes #2954) - #2988

Open
SIDDARTHAREDDY8 wants to merge 1 commit into
nebius:mainfrom
SIDDARTHAREDDY8:fix/check-runner-literal-quotes
Open

SIDDARTHAREDDY8 wants to merge 1 commit into
nebius:mainfrom
SIDDARTHAREDDY8:fix/check-runner-literal-quotes

Conversation

@SIDDARTHAREDDY8

Copy link
Copy Markdown

Problem

In helm/slurm-cluster/slurm_scripts/check_runner.py, drain_node, comment_node and uncomment_node build the scontrol argv with literal quotes: Reason="{reason}" / Comment="{comment}".

Because subprocess.run gets a list (no shell), nothing strips those quotes, so Slurm stores them as part of the value. On the OK path, comment.startswith(reason_base) then never matches, so on_ok: uncomment/undrain never fire — a node keeps its [node_problem] comment after the link heals. Reproduced from the scontrol show node --json output in #2954.

Solution

Drop the literal quotes: Reason={reason}, Comment={comment}, Comment= (3-line change, verified by diff against main).

Testing

  • Repro harness mocking subprocess.run (captures argv, simulates slurmctld storing elements verbatim): bug reproduced — stored values carry literal ", and uncomment stores '""' instead of clearing.
  • Unit suite (test_check_runner_fix.py, stdlib unittest): 8/8 pass — 5 positive (quote-free storage, uncomment clears, full comment→heal→uncomment and drain→heal→undrain round-trips through the real run_check FAIL/OK paths with the startswith(reason_base) guard) and 3 negative (old code never clears on the OK path).
  • No real Slurm or subprocesses were used; only subprocess.run was mocked.

Release Notes

N/A (bug fix, no user-facing change beyond correct node recovery).

subprocess.run receives a list, so no shell strips the quotes in
Reason="{reason}" / Comment="{comment}". Slurm stores the quote
characters as part of the value, so on the OK path
comment.startswith(reason_base) never matches and on_ok uncomment/
undrain never fire. Nodes keep stale [node_problem] comments after
recovery. Fixes nebius#2954.
@SIDDARTHAREDDY8

Copy link
Copy Markdown
Author

The Label Checker workflow requires at least one label from its list (fix, feat, bug, etc.) — this PR currently has none, so the check fails on every run. Could a maintainer please add the fix label (this is a bug fix for #2954)? I do not have label permissions on this repo.

@Uburro

Uburro commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

this is PR identical with https://github.com/nebius/soperator/pull/2955/changes

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.

2 participants