Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

rootfs: configure chronyc service with makestep #318

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

xs3c
Copy link

@xs3c xs3c commented Jun 20, 2019

The current chrony service does not step the system clock,
so add the modification to do this if the adjustment is
larger than one second

Fixes: #316

Signed-off-by: Yang, Wei wei.yang1@linux.alibaba.com

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @xs3c.

lgtm

/cc @amshinde for her thoughts. @grahamwhaley may have a view on this too from the metrics perspective.

@@ -396,6 +396,9 @@ fi

info "Configure chrony file ${chrony_conf_file}"
echo "refclock PHC /dev/ptp0 poll 3 dpoll -2 offset 0" >> ${chrony_conf_file}
# Step the system clock instead of slewing it if the adjustment is larger than
# one second, at any time
echo "makestep 1 -1" >> ${chrony_conf_file}
Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: this is fine, but now there are two changes to this file, you could instead do:

cat >>"${chrony_conf_file}"<<EOT
refclock PHC /dev/ptp0 poll 3 dpoll -2 offset 0

# Step the system clock instead of slewing it if the adjustment is larger than
# one second, at any time
echo "makestep 1 -1
EOT

Copy link
Author

Choose a reason for hiding this comment

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

Done

@jodh-intel
Copy link
Contributor

/test

@grahamwhaley
Copy link
Contributor

I think the idea is fine/right. I'd be interested to know:

  • will this time sync be guaranteed to happen before the container workload gets launched? That is, it is synchronous or async with the workload. Async could cause some interesting issues maybe for workloads in the future if their time suddenly jumps for instance.
  • have we witnessed the time being out between the host and the guest, and by how much?

/cc @mcastelino who I think first toyed with syncing the clocks

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
The current chrony service does not step the system clock,
so add the modification to do this if the adjustment is
larger than one second

Fixes: kata-containers#316

Signed-off-by: Yang, Wei <wei.yang1@linux.alibaba.com>
@xs3c xs3c force-pushed the chronyc-with-makestep branch from af2532d to add0d44 Compare June 20, 2019 10:59
@jodh-intel
Copy link
Contributor

Thanks again @xs3c! 😄

@xs3c
Copy link
Author

xs3c commented Jun 20, 2019

@jodh-intel you are welcome 😄

@jcvenegas jcvenegas requested a review from amshinde June 20, 2019 20:07
@amshinde
Copy link
Member

amshinde commented Jun 20, 2019

lgtm, as it is generally a good practice to makestep esp if the host goes to sleep.
@xs3c During testing this a while back, we had not seen a large difference in the the host and the guest time, just when the guest had booted up.
Can you provide some values on what improvements you are seeing with this approach at bootup.

@grahamwhaley I have also being toying with the idea of adding a systemd ordering for the agent and chrony, to make sure we start the agent after chrony has fully started up(perhaps allowing it to sync the time before any workload is started by the agent).
I believe we can achieve this with adding an After=chronyd.service in the agent service file, allowing the agent to start after chrony. I think with After=, the agent would still start, even if chrony failed to start, need to confirm this though.

@jcvenegas
Copy link
Member

/test

@jodh-intel
Copy link
Contributor

Restarted ARM CI now that #265 has landed.

@amshinde
Copy link
Member

@Pennyzct ARM CI still seems to be failing. Can you take a look?
Did #265 solve the chrony startup issue for you on ARM? Can you confirm? A quick glance at the failures dont show errors related to that.

@Pennyzct
Copy link
Contributor

Hi~ @amshinde
osbuilder/ repo and runtime/ repo have different test cases, the error

Makefile:55: recipe for target '/tmp/osbuilder-test.4zzNcbN/rootfs-osbuilder/.clearlinux_rootfs.done' failed
make: *** [/tmp/osbuilder-test.4zzNcbN/rootfs-osbuilder/.clearlinux_rootfs.done] Error 1

should be fixed by #320. I will restart to see how it works now. ;)
and #265 did fix ARM CI failure on runtime/ repo. you may see one proof in one new PR runtime/#1823 in runtime/ repo, ARM CI got green. ;)

@Pennyzct
Copy link
Contributor

we have all green here~~~~🎉🎊

@jodh-intel jodh-intel merged commit 0c48630 into kata-containers:master Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it better to configure chronyc service with makestep options
6 participants