add timesync_ntp_custom_settings variable for free-form local configs #54
Conversation
|
The role used to have some variables for adding settings directly to the generated config files, but IIRC they were removed in order to hide the implementation-specific details. Later was added a variable for selecting the NTP provider, so it may no longer be a goal. |
|
I would agree with this change, in fact I was checking how would be possible to add extra options in chrony.conf which should apply a similar change to the one proposed here. We rely on some customized settings and with the current templates we are not able to deploy them. I will create a new merge request for chrony based on this pull request (for keeping coherency in case that this is merged). |
|
[citest commit:ab66e75457880e7184a2d7b27d428f566ca7c40e] |
tests/tests_ntp.yml
Outdated
| @@ -11,6 +11,8 @@ | |||
| iburst: yes | |||
| minpoll: 4 | |||
| maxpoll: 6 | |||
| timesync_ntp_custom_settings: | |||
| - "tinker panic 0" | |||
There was a problem hiding this comment.
The test should check if this setting was applied correctly
There was a problem hiding this comment.
I'm not sure how to check this specific setting by inspecting runtime state, the closest I can come up with is looking for this line in the generated config file. Is that enough, or are you thinking something more?
There was a problem hiding this comment.
I'm not sure how to check this specific setting by inspecting runtime state, the closest I can come up with is looking for this line in the generated config file. Is that enough, or are you thinking something more?
I'll defer to @mlichvar about that - but IMO it is better to check the generated config file than to do nothing.
There was a problem hiding this comment.
There might be a possibility to do that with ntpq, but it would need authentication to be set up. I think it's better to check the generated file.
| # List of custom settings passed through to ntp.conf. If not defined, | ||
| # setting is ignored. Settings should be one per line (list item). | ||
| timesync_ntp_custom_settings: | ||
| - "tinker panic 0" |
There was a problem hiding this comment.
How idempotent is this? For example, if the user specifies
timesync_ntp_custom_settings:
- "tinker panic 0"
- "a b c"then sometime later specifies
timesync_ntp_custom_settings:
- "a b c"
- "tinker panic 0"Will that generate a change? Does it matter if that generates a change? For example, that will cause the ntp services to restart - is that ok?
There was a problem hiding this comment.
Do you want to allow the user to perform incremental changes? For example, if the configuration already has
timesync_ntp_custom_settings:
- "tinker panic 0"
- "a b c"and the user wants to remove "a b c" - how to do that? Or the user wants to remove "a b c" and add "b c a"? For the kernel_settings role we had a similar requirement, so we documented it - https://linux-system-roles.github.io/documentation/incremental_settings.html
|
[citest] |
|
I'm not aware of any best practices for injecting configuration snippets like this. I think it would be good to warn the users that there are no guarantees a working config will not stop working with a newer version of the role if the template is changed for instance. We don't want to make any assumption about what is injected in the config. |
|
[citest bad] |
|
[citest pending] |
|
[citest bad] |
|
[citest pending] |
|
I'll note that #80 did this for chrony settings and that PR includes a test to verify the custom settings - something similar should be done for the custom ntp settings |
|
[citest pending] |
|
[citest bad] |
|
[citest pending] |
|
please rebase to latest master branch |
|
[citest pending] |
1 similar comment
|
[citest pending] |
|
@jontow please rebase on top of latest master branch |
|
[citest pending] |
|
@jontow please rebase on top of latest master branch |
Found this small addition useful in my local installation, thought it might be helpful to others.