arturictus / sidekiq_alive

Liveness probe for Sidekiq in Kubernetes deployments
MIT License
190 stars 57 forks source link

Sinatra Thread blocks sidekiq shutdown signal #15

Closed leifcr closed 4 years ago

leifcr commented 5 years ago

After trying to debug why on(:shutdown) never was triggered, I found that removing the Sinatra thread, allows on(:shutdown) to trigger properly. (However, this brakes the intention of the gem)

The sigterm signals hit the sinatra server, and never the sidekiq process, which results in the on(:shutdown) never getting hit.

One option is to run the webserver and sidekiq separate when using sidekiq alive. This can be done by removing the severthread and creating a config.ru to start the server in a separate process using foreman. This is more complicated, but better if other rack servers are required in production.

Another option is to force the alive process to use webrick as a webserver. (Easy to implement)

for forcing the process to use webrick for the alive signal, change the top of server.rb to:

  class Server < Sinatra::Base
    set :bind, '0.0.0.0'
    set :server, :webrick
.....
leifcr commented 5 years ago

After testing further, forking instead of creating a thread works fine. I've created a commit that implements it very rough here: https://github.com/leifcr/sidekiq_alive/commit/4054b9f28931e5590a1a51927a8e0a8ce3b35a1e

arturictus commented 5 years ago

@leifcr that's great! can you make a PR. Thanks!

gsmetal commented 5 years ago

@leifcr Hello, why don't you PR you changes (very useful at first glance) into upstream?

gsmetal commented 5 years ago

Yeah, looks like your fork fixes all of our problems with this gem (and includes our patches), so I switched to it and will try on production.

leifcr commented 5 years ago

Sorry for the lack of PR on this one. Still only running this on a minikube testing env. As soon as I have it running in production and verify that it works, I'll create a new proper PR.

arturictus commented 4 years ago

fixed