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

feat: add manage option leapsectz #222

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

brakkio86
Copy link

Enhancement: support leapsectz option

Reason: this option is mandatory in order to manage leap second situation

Result: new option available

Issue Tracker Tickets (Jira or BZ if any):

@mlichvar
Copy link
Collaborator

mlichvar commented Nov 2, 2023

Clients normally accept leap second information from their servers. The leapsectz option is mainly useful to set the TAI-UTC offset of the system clock. A potential issue is incompatibility with leap-smearing servers.

templates/chrony.conf.j2 Outdated Show resolved Hide resolved
templates/chrony.conf.j2 Outdated Show resolved Hide resolved
@richm richm changed the title add manage option leapsectz feat: add manage option leapsectz Nov 6, 2023
defaults/main.yml Outdated Show resolved Hide resolved
@richm
Copy link
Collaborator

richm commented Nov 6, 2023

Clients normally accept leap second information from their servers. The leapsectz option is mainly useful to set the TAI-UTC offset of the system clock. A potential issue is incompatibility with leap-smearing servers.

If we document this in the README, is it ok to allow this feature to be supported?

@richm
Copy link
Collaborator

richm commented Nov 13, 2023

Clients normally accept leap second information from their servers. The leapsectz option is mainly useful to set the TAI-UTC offset of the system clock. A potential issue is incompatibility with leap-smearing servers.

If we document this in the README, is it ok to allow this feature to be supported?

@mlichvar ^^^

@mlichvar
Copy link
Collaborator

A warning about imcompatibility with leapsmearing servers would be nice.

However, I don't like the chrony-specific variable name. Would something like local_leap_source as a boolean work better? This would open up the possibility for support with ntpd, which has the leapfile file directive and some systems have the leap file included with their tzdata at known location.

The support for leapsectz directive was added long time ago. Is there other reason for having the >= 4.0 conditional?

@richm
Copy link
Collaborator

richm commented Nov 29, 2023

ping - any update?

@richm
Copy link
Collaborator

richm commented Dec 12, 2023

ping - any update? Can we close this?

@brakkio86
Copy link
Author

Hello,

The support for leapsectz directive was added long time ago. Is there other reason for having the >= 4.0 conditional?

My fault, I didn't find when whas introduced and then I put this check. Now I verify and removed the conditional.

@brakkio86
Copy link
Author

A warning about imcompatibility with leapsmearing servers would be nice.

I've added the documenation and warning section.

brakkio86 and others added 6 commits December 15, 2023 10:34
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
Co-authored-by: Richard Megginson <richm@stanfordalumni.org>
@richm
Copy link
Collaborator

richm commented Dec 15, 2023

Do we need this PR at all? Can you use timesync_chrony_custom_settings https://github.com/linux-system-roles/timesync#role-variables:

timesync_chrony_custom_settings:
  - "leapsectz right/UTC"

?

@brakkio86
Copy link
Author

Do we need this PR at all? Can you use timesync_chrony_custom_settings https://github.com/linux-system-roles/timesync#role-variables:

timesync_chrony_custom_settings:
  - "leapsectz right/UTC"

?

Yes, it could be use. In my philosofy, I prefer explicit setting.

@richm
Copy link
Collaborator

richm commented Jan 7, 2024

Do we need this PR at all? Can you use timesync_chrony_custom_settings https://github.com/linux-system-roles/timesync#role-variables:

timesync_chrony_custom_settings:
  - "leapsectz right/UTC"

?

Yes, it could be use. In my philosofy, I prefer explicit setting.

Either way is fine with my. One problem with having an explicit setting is that there is a chance of a duplicate entry. For example, if the user specifies

timesync_chrony_leapsectz: "right/UTC"
...
timesync_chrony_custom_settings:
  - "leapsectz left/UTC"

then I think the config file will have both settings. Yes, this is an unlikely scenario, but if you have a complex inventory with multiple hosts and host groups https://docs.ansible.com/ansible/latest/inventory_guide/index.html then you may have a "general" setting in one file with

timesync_chrony_custom_settings:
  - "leapsectz left/UTC"

and a host/region specific setting timesync_chrony_leapsectz: "right/UTC" and will have to figure out how the rules of precedence merge these values for a given host.

Of course we will document in the README something like "Do not use both timesync_chrony_leapsectz and have leapsectz in timesync_chrony_custom_settings".

Do we need to add code to the role to check if timesync_chrony_custom_settings contains leapsectz if timesync_chrony_leapsectz is set?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants