launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:oci-upload-prevent-superseded into launchpad:master with ~pappacena/launchpad:oci-upload-manifest-upsert as a prerequisite.

Commit message:
Preventing OCI registry upload when we already have another build goind on for the same architecture

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

diff --git a/lib/lp/oci/interfaces/ocirecipebuild.py b/lib/lp/oci/interfaces/ocirecipebuild.py
index 3273271..f005e95 100644
--- a/lib/lp/oci/interfaces/ocirecipebuild.py
+++ b/lib/lp/oci/interfaces/ocirecipebuild.py
@@ -94,6 +94,13 @@ class OCIRecipeBuildRegistryUploadStatus(EnumeratedType):
         This OCI build was successfully uploaded to a registry.
         """)
 
+    SUPERSEDED = Item("""
+        Superseded
+
+        The upload has been canceled because another build will upload a
+        more recent version.
+    """)
+
 
 class IOCIRecipeBuildView(IPackageBuild):
     """`IOCIRecipeBuild` attributes that require launchpad.View permission."""
@@ -207,6 +214,13 @@ class IOCIRecipeBuildView(IPackageBuild):
         value_type=Dict(key_type=TextLine()),
         required=False, readonly=True))
 
+    def hasMoreRecentBuild():
+        """Checks if this recipe has a more recent build currently building or
+        already built for the same processor.
+
+        :return: True if another build superseded this one.
+        """
+
 
 class IOCIRecipeBuildEdit(Interface):
     """`IOCIRecipeBuild` attributes that require launchpad.Edit permission."""
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index eda6fa3..7c003bc 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -437,6 +437,8 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
 
     @property
     def registry_upload_status(self):
+        if self.status == BuildStatus.SUPERSEDED:
+            return OCIRecipeBuildRegistryUploadStatus.SUPERSEDED
         job = self.last_registry_upload_job
         if job is None or job.job.status == JobStatus.SUSPENDED:
             return OCIRecipeBuildRegistryUploadStatus.UNSCHEDULED
@@ -479,6 +481,20 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
                 "uploaded.")
         getUtility(IOCIRegistryUploadJobSource).create(self)
 
+    def hasMoreRecentBuild(self):
+        """See `IOCIRecipeBuild`."""
+        status = [
+            BuildStatus.NEEDSBUILD, BuildStatus.FULLYBUILT,
+            BuildStatus.BUILDING, BuildStatus.UPLOADING
+        ]
+        recent_builds = IStore(self).find(
+            OCIRecipeBuild,
+            OCIRecipeBuild.recipe == self.recipe,
+            OCIRecipeBuild.processor == self.processor,
+            OCIRecipeBuild.status.is_in(status),
+            OCIRecipeBuild.date_created > self.date_created)
+        return not recent_builds.is_empty()
+
 
 @implementer(IOCIRecipeBuildSet)
 class OCIRecipeBuildSet(SpecificBuildFarmJobSourceMixin):
diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index 2b920cd..2567006 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -41,6 +41,7 @@ from tenacity import (
     )
 from zope.interface import implementer
 
+from lp.buildmaster.enums import BuildStatus
 from lp.oci.interfaces.ociregistryclient import (
     BlobUploadFailed,
     IOCIRegistryClient,
@@ -342,6 +343,8 @@ class OCIRegistryClient:
         :raises ManifestUploadFailed: If the final registry manifest fails to
                                       upload due to network or validity.
         """
+        if cls.updateSupersededBuilds(build):
+            return
         # Get the required metadata files
         manifest = cls._getJSONfile(build.manifest)
         digests_list = cls._getJSONfile(build.digests)
@@ -420,10 +423,30 @@ class OCIRegistryClient:
         return current_manifest
 
     @classmethod
+    def updateSupersededBuilds(cls, build):
+        """Checks if the given build was superseded by another build,
+        updating it's status in case it should have been superseded.
+
+        :return: True if the build was superseded.
+        """
+        if build.status == BuildStatus.SUPERSEDED:
+            return True
+        if build.hasMoreRecentBuild():
+            build.updateStatus(BuildStatus.SUPERSEDED)
+            return True
+        return False
+
+    @classmethod
     def uploadManifestList(cls, build_request, uploaded_builds):
         """Uploads to all build_request.recipe.push_rules the manifest list
         for the builds in the given build_request.
         """
