fix: use ntp instead of systemd-timesyncd in 18.04 #2815
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2815 +/- ##
=======================================
Coverage 72.49% 72.49%
=======================================
Files 141 141
Lines 25847 25847
=======================================
Hits 18739 18739
Misses 6021 6021
Partials 1087 1087 |
| if [[ $UBUNTU_VERSION == "18.04" ]]; then | ||
| NTP="ntp" | ||
| systemctl_stop 20 5 10 systemd-timesyncd || exit $ERR_SYSTEMCTL_STOP_FAIL | ||
| retrycmd_if_failure 120 5 25 systemctl disable systemd-timesyncd || exit $ERR_SYSTEMCTL_STOP_FAIL |
There was a problem hiding this comment.
can we put disabling timesyncd in a separate function since it's not actually part of installing any dependencies?
There was a problem hiding this comment.
I would prefer not to install the ntp package on an OS runtime w/ systemd-timesyncd running and enabled, but maybe that's paranoia?
There was a problem hiding this comment.
we can at least extract it into a func and call it from here, just want to make sure installDeps doesn't become an long list of unrelated commands
jackfrancis
force-pushed
the
1804-ntp
branch
3 times, most recently
from
a3a5921 to
009f1e7
Compare
| @@ -35,6 +35,11 @@ removeContainerd() { | |||
| fi | |||
| } | |||
|
|
|||
| disableTimeSyncd() { | |||
| systemctl_stop 20 5 10 systemd-timesyncd || exit $1 | |||
There was a problem hiding this comment.
why not just
| systemctl_stop 20 5 10 systemd-timesyncd || exit $1 | |
| systemctl_stop 20 5 10 systemd-timesyncd || exit $ERR_SYSTEMCTL_STOP_FAIL |
to stay consistent with the rest of the file? We're not passing in exit codes as args in other CSE functions
There was a problem hiding this comment.
Good question! I'm just sort of desparate to save cloud-init bytes here is the reason.
There was a problem hiding this comment.
in that case maybe we can change systemctl_stop to take more than one arg and stop both in one call
There was a problem hiding this comment.
I think that's a good suggestion for a follow-up PR, how about my latest change?
There was a problem hiding this comment.
I think it's okay but I really don't like that we have to lower code quality/readability to save space :(
| @@ -35,6 +35,11 @@ removeContainerd() { | |||
| fi | |||
| } | |||
|
|
|||
| disableTimeSyncd() { | |||
| systemctl_stop 20 5 10 systemd-timesyncd || exit $1 | |||
| retrycmd_if_failure 120 5 25 systemctl disable systemd-timesyncd || exit $1 | |||
There was a problem hiding this comment.
| retrycmd_if_failure 120 5 25 systemctl disable systemd-timesyncd || exit $1 | |
| retrycmd_if_failure 120 5 25 systemctl disable systemd-timesyncd || exit $ERR_SYSTEMCTL_STOP_FAIL |
| @@ -68,6 +73,10 @@ installDeps() { | |||
| retrycmd_if_failure 60 5 10 dpkg -i /tmp/packages-microsoft-prod.deb || exit $ERR_MS_PROD_DEB_PKG_ADD_FAIL | |||
| aptmarkWALinuxAgent hold | |||
| packages+=" cgroup-lite ceph-common glusterfs-client" | |||
| if [[ $UBUNTU_RELEASE == "18.04" ]]; then | |||
| disableTimeSyncd $ERR_SYSTEMCTL_STOP_FAIL | |||
There was a problem hiding this comment.
| disableTimeSyncd $ERR_SYSTEMCTL_STOP_FAIL | |
| disableTimeSyncd |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, jackfrancis 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 |
Reason for Change:
This PR changes the 18.04-LTS bootstrap implementation (both VHD and non-VHD) to use the
ntppackage instead of the built-insystemd-timesyncd. This is due to a hypothesis that this time synchronization configuration is more reliable.Issue Fixed:
Fixes #1306
Requirements:
Notes: