Refactor Puma restarts #257

olbrich · 2021-03-08T13:53:53Z

This PR modifies the restart process for puma so that it can do a proper restart instead of killing and restarting the app.

olbrich · 2021-03-08T13:55:10Z

.rubocop.yml

@@ -121,6 +121,14 @@ Metrics/AbcSize:
 Metrics/BlockLength:
  Enabled: false

+Metrics/ClassLength:


These cops were forcing people to smash arrays, hashes, and other things into as few lines as possible, which just impaired readability.

Dockerfile

olbrich · 2021-03-08T13:57:26Z

libraries/drivers_appserver_base.rb

        context.template file_path do
          mode '0640'
-          source 'appserver.monitrc.erb'
+          source "#{opts[:adapter]}.monitrc.erb"
+          cookbook opts[:source_cookbook].to_s


This should let a cookbook override the template if a user needs complete control. The worker driver already does this.

olbrich · 2021-03-08T13:58:14Z

libraries/drivers_appserver_base.rb

          variables opts
+          notifies :run, 'execute[monit reload]', :immediately


Forces a reload of monit when the file changes.

between this and setup there's a gotcha that's burned me in production twice now, because a Rails app server should not be started until all deploy tasks are completed; most obviously, asset compilation, but more besides (e.g. I have bespoke secrets injection at deploy time).

The catch here is that we need monit to reload the configs immediately so that later in the run when we want it to start the server, the configs that make that happen have already been loaded.

I think the way to finesse this is with the onreboot monit option.

If we mark the service in monit as onreboot nostart until all deploy tasks are complete, then the service can be programatically stopped & started when we want, but monit won't auto-start it prematurely.

Once deploy tasks are completed and the service is ready, we'd set it back to the default of onreboot start, so that it still comes up on soft reboot.

@inopinatus Interesting approach. I'll see if I can get that to work.

What I see right now in my testing is that there are times when monit will attempt to start the rails server before it is properly configured (and the code may not be there either), but it just fails. Once the deploy happens and we trigger the start or restart via monit everything should be in place and work fine.

Is there something custom about your setup that would prevent this approach from working?

@olbrich nothing custom. The problem is that monit can attempt to start the server after the code is in place, but before asset compilation has run/completed. Rails will start, but error out as soon as it attempts to consult the asset pipeline.

libraries/drivers_appserver_base.rb

libraries/drivers_appserver_puma.rb

olbrich · 2021-03-08T14:00:23Z

libraries/drivers_appserver_puma.rb

+        pidfile = "/var/run/lock/#{app['shortname']}/puma.pid"
+        context.execute "monit restart #{adapter}_#{app['shortname']}" do
+          retries 3
+          only_if { ::File.exist?(pidfile) }


Only runs if there is a pidfile, which generally means that puma is already running.

olbrich · 2021-03-08T14:01:27Z

libraries/drivers_webserver_base.rb

@@ -2,7 +2,7 @@

 module Drivers
  module Webserver
-    class Base < Drivers::Base # rubocop:disable Metrics/ClassLength


No longer needed.

olbrich · 2021-03-08T14:02:37Z

libraries/drivers_worker_base.rb

          mode '0640'
          source "#{opts[:adapter]}.monitrc.erb"
          cookbook opts[:source_cookbook].to_s
          variables opts
+          notifies :run, 'execute[monit reload]', :immediately


Only reload monit when this config file changes.

coveralls · 2021-03-08T14:02:51Z

Coverage decreased (-0.1%) to 99.894% when pulling db9543d on Mckesson-cds:monit-appserver-restarts into 3020734 on ajgon:master.

olbrich · 2021-03-08T14:03:18Z

recipes/configure.rb

@@ -37,6 +37,7 @@
  fire_hook(:configure, items: databases + [source, framework, appserver, worker, webserver])

  execute 'monit reload' do
+    action :nothing


Don't reload monit during a config run. If one of the config files change, they will notify this that a reload is needed.

olbrich · 2021-03-08T14:03:41Z

recipes/setup.rb

@@ -197,3 +197,9 @@

  fire_hook(:setup, items: databases + [source, framework, appserver, worker, webserver])
 end
+
+# setup hooks for appservers and workers may need to reload monit configs
+execute 'monit reload' do


Allow for monit reloads during setup.

olbrich · 2021-03-08T14:05:59Z

recipes/setup.rb

+# setup hooks for appservers and workers may need to reload monit configs
+execute 'monit reload' do
+  action :nothing
+  only_if 'which monit'


I think the opsworks agent may install monit for its own purposes, so maybe we can drop the conditional check for monit and maybe just make it a full blown dependency in the metadata file.

olbrich · 2021-03-08T14:06:09Z

spec/fixtures/aws_opsworks_app.rb

@@ -1,6 +1,5 @@
 # frozen_string_literal: true

-# rubocop:disable Metrics/MethodLength


No longer needed.

olbrich · 2021-03-08T14:06:23Z

spec/fixtures/aws_opsworks_rds_db_instance.rb

@@ -1,6 +1,5 @@
 # frozen_string_literal: true

-# rubocop:disable Metrics/MethodLength


No longer needed

olbrich · 2021-03-08T14:09:28Z

templates/default/thin.monitrc.erb

+check process <%= @appserver_name %>_<%= @app_shortname %> with pidfile <%= pid_dir %><%= @appserver_name %>.pid
+start program = "/bin/sh -c 'cd <%= File.join(@deploy_to, 'current') %> && <%= @environment.map {|k,v| "#{k}=\"#{v}\""}.join(' ') %> <%= @appserver_command %> | logger -t <%= @appserver_name %>-<%= @app_shortname %>'" as uid "<%= node['deployer']['user'] %>" and gid "<%= node['deployer']['group'] %>" with timeout 90 seconds
+stop program = "/bin/sh -c 'cat <%= pid_dir %><%= @appserver_name %>.pid | xargs --no-run-if-empty kill -QUIT; sleep 5'" as uid "<%= node['deployer']['user'] %>" and gid "<%= node['deployer']['group'] %>"
+stop restart = "/bin/sh -c 'cat <%= pid_dir %><%= @appserver_name %>.pid | xargs --no-run-if-empty kill -USR2; sleep 5'" as uid "<%= node['deployer']['user'] %>" and gid "<%= node['deployer']['group'] %>"


TODO: Actually, I'm not sure this is the right way to restart thin. Find the right way or just remove the 'restart' line, in which case monit will fall back to a stop-start.

@ajgon if you are familiar with Thin, you might have some insight to this one.
https://github.com/macournoyer/thin/blob/0712d603a31d97b9fa8a0260da400da2e4217d60/lib/thin/server.rb#L247 suggests that USR2 is not actually handled.

inopinatus · 2021-03-08T23:03:40Z

I agree that a refactor of these elements is strongly motivated! However, I worry that the monitrc files, like crontabs, get messy really fast, and they're not an obvious place for per-app configuration. Having any per-application configuration in /etc seems inconsistent to me when practically all other per-application configuration appears under /srv.

I suspect that pulling server-specific logic away from monit may produce the most consistent and most maintainable result, and monit should just call a standard init-style interface.

This may also help resolve #255 and #256, which have temporarily obliged me to fork opsworks_ruby and revert 37b9465 for my own production. Basically, I believe monit doesn't really understand foreground processes, and is actually timing out the server start, with severe negative consequences. I think we need to find a way that supports foregound puma, but returns quickly to monit - which to me sounds like the business of an wrapper script.

Some app servers may also need a stop-start (rather than a reload/refork) under limited circumstances, e.g. when environment variables change. There is no way I can see to squeeze that into a monitrc either, it'd be very much an edge case. In any case I'm also in favour of secrets not appearing in log files and ps output.

