Merge ~twom/launchpad:oci-fix-partial-upload-status-being-wrong into launchpad:master

[Tom Wardill]

Use a StatusSet for registry upload

We need to keep track of partial upload status in a set of builds.

Do that using a different enum.

[Thiago F. Pappacena (pappacena)]

LGTM

review: Approve

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/oci/browser/ocirecipe.py b/lib/lp/oci/browser/ocirecipe.py
index eb62596..bc978e7 100644
--- a/lib/lp/oci/browser/ocirecipe.py
+++ b/lib/lp/oci/browser/ocirecipe.py
@@ -333,7 +333,7 @@ class OCIRecipeView(LaunchpadView):
         status = removeSecurityProxy(
                     recipe_set.getStatusSummaryForBuilds([build_job]))
         # Add the registry job status
-        status["upload_scheduled"] = upload_status != unscheduled_upload
+        status["upload_requested"] = upload_status != unscheduled_upload
         status["upload"] = upload_status
         status["date"] = build_job.date
         status["date_estimated"] = build_job.estimate
diff --git a/lib/lp/oci/browser/tests/test_ocirecipe.py b/lib/lp/oci/browser/tests/test_ocirecipe.py
index b51097e..a4308ea 100644
--- a/lib/lp/oci/browser/tests/test_ocirecipe.py
+++ b/lib/lp/oci/browser/tests/test_ocirecipe.py
@@ -1525,6 +1525,7 @@ class TestOCIRecipeView(BaseTestOCIRecipeView):
             When requested
             When complete
             There are some builds waiting to be built.
+            Waiting for builds to start.
             a moment ago
             in .* \(estimated\)
             """, self.getMainText(recipe))
diff --git a/lib/lp/oci/interfaces/ocirecipebuild.py b/lib/lp/oci/interfaces/ocirecipebuild.py
index b9b64ea..22799e7 100644
--- a/lib/lp/oci/interfaces/ocirecipebuild.py
+++ b/lib/lp/oci/interfaces/ocirecipebuild.py
@@ -13,6 +13,7 @@ __all__ = [
     'IOCIRecipeBuild',
     'IOCIRecipeBuildSet',
     'OCIRecipeBuildRegistryUploadStatus',
+    'OCIRecipeBuildSetRegistryUploadStatus',
     ]
 
 from lazr.enum import (
@@ -102,6 +103,44 @@ class OCIRecipeBuildRegistryUploadStatus(EnumeratedType):
     """)
 
 
+class OCIRecipeBuildSetRegistryUploadStatus(EnumeratedType):
+    """OCI build registry upload status type
+
+    OCI builds may be uploaded to a registry. This represents the state of
+    that process.
+    """
+
+    UNSCHEDULED = Item("""
+        Unscheduled
+
+        No upload of these OCI builds to a registry is scheduled.
+        """)
+
+    PENDING = Item("""
+        Pending
+
+        These OCI builds are queued for upload to a registry.
+        """)
+
+    FAILEDTOUPLOAD = Item("""
+        Failed to upload
+
+        The last attempt to upload these OCI builds to a registry failed.
+        """)
+
+    UPLOADED = Item("""
+        Uploaded
+
+        These OCI builds were successfully uploaded to a registry.
+        """)
+
+    PARTIAL = Item("""
+        Partial
+
+        Some OCI builds have uploaded to a registry.
+    """)
+
+
 class IOCIRecipeBuildView(IPackageBuild):
     """`IOCIRecipeBuild` attributes that require launchpad.View permission."""
 
diff --git a/lib/lp/oci/model/ocirecipejob.py b/lib/lp/oci/model/ocirecipejob.py
index ac885d7..dd57542 100644
--- a/lib/lp/oci/model/ocirecipejob.py
+++ b/lib/lp/oci/model/ocirecipejob.py
@@ -33,7 +33,10 @@ from zope.interface import (
 from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
-from lp.oci.interfaces.ocirecipebuild import OCIRecipeBuildRegistryUploadStatus
+from lp.oci.interfaces.ocirecipebuild import (
+    OCIRecipeBuildRegistryUploadStatus,
+    OCIRecipeBuildSetRegistryUploadStatus,
+    )
 from lp.oci.interfaces.ocirecipejob import (
     IOCIRecipeJob,
     IOCIRecipeRequestBuildsJob,
@@ -288,41 +291,63 @@ class OCIRecipeRequestBuildsJob(OCIRecipeJobDerived):
     def addUploadedManifest(self, build_id, manifest_info):
         self.metadata["uploaded_manifests"][int(build_id)] = manifest_info
 
+    @property
     def build_status(self):
+        builds = self.builds
         # This just returns a dict, but Zope is really helpful here
         status = removeSecurityProxy(
                 getUtility(IOCIRecipeSet).getStatusSummaryForBuilds(
-                    list(self.builds)))
+                    list(builds)))
 
         # This has a really long name!
-        statusEnum = OCIRecipeBuildRegistryUploadStatus
+        singleStatus = OCIRecipeBuildRegistryUploadStatus
+        setStatus = OCIRecipeBuildSetRegistryUploadStatus
 
         # Set the pending upload status if either we're not done uploading,
         # or there was no upload requested in the first place (no push rules)
         if status['status'] == BuildSetStatus.FULLYBUILT:
             upload_status = [
-                (x.registry_upload_status == statusEnum.UPLOADED or
-                 x.registry_upload_status == statusEnum.UNSCHEDULED)
+                (x.registry_upload_status == singleStatus.UPLOADED or
+                 x.registry_upload_status == singleStatus.UNSCHEDULED)
                 for x in status['builds']]
             if not all(upload_status):
                 status['status'] = BuildSetStatus.FULLYBUILT_PENDING
 
-        # Add a flag for if we're expecting a registry upload
-        status['upload_scheduled'] = any(
-            x.registry_upload_status != statusEnum.UNSCHEDULED
-            for x in status['builds'])
-
-        # Set the equivalent of BuildSetStatus, but for registry upload
-        # If any of the builds have failed to upload
-        if any(x.registry_upload_status == statusEnum.FAILEDTOUPLOAD
-               for x in status['builds']):
-            status['upload'] = statusEnum.FAILEDTOUPLOAD
-        # If any of the builds are still waiting to upload
-        elif any(x.registry_upload_status == statusEnum.PENDING
-                 for x in status['builds']):
-            status['upload'] = statusEnum.PENDING
+        # Are we expecting an upload to be or to have been attempted?
+        # This is slightly complicated as the upload depends on the push
+        # rules at the time of build completion
+        upload_requested = False
+        # If there's an upload job for any of the builds, we have
+        # requested an upload
+        if any(x.last_registry_upload_job for x in builds):
+            upload_requested = True
+        # If all of the builds haven't finished, but the recipe currently
+        # has push rules specified, then we will attempt an upload
+        # in the future
+        if any(not x.date_finished and x.recipe.can_upload_to_registry
+               for x in builds):
+            upload_requested = True
+        status['upload_requested'] = upload_requested
+
+        # Convert the set of registry statuses into a single line
+        # for display
+        upload_status = [x.registry_upload_status for x in builds]
+        # Any of the builds failed
+        if any(x == singleStatus.FAILEDTOUPLOAD for x in upload_status):
+            status['upload'] = setStatus.FAILEDTOUPLOAD
+        # All of the builds uploaded
+        elif all(x == singleStatus.UPLOADED for x in upload_status):
+            status['upload'] = setStatus.UPLOADED
+        # All of the builds are yet to attempt an upload
+        elif all(x == singleStatus.UNSCHEDULED for x in upload_status):
+            status['upload'] = setStatus.UNSCHEDULED
+        # Any of the builds have uploaded. Set after 'all of the builds'
+        # have uploaded.
+        elif any(x == singleStatus.UPLOADED for x in upload_status):
+            status['upload'] = setStatus.PARTIAL
+        # And if it's none of the above, we're waiting
         else:
-            status['upload'] = statusEnum.UPLOADED
+            status['upload'] = setStatus.PENDING
 
         # Get the longest date and whether any of them are estimated
         # for the summary of the builds
diff --git a/lib/lp/oci/templates/ocirecipe-index.pt b/lib/lp/oci/templates/ocirecipe-index.pt
index 569ea43..e33952a 100644
--- a/lib/lp/oci/templates/ocirecipe-index.pt
+++ b/lib/lp/oci/templates/ocirecipe-index.pt
@@ -145,12 +145,13 @@
 
             </td>
             <td>
-              <tal:registry-upload tal:condition="build_status/upload_scheduled">
+
+              <tal:registry-upload tal:condition="build_status/upload_requested">
                 <span tal:content="build_status/upload/title" />
               </tal:registry-upload>
