sensu / sensu-chef

Sensu Chef cookbook.
https://supermarket.chef.io/cookbooks/sensu
Apache License 2.0
222 stars 280 forks source link

Sensu services not restarted on create actions in chef 13.x #564

Closed JHBoricua closed 6 years ago

JHBoricua commented 7 years ago

When adding checks, handlers, etc, the chef-client run doesn't restart the affected Sensu services. Interesting enough, when doing a delete action on a check/handler/etc the Sensu services are restarted.

Expected Behavior

Sensu services should be restarted when adding/deleting checks/handlers/etc. so that the configuration change is picked up by the sensu server/clients.

Current Behavior

Only delete actions are triggering the service restart

Possible Solution

I believe the issue is in the json_file.rb provider. The service trigger for the create action reads (line 2): sensu_service_trigger = !!run_context.resource_collection.detect do |r|

whereas in the delete action it reads (line 26): sensu_service_trigger = !!node.run_context.resource_collection.detect do |r|

I added the missing 'node.' to the create portion and now the sensu services are restarted when adding checks/handlers/etc.

Steps to Reproduce (for bugs)

Create a check and run chef-client.

Your Environment

cwjohnston commented 7 years ago

@JHBoricua thanks for opening this issue. @arifcse019 asserted in https://github.com/sensu/sensu-chef/pull/468 that removing node, which I believe narrows the scope of run_context examination, would solve an issue similar to the one you're describing here.

I believe that using the node object's run_context instead of the more limited run_context of the LWRP is the correct way to handle this, so I am inclined to take your suggestion and revert the change merged in https://github.com/sensu/sensu-chef/pull/468.

That said, I'd really like to implement a meaningful unit or integration test that would prove whether or not this feature is working correctly. Any ideas?

cwjohnston commented 7 years ago

I'm working on an integration test to ensure services are restarted as expected on create/update, and so far cannot reproduce the "doesn't restart" behavior described here.

cwjohnston commented 7 years ago

@tas50 any thoughts on this? I lack the context to understand if we should be examining node.run_context or run_context.

mbbroberg commented 6 years ago

Curious if you have any insights @majormoses. Also bump to @tas50 🤗

majormoses commented 6 years ago

I can try to take a closer look later, I know that we had some weird issues with restarting sensu-client failing quite a bit a while back so we extended the chef recipe class to do some terrible things that made this work consistently. When I have some time I will look at what we did and see if there is any connection.

One interesting idea not directly related to the specific problem but perhaps a better way to handle it would be to pipe the json files to the api rather than needing a restart in the first place. This also helps prep us for the sensu 2.0 style of managing config.

tmonk42 commented 6 years ago

I am seeing the same problem with sensu-client not restarting when changes are made to the config:

Version of this cookbook used: 4.0.6 Version of Sensu used: 0.26.3-1 Version of Chef used: 13.5.3-1 Operating System and version (e.g. CentOS 7, Ubuntu 14.04): Ubuntu 16.04.3

majormoses commented 6 years ago

@tmonk42 can you see if you can replicate this on 4.0.2 which is what I am running and I have not noticed this. TBH I have not looked super closely but I am not sure that we are not somehow handling that somehow in our wrapper cookbooks. I would certainly have noticed if it required me to issue a restart manually as I pushed out a check the other day and it worked as expected.

tmonk42 commented 6 years ago

@majormoses confirmed, I still have the issue: Version of this cookbook used: 4.0.2 Version of Sensu used: 1.0.3-1 Version of Chef used: 13.5.3-1 Operating System and version: Ubuntu 16.04.3

majormoses commented 6 years ago

I need to do some more research on the differences in context there to know if this is correct or not.

tmonk42 commented 6 years ago

This combo also has the issue: Version of cookbook used: 4.0.6 Version of Sensu used: 1.0.3-1 Version of Chef used: 13.5.3-1 OS: Ubuntu 14.04.5

On this system I install chef 12, and the problem went away: This combo also has the issue: Version of cookbook used: 4.0.6 Version of Sensu used: 1.0.3-1 Version of Chef used: 12.19.36-1 OS: Ubuntu 14.04.5

majormoses commented 6 years ago

OK so this is due to a breaking change in sensu-client in 13.x? @tas50 what's the best path forward?

majormoses commented 6 years ago

I will try to read through the changelog for chef 13 and see if I can glean anything when I have the time.

tmonk42 commented 6 years ago

FWIW, can use a kitchen flag to set depreciations as errors in kitchen: https://docs.chef.io/chef_deprecations_client.html and https://blog.chef.io/2017/01/30/preparing-for-chef-13/

provisioner: deprecations_as_errors: true resource_cloning: false

majormoses commented 6 years ago

ya, I'd bet that breaks all sorts of things...going to a new major in chef is not a trivial thing.

majormoses commented 6 years ago

If I had to take a guess I would guess this: https://docs.chef.io/deprecations_resource_cloning.html

scalp42 commented 6 years ago

For what it's worth, I'm working around the issue by notifying the ruby_block resource by hand:

sensu_check 'check_uchiwa_health' do
  command %|check-uchiwa-health.rb -u #{node['uchiwa']['settings']['user']} -p #{node['uchiwa']['settings']['pass']} -P #{node['uchiwa']['settings']['port']}|
  handlers %w(mailer)
  subscribers ['sensu-server']
  interval 120
  additional(
    occurrences: 2,
  )
  notifies :create, 'ruby_block[sensu_service_trigger]', :delayed
end
majormoses commented 6 years ago

I don't currently have a sensu environment to test with but I can put a PR that someone could test with chef 12 and 13 for me I am :+1: to get this fixed.

majormoses commented 6 years ago

If someone can test and confirm it working with chef 12 and 13 I will fix this up and get it released.

scalp42 commented 6 years ago

@majormoses putting this on my radar for today, give me some time 💅

scalp42 commented 6 years ago

@majormoses results in https://github.com/sensu/sensu-chef/pull/606 ✌️

majormoses commented 6 years ago

released: https://supermarket.chef.io/cookbooks/sensu/versions/4.3.1 which should fix this problem