Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

[WIP] Optionally allow users to set NTP servers for the cluster#2213

Closed
lblackstone wants to merge 1 commit intocoreos:masterfrom
lblackstone:configurable-ntp
Closed

[WIP] Optionally allow users to set NTP servers for the cluster#2213
lblackstone wants to merge 1 commit intocoreos:masterfrom
lblackstone:configurable-ntp

Conversation

@lblackstone
Copy link
Contributor

NTP servers on Container Linux default to CoreOS time servers,
which may not be accessible on private cluster deployments.
Allow users to set a list of primary NTP servers with the CoreOS
servers as the default fallback.

Fixes #1866

@coreosbot
Copy link

Can one of the admins verify this patch?

1 similar comment
@coreosbot
Copy link

Can one of the admins verify this patch?

@lblackstone
Copy link
Contributor Author

@s-urbaniak @alexsomesan I've implemented this for OpenStack so far. If this approach looks good, I can add the other platforms as well.

@mxinden
Copy link
Contributor

mxinden commented Nov 1, 2017

We did some changes (#2082) to the testing process. Please rebase on to current master, so that the basic-tests PR status is reported correctly.

@lblackstone lblackstone force-pushed the configurable-ntp branch 2 times, most recently from 65e93f1 to 3056438 Compare November 13, 2017 17:24
@lblackstone
Copy link
Contributor Author

@coreos/team-installer Could somebody take a look at this WIP and let me know if this approach is ok?

@s-urbaniak
Copy link
Contributor

@lblackstone thanks a lot for the contribution!

/cc @robszumski for the product side of things
/cc @crawford @lucab if this is the right approach from the perspective of CL

@lucab
Copy link
Contributor

lucab commented Nov 13, 2017

I'd suggest to avoid installing the config file if the list is empty, as this just hardcodes the same default values but in a way that can't be changed in the future.

}

data "ignition_file" "ntp_dropin" {
path = "/etc/systemd/timesyncd.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably go in its own dropin (i.e. /etc/systemd/timesyncd.conf.d/10-tectonic-installer.conf) and be skipped if the terraform configuration value is not set by the user (to avoid bloating userdata and hardcoding defaults in config).

@lblackstone
Copy link
Contributor Author

Addressed @lucab comments. Drop-in file is now only installed if the user specifies NTP servers.

@crawford
Copy link
Contributor

I thought the installer was moving away from including this sort of configuration in favor of it moving into a cluster config that gets applied at runtime instead.

NTP servers on Container Linux default to CoreOS time servers,
which may not be accessible on private cluster deployments.
Allow users to set a list of primary NTP servers with the CoreOS
servers as the default fallback.
@lblackstone
Copy link
Contributor Author

@crawford Do you have any further information on the direction you mentioned? Happy to revise as needed, but I hadn't seen any public-facing docs to that effect.

/cc @s-urbaniak

@crawford
Copy link
Contributor

@lblackstone Unfortunately, I don't have any more info at the moment. This repo will be undergoing a pretty massive refactor over the next couple months (all driven by CoreOS-internal needs). Hopefully by the end of this work, this repo will feel a bit more like an open source project.

@lblackstone
Copy link
Contributor Author

lblackstone commented Jan 26, 2018

#2747 finished out this work for the track-1 branch. Closing this PR, as that feature can be cherry-picked later if desired.

@lblackstone lblackstone deleted the configurable-ntp branch January 26, 2018 15:57
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.

6 participants