Merge lp:~blake-rouse/maas/fix-1558785 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Retry deadlock detection failures just like serialization failures.

[Gavin Panella (allenap)]

Cool!

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~blake-rouse/maas/fix-1558785 into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://security.ubuntu.com/ubuntu xenial-security InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease [116 kB]
Hit:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Get:5 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main Sources [1,104 kB]
Get:6 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe Sources [7,510 kB]
Get:7 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/main amd64 Packages [1,435 kB]
Get:8 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe amd64 Packages [7,242 kB]
Get:9 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe Translation-en [4,185 kB]
Err:9 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial/universe Translation-en
  Hash Sum mismatch
Fetched 21.6 MB in 1s (11.3 MB/s)
Reading package lists...

sudo: unable to resolve host juju-prod-cdo-maas-machine-3
W: Failed to fetch http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu/dists/xenial/universe/i18n/Translation-en.xz Hash Sum mismatch
E: Some index files failed to download. They have been ignored, or old ones used instead.

=== modified file 'src/maasserver/middleware.py'
--- src/maasserver/middleware.py	2016-03-15 20:25:31 +0000
+++ src/maasserver/middleware.py	2016-03-18 14:47:09 +0000
@@ -46,7 +46,7 @@
 from maasserver.exceptions import MAASAPIException
 from maasserver.models.node import RackController
 from maasserver.rpc import getAllClients
