-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fix NTP failures after #8312 #8763
Conversation
nodeup/pkg/model/ntp.go
Outdated
Type: nodetasks.FileType_File, | ||
Mode: s("0644"), | ||
}) | ||
c.AddTask(&nodetasks.Package{Name: "chrony"}) |
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.
This will affect all debian and ubuntu distros, not just the most recent versions, correct? Are we sure the chrony package is available in older apt/yum repos?
I see it in ubuntu, debian, and centos so maybe we're confident enough that the package will be available on any supported distros. Thoughts?
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.
Chrony is available on all supported distros. It is installed by default on RHEL, CentOS and Debian. Ubuntu uses systemd-timesyncd
by default though. To avoid having both running I updated the PR to configure it instead of Chrony for Ubuntu.
I tested this with the following distros and works:
- Amazon Linux 2
- CentOS Linux 7 (Core)
- Red Hat Enterprise Linux Server 7.4 (Maipo)
- Red Hat Enterprise Linux 8.1 (Ootpa)
- Debian GNU/Linux 9 (stretch)
- Debian GNU/Linux 10 (buster)
- Ubuntu 16.04.6 LTS
- Ubuntu 18.04.4 LTS
- Ubuntu Focal Fossa (development branch)
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.
We can also keep Chrony for Ubuntu, but timesyncd should be stopped and disabled.
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.
Ok, I think that is sufficient coverage 👍🏻
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman, rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks @rifelpet :) |
Is there anyway to get this merged into 1.17? We are having a hard time keeping ec2 instances in sync using ntpd. |
@nhudson this a fairly big change to be added to an already released version, but I may add a |
@hakman I'm currently working around it using a hook to install and configure chrony, remove ntp and disable timesycd. Didn't know if it was an easy backport to 1.17. Thanks for the response! |
The backport is easy, but with 1.17 being released it is not a good idea to change things. |
#8312 broke support for some our supported distros as seen in periodic e2e tests.
This should fix the support by using Chrony (already default in most of the distros).