launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:oci-multi-arch-upload into launchpad:master with ~pappacena/launchpad:oci-unify-request-builds as a prerequisite.

Commit message:
Upload multi-arch OCI images

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/391783
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:oci-multi-arch-upload into launchpad:master.

diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 8a9a31b..d5a23f1 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -2710,6 +2710,7 @@ public.account                          = SELECT
 public.archive                          = SELECT
 public.branch                           = SELECT
 public.builder                          = SELECT
+public.builderprocessor                 = SELECT
 public.buildfarmjob                     = SELECT, INSERT
 public.buildqueue                       = SELECT, INSERT, UPDATE
 public.distribution                     = SELECT
diff --git a/lib/lp/oci/interfaces/ociregistryclient.py b/lib/lp/oci/interfaces/ociregistryclient.py
index 184a93f..71af23a 100644
--- a/lib/lp/oci/interfaces/ociregistryclient.py
+++ b/lib/lp/oci/interfaces/ociregistryclient.py
@@ -54,3 +54,6 @@ class IOCIRegistryClient(Interface):
 
         :param build: The `IOCIRecipeBuild` to upload.
         """
+
+    def uploadManifestList(build_request):
+        """Upload the "fat manifest" which aggregates all platforms built."""
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 15fa5be..6ac7f1a 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -712,3 +712,8 @@ class OCIRecipeBuildRequest:
     @property
     def builds(self):
         return self.job.builds
+
+    def __eq__(self, other):
+        if not isinstance(self, self.__class__):
+            return False
+        return self.id == other.id
diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index 73fa4d6..c65aa6f 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -41,8 +41,12 @@ from lp.oci.interfaces.ociregistryclient import (
     OCIRegistryError,
     )
 from lp.services.database.enumcol import DBEnum
-from lp.services.database.interfaces import IStore
+from lp.services.database.interfaces import (
+    IMasterStore,
+    IStore,
+    )
 from lp.services.database.stormbase import StormBase
+from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import (
     EnumeratedSubclass,
     Job,
@@ -227,6 +231,36 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
         """See `IOCIRegistryUploadJob`."""
         self.json_data["errors"] = errors
 
+    @property
+    def is_multi_arch(self):
+        return len(self.build.getAllowedArchitectures()) > 1
+
+    def allBuildsUploaded(self, build_request):
+        """Returns True if all other builds already finished uploading.
+        False otherwise."""
+        builds = list(build_request.builds)
+        uploads_per_build = {i: list(i.registry_upload_jobs) for i in builds}
+        upload_jobs = sum(uploads_per_build.values(), [])
+
+        # Lock the Job rows, so no other job updates it's status until the
+        # end of this transation. This is done to avoid race conditions,
+        # where 2 upload jobs could be running simultaneously and none of them
+        # realises that is the last upload.
+        store = IMasterStore(builds[0])
+        placeholders = ', '.join('?' for _ in upload_jobs)
+        sql = (
+            "SELECT id, status FROM job WHERE id IN (%s) FOR UPDATE"
+            % placeholders)
+        store.execute(sql, [i.job_id for i in upload_jobs])
+
+        for build, upload_jobs in uploads_per_build.items():
+            has_finished_upload = any(
+                i.status == JobStatus.COMPLETED or i.job_id == self.job_id
+                for i in upload_jobs)
+            if not has_finished_upload:
+                return False
+        return True
+
     def run(self):
         """See `IRunnableJob`."""
         client = getUtility(IOCIRegistryClient)
@@ -235,6 +269,11 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
         try:
             try:
                 client.upload(self.build)
+                # The "allBuildsUploaded" call will lock, on the database,
+                # all upload jobs for update until this transaction finishes.
+                # So, make sure this is the last thing being done by this job.
+                if self.allBuildsUploaded(self.build.build_request):
+                    client.uploadManifestList(self.build.build_request)
             except OCIRegistryError as e:
                 self.error_summary = str(e)
                 self.errors = e.errors
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 0176a89..91776b5 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -15,6 +15,8 @@ from functools import partial
 import hashlib
 from io import BytesIO
 import json
+
+
 try:
     from json.decoder import JSONDecodeError
 except ImportError:
@@ -310,6 +312,21 @@ class OCIRegistryClient:
         elif len(exceptions) > 1:
             raise MultipleOCIRegistryError(exceptions)
 
+    @classmethod
+    def uploadManifestList(cls, build_request):
+        # XXX: Here, we need to do the upload of the multiple archs' manifests.
+        # In theory, we should do the following to upload the manifest list
+        # here:
+        # 1- For each build:
+        #   - Read the manifest.json, and get "Config" content. That's the
+        #     digest of that specific architecture manifest file.
+        #   - Update the list of manifests with this file's size, digest,
+        #     os, platform, etc.
+        # 2- Build the manifest list file content, with the list of
+        #     manifests created above in it.
+        # 3- Upload this new file to registry overriding manifest.json
+        pass
+
 
 class OCIRegistryAuthenticationError(Exception):
     def __init__(self, msg, http_error=None):
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
index fd3e0ff..3d90614 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildjob.py
@@ -8,6 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 from fixtures import FakeLogger
+from storm.store import Store
 from testtools.matchers import (
     Equals,
     Is,
@@ -16,9 +17,12 @@ from testtools.matchers import (
     MatchesStructure,
     )
 import transaction
+from zope.component import getUtility
 from zope.interface import implementer
+from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.oci.interfaces.ocirecipe import (
     OCI_RECIPE_ALLOW_CREATE,
     OCI_RECIPE_WEBHOOKS_FEATURE_FLAG,
@@ -27,6 +31,7 @@ from lp.oci.interfaces.ocirecipebuildjob import (
     IOCIRecipeBuildJob,
     IOCIRegistryUploadJob,
     )
+from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
 from lp.oci.interfaces.ociregistryclient import (
     BlobUploadFailed,
     IOCIRegistryClient,
@@ -42,7 +47,10 @@ from lp.services.features.testing import FeatureFixture
 from lp.services.job.runner import JobRunner
 from lp.services.webapp import canonical_url
 from lp.services.webhooks.testing import LogsScheduledWebhooks
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    admin_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.dbuser import dbuser
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.fixture import ZopeUtilityFixture
@@ -69,6 +77,7 @@ class FakeRegistryClient:
 
     def __init__(self):
         self.upload = FakeMethod()
+        self.uploadManifestList = FakeMethod()
 
 
 class FakeOCIBuildJob(OCIRecipeBuildJobDerived):
@@ -181,6 +190,54 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(ocibuild, ["Pending", "Uploaded"], logger)
 
+    def test_run_multiple_architectures(self):
+        logger = self.useFixture(FakeLogger())
+        i386 = getUtility(IProcessorSet).getByName("386")
+        amd64 = getUtility(IProcessorSet).getByName("amd64")
+        recipe = self.factory.makeOCIRecipe()
+        recipe.setProcessors([i386, amd64])
+        distroseries = self.factory.makeDistroSeries(
+            distribution=recipe.oci_project.distribution)
+        distro_i386 = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag="i386",
+            processor=i386)
+        distro_i386.addOrUpdateChroot(self.factory.makeLibraryFileAlias())
+        distro_amd64 = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag="amd64",
+            processor=amd64)
+        distro_amd64.addOrUpdateChroot(self.factory.makeLibraryFileAlias())
+
+        # Creates a build request with builds.
+        build_request = recipe.requestBuilds(recipe.owner)
+        with admin_logged_in():
+            jobs = getUtility(IOCIRecipeRequestBuildsJobSource).iterReady()
+            with dbuser(config.IOCIRecipeRequestBuildsJobSource.dbuser):
+                JobRunner(jobs).runAll()
+
+        builds = build_request.builds
+        self.assertEqual(2, builds.count())
+        self.assertEqual(builds[0].build_request, builds[1].build_request)
+
+        upload_jobs = []
+        for build in builds:
+            self.assertContentEqual([], build.registry_upload_jobs)
+            upload_jobs.append(OCIRegistryUploadJob.create(build))
+
+        client = FakeRegistryClient()
+        self.useFixture(ZopeUtilityFixture(client, IOCIRegistryClient))
+
+        with dbuser(config.IOCIRegistryUploadJobSource.dbuser):
+            JobRunner([upload_jobs[0]]).runAll()
+        self.assertEqual([((builds[0],), {})], client.upload.calls)
+        self.assertEqual([], client.uploadManifestList.calls)
+
+        with dbuser(config.IOCIRegistryUploadJobSource.dbuser):
+            JobRunner([upload_jobs[1]]).runAll()
+        self.assertEqual(
+            [((builds[0],), {}), ((builds[1],), {})], client.upload.calls)
+        self.assertEqual([((builds[1].build_request, ), {})],
+                         client.uploadManifestList.calls)
+
     def test_run_failed_registry_error(self):
         # A run that fails with a registry error sets the registry upload
         # status to FAILED, and stores the detailed errors.