+        # First, double check that the builds that will be updated in the
+        # manifest files were not superseded by newer builds.
+        uploaded_builds = [build for build in uploaded_builds
+                           if not cls.updateSupersededBuilds(build)]
+        if not uploaded_builds:
+            return
         for push_rule in build_request.recipe.push_rules:
             http_client = RegistryHTTPClient.getInstance(push_rule)
             multi_manifest_content = cls.makeMultiArchManifest(
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 05160fd..45b4e20 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -8,6 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 
 import base64
+from datetime import timedelta
 from functools import partial
 import io
 import json
@@ -38,7 +39,11 @@ import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
+from lp.oci.interfaces.ocirecipebuild import (
+    OCIRecipeBuildRegistryUploadStatus,
+    )
 from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
 from lp.oci.interfaces.ociregistryclient import (
     BlobUploadFailed,
@@ -55,6 +60,7 @@ from lp.oci.model.ociregistryclient import (
     RegistryHTTPClient,
     )
 from lp.oci.tests.helpers import OCIConfigHelperMixin
+from lp.registry.interfaces.series import SeriesStatus
 from lp.services.compat import mock
 from lp.testing import TestCaseWithFactory
 from lp.testing.fixture import ZopeUtilityFixture
@@ -212,6 +218,29 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
         }))
 
     @responses.activate
+    def test_upload_ignores_superseded_builds(self):
+        recipe = self.build.recipe
+        processor = self.build.processor
+        distribution = recipe.oci_project.distribution
+        distroseries = self.factory.makeDistroSeries(
+            distribution=distribution, status=SeriesStatus.CURRENT)
+        distro_arch_series = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag=processor.name,
+            processor=processor)
+
+        # Creates another build, more recent.
+        self.factory.makeOCIRecipeBuild(
+            recipe=recipe, distro_arch_series=distro_arch_series,
+            date_created=self.build.date_created + timedelta(seconds=1))
+
+        self.client.upload(self.build)
+        self.assertEqual(BuildStatus.SUPERSEDED, self.build.status)
+        self.assertEqual(
+            OCIRecipeBuildRegistryUploadStatus.SUPERSEDED,
+            self.build.registry_upload_status)
+        self.assertEqual(0, len(responses.calls))
+
+    @responses.activate
     def test_upload_formats_credentials(self):
         self._makeFiles()
         _upload_fixture = self.useFixture(MockPatch(
@@ -542,6 +571,31 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, SpyProxyCallsMixin,
                     MatchesStructure(errors=Is(None))))))
 
     @responses.activate
+    def test_multi_arch_manifest_upload_skips_superseded_builds(self):
+        recipe = self.build.recipe
+        processor = self.build.processor
+        distribution = recipe.oci_project.distribution
+        distroseries = self.factory.makeDistroSeries(
+            distribution=distribution, status=SeriesStatus.CURRENT)
+        distro_arch_series = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag=processor.name,
+            processor=processor)
+
+        # Creates another build for the same arch and recipe, more recent.
+        self.factory.makeOCIRecipeBuild(
+            recipe=recipe, distro_arch_series=distro_arch_series,
+            date_created=self.build.date_created + timedelta(seconds=1))
+
+        build_request = OCIRecipeBuildRequest(recipe, -1)
+        self.client.uploadManifestList(build_request, [self.build])
+
+        self.assertEqual(BuildStatus.SUPERSEDED, self.build.status)
+        self.assertEqual(
+            OCIRecipeBuildRegistryUploadStatus.SUPERSEDED,
+            self.build.registry_upload_status)
+        self.assertEqual(0, len(responses.calls))
+
+    @responses.activate
     def test_multi_arch_manifest_upload_new_manifest(self):
         """Ensure that multi-arch manifest upload works and tags correctly
         the uploaded image."""