Merge lp:~stub/launchpad/disco into lp:launchpad

[Stuart Bishop]

= Summary =

Bump pgbouncer release to a version that should pass buildbot and revert r13865 reversion.

== Proposed fix ==

== Pre-implementation notes ==

== Implementation details ==

== Tests ==

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/testing/pgsql.py
  lib/canonical/launchpad/doc/canonical-config.txt
  lib/lp/testing/tests/test_fixture.py
  versions.cfg
  setup.py
  lib/lp/testing/__init__.py
  lib/lp/testing/tests/test_pgsql.py
  lib/lp/testing/fixture.py
  lib/lp/codehosting/tests/test_rewrite.py
  scripts/branch-rewrite.py
  configs/testrunner/launchpad-lazr.conf
  lib/canonical/config/tests/test_database_config.py

./lib/lp/testing/pgsql.py
      90: Line exceeds 78 characters.
     270: Line exceeds 78 characters.
     281: Line exceeds 78 characters.
     282: Line exceeds 78 characters.
     346: Line exceeds 78 characters.
     350: Line exceeds 78 characters.
      47: E261 at least two spaces before inline comment
      90: E501 line too long (80 characters)
     133: E302 expected 2 blank lines, found 0
     136: E302 expected 2 blank lines, found 1
     142: E302 expected 2 blank lines, found 1
     151: E261 at least two spaces before inline comment
     329: E225 missing whitespace around operator
     341: E225 missing whitespace around operator
     346: E501 line too long (88 characters)
     350: E501 line too long (83 characters)
     351: E225 missing whitespace around operator
     365: E261 at least two spaces before inline comment
./lib/canonical/launchpad/doc/canonical-config.txt
       1: narrative uses a moin header.
       9: narrative uses a moin header.
      85: narrative uses a moin header.
     105: narrative uses a moin header.
./lib/lp/testing/tests/test_pgsql.py
     105: E261 at least two spaces before inline comment
     180: E231 missing whitespace after ','
./scripts/branch-rewrite.py
      15: '_pythonpath' imported but unused
./configs/testrunner/launchpad-lazr.conf
     140: Line exceeds 78 characters.

[Stuart Bishop (stub)]

review: Approve

=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf	2011-09-05 06:30:08 +0000
+++ configs/testrunner/launchpad-lazr.conf	2011-09-05 16:02:25 +0000
@@ -45,12 +45,12 @@
 error_dir: /var/tmp/codehosting.test
 
 [database]
-rw_main_master: dbname=launchpad_ftest
-rw_main_slave:  dbname=launchpad_ftest
+rw_main_master: dbname=launchpad_ftest host=localhost
+rw_main_slave:  dbname=launchpad_ftest host=localhost
 # Use our _template databases here just so that we have different values from
 # the rw_* configs.
-ro_main_master: dbname=launchpad_ftest_template
-ro_main_slave:  dbname=launchpad_ftest_template
+ro_main_master: dbname=launchpad_ftest_template host=localhost
+ro_main_slave:  dbname=launchpad_ftest_template host=localhost
 randomise_select_results: true
 
 [error_reports]

=== modified file 'lib/canonical/config/tests/test_database_config.py'
--- lib/canonical/config/tests/test_database_config.py	2011-09-05 06:30:08 +0000
+++ lib/canonical/config/tests/test_database_config.py	2011-09-05 16:02:25 +0000
@@ -28,7 +28,8 @@
         self.assertEquals('librarian', config.librarian.dbuser)
 
         dbconfig.setConfigSection('librarian')
-        expected_db = 'dbname=%s' % DatabaseLayer._db_fixture.dbname
+        expected_db = (
+            'dbname=%s host=localhost' % DatabaseLayer._db_fixture.dbname)
         self.assertEquals(expected_db, dbconfig.rw_main_master)
         self.assertEquals('librarian', dbconfig.dbuser)
 