-from maasserver.utils.orm import is_serialization_failure
+from maasserver.utils.orm import is_retryable_failure
 from maasserver.views.combo import MERGE_VIEWS
 from provisioningserver.rpc.exceptions import (
     NoConnectionsAvailable,
@@ -184,8 +184,8 @@
             # Not a path we're handling exceptions for.
             return None
 
-        if is_serialization_failure(exception):
-            # We never handle serialization failures.
+        if is_retryable_failure(exception):
+            # We never handle retryable failures.
             return None
 
         encoding = 'utf-8'

=== modified file 'src/maasserver/tests/test_middleware.py'
--- src/maasserver/tests/test_middleware.py	2016-02-02 14:20:45 +0000
+++ src/maasserver/tests/test_middleware.py	2016-03-18 14:47:09 +0000
@@ -42,7 +42,10 @@
 from maasserver.testing import extract_redirect
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
-from maasserver.utils.orm import make_serialization_failure
+from maasserver.utils.orm import (
+    make_deadlock_failure,
+    make_serialization_failure,
+)
 from maastesting.matchers import MockCalledOnceWith
 from maastesting.utils import sample_binary_data
 from mock import Mock
@@ -97,6 +100,13 @@
         exception = make_serialization_failure()
         self.assertIsNone(middleware.process_exception(request, exception))
 
+    def test_ignores_deadlock_failures(self):
+        base_path = self.make_base_path()
+        middleware = self.make_middleware(base_path)
+        request = factory.make_fake_request(base_path)
+        exception = make_deadlock_failure()
+        self.assertIsNone(middleware.process_exception(request, exception))
+
     def test_unknown_exception_generates_internal_server_error(self):
         # An unknown exception generates an internal server error with the
         # exception message.

=== modified file 'src/maasserver/utils/orm.py'
--- src/maasserver/utils/orm.py	2016-02-26 18:48:26 +0000
+++ src/maasserver/utils/orm.py	2016-03-18 14:47:09 +0000
@@ -13,13 +13,16 @@
     'get_first',
     'get_one',
     'in_transaction',
+    'is_deadlock_failure',
+    'is_retryable_failure',
     'is_serialization_failure',
+    'make_deadlock_failure',
     'make_serialization_failure',
     'post_commit',
     'post_commit_do',
     'psql_array',
     'request_transaction_retry',
-    'retry_on_serialization_failure',
+    'retry_on_retryable_failure',
     'savepoint',
     'TotallyDisconnected',
     'transactional',
@@ -66,7 +69,10 @@
 from provisioningserver.utils.network import parse_integer
 from provisioningserver.utils.twisted import callOut
 import psycopg2
-from psycopg2.errorcodes import SERIALIZATION_FAILURE
+from psycopg2.errorcodes import (
+    DEADLOCK_DETECTED,
+    SERIALIZATION_FAILURE,
+)
 from twisted.internet.defer import Deferred
 
 
@@ -211,6 +217,62 @@
     return exception
 
 
+class DeadlockFailure(psycopg2.OperationalError):
+    """Explicit deadlock failure.
+
+    A real deadlock failure, arising out of psycopg2 (and thus signalled
+    from the database) would *NOT* be an instance of this class. However, it
+    is not obvious how to create a `psycopg2.OperationalError` with ``pgcode``
+    set to `DEADLOCK_DETECTED` without subclassing. I suspect only the C
+    interface can do that.
+    """
+    pgcode = DEADLOCK_DETECTED
+
+
+def make_deadlock_failure():
+    """Make a deadlock exception.
+
+    Artificially construct an exception that resembles what Django's ORM would
+    raise when PostgreSQL fails a transaction because of a deadlock
+    failure.
+
+    :returns: an instance of :py:class:`OperationalError` that will pass the
+        `is_deadlock_failure` predicate.
+    """
+    exception = OperationalError()
+    exception.__cause__ = DeadlockFailure()
+    assert is_deadlock_failure(exception)
+    return exception
+
+
+def get_psycopg2_deadlock_exception(exception):
+    """Return the root-cause if `exception` is a deadlock failure.
+
+    PostgreSQL sets a specific error code, "40P01", when a transaction breaks
+    because of a deadlock failure.
+
+    :return: The underlying `psycopg2.Error` if it's a deadlock failure,
+    or `None` if there isn't one.
+    """
+    exception = get_psycopg2_exception(exception)
+    if exception is None:
+        return None
+    elif exception.pgcode == DEADLOCK_DETECTED:
+        return exception
+    else:
+        return None
+
+
+def is_deadlock_failure(exception):
+    """Does `exception` represent a deadlock failure?
+
+    PostgreSQL sets a specific error code, "40P01", when a transaction breaks
+    because of a deadlock failure. This is normally about the right time
+    to try again.
+    """
+    return get_psycopg2_deadlock_exception(exception) is not None
+
+
 def request_transaction_retry():
     """Raise a serialization exception.
 
@@ -223,6 +285,12 @@
     raise make_serialization_failure()
 
 
+def is_retryable_failure(exception):
+    """Does `exception` represent a serialization or deadlock failure?"""
+    return (
+        is_serialization_failure(exception) or is_deadlock_failure(exception))
+
+
 def gen_retry_intervals(base=0.01, rate=2.5, maximum=10.0):
     """Generate retry intervals based on an exponential series.
 
@@ -248,11 +316,11 @@
     """Do nothing."""
 
 
-def retry_on_serialization_failure(func, reset=noop):
-    """Retry the wrapped function when it raises a serialization failure.
+def retry_on_retryable_failure(func, reset=noop):
+    """Retry the wrapped function when it raises a retryable failure.
 
     It will call `func` a maximum of ten times, and will only retry if a
-    serialization failure is detected.
+    retryable failure is detected.
 
     BE CAREFUL WHERE YOU USE THIS.
 
@@ -263,8 +331,8 @@
 
     :param reset: An optional callable that will be called between attempts.
         It is *not* called before the first attempt. If the last attempt fails
-        with a serialization failure it will *not* be called. If an attempt
-        fails with a non-serialization failure, it will *not* be called.
+        with a retryable failure it will *not* be called. If an attempt
+        fails with a non-retryable failure, it will *not* be called.
 
     """
     @wraps(func)
@@ -274,7 +342,7 @@
             try:
                 return func(*args, **kwargs)
             except OperationalError as error:
-                if is_serialization_failure(error):
+                if is_retryable_failure(error):
                     reset()  # Which may do nothing.
                     sleep(next(intervals))
                 else:
@@ -417,10 +485,10 @@
     happy, especially in the test suite.
 
     In addition, if `func` is being invoked from outside of a transaction,
-    this will retry if it fails with a serialization failure.
+    this will retry if it fails with a retryable failure.
     """
     func_within_txn = transaction.atomic(func)  # For savepoints.
-    func_outside_txn = retry_on_serialization_failure(
+    func_outside_txn = retry_on_retryable_failure(
         func_within_txn, reset=post_commit_hooks.reset)
 
     @wraps(func)

=== modified file 'src/maasserver/utils/tests/test_orm.py'
--- src/maasserver/utils/tests/test_orm.py	2016-02-10 20:24:35 +0000
+++ src/maasserver/utils/tests/test_orm.py	2016-03-18 14:47:09 +0000
@@ -37,9 +37,12 @@
     get_first,
     get_model_object_name,
     get_one,
+    get_psycopg2_deadlock_exception,
     get_psycopg2_exception,
     get_psycopg2_serialization_exception,
     in_transaction,
+    is_deadlock_failure,
+    is_retryable_failure,
     is_serialization_failure,
     make_serialization_failure,
     post_commit,
@@ -47,7 +50,7 @@
     post_commit_hooks,
     psql_array,
     request_transaction_retry,
-    retry_on_serialization_failure,
+    retry_on_retryable_failure,
     savepoint,
     TotallyDisconnected,
     validate_in_transaction,
@@ -74,7 +77,10 @@
     DeferredValue,
 )
 import psycopg2
-from psycopg2.errorcodes import SERIALIZATION_FAILURE
+from psycopg2.errorcodes import (
+    DEADLOCK_DETECTED,
+    SERIALIZATION_FAILURE,
+)
 from testtools import ExpectedException
 from testtools.deferredruntest import extract_result
 from testtools.matchers import (
@@ -226,11 +232,16 @@
         exception = factory.make_exception()
         self.assertIsNone(get_psycopg2_serialization_exception(exception))
 
-    def test__returns_psycopg2_error_root_cause(self):
+    def test__returns_psycopg2_error_root_cause_for_serialization(self):
         exception = Exception()
         exception.__cause__ = orm.SerializationFailure()
         self.assertIs(exception.__cause__, get_psycopg2_exception(exception))
 
+    def test__returns_psycopg2_error_root_cause_for_deadlock(self):
+        exception = Exception()
+        exception.__cause__ = orm.DeadlockFailure()
+        self.assertIs(exception.__cause__, get_psycopg2_exception(exception))
+
 
 class TestGetPsycopg2SerializationException(MAASTestCase):
     """Tests for `get_psycopg2_serialization_exception`."""
@@ -251,6 +262,25 @@
             get_psycopg2_serialization_exception(exception))
 
 
+class TestGetPsycopg2DeadlockException(MAASTestCase):
+    """Tests for `get_psycopg2_deadlock_exception`."""
+
+    def test__returns_None_for_plain_psycopg2_error(self):
+        exception = psycopg2.Error()
+        self.assertIsNone(get_psycopg2_deadlock_exception(exception))
+
+    def test__returns_None_for_other_error(self):
+        exception = factory.make_exception()
+        self.assertIsNone(get_psycopg2_deadlock_exception(exception))
+
+    def test__returns_psycopg2_error_root_cause(self):
+        exception = Exception()
+        exception.__cause__ = orm.DeadlockFailure()
+        self.assertIs(
+            exception.__cause__,
+            get_psycopg2_deadlock_exception(exception))
+
+
 class TestIsSerializationFailure(SerializationFailureTestCase):
     """Tests relating to MAAS's use of SERIALIZABLE isolation."""
 
@@ -281,7 +311,75 @@
         self.assertFalse(is_serialization_failure(error))
 
 
-class TestRetryOnSerializationFailure(SerializationFailureTestCase):
+class TestIsDeadlockFailure(MAASTestCase):
+    """Tests relating to MAAS's use of catching deadlock failures."""
+
+    def test_detects_operational_error_with_matching_cause(self):
+        error = orm.make_deadlock_failure()
+        self.assertTrue(is_deadlock_failure(error))
+
+    def test_rejects_operational_error_without_matching_cause(self):
+        error = OperationalError()
+        cause = self.patch(error, "__cause__", Exception())
+        cause.pgcode = factory.make_name("pgcode")
+        self.assertFalse(is_deadlock_failure(error))
+
+    def test_rejects_operational_error_with_unrelated_cause(self):
+        error = OperationalError()
+        error.__cause__ = Exception()
+        self.assertFalse(is_deadlock_failure(error))
+
+    def test_rejects_operational_error_without_cause(self):
+        error = OperationalError()
+        self.assertFalse(is_deadlock_failure(error))
+
+    def test_rejects_non_operational_error_with_matching_cause(self):
+        error = factory.make_exception()
+        cause = self.patch(error, "__cause__", Exception())
+        cause.pgcode = DEADLOCK_DETECTED
+        self.assertFalse(is_deadlock_failure(error))
+
+
+class TestIsRetryableFailure(MAASTestCase):
+    """Tests relating to MAAS's use of catching retryable failures."""
+
+    def test_detects_serialization_failure(self):
+        error = orm.make_serialization_failure()
+        self.assertTrue(is_retryable_failure(error))
+
+    def test_detects_deadlock_failure(self):
+        error = orm.make_deadlock_failure()
+        self.assertTrue(is_retryable_failure(error))
+
+    def test_rejects_operational_error_without_matching_cause(self):
+        error = OperationalError()
+        cause = self.patch(error, "__cause__", Exception())
+        cause.pgcode = factory.make_name("pgcode")
+        self.assertFalse(is_retryable_failure(error))
+
+    def test_rejects_operational_error_with_unrelated_cause(self):
+        error = OperationalError()
+        error.__cause__ = Exception()
+        self.assertFalse(is_retryable_failure(error))
+
+    def test_rejects_operational_error_without_cause(self):
+        error = OperationalError()
+        self.assertFalse(is_retryable_failure(error))
+
+    def test_rejects_non_operational_error_with_cause_serialization(self):
+        error = factory.make_exception()
+        cause = self.patch(error, "__cause__", Exception())
+        cause.pgcode = SERIALIZATION_FAILURE
+        self.assertFalse(is_retryable_failure(error))
+
+    def test_rejects_non_operational_error_with_cause_deadlock(self):
+        error = factory.make_exception()
+        cause = self.patch(error, "__cause__", Exception())
+        cause.pgcode = DEADLOCK_DETECTED
+        self.assertFalse(is_retryable_failure(error))
+
+
+class TestRetryOnRetryableFailure(SerializationFailureTestCase):
 
     def make_mock_function(self):
         function_name = factory.make_name("function")
@@ -291,7 +389,7 @@
     def test_retries_on_serialization_failure(self):
         function = self.make_mock_function()
         function.side_effect = self.cause_serialization_failure
-        function_wrapped = retry_on_serialization_failure(function)
+        function_wrapped = retry_on_retryable_failure(function)
         self.assertRaises(OperationalError, function_wrapped)
         expected_calls = [call()] * 10
         self.assertThat(function, MockCallsMatch(*expected_calls))
@@ -301,13 +399,28 @@
             OperationalError, self.cause_serialization_failure)
         function = self.make_mock_function()
         function.side_effect = [serialization_error, sentinel.result]
