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

20platform-chrony: Add support for AWS and GCP #393

Merged
merged 3 commits into from May 14, 2020

Conversation

cgwalters
Copy link
Member

overlay.d: Rename 20azure-chrony → 20platform-chrony

In prep for adding AWS.


20platform-chrony: Add support for AWS

In AWS there's an NTP source accessible via the link-local IP,
which means it works in restricted networks (i.e. no Internet) too.

Should fix https://bugzilla.redhat.com/show_bug.cgi?id=1834252

In AWS there's an NTP source accessible via the link-local IP,
which means it works in restricted networks (i.e. no Internet) too.

Should fix https://bugzilla.redhat.com/show_bug.cgi?id=1834252
@cgwalters
Copy link
Member Author

Tested this in AWS, but not yet re-tested in Azure.

@cgwalters
Copy link
Member Author

OK now that I look, GCP has similar recommendations too:
https://cloud.google.com/compute/docs/instances/managing-instances#configure-ntp

I'll do that next.

@cgwalters
Copy link
Member Author

(Should this be part of https://github.com/coreos/afterburn/ ? )

@cgwalters
Copy link
Member Author

OK, tested on Azure, AWS, and GCP!

@miabbott
Copy link
Member

This looks pretty promising for solving clock issues in the cloud! 👍

Regarding the RHCOS downstream, should we backport this to 4.3/4.4?

@@ -0,0 +1,3 @@
{
"platforms": "aws,azure,gcp"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note of course kola isn't running on any of these by default; I tested the test manually by just copy-pasting the script to an instance and running it directly. (This is an advantage of ext tests)

@cgwalters cgwalters changed the title 20platform-chrony: Add support for AWS 20platform-chrony: Add support for AWS and GCP May 13, 2020
@lucab
Copy link
Contributor

lucab commented May 13, 2020

@cgwalters yes, 3 is a clear pattern, I think it could fit into Afterburn. It would be a normal unit in initramfs, scheduled before ignition-files.

The string-replacing logic is quite brittle though, it would be great if upstream chrony landed logic for config fragment soon-ish (I know we have a BZ somewhere).

@cgwalters
Copy link
Member Author

Though, this configuration isn't actually dynamic; we just happen to do it on boot because of the "single update stream" issue. So that's an argument against afterburn doing it.

Copy link
Contributor

@rfairley rfairley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment - otherwise LGTM!

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, but LGTM as is too!

Like AWS, GCP also runs an NTP server on the metadata IP.
@cgwalters
Copy link
Member Author

Fixed the comment comments!

@miabbott
Copy link
Member

@cgwalters yes, 3 is a clear pattern, I think it could fit into Afterburn. It would be a normal unit in initramfs, scheduled before ignition-files.

The string-replacing logic is quite brittle though, it would be great if upstream chrony landed logic for config fragment soon-ish (I know we have a BZ somewhere).

https://bugzilla.redhat.com/show_bug.cgi?id=1828434

@jlebon jlebon merged commit 7136e14 into coreos:testing-devel May 14, 2020
@cgwalters
Copy link
Member Author

In looking further into this I found
https://opensource.com/article/17/6/timekeeping-linux-vms
which basically says to modprobe ptp_kvm and then configure chrony in exactly the same way we're doing here which covers KVM (and presumably OpenStack for example).

It certainly seems to work for me; though loading a kernel module is probably arguing for refactoring this into cleaner "not bash" code.

rfairley pushed a commit to rfairley/fedora-coreos-config that referenced this pull request May 19, 2020
Add a new overlay 21dhcp-chrony which contains the NetworkManager
dispatcher script 11-coreos-dhcp-chrony. On "up" and "dhcp4-change",
the dispatcher writes out a snippet to `/var/lib/coreos-dhcp-chrony`
containing the NTP server name and default chrony configuration
`iburst makestep 1 -1` for all NTP servers specified through DHCP.
Then, `coreos-chrony-helper` is called to track which servers have
been added/removed, and use `chronyc` to set the parameters for
each new NTP server.

If an existing configuration for an NTP server exists statically in
`/etc/chrony.conf`, then no new snippet containing the default DHCP
parameters is written, and the original static chrony configuration
is maintained. This should avoid conflicting with any custom
configuration or platform-specific configuration such as
coreos#393.

11-coreos-dhcp-chrony closely follows the pattern that 11-dhclient
[1] from the dhcp package used. The logic for updating the
snippets and for running the necessary `chrony-helper` commands is
copied from chrony.sh [2] and chrony-helper [3] in the chrony
package, with the only modification being changing the path in
the `dhcp_servers_files` (originally `dhclient_servers_files`)
variable from `/var/lib/dhclient` to `/var/lib/coreos-dhcp-chrony`.
Copying code from `chrony-helper` is not ideal, and is meant as a
temporary measure. Overall, we need to handle the NTP server config
snippets in the same way, and will be able to switch to use the
official `chrony-helper` script once a fix upstream making the
`dhcp_servers_files` path general/configurable lands.

There were no changes between the Fedora and RHEL versions of [1], [2],
[3] - so there should be no differences to account for there at this
time.

Short-term fix for https://bugzilla.redhat.com/show_bug.cgi?id=1800901

[1] https://src.fedoraproject.org/rpms/dhcp/blob/master/f/11-dhclient
[2] https://src.fedoraproject.org/rpms/chrony/blob/master/f/chrony.dhclient
[3] https://src.fedoraproject.org/rpms/chrony/blob/master/f/chrony.helper
rfairley pushed a commit to rfairley/fedora-coreos-config that referenced this pull request May 19, 2020
Add a new overlay 21dhcp-chrony which contains the NetworkManager
dispatcher script 11-coreos-dhcp-chrony. On "up" and "dhcp4-change",
the dispatcher writes out a snippet to `/var/lib/coreos-dhcp-chrony`
containing the NTP server name and default chrony configuration
`iburst makestep 1 -1` for all NTP servers specified through DHCP.
Then, `coreos-chrony-helper` is called to track which servers have
been added/removed, and use `chronyc` to set the parameters for
each new NTP server.

If an existing configuration for an NTP server exists statically in
`/etc/chrony.conf`, then no new snippet containing the default DHCP
parameters is written, and the original static chrony configuration
is maintained. This should avoid conflicting with any custom
configuration or platform-specific configuration such as
coreos#393.

11-coreos-dhcp-chrony closely follows the pattern that 11-dhclient
[1] from the dhcp package used. The logic for updating the
snippets and for running the necessary `chrony-helper` commands is
copied from chrony.sh [2] and chrony-helper [3] in the chrony
package, with the only modification being changing the path in
the `dhcp_servers_files` (originally `dhclient_servers_files`)
variable from `/var/lib/dhclient` to `/var/lib/coreos-dhcp-chrony`.
Copying code from `chrony-helper` is not ideal, and is meant as a
temporary measure. Overall, we need to handle the NTP server config
snippets in the same way, and will be able to switch to use the
official `chrony-helper` script once a fix upstream making the
`dhcp_servers_files` path general/configurable lands.

There were no changes between the Fedora and RHEL versions of [1], [2],
[3] - so there should be no differences to account for there at this
time.

Short-term fix for https://bugzilla.redhat.com/show_bug.cgi?id=1800901

[1] https://src.fedoraproject.org/rpms/dhcp/blob/master/f/11-dhclient
[2] https://src.fedoraproject.org/rpms/chrony/blob/master/f/chrony.dhclient
[3] https://src.fedoraproject.org/rpms/chrony/blob/master/f/chrony.helper
rfairley pushed a commit to rfairley/fedora-coreos-config that referenced this pull request May 20, 2020
Add a new overlay 21dhcp-chrony which contains the NetworkManager
dispatcher script 11-coreos-dhcp-chrony. On "up" and "dhcp4-change",
the dispatcher writes out a snippet to `/var/lib/coreos-dhcp-chrony`
containing the NTP server name and default chrony configuration
`iburst makestep 1 -1` for all NTP servers specified through DHCP.
Then, `coreos-chrony-helper` is called to track which servers have
been added/removed, and use `chronyc` to set the parameters for
each new NTP server.

If an existing configuration for an NTP server exists statically in
`/etc/chrony.conf`, then no new snippet containing the default DHCP
parameters is written, and the original static chrony configuration
is maintained. This should avoid conflicting with any custom
configuration or platform-specific configuration such as
coreos#393.

11-coreos-dhcp-chrony closely follows the pattern that 11-dhclient
[1] from the dhcp package used. The logic for updating the
snippets and for running the necessary `chrony-helper` commands is
copied from chrony.sh [2] and chrony-helper [3] in the chrony
package, with the only modification being changing the path in
the `dhcp_servers_files` (originally `dhclient_servers_files`)
variable from `/var/lib/dhclient` to `/var/lib/coreos-dhcp-chrony`.
Copying code from `chrony-helper` is not ideal, and is meant as a
temporary measure. Overall, we need to handle the NTP server config
snippets in the same way, and will be able to switch to use the
official `chrony-helper` script once a fix upstream making the
`dhcp_servers_files` path general/configurable lands.

There were no changes between the Fedora and RHEL versions of [1], [2],
[3] - so there should be no differences to account for there at this
time.

Short-term fix for https://bugzilla.redhat.com/show_bug.cgi?id=1800901

[1] https://src.fedoraproject.org/rpms/dhcp/blob/master/f/11-dhclient
[2] https://src.fedoraproject.org/rpms/chrony/blob/master/f/chrony.dhclient
[3] https://src.fedoraproject.org/rpms/chrony/blob/master/f/chrony.helper
c4rt0 pushed a commit to c4rt0/fedora-coreos-config that referenced this pull request Mar 27, 2023
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

5 participants