=== modified file 'lib/canonical/launchpad/doc/canonical-config.txt'
--- lib/canonical/launchpad/doc/canonical-config.txt	2011-09-05 06:30:08 +0000
+++ lib/canonical/launchpad/doc/canonical-config.txt	2011-09-05 16:02:25 +0000
@@ -15,7 +15,8 @@
 
     >>> from canonical.config import config
     >>> from canonical.testing.layers import DatabaseLayer
-    >>> expected = 'dbname=%s' % DatabaseLayer._db_fixture.dbname
+    >>> expected = (
+    ...     'dbname=%s host=localhost' % DatabaseLayer._db_fixture.dbname)
     >>> expected == config.database.rw_main_master
     True
     >>> config.database.db_statement_timeout is None

=== modified file 'lib/lp/codehosting/tests/test_rewrite.py'
--- lib/lp/codehosting/tests/test_rewrite.py	2011-09-05 06:30:08 +0000
+++ lib/lp/codehosting/tests/test_rewrite.py	2011-09-05 16:02:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the dynamic RewriteMap used to serve branches over HTTP."""
@@ -14,15 +14,21 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    DatabaseLayer,
+    )
 from lp.code.interfaces.codehosting import branch_id_alias
 from lp.codehosting.rewrite import BranchRewriter
 from lp.codehosting.vfs import branch_id_to_path
 from lp.services.log.logger import BufferLogger
 from lp.testing import (
     FakeTime,
+    nonblocking_readline,
+    TestCase,
     TestCaseWithFactory,
     )
+from lp.testing.fixture import PGBouncerFixture
 
 
 class TestBranchRewriter(TestCaseWithFactory):
@@ -177,7 +183,8 @@
         transaction.commit()
         rewriter.rewriteLine('/' + branch.unique_name + '/.bzr/README')
         rewriter.rewriteLine('/' + branch.unique_name + '/.bzr/README')
-        logging_output_lines = self.getLoggerOutput(rewriter).strip().split('\n')
+        logging_output_lines = self.getLoggerOutput(
+            rewriter).strip().split('\n')
         self.assertEqual(2, len(logging_output_lines))
         self.assertIsNot(
             None,
@@ -194,7 +201,8 @@
         self.fake_time.advance(
             config.codehosting.branch_rewrite_cache_lifetime + 1)
         rewriter.rewriteLine('/' + branch.unique_name + '/.bzr/README')
-        logging_output_lines = self.getLoggerOutput(rewriter).strip().split('\n')
+        logging_output_lines = self.getLoggerOutput(
+            rewriter).strip().split('\n')
         self.assertEqual(2, len(logging_output_lines))
         self.assertIsNot(
             None,
@@ -246,7 +254,8 @@
         # buffering, write a complete line of output.
         for input_line in input_lines:
             proc.stdin.write(input_line + '\n')
-            output_lines.append(proc.stdout.readline().rstrip('\n'))
+            output_lines.append(
+                nonblocking_readline(proc.stdout, 60).rstrip('\n'))
         # If we create a new branch after the branch-rewrite.py script has
         # connected to the database, or edit a branch name that has already
         # been rewritten, both are rewritten successfully.
@@ -260,17 +269,90 @@
             'file:///var/tmp/bazaar.launchpad.dev/mirrors/%s/.bzr/README'
             % branch_id_to_path(new_branch.id))
         proc.stdin.write(new_branch_input + '\n')
-        output_lines.append(proc.stdout.readline().rstrip('\n'))
+        output_lines.append(
+            nonblocking_readline(proc.stdout, 60).rstrip('\n'))
 
         edited_branch_input = '/%s/.bzr/README' % edited_branch.unique_name
         expected_lines.append(
             'file:///var/tmp/bazaar.launchpad.dev/mirrors/%s/.bzr/README'
             % branch_id_to_path(edited_branch.id))
         proc.stdin.write(edited_branch_input + '\n')
-        output_lines.append(proc.stdout.readline().rstrip('\n'))
+        output_lines.append(
+            nonblocking_readline(proc.stdout, 60).rstrip('\n'))
 
         os.kill(proc.pid, signal.SIGINT)
         err = proc.stderr.read()
         # The script produces logging output, but not to stderr.
         self.assertEqual('', err)
         self.assertEqual(expected_lines, output_lines)
+
+
+class TestBranchRewriterScriptHandlesDisconnects(TestCase):
+    """Ensure branch-rewrite.py survives fastdowntime deploys."""
+    layer = DatabaseLayer
+
+    def spawn(self):
+        script_file = os.path.join(
+            config.root, 'scripts', 'branch-rewrite.py')
+
+        self.rewriter_proc = subprocess.Popen(
+            [script_file], stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE, bufsize=0)
+
+        self.addCleanup(self.rewriter_proc.terminate)
+
+    def request(self, query):
+        self.rewriter_proc.stdin.write(query + '\n')
+        self.rewriter_proc.stdin.flush()
+
+        # 60 second timeout as we might need to wait for the script to
+        # finish starting up.
+        result = nonblocking_readline(self.rewriter_proc.stdout, 60)
+
+        if result.endswith('\n'):
+            return result[:-1]
+        self.fail(
+            "Incomplete line or no result retrieved from subprocess: %s"
+            % repr(result.getvalue()))
+
+    def test_reconnects_when_disconnected(self):
+        pgbouncer = self.useFixture(PGBouncerFixture())
+
+        self.spawn()
+
+        # Everything should be working, and we get valid output.
+        out = self.request('foo')
+        self.assertEndsWith(out, '/foo')
+
+        pgbouncer.stop()
+
+        # Now with pgbouncer down, we should get NULL messages and
+        # stderr spam, and this keeps happening. We test more than
+        # once to ensure that we will keep trying to reconnect even
+        # after several failures.
+        for count in range(5):
+            out = self.request('foo')
+            self.assertEqual(out, 'NULL')
+
+        pgbouncer.start()
+
+        # Everything should be working, and we get valid output.
+        out = self.request('foo')
+        self.assertEndsWith(out, '/foo')
+
+    def test_starts_with_db_down(self):
+        pgbouncer = self.useFixture(PGBouncerFixture())
+
+        # Start with the database down.
+        pgbouncer.stop()
+
+        self.spawn()
+
+        for count in range(5):
+            out = self.request('foo')
+            self.assertEqual(out, 'NULL')
+
+        pgbouncer.start()
+
+        out = self.request('foo')
+        self.assertEndsWith(out, '/foo')

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-09-05 06:30:08 +0000
+++ lib/lp/testing/__init__.py	2011-09-05 16:02:25 +0000
@@ -29,6 +29,7 @@
     'logout',
     'map_branch_contents',
     'normalize_whitespace',
+    'nonblocking_readline',
     'oauth_access_token_for',
     'person_logged_in',
     'quote_jquery_expression',
@@ -69,6 +70,7 @@
 import os
 from pprint import pformat
 import re
+from select import select
 import shutil
 import subprocess
 import sys
@@ -1325,3 +1327,25 @@
 def extract_lp_cache(text):
     match = re.search(r'<script>LP.cache = (\{.*\});</script>', text)
     return simplejson.loads(match.group(1))
+
+
+def nonblocking_readline(instream, timeout):
+    """Non-blocking readline.
+
+    Files must provide a valid fileno() method. This is a test helper
+    as it is inefficient and unlikely useful for production code.
+    """
+    result = StringIO()
+    start = now = time.time()
+    deadline = start + timeout
+    while (now < deadline and not result.getvalue().endswith('\n')):
+        rlist = select([instream], [], [], deadline - now)
+        if rlist:
+            # Reading 1 character at a time is inefficient, but means
+            # we don't need to implement put-back.
+            next_char = os.read(instream.fileno(), 1)
+            if next_char == "":
+                break  # EOF
+            result.write(next_char)
+        now = time.time()
+    return result.getvalue()

=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py	2011-09-05 06:30:08 +0000
+++ lib/lp/testing/fixture.py	2011-09-05 16:02:25 +0000
@@ -11,9 +11,15 @@
     'ZopeViewReplacementFixture',
     ]
 
+from ConfigParser import SafeConfigParser
+import os.path
 from textwrap import dedent
 
-from fixtures import Fixture
+from fixtures import (
+    EnvironmentVariableFixture,
+    Fixture,
+    )
+import pgbouncer.fixture
 import rabbitfixture.server
 from zope.component import (
     getGlobalSiteManager,
@@ -27,6 +33,8 @@
     undefineChecker,
     )
 
+from canonical.config import config
+
 
 class RabbitServer(rabbitfixture.server.RabbitServer):
     """A RabbitMQ server fixture with Launchpad-specific config.
