launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-984687 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #984687 in MAAS: "Starting redundant pserv/txlongpoll leaves bogus pidfiles"
  https://bugs.launchpad.net/maas/+bug/984687

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-984687/+merge/102499

This wraps the startup calls to our Twisted apps to clean up their pidfiles when they exit.  In principle Twisted can do this itself, but it's not very thorough.  In particular, this fixes bug 984687: if the service exits immediately on startup because another branch is already running the same service, this won't leave a pointless pidfile in place.

I stole the code for parsing Twisted's options from the wrapped function.  That does mean that we end up parsing the command line twice.  Probably better than replicating twisted's run() function locally just to get at its options object.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/bug-984687/+merge/102499
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-984687 into lp:maas.

=== modified file 'buildout.cfg'
--- buildout.cfg	2012-04-11 14:29:49 +0000
+++ buildout.cfg	2012-04-18 11:52:29 +0000
@@ -118,7 +118,7 @@
   twisted
   txamqp
 entry-points =
-  twistd.pserv=twisted.scripts.twistd:run
+  twistd.pserv=maasserver.twisted_wrapper:run
 extra-paths = ${common:extra-paths}
 scripts =
   twistd.pserv
@@ -174,5 +174,6 @@
   pyyaml
   twisted
   txamqp
-entry-points = twistd.txlongpoll=twisted.scripts.twistd:run
+entry-points = twistd.txlongpoll=maasserver.twisted_wrapper:run
+extra-paths = ${common:extra-paths}
 scripts = twistd.txlongpoll

=== added file 'src/maasserver/tests/test_twisted_wrapper.py'
--- src/maasserver/tests/test_twisted_wrapper.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_twisted_wrapper.py	2012-04-18 11:52:29 +0000
@@ -0,0 +1,146 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for our Twisted wrapper."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+import os.path
+import sys
+
+from maasserver.twisted_wrapper import (
+    clean_up_pidfile,
+    file_exists,
+    get_pidfile_to_write,
+    get_server_options,
+    run,
+    )
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+import twisted.scripts.twistd
+
+
+def do_nothing(*args, **kwargs):
+    """Do absolutely nothing."""
+
+
+def write_file(filename):
+    """Write data to given file, and flush."""
+    with open(filename, 'w') as tf:
+        tf.write(factory.getRandomString().encode('ascii'))
+
+
+class TestTwistedWrapper(TestCase):
+
+    def make_nonexistent_filename(self):
+        """Compose a filename (including path) that does not exist.
+
+        However, the file is in a temporary directory.  So a test is welcome
+        to create it and write to it.
+        """
+        return os.path.join(self.make_dir(), factory.getRandomString())
+
+    def make_temp_file(self):
+        """Create and populate a temporary file."""
+        temp_file = self.make_nonexistent_filename()
+        write_file(temp_file)
+        return temp_file
+
+    def patch_args(self, pidfile=None):
+        """Patch command line arguments to look like twistd options."""
+        if pidfile is None:
+            pidfile = self.make_nonexistent_filename()
+        self.patch(sys, 'argv', [sys.argv[0], '--pidfile', pidfile])
+
+    def test_file_exists_says_True_if_file_exists(self):
+        self.assertTrue(file_exists(self.make_temp_file()))
+
+    def test_file_exists_says_False_if_file_does_not_exist(self):
+        self.assertFalse(file_exists(self.make_nonexistent_filename()))
+
+    def test_get_server_options_recognizes_pidfile_option(self):
+        pidfile = self.make_temp_file()
+        self.patch_args(pidfile)
+        self.assertEqual(pidfile, get_server_options().get('pidfile'))
+
+    def test_get_pidfile_to_write_returns_pidfile_from_options(self):
+        fake_options = {'pidfile': self.make_nonexistent_filename()}
+        self.assertEqual(
+            fake_options['pidfile'], get_pidfile_to_write(fake_options))
+
+    def test_get_pidfile_to_write_returns_None_if_already_exists(self):
+        self.assertIsNone(
+            get_pidfile_to_write({'pidfile': self.make_temp_file()}))
+
+    def test_clean_up_pidfile_deletes_pidfile(self):
+        pidfile = self.make_temp_file()
+        clean_up_pidfile(pidfile)
+        self.assertFalse(file_exists(pidfile))
+
+    def test_clean_up_pidfile_accepts_None(self):
+        clean_up_pidfile(None)
+        # The test is that this does not break.
+        pass
+
+    def test_clean_up_pidfile_does_not_break_if_file_is_already_gone(self):
+        nonexistent_name = self.make_nonexistent_filename()
+        clean_up_pidfile(nonexistent_name)
+        self.assertFalse(file_exists(nonexistent_name))
+
+    def test_leaves_pre_existing_pidfile_in_place(self):
+        # If a pidfile already existed, the twisted process will exit
+        # immediately.  The abortive run won't clean up the pidfile,
+        # since it doesn't own it.
+        pidfile = self.make_temp_file()
+        self.patch_args(pidfile)
+        self.patch(twisted.scripts.twistd, 'run', do_nothing)
+        run()
+        self.assertTrue(file_exists(pidfile))
+
+    def test_removes_new_pidfile_after_normal_exit(self):
+        # If a pidfile does not exist, the Twisted app writes it but the
+        # wrapper deletes it afterwards.
+        pidfile = self.make_nonexistent_filename()
+
+        def write_pidfile(*args, **kwargs):
+            write_file(pidfile)
+
+        self.patch_args(pidfile)
+        self.patch(twisted.scripts.twistd, 'run', write_pidfile)
+        run()
+        self.assertFalse(file_exists(pidfile))
+
+    def test_removes_new_pidfile_after_failure(self):
+        pidfile = self.make_nonexistent_filename()
+
+        def fail(*args, **kwargs):
+            write_file(pidfile)
+            raise Exception("Deliberate failure")
+
+        self.patch_args(pidfile)
+        self.patch(twisted.scripts.twistd, 'run', fail)
+        try:
+            run()
+        except Exception:
+            pass
+        self.assertFalse(file_exists(pidfile))
+
+    def test_reraises_exception(self):
+
+        class Kaboom(Exception):
+            pass
+
+        def fail(*args, **kwargs):
+            raise Kaboom()
+
+        self.patch_args()
+        self.patch(twisted.scripts.twistd, 'run', fail)
+
+        self.assertRaises(Kaboom, run)

=== added file 'src/maasserver/twisted_wrapper.py'
--- src/maasserver/twisted_wrapper.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/twisted_wrapper.py	2012-04-18 11:52:29 +0000
@@ -0,0 +1,63 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Wrapper for Twisted that cleans up pidfile when done."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'run',
+    ]
+
+import os
+
+import twisted.scripts.twistd
+
+
+def file_exists(path):
+    """Does the given file exist?"""
+    return os.access(path, os.F_OK)
+
+
+def get_server_options():
+    """Get Twisted's server options as parsed from the command line."""
+    options = twisted.scripts.twistd.ServerOptions()
+    options.parseOptions()
+    return options
+
+
+def get_pidfile_to_write(server_options):
+    """Get name of the pidfile we're going to write, or None.
+
+    Returns None if the pidfile already exists.  (In which case it's not up
+    to us to write it, nor to delete it later).
+    """
+    pidfile = server_options.get('pidfile')
+    if pidfile is not None and file_exists(pidfile):
+        # Our pidfile already exists.  That means we're not responsible
+        # for writing it, or for cleaning it up.
+        return None
+    else:
+        # We're still to write the pidfile.  And when pserv exists,
+        # clean it up as well.
+        return pidfile
+
+
+def clean_up_pidfile(pidfile=None):
+    """Clean up `pidfile`, if given."""
+    if pidfile is not None and file_exists(pidfile):
+        os.remove(pidfile)
+
+
+def run():
+    """Run app and, if we're using a pidfile, clean it up afterwards."""
+    pidfile = get_pidfile_to_write(get_server_options())
+    try:
+        twisted.scripts.twistd.run()
+    finally:
+        clean_up_pidfile(pidfile)

=== modified file 'src/maastesting/testcase.py'
--- src/maastesting/testcase.py	2012-04-16 10:00:51 +0000
+++ src/maastesting/testcase.py	2012-04-18 11:52:29 +0000
@@ -16,6 +16,8 @@
     'TransactionTestCase',
     ]
 
+from shutil import rmtree
+from tempfile import mkdtemp
 import unittest
 
 from django.conf import settings
@@ -55,6 +57,12 @@
     # probably only added to support compatibility with python 2.6.
     assertItemsEqual = unittest.TestCase.assertItemsEqual
 
+    def make_dir(self):
+        """Create a temporary directory for the duration of one test."""
+        temp_dir = mkdtemp()
+        self.addCleanup(rmtree, temp_dir, ignore_errors=True)
+        return temp_dir
+
 
 class TestCase(TestCaseBase, django.test.TestCase):
     """`TestCase` for Metal as a Service.