Merge lp:~allenap/maas/fix-image-imports into lp:~maas-committers/maas/trunk

[Gavin Panella]

Ensure that _import_resources() is never called within a transaction.

Previously this requirement was not asserted, tested, or documented so it was only a matter of time before a regression was introduced.

[Gavin Panella (allenap)]

This alone is the fix:

     return deferToThreadPool(
         reactor, reactor.threadpoolForDatabase,
-        transactional(func), *args, **kwargs)
+        func, *args, **kwargs)

The rest is tests, docs, and preventing _import_resources() from causing
a regression again.

[Mike Pontillo (mpontillo)]

Looks good.

review: Approve

[Andres Rodriguez (andreserl)]

lgtm!

review: Approve

=== modified file 'src/maasserver/bootresources.py'
--- src/maasserver/bootresources.py	2015-09-22 12:21:54 +0000
+++ src/maasserver/bootresources.py	2015-09-23 10:38:06 +0000
@@ -71,6 +71,7 @@
 )
 from maasserver.utils.orm import (
     get_one,
+    in_transaction,
     transactional,
 )
 from maasserver.utils.threads import deferToDatabase
@@ -920,10 +921,8 @@
         kill_event.wait()
 
 
-# FIXME: How this function deals with transactions is not clear. It uses the
-# ORM without ensuring there's a transaction. It also does network IO and we
-# don't want to be holding open a transaction for an indefinite time while
-# that happens.
+# FIXME: This does network IO while holding two database connections and two
+# transactions. This is bad for concurrency.
 @synchronous
 def _import_resources(force=False):
     """Import boot resources.
@@ -931,10 +930,16 @@
     Pulls the sources from `BootSource`. This only starts the process if
     some SYNCED `BootResource` already exist.
 
+    This MUST be called from outside of a database transaction.
+
     :param force: True will force the import, even if no SYNCED `BootResource`
         exist. This is used because we want the user to start the first import
         action, not let it run automatically.
     """
+    assert not in_transaction(), (
+        "_import_resources() must not be called within a preexisting "
+        "transaction; it manages its own.")
+
     # If the lock is already held, then import is already running.
     if locks.import_images.is_locked():
         maaslog.debug("Skipping import as another import is already running.")

=== added file 'src/maasserver/testing/dblocks.py'
--- src/maasserver/testing/dblocks.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/testing/dblocks.py	2015-09-23 10:38:06 +0000
@@ -0,0 +1,45 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helpers for testing database locks and related."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+str = None
+
+__metaclass__ = type
+__all__ = [
+    "lock_held_in_other_thread",
+]
+
+from contextlib import contextmanager
+import threading
+
+from maasserver.utils.orm import transactional
+
+
+@contextmanager
+def lock_held_in_other_thread(lock, timeout=10):
+    """Hold `lock` in another thread."""
+    held = threading.Event()
+    done = threading.Event()
+
+    @transactional
+    def hold():
+        with lock:
+            held.set()
+            done.wait(timeout)
+
+    thread = threading.Thread(target=hold)
+    thread.start()
+
+    held.wait(timeout)
+    try:
+        yield
+    finally:
+        done.set()
+        thread.join()

=== modified file 'src/maasserver/tests/test_bootresources.py'
--- src/maasserver/tests/test_bootresources.py	2015-09-15 11:29:37 +0000
+++ src/maasserver/tests/test_bootresources.py	2015-09-23 10:38:06 +0000
@@ -61,6 +61,7 @@
 )
 from maasserver.rpc.testing.fixtures import MockLiveRegionToClusterRPCFixture
 from maasserver.testing.config import RegionConfigurationFixture
+from maasserver.testing.dblocks import lock_held_in_other_thread
 from maasserver.testing.eventloop import (
     RegionEventLoopFixture,
     RunningEventLoopFixture,
@@ -1144,9 +1145,8 @@
     def test__import_resources_exits_early_if_lock_held(self):
         has_synced_resources = self.patch_autospec(
             bootresources, "has_synced_resources")
-        with transaction.atomic():
-            with bootresources.locks.import_images:
-                bootresources._import_resources(force=True)
+        with lock_held_in_other_thread(bootresources.locks.import_images):
+            bootresources._import_resources(force=True)
         # The test for already-synced resources is not called if the
         # lock is already held.
         self.assertThat(has_synced_resources, MockNotCalled())

=== modified file 'src/maasserver/utils/orm.py'
--- src/maasserver/utils/orm.py	2015-09-11 19:21:06 +0000
+++ src/maasserver/utils/orm.py	2015-09-23 10:38:06 +0000
@@ -22,6 +22,7 @@
     'get_exception_class',
     'get_first',
     'get_one',
+    'in_transaction',
     'is_serialization_failure',
     'macs_contain',
     'macs_do_not_contain',
@@ -552,7 +553,7 @@
             depth = depth - 1
 
 
-def in_transaction(connection):
+def in_transaction(connection=connection):
     """Is `connection` in the midst of a transaction?
 
     This only enquires as to Django's perspective on the situation. It does

=== modified file 'src/maasserver/utils/tests/test_dblocks.py'
--- src/maasserver/utils/tests/test_dblocks.py	2015-09-15 17:27:21 +0000
+++ src/maasserver/utils/tests/test_dblocks.py	2015-09-23 10:38:06 +0000
@@ -20,18 +20,17 @@
 )
 from random import randint
 import sys
-import threading
 
 from django.db import (
     connection,
     transaction,
 )
+from maasserver.testing.dblocks import lock_held_in_other_thread
 from maasserver.testing.testcase import (
     MAASServerTestCase,
     MAASTransactionServerTestCase,
 )
 from maasserver.utils import dblocks
-from maasserver.utils.orm import transactional
 from testtools.matchers import Equals
 
 
@@ -79,29 +78,6 @@
         query["sql"] for query in connection.queries)
 
 
-@contextmanager
-def lock_held_in_other_thread(lock, timeout=10):
-    """Hold `lock` in another thread."""
-    held = threading.Event()
-    done = threading.Event()
-
-    @transactional
-    def hold():
-        with lock:
-            held.set()
-            done.wait(timeout)
-
-    thread = threading.Thread(target=hold)
-    thread.start()
-
-    held.wait(timeout)
-    try:
-        yield
-    finally:
-        done.set()
-        thread.join()
-
-
 class TestDatabaseLock(MAASTransactionServerTestCase):
 
     scenarios = tuple(

=== modified file 'src/maasserver/utils/tests/test_orm.py'
--- src/maasserver/utils/tests/test_orm.py	2015-09-11 21:30:32 +0000
+++ src/maasserver/utils/tests/test_orm.py	2015-09-23 10:38:06 +0000
@@ -47,6 +47,7 @@
     get_one,
     get_psycopg2_exception,
     get_psycopg2_serialization_exception,
+    in_transaction,
     is_serialization_failure,
     macs_contain,
     macs_do_not_contain,
@@ -834,6 +835,24 @@
             context_manager.__exit__, MockCalledOnceWith(None, None, None))
 
 
+class TestInTransaction(DjangoTransactionTestCase):
+    """Tests for `in_transaction`."""
+
+    def test__true_within_atomic_block(self):
+        with transaction.atomic():
+            self.assertTrue(in_transaction())
+
+    def test__true_when_legacy_transaction_is_active(self):
+        transaction.enter_transaction_management()
+        try:
+            self.assertTrue(in_transaction())
+        finally:
+            transaction.leave_transaction_management()
+
+    def test__false_when_no_transaction_is_active(self):
+        self.assertFalse(in_transaction())
+
+
 class TestValidateInTransaction(DjangoTransactionTestCase):
     """Tests for `validate_in_transaction`."""
 

=== modified file 'src/maasserver/utils/tests/test_threads.py'
--- src/maasserver/utils/tests/test_threads.py	2015-09-11 19:21:51 +0000
+++ src/maasserver/utils/tests/test_threads.py	2015-09-23 10:38:06 +0000
@@ -116,6 +116,7 @@
     @inlineCallbacks
     def test__defers_to_database_threadpool(self):
 
+        @orm.transactional
         def call_in_database_thread(a, b):
             orm.validate_in_transaction(connection)
             return sentinel.called, a, b
@@ -132,6 +133,7 @@
     @inlineCallbacks
     def test__calls_out_to_database_threadpool(self):
 
+        @orm.transactional
         def call_in_database_thread(a, b):
             orm.validate_in_transaction(connection)
             return sentinel.bar, a, b

=== modified file 'src/maasserver/utils/threads.py'
--- src/maasserver/utils/threads.py	2015-09-11 19:21:51 +0000
+++ src/maasserver/utils/threads.py	2015-09-23 10:38:06 +0000
@@ -32,7 +32,6 @@
     ExclusivelyConnected,
     FullyConnected,
     TotallyDisconnected,
-    transactional,
 )
 from provisioningserver.utils.twisted import (
     asynchronous,
@@ -147,7 +146,7 @@
     """Call `func` in a thread where database activity is permitted."""
     return deferToThreadPool(
         reactor, reactor.threadpoolForDatabase,
-        transactional(func), *args, **kwargs)
+        func, *args, **kwargs)
 
 
 def callOutToDatabase(thing, func, *args, **kwargs):