-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
nixos/timesyncd: allow NTP servers advertised by DHCP to be used #335755
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -2,33 +2,52 @@ | |||
|
|
||||
| with lib; | ||||
|
|
||||
| let | ||||
| cfg = config.services.timesyncd; | ||||
| in | ||||
| { | ||||
|
|
||||
| options = { | ||||
|
|
||||
| services.timesyncd = { | ||||
| services.timesyncd = with types; { | ||||
| enable = mkOption { | ||||
| default = !config.boot.isContainer; | ||||
| defaultText = literalExpression "!config.boot.isContainer"; | ||||
| type = types.bool; | ||||
| type = bool; | ||||
| description = '' | ||||
| Enables the systemd NTP client daemon. | ||||
| ''; | ||||
| }; | ||||
| servers = mkOption { | ||||
| default = null; | ||||
| type = nullOr (listOf str); | ||||
| description = '' | ||||
| The set of NTP servers from which to synchronise. | ||||
|
|
||||
| Setting this option to an empty list will write `NTP=` to the | ||||
| `timesyncd.conf` file as opposed to setting this option to null which | ||||
| will remove `NTP=` entirely. | ||||
|
|
||||
| See man:timesyncd.conf(5) for details. | ||||
| ''; | ||||
| }; | ||||
| fallbackServers = mkOption { | ||||
| default = config.networking.timeServers; | ||||
| defaultText = literalExpression "config.networking.timeServers"; | ||||
| type = types.listOf types.str; | ||||
| type = nullOr (listOf str); | ||||
| description = '' | ||||
| The set of NTP servers from which to synchronise. | ||||
| Note if this is set to an empty list, the defaults systemd itself is | ||||
| compiled with ({0..4}.nixos.pool.ntp.org) apply, | ||||
| In case you want to disable timesyncd altogether, use the `enable` option. | ||||
| The set of fallback NTP servers from which to synchronise. | ||||
|
|
||||
| Setting this option to an empty list will write `FallbackNTP=` to the | ||||
| `timesyncd.conf` file as opposed to setting this option to null which | ||||
| will remove `FallbackNTP=` entirely. | ||||
|
|
||||
| See man:timesyncd.conf(5) for details. | ||||
| ''; | ||||
| }; | ||||
| extraConfig = mkOption { | ||||
| default = ""; | ||||
| type = types.lines; | ||||
| type = lines; | ||||
| example = '' | ||||
| PollIntervalMaxSec=180 | ||||
| ''; | ||||
|
|
@@ -41,7 +60,7 @@ with lib; | |||
| }; | ||||
| }; | ||||
|
|
||||
| config = mkIf config.services.timesyncd.enable { | ||||
| config = mkIf cfg.enable { | ||||
|
|
||||
| systemd.additionalUpstreamSystemUnits = [ "systemd-timesyncd.service" ]; | ||||
|
|
||||
|
|
@@ -82,9 +101,14 @@ with lib; | |||
|
|
||||
| environment.etc."systemd/timesyncd.conf".text = '' | ||||
| [Time] | ||||
| NTP=${concatStringsSep " " config.services.timesyncd.servers} | ||||
| ${config.services.timesyncd.extraConfig} | ||||
| ''; | ||||
| '' | ||||
| + optionalString (cfg.servers != null) '' | ||||
| NTP=${concatStringsSep " " cfg.servers} | ||||
|
||||
| mesonFlagsArray+=(-Dntp-servers="0.nixos.pool.ntp.org 1.nixos.pool.ntp.org 2.nixos.pool.ntp.org 3.nixos.pool.ntp.org") |
or remove the empty
NTP=altogether whenserversis the empty list:
I think we should remove it because we do not intent do explicitly remove any previously set value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just do not want to break compatibility for users who are setting networking.timeServers to something different than the compiled-in values (i.e. {0..4}.nixos.pool.ntp.org). Hence the necessity to add the fallbackServers option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to let the module user choose, and by default we do not write NTP= (with empty string) so there is no risk of resetting anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the advice from systemd experts.
It seems that my worries about drop-ins are not justified after all.
When the empty string is assigned, the list of NTP servers is reset, and all prior assignments will have no effect.
I believe there will be no differences between writing NTP= or not writing NTP= at all despite the systemd documentation. That is because we are configuring the "main configuration file" which has lower precedence and therefore, there are no prior assignments. See CONFIGURATION DIRECTORIES AND PRECEDENCE in man:timesyncd.conf(5).
Do you agree, should I remove the possibility to set null?
Uh oh!
There was an error while loading. Please reload this page.