nixos/systemd: add StartLimitIntervalSec to unit config #50849

cyounkins · 2018-11-20T17:37:52Z

Motivation for this change

Adds the new attribute to configure unit start rate limiting.
Documentation: https://www.freedesktop.org/software/systemd/man/systemd.unit.html
Ref #45785

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

grahamc · 2018-11-20T17:39:07Z

nixos/modules/system/boot/systemd-unit-options.nix

@@ -210,6 +210,16 @@ in rec {
      '';
    };

+    startLimitIntervalSec = mkOption {
+       default = 0;


Probably shouldn't have a default.

I'm not sure I understand. When default is set as it is here, the attribute is not written out to the systemd unit file. Why it isn't written isn't quite clear to me.

If I remove the default = 0; line, I get The option systemd.services.audit.startLimitIntervalSec' is used but not defined.` for all services.

infinisil · 2018-11-20T20:53:41Z

nixos/modules/system/boot/systemd.nix

@@ -219,7 +219,9 @@ let
        // optionalAttrs (config.documentation != []) {
          Documentation = toString config.documentation; }
        // optionalAttrs (config.onFailure != []) {
-          OnFailure = toString config.onFailure;
+          OnFailure = toString config.onFailure; }
+        // optionalAttrs (config.startLimitIntervalSec != []) {


Remove the default, add an options argument at line 196, then change this here to options.startLimitIntervalSec.isDefined

Thanks! It looks like the comparisons at

nixpkgs/nixos/modules/system/boot/systemd.nix

Line 199 in 04b4ca3

optionalAttrs (config.requires != [])

are against their defaults. Should they be changed to use isDefined?

I'm actually not sure now what would be better

Property Comparing with default Using isDefined Importance (subjective)

Defining as default doesn't trigger generation Yes No 2

Is usually done Yes No 5

Can use value without having set it (getting an empty list makes sense and is useful) Yes No 10

No need to duplicate upstream defaults No Yes 10

Reflects the fact that set values have an influence, unset ones don't No Yes 3

Both have advantages and disadvantages. Assigning some subjective importances to these properties, it's 17 vs. 13. The fact that we don't need to duplicate upstream defaults is a pretty good seller for me, but this also means we can't use these config values without having set them, which can be very useful.

After this consideration, I think it would be better to compare everything with the default value instead, so you could change it back again to the original way and use the upstream default is the options default. Now maybe other people have different opinions, but this is mine :)

@grahamc Could we get your opinion?

@infinisil You seem to suggest that "Defining as default doesn't trigger generation" is a good thing - I would say the opposite. If I explicitly and concretely specify a parameter (eg 0), I would want it written to the config file even if it matches the default. Without the intermediate nix layer, I would have explicitly written it to the config myself.

If I explicitly and abstractly specify the parameter as default, I don't have a strong opinion on if it should be written to the config. I could go either way.

I guess that's a major part of Nix that perhaps needs more documentation. It aims to be the configuration of all configurations, a superset of all others, and we should be consistent on how we handle different styles of configuration.

Could you elaborate on your thinking when you say "Can use value without having set it (getting an empty list makes sense and is useful)" - While getting an empty list makes sense, I don't see how it's useful if nothing is written out. I guess you mean that you can use the variable the same way whether or not it was specified instead of conditioning on isDefined?

rasendubi · 2018-12-10T00:04:56Z

(triage) @infinisil is PR good to go now?

rasendubi · 2019-01-04T00:25:42Z

(triage) @grahamc could you chime in?

infinisil · 2019-01-08T03:04:50Z

Actually, why do we need this option at all? Users can just set serviceConfig.StartLimitIntervalSec

cyounkins · 2019-01-08T03:16:11Z

@infinisil can you elaborate? I see two references to StartLimitIntervalSec in the repo, but do those actually work? I don't have access to my test box right now. I thought all the unit configuration options needed to be enumerated in nixos/modules/system/boot/systemd-unit-options.nix for them to be usable.

This commit also adds the documentation in line with all the other unit configuration options.

infinisil

