From: Itamar Turner-Trauring <itamar@blake.(none)>
Date: Fri, 19 Apr 2013 18:12:11 +0000 (-0400)
Subject: Some improvements and bug fixes.
X-Git-Url: https://git.rkrishnan.org/reliability?a=commitdiff_plain;h=a1083886df0aac8d86382e6081c2e4db19d2c81a;p=tahoe-lafs%2Ftahoe-lafs.git

Some improvements and bug fixes.

1. Discard body even if response code indicates a problem, when doing cloud backend HTTP requests. I believe this was triggering a bug in Twistd.
2. Google backend retries on 403 and other 4xx codes, not just 401.
3. More logging.
---

diff --git a/src/allmydata/storage/backends/cloud/cloud_common.py b/src/allmydata/storage/backends/cloud/cloud_common.py
index 72ac66e6..a62d94f3 100644
--- a/src/allmydata/storage/backends/cloud/cloud_common.py
+++ b/src/allmydata/storage/backends/cloud/cloud_common.py
@@ -401,12 +401,14 @@ class ContainerRetryMixin:
                 retry = False
 
         if retry:
+            log.msg("Rescheduling failed task for retry in %d seconds." % (BACKOFF_SECONDS_BEFORE_RETRY[trynum-1],))
             d = task.deferLater(self._reactor, BACKOFF_SECONDS_BEFORE_RETRY[trynum-1], operation, *args, **kwargs)
             d.addErrback(self._handle_error, trynum+1, first_err_and_tb, description, operation, *args, **kwargs)
             return d
 
         # If we get an error response for which _react_to_error says we should not retry,
         # raise that error even if the request was itself a retry.
+        log.msg("Giving up, no retry for %s" % (err,))
         raise err.__class__, err, tb
 
 
@@ -673,6 +675,7 @@ class HTTPClientMixin:
                     what=what, code=response.code, phrase=response.phrase, level=log.OPERATIONAL)
 
             if response.code < 200 or response.code >= 300:
+                response.deliverBody(Discard())
                 raise self.ServiceError(None, response.code,
                                         message="unexpected response code %r %s" % (response.code, response.phrase))
 
diff --git a/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py b/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py
index 369c9114..47c2ff24 100644
--- a/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py
+++ b/src/allmydata/storage/backends/cloud/googlestorage/googlestorage_container.py
@@ -29,6 +29,7 @@ except ImportError:
 
 from zope.interface import implements
 
+from allmydata.util import log
 from allmydata.storage.backends.cloud.cloud_common import IContainer, \
      ContainerItem, ContainerListing, CommonContainerMixin
 
@@ -70,7 +71,13 @@ class AuthenticationClient(object):
                 # Generally using a task-specific thread pool is better than using
                 # the reactor one. However, this particular call will only run
                 # once an hour, so it's not likely to tie up all the threads.
-                return self._deferToThread(self._credentials.refresh, httplib2.Http())
+                log.msg("Reauthenticating against Google Cloud Storage.")
+                def finished(result):
+                    log.msg("Done reauthenticating against Google Cloud Storage.")
+                    return result
+                d = self._deferToThread(self._credentials.refresh, httplib2.Http())
+                d.addBoth(finished)
+                return d
         return self._lock.run(run)
 
     def get_authorization_header(self):
@@ -108,7 +115,9 @@ class GoogleStorageContainer(CommonContainerMixin):
         self._project_id = project_id # Only need for bucket creation/deletion
 
     def _react_to_error(self, response_code):
-        if response_code == UNAUTHORIZED:
+        if response_code >= 400 and response_code < 500:
+            # Unauthorized/forbidden/etc. we should retry, eventually we will
+            # reauthenticate:
             return True
         else:
             return CommonContainerMixin._react_to_error(self, response_code)
diff --git a/src/allmydata/test/test_storage.py b/src/allmydata/test/test_storage.py
index 01a92651..8e3843bb 100644
--- a/src/allmydata/test/test_storage.py
+++ b/src/allmydata/test/test_storage.py
@@ -1118,6 +1118,16 @@ class GoogleStorageBackend(unittest.TestCase, CloudStorageBackendMixin):
         third.callback(done)
         self.assertEqual(result, [done])
 
+    def test_react_to_error(self):
+        """
+        GoogleStorageContainer._react_to_error() will return True (i.e. retry)
+        for any response code between 400 and 599.
+        """
+        self.assertFalse(self.container._react_to_error(399))
+        self.assertFalse(self.container._react_to_error(600))
+        for i in range(400, 600):
+            self.assertTrue(self.container._react_to_error(i))
+
     def test_head_object(self):
         """
         GoogleStorageContainer.head_object() sends the appropriate HTTP