reload on service, checks and watch changes #152
Conversation
|
Obsoletes to #48 Looks good! I'm a little worried about regressions on this feature. Do you feel capable writing an acceptance test to ensure this doesn't break again? context "When removing a service that has been added" do
new_service_pp = <<-EOF
'consul::service { 'newservice':
... '
EOF
# Run it twice and test for idempotency
expect(apply_manifest(pp).exit_code).to_not eq(1)
expect(apply_manifest(pp).exit_code).to eq(0)
# Grab the pid
consul_pid = process('consul').get_column('pid')
# Remove the service
remove_service_pp = <<-EOF
'consul::service { 'newservice': ensure => absent }'
EOF
# Apply remove_service_pp
new_consul_pid = process('consul').get_column('pid')
it "should have the same pid after the removal (should reload)" do
new_consul_pid.should == consul_pid
end
endI'm just kinda eyeballing that. This is not going to be required to pull in this PR, but it would give it a better chance of sticking. (it would be pretty easy for the behavior of reload/restart to change based on the notification of the whole folder or whatever) Also can you rebase because I merged in your other PR first (I think) |
|
@solarkennedy sure thing, but looks like the acceptance tests are broken? There seems to be a conflict between the https://github.com/solarkennedy/puppet-consul/blob/master/Gemfile#L5 |
|
Very much looking forward to these changes! ✨ Was literally about to start on them myself. |
|
Er, hold on, let me try to use a gemfile I know works... |
|
Uh... aparently we have not been testing against the version of puppet that travis has been injecting... |
|
Can you merge master and try to run the acceptance tests again? Again I wouldn't sink that much time into this unless you are very eager. |
|
@solarkennedy Yah, I've put almost my entire day into this so I want to make this work! 😄 If I don't by tomorrow evening or something lets merge this since a lot of people are looking forward to this, apparently 😛 I'll let you know what happens. |
|
@solarkennedy I'm still working on these acceptance tests, but if its ok with you I'd say we merge this and when I have the tests ready I will make a new PR. Thoughts? |
|
Yes. My only other concern here is: are there configuration options that do require a restart? Can this code change get people into a situation where they make some sort of config change and it does not take effect? |
|
@solarkennedy Thats a valid concern. I tested each individual param to ensure just reloading is sufficient. Unfortunately the docs don't mention any config that does require a restart, it just puts service/check/watch under the "reloadable items" umbrella. I'm actively using this change on a lot of instances and so far everything seems to be ok. |
reload on service, checks and watch changes
|
Thanks! You are also now a contributor if you didn't get a notification. No need to fork if you don't want to. |
|
sweet! thanks 😄 |
|
Yup, all of the configurable params for watches, services, and checks seem to be reloadable - at least I've not had any issues where the changes haven't applied after a reload. Thanks a bunch for working on these changes & getting them merged in @aj-jester & @solarkennedy! 👍 👍 👍 👍 |
Certain config changes can trigger a reload by sending
SIGHUPsignal (whichconsul reloadessentially does). I implemented reload for services, checks and watches although more are supported.Also confirmed while adding, modifying and removing these resources consul PID remained unchanged throughout.
https://www.consul.io/docs/commands/reload.html
https://www.consul.io/docs/agent/options.html#reloadable-configuration