Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NixOS services: When to have a package option #50476

Closed
infinisil opened this issue Nov 16, 2018 · 22 comments
Closed

NixOS services: When to have a package option #50476

infinisil opened this issue Nov 16, 2018 · 22 comments
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: policy discussion 9.needs: community feedback

Comments

@infinisil
Copy link
Member

infinisil commented Nov 16, 2018

Issue description

It has become somewhat of a pattern to add a package option to NixOS service modules for selecting a package to use for the service. Doing a quick count, of the ~600 service modules in nixpkgs about 120 of them have a package option. For comparison, the total amount of options in nixpkgs is about 7000. Every new NixOS option increases evaluation time for all NixOS system evaluations, whether you use it or not. Adding an option just because other services do it doesn't sound sensible. I'd rather add this option only once a need for it arises.

This is an issue to track opinions about this.

Alternative

In general, all packages can already be overridden via overlays. To override the package foo with foo2:

{
  nixpkgs.overlays = [(self: super: {
    foo = self.foo2;
  })];
}

The need for such an option

The 3 uses I see for a package option are:

  • There are alternate packages you would want to use and being able to use different packages is an integral part of the module. Having an option makes this possibility discoverable. postgresql is such an example (postgresql_9_3, ..., postgresql_11).
  • Using an overlay for it would require a lot of rebuilds, because overlays not only change the package the service uses, but all the other parts of the system as well. curl would be such an example.
  • Using an overlay for it would change some other part of the system you don't want to change. E.g. 2 services that need the same package but different versions of it. An overlay couldn't change just one of them.

Related

For reference, I used this to find these:

while true; do firefox $(git blame $(fzf) | rg '([^ ]+) .*package = mkOption' -or 'https://github.com/NixOS/nixpkgs/commit/$1'); done

Related discussions

Justified addings of the option

Commits/PRs that add the option without justification

Mostly these are probably just people not knowing they can use overlays to do it. I'm pinging all of you in the hopes that you see that overlays could have been used instead or to potentially find other reasons to introduce such an option.

@domenkozar
Copy link
Member

I'd be happy to discuss this once we see how much impact do these 600 options have on evaluation time :)

@infinisil
Copy link
Member Author

A benchmark sure would be nice, but I'm mainly concerned about option bloat in general, having lots of options that can be achieved another way and aren't used by anybody.

@zimbatm
Copy link
Member

zimbatm commented Nov 16, 2018

The package option is quite convenient, it allows to select a different version without reading the module source or triggering too many rebuilds.

@copumpkin
Copy link
Member

My vote is to put it in for all services that have a relevant notion of packages backing them. The main reason for this is that it's not always clear which packages are being used by a service (e.g., we have services.httpd which uses pkgs.apacheHttpd; yes the module lives in services/apache-httpd but in general nothing requires them to line up) so unless someone pokes at the nixpkgs source code, they don't necessarily know which module to override and there's no real feedback to figure out which was overridden other than hoping to see the .service unit get rebuilt if you got it right.

The reasons you give in favor of the field also factor into my preference. I'm already frustrated at how "singleton"-flavored our services all are, so forcing all of them to go through a single registry of versions seems annoying.

@infinisil
Copy link
Member Author

@copumpkin That's a good point, I haven't thought of this.

Eventually I hope that we can have a generic service module creator function that contains stuff like enable, user, group, package, extraArgs, configFile, etc. (with some configurability)

@matthewbauer
Copy link
Member

I favor the idea of using overlays here. You should be able to accomplish the same thing as the package option with a simple overlay of pkgs. This works without the extra code of implementing and documenting extra options.

@jtojnar
Copy link
Member

jtojnar commented Nov 17, 2018

Because there are multiple upower versions now: df95a8c

Not any more 2a2cb83#diff-0bcefda72ac48cd4aa6372226ec3f6a7

@7c6f434c
Copy link
Member

