Merge lp:~lifeless/launchpad/run into lp:launchpad

[Robert Collins]

Improvements and prep work for consolidating Service with TacTestSetup.

Improvements and prep work for consolidating Service with TacTestSetup

[Robert Collins (lifeless)]

Note that the changes to ignore should_launch are because some services deliberately ignore it - they don't run by default but when explicitly chosen they need to run; their 'should_launch' is defined as 'return False' : we already filter so that only wanted services are attempted, so checking for should_launch on setUp is a harmful (breaks consistency, causes more code) belts-and-braces.

[Michael Hudson-Doyle (mwhudson)]

166 + """kill process and BLOCK until process dies.

Should have an initial capital.

I also wonder if stop_process should get more forceful than TERM, but well, that's something for another branch.

The changes look nice, btw.

review: Approve

=== modified file 'Makefile'
--- Makefile	2010-09-07 18:15:01 +0000
+++ Makefile	2010-10-11 04:11:44 +0000
@@ -255,7 +255,6 @@
 	$(RM) thread*.request
 	bin/run -r librarian,sftp,codebrowse -i $(LPCONFIG)
 
-
 start_librarian: compile
 	bin/start_librarian
 

=== modified file 'lib/canonical/launchpad/scripts/runlaunchpad.py'
--- lib/canonical/launchpad/scripts/runlaunchpad.py	2010-09-17 20:46:58 +0000
+++ lib/canonical/launchpad/scripts/runlaunchpad.py	2010-10-11 04:11:44 +0000
@@ -7,16 +7,18 @@
 __all__ = ['start_launchpad']
 
 
-import atexit
+from contextlib import nested
 import os
 import signal
 import subprocess
 import sys
 
+import fixtures
 from zope.app.server.main import main
 
 from canonical.config import config
 from lp.services.mailman import runmailman
+from canonical.launchpad.daemons import tachandler
 from canonical.launchpad.testing import googletestservice
 from canonical.lazr.pidfile import (
     make_pidfile,
@@ -28,20 +30,25 @@
     return os.path.abspath(os.path.join(config.root, *path.split('/')))
 
 
-TWISTD_SCRIPT = make_abspath('bin/twistd')
-
-
-class Service(object):
+class Service(fixtures.Fixture):
 
     @property
     def should_launch(self):
-        """Return true if this service should be launched."""
+        """Return true if this service should be launched by default."""
         return False
 
     def launch(self):
-        """Launch the service, but do not block."""
+        """Run the service in a thread or external process.
+        
+        May block long enough to kick it off, but must return control to
+        the caller without waiting for it to shutdown.
+        """
         raise NotImplementedError
 
+    def setUp(self):
+        super(Service, self).setUp()
+        self.launch()
+
 
 class TacFile(Service):
 
@@ -56,8 +63,7 @@
         :param pre_launch: A callable that is called before the launch
             process.
         """
-        # No point calling super's __init__.
-        # pylint: disable-msg=W0231
+        super(TacFile, self).__init__()
         self.name = name
         self.tac_filename = tac_filename
         self.section_name = section_name
@@ -80,10 +86,6 @@
         return config[self.section_name].logfile
 
     def launch(self):
-        # Don't run the server if it wasn't asked for.
-        if not self.should_launch:
-            return
-
         self.pre_launch()
 
         pidfile = pidfile_path(self.name)
@@ -91,7 +93,7 @@
         tacfile = make_abspath(self.tac_filename)
 
         args = [
-            TWISTD_SCRIPT,
+            tachandler.twistd_script,
             "--no_save",
             "--nodaemon",
             "--python", tacfile,
@@ -109,15 +111,8 @@
         # ability to cycle the log files by sending a signal to the twisted
         # process.
         process = subprocess.Popen(args, stdin=subprocess.PIPE)
+        self.addCleanup(stop_process, process)
         process.stdin.close()
-        # I've left this off - we still check at termination and we can
-        # avoid the startup delay. -- StuartBishop 20050525
-        #time.sleep(1)
-        #if process.poll() != None:
-        #    raise RuntimeError(
-        #        "%s did not start: %d"
-        #        % (self.name, process.returncode))
-        stop_at_exit(process)
 
 
 class MailmanService(Service):
@@ -127,11 +122,8 @@
         return config.mailman.launch
 
     def launch(self):
-        # Don't run the server if it wasn't asked for.  Also, don't attempt to
-        # shut it down at exit.
-        if self.should_launch:
-            runmailman.start_mailman()
-            atexit.register(runmailman.stop_mailman)
+        runmailman.start_mailman()
+        self.addCleanup(runmailman.stop_mailman)
 
 
 class CodebrowseService(Service):
@@ -144,8 +136,8 @@
         process = subprocess.Popen(
             ['make', 'run_codebrowse'],
             stdin=subprocess.PIPE)
+        self.addCleanup(stop_process, process)
         process.stdin.close()
-        stop_at_exit(process)
 
 
 class GoogleWebService(Service):
@@ -155,9 +147,7 @@
         return config.google_test_service.launch
 
     def launch(self):
-        if self.should_launch:
-            process = googletestservice.start_as_process()
-            stop_at_exit(process)
+        self.addCleanup(stop_process, googletestservice.start_as_process())
 
 
 class MemcachedService(Service):
@@ -180,23 +170,18 @@
         else:
             cmd.append('-v')
         process = subprocess.Popen(cmd, stdin=subprocess.PIPE)
+        self.addCleanup(stop_process, process)
         process.stdin.close()
-        stop_at_exit(process)
-
-
-def stop_at_exit(process):
-    """Create and register an atexit hook for killing a process.
-
-    The hook will BLOCK until the process dies.
+
+
+def stop_process(process):
+    """kill process and BLOCK until process dies.
 
     :param process: An instance of subprocess.Popen.
     """
-
-    def stop_process():
-        if process.poll() is None:
-            os.kill(process.pid, signal.SIGTERM)
-            process.wait()
-    atexit.register(stop_process)
+    if process.poll() is None:
+        os.kill(process.pid, signal.SIGTERM)
+        process.wait()
 
 
 def prepare_for_librarian():
@@ -273,26 +258,24 @@
     services, argv = split_out_runlaunchpad_arguments(argv[1:])
     argv = process_config_arguments(argv)
     services = get_services_to_run(services)
-    for service in services:
-        service.launch()
-
-    # Store our process id somewhere
-    make_pidfile('launchpad')
-
     # Create the ZCML override file based on the instance.
     config.generate_overrides()
 
-    if config.launchpad.launch:
-        main(argv)
-    else:
-        # We just need the foreground process to sit around forever waiting
-        # for the signal to shut everything down.  Normally, Zope itself would
-        # be this master process, but we're not starting that up, so we need
-        # to do something else.
-        try:
-            signal.pause()
-        except KeyboardInterrupt:
-            pass
+    with nested(*services):
+        # Store our process id somewhere
+        make_pidfile('launchpad')
+
+        if config.launchpad.launch:
+            main(argv)
+        else:
+            # We just need the foreground process to sit around forever waiting
+            # for the signal to shut everything down.  Normally, Zope itself would
+            # be this master process, but we're not starting that up, so we need
+            # to do something else.
+            try:
+                signal.pause()
+            except KeyboardInterrupt:
+                pass
 
 
 def start_librarian():
@@ -303,7 +286,7 @@
     prepare_for_librarian()
     pidfile = pidfile_path('librarian')
     cmd = [
-        TWISTD_SCRIPT,
+        tachandler.twistd_script,
         "--python", 'daemons/librarian.tac',
         "--pidfile", pidfile,
         "--prefix", 'Librarian',

=== modified file 'lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py'
--- lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/scripts/tests/test_runlaunchpad.py	2010-10-11 04:11:44 +0000
@@ -13,7 +13,8 @@
 import os
 import shutil
 import tempfile
-import unittest
+
+import testtools
 
 import canonical.config
 from canonical.config import config
@@ -119,12 +120,12 @@
         self.assertEquals('test', config.instance_name)
 
 
-class ServersToStart(unittest.TestCase):
+class ServersToStart(testtools.TestCase):
     """Test server startup control."""
 
     def setUp(self):
         """Make sure that only the Librarian is configured to launch."""
-        unittest.TestCase.setUp(self)
+        testtools.TestCase.setUp(self)
         launch_data = """
             [librarian_server]
             launch: True
@@ -134,11 +135,7 @@
             launch: False
             """
         config.push('launch_data', launch_data)
-
-    def tearDown(self):
-        """Restore the default configuration."""
-        config.pop('launch_data')
-        unittest.TestCase.tearDown(self)
+        self.addCleanup(config.pop, 'launch_data')
 
     def test_nothing_explictly_requested(self):
         """Implicitly start services based on the config.*.launch property.
@@ -167,7 +164,3 @@
 
     def test_launchpad_systems_red(self):
         self.failIf(config.launchpad.launch)
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)