nixos/slurm: rewrite module to RFC 0042 style settings

[markuskowa]

Motivation for this change

Use free-form settings following RFC 0042 to generate config files. This should make the module more flexible, but it also is a breaking change, which requires users to adapt their config. In either case, it should make the module easier to maintain.

See also #144575

Things done

• Added a note to the release notes
• Re-write of module to use settings for slurm.conf, slurmdbd.cong, and cgroup.conf
• Adapted the slurm test.
• Remove and older "removed" options message
• Removed the Slurm tools wrapper, which fed SLURM_CONF to all binaries. It is now exported directly as a system-wide environment variable.

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.05 Release Notes (or backporting 21.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[markuskowa]

@GrahamcOfBorg test slurm

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/8

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/758

[markuskowa]

Rebased for 22.11 release.

[RaitoBezarius]

So sorry this did not get any attention, can you fix the conflicts? I'm interested into getting this merged. Though, I do not have extensive experience with Slurm.

[markuskowa]

@RaitoBezarius There seems to be a bug that causes an infinite recursion which still needs to be fixed. However, due to the lack of feedback I did not prioritize working on it lately. It would also be good to get some feedback on the "settings" conversion.
I will pick this up again and hopefully we can include in the 23.05 release.

[RaitoBezarius]

I would advise keeping the legacy options for a release cycle, while deprecating them.
Then, we can proceed to remove them.

I don't know how many NixOS users of slurm are out there, but I think it would be annoying to find out about if some of them do not read the release notes.

[RaitoBezarius]

IMHO all the legacy options should be kept at least for one release cycle while putting a deprecation warning.
Then, we can all mkRemovedOption all of them after this.

[markuskowa]

I would prefer to make a hard change here for the following reason. It may lead to confusion when both, legacy and settings are used. Which one should be used if it affects one an the same setting? It would make the module more messy trying to handle both. The conversion of a config file should be rather straight forward.

We could use mkRenamedOptionModule where applicable and mkRemovedOption for the rest?
EDIT: I added this version now to the module to ease the transition.

[markuskowa]

@posch Are you using slurm on NixOS via the module?

[posch]

@posch Are you using slurm on NixOS via the module?

No, I'm not. I'm using a stand-alone slurm.conf. My Nixos config basically contains only the systemd services:

let
        mungeKeyFile = "/...";    
        slurmConfigFile = "/.../slurm.conf";
        slurmStateSaveLocation = "/var/spool/slurm/ctld";
in
{
        users.groups.munge = {};
        users.users.munge = {
                isSystemUser = true;
                group = "munge";
        };

        systemd.services.munge = {
                wantedBy = ["multi-user.target"];
                after = ["network.target" "time-sync.target"];
                serviceConfig = {
                        Type = "simple";
                        ExecStart = "${pkgs.munge}/bin/munged -F --key-file ${mungeKeyFile}";
                        ExecStartPre = [
                                "+${pkgs.coreutils}/bin/chown munge:munge ${mungeKeyFile}"
                        ];
                        PIDFile = "/run/munged.pid";
                        User = "munge";
                        Group = "munge";
                        Restart = "on-abort";
                        StateDirectory = "munge";
                        StateDirectoryMode = "0711";
                        RuntimeDirectory = "munge";
                };
        };      

        systemd.services.slurmctld = {
                wantedBy = [ "multi-user.target" ];
                after = [ "network.target" "munge.service" "nslcd.service" ];
                requires = [ "munge.service" ];
                serviceConfig = {
                        Type = "simple";
                        ExecStart = "/ngt/slurm/bin/slurmctld -D -f ${slurmConfigFile}";
                        PIDFile = "/run/slurmctld.pid";
                        ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
                        LimitNOFILE = 65536;
                };
                preStart = ''
                        mkdir -p ${slurmStateSaveLocation}
                '';

        };

        systemd.services.nslcd.after = [ "network.target" ];

[posch]

I understand that Nix attribute sets seem to be the obvious choice for Node lines, but it looks a bit overengineered to me. I usually copy+paste Node lines from the output of slurmd -C:

# /.../slurmd -C
NodeName=... CPUs=192 Boards=1 SocketsPerBoard=4 CoresPerSocket=24 ThreadsPerCore=2 RealMemory=772645

Is there a concrete advantage of having this split up and rewritten into an attribute set?

[markuskowa]

The advantages that I see is that you can now (1) access the parameters for a node or a partition directly and use them in another place in your config (e.g. for CPUs or memory). (2) The attributes can be merged, when they are defined in separate locations. This is otherwise only possible if you define nodeName, partitionName separately (now also nodeSet exists). (3) It makes the module easier to maintain and can automatically handle new features without the need to modify the module.

It may look over engineered at first glance, but I think it is more consistent to go all the way here.

[markuskowa]

@infinisil @RaitoBezarius Are good to go, or do you have other comments?

[RaitoBezarius]

To ease transition and offer escape hatches (as I know of one complicated NixOS module where it would have been great to have this), I wish this could be an actual option overrideable by the user who could just bypass settings and write the configuration file itself and the settings would be the defaults.

Plus, an assertion could be done to prevent settings & config file being written.

Once this is done, I think it would be fine to merge it. :)

[RaitoBezarius]

I appreciate the change, but it was not what I suggested — adding a extraConfig is not the same as adding an escape hatch that replaces the whole configFile and is an acceptable alternative to RFC42 rather than stringly-typed extraConfig IMHO (because the user can plug his generator).

[markuskowa]

Alright. I put the extraConfig back in. Its content now gets appended. You can now override settings completely by just setting it to an empty set.

[RaitoBezarius]

Thank you a lot for your patience!

[markuskowa]

OK, I think I see what you mean now. Maybe as it is right now, it is a good solution? Using an alternative configFile adds more complexity as we need to define StateSaveLocation and SlurmSpoolDir separately, since they are needed by the service.

[RaitoBezarius]

I think it's more or less fine, the only problem with the current solution is that it's not clear we can sidestep completely all the settings, but I think it's fine for now.

[RaitoBezarius]

@ofborg test slurm