-        function_wrapped = retry_on_serialization_failure(function)
+        function_wrapped = retry_on_retryable_failure(function)
+        self.assertEqual(sentinel.result, function_wrapped())
+        self.assertThat(function, MockCallsMatch(call(), call()))
+
+    def test_retries_on_deadlock_failure(self):
+        function = self.make_mock_function()
+        function.side_effect = orm.make_deadlock_failure()
+        function_wrapped = retry_on_retryable_failure(function)
+        self.assertRaises(OperationalError, function_wrapped)
+        expected_calls = [call()] * 10
+        self.assertThat(function, MockCallsMatch(*expected_calls))
+
+    def test_retries_on_deadlock_failure_until_successful(self):
+        function = self.make_mock_function()
+        function.side_effect = [orm.make_deadlock_failure(), sentinel.result]
+        function_wrapped = retry_on_retryable_failure(function)
         self.assertEqual(sentinel.result, function_wrapped())
         self.assertThat(function, MockCallsMatch(call(), call()))
 
     def test_passes_args_to_wrapped_function(self):
         function = lambda a, b: (a, b)
-        function_wrapped = retry_on_serialization_failure(function)
+        function_wrapped = retry_on_retryable_failure(function)
         self.assertEqual(
             (sentinel.a, sentinel.b),
             function_wrapped(sentinel.a, b=sentinel.b))