Ah sorry you're right, I actually didn't know that, but it makes sense looking at the code.

infinisil · 2019-01-08T15:15:58Z

Only thing left to do is squash the commits into one. The isDefined thing should be fine.

cyounkins · 2019-01-27T23:04:19Z

You can squash in the GitHub UI right? Or do you need me to squash them?

infinisil · 2019-01-27T23:16:02Z

I prefer the PR owners to do the squash. This keeps potential signatures intact, makes the PR have the same commit hash as the merged commit, and it lets the PR author write a suitable combined commit message.

cyounkins · 2019-01-28T00:43:22Z

@infinisil Done!

nixos-discourse · 2024-08-10T17:22:04Z

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

https://discourse.nixos.org/t/module-with-a-systemd-service-option/50421/1

cyounkins requested a review from infinisil as a code owner November 20, 2018 17:37

grahamc reviewed Nov 20, 2018

View reviewed changes

infinisil requested changes Nov 20, 2018

View reviewed changes

cyounkins force-pushed the startlimitinterval branch from d64d3ca to ef54417 Compare November 20, 2018 21:09

oxij mentioned this pull request Nov 23, 2018

nixos/tor: add HiddenServiceVersion option #50950

Merged

10 tasks

cyounkins mentioned this pull request Dec 6, 2018

nixos: Decide and document scope of configuration #51630

Open

infinisil approved these changes Jan 8, 2019

View reviewed changes

nixos/systemd: add StartLimitIntervalSec to unit config

6f6287f

cyounkins force-pushed the startlimitinterval branch from ef54417 to 6f6287f Compare January 28, 2019 00:31

infinisil merged commit 2a40236 into NixOS:master Jan 28, 2019

Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/systemd: add StartLimitIntervalSec to unit config #50849

nixos/systemd: add StartLimitIntervalSec to unit config #50849

cyounkins commented Nov 20, 2018 •

edited

Loading

grahamc Nov 20, 2018

cyounkins Nov 20, 2018

infinisil Nov 20, 2018

cyounkins Nov 20, 2018

infinisil Nov 21, 2018

cyounkins Nov 21, 2018

rasendubi commented Dec 10, 2018

rasendubi commented Jan 4, 2019

infinisil commented Jan 8, 2019

cyounkins commented Jan 8, 2019

infinisil left a comment

infinisil commented Jan 8, 2019

cyounkins commented Jan 27, 2019

infinisil commented Jan 27, 2019

cyounkins commented Jan 28, 2019

nixos-discourse commented Aug 10, 2024

Property	Comparing with default	Using `isDefined`	Importance (subjective)
Defining as default doesn't trigger generation	Yes	No	2
Is usually done	Yes	No	5
Can use value without having set it (getting an empty list makes sense and is useful)	Yes	No	10
No need to duplicate upstream defaults	No	Yes	10
Reflects the fact that set values have an influence, unset ones don't	No	Yes	3

nixos/systemd: add StartLimitIntervalSec to unit config #50849

nixos/systemd: add StartLimitIntervalSec to unit config #50849

Conversation

cyounkins commented Nov 20, 2018 • edited Loading

Motivation for this change

Things done

grahamc Nov 20, 2018

Choose a reason for hiding this comment

cyounkins Nov 20, 2018

Choose a reason for hiding this comment

infinisil Nov 20, 2018

Choose a reason for hiding this comment

cyounkins Nov 20, 2018

Choose a reason for hiding this comment

infinisil Nov 21, 2018

Choose a reason for hiding this comment

cyounkins Nov 21, 2018

Choose a reason for hiding this comment

rasendubi commented Dec 10, 2018

rasendubi commented Jan 4, 2019

infinisil commented Jan 8, 2019

cyounkins commented Jan 8, 2019

infinisil left a comment

Choose a reason for hiding this comment

infinisil commented Jan 8, 2019

cyounkins commented Jan 27, 2019

infinisil commented Jan 27, 2019

cyounkins commented Jan 28, 2019

nixos-discourse commented Aug 10, 2024

cyounkins commented Nov 20, 2018 •

edited

Loading