Skip to content

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

Merged
fabrizio2210 merged 3 commits into
agent/rpc-free-node-real-memoryfrom
SCHED-1897-new-idle-memory-check
Jul 15, 2026
Merged

SCHED-1897: Add idle node memory health check#2742
fabrizio2210 merged 3 commits into
agent/rpc-free-node-real-memoryfrom
SCHED-1897-new-idle-memory-check

Conversation

@fabrizio2210

@fabrizio2210 fabrizio2210 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Historical status

This PR was accidentally merged into the #2743 feature branch. It is retained as the historical review record and should not be used as the active dependency stack.

The clean replacement stack is:

  1. Expose node RealMemory without controller RPC #2774 — expose node RealMemory without a controller RPC
  2. SCHED-1897: Add idle node memory health check #2773 — add the dependent idle-memory health check

Original scope

This PR introduced the SCHED-1897 idle_mem_used periodic health check. It uses local scontrol listjobs for RPC-free idle detection, consumes authoritative Slurm RealMemory, derives the idle-memory threshold from MemTotal - RealMemory, and drains idle nodes with an actionable diagnostic when available memory is unexpectedly low.

See #2773 for the complete replacement description, validation, and isolated five-file feature diff.

@fabrizio2210 fabrizio2210 added feature and removed new labels Jul 14, 2026
@fabrizio2210
fabrizio2210 marked this pull request as ready for review July 14, 2026 15:23
"reason_append_details": true,
"run_in_jail": false,
"log": "slurm_scripts/$worker.$name.$context.out",
"need_env": ["CHECKS_NODE_STATE_FLAGS"]

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.

This env var is populated based on scontrol show node output, so it won't allow us to get rid of these RPCs completely. It doesn't run in the Prolog context, so it's better, but just highlighting that RPCs are still in place.
Theoretically, we can check for node idleness by using scontrol listjobs, which doesn't make any RPCs to the Slurm controller (it requests the local worker instead of the controller).

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.

os.environ["CHECKS_NODE_STATE_FLAGS"] = "+".join(get_node_info().state_flags)

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.

I was aware of keeping an RPC, but I didn't know about scontrol listjobs. I will substitute it

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.

In an empty node it returns RC 1, is it correct?

$ kubectl -n soperator exec worker-0 -- chroot /mnt/jail  scontrol listjobs
No slurmstepd's found on this node
command terminated with exit code 1

While during the job:

$ kubectl -n soperator exec login-0 -- chroot /mnt/jail srun --mem=2048 -N1 -w worker-1 scontrol listjobs
cpu-bind=MASK - worker-1, task  0  0 [3560]: mask 0x3 set
JOBID
1011    

@fabrizio2210 fabrizio2210 Jul 15, 2026

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.

My understanding is that RC=1 is expected, also when running hc_program because it doesn't spawn the slurmstepd process. I updated the code

@@ -0,0 +1,16 @@
{
"name": "idle_mem_used",
"command": "IDLE_MEM_USED_MAX_USED_GB={{ required `Idle memory maximum used GB must be provided.` (index .Values.slurmScripts.builtIn `idle_mem_used.sh`).maxUsedGB }} ./idle_mem_used.sh",

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.

This is a good start, but I think we should eliminate the need to pass this setting to Helm values for each cluster.
How it is now:

  • Slurm nodes have RealMemory setting. It shows the amount of RAM on this node that Slurm will use.
  • Soperator guarantees that this setting is equal to the worker K8s pods memory limit.
    But when we have a separate Helm value, then there is no such guarantee anymore. A person can update one of the values but forget to update another, which can lead to nodes being drained wrongly.

What I suggest:

  • Update the nodeset reconciliation logic.
  • When Soperator creates the worker statefulset with some specific memory limit, it also adds the same memory limit to the environment variable for this container
  • Container entrypoint (slurmd_entrypoint.sh) reads this env var, and stores it to a file on the ephemeral container filesystem
  • The check runner reads this file, instead of using the Helm value + env var

The solution can also be generalized for other cases and values where we need to communicate some metadata about the node from Soperator to various scripts (i.e., VM ID, GPU platform, the presence of IB, or any other metadata that would be useful in other scripts without querying KubeAPI or other services).

For example, we can have a Helm value like "Soperator exported metadata env", which is a list of environment variable names that Soperator should export inside the node.
Then, in the entrypoint script, we write an env file to some node-local volume inside the jail (e.g., /etc/soperator_exported_meta.env).
And then any script (ours or the user's) that runs there can execute source /etc/soperator_exported_meta.env and have this metadata available.
It's also customizable and configurable. We can export more vars if needed by adding them to the StatefulSet spec and allowing them in the Helm value.
Or maybe we can put all environment variables there, even without allowlists.

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.

I see, it seems interesting, so we can be reusable. But it seems a bigger/separate change. Do you want me to create a separate PR? And then stack this (i.e. 2742) PR on top of it?

@fabrizio2210
fabrizio2210 force-pushed the SCHED-1897-new-idle-memory-check branch from fbeb44e to 2f53da1 Compare July 15, 2026 12:04
@fabrizio2210
fabrizio2210 changed the base branch from main to agent/rpc-free-node-real-memory July 15, 2026 12:05
@fabrizio2210
fabrizio2210 force-pushed the agent/rpc-free-node-real-memory branch from 9b5ea2c to 049181b Compare July 15, 2026 12:30
@fabrizio2210
fabrizio2210 force-pushed the SCHED-1897-new-idle-memory-check branch from 2f53da1 to 4ca0fad Compare July 15, 2026 12:31
@fabrizio2210
fabrizio2210 merged commit a39a0ac into agent/rpc-free-node-real-memory Jul 15, 2026
22 checks passed
@fabrizio2210
fabrizio2210 deleted the SCHED-1897-new-idle-memory-check branch July 15, 2026 14:18
@fabrizio2210

Copy link
Copy Markdown
Collaborator Author

By mistake I collapsed this request onto #2743

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.

2 participants