Merge ~ilasc/launchpad:add-get-build-by-store-revision-to-snap into launchpad:master

[Ioana Lasc]

Add getBuildByStoreRevision to Snap

[Ioana Lasc (ilasc)]

For the query I tried using the _getBuilds method with a "filter_term = (SnapBuild.store_upload_revision == store_upload_revision)" but that will return an empty set, I assume because store_upload_revision is property and not a persisted attribute ?

[Colin Watson (cjwatson)]

My apologies for blithely saying this was just a simple query; I'd forgotten about the way in which store revisions are currently stored. This may be a bit more work than I initially thought. On the other hand: useful database engineering exercise, I guess! As penance, I've walked through my thought process in detail here.

You'll definitely need to find an alternative approach, as walking through all the builds for the snap recipe in Python will be much too slow for snap recipes with more than a few builds.

First, consider doing this in a single PostgreSQL query. This will be a bit fiddly because conceptually what you want is to join with `SnapBuildJob` but only consider those cases from the join where the `SnapBuildJob` is the one with the highest job ID for each snap recipe, and then look at the union of `SnapBuildJob.metadata` and `SnapBuild.store_upload_metadata` and extract the `store_revision` field from that: this emulates the behaviour of the `SnapBuild.store_upload_revision` property (drilling down into `SnapBuild.last_store_upload_job`, `SnapStoreUploadJob.store_revision`, and `SnapStoreUploadJob.store_metadata`). The tools you'd need for this would be PostgreSQL window functions (https://www.postgresql.org/docs/10/tutorial-window.html) and JSON functions (https://www.postgresql.org/docs/10/functions-json.html); a relevant example of using window functions is in `lp.scripts.garbo.SnapBuildJobPruner`.

Looking at this, though, I'm a bit concerned that it may not be possible to implement it efficiently with the current database schema, because PostgreSQL is going to have to walk through all the builds and jobs internally and extract all their JSON metadata fields, then cast them to `jsonb` (since these columns predate us being able to use PostgreSQL's `jsonb` type and are thus just `text`), concatenate `SnapBuildJob.metadata` and `SnapBuild.store_upload_metadata`, and extract the appropriate field from the result. I'm not convinced that it will be possible to construct an index that allows PostgreSQL to do that efficiently for snap recipes with large numbers of builds. It's probably worth having a go at constructing the appropriate query and throwing it at `EXPLAIN ANALYZE` on a database with a respectable number of builds for the relevant snap recipe to see if it can be indexed usefully, if nothing else because window functions are sometimes handy and it's a good opportunity to learn about them. However, there seems a good chance that this won't work out, so I'd advise timeboxing that exercise: no more than half a day or so.

So, the fallback option: let's consider denormalizing `SnapBuild.store_upload_revision` (that is, adding an explicit database column for it on `SnapBuild` that's maintained by either PostgreSQL trigger functions or more likely by Python code called from anywhere that might alter the result of the current `SnapBuild.store_upload_revision` property). We could then add an index on `SnapBuild (snap, store_upload_revision)` that would efficiently serve this query. Note that we'll need to backfill this column using a garbo job before it can be used. This approach seems much more likely to work, since it k...

review: Needs Fixing

[Ioana Lasc (ilasc)]

The work for this has been split into several tickets and MPs and will mark this as rejected. The new MP for this is: https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/407781

Unmerged commits

079a40b... by Ioana Lasc on 2021-08-17

Add getBuildByStoreRevision to Snap

diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 2445553..400b92e 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -529,6 +529,21 @@ class ISnapView(Interface):
 
     @call_with(user=REQUEST_USER)
     @operation_parameters(
+        store_upload_revision=Int(title="Store revision",
+                                  required=True))
+    @export_read_operation()
+    @operation_for_version("devel")
+    def getBuildByStoreRevision(store_upload_revision, user=None):
+        """Returns the build (if any) of that snap recipe
+            that has the given store_upload_revision.
+
+        :param store_upload_revision: A list of snap build request IDs.
+        :param user: The `IPerson` requesting this information.
+        :return: An 'ISnapBuild' or None.
+        """
+
+    @call_with(user=REQUEST_USER)
+    @operation_parameters(
         request_ids=List(
             title=_("A list of snap build request IDs."), value_type=Int(),
             required=False),
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 24aa2be..fa20509 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1111,6 +1111,15 @@ class Snap(Storm, WebhookTargetMixin):
 
         return result
 
+    def getBuildByStoreRevision(self, store_upload_revision):
+        results = list(Store.of(self).find(
+            SnapBuild,
+            SnapBuild.snap == self))
+        for build in results:
+            if build.store_upload_revision == store_upload_revision:
+                return build
+        return None
+
     @property
     def builds(self):
         """See `ISnap`."""
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 4d9c92f..b64ddc4 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -124,8 +124,8 @@ from lp.snappy.interfaces.snapbase import (
     )
 from lp.snappy.interfaces.snapbuild import (
     ISnapBuild,
-    ISnapBuildSet,
-    )
+    ISnapBuildSet, SnapBuildStoreUploadStatus,
+)
 from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource
 from lp.snappy.interfaces.snapjob import ISnapRequestBuildsJobSource
 from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
@@ -137,6 +137,9 @@ from lp.snappy.model.snap import (
 from lp.snappy.model.snapbuild import SnapFile
 from lp.snappy.model.snapbuildjob import SnapBuildJob
 from lp.snappy.model.snapjob import SnapJob
+from lp.snappy.tests.test_snapbuildjob import (
+    FakeSnapStoreClient,
+    run_isolated_jobs)
 from lp.testing import (
     admin_logged_in,
     ANONYMOUS,
@@ -1079,6 +1082,45 @@ class TestSnap(TestCaseWithFactory):
             snap.destroySelf()
         self.assertFalse(getUtility(ISnapSet).exists(owner, "condemned"))
 
+    def test_getBuildByStoreRevision(self):
+        snap1 = self.factory.makeSnap()
+        build = self.factory.makeSnapBuild(
+            snap=snap1,
+            status=BuildStatus.FULLYBUILT)
+        job = getUtility(ISnapStoreUploadJobSource).create(build)
+        client = FakeSnapStoreClient()
+        client.upload.result = (
+            "http://sca.example/dev/api/snaps/1/builds/1/status")
+        client.checkStatus.result = (
+            "http://sca.example/dev/click-apps/1/rev/1/", 1)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+        self.assertEqual(
+            SnapBuildStoreUploadStatus.UPLOADED, build.store_upload_status)
+        self.assertEqual(build.store_upload_revision, 1)
+        self.assertEqual(snap1.getBuildByStoreRevision(1), build)
+
+        # build & upload again, check revision
+        # and that we return the second build for revision 2
+        build2 = self.factory.makeSnapBuild(
+            snap=snap1,
+            status=BuildStatus.FULLYBUILT)
+        job = getUtility(ISnapStoreUploadJobSource).create(build2)
+        client = FakeSnapStoreClient()
+        client.upload.result = (
+            "http://sca.example/dev/api/snaps/1/builds/2/status")
+        client.checkStatus.result = (
+            "http://sca.example/dev/click-apps/1/rev/2/", 2)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+        self.assertEqual(
+            SnapBuildStoreUploadStatus.UPLOADED, build2.store_upload_status)
+        self.assertEqual(build2.store_upload_revision, 2)
+        self.assertEqual(snap1.getBuildByStoreRevision(2), build2)
+        self.assertEqual(snap1.getBuildByStoreRevision(1), build)
+
     def test_getBuildSummariesForSnapBuildIds(self):
         snap1 = self.factory.makeSnap()
         snap2 = self.factory.makeSnap()