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 into lp:launchpad with lp:~twom/launchpad/snapbuild-json-data-column as a prerequisite.

Commit message:
Move some of SnapBuildJob.metadata up to SnapBuilt to enable better retryable state.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Storing the metadata (store_url and status_url) on the SnapBuildJob meant that on each retry it needed to be set. This meant that it was possible to get to an unretryable state if the upload succeeded, but the release failed.

Moved the appropriate metadata up to the SnapBuild level, keeps better track of the build progress.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/move-snap-building-metadata-to-snapbuild into lp:launchpad.

=== 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-06 14:08:45 +0000
@@ -175,9 +175,11 @@
 
     failure_count = Int(name='failure_count', allow_none=False)
 
+    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, metadata, 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.metadata = metadata
         if build_request is not None:
             self.build_request_id = build_request.id
         self.status = BuildStatus.NEEDSBUILD
@@ -507,8 +510,11 @@
 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,
+            build_request=None, metadata=None):
         """See `ISnapBuildSet`."""
+        if not metadata:
+            metadata = {}
         store = IMasterStore(SnapBuild)
         build_farm_job = getUtility(IBuildFarmJobSource).new(
             SnapBuild.job_type, BuildStatus.NEEDSBUILD, date_created, None,
@@ -518,7 +524,7 @@
             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, 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-06 14:08:45 +0000
@@ -244,22 +244,22 @@
     @property
     def store_url(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("store_url")
+        return self.snapbuild.metadata.get("store_url")
 
     @store_url.setter
     def store_url(self, url):
         """See `ISnapStoreUploadJob`."""
-        self.metadata["store_url"] = url
+        self.snapbuild.metadata["store_url"] = url
 
     @property
     def store_revision(self):
         """See `ISnapStoreUploadJob`."""
-        return self.metadata.get("store_revision")
+        return self.snapbuild.metadata.get("store_revision")
 
     @store_revision.setter
     def store_revision(self, revision):
         """See `ISnapStoreUploadJob`."""
-        self.metadata["store_revision"] = revision
+        self.snapbuild.metadata["store_revision"] = revision
 
     # 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 +297,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.snapbuild.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 +313,14 @@
         """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.snapbuild.metadata:
+                self.snapbuild.metadata["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.snapbuild.metadata["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-06 14:08:45 +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.metadata)
             self.assertIsNone(job.store_url)
             self.assertEqual(
                 timedelta(seconds=expected_delay), job.retry_delay)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2018-10-15 14:44:25 +0000
+++ lib/lp/testing/factory.py	2018-12-06 14:08:45 +0000
@@ -4770,7 +4770,7 @@
                       archive=None, distroarchseries=None, pocket=None,
                       channels=None, date_created=DEFAULT,
                       status=BuildStatus.NEEDSBUILD, builder=None,
-                      duration=None, **kwargs):
+                      duration=None, metadata=None, **kwargs):
         """Make a new SnapBuild."""
         if requester is None:
             requester = self.makePerson()