Replace Unicorn with Puma #8392
Conversation
Newly generated binstubs will check for the string `This file was generated by Bundler` inside `bin/bundle`, so we'd have to update that anyway. Also, there is a non-zero chance the updated `bundle` binstub resolves some of the setup-specific issues we've seen.
It's… safe to assume that nobody is using this, as this has been broken for quite some time.
There was a problem hiding this comment.
I found two little things.
Also, as podmins shouldn't change web_workers anymore, I was also wondering if we should add a config option to set the maximum number of threads, or if the default (5) is fine for all pod sizes? Or for really big pods would that still be the point where they would increase the workers if they need more than 5 threads?
denschub
force-pushed
the
unicorn-dust
branch
from
ae5740d
to
68e407a
Compare
|
Review feedback addressed in the force push.
So... I don't think so. Puma will auto-scale its thread pool with a default range of
There, I had a bit of a (self-induced) load spike, and Puma kept the extra threads around for more than a day, just in case, before eventually releasing it again. I don't think that the maximum of 5 is a problem. Puma will not scale up if the memory pressure is high, so there should not be ever a case where we're risking OOM. Large pods with lots of requests will see higher memory usage than smaller pods, but that's expected. Likewise, I don't think podmins will ever need more than 5 threads. I've never seen Geraspora exceed three threads, and I'm fairly sure that a pod would be completely on fire because of database latency before Puma scaling becomes a problem. So, I don't think we need to make threads configurable. And, writing that, I realize I might just have made a case for removing the workers pref altogether - I don't think there is ever a case where a podmin would need more than one worker. And if a podmin does, then they're able to change the Puma config directly. |
Yes, I think we should remove the |
… and drop the single_process_mode. See the included Changelog entry for full details on what this change means.
denschub
force-pushed
the
unicorn-dust
branch
from
68e407a
to
97cfc80
Compare
|
Setting removed - and Changelog changed accordingly - in another force-push. |
|
(Note to bystanders: rspec tests are unhappy in this PR. But that's not my fault. It's September 9th, which means the profile that's used to verify that the Birthday Notification System isn't spamming everyone, ... has its birthday today...) |
With diaspora#8392 the `single_process_mode` was removed, which means that development now also requires a redis.
When puma was introduced in diaspora#8392 the default listen configuration was set to only localhost, which makes sense for most development setups, but when run within docker, it needs to listen on all IPs so the port can be forwarded to be accessable outside of docker. Because the new default makes sense without docker, I overwrite the option with a environment variable only in the docker-setup. This also ensures that it always contains the right value needed for the docker-setup to work, no matter what was configured outside of docker.
With diaspora#8392 the `single_process_mode` was removed, which means that development now also requires a redis.
When puma was introduced in diaspora#8392 the default listen configuration was set to only localhost, which makes sense for most development setups, but when run within docker, it needs to listen on all IPs so the port can be forwarded to be accessable outside of docker. Because the new default makes sense without docker, I overwrite the option with a environment variable only in the docker-setup. This also ensures that it always contains the right value needed for the docker-setup to work, no matter what was configured outside of docker.
This PR primarily replaces Unicorn with Puma. Unicorn has been unmaintained for quite some time, and on the other hand, Puma has been in production on at least Geraspora and Nerdpol for quite some time.
For podmins that still run the default setup, this will probably result in a significant reduction in memory usage, as Puma is a lot more efficient. (It also doesn't need to be killed regularly...) Unfortunately, this change means some breaking config changes, but oh well. I have included a Changelog entry, so you don't need to come up with something during the merge.
This PR also contains one commit that removes traces of a 12+ years old attempt at creating some federation integration testing, and it updates all binstubs.