What this adds up to is a suggestion of a slightly different refactoring (in some ways, a partial reversion to pre-1.20 structures), using a standard monitrc, to invoke a #{deploy_to}/shared/scripts/service wrapper that's templated per-server-type. What do you think?

inopinatus · 2021-03-08T23:20:31Z

can I separately suggest, teasing the rubocop/style changes into a separate PR.

I know it's not always possible, and for minor deltas hardly matters, but I recommend it for all refactorings of substance, and particularly in Ruby code where the distinction between "style" and "substance" is not always clear cut and we have to build a mental model of the author's preferences and opinions to comprehend the diff.

olbrich · 2021-03-12T17:53:08Z

TODO: unmonitoring via monit won't work if the the server fails to setup properly in the first place and can throw errors on a shutdown.

templates/default/puma.monitrc.erb

olbrich · 2021-04-28T16:55:30Z

FYI, this seems to be working well for us right now. I'll clean this up and get it reviewed soon.

olbrich · 2021-06-08T00:40:01Z

@inopinatus We've been using this modification successfully in production for a month or two now. If you haven't already, can you give it a try with your setup and see if you run into any snags or gotchas like the ones you mentioned before?

stale · 2021-08-07T01:50:30Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

olbrich · 2021-08-31T17:01:41Z

Definitely don't want to forget about this one. Still good and still working well in production.

ajgon · 2021-09-02T16:33:54Z

@olbrich can I consider it stable enough, to merge it?

olbrich · 2021-09-13T21:06:22Z

@ajgon I think so, let me take another look through and see if everything looks right.

olbrich · 2021-10-08T16:14:40Z

