Merge lp:~allenap/maas/retry-context-not-active into lp:~maas-committers/maas/trunk

[Gavin Panella]

Use the retry context in web requests too.

Previously the retry context was only activated when using the @transactional decorator.

[Blake Rouse (blake-rouse)]

Looks good.

I am guessing this issue only popped up because of the changes you made to the static IP address allocation at the beginning of the cycle?

review: Approve

[Gavin Panella (allenap)]

> I am guessing this issue only popped up because of the changes you
> made to the static IP address allocation at the beginning of the
> cycle?

Yeah :-/

Thanks for the review.

=== modified file 'src/maasserver/utils/orm.py'
--- src/maasserver/utils/orm.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/utils/orm.py	2017-03-16 16:43:12 +0000
@@ -24,6 +24,7 @@
     'post_commit_do',
     'psql_array',
     'request_transaction_retry',
+    'retry_context',
     'retry_on_retryable_failure',
     'savepoint',
     'TotallyDisconnected',

=== modified file 'src/maasserver/utils/tests/test_views.py'
--- src/maasserver/utils/tests/test_views.py	2017-01-28 00:51:47 +0000
+++ src/maasserver/utils/tests/test_views.py	2017-03-16 16:43:12 +0000
@@ -7,6 +7,7 @@
 
 import http.client
 import io
+from itertools import count
 import logging
 from random import (
     randint,
@@ -40,6 +41,8 @@
 from maasserver.utils.orm import (
     make_deadlock_failure,
     post_commit_hooks,
+    request_transaction_retry,
+    retry_context,
     validate_in_transaction,
 )
 from maasserver.utils.views import HttpResponseConflict
@@ -386,6 +389,49 @@
             response.reason_phrase,
             Equals(REASON_PHRASES[http.client.CONFLICT]))
 
+    def test__get_response_prepare_retry_context_before_each_try(self):
+
+        class ObserveContext:
+
+            def __init__(self, callback, name):
+                self.callback = callback
+                self.name = name
+
+            def __enter__(self):
+                self.callback("%s:enter" % self.name)
+
+            def __exit__(self, *exc_info):
+                self.callback("%s:exit" % self.name)
+
+        counter = count(1)
+        calls = []
+
+        def get_response_and_retry(request):
+            calls.append("get_response")
+            request_transaction_retry(ObserveContext(
+                calls.append, "context#%d" % next(counter)))
+
+        get_response = self.patch(WSGIHandler, "get_response")
+        get_response.side_effect = get_response_and_retry
+
+        reset_request = self.patch_autospec(views, "reset_request")
+        reset_request.side_effect = lambda request: request
+
+        request = make_request()
+        request.path = factory.make_name("path")
+        handler = views.WebApplicationHandler(3)
+        handler.get_response(request)
+
+        self.assertThat(calls, Equals([
+            "get_response",     # 1st attempt.
+            "context#1:enter",  # Retry requested, enter 1st new context.
+            "get_response",     # 2nd attempt.
+            "context#2:enter",  # Retry requested, enter 2nd new context.
+            "get_response",     # 3rd attempt, and last.
+            "context#2:exit",   # Exit 2nd context.
+            "context#1:exit",   # Exit 1st context.
+        ]))
+
     def test__get_response_logs_retry_and_resets_request(self):
         timeout = 1.0 + (random() * 99)
         handler = views.WebApplicationHandler(2, timeout)
@@ -430,6 +476,23 @@
 
         self.assertThat(get_response, MockCalledOnceWith(request))
 
+    def test__get_response_is_in_retry_context_in_transaction(self):
+        handler = views.WebApplicationHandler(2)
+
+        def check_retry_context_active(request):
+            self.assertThat(retry_context.active, Is(True))
+
+        get_response = self.patch(WSGIHandler, "get_response")
+        get_response.side_effect = check_retry_context_active
+
+        request = make_request()
+        request.path = factory.make_name("path")
+
+        self.assertThat(retry_context.active, Is(False))
+        handler.get_response(request)
+        self.assertThat(retry_context.active, Is(False))
+        self.assertThat(get_response, MockCalledOnceWith(request))
+
     def test__get_response_restores_files_across_requests(self):
         handler = views.WebApplicationHandler(3)
         file_content = sample_binary_data

=== modified file 'src/maasserver/utils/views.py'
--- src/maasserver/utils/views.py	2016-10-21 06:19:34 +0000
+++ src/maasserver/utils/views.py	2017-03-16 16:43:12 +0000
@@ -23,6 +23,8 @@
     gen_retry_intervals,
     is_retryable_failure,
     post_commit_hooks,
+    retry_context,
+    RetryTransaction,
 )
 from piston3.authentication import initialize_server_request
 from piston3.models import Nonce
@@ -145,7 +147,9 @@
         # Add it to the retry set if this response was caused by a
         # retryable failure.
         exc_type, exc_value, exc_traceback = exc_info
-        if is_retryable_failure(exc_value):
+        if isinstance(exc_value, RetryTransaction):
+            self.__retry.add(response)
+        elif is_retryable_failure(exc_value):
             self.__retry.add(response)
         else:
             logger.error(
@@ -220,21 +224,24 @@
         retry_attempts = self.__retry_attempts
         retry_set = self.__retry
 
-        for attempt in count(1):
-            response = get_response(request)
-            if response in retry_set:
-                elapsed, remaining, wait = next(retry_details)
-                if attempt == retry_attempts or wait == 0:
-                    # Time's up: this was the final attempt.
-                    log_final_failed_attempt(request, attempt, elapsed)
-                    conflict_response = HttpResponseConflict(response)
-                    conflict_response.render()
-                    return conflict_response
-
-                # We'll retry after a brief interlude.
-                log_failed_attempt(request, attempt, elapsed, remaining, wait)
-                delete_oauth_nonce(request)
-                request = reset_request(request)
-                sleep(wait)
-            else:
-                return response
+        with retry_context:
+            for attempt in count(1):
+                retry_context.prepare()
+                response = get_response(request)
+                if response in retry_set:
+                    elapsed, remaining, wait = next(retry_details)
+                    if attempt == retry_attempts or wait == 0:
+                        # Time's up: this was the final attempt.
+                        log_final_failed_attempt(request, attempt, elapsed)
+                        conflict_response = HttpResponseConflict(response)
+                        conflict_response.render()
+                        return conflict_response
+                    else:
+                        # We'll retry after a brief interlude.
+                        log_failed_attempt(
+                            request, attempt, elapsed, remaining, wait)
+                        delete_oauth_nonce(request)
+                        request = reset_request(request)
+                        sleep(wait)
+                else:
+                    return response