launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24159
[Merge] ~twom/launchpad:oci-buildbehaviour into launchpad:master
Tom Wardill has proposed merging ~twom/launchpad:oci-buildbehaviour into launchpad:master with ~twom/launchpad:oci-buildjob as a prerequisite.
Commit message:
Add OCIRecipeBuildBehaviour
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/376678
Add a buildbehaviour to drive OCI image builds.
Use mixin tests where sensible, override for places where the OCI methods differ.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:oci-buildbehaviour into launchpad:master.
diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index 2f3515a..927d115 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.
@@ -349,7 +365,7 @@ class BuildFarmJobBehaviourBase:
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..8f3f5e2 100644
--- a/lib/lp/buildmaster/tests/mock_slaves.py
+++ b/lib/lp/buildmaster/tests/mock_slaves.py
@@ -217,6 +217,25 @@ class WaitingSlave(OkSlave):
return defer.succeed(None)
+class WaitingSlaveWithFiles(WaitingSlave):
+ """A mock slave that has files that can be downloaded."""
+
+ def __init__(self, *args, **kwargs):
+ self._test_files = {}
+ self._got_file_record = []
+ super(WaitingSlaveWithFiles, self).__init__(*args, **kwargs)
+
+ def getFile(self, hash, file_to_write):
+ if hash in self.valid_file_hashes:
+ if isinstance(file_to_write, types.StringTypes):
+ file_to_write = open(file_to_write, 'wb')
+ with open(self._test_files[hash], 'rb') as source:
+ file_to_write.write(source.read())
+ file_to_write.close()
+ self._got_file_record.append(hash)
+ return defer.succeed(None)
+
+
class AbortingSlave(OkSlave):
"""A mock slave that looks like it's in the process of aborting."""
diff --git a/lib/lp/oci/configure.zcml b/lib/lp/oci/configure.zcml
index 8707b6f..70bb3bc 100644
--- a/lib/lp/oci/configure.zcml
+++ b/lib/lp/oci/configure.zcml
@@ -52,4 +52,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/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index de31d55..119871c 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -45,6 +45,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
@@ -53,7 +54,6 @@ from lp.services.database.interfaces import (
IMasterStore,
IStore,
)
-from lp.services.librarian.browser import ProxiedLibraryFileAlias
from lp.services.librarian.model import (
LibraryFileAlias,
LibraryFileContent,
@@ -210,6 +210,14 @@ class OCIRecipeBuild(PackageBuildMixin, Storm):
"""See `IBuildFarmJob`."""
return 2510 + self.archive.relative_build_score
+ 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..be25ae4
--- /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 _fetchIntermediaryFile(self, name, filemap, upload_path):
+ file_hash = filemap[name]
+ file_path = os.path.join(upload_path, name)
+ self._slave.getFile(file_hash, file_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.realpath(file_path).startswith(upload_path):
+ raise BuildDaemonError(
+ "Build returned a file named %r." % name)
+
+ 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, filename, filemap):
+ out_path = os.path.join(upload_path, filename)
+ if not os.path.realpath(out_path).startswith(upload_path):
+ raise BuildDaemonError(
+ "Build returned a file named %r." % filename)
+ return (filemap[filename], out_path)
+
+ @defer.inlineCallbacks
+ def _downloadFiles(self, filemap, upload_path, logger):
+ """Download required artifact files."""
+ # We don't want to download all of the file 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, a
+ # `OCIRecipeBuild` does not use an archive in this manner.
+ return True
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
new file mode 100644
index 0000000..f19c868
--- /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 WaitingSlaveWithFiles
+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):
+
+ layer = LaunchpadZopelessLayer
+
+ def _create_test_file(self, name, content, hash):
+ path = os.path.join(self.test_files_dir, name)
+ with open(path, 'wb') as fp:
+ fp.write(content)
+ self.slave._test_files[hash] = path
+ self.slave.valid_file_hashes.append(hash)
+
+ 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 = WaitingSlaveWithFiles('BuildStatus.OK')
+ self.slave.valid_file_hashes.append('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
+ tmp_builddmaster_root = """
+ [builddmaster]
+ root: %s
+ """ % self.upload_root
+ config.push('tmp_builddmaster_root', tmp_builddmaster_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._create_test_file('buildlog', '', 'buildlog')
+ self._create_test_file(
+ 'manifest.json',
+ '[{"Config": "config_file_1.json", '
+ '"Layers": ["layer_1/layer.tar", "layer_2/layer.tar"]}]',
+ 'manifest_hash')
+ self._create_test_file(
+ 'digests.json',
+ json.dumps(digests),
+ 'digests_hash')
+ self._create_test_file(
+ 'config_file_1.json',
+ '{"rootfs": {"diff_ids": ["diff_id_1", "diff_id_2"]}}',
+ 'config_1_hash')
+ self._create_test_file(
+ '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.assertTrue('layer_1_hash' not in 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._create_test_file(
+ '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 "
+ "u'/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._create_test_file(
+ '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 u'../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/testing/factory.py b/lib/lp/testing/factory.py
index 342e193..59793b6 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -5011,12 +5011,13 @@ class BareLaunchpadObjectFactory(ObjectFactory):
return oci_build
def makeOCIFile(self, build=None, library_file=None,
- layer_file_digest=DEFAULT):
+ layer_file_digest=DEFAULT, 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)
if layer_file_digest == DEFAULT:
layer_file_digest is None
# layer_file_digest can be None
References
