saltstack-formulas / ntp-formula

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

ntp.service fails over missing dependency #28

Open sroegner opened 8 years ago

sroegner commented 8 years ago

This is CentOS release 6.5 (Final), salt 2015.8.8.2 (Beryllium)

salt-call state.sls ntp.server

local:
----------
          ID: ntp
    Function: pkg.installed
      Result: True
     Comment: Package ntp is already installed
     Started: 15:30:51.197693
    Duration: 546.288 ms
     Changes:   
----------
          ID: ntp_running
    Function: service.running
        Name: ntpd
      Result: False
     Comment: The following requisites were not found:
                                 watch:
                                     file: /etc/ntp.conf
     Started: 
    Duration: 
     Changes:   
----------
          ID: ntpd
    Function: service.running
      Result: True
     Comment: The service ntpd is already running
     Started: 15:30:51.747220
    Duration: 119.102 ms
     Changes:   

Summary for local
------------
Succeeded: 2
Failed:    1
------------
sroegner commented 8 years ago

So ntpd is checked in init.sls and in server.sls?

gravyboat commented 8 years ago

So this was added a couple weeks ago (https://github.com/saltstack-formulas/ntp-formula/commit/5231581c85032234b0d93ab918cf7be790b5348e) and I merged it in via the associated PR. I'm not quite sure why the requisite is failing there because the file is set by default.

sroegner commented 8 years ago

@gravyboat It is failing because the condition for the config file is different from the service in init.sls right? But is there really a case to (re)start the service in both states?

gravyboat commented 8 years ago

Let me tag @markwherry9 since he worked on this. There are actually two configs it looks like @sroegner, one that is ntp, and one that is ntpd, but both are supported in the default pillar already, so maybe I'm just missing it. I think the case was fair for a restart but maybe we overlooked something. Looking forward to your input @markwherry9.

tiadobatima commented 8 years ago

Hi guys... We're seeing the same problem on ubuntu 14.04

gravyboat commented 8 years ago

@sroegner Okay just double checked, yes it's because of the naming: https://github.com/saltstack-formulas/ntp-formula/blob/master/ntp/init.sls#L12 versus https://github.com/saltstack-formulas/ntp-formula/blob/master/ntp/server.sls#L25, I looked through the code just a few minutes ago and this looks to be due to some confusion because of the damn NG formula using ntp and ntpd.

gravyboat commented 8 years ago

@sroegner, @tiadobatima, can you guys please give this a shot: https://github.com/saltstack-formulas/ntp-formula/pull/29

sroegner commented 8 years ago

@gravyboat am still getting the same error calling init.sls

local:
----------
          ID: ntp
    Function: pkg.installed
      Result: True
     Comment: Package ntp is already installed.
     Started: 14:50:34.437156
    Duration: 326.221 ms
     Changes:   
----------
          ID: ntp_running
    Function: service.running
        Name: ntpd
      Result: False
     Comment: The following requisites were not found:
                                 watch:
                                     file: ntp_conf
     Started: 
    Duration: 
     Changes:   

Summary
------------
Succeeded: 1
Failed:    1
------------

I think the problem are the two different conditions - {% if ntp_conf_src %} for the ntp_conf file and {% if ntp.ntp_conf -%} for the service (which i still don't think belongs into init.sls)

markwherry9 commented 8 years ago

Hi All, I added the watch on the service in init.sls because previously the formula was not reflecting changes to ntp configuration file and checking ntpq showed peers were always the default NOT those specified by the user. In fact, a manual service restart was needed to make ntpd peer to the correct servers.

I didn't realise that server.sls already had a watch. However, the watch in server.sls looks wrong to me because it's trying to watch 'ntp_conf' wheras in init.sls it watches "file: {{ ntp.ntp_conf }}" which should work across OSs.

I'm quite new to Saltstack so I don't know the best location for this service watch. However, I'm pretty sure one is needed and it should probably use something like file: {{ ntp.ntp_confi }}"

Thanks - Mark

sroegner commented 8 years ago

Thanks @markwherry9 for joining in - there is no doubt that the watch on the config makes sense but if that is ok with you (and the way you use the formula) the watch should probably be on the one service definition in server.sls. Otherwise using server.sls (as it includes init.sls) will attempt to start the same ntpd twice everywhere.

markwherry9 commented 8 years ago

Hi, that sounds fine to me @sroegner Just as long as there is a working watch function I'm happy.

sroegner commented 8 years ago

I believe that my PR fixes the issue at hand on Redhat - i am certainly not sure about any regressions especially on other platforms. Maybe @tiadobatima could take a look at this on Ubuntu?