launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging lp:~twom/launchpad/move-snap-building-metadata-to-snapbuild-take-2 into lp:launchpad.

Commit message:
Move snapbuilduploadjob metadata to snapbuildjob, to prevent getting into an unretryable state.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/move-snap-building-metadata-to-snapbuild-take-2/+merge/360784

Supersedes https://code.launchpad.net/~twom/launchpad/move-snap-building-metadata-to-snapbuild/+merge/360192
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/move-snap-building-metadata-to-snapbuild-take-2 into lp:launchpad.

=== modified file 'lib/lp/snappy/interfaces/snapbuild.py'
--- lib/lp/snappy/interfaces/snapbuild.py	2018-10-09 10:35:44 +0000
+++ lib/lp/snappy/interfaces/snapbuild.py	2018-12-12 10:52:05 +0000
@@ -34,7 +34,10 @@
     Reference,
     )
 from zope.component.interfaces import IObjectEvent
-from zope.interface import Interface
+from zope.interface import (
+    Attribute,
+    Interface,
+    )
 from zope.schema import (
     Bool,
     Choice,
@@ -242,6 +245,9 @@
         value_type=Dict(key_type=TextLine()),
         required=False, readonly=True))
 
+    store_upload_metadata = Attribute(
+        _("A dict of data about store upload progress."))
+
     def getFiles():
         """Retrieve the build's `ISnapFile` records.
 

=== modified file 'lib/lp/snappy/interfaces/snapbuildjob.py'
--- lib/lp/snappy/interfaces/snapbuildjob.py	2017-04-18 17:54:03 +0000
+++ lib/lp/snappy/interfaces/snapbuildjob.py	2018-12-12 10:52:05 +0000
@@ -54,6 +54,9 @@
 class ISnapStoreUploadJob(IRunnableJob):
     """A Job that uploads a snap build to the store."""
 
+    store_metadata = Attribute(
+        _("Combined metadata for this job and matching snapbuild"))
+
     error_message = TextLine(
         title=_("Error message"), required=False, readonly=True)
 
@@ -68,6 +71,10 @@
         title=_("The revision assigned to this build by the store"),
         required=False, readonly=True)
 
+    status_url = TextLine(
+        title=_("The URL on the store to get the status of this build"),
+        required=False, readonly=True)
+
 
 class ISnapStoreUploadJobSource(IJobSource):
 

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2018-10-09 10:35:44 +0000
+++ lib/lp/snappy/model/snapbuild.py	2018-12-12 10:52:05 +0000
@@ -175,9 +175,11 @@
 
     failure_count = Int(name='failure_count', allow_none=False)
 
+    store_upload_metadata = JSON('store_upload_json_data', allow_none=False)
+
     def __init__(self, build_farm_job, requester, snap, archive,
                  distro_arch_series, pocket, channels, processor, virtualized,
-                 date_created, build_request=None):
+                 date_created, store_upload_metadata=None, build_request=None):
         """Construct a `SnapBuild`."""
         super(SnapBuild, self).__init__()
         self.build_farm_job = build_farm_job
@@ -190,6 +192,7 @@
         self.processor = processor
         self.virtualized = virtualized
         self.date_created = date_created
+        self.store_upload_metadata = store_upload_metadata or {}
         if build_request is not None:
             self.build_request_id = build_request.id
         self.status = BuildStatus.NEEDSBUILD
@@ -507,7 +510,8 @@
 class SnapBuildSet(SpecificBuildFarmJobSourceMixin):
 
     def new(self, requester, snap, archive, distro_arch_series, pocket,
-            channels=None, date_created=DEFAULT, build_request=None):
+            channels=None, date_created=DEFAULT,
+            store_upload_metadata=None, build_request=None):
         """See `ISnapBuildSet`."""
         store = IMasterStore(SnapBuild)
         build_farm_job = getUtility(IBuildFarmJobSource).new(
@@ -518,7 +522,8 @@
             pocket, channels, distro_arch_series.processor,
             not distro_arch_series.processor.supports_nonvirtualized
             or snap.require_virtualized or archive.require_virtualized,
-            date_created, build_request=build_request)
+            date_created, store_upload_metadata=store_upload_metadata,
+            build_request=build_request)
         store.add(snapbuild)
         return snapbuild
 

=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py	2018-09-18 14:13:16 +0000
+++ lib/lp/snappy/model/snapbuildjob.py	2018-12-12 10:52:05 +0000
@@ -212,9 +212,17 @@
         return job
 
     @property
+    def store_metadata(self):
+        """See `ISnapStoreUploadJob`."""
+        intermediate = {}
+        intermediate.update(self.metadata)
+        intermediate.update(self.snapbuild.store_upload_metadata)
+        return intermediate
+
+    @property
     def error_message(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("error_message")
+        return self.store_metadata.get("error_message")
 
     @error_message.setter
     def error_message(self, message):
@@ -224,7 +232,7 @@
     @property
     def error_detail(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("error_detail")
+        return self.store_metadata.get("error_detail")
 
     @error_detail.setter
     def error_detail(self, detail):
@@ -234,7 +242,7 @@
     @property
     def error_messages(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("error_messages")
+        return self.store_metadata.get("error_messages")
 
     @error_messages.setter
     def error_messages(self, messages):
@@ -244,22 +252,31 @@
     @property
     def store_url(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("store_url")
+        return self.store_metadata.get("store_url")
 
     @store_url.setter
     def store_url(self, url):
         """See `ISnapStoreUploadJob`."""
-        self.metadata["store_url"] = url
+        self.snapbuild.store_upload_metadata["store_url"] = url
 
     @property
     def store_revision(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("store_revision")
+        return self.store_metadata.get("store_revision")
 
     @store_revision.setter
     def store_revision(self, revision):
         """See `ISnapStoreUploadJob`."""
-        self.metadata["store_revision"] = revision
+        self.snapbuild.store_upload_metadata["store_revision"] = revision
+
+    @property
+    def status_url(self):
+        """See `ISnapStoreUploadJob`."""
+        return self.store_metadata.get('status_url')
+
+    @status_url.setter
+    def status_url(self, url):
+        self.snapbuild.store_upload_metadata["status_url"] = url
 
     # Ideally we'd just override Job._set_status or similar, but
     # lazr.delegates makes that difficult, so we use this to override all
@@ -297,7 +314,7 @@
     @property
     def retry_delay(self):
         """See `BaseRunnableJob`."""
-        if "status_url" in self.metadata and self.store_url is None:
+        if "status_url" in self.store_metadata and self.store_url is None:
             # At the moment we have to poll the status endpoint to find out
             # if the store has finished scanning.  Try to deal with easy
             # cases quickly without hammering our job runners or the store
@@ -313,13 +330,13 @@
         """See `IRunnableJob`."""
         client = getUtility(ISnapStoreClient)
         try:
-            if "status_url" not in self.metadata:
-                self.metadata["status_url"] = client.upload(self.snapbuild)
+            if "status_url" not in self.store_metadata:
+                self.status_url = client.upload(self.snapbuild)
                 # We made progress, so reset attempt_count.
                 self.attempt_count = 1
             if self.store_url is None:
                 self.store_url, self.store_revision = (
-                    client.checkStatus(self.metadata["status_url"]))
+                    client.checkStatus(self.status_url))
                 # We made progress, so reset attempt_count.
                 self.attempt_count = 1
             if self.snapbuild.snap.store_channels:

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2018-08-03 13:53:20 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2018-12-12 10:52:05 +0000
@@ -628,7 +628,7 @@
         for expected_delay in (15, 15, 30, 30, 60):
             with dbuser(config.ISnapStoreUploadJobSource.dbuser):
                 JobRunner([job]).runAll()
-            self.assertIn("status_url", job.metadata)
+            self.assertIn("status_url", job.snapbuild.store_upload_metadata)
             self.assertIsNone(job.store_url)
             self.assertEqual(
                 timedelta(seconds=expected_delay), job.retry_delay)
@@ -641,3 +641,40 @@
         self.assertIsNone(job.error_message)
         self.assertEqual([], pop_notifications())
         self.assertEqual(JobStatus.COMPLETED, job.job.status)
+
+    def test_retry_after_upload_does_not_upload(self):
+        # If the job has uploaded, but failed to release, it should
+        # not attempt to upload again on the next run.
+        self.useFixture(FakeLogger())
+        snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"])
+        self.assertContentEqual([], snapbuild.store_upload_jobs)
+        job = SnapStoreUploadJob.create(snapbuild)
+        client = FakeSnapStoreClient()
+        client.upload.result = self.status_url
+        client.checkStatus.result = (self.store_url, 1)
+        client.release.failure = UploadFailedResponse(
+            "Proxy error", can_retry=True)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            JobRunner([job]).runAll()
+
+        previous_upload = client.upload.calls
+        previous_checkStatus = client.checkStatus.calls
+        len_previous_release = len(client.release.calls)
+
+        # Check we uploaded as expected
+        self.assertEqual(self.store_url, job.store_url)
+        self.assertEqual(1, job.store_revision)
+        self.assertEqual(timedelta(seconds=60), job.retry_delay)
+        self.assertEqual(1, len(client.upload.calls))
+        self.assertIsNone(job.error_message)
+
+        # Run the job again
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            JobRunner([job]).runAll()
+
+        # We should not have called `upload`, but moved straight to `release`
+        self.assertEqual(previous_upload, client.upload.calls)
+        self.assertEqual(previous_checkStatus, client.checkStatus.calls)
+        self.assertEqual(len_previous_release + 1, len(client.release.calls))
+        self.assertIsNone(job.error_message)