@@ -46,6 +54,68 @@
             """ % self.config.port)
 
 
+class PGBouncerFixture(pgbouncer.fixture.PGBouncerFixture):
+    """Inserts a controllable pgbouncer instance in front of PostgreSQL.
+
+    The pgbouncer proxy can be shutdown and restarted at will, simulating
+    database outages and fastdowntime deployments.
+    """
+
+    def __init__(self):
+        super(PGBouncerFixture, self).__init__()
+
+        # Known databases
+        from canonical.testing.layers import DatabaseLayer
+        dbnames = [
+            DatabaseLayer._db_fixture.dbname,
+            DatabaseLayer._db_template_fixture.dbname,
+            'session_ftest',
+            'launchpad_empty',
+            ]
+        for dbname in dbnames:
+            self.databases[dbname] = 'dbname=%s port=5432 host=localhost' % (
+                dbname,)
+
+        # Known users, pulled from security.cfg
+        security_cfg_path = os.path.join(
+            config.root, 'database', 'schema', 'security.cfg')
+        security_cfg_config = SafeConfigParser({})
+        security_cfg_config.read([security_cfg_path])
+        for section_name in security_cfg_config.sections():
+            self.users[section_name] = 'trusted'
+            self.users[section_name + '_ro'] = 'trusted'
+        self.users[os.environ['USER']] = 'trusted'
+
+    def setUp(self):
+        super(PGBouncerFixture, self).setUp()
+
+        # reconnect_store cleanup added first so it is run last, after
+        # the environment variables have been reset.
+        self.addCleanup(self._maybe_reconnect_stores)
+
+        # Abuse the PGPORT environment variable to get things connecting
+        # via pgbouncer. Otherwise, we would need to temporarily
+        # overwrite the database connection strings in the config.
+        self.useFixture(EnvironmentVariableFixture('PGPORT', str(self.port)))
+
+        # Reset database connections so they go through pgbouncer.
+        self._maybe_reconnect_stores()
+
+    def _maybe_reconnect_stores(self):
+        """Force Storm Stores to reconnect if they are registered.
+
+        This is a noop if the Component Architecture is not loaded,
+        as we are using a test layer that doesn't provide database
+        connections.
+        """
+        from canonical.testing.layers import (
+            reconnect_stores,
+            is_ca_available,
+            )
+        if is_ca_available():
+            reconnect_stores()
+
+
 class ZopeAdapterFixture(Fixture):
     """A fixture to register and unregister an adapter."""
 

=== modified file 'lib/lp/testing/pgsql.py'
--- lib/lp/testing/pgsql.py	2011-09-05 06:30:08 +0000
+++ lib/lp/testing/pgsql.py	2011-09-05 16:02:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 '''
@@ -41,7 +41,10 @@
     def close(self):
         if self in PgTestSetup.connections:
             PgTestSetup.connections.remove(self)
-            self.real_connection.close()
+            try:
+                self.real_connection.close()
+            except psycopg2.InterfaceError:
+                pass # Already closed, killed etc. Ignore.
 
     def rollback(self, InterfaceError=psycopg2.InterfaceError):
         # In our test suites, rollback ends up being called twice in some
@@ -186,8 +189,8 @@
                 # available.
                 # Avoid circular imports
                 section = """[database]
-rw_main_master: dbname=%s
-rw_main_slave:  dbname=%s
+rw_main_master: dbname=%s host=localhost
+rw_main_slave:  dbname=%s host=localhost
 
 """ % (self.dbname, self.dbname)
                 if BaseLayer.config_fixture is not None:

=== modified file 'lib/lp/testing/tests/test_fixture.py'
--- lib/lp/testing/tests/test_fixture.py	2011-09-05 06:30:08 +0000
+++ lib/lp/testing/tests/test_fixture.py	2011-09-05 16:02:25 +0000
@@ -8,6 +8,8 @@
 from textwrap import dedent
 
 from fixtures import EnvironmentVariableFixture
+import psycopg2
+from storm.exceptions import DisconnectionError
 from zope.component import (
     adapts,
     queryAdapter,
@@ -17,9 +19,17 @@
     Interface,
     )
 
-from canonical.testing.layers import BaseLayer
+from canonical.config import dbconfig
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
+from canonical.testing.layers import (
+    BaseLayer,
+    DatabaseLayer,
+    LaunchpadZopelessLayer,
+    )
+from lp.registry.model.person import Person
 from lp.testing import TestCase
 from lp.testing.fixture import (
+    PGBouncerFixture,
     RabbitServer,
     ZopeAdapterFixture,
     )
@@ -89,3 +99,109 @@
             self.assertIsInstance(adapter, FooToBar)
         # The adapter is no longer registered.
         self.assertIs(None, queryAdapter(context, IBar))
+
+
+class TestPGBouncerFixtureWithCA(TestCase):
+    """PGBouncerFixture reconnect tests for Component Architecture layers.
+
+    Registered Storm Stores should be reconnected through pgbouncer.
+    """
+    layer = LaunchpadZopelessLayer
+
+    def is_connected(self):
+        # First rollback any existing transaction to ensure we attempt
+        # to reconnect. We currently rollback the store explicitely
+        # rather than call transaction.abort() due to Bug #819282.
+        store = IMasterStore(Person)
+        store.rollback()
+
+        try:
+            store.find(Person).first()
+            return True
+        except DisconnectionError:
+            return False
+
+    def test_stop_and_start(self):
+        # Database is working.
+        assert self.is_connected()
+
+        # And database with the fixture is working too.
+        pgbouncer = PGBouncerFixture()
+        with PGBouncerFixture() as pgbouncer:
+            assert self.is_connected()
+
+            # pgbouncer is transparant. To confirm we are connecting via
+            # pgbouncer, we need to shut it down and confirm our
+            # connections are dropped.
+            pgbouncer.stop()
+            assert not self.is_connected()
+
+            # If we restart it, things should be back to normal.
+            pgbouncer.start()
+            assert self.is_connected()
+
+        # Database is still working.
+        assert self.is_connected()
+
+    def test_stop_no_start(self):
+        # Database is working.
+        assert self.is_connected()
+
+        # And database with the fixture is working too.
+        with PGBouncerFixture() as pgbouncer:
+            assert self.is_connected()
+
+            # pgbouncer is transparant. To confirm we are connecting via
+            # pgbouncer, we need to shut it down and confirm our
+            # connections are dropped.
+            pgbouncer.stop()
+            assert not self.is_connected()
+
+        # Database is working again.
+        assert self.is_connected()
+
+
+class TestPGBouncerFixtureWithoutCA(TestCase):
+    """PGBouncerFixture tests for non-Component Architecture layers."""
+    layer = DatabaseLayer
+
+    def is_db_available(self):
+        # Direct connection to the DB.
+        con_str = dbconfig.rw_main_master + ' user=launchpad_main'
+        try:
+            con = psycopg2.connect(con_str)
+            cur = con.cursor()
+            cur.execute("SELECT id FROM Person LIMIT 1")
+            con.close()
+            return True
+        except psycopg2.OperationalError:
+            return False
+
+    def test_install_fixture(self):
+        self.assert_(self.is_db_available())
+
+        with PGBouncerFixture() as pgbouncer:
+            self.assertTrue(self.is_db_available())
+
+            pgbouncer.stop()
+            self.assertFalse(self.is_db_available())
+
+        # This confirms that we are again connecting directly to the
+        # database, as the pgbouncer process was shutdown.
+        self.assertTrue(self.is_db_available())
+
+    def test_install_fixture_with_restart(self):
+        self.assert_(self.is_db_available())
+
+        with PGBouncerFixture() as pgbouncer:
+            self.assertTrue(self.is_db_available())
+
+            pgbouncer.stop()
+            self.assertFalse(self.is_db_available())
+
+            pgbouncer.start()
+            self.assertTrue(self.is_db_available())
+
+        # Note that because pgbouncer was left running, we can't confirm
+        # that we are now connecting directly to the database.
+        self.assertTrue(self.is_db_available())

=== modified file 'lib/lp/testing/tests/test_pgsql.py'
--- lib/lp/testing/tests/test_pgsql.py	2011-09-05 06:30:08 +0000
+++ lib/lp/testing/tests/test_pgsql.py	2011-09-05 16:02:25 +0000
@@ -53,7 +53,7 @@
         fixture.setUp()
         self.addCleanup(fixture.dropDb)
         self.addCleanup(fixture.tearDown)
-        expected_value = 'dbname=%s' % fixture.dbname
+        expected_value = 'dbname=%s host=localhost' % fixture.dbname
         self.assertEqual(expected_value, dbconfig.rw_main_master)
         self.assertEqual(expected_value, dbconfig.rw_main_slave)
         with ConfigUseFixture(BaseLayer.appserver_config_name):

=== modified file 'scripts/branch-rewrite.py'
--- scripts/branch-rewrite.py	2011-09-05 06:30:08 +0000
+++ scripts/branch-rewrite.py	2011-09-05 16:02:25 +0000
@@ -1,6 +1,6 @@
 #!/usr/bin/python -uS
 #
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=W0403
@@ -19,6 +19,8 @@
 
 from canonical.database.sqlbase import ISOLATION_LEVEL_AUTOCOMMIT
 from canonical.config import config
+from canonical.launchpad.interfaces.lpstorm import ISlaveStore
+from lp.code.model.branch import Branch
 from lp.codehosting.rewrite import BranchRewriter
 from lp.services.log.loglevels import INFO, WARNING
 from lp.services.scripts.base import LaunchpadScript
@@ -60,9 +62,19 @@
                     return
             except KeyboardInterrupt:
                 sys.exit()
-            except:
+            except Exception:
                 self.logger.exception('Exception occurred:')
                 print "NULL"
+                # The exception might have been a DisconnectionError or
+                # similar. Cleanup such as database reconnection will
+                # not happen until the transaction is rolled back.
+                # XXX StuartBishop 2011-08-31 bug=819282: We are
+                # explicitly rolling back the store here as a workaround
+                # instead of using transaction.abort()
+                try:
+                    ISlaveStore(Branch).rollback()
+                except Exception:
+                    self.logger.exception('Exception occurred in rollback:')
 
 
 if __name__ == '__main__':

=== modified file 'setup.py'
--- setup.py	2011-09-05 06:30:08 +0000
+++ setup.py	2011-09-05 16:02:25 +0000
@@ -60,6 +60,7 @@
         'oops_datedir_repo',
         'oops_wsgi',
         'paramiko',
+        'pgbouncer',
         'psycopg2',
         'python-memcached',
         'pyasn1',

=== modified file 'versions.cfg'
--- versions.cfg	2011-09-05 06:30:08 +0000
+++ versions.cfg	2011-09-05 16:02:25 +0000
@@ -55,6 +55,7 @@
 paramiko = 1.7.4
 Paste = 1.7.2
 PasteDeploy = 1.3.3
+pgbouncer = 0.0.3
 plone.recipe.command = 1.1
 psycopg2 = 2.2.2
 pyasn1 = 0.0.9a