7c6f434c commented Nov 17, 2018 via email

@Ekleog
Copy link
Member

Ekleog commented Nov 17, 2018

@7c6f434c See also NixOS/rfcs#22

@LnL7
Copy link
Member

LnL7 commented Nov 17, 2018

The package option is also useful for overrides or even using a different channel. Having to override the default attribute means there's no choice to do this in a contained way anymore. Even when using overlays I always do this instead.

{
  services.foo.package = pkgs.namespace.foo;
  nixpkgs.overlays = [(self: super: {
     namespace.foo = super.foo.override { withBar = true; };
  })];
}

@Ekleog
Copy link
Member

Ekleog commented Nov 17, 2018

I've tried benchmarking with this commit.

The results are, for a removal of 19 package = mkOption arguments (got bored after that), for my configuration on my computer, all tested with time nixos-rebuild -I nixpkgs=../prog/nixpkgs build && time nixos-rebuild -I nixpkgs=../prog/nixpkgs build after the actual build was already complete, to warm up the disk cache:

  • Before, 9.47s user 0.68s system 133% cpu 7.599 total
  • After, 9.11s user 0.56s system 135% cpu 7.118 total

I also removed package lines that might be useful, given I didn't care about having an actually useful nixpkgs in the end, just to benchmark removal of a few options. Feel free to remove more options on top of that commit and to bench again on your side :)

Special mention for hadoop, that has a package argument that's never used (at least in its file, and whose removal did not to cause an evaluation error).

@joachifm
Copy link
Contributor

joachifm commented Nov 17, 2018

To me, the problem is mechanically adding options "just because" we can. Sure, there are cases where it makes sense to expose the underlying package, but it seems preferable to default to treating the package(s) as an implementation detail to be exposed only when absolutely necessary. My view is that fewer options is better and no options is best (regardless of eval time).
EDIT: I realized my original wording was a little bit uncharitable, sorry

@7c6f434c
Copy link
Member

@joachifm I guess a nontrivial subset of Nixpkgs/NixOS developers have experience that unchangeable defaults end up in a state not fitting our needs. This leads to the ideal of having a lot of knobs that can be ignored if everything acceptable, but provide an escape hatch if something exotic/complicated is desired. Then it turns out there are evaluation time issues…

I might be wrong, as I don't use NixOS proper anymore, and I use only a reasonably small subset of NixOS module code. (A few unchangeable defaults ended p annoying me too much).

@coretemp
Copy link
Contributor

Essentially echoing @7c6f434c with some details:

The overlay system provides a single global overlay. There is no way to say that you want one service to use an overridden version and for another you don't, which is actually something I use currently.

There are workarounds for that too, but your proposed solution is inferior.

AFAIK, I need to fork the module in order to modify it to my needs in that case. Having a package option means I need to fork less often, which is what most people probably want.

P.S. Kind of curious as to what @7c6f434c is using.

@joachifm
Copy link
Contributor

joachifm commented Nov 17, 2018

@7c6f434c that's fair, but I take it you don't think we should add options willy nilly without any regard for whether they make sense? All I really ask is for some justification. It doesn't have to make sense for everybody, but it has to make sense (realistically) for somebody.

I see several general claims that overriding a specific package only for a specific module is useful but few concrete examples. For it to be a general policy I'd hope we could agree that it has to make sense more often than not.

I think there are real costs associated with greater variability implied by adding options beyond eval time, and so that there is a benefit to being selective about what is added.

@7c6f434c
Copy link
Member

@joachifm About overlays — maybe in general, given discussions like #43266 not everyone fully trusts overlays for all use cases. And we usually ship a single client+server package and people might want to override the two parts independently.

You are not alone in thinking there is a cost — different people get different levels of annoyance from having to deal with an interface with many optional knobs, and also from not having access to obvious knobs. I guess we never had a Nix* survery to know the local shape of the distribution (there is a weird enough selection criterion that I don't expect general population studies of option paralysis, cost of choice etc. to apply)

Of course, reminding people about disabledModules to replace a single module (like in #46464 discussion) is useful.

@coretemp I guess explicitly saying what service you are talking about might help to discuss specifics.

Re: P.S. — feel free to email me or open an issue about lack of documentation against my lang-os repo or something; by the way, there you can find an example of grabbing things out of a separate NixOS import — maybe this can be combined with overriding Nixpkgs in a reimport of NixOS for some intents.

@coretemp
Copy link
Contributor

@7c6f434c For $CENSORED_REASONS I was vague on this topic. It was a fair question. Perhaps I can answer the question in a fairly long time.

@Shados
Copy link
Member

Shados commented May 10, 2019

@Ekleog

* Before, `9.47s user 0.68s system 133% cpu 7.599 total`

* After, `9.11s user 0.56s system 135% cpu 7.118 total`

This is interesting, but for the results to be useful you'd really need to take some steps to exclude other factors from influencing the benchmark (e.g. by running each case many times over and looking at the distribution of results). 0.48s is a lot more of a change than I would expect for only 19 less options; it seems likely the difference has more to do with other things happening on the system during the tests.


Some thoughts:

  • A package option is the only visible thing blocking nixos/darkhttpd: module to enable darkhttpd #48036 and dnscrypt-proxy2: 2.0.15 -> 2.0.17, add NixOS module #48289 from being merged, which is odd given this issue doesn't appear to have reached any kind of consensus.
  • While it is true you can use overlays to override service packages, this becomes more problematic if you are sharing a nixpkgs build between several machines (e.g. because you're using nixops or morph) and only want to override the package on one of them (example issue using morph). This is an issue that could be fixed with nixpkgs changes, but is not at this point.
  • The stated concerns are evaluation time, and "option bloat in general". Is impact on evaluation time significant? Is "option bloat" an issue for non-performance reasons? Will the answers to those questions still be true if/when either nix-flakes, or configurations as a language feature, are implemented?

@zimbatm
Copy link
Member

zimbatm commented May 16, 2019

I propose to close this issue and go toward adding package attribute to all the modules that are backed by a single package.

Without a proper benchmark it's not worth talking about hypothetical performance improvements. NixOS evaluation is highly sensitive to the disk cache and needs to be repeated many times.

On the other hand, having a consistent interface is useful on many levels. Like @copumpkin said, the user can rely on the option being there and not have to dig into the nixos source. Not everybody has write access to nixpkgs to add the package option "when needed".

@infinisil
Copy link
Member Author

I think the ideal way to solve this would be to add a generic nixos.services.* option (or tasks.* or jobs.* or whatever), defined somewhat like this:

{
  options.nixos.services = mkOption {
    type = types.attrsOf (types.submodule {
      options = {
        package = mkOption {
          type = types.package;
        };
        
        user = mkOption {
          type = types.str;
        };

        systemd = mkOption {
          type = /* ... */;
          description = "systemd.services.* link";
        };
      };
    });
  };
}

This is then like a layer on top of systemd.services which defines all the additional options NixOS services ought to have, like package, user, group, maybe even dataDir, config and/or configFile.

In this new abstraction layer we could define all these options without having to clutter all service definitions with duplicate definitions.

This would then be customizable like this:

{ pkgs, ... }: {
  services.foo.service.package = pkgs.foo_1;
  services.foo.someFooSpecificOption = 10;
  services.foo.service.systemd.preStart = "echo hi";
}

Where services.foo.service is a link to the corresponding nixos.services.foo option set.

@stale
Copy link

stale bot commented Jun 2, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@infinisil
Copy link
Member Author

I guess we can close this, most people seem to be for always having a package option. Previously I was against this, but I've since changed my opinion and think that it might very well be alright to always have it, even if it's not immediately useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: policy discussion 9.needs: community feedback
Projects
None yet
Development

No branches or pull requests