Merge ~twom/launchpad:oci-file-upload-deduplication into launchpad:master

[Tom Wardill]

Reuse existing image layers in new uploads

Rather than uploading each layer for each image to the librarian, reuse any that are existing.

This could introduce a race condition with garbage collection, but as we don't currently have any, it's safe for now, but should be considered in the design for garbo.

[Thiago F. Pappacena (pappacena)]

Looks good!

review: Approve

[Ioana Lasc (ilasc)]

All makes sense to me and looks good code-wise.

review: Approve

[Colin Watson (cjwatson)]

review: Approve

59920d5... by Tom Wardill on 2020-05-14

Drop security.py declaration in favour of allow

296522e... by Tom Wardill on 2020-05-14

Fix comment

diff --git a/lib/lp/archiveuploader/ocirecipeupload.py b/lib/lp/archiveuploader/ocirecipeupload.py
index c3db6cb..3ace0da 100644
--- a/lib/lp/archiveuploader/ocirecipeupload.py
+++ b/lib/lp/archiveuploader/ocirecipeupload.py
@@ -17,6 +17,7 @@ from zope.component import getUtility
 
 from lp.archiveuploader.utils import UploadError
 from lp.buildmaster.enums import BuildStatus
+from lp.oci.interfaces.ocirecipebuild import IOCIFileSet
 from lp.services.helpers import filenameToContentType
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 
@@ -62,6 +63,17 @@ class OCIRecipeUpload:
                         "{}.tar.gz".format(layer_id)
                     )
                     self.logger.debug("Layer path: {}".format(layer_path))
+                    # If the file is already in the librarian,
+                    # we can just reuse it.
+                    existing_file = getUtility(IOCIFileSet).getByLayerDigest(
+                        digest)
+                    # XXX 2020-05-14 twom This will need to respect restricted
+                    # when we do private builds.
+                    if existing_file:
+                        build.addFile(
+                            existing_file.library_file,
+                            layer_file_digest=digest)
+                        continue
                     if not os.path.exists(layer_path):
                         raise UploadError(
                             "Missing layer file: {}.".format(layer_id))
diff --git a/lib/lp/archiveuploader/tests/test_ocirecipeupload.py b/lib/lp/archiveuploader/tests/test_ocirecipeupload.py
index bae4f4a..75e2455 100644
--- a/lib/lp/archiveuploader/tests/test_ocirecipeupload.py
+++ b/lib/lp/archiveuploader/tests/test_ocirecipeupload.py
@@ -113,3 +113,31 @@ class TestOCIRecipeUploads(TestUploadProcessorBase):
             "ERROR Missing layer file: layer_2.",
             self.log.getLogBuffer())
         self.assertFalse(self.build.verifySuccessfulUpload())
+
+    def test_reuse_existing_file(self):
+        # The digests.json specifies a file that already exists in the
+        # librarian, but not on disk
+        self.assertFalse(self.build.verifySuccessfulUpload())
+        del get_property_cache(self.build).manifest
+        del get_property_cache(self.build).digests
+        upload_dir = os.path.join(
+            self.incoming_folder, "test", str(self.build.id), "ubuntu")
+        write_file(os.path.join(upload_dir, "layer_1.tar.gz"), b"layer_1")
+        write_file(os.path.join(upload_dir, "manifest.json"), b"manifest")
+        write_file(
+            os.path.join(upload_dir, "digests.json"), json.dumps(self.digests))
+
+        # create the existing file
+        self.switchToAdmin()
+        layer_2 = self.factory.makeOCIFile(layer_file_digest="digest_2")
+        Store.of(layer_2.build).flush()
+        self.switchToUploader()
+
+        handler = UploadHandler.forProcessor(
+            self.uploadprocessor, self.incoming_folder, "test", self.build)
+        result = handler.processOCIRecipe(self.log)
+        self.assertEqual(
+            UploadStatusEnum.ACCEPTED, result,
+            "OCI upload failed\nGot: %s" % self.log.getLogBuffer())
+        self.assertEqual(BuildStatus.FULLYBUILT, self.build.status)
+        self.assertTrue(self.build.verifySuccessfulUpload())
diff --git a/lib/lp/oci/configure.zcml b/lib/lp/oci/configure.zcml
index 616fd2a..1f7a6c7 100644
--- a/lib/lp/oci/configure.zcml
+++ b/lib/lp/oci/configure.zcml
@@ -97,6 +97,18 @@
         factory="lp.oci.model.ocirecipebuildbehaviour.OCIRecipeBuildBehaviour"
         permission="zope.Public" />
 
+    <!-- OCIFile -->
+    <class class="lp.oci.model.ocirecipebuild.OCIFile">
+        <allow interface="lp.oci.interfaces.ocirecipebuild.IOCIFile" />
+    </class>
+
+    <!-- OCIFileSet -->
+    <securedutility
+        class="lp.oci.model.ocirecipebuild.OCIFileSet"
+        provides="lp.oci.interfaces.ocirecipebuild.IOCIFileSet">
+        <allow interface="lp.oci.interfaces.ocirecipebuild.IOCIFileSet" />
+    </securedutility>
+
     <!-- OCIRegistryCredentials -->
     <class class="lp.oci.model.ociregistrycredentials.OCIRegistryCredentials">
         <require
diff --git a/lib/lp/oci/interfaces/ocirecipebuild.py b/lib/lp/oci/interfaces/ocirecipebuild.py
index 1f00269..95539c0 100644
--- a/lib/lp/oci/interfaces/ocirecipebuild.py
+++ b/lib/lp/oci/interfaces/ocirecipebuild.py
@@ -8,6 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 __metaclass__ = type
 __all__ = [
     'IOCIFile',
+    'IOCIFileSet',
     'IOCIRecipeBuild',
     'IOCIRecipeBuildSet',
     'OCIRecipeBuildRegistryUploadStatus',
@@ -82,6 +83,13 @@ class OCIRecipeBuildRegistryUploadStatus(EnumeratedType):
         """)
 
 
+class IOCIFileSet(Interface):
+    """A file artifact of an OCIRecipeBuild."""
+
+    def getByLayerDigest(layer_file_digest):
+        """Return an `IOCIFile` with the matching layer_file_digest."""
+
+
 class IOCIRecipeBuildView(IPackageBuild):
     """`IOCIRecipeBuild` attributes that require launchpad.View permission."""
 
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index 6b8e023..94b7326 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -43,6 +43,7 @@ from lp.buildmaster.model.packagebuild import PackageBuildMixin
 from lp.oci.interfaces.ocirecipe import IOCIRecipeSet
 from lp.oci.interfaces.ocirecipebuild import (
     IOCIFile,
+    IOCIFileSet,
     IOCIRecipeBuild,
     IOCIRecipeBuildSet,
     OCIRecipeBuildRegistryUploadStatus,
@@ -98,6 +99,16 @@ class OCIFile(Storm):
         self.layer_file_digest = layer_file_digest
 
 
+@implementer(IOCIFileSet)
+class OCIFileSet:
+
+    def getByLayerDigest(self, layer_file_digest):
+        return IStore(OCIFile).find(
+            OCIFile,
+            OCIFile.layer_file_digest == layer_file_digest).order_by(
+                OCIFile.id).first()
+
+
 @implementer(IOCIRecipeBuild)
 class OCIRecipeBuild(PackageBuildMixin, Storm):
 
diff --git a/lib/lp/oci/model/ocirecipebuildbehaviour.py b/lib/lp/oci/model/ocirecipebuildbehaviour.py
index dbef696..6205cfd 100644
--- a/lib/lp/oci/model/ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/model/ocirecipebuildbehaviour.py
@@ -18,6 +18,7 @@ import json
 import os
 
 from twisted.internet import defer
+from zope.component import getUtility
 from zope.interface import implementer
 
 from lp.app.errors import NotFoundError
@@ -32,6 +33,7 @@ from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
 from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
+from lp.oci.interfaces.ocirecipebuild import IOCIFileSet
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.librarian.utils import copy_and_close
 from lp.snappy.model.snapbuildbehaviour import SnapProxyMixin
@@ -134,12 +136,17 @@ class OCIRecipeBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
                 # This is in the form '<id>/layer.tar', we only need the first
                 layer_filename = "{}.tar.gz".format(layer_id.split('/')[0])
                 digest = digests_section[diff_id]['digest']
-                try:
-                    _, librarian_file, _ = self.build.getLayerFileByDigest(
-                        digest)
-                except NotFoundError:
+                # Check if the file already exists in the librarian
+                oci_file = getUtility(IOCIFileSet).getByLayerDigest(
+                    digest)
+                if oci_file:
+                    librarian_file = oci_file.library_file
+                # If it doesn't, we need to download it
+                else:
                     files.add(layer_filename)
                     continue
+                # If the file already exists, retrieve it from the librarian
+                # so we can add it to the build artifacts
                 layer_path = os.path.join(upload_path, layer_filename)
                 librarian_file.open()
                 copy_and_close(librarian_file, open(layer_path, 'wb'))
diff --git a/lib/lp/oci/tests/test_ocirecipebuild.py b/lib/lp/oci/tests/test_ocirecipebuild.py
index 84a69f4..2ebffde 100644
--- a/lib/lp/oci/tests/test_ocirecipebuild.py
+++ b/lib/lp/oci/tests/test_ocirecipebuild.py
@@ -29,6 +29,7 @@ from lp.oci.interfaces.ocirecipe import (
     OCI_RECIPE_WEBHOOKS_FEATURE_FLAG,
     )
 from lp.oci.interfaces.ocirecipebuild import (
+    IOCIFileSet,
     IOCIRecipeBuild,
     IOCIRecipeBuildSet,
     )
@@ -52,6 +53,32 @@ from lp.testing.layers import (
 from lp.testing.matchers import HasQueryCount
 
 
+class TestOCIFileSet(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestOCIFileSet, self).setUp()
+        self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+
+    def test_implements_interface(self):
+        file_set = getUtility(IOCIFileSet)
+        self.assertProvides(file_set, IOCIFileSet)
+
+    def test_getByLayerDigest(self):
+        digest = "test_digest"
+        oci_file = self.factory.makeOCIFile(layer_file_digest=digest)
+        for _ in range(3):
+            self.factory.makeOCIFile()
+
+        result = getUtility(IOCIFileSet).getByLayerDigest(digest)
+        self.assertEqual(oci_file, result)
+
+    def test_getByLayerDigest_not_matching(self):
+        result = getUtility(IOCIFileSet).getByLayerDigest("not existing")
+        self.assertIsNone(result)
+
+
 class TestOCIRecipeBuild(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index 8536072..ee18740 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -519,6 +519,41 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
             self.assertEqual(contents, b'retrieved from librarian')
 
     @defer.inlineCallbacks
+    def test_handleStatus_OK_reuse_from_other_build(self):
+        """We should be able to reuse a layer file from a separate build."""
+        self.factory.makeOCIFile(
+            layer_file_digest=u'digest_2',
+            content="layer 2 retrieved from librarian")
+
+        now = datetime.now()
+        mock_datetime = self.useFixture(MockPatch(
+            'lp.buildmaster.model.buildfarmjobbehaviour.datetime')).mock
+        mock_datetime.now = lambda: now
+        with dbuser(config.builddmaster.dbuser):
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, 'OK',
+                {'filemap': self.filemap})
+        self.assertEqual(
+            ['buildlog', 'manifest_hash', 'digests_hash', 'config_1_hash'],
+            self.slave._got_file_record)
+        # This hash should not appear as it is already in the librarian
+        self.assertNotIn('layer_1_hash', self.slave._got_file_record)
+        self.assertNotIn('layer_2_hash', self.slave._got_file_record)
+
+        # layer_2 should have been retrieved from the librarian
+        layer_2_path = os.path.join(
+            self.upload_root,
+            "incoming",
+            self.behaviour.getUploadDirLeaf(self.build.build_cookie),
+            str(self.build.archive.id),
+            self.build.distribution.name,
+            "layer_2.tar.gz"
+        )
+        with open(layer_2_path, 'rb') as layer_2_fp:
+            contents = layer_2_fp.read()
+            self.assertEqual(contents, b'layer 2 retrieved from librarian')
+
+    @defer.inlineCallbacks
     def test_handleStatus_OK_absolute_filepath(self):
 
         self._createTestFile(