sous-chefs / consul

Development repository for the consul cookbook
https://supermarket.chef.io/cookbooks/consul
Apache License 2.0
192 stars 244 forks source link

Improved the _start() method in the sysv initscript. Until now a sta… #433

Closed MichaelKueller closed 7 years ago

MichaelKueller commented 7 years ago

…rt of the consul agent followed by a reload will interrupt the start process and the consul agent will be stopped

You can simply reproduce this by calling: /etc/init.d/consul restart; /etc/init.d/consul reload; /etc/init.d/consul status

This is likely to happen if you make a change in a wrapper cookbook that triggeres a consul restart and another change which triggeres a consul reload. Your consul agent will be stopped afterwards.

To fix this this change will make sure the consul service is started before leaving the _start() method.

legal90 commented 7 years ago

@MichaelKueller Thank you! But I see that this fix covers only Debian platform. Could you please make it universal by moving this hook to a standalone function, like _wait_for_listening?

codecov-io commented 7 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@b4c3c47). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #433   +/-   ##
=========================================
  Coverage          ?   55.02%           
=========================================
  Files             ?        7           
  Lines             ?      358           
  Branches          ?        0           
=========================================
  Hits              ?      197           
  Misses            ?      161           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b4c3c47...2df9715. Read the comment docs.

MichaelKueller commented 7 years ago

@legal90 good point! I changed the code accordingly.

MichaelKueller commented 7 years ago

@legal90 I moved the hook to a _wait_for_listening function. I did not call it from the non-debian _start function though. After testing this with centos it turns out that the underlying issue does not exist there.

legal90 commented 7 years ago

@MichaelKueller @ZbigniewZabost-zanox Thank you!


@johnbellone Could you please disable Codecov integration for pull-requests?

legal90 commented 7 years ago

@MichaelKueller @ZbigniewZabost-zanox Hi guys! Seems like the method we've introduced here will not work with Consul 0.8+ having ACL enabled. In this case consul info could not be used for checking a service status because it always returns "403 Permission Denied", requesting the ACL token to be passed there. Here is the test failure on Debian 7: https://travis-ci.org/johnbellone/consul-cookbook/jobs/235152247 (from PR: https://github.com/johnbellone/consul-cookbook/pull/434)

Do you have any ideas how we can avoid using consul info in init scripts ?

moofish32 commented 7 years ago

@legal90 -- I think we have to move the ACL into the environment of the process manager scripts. The v8 ACL touches all commands AFAIK. The variable would be CONSUL_HTTP_TOKEN I think. The other option is prevent the cookbook from calling restart followed by reload and add a note for the user. The issue I opened #438 is a duplicate of the problem here. What are the key use cases requiring reload for current consumers? As 0.8 is a fairly serious change (with the raft update), perhaps this is a good opportunity to roll a major here as well and remove the reload.

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.