Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs improvement (or fork_worker not working?): trying to understand fork_worker, preload_app! and Phased restarts working together #2466

Closed
feliperaul opened this issue Oct 28, 2020 · 6 comments
Labels

Comments

@feliperaul
Copy link

I'm on Puma 5.0.3 and willing to issue a pull request to improve Puma's documentation regarding these points, but first I'd love some help better understanding them.

On the 5.0 upgrade guide, it states this:

fork_worker allows the usage of preload with phased_restart. It also may improve memory usage because the worker process loads additional code after processing requests.

However, If I have both these options in my puma.rb config file:

preload_app!
fork_worker

When I issue a kill -USR1 $(cat /var/www/shared/tmp/pids/puma.pid), I get a * phased-restart called but not available, restarting normally. in the logs, and it doesn't do a phased restart.

Moving on in my experimentation and removing the preload_app! from the config file, the docs state here that This allows a phased restart to complete as quickly as a hot restart (SIGUSR2 or pumactl restart), while still minimizing downtime by staggering the restart across cluster workers.

However, I timed kill -USR1 $(cat /var/www/shared/tmp/pids/puma.pid), and found out that:

  • It takes in average 20 seconds to terminate all 4 workers without fork_worker in the config file
  • It takes in average 20 seconds to terminate all 4 workers with fork_worker in the config file

Just as a side note, a simple systemctl restart puma.service is taking around 6 seconds to do a hot restart (so, not phased, killing all workers at once).

There's also no indication in the puma stdout.log that a phased restart with fork_worker is being performed (I couldn't find any code to log so, maybe that would be useful so we can be sure that the option is in effect). Am I missing something?

If it's of interest, here's my systemd service, and we use socket activation:

{{ ansible_managed | comment }}

[Unit]
Description=Puma {{ param_app_name }} application server
After=network.target

# Socket activation
Requires=puma-{{ param_app_name}}.socket

[Service]
Type=simple

User={{ param_user_name }}

WorkingDirectory=/var/www/{{ param_app_name }}/application

ExecStart=/home/{{ param_user_name }}/.rbenv/shims/bundle exec --keep-file-descriptors puma -C /var/www/{{ param_app_name }}/shared/config/puma.rb

Restart=always

[Install]
WantedBy=multi-user.target
@dentarg
Copy link
Member

dentarg commented Oct 28, 2020

According to https://github.com/puma/puma/blob/v5.0.4/docs/fork_worker.md it is SIGURG you need to send (kill -URG <pid>) when using fork_worker preload_app! + fork_worker. I can then see Puma logging Stopping <PID of Worker 0> for phased upgrade...... but I'm not sure I'm doing it right or if it is working, see #2467

dentarg added a commit to dentarg/puma that referenced this issue Oct 28, 2020
dentarg added a commit to dentarg/puma that referenced this issue Oct 28, 2020
@dentarg
Copy link
Member

dentarg commented Oct 28, 2020

So fork_worker and preload_app! is not compatible with each other... I opened #2468 to improve the upgrade guide about that

There's also no indication in the puma stdout.log that a phased restart with fork_worker is being performed

(without preload_app!) I can see Puma logging Starting phased worker restart... see the full output in #2467 (comment)

nateberkopec pushed a commit that referenced this issue Oct 29, 2020
@nateberkopec
Copy link
Member

Does that resolve the issue for you @feliperaul?

@dentarg
Copy link
Member

dentarg commented Feb 6, 2021

I'll close this one as I think the docs now cover the issue, let us know if you think there's something still missing @feliperaul

@dentarg dentarg closed this as completed Feb 6, 2021
@jrochkind
Copy link

If you configure both fork_worker and preload_app!.... you just don't get the fork_worker? But do get the preload_app!?

I think documentating "they are incompatible" isn't enough; I think if you configure incompatibly you should at least get a warning on boot, if not an error, no?

@dentarg
Copy link
Member

dentarg commented Mar 22, 2021

@jrochkind I agree, that sounds more clear to me. There was more discussion around the these config options in #2483. Perhaps that's a better place to continue the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants