arturictus / sidekiq_alive

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

Replace rack with basic http server based on gserver #106

Closed andrcuns closed 8 months ago

andrcuns commented 8 months ago

This change introduces basic http server based on gserver that can be used instead of rack based servers.

Default http server will be used if server configuration option is not set. Fallback to default also happens if rackup gem is not available with rack > 3 version.

Based on https://github.com/arturictus/sidekiq_alive/pull/101#issuecomment-1784521374

Closes https://github.com/arturictus/sidekiq_alive/pull/75, https://github.com/arturictus/sidekiq_alive/pull/101

andrcuns commented 8 months ago

@arturictus I played around with the idea of implementing a very simple http server in pure ruby.

This would be a breaking change because we remove ability to set which web server to use, but honestly, I'm not sure how useful that was in the first place. Our use case is very simple, so I think such simple implementation without additional dependencies makes sense.

arturictus commented 8 months ago

@andrcuns , that looks great!, great work!

How confident are you about the reliability, memory leaks, etc? If you are very confident that it works we can ship it as it is.

This would be a breaking change because we remove ability to set which web server to use

We could leave the config and add a deprecation warning or fall back to rack if the configuration is present and is installed

I'm not sure how useful that was in the first place.

I think is great to have the least dependencies as possible.

Let me know your thoughts

andrcuns commented 8 months ago

How confident are you about the reliability, memory leaks, etc? If you are very confident that it works we can ship it as it is.

Not confident at all 😄 But I could build the gem from branch and run it in my application for a couple of days to see if everything functions same way as before

We could leave the config and add a deprecation warning or fall back to rack if the configuration is present and is installed

It's tricky because we need to leave all the previous implementation with using rack but I think it might be doable.

andrcuns commented 8 months ago

Played around a bit and I think we can support all cases. Use plain simple server by default but still allow using rack if explicitly configured without restricting version.

andrcuns commented 8 months ago

@arturictus I think this is ready. I have spent a bit of time experimenting and have been running this in my own cluster and it seems to be working as expected, no healthcheck fails or memory leaks.

I ended up leaving support for custom server. I think we can leave that option with having in mind, that if used, user is responsible for making sure the right gems are present in the project.

I also have a follow up for this mr as well which should fix other issues present in the project currently

andrcuns commented 8 months ago

@arturictus Yes, I have been using gem built from this branch in my app running on google kubernetes cluster for a couple of days and did not notice any issues, the healthcheck worked exactly as expected.

My proposal is to merge this and I already have a follow up that improves some other stuff. After merging the follow up, before releasing, I could use the gem built from master for a week or so, just to be sure. It is a fairly substantial change after all.