Merge lp:~allenap/maas/database-locks-revisited--1.5 into lp:maas/1.5

[Gavin Panella]

Backport of trunk r2395: Tighten up function and use of DatabaseLock.

Previously Django's aggressive connection closing policy would prevent session-level locks from being maintained.

[Gavin Panella (allenap)]

review: Approve

=== modified file 'src/maasserver/rpc/regionservice.py'
--- src/maasserver/rpc/regionservice.py	2014-04-01 17:26:20 +0000
+++ src/maasserver/rpc/regionservice.py	2014-06-02 20:22:06 +0000
@@ -325,8 +325,8 @@
 
     @synchronous
     @synchronised(lock)
+    @transactional
     @synchronised(locks.eventloop)
-    @transactional
     def prepare(self):
         """Ensure that the ``eventloops`` table exists.
 

=== modified file 'src/maasserver/rpc/tests/test_regionservice.py'
--- src/maasserver/rpc/tests/test_regionservice.py	2014-04-15 21:04:34 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2014-06-02 20:22:06 +0000
@@ -49,6 +49,7 @@
     common,
     exceptions,
     )
+from provisioningserver.rpc.interfaces import IConnection
 from provisioningserver.rpc.region import (
     Identify,
     ReportBootImages,
@@ -82,6 +83,7 @@
 from twisted.internet.threads import deferToThread
 from twisted.protocols import amp
 from twisted.python import log
+from zope.interface.verify import verifyObject
 
 
 class TestRegionProtocol_Identify(MAASTestCase):
@@ -195,10 +197,6 @@
         return d.addCallback(check)
 
 
-from provisioningserver.rpc.interfaces import IConnection
-from zope.interface.verify import verifyObject
-
-
 class TestRegionServer(MAASServerTestCase):
 
     def test_interfaces(self):

=== modified file 'src/maasserver/utils/dblocks.py'
--- src/maasserver/utils/dblocks.py	2014-04-01 17:23:19 +0000
+++ src/maasserver/utils/dblocks.py	2014-06-02 20:22:06 +0000
@@ -14,6 +14,8 @@
 __metaclass__ = type
 __all__ = [
     "DatabaseLock",
+    "DatabaseLockAttemptOutsideTransaction",
+    "DatabaseLockNotHeld",
 ]
 
 from contextlib import closing
@@ -26,6 +28,19 @@
 classid = 20120116
 
 
+class DatabaseLockAttemptOutsideTransaction(Exception):
+    """A locking attempt was made outside of a transaction.
+
+    :class:`DatabaseLock` should only be used within a transaction.
+    Django agressively closes connections outside of atomic blocks to
+    the extent that session-level locks are rendered unreliable at best.
+    """
+
+
+class DatabaseLockNotHeld(Exception):
+    """A particular lock was not held."""
+
+
 class DatabaseLock(tuple):
     """An advisory lock held in the database.
 
@@ -58,12 +73,16 @@
         return super(cls, DatabaseLock).__new__(cls, (classid, objid))
 
     def __enter__(self):
+        if not connection.in_atomic_block:
+            raise DatabaseLockAttemptOutsideTransaction(self)
         with closing(connection.cursor()) as cursor:
             cursor.execute("SELECT pg_advisory_lock(%s, %s)", self)
 
     def __exit__(self, *exc_info):
         with closing(connection.cursor()) as cursor:
             cursor.execute("SELECT pg_advisory_unlock(%s, %s)", self)
+            if cursor.fetchone() != (True,):
+                raise DatabaseLockNotHeld(self)
 
     def __repr__(self):
         return b"<%s classid=%d objid=%d>" % (
@@ -71,8 +90,18 @@
 
     def is_locked(self):
         stmt = (
-            "SELECT 1 FROM pg_locks"
-            " WHERE classid = %s AND objid = %s AND granted"
+            "SELECT 1 FROM pg_locks, pg_database"
+            " WHERE pg_locks.locktype = 'advisory'"
+            "   AND pg_locks.classid = %s"
+            "   AND pg_locks.objid = %s"
+            # objsubid is 2 when using the 2-argument version of the
+            # pg_advisory_* locking functions.
+            "   AND pg_locks.objsubid = 2"
+            "   AND pg_locks.granted"
+            # Advisory locks are local to each database so we join to
+            # pg_databases to discover the OID of the currrent database.
+            "   AND pg_locks.database = pg_database.oid"
+            "   AND pg_database.datname = current_database()"
         )
         with closing(connection.cursor()) as cursor:
             cursor.execute(stmt, self)

=== modified file 'src/maasserver/utils/tests/test_dblocks.py'
--- src/maasserver/utils/tests/test_dblocks.py	2014-03-24 10:43:54 +0000
+++ src/maasserver/utils/tests/test_dblocks.py	2014-06-02 20:22:06 +0000
@@ -16,7 +16,10 @@
 
 from contextlib import closing
 
-from django.db import connection
+from django.db import (
+    connection,
+    transaction,
+    )
 from maasserver.utils import dblocks
 from maastesting.testcase import MAASTestCase
 
@@ -40,6 +43,7 @@
         lock = dblocks.DatabaseLock(self.getUniqueInteger())
         self.assertEqual(lock, (lock.classid, lock.objid))
 
+    @transaction.atomic
     def test_lock_actually_locked(self):
         objid = self.getUniqueInteger()
         lock = dblocks.DatabaseLock(objid)
@@ -55,6 +59,7 @@
         locks_released = locks_held - locks_held_after
         self.assertEqual({objid}, locks_released)
 
+    @transaction.atomic
     def test_is_locked(self):
         objid = self.getUniqueInteger()
         lock = dblocks.DatabaseLock(objid)
@@ -64,6 +69,18 @@
             self.assertTrue(lock.is_locked())
         self.assertFalse(lock.is_locked())
 
+    def test_obtaining_lock_fails_when_outside_of_transaction(self):
+        objid = self.getUniqueInteger()
+        lock = dblocks.DatabaseLock(objid)
+        self.assertRaises(
+            dblocks.DatabaseLockAttemptOutsideTransaction,
+            lock.__enter__)
+
+    def test_releasing_lock_fails_when_lock_not_held(self):
+        objid = self.getUniqueInteger()
+        lock = dblocks.DatabaseLock(objid)
+        self.assertRaises(dblocks.DatabaseLockNotHeld, lock.__exit__)
+
     def test_repr(self):
         lock = dblocks.DatabaseLock(self.getUniqueInteger())
         self.assertEqual(