SelectTypeParameters=CR_Core_Memory; DefMemPerCPU

[elelaysh]

With SelectTypeParameters=CR_Core_Memory, SLURM scheduler will consider memory
as a consumable resource:
a job will be scheduled to a node only if the node has sufficient remaining memory for it.

With default openhpc_cgroup_config, the job is constrained to not use more:

• memory (ConstrainRAMSpace=yes)
• swap (ConstrainSwapSpace=yes)

than requested. This is a change from previous SelectTypeParameters=CR_Core:
memory was not taken into account and a job's memory use was only limited
by the total memory and swap on the node at each time.

With SelectTypeParameters=CR_Core_Memory, DefMemPerCPU must be defined to prevent a job using a fraction of a node's CPUs from taking all the node's memory. We compute a default value as the minimal memory per thread accross the cluster but it can be changed in the appliance configuration.

Also create the ohpc_nodegroups_computed variable,
to share computed values per nodegroup between multiple places of the role.
If a nodegroup is in ohpc_nodegroups_computed it has at least one host. It stored

• first_host: name of the first host in the nodegroup; use hostvars[computed.first_host]
  to access its hostvars
• inventory_group_name: name of the inventory group for this nodegroup; use groups[computed.inventory_group_name] to access the host list

[gemini-code-assist]

Code Review

This pull request introduces a significant improvement by making memory a consumable resource in Slurm, which will help prevent node oversubscription. The refactoring of nodegroup-specific computations into the ohpc_nodegroup_computed variable is a great step towards better code organization and reusability.

My review includes a few suggestions:

• A high-severity suggestion to make the memory-per-CPU calculation more robust by using ansible_processor_vcpus and handling cases where this fact might be missing. This will prevent potential playbook failures.
• A couple of medium-severity comments to address inconsistent indentation (mixing tabs and spaces) in the Jinja2 template, which will improve code readability and maintainability.

[elelaysh]

Re ohpc_nodegroup_computed:

It is awkward to have the ohpc_nodegroup as a list (instead of a dict, because we can't have multiple nodegroups with same name, etc), but since it's an input parameter, it shouldn't be changed.

I had tried a version with all fields of ohpc_nodegroup copied to ohpc_nodegroup_computed, but it wasn't that useful in slurm.conf.j2 we need to iterate other empty nodegroups as well. I preferred ohpc_nodegroup_computed to only contain non-empty nodegroups, to have correct values for computed values ram_mb, def_mem_per_cpu(to later combine usingmin` without needing to filter).

If one would prefer to use ohpc_nodegroup_computed everywhere the code could be adapted.

Before, I tried computing the nodegroup and DefMemPerCPU for each host, but it was very awkward.

Also the naive approach would be to duplicate the whole block iterating over ohpc_nodegroup in slurm.conf.j2 to compute DefMemPerCPU. I hope having the ohpc_nodegroup_computed would be more useful.

We could also compute all slurm.conf attributes of each nodegroup in a variable, then just serialize it in slurm.conf.j2 (we can pick some attributes to put first the order of our choosing (NodeList, Features, State), then append the rest...

[elelaysh]

ohpc_nodegroup_computed should be plural I guess

[sjpb]

I think the PR description should describe what DefMemPerCPU is set to

[sjpb]

Otherwise looks good once merge conflict fixed

[elelaysh]

I think the PR description should describe what DefMemPerCPU is set to

Yes, good point

[elelaysh]

ohpc_nodegroup_computed should be plural I guess

Fixed it to be plural