Merge ~ilasc/launchpad:populate-store-upload-revision into launchpad:master

[Ioana Lasc]

Populate store_upload_revision

This populates the new DB column store_upload_revision on SnapBuild.
We're renaming the existent "store_upload_revision" property on SnapBuild to "store_upload_revision_property". We will be using this property to populate the new DB column "store_upload_revision" in PopulateSnapBuildStoreRevision.

This work will be followed by a second MP for LP-349 to start updating the new DB column "store_upload_revision" when uploading the snap to the store.

[Colin Watson (cjwatson)]

review: Needs Fixing

[Ioana Lasc (ilasc)]

Thanks Colin! That helped debug interactively and get to the bottom of it. It turns out the script was in fact getting triggered but it was failing with a "hidden" SilentLaunchpadScriptFailure caused by garbo_daily not having enough permissions to update SnapBuild.

In terms of breaking API clients that expect the `store_upload_revision` attribute of a snap build to contain useful data, indeed I believe we have a problem there:
I might be misunderstanding this but it looks like we now have a collision between the DB column name and the property that I didn't realize was exposed through the API before we landed the DB patch. I think we won't be able to define a getter with the same name and have it exposed through API same as before along the lines of:

    def store_upload_revision(self):
        if self.store_upload_revision is None:
            return self._store_upload_revision

because that will interfere with the query to populate the DB (the new getter will not return None when it should so we can backfill, it would return the value in the former property).

I could propose a patch to alter the name of the currently empty column from "store_upload_revision" to "store_revision".

It's entirely possible I misunderstood what's possible / what's required (for backward compatibility) here though...

[Colin Watson (cjwatson)]

There's certainly no need to rename the DB column; for this sort of thing it's always easier to handle any necessary renamings in Python, and there are various primitives available for that. The underlying DB column name isn't required to match the name of the Storm column (it's set using the 'name' parameter to the Storm variable constructor), and there are quite a few cases where we have something like this:

    _foo = Int(name='foo')

    @property
    def foo(self):
        if self._foo is not None:
            return self._foo
        else:
            # compute and return something

Of course anything that sets the actual DB column needs to use the Storm variable name, unless we also use a setter property (which can sometimes be useful), and Storm queries typically need to be written to refer to the name of the Storm variable rather than the wrapping property. The important thing is that any names used in API-exported attributes need to be defined in a way that preserves compatibility.

Happy to help with getting the details of this right tomorrow if you need it.

[Ioana Lasc (ilasc)]

Thanks Colin! Updates done, MP ready for review.

[Colin Watson (cjwatson)]

It's a bit strange to land the backfill job before starting to populate the new column in the "ordinary" inline way (in this case having `SnapStoreUploadJob` populate it) - we'd normally do it the other way round. I don't think it's actually a problem here, though, since everything should still catch up eventually.

When you do start populating the new column in `SnapStoreUploadJob`, be careful to run your garbo job tests; it looks like they'll probably need rearranging at that point to manually simulate an old job that's run but that hasn't populated the new column.

review: Approve

[Colin Watson (cjwatson)]

review: Approve

diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 7f39796..8567419 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -2530,6 +2530,7 @@ public.revisionauthor                   = SELECT, UPDATE
 public.revisioncache                    = SELECT, DELETE
 public.snap                             = SELECT, UPDATE
 public.snapbase                         = SELECT
+public.snapbuild                        = SELECT, UPDATE
 public.snapfile                         = SELECT, DELETE
 public.snappydistroseries               = SELECT
 public.snappyseries                     = SELECT
diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
index fba6c29..f416b3f 100644
--- a/lib/lp/scripts/garbo.py
+++ b/lib/lp/scripts/garbo.py
@@ -37,6 +37,7 @@ from storm.expr import (
     Except,
     In,
     Join,
+    LeftJoin,
     Max,
     Min,
     Or,
@@ -134,8 +135,14 @@ from lp.services.verification.model.logintoken import LoginToken
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webhooks.interfaces import IWebhookJobSource
 from lp.services.webhooks.model import WebhookJob
-from lp.snappy.model.snapbuild import SnapFile
-from lp.snappy.model.snapbuildjob import SnapBuildJobType
+from lp.snappy.model.snapbuild import (
+    SnapBuild,
+    SnapFile,
+    )
+from lp.snappy.model.snapbuildjob import (
+    SnapBuildJob,
+    SnapBuildJobType,
+    )
 from lp.soyuz.enums import ArchiveSubscriberStatus
 from lp.soyuz.interfaces.publishing import active_publishing_status
 from lp.soyuz.model.archive import Archive
@@ -1717,6 +1724,47 @@ class ArchiveAuthTokenDeactivator(BulkPruner):
         transaction.commit()
 
 
+class PopulateSnapBuildStoreRevision(TunableLoop):
+    """Populates snapbuild.store_revision if not set."""
+
+    maximum_chunk_size = 5000
+
+    def __init__(self, log, abort_time=None):
+        super(PopulateSnapBuildStoreRevision, self).__init__(log, abort_time)
+        self.start_at = 1
+        self.store = IMasterStore(SnapBuild)
+
+    def findSnapBuilds(self):
+        origin = [
+            SnapBuild,
+            LeftJoin(
+                SnapBuildJob,
+                SnapBuildJob.snapbuild_id == SnapBuild.id),
+            LeftJoin(
+                Job,
+                Job.id == SnapBuildJob.job_id)
+            ]
+        builds = self.store.using(*origin).find(
+            SnapBuild,
+            SnapBuild.id >= self.start_at,
+            SnapBuild._store_upload_revision == None,
+            SnapBuildJob.job_type == SnapBuildJobType.STORE_UPLOAD,
+            Job._status == JobStatus.COMPLETED)
+
+        return builds.order_by(SnapBuild.id)
+
+    def isDone(self):
+        return self.findSnapBuilds().is_empty()
+
+    def __call__(self, chunk_size):
+        builds = list(self.findSnapBuilds()[:chunk_size])
+        for build in builds:
+            build._store_upload_revision = build.store_upload_revision
+        if len(builds):
+            self.start_at = builds[-1].id + 1
+        transaction.commit()
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -2008,6 +2056,7 @@ class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
         OCIFilePruner,
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
+        PopulateSnapBuildStoreRevision,
         POTranslationPruner,
         PreviewDiffPruner,
         ProductVCSPopulator,
diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index 0d34fce..3a43045 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -97,6 +97,7 @@ from lp.scripts.garbo import (
     load_garbo_job_state,
     LoginTokenPruner,
     OpenIDConsumerAssociationPruner,
+    PopulateSnapBuildStoreRevision,
     ProductVCSPopulator,
     save_garbo_job_state,
     UnusedPOTMsgSetPruner,
@@ -130,11 +131,17 @@ from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.model.logintoken import LoginToken
 from lp.services.worlddata.interfaces.language import ILanguageSet
 from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
+from lp.snappy.interfaces.snapbuildjob import ISnapStoreUploadJobSource
+from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
 from lp.snappy.model.snapbuild import SnapFile
 from lp.snappy.model.snapbuildjob import (
     SnapBuildJob,
     SnapStoreUploadJob,
     )
+from lp.snappy.tests.test_snapbuildjob import (
+    FakeSnapStoreClient,
+    run_isolated_jobs,
+    )
 from lp.soyuz.enums import (
     ArchiveSubscriberStatus,
     PackagePublishingStatus,
@@ -153,7 +160,11 @@ from lp.testing import (
     TestCase,
     TestCaseWithFactory,
     )
-from lp.testing.dbuser import switch_dbuser
+from lp.testing.dbuser import (
+    dbuser,
+    switch_dbuser,
+    )
+from lp.testing.fixture import ZopeUtilityFixture
 from lp.testing.layers import (
     DatabaseLayer,
     LaunchpadScriptLayer,
@@ -1978,6 +1989,77 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         self.assertIsNotNone(token.date_deactivated)
         self.assertEmailQueueLength(0)
 
+    def test_PopulateSnapBuildStoreRevision(self):
+        switch_dbuser('testadmin')
+        snap1 = self.factory.makeSnap()
+        build1 = self.factory.makeSnapBuild(
+            snap=snap1,
+            status=BuildStatus.FULLYBUILT)
+
+        # test that build1 does not get picked up
+        # as it is a build without a store upload
+        populator = PopulateSnapBuildStoreRevision(None)
+        rs = populator.findSnapBuilds()
+        self.assertEqual(0, rs.count())
+
+        # Upload build
+        job = getUtility(ISnapStoreUploadJobSource).create(build1)
+        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])
+
+        # this mimics what we have in the DB right now:
+        # uploaded snaps that do not have the new DB column
+        # _store_upload_revision populated yet
+        populator = PopulateSnapBuildStoreRevision(None)
+        filter = populator.findSnapBuilds()
+        build1 = removeSecurityProxy(build1)
+        self.assertEqual(1, filter.count())
+        self.assertEqual(build1, filter.one())
+        self.assertEqual(build1._store_upload_revision, None)
+
+        # run the garbo job and verify _store_upload_revision
+        # is not populated with the value assigned to the build during upload
+        self.runDaily()
+        switch_dbuser('testadmin')
+        self.assertEqual(build1._store_upload_revision, 1)
+
+        # Tests that of all builds for the same snap only those that have
+        # been uploaded to the store will get
+        # their new _store_upload_revision DB column updated
+        build2 = self.factory.makeSnapBuild(
+            snap=snap1,
+            status=BuildStatus.FULLYBUILT)
+        build3 = 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/", 1)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+
+        populator = PopulateSnapBuildStoreRevision(None)
+        filter = populator.findSnapBuilds()
+        self.assertEqual(1, filter.count())
+        self.assertEqual(build2, filter.one())
+
+        self.runDaily()
+        switch_dbuser('testadmin')
+        build2 = removeSecurityProxy(build2)
+        self.assertEqual(build2._store_upload_revision, 1)
+        build3 = removeSecurityProxy(build3)
+        self.assertIsNone(build3._store_upload_revision)
+
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer
diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
index ca5285a..23864c9 100644
--- a/lib/lp/snappy/model/snapbuild.py
+++ b/lib/lp/snappy/model/snapbuild.py
@@ -191,6 +191,8 @@ class SnapBuild(PackageBuildMixin, Storm):
 
     failure_count = Int(name='failure_count', allow_none=False)
 
+    _store_upload_revision = Int(name='store_upload_revision', allow_none=True)
+
     store_upload_metadata = JSON('store_upload_json_data', allow_none=True)
 
     def __init__(self, build_farm_job, requester, snap, archive,
@@ -527,8 +529,18 @@ class SnapBuild(PackageBuildMixin, Storm):
 
     @property
     def store_upload_revision(self):
-        job = self.last_store_upload_job
-        return job and job.store_revision
+        # We are now persisting the revision assigned by the store
+        # on package upload in the new DB column _store_upload_revision.
+        # We backfill _store_upload_revision with
+        # PopulateSnapBuildStoreRevision.
+        # If the persisted field (_store_upload_revision)
+        # is not populated yet we return the old way of computing this
+        # value so that we don't break existing API clients.
+        if self._store_upload_revision:
+            return self._store_upload_revision
+        else:
+            job = self.last_store_upload_job
+            return job and job.store_revision
 
     @property
     def store_upload_error_message(self):