launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-upload-retry-502-503 into lp:launchpad with lp:~cjwatson/launchpad/snap-better-upload-failures as a prerequisite.

Commit message:
Automatically retry snap store upload attempts that return 502 or 503.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-upload-retry-502-503/+merge/325536
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-upload-retry-502-503 into lp:launchpad.

=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py	2017-06-12 22:07:19 +0000
+++ lib/lp/snappy/interfaces/snapstoreclient.py	2017-06-12 22:07:19 +0000
@@ -29,10 +29,11 @@
 
 class SnapStoreError(Exception):
 
-    def __init__(self, message="", detail=None):
+    def __init__(self, message="", detail=None, can_retry=False):
         super(SnapStoreError, self).__init__(message)
         self.message = message
         self.detail = detail
+        self.can_retry = can_retry
 
 
 @error_status(httplib.INTERNAL_SERVER_ERROR)

=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py	2017-06-12 22:07:19 +0000
+++ lib/lp/snappy/model/snapbuildjob.py	2017-06-12 22:07:19 +0000
@@ -172,6 +172,10 @@
     pass
 
 
+class RetryableSnapStoreError(SnapStoreError):
+    pass
+
+
 @implementer(ISnapBuildStoreUploadStatusChangedEvent)
 class SnapBuildStoreUploadStatusChangedEvent(ObjectEvent):
     """See `ISnapBuildStoreUploadStatusChangedEvent`."""
@@ -191,8 +195,7 @@
         ReleaseFailedResponse,
         )
 
-    # XXX cjwatson 2016-05-04: identify transient upload failures and retry
-    retry_error_types = (UploadNotScannedYetResponse,)
+    retry_error_types = (UploadNotScannedYetResponse, RetryableSnapStoreError)
     retry_delay = timedelta(minutes=1)
     max_retries = 20
 
@@ -300,6 +303,9 @@
         except self.retry_error_types:
             raise
         except Exception as e:
+            if (isinstance(e, SnapStoreError) and e.can_retry and
+                    self.attempt_count <= self.max_retries):
+                raise RetryableSnapStoreError(e.message, detail=e.detail)
             self.error_message = str(e)
             self.error_detail = getattr(e, "detail", None)
             if isinstance(e, UnauthorizedUploadResponse):

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2017-06-12 22:07:19 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2017-06-12 22:07:19 +0000
@@ -152,7 +152,8 @@
         detail = requests_error.response.content
         if isinstance(detail, bytes):
             detail = detail.decode("UTF-8", errors="replace")
-        return error_class(error_message, detail=detail)
+        can_retry = requests_error.response.status_code in (502, 503)
+        return error_class(error_message, detail=detail, can_retry=can_retry)
 
     @classmethod
     def requestPackageUploadPermission(cls, snappy_series, snap_name):

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2017-06-12 22:07:19 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2017-06-12 22:07:19 +0000
@@ -230,6 +230,50 @@
         self.assertWebhookDeliveries(
             snapbuild, ["Pending", "Failed to upload"])
 
+    def test_run_502_retries(self):
+        # A run that gets a 502 error from the store schedules itself to be
+        # retried.
+        self.useFixture(FakeLogger())
+        snapbuild = self.makeSnapBuild()
+        self.assertContentEqual([], snapbuild.store_upload_jobs)
+        job = SnapStoreUploadJob.create(snapbuild)
+        client = FakeSnapStoreClient()
+        client.upload.failure = UploadFailedResponse(
+            "Proxy error", can_retry=True)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            JobRunner([job]).runAll()
+        self.assertEqual([((snapbuild,), {})], client.upload.calls)
+        self.assertEqual([], client.checkStatus.calls)
+        self.assertEqual([], client.release.calls)
+        self.assertContentEqual([job], snapbuild.store_upload_jobs)
+        self.assertIsNone(job.store_url)
+        self.assertIsNone(job.store_revision)
+        self.assertIsNone(job.error_message)
+        self.assertEqual([], pop_notifications())
+        self.assertEqual(JobStatus.WAITING, job.job.status)
+        self.assertWebhookDeliveries(snapbuild, ["Pending"])
+        # Try again.  The upload part of the job is retried, and this time
+        # it succeeds.
+        job.lease_expires = None
+        job.scheduled_start = None
+        client.upload.calls = []
+        client.upload.failure = None
+        client.upload.result = self.status_url
+        client.checkStatus.result = (self.store_url, 1)
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            JobRunner([job]).runAll()
+        self.assertEqual([((snapbuild,), {})], client.upload.calls)
+        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual([], client.release.calls)
+        self.assertContentEqual([job], snapbuild.store_upload_jobs)
+        self.assertEqual(self.store_url, job.store_url)
+        self.assertEqual(1, job.store_revision)
+        self.assertIsNone(job.error_message)
+        self.assertEqual([], pop_notifications())
+        self.assertEqual(JobStatus.COMPLETED, job.job.status)
+        self.assertWebhookDeliveries(snapbuild, ["Pending", "Uploaded"])
+
     def test_run_upload_failure_notifies(self):
         # A run that gets some other upload failure from the store sends
         # mail.

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2017-06-12 22:07:19 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2017-06-12 22:07:19 +0000
@@ -483,6 +483,7 @@
                     UploadFailedResponse, self.client.upload, snapbuild)
                 self.assertEqual(
                     "Developer has not signed agreement.", str(err))
+                self.assertFalse(err.can_retry)
 
     def test_upload_file_error(self):
         @urlmatch(path=r".*/unscanned-upload/$")
@@ -502,6 +503,7 @@
                     UploadFailedResponse, self.client.upload, snapbuild)
                 self.assertEqual("502 Server Error: Proxy Error", str(err))
                 self.assertEqual(b"The proxy exploded.\n", err.detail)
+                self.assertTrue(err.can_retry)
 
     def test_refresh_discharge_macaroon(self):
         store_secrets = self._make_store_secrets()