@ajgon can you re-open this PR (#257). I'd like to get it finished off and merged.

ajgon · 2021-10-08T17:14:10Z

@olbrich Done.

…llow them to be sourced from a different cookbook. Fixes puma restart

…it for puma

- It may not get setup in the first place (because an instance failed to create)

…andle this as you might expect

olbrich · 2021-10-09T16:08:34Z

@ajgon I think I have most of the tests / integration tests working now, but I'm stumped by this failure ... https://github.com/ajgon/opsworks_ruby/runs/3847204234?check_suite_focus=true#step:7:9417. Any ideas?

ajgon · 2021-10-09T20:04:19Z

@ajgon I think I have most of the tests / integration tests working now, but I'm stumped by this failure ... https://github.com/ajgon/opsworks_ruby/runs/3847204234?check_suite_focus=true#step:7:9417. Any ideas?

This is weird, and it looks like it's completely unrelated to the PR.

This integration test, checks deployment of an app stored as an archive on S3. To do so, it needs AWS access/secret key to actually fetch the app. The key is stored as repository secret, and then used by workflow to download the file. For whatever reason for this PR, those secrets are not populated - not sure if it's an github issue or test itself.

Don't worry about it, consider this test as passing - I'll review the PR tomorrow, if everything is okay, I'll merge it and check the build process again against master.

ajgon · 2021-10-09T20:13:32Z

I also see, that you introduce new option monit_template_cookbook to appserver and worker - can you please update docs/attributes.md accordingly?

Can you also tell me, if those changes introduce any "breaking changes"?

olbrich · 2021-10-12T20:56:31Z

@ajgon I don't know of any breaking changes. I'll look into the documentation update.

…estart instead of killing and restarting the app (ajgon#257)

olbrich commented Mar 8, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

olbrich commented Mar 8, 2021

View reviewed changes

libraries/drivers_appserver_base.rb Show resolved Hide resolved

olbrich commented Mar 8, 2021

View reviewed changes

libraries/drivers_appserver_puma.rb Show resolved Hide resolved

olbrich commented Mar 8, 2021

View reviewed changes

olbrich commented Mar 13, 2021

View reviewed changes

templates/default/puma.monitrc.erb Outdated Show resolved Hide resolved

stale bot added the stale label Aug 7, 2021

stale bot closed this Aug 14, 2021

ajgon reopened this Oct 8, 2021

stale bot removed the stale label Oct 8, 2021

olbrich added 9 commits October 8, 2021 17:15

Split out monit configs for appserver into individual templates and a…

5c1d47f

…llow them to be sourced from a different cookbook. Fixes puma restart

Fix flag in puma monit scripts

9e3c9c6

Modify after_deploy hook to either start or restart appserver via mon…

80339dd

…it for puma

Fix syntax for createas and not_if

f91f560

Fix backwards logic for restarting puma

b8d093b

Restore Dockerfile

5eee5cf

Switch to using bundler to run pumactl instead of binstubs

7ebd92a

Don't try to unmonitor the appserver

0d4e37f

- It may not get setup in the first place (because an instance failed to create)

Move interpolation out of the execute block as chef doesn't seem to h…

36ac222

…andle this as you might expect

olbrich force-pushed the monit-appserver-restarts branch from ac289e4 to 36ac222 Compare October 8, 2021 21:31

olbrich added 6 commits October 8, 2021 17:38

remove redundant rubocop

48b64a4

Stub command for specs

92160ef

adjust restarting of thin

72dd86a

fix specs

20ea942

fix specs

cf59435

fix specs

6da95fb

Add documentation about 'monit_template_cookbook' for appservers

0d8c729

olbrich marked this pull request as ready for review October 12, 2021 21:07

ajgon merged commit 035ffb9 into ajgon:master Oct 13, 2021

dotnofoolin pushed a commit to dotnofoolin/opsworks_ruby that referenced this pull request Nov 23, 2021

fix: modify the restart process for puma so that it can do a proper r…

7260948

…estart instead of killing and restarting the app (ajgon#257)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Puma restarts #257

Refactor Puma restarts #257

olbrich commented Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

inopinatus Mar 8, 2021

olbrich Mar 9, 2021

inopinatus Mar 12, 2021

olbrich Mar 13, 2021

inopinatus Mar 14, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

coveralls commented Mar 8, 2021 •

edited

Loading

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Mar 8, 2021

olbrich Sep 13, 2021

inopinatus commented Mar 8, 2021 •

edited

Loading

inopinatus commented Mar 8, 2021

olbrich commented Mar 12, 2021

olbrich commented Apr 28, 2021

olbrich commented Jun 8, 2021

stale bot commented Aug 7, 2021

olbrich commented Aug 31, 2021

ajgon commented Sep 2, 2021

olbrich commented Sep 13, 2021

olbrich commented Oct 8, 2021

ajgon commented Oct 8, 2021

olbrich commented Oct 9, 2021

ajgon commented Oct 9, 2021 •

edited

Loading

ajgon commented Oct 9, 2021

olbrich commented Oct 12, 2021

		variables opts
		notifies :run, 'execute[monit reload]', :immediately

		@@ -1,6 +1,5 @@
		# frozen_string_literal: true

		# rubocop:disable Metrics/MethodLength

Refactor Puma restarts #257

Refactor Puma restarts #257

Conversation

olbrich commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inopinatus commented Mar 8, 2021 • edited Loading

inopinatus commented Mar 8, 2021

olbrich commented Mar 12, 2021

olbrich commented Apr 28, 2021

olbrich commented Jun 8, 2021

stale bot commented Aug 7, 2021

olbrich commented Aug 31, 2021

ajgon commented Sep 2, 2021

olbrich commented Sep 13, 2021

olbrich commented Oct 8, 2021

ajgon commented Oct 8, 2021

olbrich commented Oct 9, 2021

ajgon commented Oct 9, 2021 • edited Loading

ajgon commented Oct 9, 2021

olbrich commented Oct 12, 2021

coveralls commented Mar 8, 2021 •

edited

Loading

inopinatus commented Mar 8, 2021 •

edited

Loading

ajgon commented Oct 9, 2021 •

edited

Loading