search

saltstack-formulas / ntp-formula

http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
23 stars 158 forks source link

fix for #28: move config file watch, remove service check from ntp state #30

Closed sroegner closed 5 years ago

gravyboat commented 8 years ago

@sroegner Did you review https://github.com/saltstack-formulas/ntp-formula/pull/29?

gravyboat commented 8 years ago

I'm fine with this one, but would like it if you would weigh in on the change regarding the ntp package name. In reality we should probably change this so the init is assumed to be for the general NTP service, and the server.sls supports the server entirely by itself. This would mean everyone is happy regarding concerns over services starting as I do like the original change.

markwherry9 commented 8 years ago

Above it was suggested to remove the watch from init.sls and rely on the one in server.sls. However, an ntp client won't then have a watch on its configuration file and the formula therefore will fail to start ntpd using the servers in the configuration file for clients.. Unless I'm mistaken server.sls doesn't apply to clients, so the watch should be init.sls where I placed it. I'm quite new to SaltStack so might be wrong about this. Leave it with you guys to figure out.

sroegner commented 8 years ago

@markwherry9 i think you are probably right about this PR not being a good solution either. The problem I am having is not a salt problem at all - I fail to understand how restarting ntpd is different from restarting ntpd - this is what you would get if calling server.sls - it includes ntp (which installs the package, puts what appears to be called the ntp client config in place and watches ntpd), after which the server state maintains what is referred to as the ntp server config and - again - restarts ntpd.

I don't know anything about ntpd - is the daemon run as client or a server? Also: in the manpages I looked at i could never find any mention of /etc/ntpd.conf and it is also not in the packages i looked at. IMO the problem we're having here is for lack of specification - the implementation is rather trivial.

@gravyboat I looked at your PR and it didn't fix the problem - sorry I only commented on that in the bug discussion.

markwherry9 commented 8 years ago

http://www.tldp.org/LDP/sag/html/basic-ntp-config.html The NTP program is configured using either the /etc/ntp.conf or /etc/xntp.conf file depending on what distribution of Linux you have. I won't go into too much detail on how to configure NTP. Instead I'll just cover the basics.

You do need to watch this file if the server / client is going to peer with the correct servers.

gravyboat commented 8 years ago

@sroegner, @markwherry9: Okay here is my proposal for a fix, we drop including the init in the server state (there is literally no reason to do this anyways), then modify the init to only support the client since there is so much duplication between the two. What do you guys think? This would address the idea that ntp doesn't restart properly for the client so mark's use case is resolved, and making the server state its own independent state addresses Sroegner's issue.

sroegner commented 8 years ago

@gravyboat In general I agree with the solution but then the server state is stuck watching a file that is useless (/etc/ntpd.conf) in Linux as opposed to watching /etc/ntp.conf. In essence it appears to me that at it's current state server.sls should be dropped entirely. Not sure :facepalm:

tiadobatima commented 8 years ago

Hi guys... a bit late to the party, but right now now this formula got broken by commit https://github.com/saltstack-formulas/ntp-formula/commit/5231581c85032234b0d93ab918cf7be790b5348e in #22 

          ID: ntp_running
    Function: service.running
        Name: ntp
      Result: False
     Comment: The following requisites were not found:
         watch:
             file: /etc/ntp.conf
     Started: 
    Duration: 
     Changes:

Basically, if 'ntp:ntp_conf' pillar is not explicitly set by the user, line 11 of init.sls evaluates to None, and 'ntp_conf' states is not created, making that watch fail on line 27. The Ubuntu image for EC2 does provide a good default config, so there's no point in setting my own pillar/config. Maybe that 'if' statement around 'ntp_conf' state should not exist, and instead 'ntp_conf' could make sure some permissions are correct so that state at least exist when running with a default config.

gwaters commented 8 years ago

By default the formula should work, quoting this from the Salt documentation: "Each Formula is intended to be immediately usable with sane defaults without any additional configuration", so I agree with @tiadobatima there, and I am having the same problem with my Centos 7 builds of if statement not creating the file by default correctly. Which is weird because I see in the map.jinja a Redhat OS family. I am new to this but I ll try to dig in more tomorrow.

gwaters commented 8 years ago

@tiadobatima I made a new PR #32 to address what you discovered tha the ntp_conf_src is not set unless the pillar is explicitly set by the user. The PR checks if ntp_conf_src has been set, if it hasnt, set the ntp_conf variable to be the default ntp.conf that comes with this formula.