Add cloud ntp addresses #8312

ghost · 2020-01-11T00:04:39Z

Hi

This is a in working progress PR which will add the cloud specific NTP server into the NTP config. This will prevent clock drift which could end up in resulting in downtime. AWS as an example will prevent you speaking with there APIs once your clock is out by five minutes.

It would be great to get some feedback on whether you feel this is the best approach I am taking and also if you would prefer more parameters in the function I created rather pre determining the values inside the function with switch statements.

I used switch statements to prevent having to have the same logic again and again when determining the ntp install type.

I would like in another PR to maybe offer custom NTPs that a user could pass into the cluster spec. This might be handy when you have custom NTP servers when hosting Kubernetes on prem.

@mikesplain hope you don't mind me tagging you into this and we already sort of discussed this on slack :)

Simon

k8s-ci-robot · 2020-01-11T00:04:48Z

Hi @simonmacklin. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

joshbranham · 2020-01-11T00:09:05Z

/ok-to-test

nodeup/pkg/model/ntp.go

k8s-ci-robot · 2020-01-12T20:22:09Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ghost · 2020-01-12T20:40:29Z

I have made some changes requested by @johngmyers. I have tested it with the default image and all seems to be working fine. I just would like to run some more tests and double check the code before this is considered to be merged. The regex strings are not perfect either so I need to work on those.

I am really short on time at the moment so I will probably not get a chance to sit and concentrate on this until Tuesday.

Thanks

Simon

johngmyers · 2020-01-13T00:00:57Z

/hold
per previous comment

rifelpet · 2020-01-13T00:05:23Z

@simonmacklin you may need to squash your commits and rebase in order to fix the CLA issue. It looks like this is the offending commit.

added replace method added cloud ips updated the func params removed whitespace at gce address removed sample ntp.conf removed whitespace from gce ntp address created const var ntp type added a period at the end of the func comment and used the const vars on the case statement. Will finish sometime this weekend unexported func and const type trying to fix git email config issue changed func param

ghost · 2020-01-13T10:43:45Z

Thanks @rifelpet I think I have fixed it. I formatted my mac and forgot to set my email in the git config. Sorry about that

Simon

johngmyers · 2020-01-15T19:08:50Z

This doesn't have support for an "in house" cloud provider, so that case isn't relevant.

ghost · 2020-01-15T19:22:01Z

@johngmyers

I stand corrected. I thought KOPS supported self hosted OpenStack and VSphere. So I assume they are only the cloud offerings.

Thanks

johngmyers · 2020-01-15T19:25:23Z

Kops does support those to some extent, but this added code only acts when the provider is AWS or GCE.

oded-dd · 2020-01-24T14:35:14Z

I am not sure you support NTP for CoreOs, as it defaults to timesyncd NTP service

The way we support it is by Kops hooks:

‘’’
amazon_time_sync_service_ip_address =
"169.254.169.123"

Type=oneshot
RemainAfterExit=no
ExecStart=/bin/sh -c "printf "[Time]\nNTP=${amazon_time_sync_service_ip_address}\n" > /etc/systemd/timesyncd.conf"
ExecStartPost=/bin/systemctl restart systemd-timesyncd.service
‘’’

Hope it can be supported as part of this PR

ReillyTevera · 2020-01-24T17:46:12Z

I've been of the opinion that it would be better to use timesyncd instead of chrony or ntpd on all of the supported platforms in fact.

johngmyers · 2020-03-02T05:40:24Z

timesyncd would indeed seem to be better. Still, this PR reasonably accomplishes what it is intending to do. A competing or revised PR would also be welcome.

ghost · 2020-03-02T13:46:56Z

I agree but this PR was only intended to add the missing cloud NTPs. I will be more then happy to create another PR to switch over to timesyncd another time.

justinsb · 2020-03-14T18:57:59Z

nodeup/pkg/model/ntp.go

+	case "aws":
+		ntpIP = "169.254.169.123"
+	case "gce":
+		ntpIP = "time.google.com"


Nit: I think this should probably be metadata.google.internal or metadata.google, based on https://cloud.google.com/compute/docs/instances/managing-instances#configure_ntp_for_your_instances

I think you are right. I got the information from this doc https://developers.google.com/time/faq but it looks like time.google.com is public for all people to use even outside of GCP.

I can't resolve metadata.google.internal from my laptop but I assume it will when running on GCP.

Shall I update?

justinsb · 2020-03-14T18:59:26Z

nodeup/pkg/model/ntp.go

+// updateNtpIP takes a ip and a ntpDaemon and will comment out
+// the default server or pool values and append the correct cloud
+// ip to the ntp config file.
+func updateNtpIP(ip string, daemon ntpDaemon) ([]byte, error) {


We may do better to write the full ntp configuration file, but this is the less intrusive change so let's run with it!

justinsb · 2020-03-14T19:01:05Z

Thanks @simonmacklin

/approve
/lgtm

k8s-ci-robot · 2020-03-14T19:01:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

johngmyers · 2020-03-14T20:29:50Z

/retest

rifelpet · 2020-03-15T00:18:59Z

It looks like this is causing issues on the kuberouter e2e job which uses Ubuntu 18.04.
nodeup is failing on all nodes:

Mar 14 23:15:07.576966 ip-172-20-54-66 nodeup[1634]: W0314 23:15:07.576951 1634 main.go:138] got error running nodeup (will retry in 30s): error building loader: open /etc/ntp.conf: no such file or directory

rifelpet · 2020-03-15T00:36:48Z

Some research shows that Ubuntu 18.04 uses chronyd rather than ntpd so we'll probably need to update the logic to be more specific than just debian family vs rhel family.

johngmyers · 2020-03-15T00:36:57Z

Looks like Ubuntu uses timesyncd instead.

ghost · 2020-03-15T09:01:39Z

When I wrote the code back in Jan the e2e tests all passed. Has Ubuntu 18.04 recently been added to the e2e tests?

hakman · 2020-03-15T09:05:57Z

@simonmacklin the e2e tests in PR are run only on Debian 9. There are more extensive e2e tests that run periodically on master:
https://testgrid.k8s.io/sig-cluster-lifecycle-kops#Summary

ghost · 2020-03-15T11:33:18Z

@hakman

Right that makes sense.

Do you think this could just be a race condition or maybe the wrong config path for ubuntu? I understand like @johngmyers said the default is timesyncd. But nodeup installs ntpd anyway https://github.com/kubernetes/kops/blob/master/nodeup/pkg/model/ntp.go#L71

So I am wondering if nodeup is trying to write the config before it installs the package.

I will have a closer look when I get home tonight

Simon

hakman · 2020-03-15T11:50:01Z

@simonmacklin I think the problem is the updateNtpIP function:

kops/nodeup/pkg/model/ntp.go

Lines 110 to 119 in 2b04d7d

    
           bytes, err := updateNtpIP(ntpIP, ntpd) 
        
           if err != nil { 
        
           	return err 
        
           } 
        
           c.AddTask(&nodetasks.File{ 
        
           	Path:     "/etc/ntp.conf", 
        
           	Contents: fi.NewBytesResource(bytes), 
        
           	Type:     nodetasks.FileType_File, 
        
           	Mode:     s("0644"), 
        
           })

This runs before all the tasks are created and run. The config file will not exist at all at that time if the package is not installed.

IMHO, the correct solution would be to write the full config files as we do everywhere else in nodeup and as @justinsb suggested: #8312 (comment).

ghost · 2020-03-15T12:08:16Z

Wouldn't line

kops/nodeup/pkg/model/ntp.go

Line 107 in 2b04d7d

c.AddTask(&nodetasks.Package{Name: "ntp"})

install ntp first before running the code under it?

hakman · 2020-03-15T12:12:16Z

Wouldn't line install ntp first before running the code under it?

AddTask just creates and queues a task to install the package. Ordering and the actual action is done later.

ghost · 2020-03-15T12:17:22Z

Right OK thanks @hakman

Fix NTP failures after #8312

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 11, 2020

k8s-ci-robot requested review from joshbranham and robinpercy January 11, 2020 00:05

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2020

joshbranham reviewed Jan 11, 2020

View reviewed changes

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

hakman reviewed Jan 11, 2020

View reviewed changes

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

johngmyers reviewed Jan 11, 2020

View reviewed changes

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

johngmyers reviewed Jan 11, 2020

View reviewed changes

nodeup/pkg/model/ntp.go Show resolved Hide resolved

johngmyers requested changes Jan 11, 2020

View reviewed changes

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

nodeup/pkg/model/ntp.go Outdated Show resolved Hide resolved

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 12, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 13, 2020

updated case to use the const

402359d

justinsb reviewed Mar 14, 2020

View reviewed changes

k8s-ci-robot assigned justinsb Mar 14, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2020

k8s-ci-robot merged commit e6803d0 into kubernetes:master Mar 14, 2020

k8s-ci-robot added this to the v1.18 milestone Mar 14, 2020

hakman pushed a commit to hakman/kops that referenced this pull request Mar 17, 2020

Fix NTP failures after kubernetes#8312

5003bd2

hakman mentioned this pull request Mar 17, 2020

Fix NTP failures after #8312 #8763

Merged

k8s-ci-robot added a commit that referenced this pull request Mar 18, 2020

Merge pull request #8763 from hakman/fix-ntp

8225736

Fix NTP failures after #8312

ari-becker mentioned this pull request May 10, 2020

Change AWS NTP source to Amazon Time Sync Service #4021

Closed

johngmyers mentioned this pull request May 27, 2020

Use systemd-timesyncd for Ubuntu 20.04 #9182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cloud ntp addresses #8312

Add cloud ntp addresses #8312

ghost commented Jan 11, 2020

k8s-ci-robot commented Jan 11, 2020

joshbranham commented Jan 11, 2020

k8s-ci-robot commented Jan 12, 2020

ghost commented Jan 12, 2020

johngmyers commented Jan 13, 2020

rifelpet commented Jan 13, 2020

ghost commented Jan 13, 2020

johngmyers commented Jan 15, 2020

ghost commented Jan 15, 2020

johngmyers commented Jan 15, 2020 •

edited

Loading

oded-dd commented Jan 24, 2020

ReillyTevera commented Jan 24, 2020

johngmyers commented Mar 2, 2020

ghost commented Mar 2, 2020

justinsb Mar 14, 2020

ghost Mar 14, 2020 •

edited by ghost

Loading

justinsb Mar 14, 2020

justinsb commented Mar 14, 2020

k8s-ci-robot commented Mar 14, 2020

johngmyers commented Mar 14, 2020

rifelpet commented Mar 15, 2020

rifelpet commented Mar 15, 2020

johngmyers commented Mar 15, 2020

ghost commented Mar 15, 2020

hakman commented Mar 15, 2020

ghost commented Mar 15, 2020

hakman commented Mar 15, 2020

ghost commented Mar 15, 2020

hakman commented Mar 15, 2020 •

edited

Loading

ghost commented Mar 15, 2020

Add cloud ntp addresses #8312

Add cloud ntp addresses #8312

Conversation

ghost commented Jan 11, 2020

k8s-ci-robot commented Jan 11, 2020

joshbranham commented Jan 11, 2020

k8s-ci-robot commented Jan 12, 2020

ghost commented Jan 12, 2020

johngmyers commented Jan 13, 2020

rifelpet commented Jan 13, 2020

ghost commented Jan 13, 2020

johngmyers commented Jan 15, 2020

ghost commented Jan 15, 2020

johngmyers commented Jan 15, 2020 • edited Loading

oded-dd commented Jan 24, 2020

ReillyTevera commented Jan 24, 2020

johngmyers commented Mar 2, 2020

ghost commented Mar 2, 2020

justinsb Mar 14, 2020

Choose a reason for hiding this comment

ghost Mar 14, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

justinsb Mar 14, 2020

Choose a reason for hiding this comment

justinsb commented Mar 14, 2020

k8s-ci-robot commented Mar 14, 2020

johngmyers commented Mar 14, 2020

rifelpet commented Mar 15, 2020

rifelpet commented Mar 15, 2020

johngmyers commented Mar 15, 2020

ghost commented Mar 15, 2020

hakman commented Mar 15, 2020

ghost commented Mar 15, 2020

hakman commented Mar 15, 2020

ghost commented Mar 15, 2020

hakman commented Mar 15, 2020 • edited Loading

ghost commented Mar 15, 2020

johngmyers commented Jan 15, 2020 •

edited

Loading

ghost Mar 14, 2020 •

edited by ghost

Loading

hakman commented Mar 15, 2020 •

edited

Loading