-              <tal:registry-upload tal:condition="not:build_status/upload_scheduled">
-                <span tal:condition="python: 'FULLYBUILT' in build_status['status'].title">No registry upload requested.</span>
-                <span tal:condition="python: 'FAILEDTOBUILD' in build_status['status'].title">No registry upload requested.</span>
+              <tal:registry-upload tal:condition="not:build_status/upload_requested">
+                <span tal:condition="build_status/status/enumvalue:NEEDSBUILD">Waiting for builds to start.</span>
+                <span tal:condition="not: build_status/status/enumvalue:NEEDSBUILD">No registry upload requested.</span>
               </tal:registry-upload>
             </td>
             <td>
@@ -183,7 +184,7 @@
               </ul>
             </td>
             <td>
-              <ul tal:condition="build_status/upload_scheduled" tal:repeat="build build_request/builds">
+              <ul tal:condition="build_status/upload_requested" tal:repeat="build build_request/builds">
                 <li  style="padding-left: 22px;">
                   <strong><a class="sprite distribution"
                     tal:define="processor build/processor"
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
index 0ff2938..95718bb 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildjob.py
@@ -7,14 +7,8 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 
-from datetime import (
-    datetime,
-    timedelta,
-    )
 import os
 import signal
-import threading
-import time
 
 from fixtures import FakeLogger
 from storm.locals import Store
@@ -22,7 +16,6 @@ from testtools.matchers import (
     Equals,
     MatchesDict,
     MatchesListwise,
-    MatchesSetwise,
     MatchesStructure,
     )
 import transaction
@@ -54,7 +47,6 @@ from lp.oci.model.ocirecipebuildjob import (
 from lp.services.compat import mock
 from lp.services.config import config
 from lp.services.database.locking import (
-    AdvisoryLockHeld,
     LockType,
     try_advisory_lock,
     )
diff --git a/lib/lp/oci/tests/test_ocirecipejob.py b/lib/lp/oci/tests/test_ocirecipejob.py
new file mode 100644
index 0000000..962d442
--- /dev/null
+++ b/lib/lp/oci/tests/test_ocirecipejob.py
@@ -0,0 +1,80 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for classes in OCIRecipeJob."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+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.ocirecipe import (
+    OCI_RECIPE_ALLOW_CREATE
+    )
+from lp.oci.interfaces.ocirecipebuild import (
+    OCIRecipeBuildSetRegistryUploadStatus,
+    )
+from lp.oci.interfaces.ocirecipebuildjob import IOCIRegistryUploadJobSource
+from lp.oci.interfaces.ocirecipejob import IOCIRecipeRequestBuildsJobSource
+from lp.registry.interfaces.series import SeriesStatus
+from lp.services.features.testing import FeatureFixture
+from lp.services.job.interfaces.job import JobStatus
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestOCIRecipeRequestBuildsJob(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestOCIRecipeRequestBuildsJob, self).setUp()
+        self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+
+    def getDistroArchSeries(self, distroseries, proc_name="386",
+                            arch_tag="i386"):
+        processor = getUtility(IProcessorSet).getByName(proc_name)
+
+        das = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag=arch_tag,
+            processor=processor)
+        fake_chroot = self.factory.makeLibraryFileAlias(
+            filename="fake_chroot.tar.gz", db_only=True)
+        das.addOrUpdateChroot(fake_chroot)
+        return das
+
+    def test_partial_on_failure(self):
+        ocirecipe = removeSecurityProxy(self.factory.makeOCIRecipe(
+            require_virtualized=False))
+        owner = ocirecipe.owner
+        distro = ocirecipe.oci_project.distribution
+        series = self.factory.makeDistroSeries(
+            distribution=distro, status=SeriesStatus.CURRENT)
+        self.getDistroArchSeries(series, "386", "386")
+        self.getDistroArchSeries(series, "hppa", "hppa")
+        job = getUtility(IOCIRecipeRequestBuildsJobSource).create(
+            ocirecipe, owner)
+
+        with person_logged_in(job.requester):
+            builds = ocirecipe.requestBuildsFromJob(
+                job.requester, build_request=job.build_request)
+            removeSecurityProxy(job).builds = builds
+
+        builds[0].updateStatus(BuildStatus.FAILEDTOBUILD)
+        builds[1].updateStatus(BuildStatus.FULLYBUILT)
+        upload_job = getUtility(IOCIRegistryUploadJobSource).create(builds[1])
+        removeSecurityProxy(upload_job).job._status = JobStatus.COMPLETED
+
+        status = job.build_status
+
+        self.assertTrue(status['upload_requested'])
+        self.assertEqual(
+            OCIRecipeBuildSetRegistryUploadStatus.PARTIAL,
+            status['upload'])