@@ -316,7 +429,7 @@
         reset = Mock()
         function = self.make_mock_function()
         function.side_effect = self.cause_serialization_failure
-        function_wrapped = retry_on_serialization_failure(function, reset)
+        function_wrapped = retry_on_retryable_failure(function, reset)
         self.assertRaises(OperationalError, function_wrapped)
         expected_function_calls = [call()] * 10
         self.expectThat(function, MockCallsMatch(*expected_function_calls))
@@ -328,7 +441,7 @@
         reset = Mock()
         function = self.make_mock_function()
         function.return_value = sentinel.all_is_okay
-        function_wrapped = retry_on_serialization_failure(function, reset)
+        function_wrapped = retry_on_retryable_failure(function, reset)
         function_wrapped()
         self.assertThat(reset, MockNotCalled())
 

=== modified file 'src/maasserver/utils/tests/test_views.py'
--- src/maasserver/utils/tests/test_views.py	2016-02-24 15:42:58 +0000
+++ src/maasserver/utils/tests/test_views.py	2016-03-18 14:47:09 +0000
@@ -32,6 +32,7 @@
 )
 from maasserver.utils import views
 from maasserver.utils.orm import (
+    make_deadlock_failure,
     post_commit_hooks,
     validate_in_transaction,
 )
@@ -283,6 +284,20 @@
         self.assertThat(
             response, IsInstance(HttpResponseConflict))
 
