Skip to content

fix(nomad): handle "No running jobs" text output from nomad job status#586

Merged
veeso merged 1 commit intomainfrom
ci/fix-count-eligible-nodes-no-running-jobs
Mar 18, 2026
Merged

fix(nomad): handle "No running jobs" text output from nomad job status#586
veeso merged 1 commit intomainfrom
ci/fix-count-eligible-nodes-no-running-jobs

Conversation

@veeso
Copy link
Member

@veeso veeso commented Mar 18, 2026

Nomad ignores the -json flag when there are no jobs and returns plain
text instead of an empty JSON array, which caused jq parse failures in
count_eligible_nodes.sh. Treat this case as zero busy nodes.

Summary

TODO:

  • All code changes are reflected in docs, including module-level docs
  • All new/edited/removed scenarios are reflected in summary visualiser tool (see checklist)
  • I ran the Nomad CI workflow successfully on my branch

Note that all commits in a PR must follow Conventional Commits before it can be merged, as these are used to generate the changelog

Summary by CodeRabbit

  • Bug Fixes

    • Improved error messaging and warnings for command failures and invalid outputs.
    • Correct handling of cases with no running jobs and more accurate running-allocation counts.
  • Chores

    • Made data collection and validation more robust to prevent silent failures during processing.

@claude
Copy link

claude bot commented Mar 18, 2026

Claude finished @veeso's task in 20s —— View job


No issues found.

@github-actions
Copy link
Contributor

The following will be added to the changelog


[0.6.1] - 2026-03-18

Bug Fixes

  • (nomad) Handle "No running jobs" text output from nomad job status
    • Nomad ignores the -json flag when there are no jobs and returns plain text instead of an empty JSON array, which caused jq parse failures in count_eligible_nodes.sh. Treat this case as zero busy nodes.

@coderabbitai
Copy link

coderabbitai bot commented Mar 18, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7d0a1484-6969-4d50-96b0-ad98dc1bdcd1

📥 Commits

Reviewing files that changed from the base of the PR and between 2e0f0d9 and d6aaf2a.

📒 Files selected for processing (1)
  • nomad/scripts/count_eligible_nodes.sh

Walkthrough

Replaced a streamed jq pipeline in nomad/scripts/count_eligible_nodes.sh with buffered capture of Nomad stdout/stderr, added explicit error and stderr reporting, handled "No running jobs" text, validated JSON before parsing, and computed running allocations from the validated JSON.

Changes

Cohort / File(s) Summary
Shell script robustness & error handling
nomad/scripts/count_eligible_nodes.sh
Capture Nomad stdout/stderr into variables, print detailed errors on command failure, warn on stderr output, handle "No running jobs" case, validate JSON before jq processing, and compute busy_json from the validated jobs_json.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • cdunster
  • ThetaSinner
  • ddd-mtl
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: handling the 'No running jobs' text output from nomad job status command, which matches the core fix in the changeset.
Description check ✅ Passed The PR description explains the issue (Nomad ignoring -json flag and returning plain text), its impact (jq parse failures), and the solution (treating it as zero busy nodes). It includes the required template sections and TODO checklist items.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/fix-count-eligible-nodes-no-running-jobs
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@veeso veeso requested a review from a team March 18, 2026 14:34
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
nomad/scripts/count_eligible_nodes.sh (1)

72-74: Please mirror this fallback in the module docs too.

The inline note is helpful, but this Nomad quirk now affects the script’s contract and should also be called out in the file header/module docs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nomad/scripts/count_eligible_nodes.sh` around lines 72 - 74, Add a short
module-docs/header note explaining the Nomad quirk: when there are no running
jobs the API returns the literal string "No running jobs" rather than JSON, and
this script maps that to an empty JSON array by setting busy_json='[]' (see the
conditional that checks jobs_json and assigns busy_json). Update the top-of-file
comment block (module docs) to describe this contract, include the exact string
"No running jobs" and the resulting behavior (busy_json becomes '[]'), and
mirror that note in any accompanying README or module documentation to make the
fallback explicit to callers and maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@nomad/scripts/count_eligible_nodes.sh`:
- Around line 67-79: The script currently mixes stdout and stderr into jobs_json
(via 2>&1) which breaks the "No running jobs" check and jq validation; change to
capture stdout and stderr separately (e.g., capture stdout into jobs_stdout and
stderr into jobs_stderr), perform the "No running jobs" string comparison and jq
-e validation only against jobs_stdout, and only log jobs_stderr when non-empty;
also avoid buffering the entire payload into a single variable by streaming
jobs_stdout directly into jq to produce busy_json (reference the
jobs_json/busy_json variables and the nomad job status invocation via $NOMAD).

---

Nitpick comments:
In `@nomad/scripts/count_eligible_nodes.sh`:
- Around line 72-74: Add a short module-docs/header note explaining the Nomad
quirk: when there are no running jobs the API returns the literal string "No
running jobs" rather than JSON, and this script maps that to an empty JSON array
by setting busy_json='[]' (see the conditional that checks jobs_json and assigns
busy_json). Update the top-of-file comment block (module docs) to describe this
contract, include the exact string "No running jobs" and the resulting behavior
(busy_json becomes '[]'), and mirror that note in any accompanying README or
module documentation to make the fallback explicit to callers and maintainers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: da8cd680-b948-4ffd-b70f-b50cc61f8819

📥 Commits

Reviewing files that changed from the base of the PR and between b9fb479 and 2e0f0d9.

📒 Files selected for processing (1)
  • nomad/scripts/count_eligible_nodes.sh

ThetaSinner
ThetaSinner previously approved these changes Mar 18, 2026
Copy link
Member

@ThetaSinner ThetaSinner left a comment

Choose a reason for hiding this comment

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

Looks good, coderabbit suggestion may be worth a look

Nomad ignores the -json flag when there are no jobs and returns plain
text instead of an empty JSON array, which caused jq parse failures in
count_eligible_nodes.sh. Treat this case as zero busy nodes.
@veeso veeso force-pushed the ci/fix-count-eligible-nodes-no-running-jobs branch from 2e0f0d9 to d6aaf2a Compare March 18, 2026 15:03
@cocogitto-bot
Copy link

cocogitto-bot bot commented Mar 18, 2026

✔️ d6aaf2a - Conventional commits check succeeded.

@veeso veeso requested a review from ThetaSinner March 18, 2026 15:09
@veeso veeso merged commit 46fe784 into main Mar 18, 2026
23 of 36 checks passed
@veeso veeso deleted the ci/fix-count-eligible-nodes-no-running-jobs branch March 18, 2026 15:36
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