launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/python-pgbouncer/reliable-shutdown into lp:python-pgbouncer.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/python-pgbouncer/reliable-shutdown/+merge/80382

While working on lp:~allenap/storm/oneiric-admin-shutdown-bug-871596
I've had problems with the pgbouncer not being quite shutdown right
after calling stop(). This branch changes the way that pgbouncer is
started so that we can be confident that we have stopped it later on.

List of changes:

- Does not start pgbouncer with -d (--daemon) so that the Popen object
  can be used to directly monitor and control the process. Previously
  it was done at one remove, reading in the daemon's pid from a pid
  file.

- Raise an exception if pgbouncer does not stop with 5 seconds of
  calling stop(). Previously it would wait for pgbouncer to stop but
  then do nothing.

- Adds the pgbouncer output (from stdout and stderr) as a fixture
  detail. This means that tests that use the fixture can get
  interesting debug information when the test fails.

- Fixes the tests so that they are always loaded into an
  OptimizingTestSuite. If the tests were run as suggested in the
  README they were being loaded into a standard TestSuite.

- Refactors the tests so that they're a lot shorter and a bit easier
  to see what they're trying to demonstrate.

- Update the README.

-- 
https://code.launchpad.net/~allenap/python-pgbouncer/reliable-shutdown/+merge/80382
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/python-pgbouncer/reliable-shutdown into lp:python-pgbouncer.

=== modified file '.bzrignore'
--- .bzrignore	2011-07-18 03:31:27 +0000
+++ .bzrignore	2011-10-25 20:14:23 +0000
@@ -6,3 +6,5 @@
 ./parts
 ./eggs
 ./download-cache
+TAGS
+tags

=== modified file 'README'
--- README	2011-09-05 14:35:29 +0000
+++ README	2011-10-25 20:14:23 +0000
@@ -30,12 +30,18 @@
 * python-fixtures (https://launchpad.net/python-fixtures or
   http://pypi.python.org/pypi/fixtures)
 
+* testtools (http://pypi.python.org/pypi/testtools)
+
 Testing Dependencies
 ====================
 
+In addition to the above, the tests also depend on:
+
+* psycopg2 (http://pypi.python.org/pypi/psycopg2)
+
 * subunit (http://pypi.python.org/pypi/python-subunit) (optional)
 
-* testtools (http://pypi.python.org/pypi/testtools)
+* testresources (http://pypi.python.org/pypi/testresources)
 
 * van.pg (http://pypi.python.org/pypi/van.pg)
 
@@ -75,4 +81,10 @@
 immediately available, you can use ./bootstrap.py to create bin/buildout, then
 bin/py to get a python interpreter with the dependencies available.
 
-To run the tests, run 'bin/py pgbouncer/tests.py'.
+To run the tests, run either:
+
+ $ bin/py pgbouncer/tests.py
+
+or:
+
+ $ bin/py -m testtools.run pgbouncer.tests.test_suite

=== modified file 'pgbouncer/fixture.py'
--- pgbouncer/fixture.py	2011-09-12 23:38:28 +0000
+++ pgbouncer/fixture.py	2011-10-25 20:14:23 +0000
@@ -20,11 +20,12 @@
 
 import os.path
 import socket
-import signal
 import subprocess
 import time
 
 from fixtures import Fixture, TempDir
+from testtools.content import content_from_file
+
 
 def _allocate_ports(n=1):
     """Allocate `n` unused ports.
@@ -76,7 +77,6 @@
         self.port = _allocate_ports()[0]
         self.configdir = self.useFixture(TempDir())
         self.auth_type = 'trust'
-        self.process_pid = None
         self.setUpConf()
         self.start()
 
@@ -111,46 +111,56 @@
                 authfile.write('"%s" "%s"\n' % user_creds)
 
     def stop(self):
-        if self.process_pid is None:
-            return
-        os.kill(self.process_pid, signal.SIGTERM)
-        # Wait for the shutdown to occur
+        if self.process is None:
+            # pgbouncer has not been started.
+            return
+        if self.process.poll() is not None:
+            # pgbouncer has exited already.
+            return
+        # Terminate and wait.
+        self.process.terminate()
         start = time.time()
         stop = start + 5.0
         while time.time() < stop:
-            if not os.path.isfile(self.pidpath):
-                self.process_pid = None
-                return
-        # If its not going away, we might want to raise an error, but for now
-        # it seems reliable.
+            if self.process.poll() is None:
+                time.sleep(0.1)
+            else:
+                break
+        else:
+            raise Exception(
+                'timeout waiting for pgbouncer to exit')
 
     def start(self):
         self.addCleanup(self.stop)
 
         # Add /usr/sbin if necessary to the PATH for magic just-works
         # behavior with Ubuntu.
-        env = None
+        env = os.environ.copy()
         if not self.pgbouncer.startswith('/'):
-            path = os.environ['PATH'].split(':')
+            path = env['PATH'].split(os.pathsep)
             if '/usr/sbin' not in path:
                 path.append('/usr/sbin')
-                env = os.environ.copy()
-                env['PATH'] = ':'.join(path)
-
-        outputfile = open(self.outputpath, 'wt')
-        self.process = subprocess.Popen(
-            [self.pgbouncer, '-d', self.inipath], env=env,
-            stdout=outputfile.fileno(), stderr=outputfile.fileno())
-        self.process.communicate()
-        # Wait up to 5 seconds for the pid file to exist
+                env['PATH'] = os.pathsep.join(path)
+
+        with open(self.outputpath, "wb") as outputfile:
+            with open(os.devnull, "rb") as devnull:
+                self.process = subprocess.Popen(
+                    [self.pgbouncer, self.inipath], env=env, stdin=devnull,
+                    stdout=outputfile, stderr=outputfile)
+
+        self.addDetail(
+            os.path.basename(self.outputpath),
+            content_from_file(self.outputpath))
+
+        # Wait up to 5 seconds for the pid file to exist.
         start = time.time()
         stop = start + 5.0
         while time.time() < stop:
             if os.path.isfile(self.pidpath):
-                try:
-                    self.process_pid = int(file(self.pidpath, 'rt').read())
-                except ValueError:
-                    # Empty pid files -> ValueError.
-                    continue
-                return
-        raise Exception('timeout waiting for pgbouncer to create pid file')
+                with open(self.pidpath, "rb") as pidfile:
+                    if pidfile.read().strip().isdigit():
+                        break
+            time.sleep(0.1)
+        else:
+            raise Exception(
+                'timeout waiting for pgbouncer to create pid file')

=== modified file 'pgbouncer/tests.py'
--- pgbouncer/tests.py	2011-09-05 15:01:52 +0000
+++ pgbouncer/tests.py	2011-10-25 20:14:23 +0000
@@ -15,7 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-from unittest import main, TestLoader
+import unittest
 
 import fixtures
 import psycopg2
@@ -28,10 +28,12 @@
 # A 'just-works' workaround for Ubuntu not exposing initdb to the main PATH.
 os.environ['PATH'] = os.environ['PATH'] + ':/usr/lib/postgresql/8.4/bin'
 
+
+test_loader = testresources.TestLoader()
+
+
 def test_suite():
-    result = testresources.OptimisingTestSuite()
-    result.addTest(TestLoader().loadTestsFromName(__name__))
-    return result
+    return test_loader.loadTestsFromName(__name__)
 
 
 class ResourcedTestCase(testtools.TestCase, testresources.ResourcedTestCase):
@@ -49,15 +51,20 @@
 
     resources = [('db', DatabaseManager(initialize_sql=setup_user))]
 
+    def setUp(self):
+        super(TestFixture, self).setUp()
+        self.bouncer = PGBouncerFixture()
+        self.bouncer.databases[self.db.database] = 'host=' + self.db.host
+        self.bouncer.users['user1'] = ''
+
+    def connect(self):
+        return psycopg2.connect(
+            host=self.bouncer.host, port=self.bouncer.port,
+            database=self.db.database, user='user1')
+
     def test_dynamic_port_allocation(self):
-        bouncer = PGBouncerFixture()
-        db = self.db
-        bouncer.databases[db.database] = 'host=%s' % (db.host,)
-        bouncer.users['user1'] = ''
-        with bouncer:
-            conn = psycopg2.connect(host=bouncer.host, port=bouncer.port,
-                user='user1', database=db.database)
-            conn.close()
+        self.useFixture(self.bouncer)
+        self.connect().close()
 
     def test_stop_start_facility(self):
         # Once setup the fixture can be stopped, and started again, retaining
@@ -65,36 +72,20 @@
         # potentially be used by a different process, so this isn't perfect,
         # but its pretty reliable as a test helper, and manual port allocation
         # outside the dynamic range should be fine.
-        bouncer = PGBouncerFixture()
-        db = self.db
-        bouncer.databases[db.database] = 'host=%s' % (db.host,)
-        bouncer.users['user1'] = ''
-        def check_connect():
-            conn = psycopg2.connect(host=bouncer.host, port=bouncer.port,
-                user='user1', database=db.database)
-            conn.close()
-        with bouncer:
-            current_port = bouncer.port
-            bouncer.stop()
-            self.assertRaises(psycopg2.OperationalError, check_connect)
-            bouncer.start()
-            check_connect()
+        self.useFixture(self.bouncer)
+        self.bouncer.stop()
+        self.assertRaises(psycopg2.OperationalError, self.connect)
+        self.bouncer.start()
+        self.connect().close()
 
     def test_unix_sockets(self):
-        db = self.db
         unix_socket_dir = self.useFixture(fixtures.TempDir()).path
-        bouncer = PGBouncerFixture()
-        bouncer.databases[db.database] = 'host=%s' % (db.host,)
-        bouncer.users['user1'] = ''
-        bouncer.unix_socket_dir = unix_socket_dir
-        with bouncer:
-            # Connect to pgbouncer via a Unix domain socket. We don't
-            # care how pgbouncer connects to PostgreSQL.
-            conn = psycopg2.connect(
-                host=unix_socket_dir, user='user1',
-                database=db.database, port=bouncer.port)
-            conn.close()
+        self.bouncer.unix_socket_dir = unix_socket_dir
+        self.useFixture(self.bouncer)
+        # Connect to pgbouncer via a Unix domain socket. We don't
+        # care how pgbouncer connects to PostgreSQL.
+        self.connect().close()
 
 
 if __name__ == "__main__":
-    main(testLoader=TestLoader())
+    unittest.main(testLoader=test_loader)