Update puma service on deployment with up to date service config - #2118
Update puma service on deployment with up to date service config#2118ianheggie-oaf wants to merge 8 commits into
Conversation
4eb146e to
d75daf8
Compare
benrfairless
left a comment
benrfairless
left a comment
There was a problem hiding this comment.
Good change overall, and the testing writeup is exactly right: SIGSTOPping the master to prove the watchdog actually fires is much better evidence than assuming Type=notify works. Moving the memory and timeout tuning out of the template and into puma_service_unit_props is the right direction too.
Requesting changes on three things:
MemoryMaxhas gone from 1680M back to 1500M while the comment above it still says 1,680. Looks like an accidental revert of 311c52d.puma_systemctl_useris being used as a username, but the gem treats it as a:system/:usermode flag. It works by luck today, and the coupling withcheck_lingeringisn't visible from either file.- The template file is now byte-identical to the gem's, so it'd be better deleted than kept in sync.
Two more worth a look before merge: puma:install runs after puma:smart_restart, so Type= and WatchdogSec= won't actually take effect until the next reboot or manual restart, and check_lingering raises after the deploy has already published and restarted.
One framing note that might save some time. A few of the new settings aren't doing what the description implies. Type=<%= service_unit_type %> is an unchanged context line in the template diff and the gem's helper already returns "notify" for Puma 6.1+, puma_systemd_watchdog_sec matches the gem default of 10, and the hardcoded log paths equal what the gem derives from shared_path. The real payload here is puma_enable_lingering, false, the puma:install hook, and puma_service_unit_props. The simple to notify switch comes from finally regenerating a unit file that had been frozen at 6.0.0.beta.1, not from the new settings.
Detail is in the three file comments. Nothing security-related found, and I'm not raising a test coverage gap given there's no practical harness for Capistrano hooks in this repo.
4dda37d to
2b5d608
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
9c0bb86 to
a7b7fab
Compare
@benrfairless - I have updated these, rebased and its ready for reviewing again |
2030676 to
0c68e64
Compare
|
@benrfairless back to you for review pls |
0c68e64 to
83c08e0
Compare
Requested changes have been done, but was unable to mark it resolved in github
83c08e0 to
7362b08
Compare
ee23ee7 to
62f0c70
Compare
| namespace :puma do | ||
| desc "Check lingering is already set" | ||
| task :check_lingering do | ||
| on roles(fetch(:puma_role)) do |host| |
There was a problem hiding this comment.
Bug: The puma:check_lingering task defaults to the :web role, but production servers use the :app role, causing the task to silently skip execution without performing its check.
Severity: MEDIUM
Suggested Fix
Explicitly set the :puma_role in the deployment configuration to match the role used by the application servers. For example, add set :puma_role, :app to your deploy configuration file.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: lib/capistrano/tasks/puma_check_lingering.rake#L9
Potential issue: The `puma:check_lingering` task runs on servers with the role defined
by `:puma_role`. This variable is not set in the configuration, causing it to fall back
to the `capistrano3-puma` gem's default value of `:web`. However, the production servers
are configured with the `:app` role. As a result, the `on roles(fetch(:puma_role))`
command will not match any servers in the production environment. Capistrano silently
skips execution when no hosts match, meaning the linger check is never actually
performed. The task will complete without any errors or warnings, defeating its purpose
of verifying the server configuration.
* Fixed template error from gem * Check linger is set but doesn't try and fix (we dont have password less sudo)
…used This is a jump up from max 67.6% with MemoryMax=1500M for last 2 months
…sk to reflect purpose
ed25519 and bcrypt_pbkdf are left required because they are general not capistrano specific gems
62f0c70 to
59b9f96
Compare
|
benrfairless
left a comment
benrfairless
left a comment
There was a problem hiding this comment.
See comments from sentry that need review
2 participants
Description
Update puma service to reflect recent capistrano puma gem
Based on (rather than stack as 3 PRs depend directly on it):
Motivation and Context
Fixes service config was not being updated in normal deploy (and puma:enable calls
sudoby default which deploy user doesn't have access to):How Has This Been Tested?
Developed and tested on Ubuntu 24.04.4 LTS x86_64 with ruby installed by mise, mysql 8.0, redis 5:7.0, make 4.3
Deployed using updated capistrano config.
Checked systemd was being alerted to status (each 5 seconds) and that it restarts a process that dies:
Screenshots (if appropriate):
Types of Changes
Checklist: