Merge ~twom/launchpad:oci-buildbehaviour into launchpad:master

[Tom Wardill]

Add OCIRecipeBuildBehaviour

Add a buildbehaviour to drive OCI image builds.
Use mixin tests where sensible, override for places where the OCI methods differ.

[Colin Watson (cjwatson)]

review: Needs Fixing

diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index 2f3515a..4b28693 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -301,6 +301,22 @@ class BuildFarmJobBehaviourBase:
         transaction.commit()
 
     @defer.inlineCallbacks
+    def _downloadFiles(self, filemap, upload_path, logger):
+        filenames_to_download = []
+        for filename, sha1 in filemap.items():
+            logger.info("Grabbing file: %s (%s)" % (
+                filename, self._slave.getURL(sha1)))
+            out_file_name = os.path.join(upload_path, filename)
+            # If the evaluated output file name is not within our
+            # upload path, then we don't try to copy this or any
+            # subsequent files.
+            if not os.path.realpath(out_file_name).startswith(upload_path):
+                raise BuildDaemonError(
+                    "Build returned a file named %r." % filename)
+            filenames_to_download.append((sha1, out_file_name))
+        yield self._slave.getFiles(filenames_to_download, logger=logger)
+
+    @defer.inlineCallbacks
     def handleSuccess(self, slave_status, logger):
         """Handle a package that built successfully.
 
@@ -337,19 +353,7 @@ class BuildFarmJobBehaviourBase:
             grab_dir, str(build.archive.id), build.distribution.name)
         os.makedirs(upload_path)
 
-        filenames_to_download = []
-        for filename, sha1 in filemap.items():
-            logger.info("Grabbing file: %s (%s)" % (
-                filename, self._slave.getURL(sha1)))
-            out_file_name = os.path.join(upload_path, filename)
-            # If the evaluated output file name is not within our
-            # upload path, then we don't try to copy this or any
-            # subsequent files.
-            if not os.path.realpath(out_file_name).startswith(upload_path):
-                raise BuildDaemonError(
-                    "Build returned a file named %r." % filename)
-            filenames_to_download.append((sha1, out_file_name))
-        yield self._slave.getFiles(filenames_to_download, logger=logger)
+        yield self._downloadFiles(filemap, upload_path, logger)
 
         transaction.commit()
 
diff --git a/lib/lp/buildmaster/tests/mock_slaves.py b/lib/lp/buildmaster/tests/mock_slaves.py
index 8b00c32..eca171a 100644
--- a/lib/lp/buildmaster/tests/mock_slaves.py
+++ b/lib/lp/buildmaster/tests/mock_slaves.py
@@ -194,7 +194,8 @@ class WaitingSlave(OkSlave):
 
         # By default, the slave only has a buildlog, but callsites
         # can update this list as needed.
-        self.valid_file_hashes = ['buildlog']
+        self.valid_files = {'buildlog': ''}
+        self._got_file_record = []
 
     def status(self):
         self.call_log.append('status')
@@ -208,12 +209,17 @@ class WaitingSlave(OkSlave):
 
     def getFile(self, hash, file_to_write):
         self.call_log.append('getFile')
-        if hash in self.valid_file_hashes:
-            content = "This is a %s" % hash
+        if hash in self.valid_files:
             if isinstance(file_to_write, types.StringTypes):
                 file_to_write = open(file_to_write, 'wb')
+            if not self.valid_files[hash]:
+                content = "This is a %s" % hash
+            else:
+                with open(self.valid_files[hash], 'rb') as source:
+                    content = source.read()
             file_to_write.write(content)
             file_to_write.close()
+            self._got_file_record.append(hash)
         return defer.succeed(None)
 
 
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index 697ff52..d6ecd85 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
@@ -337,7 +337,7 @@ class TestHandleStatusMixin:
         self.builder = self.factory.makeBuilder()
         self.build.buildqueue_record.markAsBuilding(self.builder)
         self.slave = WaitingSlave('BuildStatus.OK')
-        self.slave.valid_file_hashes.append('test_file_hash')
+        self.slave.valid_file_hashes['test_file_hash'] = ''
         self.interactor = BuilderInteractor()
         self.behaviour = self.interactor.getBuildBehaviour(
             self.build.buildqueue_record, self.builder, self.slave)
diff --git a/lib/lp/oci/configure.zcml b/lib/lp/oci/configure.zcml
index 30212b9..51e1d31 100644
--- a/lib/lp/oci/configure.zcml
+++ b/lib/lp/oci/configure.zcml
@@ -56,4 +56,10 @@
         <allow interface="lp.buildmaster.interfaces.buildfarmjob.ISpecificBuildFarmJobSource" />
     </securedutility>
 
+    <adapter
+        for="lp.oci.interfaces.ocirecipebuild.IOCIRecipeBuild"
+        provides="lp.buildmaster.interfaces.buildfarmjobbehaviour.IBuildFarmJobBehaviour"
+        factory="lp.oci.model.ocirecipebuildbehaviour.OCIRecipeBuildBehaviour"
+        permission="zope.Public" />
+
 </configure>
diff --git a/lib/lp/oci/interfaces/ocirecipebuildjob.py b/lib/lp/oci/interfaces/ocirecipebuildjob.py
deleted file mode 100644
index cd72177..0000000
--- a/lib/lp/oci/interfaces/ocirecipebuildjob.py
+++ /dev/null
@@ -1,38 +0,0 @@
-# Copyright 2019 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""OCIRecipe build job interfaces"""
-
-from __future__ import absolute_import, print_function, unicode_literals
-
-__metaclass__ = type
-__all__ = [
-    'IOCIRecipeBuildJob',
-    ]
-
-from lazr.restful.fields import Reference
-from zope.interface import (
-    Attribute,
-    Interface,
-    )
-from zope.schema import TextLine
-
-from lp import _
-from lp.oci.interfaces.ocirecipebuild import IOCIRecipeBuild
-from lp.services.job.interfaces.job import (
-    IJob,
-    IJobSource,
-    IRunnableJob,
-    )
-
-
-class IOCIRecipeBuildJob(Interface):
-    job = Reference(
-        title=_("The common Job attributes."), schema=IJob,
-        required=True, readonly=True)
-
-    build = Reference(
-        title=_("The OCI Recipe Build to use for this job."),
-        schema=IOCIRecipeBuild, required=True, readonly=True)
-
-    json_data = Attribute(_("A dict of data about the job."))
diff --git a/lib/lp/oci/model/ocirecipe.py b/lib/lp/oci/model/ocirecipe.py
index 300f335..8a01b2d 100644
--- a/lib/lp/oci/model/ocirecipe.py
+++ b/lib/lp/oci/model/ocirecipe.py
@@ -308,15 +308,5 @@ class OCIRecipeSet:
             person_ids.add(recipe.registrant_id)
             person_ids.add(recipe.owner_id)
 
-        repositories = load_related(
-            GitRepository, recipes, ["git_repository_id"])
-        if repositories:
-            GenericGitCollection.preloadDataForRepositories(repositories)
-        GenericGitCollection.preloadVisibleRepositories(repositories, user)
-
-        # We need the target repository owner as well; unlike branches,
-        # repository unique names aren't trigger-maintained.
-        person_ids.update(repository.owner_id for repository in repositories)
-
         list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
             person_ids, need_validity=True))
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index 307fbd2..5d2d21d 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -44,6 +44,7 @@ from lp.oci.interfaces.ocirecipebuild import (
     IOCIRecipeBuildSet,
     )
 from lp.registry.model.person import Person
+from lp.services.config import config
 from lp.services.database.bulk import load_related
 from lp.services.database.constants import DEFAULT
 from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -208,6 +209,14 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
         # pillar isn't just a distribution
         return self.recipe.oci_project.distribution
 
+    def notify(self, extra_info=None):
+        """See `IPackageBuild`."""
+        if not config.builddmaster.send_build_notification:
+            return
+        if self.status == BuildStatus.FULLYBUILT:
+            return
+        # XXX twom 2019-12-11 This should send mail
+
 
 @implementer(IOCIRecipeBuildSet)
 class OCIRecipeBuildSet(SpecificBuildFarmJobSourceMixin):
diff --git a/lib/lp/oci/model/ocirecipebuildbehaviour.py b/lib/lp/oci/model/ocirecipebuildbehaviour.py
new file mode 100644
index 0000000..e1a1215
--- /dev/null
+++ b/lib/lp/oci/model/ocirecipebuildbehaviour.py
@@ -0,0 +1,115 @@
+# Copyright 2019 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""An `IBuildFarmJobBehaviour` for `OCIRecipeBuild`.
+
+Dispatches OCI image build jobs to build-farm slaves.
+"""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'OCIRecipeBuildBehaviour',
+    ]
+
+
+import json
+import os
+
+from twisted.internet import defer
+from zope.interface import implementer
+
+from lp.app.errors import NotFoundError
+from lp.buildmaster.enums import BuildBaseImageType
+from lp.buildmaster.interfaces.builder import BuildDaemonError
+from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
+    IBuildFarmJobBehaviour,
+    )
+from lp.buildmaster.model.buildfarmjobbehaviour import (
+    BuildFarmJobBehaviourBase,
+    )
+from lp.services.librarian.utils import copy_and_close
+
+
+@implementer(IBuildFarmJobBehaviour)
+class OCIRecipeBuildBehaviour(BuildFarmJobBehaviourBase):
+
+    builder_type = "oci"
+    image_types = [BuildBaseImageType.LXD]
+
+    # These attributes are defined in `IOCIBuildFarmJobBehaviour`,
+    # but are not used in this implementation.
+    distro_arch_series = None
+
+    def _ensureFilePath(self, file_name, file_path, upload_path):
+        # If the evaluated output file name is not within our
+        # upload path, then we don't try to copy this or any
+        # subsequent files.
+        if not os.path.normpath(file_path).startswith(upload_path):
+            raise BuildDaemonError(
+                "Build returned a file named '%s'." % file_name)
+
+    def _fetchIntermediaryFile(self, name, filemap, upload_path):
+        file_hash = filemap[name]
+        file_path = os.path.join(upload_path, name)
+        self._ensureFilePath(name, file_path, upload_path)
+        self._slave.getFile(file_hash, file_path)
+
+        with open(file_path, 'r') as file_fp:
+            contents = json.load(file_fp)
+        return contents
+
+    def _extractLayerFiles(self, upload_path, section, config, digests, files):
+        # These are different sets of ids, in the same order
+        # layer_id is the filename, diff_id is the internal (docker) id
+        for diff_id in config['rootfs']['diff_ids']:
+            layer_id = digests[diff_id]['layer_id']
+            # This is in the form '<id>/layer.tar', we only need the first
+            layer_filename = "{}.tar.gz".format(layer_id.split('/')[0])
+            digest = digests[diff_id]['digest']
+            try:
+                _, librarian_layer_file, _ = self.build.getLayerFileByDigest(
+                    digest)
+            except NotFoundError:
+                files.add(layer_filename)
+                continue
+            layer_path = os.path.join(upload_path, layer_filename)
+            librarian_layer_file.open()
+            with open(layer_path, 'wb') as layer_fp:
+                copy_and_close(librarian_layer_file, layer_fp)
+
+    def _convertToRetrievableFile(self, upload_path, file_name, filemap):
+        file_path = os.path.join(upload_path, file_name)
+        self._ensureFilePath(file_name, file_path, upload_path)
+        return (filemap[file_name], file_path)
+
+    @defer.inlineCallbacks
+    def _downloadFiles(self, filemap, upload_path, logger):
+        """Download required artifact files."""
+        # We don't want to download all of the files that have been created,
+        # just the ones that are mentioned in the manifest and config.
+
+        manifest = self._fetchIntermediaryFile(
+            'manifest.json', filemap, upload_path)
+        digests = self._fetchIntermediaryFile(
+            'digests.json', filemap, upload_path)
+
+        files = set()
+        for section in manifest:
+            config = self._fetchIntermediaryFile(
+                section['Config'], filemap, upload_path)
+            self._extractLayerFiles(
+                upload_path, section, config, digests, files)
+
+        files_to_download = [
+            self._convertToRetrievableFile(upload_path, filename, filemap)
+            for filename in files]
+        yield self._slave.getFiles(files_to_download, logger=logger)
+
+    def verifySuccessfulBuild(self):
+        """See `IBuildFarmJobBehaviour`."""
+        # The implementation in BuildFarmJobBehaviourBase checks whether the
+        # target suite is modifiable in the target archive.  However, an
+        # `OCIRecipeBuild` does not use an archive in this manner.
+        return True
diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
deleted file mode 100644
index 6df00cf..0000000
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ /dev/null
@@ -1,143 +0,0 @@
-# Copyright 2019 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""OCIRecipe build jobs."""
-
-from __future__ import absolute_import, print_function, unicode_literals
-
-__metaclass__ = type
-__all__ = [
-    'OCIRecipeBuildJob',
-    'OCIRecipeBuildJobType',
-    ]
-
-import json
-
-from lazr.delegates import delegate_to
-from lazr.enum import (
-    DBEnumeratedType,
-    DBItem,
-    )
-from storm.locals import (
-    Int,
-    JSON,
-    Reference,
-    )
-from zope.interface import (
-    implementer,
-    provider,
-    )
-
-from lp.app.errors import NotFoundError
-from lp.oci.interfaces.ocirecipebuildjob import IOCIRecipeBuildJob
-from lp.services.database.enumcol import DBEnum
-from lp.services.database.interfaces import (
-    IMasterStore,
-    IStore,
-    )
-from lp.services.database.stormbase import StormBase
-from lp.services.job.model.job import (
-    EnumeratedSubclass,
-    Job,
-    )
-from lp.services.job.runner import BaseRunnableJob
-from lp.services.propertycache import get_property_cache
-
-
-class OCIRecipeBuildJobType(DBEnumeratedType):
-    """Values that `OCIBuildJobType.job_type` can take."""
-
-    REGISTRY_UPLOAD = DBItem(0, """
-        Registry upload
-
-        This job uploads an OCI Image to registry.
-        """)
-
-
-@implementer(IOCIRecipeBuildJob)
-class OCIRecipeBuildJob(StormBase):
-    """See `IOCIRecipeBuildJob`."""
-
-    __storm_table__ = 'OCIRecipeBuildJob'
-
-    job_id = Int(name='job', primary=True, allow_none=False)
-    job = Reference(job_id, 'Job.id')
-
-    build_id = Int(name='build', allow_none=False)
-    build = Reference(build_id, 'OCIRecipeBuild.id')
-
-    job_type = DBEnum(enum=OCIRecipeBuildJobType, allow_none=True)
-
-    json_data = JSON('json_data', allow_none=False)
-
-    def __init__(self, build, job_type, json_data, **job_args):
-        """Constructor.
-
-        Extra keyword arguments are used to construct the underlying Job
-        object.
-
-        :param build: The `IOCIRecipeBuild` this job relates to.
-        :param job_type: The `OCIRecipeBuildJobType` of this job.
-        :param json_data: The type-specific variables, as a JSON-compatible
-            dict.
-        """
-        super(OCIRecipeBuildJob, self).__init__()
-        self.job = Job(**job_args)
-        self.build = build
-        self.job_type = job_type
-        self.json_data = json_data
-
-    def makeDerived(self):
-        return OCIRecipeBuildJob.makeSubclass(self)
-
-
-@delegate_to(IOCIRecipeBuildJob)
-class OCIRecipeBuildJobDerived(BaseRunnableJob):
-
-    __metaclass__ = EnumeratedSubclass
-
-    def __init__(self, oci_build_job):
-        self.context = oci_build_job
-
-    def __repr__(self):
-        """An informative representation of the job."""
-        return "<%s for %s>" % (
-            self.__class__.__name__, self.build.id)
-
-    @classmethod
-    def get(cls, job_id):
-        """Get a job by id.
-
-        :return: The `OCIBuildJob` with the specified id, as the current
-            `OCIBuildJobDerived` subclass.
-        :raises: `NotFoundError` if there is no job with the specified id,
-            or its `job_type` does not match the desired subclass.
-        """
-        oci_build_job = IStore(OCIRecipeBuildJob).get(
-            OCIRecipeBuildJob, job_id)
-        if oci_build_job.job_type != cls.class_job_type:
-            raise NotFoundError(
-                "No object found with id %d and type %s" %
-                (job_id, cls.class_job_type.title))
-        return cls(oci_build_job)
-
-    @classmethod
-    def iterReady(cls):
-        """See `IJobSource`."""
-        jobs = IMasterStore(OCIRecipeBuildJob).find(
-            OCIRecipeBuildJob,
-            OCIRecipeBuildJob.job_type == cls.class_job_type,
-            OCIRecipeBuildJob.job == Job.id,
-            Job.id.is_in(Job.ready_jobs))
-        return (cls(job) for job in jobs)
-
-    def getOopsVars(self):
-        """See `IRunnableJob`."""
-        oops_vars = super(OCIRecipeBuildJobDerived, self).getOopsVars()
-        oops_vars.extend([
-            ('job_type', self.context.job_type.title),
-            ('build_id', self.context.build.id),
-            ('owner_id', self.context.build.recipe.owner.id),
-            ('project_name', self.context.build.recipe.ociproject.name)
-            ])
-        return oops_vars
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
new file mode 100644
index 0000000..56d3592
--- /dev/null
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -0,0 +1,312 @@
+# Copyright 2015-2019 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for `OCIRecipeBuildBehavior`."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+import json
+import os
+import shutil
+import tempfile
+
+from testtools import ExpectedException
+from twisted.internet import defer
+from zope.security.proxy import removeSecurityProxy
+
+from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.interactor import BuilderInteractor
+from lp.buildmaster.interfaces.builder import BuildDaemonError
+from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
+    IBuildFarmJobBehaviour,
+    )
+from lp.buildmaster.tests.mock_slaves import WaitingSlave
+from lp.buildmaster.tests.test_buildfarmjobbehaviour import (
+    TestGetUploadMethodsMixin,
+    )
+from lp.oci.model.ocirecipebuildbehaviour import OCIRecipeBuildBehaviour
+from lp.services.config import config
+from lp.testing import TestCaseWithFactory
+from lp.testing.dbuser import dbuser
+from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing.mail_helpers import pop_notifications
+
+
+class MakeOCIBuildMixin:
+
+    def makeBuild(self):
+        build = self.factory.makeOCIRecipeBuild()
+        build.queueBuild()
+        return build
+
+    def makeUnmodifiableBuild(self):
+        build = self.factory.makeOCIRecipeBuild()
+        build.distro_arch_series = 'failed'
+        build.queueBuild()
+        return build
+
+
+class TestOCIBuildBehaviour(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_provides_interface(self):
+        # OCIRecipeBuildBehaviour provides IBuildFarmJobBehaviour.
+        job = OCIRecipeBuildBehaviour(None)
+        self.assertProvides(job, IBuildFarmJobBehaviour)
+
+    def test_adapts_IOCIRecipeBuild(self):
+        # IBuildFarmJobBehaviour adapts an IOCIRecipeBuild.
+        build = self.factory.makeOCIRecipeBuild()
+        job = IBuildFarmJobBehaviour(build)
+        self.assertProvides(job, IBuildFarmJobBehaviour)
+
+
+class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
+                                        TestCaseWithFactory):
+    # This is mostly copied from TestHandleStatusMixin, however
+    # we can't use all of those tests, due to the way OCIRecipeBuildBehaviour
+    # parses the file contents, rather than just retrieving all that are
+    # available. There's also some differences in the filemap handling, as
+    # we need a much more complex filemap here.
+
+    layer = LaunchpadZopelessLayer
+
+    def _createTestFile(self, name, content, hash):
+        path = os.path.join(self.test_files_dir, name)
+        with open(path, 'wb') as fp:
+            fp.write(content)
+        self.slave.valid_files[hash] = path
+
+    def setUp(self):
+        super(TestHandleStatusForOCIRecipeBuild, self).setUp()
+        self.factory = LaunchpadObjectFactory()
+        self.build = self.makeBuild()
+        # For the moment, we require a builder for the build so that
+        # handleStatus_OK can get a reference to the slave.
+        self.builder = self.factory.makeBuilder()
+        self.build.buildqueue_record.markAsBuilding(self.builder)
+        self.slave = WaitingSlave('BuildStatus.OK')
+        self.slave.valid_files['test_file_hash'] = ''
+        self.interactor = BuilderInteractor()
+        self.behaviour = self.interactor.getBuildBehaviour(
+            self.build.buildqueue_record, self.builder, self.slave)
+
+        # We overwrite the buildmaster root to use a temp directory.
+        tempdir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, tempdir)
+        self.upload_root = tempdir
+        self.pushConfig('builddmaster', root=self.upload_root)
+
+        # We stub out our builds getUploaderCommand() method so
+        # we can check whether it was called as well as
+        # verifySuccessfulUpload().
+        removeSecurityProxy(self.build).verifySuccessfulUpload = FakeMethod(
+            result=True)
+
+        digests = {
+            "diff_id_1": {
+                "digest": "digest_1",
+                "source": "test/base_1",
+                "layer_id": "layer_1"
+            },
+            "diff_id_2": {
+                "digest": "digest_2",
+                "source": "",
+                "layer_id": "layer_2"
+            }
+        }
+
+        self.test_files_dir = tempfile.mkdtemp()
+        self._createTestFile('buildlog', '', 'buildlog')
+        self._createTestFile(
+            'manifest.json',
+            '[{"Config": "config_file_1.json", '
+            '"Layers": ["layer_1/layer.tar", "layer_2/layer.tar"]}]',
+            'manifest_hash')
+        self._createTestFile(
+            'digests.json',
+            json.dumps(digests),
+            'digests_hash')
+        self._createTestFile(
+            'config_file_1.json',
+            '{"rootfs": {"diff_ids": ["diff_id_1", "diff_id_2"]}}',
+            'config_1_hash')
+        self._createTestFile(
+            'layer_2.tar.gz',
+            '',
+            'layer_2_hash'
+        )
+
+        self.filemap = {
+            'manifest.json': 'manifest_hash',
+            'digests.json': 'digests_hash',
+            'config_file_1.json': 'config_1_hash',
+            'layer_1.tar.gz': 'layer_1_hash',
+            'layer_2.tar.gz': 'layer_2_hash'
+        }
+        self.factory.makeOCIFile(
+            build=self.build, layer_file_digest=u'digest_1',
+            content="retrieved from librarian")
+
+    def assertResultCount(self, count, result):
+        self.assertEqual(
+            1, len(os.listdir(os.path.join(self.upload_root, result))))
+
+    @defer.inlineCallbacks
+    def test_handleStatus_OK_normal_image(self):
+        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',
+             'layer_2_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.assertEqual(BuildStatus.UPLOADING, self.build.status)
+        self.assertResultCount(1, "incoming")
+
+        # layer_1 should have been retrieved from the librarian
+        layer_1_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_1.tar.gz"
+        )
+        with open(layer_1_path, 'rb') as layer_1_fp:
+            contents = layer_1_fp.read()
+            self.assertEqual(contents, b'retrieved from librarian')
+
+    @defer.inlineCallbacks
+    def test_handleStatus_OK_absolute_filepath(self):
+
+        self._createTestFile(
+            'manifest.json',
+            '[{"Config": "/notvalid/config_file_1.json", '
+            '"Layers": ["layer_1/layer.tar", "layer_2/layer.tar"]}]',
+            'manifest_hash')
+
+        self.filemap['/notvalid/config_file_1.json'] = 'config_1_hash'
+
+        # A filemap that tries to write to files outside of the upload
+        # directory will not be collected.
+        with ExpectedException(
+                BuildDaemonError,
+                "Build returned a file named "
+                "'/notvalid/config_file_1.json'."):
+            with dbuser(config.builddmaster.dbuser):
+                yield self.behaviour.handleStatus(
+                    self.build.buildqueue_record, 'OK',
+                    {'filemap': self.filemap})
+
+    @defer.inlineCallbacks
+    def test_handleStatus_OK_relative_filepath(self):
+
+        self._createTestFile(
+            'manifest.json',
+            '[{"Config": "../config_file_1.json", '
+            '"Layers": ["layer_1/layer.tar", "layer_2/layer.tar"]}]',
+            'manifest_hash')
+
+        self.filemap['../config_file_1.json'] = 'config_1_hash'
+        # A filemap that tries to write to files outside of
+        # the upload directory will not be collected.
+        with ExpectedException(
+                BuildDaemonError,
+                "Build returned a file named '../config_file_1.json'."):
+            with dbuser(config.builddmaster.dbuser):
+                yield self.behaviour.handleStatus(
+                    self.build.buildqueue_record, 'OK',
+                    {'filemap': self.filemap})
+
+    @defer.inlineCallbacks
+    def test_handleStatus_OK_sets_build_log(self):
+        # The build log is set during handleStatus.
+        self.assertEqual(None, self.build.log)
+        with dbuser(config.builddmaster.dbuser):
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, 'OK',
+                {'filemap': self.filemap})
+        self.assertNotEqual(None, self.build.log)
+
+    @defer.inlineCallbacks
+    def test_handleStatus_ABORTED_cancels_cancelling(self):
+        with dbuser(config.builddmaster.dbuser):
+            self.build.updateStatus(BuildStatus.CANCELLING)
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, "ABORTED", {})
+        self.assertEqual(0, len(pop_notifications()), "Notifications received")
+        self.assertEqual(BuildStatus.CANCELLED, self.build.status)
+
+    @defer.inlineCallbacks
+    def test_handleStatus_ABORTED_illegal_when_building(self):
+        self.builder.vm_host = "fake_vm_host"
+        self.behaviour = self.interactor.getBuildBehaviour(
+            self.build.buildqueue_record, self.builder, self.slave)
+        with dbuser(config.builddmaster.dbuser):
+            self.build.updateStatus(BuildStatus.BUILDING)
+            with ExpectedException(
+                    BuildDaemonError,
+                    "Build returned unexpected status: u'ABORTED'"):
+                yield self.behaviour.handleStatus(
+                    self.build.buildqueue_record, "ABORTED", {})
+
+    @defer.inlineCallbacks
+    def test_handleStatus_ABORTED_cancelling_sets_build_log(self):
+        # If a build is intentionally cancelled, the build log is set.
+        self.assertEqual(None, self.build.log)
+        with dbuser(config.builddmaster.dbuser):
+            self.build.updateStatus(BuildStatus.CANCELLING)
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, "ABORTED", {})
+        self.assertNotEqual(None, self.build.log)
+
+    @defer.inlineCallbacks
+    def test_date_finished_set(self):
+        # The date finished is updated during handleStatus_OK.
+        self.assertEqual(None, self.build.date_finished)
+        with dbuser(config.builddmaster.dbuser):
+            yield self.behaviour.handleStatus(
+                self.build.buildqueue_record, 'OK',
+                {'filemap': self.filemap})
+        self.assertNotEqual(None, self.build.date_finished)
+
+    @defer.inlineCallbacks
+    def test_givenback_collection(self):
+        with ExpectedException(
+                BuildDaemonError,
+                "Build returned unexpected status: u'GIVENBACK'"):
+            with dbuser(config.builddmaster.dbuser):
+                yield self.behaviour.handleStatus(
+                    self.build.buildqueue_record, "GIVENBACK", {})
+
+    @defer.inlineCallbacks
+    def test_builderfail_collection(self):
+        with ExpectedException(
+                BuildDaemonError,
+                "Build returned unexpected status: u'BUILDERFAIL'"):
+            with dbuser(config.builddmaster.dbuser):
+                yield self.behaviour.handleStatus(
+                    self.build.buildqueue_record, "BUILDERFAIL", {})
+
+    @defer.inlineCallbacks
+    def test_invalid_status_collection(self):
+        with ExpectedException(
+                BuildDaemonError,
+                "Build returned unexpected status: u'BORKED'"):
+            with dbuser(config.builddmaster.dbuser):
+                yield self.behaviour.handleStatus(
+                    self.build.buildqueue_record, "BORKED", {})
+
+
+class TestGetUploadMethodsForOCIRecipeBuild(
+    MakeOCIBuildMixin, TestGetUploadMethodsMixin, TestCaseWithFactory):
+    """IPackageBuild.getUpload-related methods work with OCI recipe builds."""
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
deleted file mode 100644
index cda3eef..0000000
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ /dev/null
@@ -1,49 +0,0 @@
-# Copyright 2019 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""OCIRecipeBuildJob tests"""
-
-from __future__ import absolute_import, print_function, unicode_literals
-
-__metaclass__ = type
-
-
-from lp.oci.interfaces.ocirecipebuildjob import IOCIRecipeBuildJob
-from lp.oci.model.ocirecipebuildjob import (
-    OCIRecipeBuildJob,
-    OCIRecipeBuildJobDerived,
-    OCIRecipeBuildJobType,
-    )
-from lp.testing import TestCaseWithFactory
-from lp.testing.layers import DatabaseFunctionalLayer
-
-
-class FakeOCIBuildJob(OCIRecipeBuildJobDerived):
-    """For testing OCIRecipeBuildJobDerived without a child class."""
-
-
-class TestOCIRecipeBuildJob(TestCaseWithFactory):
-
-    layer = DatabaseFunctionalLayer
-
-    def test_provides_interface(self):
-        oci_build = self.factory.makeOCIRecipeBuild()
-        self.assertProvides(
-            OCIRecipeBuildJob(
-                oci_build, OCIRecipeBuildJobType.REGISTRY_UPLOAD, {}),
-            IOCIRecipeBuildJob)
-
-    def test_getOopsVars(self):
-        oci_build = self.factory.makeOCIRecipeBuild()
-        build_job = OCIRecipeBuildJob(
-                oci_build, OCIRecipeBuildJobType.REGISTRY_UPLOAD, {})
-        derived = FakeOCIBuildJob(build_job)
-        oops = derived.getOopsVars()
-        expected = [
-            ('job_id', build_job.job.id),
-            ('job_type', build_job.job_type.title),
-            ('build_id', oci_build.id),
-            ('owner_id', oci_build.recipe.owner.id),
-            ('project_name', oci_build.recipe.ociproject.name),
-            ]
-        self.assertEqual(expected, oops)
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index f0788d8..4a25c88 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -5005,12 +5005,13 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         return oci_build
 
     def makeOCIFile(self, build=None, library_file=None,
-                    layer_file_digest=None):
+                    layer_file_digest=None, content=None, filename=None):
         """Make a new OCIFile."""
         if build is None:
             build = self.makeOCIRecipeBuild()
         if library_file is None:
-            library_file = self.makeLibraryFileAlias()
+            library_file = self.makeLibraryFileAlias(
+                content=content, filename=filename)
         return OCIFile(build=build, library_file=library_file,
                        layer_file_digest=layer_file_digest)
 
diff --git a/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py b/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
index b387251..32dd553 100644
--- a/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
+++ b/lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py
@@ -138,7 +138,7 @@ class TestTranslationTemplatesBuildBehaviour(
         buildqueue = FakeBuildQueue(behaviour)
         path = behaviour.templates_tarball_path
         # Poke the file we're expecting into the mock slave.
-        behaviour._slave.valid_file_hashes.append(path)
+        behaviour._slave.valid_files[path] = ''
 
         def got_tarball(filename):
             tarball = open(filename, 'r')