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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use chronyd instead of ntpdate #5705

Merged
merged 4 commits into from Dec 7, 2015

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented Dec 3, 2015

ntpdate is to be retired and chronyd is the new default time sync daemon on Centos 7 and has been running on the appliance along side our code running ntpdate. This opens up the possibility of trying to sync with both the timeservers configured in /etc/chrony.conf which have always been set to the defaults and /etc/ntp.conf which is what is edited when the user changes ntp servers.

Also the motivation behind using a custom script which ran ntpdate was to have the schedule worker protected from large jumps in time by dropping an exit file and waiting for it to exit.

The schedule worker has not responded to the exit file since commit 85d34a9a869ce2eefcf69d7c58644a24cc2359ba was merged in 2012.

@jrafanie @gtanzillo
馃敟 鉁傦笍 ??

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

@carbonin
Copy link
Member Author

carbonin commented Dec 3, 2015

Also requires ManageIQ/linux_admin#146

@carbonin carbonin changed the title Use chronyd instead of ntpdate [WIP] Use chronyd instead of ntpdate Dec 3, 2015
@carbonin
Copy link
Member Author

carbonin commented Dec 3, 2015

Marking as WIP until LinuxAdmin PR is merged
@miq-bot add_label wip

@miq-bot miq-bot added the wip label Dec 3, 2015
@jrafanie
Copy link
Member

jrafanie commented Dec 4, 2015

Looks good once the LinuxAdmin change is in. Nice work @carbonin

@jrafanie
Copy link
Member

jrafanie commented Dec 4, 2015

Look at all the 鉁傦笍 馃敟 (deletions)

@ntp_settings = ntp_settings
end

def apply_ntp_server_settings(settings)
return unless Sys::Platform::IMPL == :linux
Copy link
Member

Choose a reason for hiding this comment

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

do we want/need a log message that says we don't support this platform?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, I just tried it keep it as close to what was being done here.
This will probably only be an issue when running in development (not on an appliance) right?

Copy link
Member

Choose a reason for hiding this comment

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

馃憤 keep as is

@carbonin carbonin force-pushed the use_chrony_instead_of_ntpdate branch from e00a4dd to f63db23 Compare December 4, 2015 17:06
@carbonin carbonin changed the title [WIP] Use chronyd instead of ntpdate Use chronyd instead of ntpdate Dec 4, 2015
@carbonin
Copy link
Member Author

carbonin commented Dec 4, 2015

@miq-bot rm_label wip

@miq-bot miq-bot removed the wip label Dec 4, 2015
@kbrock
Copy link
Member

kbrock commented Dec 4, 2015

:shipit: LGTM

@gtanzillo
Copy link
Member

馃憤 LGTM

@carbonin carbonin force-pushed the use_chrony_instead_of_ntpdate branch from f63db23 to d4b1d47 Compare December 4, 2015 19:17
ntpdate is to be retired and chronyd is the new default
time sync daemon on Centos 7 and has been running on the
appliance along side our code running ntpdate. This opens
up the possibility of trying to sync with both the timeservers
configured in `/etc/chrony.conf` which have always been set to the
defaults and `/etc/ntp.conf` which is what is edited when the user
changes ntp servers.

Also the motivation behind using a custom script which ran ntpdate
was to have the schedule worker protected from large jumps in time
by dropping an exit file and waiting for it to exit.

The schedule worker has not responded to the exit file since
commit 85d34a9a869ce2eefcf69d7c58644a24cc2359ba was merged in 2012.

https://bugzilla.redhat.com/show_bug.cgi?id=1275677
@carbonin carbonin force-pushed the use_chrony_instead_of_ntpdate branch from d4b1d47 to ffcf265 Compare December 4, 2015 20:54
@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2015

<github_pr_commenter_batch />Some comments on commits carbonin/manageiq@3ba88b3~...ffcf265

@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2015

Checked commits carbonin/manageiq@3ba88b3~...ffcf265 with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
4 files checked, 1 offense detected

spec/models/miq_server/rhn_mirror_spec.rb

@jrafanie
Copy link
Member

jrafanie commented Dec 4, 2015

@miq-bot merge_when_green

^ I wish

gtanzillo added a commit that referenced this pull request Dec 7, 2015
@gtanzillo gtanzillo merged commit b736da4 into ManageIQ:master Dec 7, 2015
@gtanzillo gtanzillo added this to the Sprint 33 Ending Dec 7, 2015 milestone Dec 7, 2015
dclarizio pushed a commit to dclarizio/manageiq that referenced this pull request Dec 16, 2015
Use chrony instead of ntpdate

ntpdate is to be retired and chronyd is the new default
time sync daemon on Centos 7 and has been running on the
appliance along side our code running ntpdate. This opens
up the possibility of trying to sync with both the timeservers
configured in `/etc/chrony.conf` which have always been set to the
defaults and `/etc/ntp.conf` which is what is edited when the user
changes ntp servers.

Also the motivation behind using a custom script which ran ntpdate
was to have the schedule worker protected from large jumps in time
by dropping an exit file and waiting for it to exit.

The schedule worker has not responded to the exit file since
commit 85d34a9a was merged in 2012.

Upstream PR: ManageIQ#5705

Two empty commits in upstream not cherry-picked to this branch. The other cherry-picks were clean.

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

See merge request !609
@carbonin carbonin deleted the use_chrony_instead_of_ntpdate branch February 12, 2016 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants