Merge lp:~gary/launchpad/bug-687951 into lp:launchpad

[Gary Poster]

This is an expedient solution to the problem described in bug 687951--or put another way, it is a good solution to the buildbot and operational problem, while not addressing the underlying Launchpad problem. I will create another bug for the Launchpad problem, which Robert addressed in the IRC conversation I quoted in that bug's comments.

The test_on_merge.py is operational code for which we do not have tests. To demonstrate the way this patch works, do the following.

1) See if your machine already has some of the problematic old databases. Run psql -l and you may see database names of the form launchpad_ftest_NUMBER, like lines 10 through 43 of https://pastebin.canonical.com/40714/ . If you don't have any, you can create some for the test using psql. "create database launchpad_ftest_30322;" will give you one to work with.

2) run test_on_merge.py, with some argument to limit the tests run. For instance, "./test_on_merge.py -t foobar" will not run any tests. There's probably a nicer way to do this, but I'm not worrying about it right now. When you do this, the first one or more lines will look like this: "Dropped old ftest database launchpad_ftest_30322. This should not happen. See bug 687951." After test_on_merge.py runs, psql -l will show you that the databases have been dropped.

[Robert Collins (lifeless)]

As this stands it will break parallel testing via testr :(

On 21/12/2010 12:18 PM, "Gary Poster" <email address hidden> wrote:

Gary Poster has proposed merging lp:~gary/launchpad/bug-687951 into
lp:launchpad.

Requested reviews:
 Launchpad code reviewers (launchpad-reviewers)
Related bugs:
 #687951 Large number of DBs left around after test suite runs in buildbot
 https://bugs.launchpad.net/bugs/687951

This is an expedient solution to the problem described in bug 687951--or put
another way, it is a good solution to the buildbot and operational problem,
while not addressing the underlying Launchpad problem. I will create
another bug for the Launchpad problem, which Robert addressed in the IRC
conversation I quoted in that bug's comments.

The test_on_merge.py is operational code for which we do not have tests. To
demonstrate the way this patch works, do the following.

1) See if your machine already has some of the problematic old databases.
 Run psql -l and you may see database names of the form
launchpad_ftest_NUMBER, like lines 10 through 43 of
https://pastebin.canonical.com/40714/ . If you don't have any, you can
create some for the test using psql. "create database
launchpad_ftest_30322;" will give you one to work with.

2) run test_on_merge.py, with some argument to limit the tests run. For
instance, "./test_on_merge.py -t foobar" will not run any tests. There's
probably a nicer way to do this, but I'm not worrying about it right now.
 When you do this, the first one or more lines will look like this: "Dropped
old ftest database launchpad_ftest_30322. This should not happen. See bug
687951." After test_on_merge.py runs, psql -l will show you that the
databases have been dropped.
--
https://code.launchpad.net/~gary/launchpad/bug-687951/+merge/44300
Your team Launchpad code reviewers from Canonical is subscribed to branch
lp:launchpad.

=== modified file 'test_on_merge.py'
--- test_on_merge.py 2010-09-28 12:37:55 +0000
+++ test_on_merge.py 2010-12-20 23:18:07 +0000
@@ -8,6 +8,7 @@

 import sys, time
 import os, errno
+from psycopg2.extensions import ISOLATION_LEVEL_AUTOCOMMIT
 import tabnanny
 from StringIO import StringIO
 import psycopg2
@@ -34,6 +35,7 @@

    Returns 1 on error, otherwise it returns the testrunner's exit code.
    """
+ cleanup_old_ftest_databases() # See bug 687951.
    if setup_test_database() != 0:
        return 1

@@ -224,6 +226,32 @@
    return rv

+def cleanup_old_ftest_databases():
+ """Kill old ftest databases."""
+ conn = psycopg2.connect('dbname=template1')
+ conn.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
+ cur = conn.cursor()
+ try:
+ cur.execute(r"""
+ SELECT d.datname
+ FROM pg_catalog.pg_database d
+ WHERE d.datname ~ '^launchpad_ftest_[0-9]+$';""")
+ old_databases = cur.fetchall()
+ for (database_name,) in old_databases:
+ print ("Dropped old ftest database %s. "
+ "This should not happen. See bug 687951.") % (
+ database_name,)
+ # We are not doing db-level parameterization because it does
not work
+ # (is not designed for) schema e...

[Robert Collins (lifeless)]

Wow, android handle mail reply is terrible. Sorry about that.

Anyhow, while expedient, this will regress efforts to make the test environment sane.

buildbot is fixable when the alerts flash, and we /need/ to track down and fix the causes of these leaks otherwise more serious parallel test runs will be very hard to execute.

This stopgap will actively prevent us fixing this: the cure would be worse than the disease.

review: Disapprove

[Gary Poster (gary)]

Unfortunately, I suspect that I don't have time or resources to address the root cause between now and the end of the year. I will check with mthaddon as to whether this is an operational issue that needs a stopgap for the rest of 2010.

[Gary Poster (gary)]

mthaddon reports that addressing this problem can be postponed to 2011 as long as it has a high priority then. Therefore, I'm moving on.

Unmerged revisions

12120. By Gary Poster on 2010-12-20

This is an expedient solution to the immediate problem of buildbot falling over.

=== modified file 'test_on_merge.py'
--- test_on_merge.py	2010-09-28 12:37:55 +0000
+++ test_on_merge.py	2010-12-20 23:18:07 +0000
@@ -8,6 +8,7 @@
 
 import sys, time
 import os, errno
+from psycopg2.extensions import ISOLATION_LEVEL_AUTOCOMMIT
 import tabnanny
 from StringIO import StringIO
 import psycopg2
@@ -34,6 +35,7 @@
 
     Returns 1 on error, otherwise it returns the testrunner's exit code.
     """
+    cleanup_old_ftest_databases() # See bug 687951.
     if setup_test_database() != 0:
         return 1
 
@@ -224,6 +226,32 @@
     return rv
 
 
+def cleanup_old_ftest_databases():
+    """Kill old ftest databases."""
+    conn = psycopg2.connect('dbname=template1')
+    conn.set_isolation_level(ISOLATION_LEVEL_AUTOCOMMIT)
+    cur = conn.cursor()
+    try:
+        cur.execute(r"""
+            SELECT d.datname
+            FROM pg_catalog.pg_database d
+            WHERE d.datname ~ '^launchpad_ftest_[0-9]+$';""")
+        old_databases = cur.fetchall()
+        for (database_name,) in old_databases:
+            print ("Dropped old ftest database %s.  "
+                   "This should not happen.  See bug 687951.") % (
+                   database_name,)
+            # We are not doing db-level parameterization because it does not work
+            # (is not designed for) schema entities.
+            cur.execute(r"""
+                DROP DATABASE %s;""" % (database_name,))
+    finally:
+        cur.close()
+        del cur
+        conn.close()
+        del conn
+
+
 def cleanup_hung_testrunner(process):
     """Kill and clean up the testrunner process and its children."""
     print