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.

Sign up for GitHub

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

Jump to bottom

Use systemd-timesyncd for Ubuntu 20.04 #9182

Merged
merged 1 commit into from
May 27, 2020
Merged

Use systemd-timesyncd for Ubuntu 20.04 #9182

merged 1 commit into from
May 27, 2020

Conversation

hakman
Copy link
Member

@hakman hakman commented May 26, 2020 •
edited
Loading

systemd-timesyncd is the default service for clock synchronisation in Ubuntu 20.04. Installing ntp seems to make kops-configuration crash as reported in #9180.

We already use systemd-timesyncd for Ubuntu in 1.18 and this issue is not noticeable. Using it in 1.17 and 1.16 instead of ntp seems the best way to go.

kops/nodeup/pkg/model/ntp.go

Lines 60 to 64 in 8225736

if b.Distribution.IsUbuntu() {
if ntpHost != "" {
c.AddTask(b.buildTimesyncdConf("/etc/systemd/timesyncd.conf", ntpHost))
}
c.AddTask((&nodetasks.Service{Name: "systemd-timesyncd"}).InitDefaults())

@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone May 26, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 26, 2020
@hakman
Copy link
Member Author

hakman commented May 26, 2020

/cc @rifelpet

@k8s-ci-robot k8s-ci-robot requested a review from rifelpet May 26, 2020 19:42
johngmyers
johngmyers reviewed May 26, 2020
View reviewed changes
nodeup/pkg/model/ntp.go
@@ -46,7 +46,10 @@ func (b *NTPBuilder) Build(c *fi.ModelBuilderContext) error {
return nil
}

if b.Distribution.IsDebianFamily() {
if b.Distribution == distros.DistributionFocal {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if b.Distribution == distros.DistributionFocal {
if b.Distribution.IsUbuntu() && b.Distribution != distros.DistributionXenial && b.Distribution != distros.DistributionBionic {

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be ok to leave it as is? Seems simpler.

Copy link
Member

Choose a reason for hiding this comment

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

The issue would be whether a new major version of Ubuntu is used with kops 1.17. But I suppose code would have to be added to 1.17 to support it and that's not likely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. I don't think there is any plan to support any other distro in the near future. Thanks!

@johngmyers
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2020
@justinsb
Copy link
Member

Sounds good to me; I'm also wondering whether we should install this in more distros, but thinking about e.g. debian buster.

In any case, this is a good change - thanks @hakman, and @johngmyers for reviewing!

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, justinsb

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2020
@k8s-ci-robot k8s-ci-robot merged commit 4dd5917 into kubernetes:release-1.17 May 27, 2020
@hakman hakman mentioned this pull request May 27, 2020
Merged
@johngmyers
Copy link
Member

This probably undid the effect of #8312 for Focal. We should see about configuring systemd-timesyncd with any cloud provider supplied time source.

@hakman
Copy link
Member Author

hakman commented May 27, 2020

@johngmyers what do you mean? I think that is only in 1.18+.

@hakman hakman deleted the focal-timesyncd branch May 27, 2020 16:08
@johngmyers
Copy link
Member

I mean the corresponding change in 1.18.

@hakman
Copy link
Member Author

hakman commented May 27, 2020

I think my change in 1.18 kept the spirit and also fixed what was broken in that PR:
5003bd2#diff-ae9bef714f1963c836e9427f8dfea1f2
9ae9466#diff-ae9bef714f1963c836e9427f8dfea1f2

k8s-ci-robot added a commit that referenced this pull request May 28, 2020
@k8s-ci-robot
Merge pull request #9186 from hakman/automated-cherry-pick-of-#9182-u…
bed56d9 
…pstream-release-1.16

Automated cherry pick of #9182: Use systemd-timesyncd for Ubuntu 20.04
@paalkr paalkr mentioned this pull request May 28, 2020
Closed
@jsleeio jsleeio mentioned this pull request Jun 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers left review comments

@justinsb justinsb Awaiting requested review from justinsb

@mikesplain mikesplain Awaiting requested review from mikesplain

@rifelpet rifelpet Awaiting requested review from rifelpet

Assignees

@justinsb justinsb

@johngmyers johngmyers

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.
4 participants
@hakman @johngmyers @justinsb @k8s-ci-robot