+    def test__get_response_catches_deadlock_failures(self):
+        get_response = self.patch(WSGIHandler, "get_response")
+        get_response.side_effect = make_deadlock_failure()
+
+        handler = views.WebApplicationHandler(1)
+        request = make_request()
+        request.path = factory.make_name("path")
+        response = handler.get_response(request)
+
+        self.assertThat(
+            get_response, MockCalledOnceWith(request))
+        self.assertThat(
+            response, IsInstance(HttpResponseConflict))
+
     def test__get_response_sends_signal_on_serialization_failures(self):
         get_response = self.patch(WSGIHandler, "get_response")
         get_response.side_effect = (
@@ -300,6 +315,22 @@
             send_request_exception, MockCalledOnceWith(
                 sender=views.WebApplicationHandler, request=request))
 
+    def test__get_response_sends_signal_on_deadlock_failures(self):
+        get_response = self.patch(WSGIHandler, "get_response")
+        get_response.side_effect = make_deadlock_failure()
+
+        send_request_exception = self.patch_autospec(
+            signals.got_request_exception, "send")
+
+        handler = views.WebApplicationHandler(1)
+        request = make_request()
+        request.path = factory.make_name("path")
+        handler.get_response(request)
+
+        self.assertThat(
+            send_request_exception, MockCalledOnceWith(
+                sender=views.WebApplicationHandler, request=request))
+
     def test__get_response_tries_only_once(self):
         get_response = self.patch(WSGIHandler, "get_response")
         get_response.return_value = sentinel.response

=== modified file 'src/maasserver/utils/views.py'
--- src/maasserver/utils/views.py	2016-02-24 15:42:58 +0000
+++ src/maasserver/utils/views.py	2016-03-18 14:47:09 +0000
@@ -21,7 +21,7 @@
 from django.template.response import SimpleTemplateResponse
 from maasserver.utils.orm import (
     gen_retry_intervals,
-    is_serialization_failure,
+    is_retryable_failure,
     post_commit_hooks,
 )
 from piston3.authentication import initialize_server_request
@@ -126,7 +126,7 @@
     :ivar __retry_timeout: The number of seconds after which this request will
         no longer be considered for a retry.
     :ivar __retry: A weak set containing responses that have been generated as
-        a result of a serialization failure.
+        a result of a retryable failure.
     """
 
     def __init__(self, attempts=10, timeout=90.0):
@@ -139,15 +139,15 @@
     def handle_uncaught_exception(self, request, resolver, exc_info):
         """Override `BaseHandler.handle_uncaught_exception`.
 
-        If a serialization failure is detected, a retry is requested. It's up
+        If a retryable failure is detected, a retry is requested. It's up
         to ``get_response`` to actually do the retry.
         """
         upcall = super(WebApplicationHandler, self).handle_uncaught_exception
         response = upcall(request, resolver, exc_info)
         # Add it to the retry set if this response was caused by a
-        # serialization failure.
+        # retryable failure.
         exc_type, exc_value, exc_traceback = exc_info
-        if is_serialization_failure(exc_value):
+        if is_retryable_failure(exc_value):
             self.__retry.add(response)
         else:
             # Log the error to the regiond.log.
@@ -155,7 +155,7 @@
                 exc_value=exc_value, exc_type=exc_type, exc_tb=exc_traceback)
             log.err(failure, _why="500 Error - %s" % request.path)
         # Return the response regardless. This means that we'll get Django's
-        # error page when there's a persistent serialization failure.
+        # error page when there's a persistent retryable failure.
         return response
 
     def make_view_atomic(self, view):
@@ -193,7 +193,7 @@
 
         def get_response(request):
             # Up-call to Django's get_response() in a transaction. This
-            # transaction may fail because of a serialization conflict, so
+            # transaction may fail because of a retryable conflict, so
             # pass errors to handle_uncaught_exception().
             try:
                 with post_commit_hooks: