nixos/github-runner: allow adding custom runner name suffix

[kwbauson]

Description of changes

This change can be useful in situations where you may want to run more than a single instance of github-runner per nixos host and auto-scale those hosts, such as in an AWS auto scaling group. Also if no name is provided, it lets github-runner discover the hostname of the machine, such assigned hostnames from AWS/GCP

Example:

  services.github-runners = genAttrs runnerNames (name: {
    enable = true;
    ephemeral = true;
    replace = true;
    rawNameSuffix = ''-''${HOSTNAME%%.*}'';
    ...
  });

Things done

• 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/)
• 23.05 Release Notes (or backporting 22.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.

[veehaitch]

Thanks for the PR @kwbauson! Unfortunately, I didn't see this until now. Feel free to ping me in the future (and I'm quite sure @newAM and @aanderse would also be happy to take a look)!

It definitely makes sense to omit the --name argument altogether if the NixOS module option is not defined. I wonder, however, if we should just adopt name to accept types.str and remove the shell escaping. Or am I missing something?

[newAM]

What's the advantage of this vs doing it in nix?

{
  services.github-runners = listToAttrs (map (runnerName:
    {
      name = "${runnerName}-${config.networking.hostName}";
      value = {
        enable = true;
        ephemeral = true;
        replace = true;
        # ...
      };
    }
    runnerNames));
}

[newAM]

Suggested change

	rawNameSuffix = mkOption {
	nameSuffix = mkOption {

Minor bikeshed.

[kwbauson]

I ended up just using name and removing the escapeShellArg, so removed this option

[aanderse]

What's the advantage of this vs doing it in nix?

Yeah, the complexity of this module has grown more than I would like already... this change doesn't help lower the complexity.

[kwbauson]

What's the advantage of this vs doing it in nix?

Yeah, the complexity of this module has grown more than I would like already... this change doesn't help lower the complexity.

Yeah I don't love that I've had to do this, but unfortunately from what I can tell GitHub rejects multiple runners with the same name (I haven't found documentation except this thread). My use case is that I want multiple runner instances in each machine of an AWS autoscaling group, so I can't use the nixos defined hostname since it'll just be the same for each autoscaling instance. But by not setting the hostname at all $HOSTNAME is the AWS assigned hostname, allowing me to have a setup like this in github:

which is two auto-scaled machines with four runners each.

Hope that makes sense, and if you have any alternatives I'm all ears! Injecting from bash feels a bit gross 😅

[aanderse]

@kwbauson I will mention that the module authors have done an absolute lovely job of making the github-runner/github-runners module(s) very extensible. I have some machines that run github-runner but have special requirements which make all the systemd hardening a real burden - it was trivial to override the NixOS options with my customized version of the upstream module.

Check it out and see if you think the ideas here can help you.

If you still feel like this functionality is absolutely necessary and belongs upstream I guess we can keep discussing.

[kwbauson]

Yeah I'm a huge fan of this module and its configurability! Also very glad about the systemd hardening, makes me feel a lot more confident using it!

The reason for my suggested change is that:

• as-is, the name is hard-coded at nix build time, but for my use-case I need unique dynamic names at runtime
• the name that's set is a few levels deep in the service config and passed to the configure script within a let block which isn't quite overrideable enough, so I'd have to copy out most of the logic to configure the runner to override ExecStartPre

[aanderse]

Thinking about your issue a little bit more it makes me wonder if we went the right path with generating unique systemd services to start with... Why don't we switch to using systemd templates? A more robust answer may be for us to take advantage of the new systemd.services..overrideStrategy option... 🤔 I think that can resolve your issue, right?

[kwbauson]

Hmmm, I don't think so by itself? It could be an alternative to the existing serviceOverrides option, but I'm happy with that 😅. The reason I can't just override a small piece of the module for my case (multiple deploys of the same nixos configuration having unique names for their runners) is that github-runner itself is largely configured in bash, so any "odd" configuration like this has to be built into the module instead of letting the runner program handle it. If github-runner allowed itself to be configured by environment variables or json, it'd be easier to allow arbitrary module-consumer changes, but sadly the runner doesn't seem to support that

[aanderse]

If there was a github-runner@.service template you could use any mechanism you wanted say runtime to fire off additional instances of the service using data that isn't available at build time. Additionally, using overrideStrategy would allow us to continue to define runners at build time as we currently do. I'm not sure what part of the problem isn't solved by this.

Maybe you're not understanding what I'm proposing?

[kwbauson]

Ahhhh yeah sorry, I wasn't understanding you were suggesting moving away from setting the name in nix. I'll try to make an update to use overrideStrategy in the next couple of days.