Skip to content

SCHED-1897: Add idle node memory health check#2773

Open
fabrizio2210 wants to merge 8 commits into
mainfrom
agent/sched-1897-idle-memory-check
Open

SCHED-1897: Add idle node memory health check#2773
fabrizio2210 wants to merge 8 commits into
mainfrom
agent/sched-1897-idle-memory-check

Conversation

@fabrizio2210

@fabrizio2210 fabrizio2210 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add the idle_mem_used periodic health check for hc_program
  • determine whether the node has local jobs with RPC-free scontrol listjobs
  • consume the authoritative Slurm RealMemory value exposed by Expose node RealMemory without controller RPC #2774
  • derive the maximum idle usage as MemTotal - RealMemory, without a separate Helm threshold
  • drain idle nodes whose MemAvailable is below RealMemory, with an actionable diagnostic reason
  • add shell behavior and Helm rendering coverage

Why

This PR is related to the SCHED-1897 Jira ticket. Idle nodes retaining unexpectedly high memory usage may have leaked or spurious processes left behind by previous jobs. Detecting that condition periodically prevents affected nodes from silently returning to service.

Using local scontrol listjobs avoids a controller RPC for idle detection. Deriving the threshold from Slurm RealMemory keeps the check aligned with the worker memory configured by Soperator.

Dependency

This PR is stacked on #2774, which publishes node RealMemory locally and makes CHECKS_NODE_REAL_MEM_BYTES RPC-free in the normal path. Merge #2774 first, then retarget this PR to main.

This replaces the feature diff from #2742. That PR was accidentally merged into #2743; #2774 replaces the detached #2743 pull ref and this PR carries only the dependent idle-memory check.

Impact

The check is enabled by default and runs only through Slurm's HealthCheckProgram. Nodes with local Slurm jobs are left unchanged. Unexpected listjobs failures, unavailable memory data, missing RealMemory, and inconsistent values fail open.

On failure, the drain reason reports available memory, Slurm RealMemory, the deficit, current used memory, and the derived maximum idle usage.

Validation

  • python3 -m unittest discover -s helm/slurm-cluster/slurm_scripts -p '*_test.py' -v (15 tests passed)
  • bash -n helm/slurm-cluster/slurm_scripts/idle_mem_used.sh
  • parsed the check specification as JSON
  • parsed helm/slurm-cluster/tests/idle_mem_used_test.yaml as YAML
  • git diff --check agent/rpc-free-node-real-memory...HEAD
  • confirmed the stable patch ID exactly matches the original SCHED-1897: Add idle node memory health check #2742 feature diff

@fabrizio2210
fabrizio2210 force-pushed the agent/rpc-free-node-real-memory branch from 8ba59e7 to 137f04e Compare July 23, 2026 10:12
@fabrizio2210
fabrizio2210 force-pushed the agent/sched-1897-idle-memory-check branch from f883250 to 7ae1a4e Compare July 23, 2026 10:12
Base automatically changed from agent/rpc-free-node-real-memory to main July 23, 2026 12:49
@fabrizio2210
fabrizio2210 force-pushed the agent/sched-1897-idle-memory-check branch from 7ae1a4e to 9e0cbb1 Compare July 23, 2026 12:49
Comment thread helm/slurm-cluster/slurm_scripts/idle_mem_used.sh Outdated
@fabrizio2210
fabrizio2210 marked this pull request as ready for review July 23, 2026 13:15
@fabrizio2210
fabrizio2210 requested a review from rdjjke July 23, 2026 13:16
Comment thread helm/slurm-cluster/slurm_scripts/idle_mem_used.drain.sh
@@ -0,0 +1,91 @@
#!/bin/bash

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some of our checks are already written in Python. Given that this check has Python tests, maybe write it in Python as well, and the testing part would be even easier?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Python would simplify JSON parsing and enable more direct unit tests. However, most of the checks in this folder currently invoke shell scripts, and these Python tests deliberately exercise the shell command as a black box.
The scripts are mostly set of system commands.
I suggest keeping this check in shell for this PR and reconsidering Python if its logic grows.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's a historical mistake that we wrote these checks in Bash. Bash scripts are getting more and more complex, and it's very hard for humans to deal with this, even though they are written by AI agents now.
E.g., in this script, there is JSON parsing and several other parsers and validators; to work with them, you have to be a bash+jq+awk+whatever tools are used. With Python, you only need to be a Python expert.

But it's up to you, we have a lot of bash already :-)

Comment thread helm/slurm-cluster/slurm_scripts/idle_mem_used.sh Outdated
Comment thread helm/slurm-cluster/slurm_scripts/idle_mem_used.drain.sh.json
Comment thread helm/slurm-cluster/slurm_scripts/idle_mem_used.drain.sh.json
Comment thread helm/slurm-cluster/slurm_scripts/idle_mem_used.sh.json Outdated
"node_states": ["any"],
"on_fail": "drain",
"on_ok": "none",
"reason_base": "[user_problem] $name",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@rdjjke We're introducing a new drain reason and it is fine. But what about the old alloc_mem_used drain reason? When we remove the old check we might have drained nodes which will never automatically be undrained.
So, we can avoid this by defining alloc_mem_used as a drain reason here, so we carry over the drain reason. Wht do you think? Do we have a lot of those nodes drained for this reason?

if (.jobs | type) == "array" then
.jobs | length
else
error(".jobs must be an array")

@theyoprst theyoprst Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What if the next version of Slurm returns not [], but null? Or even skip the job key (like in Go by default)? Looks like this check will stop working, always exiting 0

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants