Merge pull request #22 from gds-operations/worker_wait_parameter

Parameterise time to wait before killing old unicorns when reloading

README.rst

@@ -53,8 +53,8 @@ intended to be daemonized.
 
 Unicorn Herder *also* intercepts ``SIGHUP``, because this is the signal sent by
 Upstart when you call ``initctl reload``, and uses it to trigger a hot-reload of
-its Unicorn instance. This process will take two minutes, in order to give the
-new workers time to start up.
+its Unicorn instance. This process will take two minutes by default, in order to
+give the new workers time to start up.
 
 **NB**: There will be a period during hot-reload when requests are served by
 both old and new workers. This might have serious implications if you are

test/helpers.py

@@ -1,7 +1,7 @@
 import contextlib
 
-from nose.tools import *
-from mock import *
+from nose.tools import assert_equal, assert_false, assert_raises, assert_true
+from mock import call, patch, MagicMock
 
 @contextlib.contextmanager
 def fake_timeout_fail(*args, **kwargs):

test/test_herder.py

@@ -73,6 +73,27 @@ def test_configurable_boot_timeout(self, timeout_mock, popen_mock):
         assert_false(ret)
         popen_mock.return_value.terminate.assert_called_once_with()
 
+    @patch('unicornherder.herder.time.sleep')
+    @patch('unicornherder.herder.psutil.Process')
+    @patch('%s.open' % builtin_mod)
+    def test_configurable_overlap(self, open_mock, process_mock, sleep_mock):
+        h = Herder(overlap=17)
+
+        # Set up an initial dummy master process for the herder to kill later
+        open_mock.return_value.read.return_value = '123\n'
+        process_mock.return_value = MagicMock(pid=123)
+        h._loop_inner()
+
+        # Simulate a reloaded Unicorn
+        open_mock.return_value.read.return_value = '456\n'
+        process_mock.return_value = MagicMock(pid=456)
+
+        # Simulate SIGHUP, so the Herder thinks it's reloading
+        h._handle_HUP(signal.SIGHUP, None)
+
+        h._loop_inner()
+        sleep_mock.assert_any_call(17)
+
     @patch('unicornherder.herder.time.sleep')
     @patch('unicornherder.herder.psutil.Process')
     @patch('%s.open' % builtin_mod)
@@ -158,6 +179,8 @@ def test_loop_reload_pidchange_signals(self, open_mock, process_mock,
         ret = h._loop_inner()
         assert_equal(ret, True)
 
+        sleep_mock.assert_any_call(120)  # Check for the default overlap
+
         expected_calls = [call.send_signal(signal.SIGUSR2),
                           call.send_signal(signal.SIGWINCH),
                           call.send_signal(signal.SIGQUIT)]

unicornherder/command.py

@@ -23,7 +23,10 @@
 parser.add_argument('-p', '--pidfile', metavar='PATH',
                     help='Path to the pidfile that unicorn will write')
 parser.add_argument('-t', '--timeout', default=30, type=int, metavar='30', dest='boot_timeout',
-                    help='Timeout in seconds to start workers')
+                    help='Time to wait for new processes to daemonize themselves')
+parser.add_argument('-o', '--overlap', default=120, type=int, metavar='120',
+                    dest='overlap',
+                    help='Time to wait before killing old unicorns when reloading')
 parser.add_argument('-v', '--version', action='version', version=__version__)
 parser.add_argument('args', nargs=argparse.REMAINDER,
                     help='Any additional arguments will be passed to unicorn/'

unicornherder/herder.py

@@ -21,8 +21,6 @@
 
 MANAGED_PIDS = set([])
 
-WORKER_WAIT = 120
-
 
 class HerderError(Exception):
     pass
@@ -48,7 +46,7 @@ class Herder(object):
     """
 
     def __init__(self, unicorn='gunicorn', unicorn_bin=None, gunicorn_bin=None,
-                 pidfile=None, boot_timeout=30, args=''):
+                 pidfile=None, boot_timeout=30, overlap=120, args=''):
         """
 
         Creates a new Herder instance.
@@ -62,6 +60,8 @@ def __init__(self, unicorn='gunicorn', unicorn_bin=None, gunicorn_bin=None,
         pidfile      - path of the pidfile to write
                        (Default: gunicorn.pid or unicorn.pid depending on the value of
                         the unicorn parameter)
+        boot_timeout - how long to wait for the new process to daemonize itself
+        overlap      - how long to wait before killing the old unicorns when reloading
         args         - any additional arguments to pass to the unicorn executable
                        (Default: '')
 
@@ -79,6 +79,7 @@ def __init__(self, unicorn='gunicorn', unicorn_bin=None, gunicorn_bin=None,
         self.pidfile = '%s.pid' % self.unicorn if pidfile is None else pidfile
         self.args = args
         self.boot_timeout = boot_timeout
+        self.overlap = overlap
 
         try:
             if not unicorn_bin and not gunicorn_bin:
@@ -189,7 +190,7 @@ def _loop_inner(self):
             MANAGED_PIDS.add(self.master.pid)
 
             if self.reloading:
-                _wait_for_workers(self.master)
+                _wait_for_workers(self.overlap)
                 _kill_old_master(old_master)
                 self.reloading = False
 
@@ -270,12 +271,26 @@ def _emergency_slaughter():
             pass
 
 
-def _wait_for_workers(process):
+def _wait_for_workers(overlap):
     # TODO: do something smarter here
-    time.sleep(WORKER_WAIT)
+    time.sleep(overlap)
 
 
 def _kill_old_master(process):
+    """Shut down the old server gracefully.
+
+    There's a bit of extra complexity here, because Unicorn and Gunicorn handle
+    signals differently : both respond to SIGWINCH by gracefully stopping their
+    workers, but while Unicorn treats SIGQUIT as a graceful shutdown and
+    SIGTERM as a quick shutdown, Gunicorn reverses the meaning of these two.
+
+    <http://unicorn.bogomips.org/SIGNALS.html>
+    <http://gunicorn-docs.readthedocs.org/en/latest/signals.html>
+
+    We get around this by sending SIGWINCH first, giving the worker processes
+    some time to shut themselves down first.
+
+    """
     log.debug("Sending WINCH to old master (PID %s)", process.pid)
     process.send_signal(signal.SIGWINCH)
     time.sleep(1)