Merge lp:~allenap/maas/rpc-create-table-race into lp:~maas-committers/maas/trunk

[Gavin Panella]

Take an advisory lock to prevent concurrent creation of the eventloops table.

Creating tables in PostgreSQL is a transactional operation like any
other. If the isolation level is not sufficient - the default in Django
- it is susceptible to races. Using a higher isolation level may lead to
serialisation failures, for example. Advisory locking side-steps this.

[Jeroen T. Vermeulen (jtv)]

Actually postgres does guard against races when creating new tables. The operation is transactional, much like a normal data manipulation statement. I suspect the problem happens at a higher level — possibly because of the lack of a transaction.

[Gavin Panella (allenap)]

> Actually postgres does guard against races when creating new tables. The
> operation is transactional, much like a normal data manipulation statement. I
> suspect the problem happens at a higher level — possibly because of the lack
> of a transaction.

I've changed the comment. Can you see if that's okay?

[Jeroen T. Vermeulen (jtv)]

Thank you. Very happy to see this resolved in the proper way.

One small note about the test: I'd prefer a patch for _do_create that merely recorded the result of is_locked(), followed by regular straight-line test code at the end to assert that the value was True. A sensible default (e.g. None) will also protect the test from possible future code changes that might cut _do_create out of the call chain.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/rpc-create-table-race into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:1 http://nova.clouds.archive.ubuntu.com trusty Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg
Get:2 http://nova.clouds.archive.ubuntu.com trusty Release [58.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates Release
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:3 http://nova.clouds.archive.ubuntu.com trusty/main Sources [1,075 kB]
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Get:4 http://nova.clouds.archive.ubuntu.com trusty/universe Sources [6,404 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages [1,367 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages [5,863 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 14.8 MB in 5s (2,613 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-netaddr python-net...

=== modified file 'src/maasserver/locks.py'
--- src/maasserver/locks.py	2014-03-25 21:48:21 +0000
+++ src/maasserver/locks.py	2014-04-01 17:26:35 +0000
@@ -13,6 +13,7 @@
 
 __metaclass__ = type
 __all__ = [
+    "eventloop",
     "security",
     "startup",
 ]
@@ -25,3 +26,6 @@
 # Lock around performing critical security-related operations, like
 # generating or signing certificates.
 security = DatabaseLock(2)
+
+# Lock used when starting up the event-loop.
+eventloop = DatabaseLock(3)

=== modified file 'src/maasserver/rpc/regionservice.py'
--- src/maasserver/rpc/regionservice.py	2014-03-20 22:44:06 +0000
+++ src/maasserver/rpc/regionservice.py	2014-04-01 17:26:35 +0000
@@ -30,7 +30,10 @@
     connection,
     transaction,
     )
-from maasserver import eventloop
+from maasserver import (
+    eventloop,
+    locks,
+    )
 from maasserver.utils import synchronised
 from provisioningserver.rpc import (
     cluster,
@@ -322,6 +325,7 @@
 
     @synchronous
     @synchronised(lock)
+    @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-03-29 17:30:11 +0000
+++ src/maasserver/rpc/tests/test_regionservice.py	2014-04-01 17:26:35 +0000
@@ -25,7 +25,10 @@
 from crochet import wait_for_reactor
 from django.db import connection
 from django.db.utils import ProgrammingError
-from maasserver import eventloop
+from maasserver import (
+    eventloop,
+    locks,
+    )
 from maasserver.rpc import regionservice
 from maasserver.rpc.regionservice import (
     Region,
@@ -638,6 +641,34 @@
                 cursor.execute("SELECT * FROM eventloops")
                 self.assertEqual([], list(cursor))
 
+    def test_prepare_holds_startup_lock(self):
+        # Creating tables in PostgreSQL is a transactional operation
+        # like any other. If the isolation level is not sufficient - the
+        # default in Django - it is susceptible to races. Using a higher
+        # isolation level may lead to serialisation failures, for
+        # example. However, PostgreSQL provides advisory locking
+        # functions, and that's what RegionAdvertisingService.prepare
+        # takes advantage of to prevent concurrent creation of the
+        # eventloops table.
+
+        # A record of the lock's status, populated when a custom
+        # patched-in _do_create() is called.
+        locked = []
+
+        def _do_create(cursor):
+            locked.append(locks.eventloop.is_locked())
+
+        service = RegionAdvertisingService()
+        service._do_create = _do_create
+
+        # The lock is not held before and after prepare() is called.
+        self.assertFalse(locks.eventloop.is_locked())
+        service.prepare()
+        self.assertFalse(locks.eventloop.is_locked())
+
+        # The lock was held when _do_create() was called.
+        self.assertEqual([True], locked)
+
     def test_update(self):
         example_addresses = [
             (factory.getRandomIPAddress(), factory.getRandomPort()),

=== modified file 'src/maasserver/utils/dblocks.py'
--- src/maasserver/utils/dblocks.py	2014-03-25 19:18:19 +0000
+++ src/maasserver/utils/dblocks.py	2014-04-01 17:26:35 +0000
@@ -70,7 +70,10 @@
             self.__class__.__name__, self[0], self[1])
 
     def is_locked(self):
-        stmt = "SELECT 1 FROM pg_locks WHERE classid = %s AND objid = %s"
+        stmt = (
+            "SELECT 1 FROM pg_locks"
+            " WHERE classid = %s AND objid = %s AND granted"
+        )
         with closing(connection.cursor()) as cursor:
             cursor.execute(stmt, self)
             return len(cursor.fetchall()) >= 1