← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~cjwatson/launchpad-buildd/target-abstractions into lp:launchpad-buildd

 

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/target-abstractions into lp:launchpad-buildd.

Commit message:
Add better abstractions in lpbuildd.target for backends and operations.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/target-abstractions/+merge/328553

Now that I've actually got builds working in LXD containers (though that's several MPs away), I have a better idea of what shape I need the various abstractions to be.  We now have:

  Backend (Chroot, [future] LXD)
    The environment where we run builds.  Loosely inspired by Sbuild::Chroot.  This will gain several more methods for things like copying files out, starting/stopping the backend, and so on.  Ultimately, everything that currently pokes at the chroot directly should go through this.

  Operation (OverrideSourcesList, AddTrustedKeys, Update, ...)
    An operation to perform on the target environment, in a subprocess so that launchpad-buildd can easily continue responding to XML-RPC requests.  These have a newly-uniform command-line syntax for common arguments (backend name, series, architecture, build ID), and the new BuildManager.runTargetSubProcess knows how to call this; the idea here is that, once all the necessary operations are converted, we'll be able to switch a specific BuildManager over to a different backend just by setting its backend_name attribute.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/target-abstractions into lp:launchpad-buildd.
=== modified file 'bin/add-trusted-keys'
--- bin/add-trusted-keys	2017-07-28 23:14:48 +0000
+++ bin/add-trusted-keys	2017-08-04 01:13:06 +0000
@@ -12,23 +12,15 @@
 
 __metaclass__ = type
 
-from argparse import ArgumentParser
 import sys
 
-from lpbuildd.target.chroot import ChrootSetup
+from lpbuildd.target.operation import configure_logging
+from lpbuildd.target.add_trusted_keys import AddTrustedKeys
 
 
 def main():
-    parser = ArgumentParser(
-        description="Write out new trusted keys in the chroot.")
-    parser.add_argument(
-        "build_id", metavar="ID", help="write trusted keys for build ID")
-    args = parser.parse_args()
-
-    print("Adding trusted keys to build-%s" % args.build_id)
-    setup = ChrootSetup(args.build_id)
-    setup.add_trusted_keys(sys.stdin)
-    return 0
+    configure_logging()
+    return AddTrustedKeys().run()
 
 
 if __name__ == "__main__":

=== modified file 'bin/override-sources-list'
--- bin/override-sources-list	2017-07-28 20:34:49 +0000
+++ bin/override-sources-list	2017-08-04 01:13:06 +0000
@@ -9,25 +9,15 @@
 
 __metaclass__ = type
 
-from argparse import ArgumentParser
 import sys
 
-from lpbuildd.target.chroot import ChrootSetup
+from lpbuildd.target.operation import configure_logging
+from lpbuildd.target.override_sources_list import OverrideSourcesList
 
 
 def main():
-    parser = ArgumentParser(
-        description="Override sources.list in the chroot.")
-    parser.add_argument(
-        "build_id", metavar="ID", help="update chroot for build ID")
-    parser.add_argument(
-        "archives", metavar="ARCHIVE", nargs="+", help="sources.list lines")
-    args = parser.parse_args()
-
-    print("Overriding sources.list in build-%s" % args.build_id)
-    setup = ChrootSetup(args.build_id)
-    setup.override_sources_list(args.archives)
-    return 0
+    configure_logging()
+    return OverrideSourcesList().run()
 
 
 if __name__ == "__main__":

=== modified file 'bin/update-debian-chroot'
--- bin/update-debian-chroot	2017-07-28 20:34:49 +0000
+++ bin/update-debian-chroot	2017-08-04 01:13:06 +0000
@@ -9,26 +9,15 @@
 
 __metaclass__ = type
 
-from argparse import ArgumentParser
 import sys
 
-from lpbuildd.target.chroot import ChrootSetup
+from lpbuildd.target.operation import configure_logging
+from lpbuildd.target.update import Update
 
 
 def main():
-    parser = ArgumentParser(description="Update a Debian-style chroot.")
-    parser.add_argument(
-        "--series", metavar="SERIES", help="update chroot for series SERIES")
-    parser.add_argument(
-        "--arch", metavar="ARCH", help="update chroot for architecture ARCH")
-    parser.add_argument(
-        "build_id", metavar="ID", help="update chroot for build ID")
-    args = parser.parse_args()
-
-    print("Updating debian chroot for build %s" % args.build_id)
-    setup = ChrootSetup(args.build_id, args.series, args.arch)
-    setup.update()
-    return 0
+    configure_logging()
+    return Update().run()
 
 
 if __name__ == "__main__":

=== modified file 'lpbuildd/debian.py'
--- lpbuildd/debian.py	2017-07-28 13:57:47 +0000
+++ lpbuildd/debian.py	2017-08-04 01:13:06 +0000
@@ -52,8 +52,6 @@
 
     def initiate(self, files, chroot, extra_args):
         """Initiate a build with a given set of files and chroot."""
-        self.series = extra_args['series']
-        self.arch_tag = extra_args.get('arch_tag', self._slave.getArch())
         self.sources_list = extra_args.get('archives')
         self.trusted_keys = extra_args.get('trusted_keys')
 
@@ -64,26 +62,20 @@
 
         Mainly used for PPA builds.
         """
-        args = ["override-sources-list", self._buildid]
+        args = ["override-sources-list"]
         args.extend(self.sources_list)
-        self.runSubProcess(self._sourcespath, args)
+        self.runTargetSubProcess(self._sourcespath, args)
 
     def doTrustedKeys(self):
         """Add trusted keys."""
         trusted_keys = b"".join(
             base64.b64decode(key) for key in self.trusted_keys)
-        self.runSubProcess(
-            self._keyspath, ["add-trusted-keys", self._buildid],
-            stdin=trusted_keys)
+        self.runTargetSubProcess(
+            self._keyspath, ["add-trusted-keys"], stdin=trusted_keys)
 
     def doUpdateChroot(self):
         """Perform the chroot upgrade."""
-        self.runSubProcess(
-            self._updatepath,
-            ["update-debian-chroot",
-             "--series", self.series,
-             "--arch", self.arch_tag,
-             self._buildid])
+        self.runTargetSubProcess(self._updatepath, ["update-debian-chroot"])
 
     def doRunBuild(self):
         """Run the main build process.

=== modified file 'lpbuildd/slave.py'
--- lpbuildd/slave.py	2017-07-28 13:19:47 +0000
+++ lpbuildd/slave.py	2017-08-04 01:13:06 +0000
@@ -22,6 +22,9 @@
 from twisted.python import log
 from twisted.web import xmlrpc
 
+from lpbuildd.target.backend import Backend
+
+
 devnull = open("/dev/null", "r")
 
 
@@ -101,6 +104,8 @@
 class BuildManager(object):
     """Build Daemon slave build manager abstract parent"""
 
+    backend_name = "chroot"
+
     def __init__(self, slave, buildid, reactor=None):
         """Create a BuildManager.
 
@@ -132,7 +137,7 @@
         return self.is_archive_private
 
     def runSubProcess(self, command, args, iterate=None, stdin=None, env=None):
-        """Run a sub process capturing the results in the log."""
+        """Run a subprocess capturing the results in the log."""
         if iterate is None:
             iterate = self.iterate
         self._subprocess = RunCapture(self._slave, iterate, stdin=stdin)
@@ -146,6 +151,16 @@
             self._subprocess, command, args, env=env,
             path=self.home, childFDs=childfds)
 
+    def runTargetSubProcess(self, command, args, **kwargs):
+        """Run a subprocess that operates on the target environment."""
+        base_args = [
+            "--backend=%s" % self.backend_name,
+            "--series=%s" % self.series,
+            "--arch=%s" % self.arch_tag,
+            self._buildid,
+            ]
+        self.runSubProcess(command, [args[0]] + base_args + args[1:], **kwargs)
+
     def doUnpack(self):
         """Unpack the build chroot."""
         self.runSubProcess(
@@ -204,6 +219,9 @@
                                             self._buildid, f))
         self._chroottarfile = self._slave.cachePath(chroot)
 
+        self.series = extra_args['series']
+        self.arch_tag = extra_args.get('arch_tag', self._slave.getArch())
+
         # Check whether this is a build in a private archive and
         # whether the URLs in the buildlog file should be sanitized
         # so that they do not contain any embedded authentication
@@ -211,6 +229,10 @@
         if extra_args.get('archive_private'):
             self.is_archive_private = True
 
+        self.backend = Backend.get(
+            self.backend_name, self._buildid,
+            series=self.series, arch=self.arch_tag)
+
         self.runSubProcess(self._preppath, ["slave-prep"])
 
     def status(self):

=== added file 'lpbuildd/target/add_trusted_keys.py'
--- lpbuildd/target/add_trusted_keys.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/add_trusted_keys.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,30 @@
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import logging
+import sys
+
+from lpbuildd.target.operation import Operation
+
+
+logger = logging.getLogger(__name__)
+
+
+class AddTrustedKeys(Operation):
+
+    description = "Write out new trusted keys."
+
+    def __init__(self, args=None, input_file=None):
+        super(AddTrustedKeys, self).__init__(args=args)
+        self.input_file = input_file or sys.stdin
+
+    def run(self):
+        """Add trusted keys from an input file."""
+        logger.info("Adding trusted keys to build-%s", self.args.build_id)
+        self.backend.run(["apt-key", "add", "-"], stdin=self.input_file)
+        self.backend.run(["apt-key", "list"])
+        return 0

=== added file 'lpbuildd/target/backend.py'
--- lpbuildd/target/backend.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/backend.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,55 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import os.path
+
+
+class BackendException(Exception):
+    pass
+
+
+class Backend:
+    """A backend implementation for the environment where we run builds."""
+
+    def __init__(self, build_id, series=None, arch=None):
+        self.build_id = build_id
+        self.series = series
+        self.arch = arch
+        self.build_path = os.path.join(os.environ["HOME"], "build-" + build_id)
+
+    @staticmethod
+    def get(name, build_id, series=None, arch=None):
+        if name == "chroot":
+            from lpbuildd.target.chroot import Chroot
+            backend_factory = Chroot
+        else:
+            raise KeyError("Unknown backend: %s" % name)
+        return backend_factory(build_id, series=series, arch=arch)
+
+    def run(self, args, env=None, input_text=None, **kwargs):
+        """Run a command in the target environment.
+
+        :param args: the command and arguments to run.
+        :param env: additional environment variables to set.
+        :param input_text: input text to pass on the command's stdin.
+        :param kwargs: additional keyword arguments for `subprocess.Popen`.
+        """
+        raise NotImplementedError
+
+    def copy_in(self, source_path, target_path):
+        """Copy a file into the target environment.
+
+        The target file will be owned by root/root and have the same
+        permission mode as the source file.
+
+        :param source_path: the path to the file that should be copied from
+            the host system.
+        :param target_path: the path where the file should be installed
+            inside the target environment, relative to the target
+            environment's root.
+        """
+        raise NotImplementedError

=== modified file 'lpbuildd/target/chroot.py'
--- lpbuildd/target/chroot.py	2017-08-03 12:14:34 +0000
+++ lpbuildd/target/chroot.py	2017-08-04 01:13:06 +0000
@@ -6,43 +6,34 @@
 __metaclass__ = type
 
 import os.path
+import stat
 import subprocess
-import sys
-import tempfile
-import time
 
+from lpbuildd.target.backend import Backend
 from lpbuildd.util import (
     set_personality,
     shell_escape,
     )
 
 
-class ChrootSetup:
+class Chroot(Backend):
     """Sets up a chroot."""
 
     def __init__(self, build_id, series=None, arch=None):
-        self.build_id = build_id
-        self.series = series
-        self.arch = arch
-        self.chroot_path = os.path.join(
-            os.environ["HOME"], "build-" + build_id, "chroot-autobuild")
-
-    def chroot(self, args, env=None, input_text=None, **kwargs):
-        """Run a command in the chroot.
-
-        :param args: the command and arguments to run.
-        """
+        super(Chroot, self).__init__(build_id, series=series, arch=arch)
+        self.chroot_path = os.path.join(self.build_path, "chroot-autobuild")
+
+    def run(self, args, env=None, input_text=None, **kwargs):
+        """See `Backend`."""
         if env:
             args = ["env"] + [
                 "%s=%s" % (key, shell_escape(value))
                 for key, value in env.items()] + args
         if self.arch is not None:
             args = set_personality(args, self.arch, series=self.series)
-        cmd = ["/usr/bin/sudo", "/usr/sbin/chroot", self.chroot_path] + args
-        if "cwd" not in kwargs:
-            kwargs["cwd"] = self.chroot_path
+        cmd = ["sudo", "/usr/sbin/chroot", self.chroot_path] + args
         if input_text is None:
-            subprocess.check_call(cmd, **kwargs)
+            subprocess.check_call(cmd, cwd=self.chroot_path, **kwargs)
         else:
             proc = subprocess.Popen(
                 cmd, stdin=subprocess.PIPE, universal_newlines=True, **kwargs)
@@ -50,50 +41,14 @@
             if proc.returncode:
                 raise subprocess.CalledProcessError(proc.returncode, cmd)
 
-    def insert_file(self, source_path, target_path, mode=0o644):
-        """Insert a file into the chroot.
-
-        :param source_path: the path to the file outside the chroot.
-        :param target_path: the path where the file should be installed
-            inside the chroot.
-        """
+    def copy_in(self, source_path, target_path):
+        """See `Backend`."""
+        # Use install(1) so that we can end up with root/root ownership with
+        # a minimum of subprocess calls; the buildd user may not make sense
+        # in the target.
+        mode = stat.S_IMODE(os.stat(source_path).st_mode)
         full_target_path = os.path.join(
             self.chroot_path, target_path.lstrip("/"))
         subprocess.check_call(
-            ["/usr/bin/sudo", "install",
-             "-o", "root", "-g", "root", "-m", "%o" % mode,
+            ["sudo", "install", "-o", "root", "-g", "root", "-m", "%o" % mode,
              source_path, full_target_path])
-
-    def update(self):
-        with open("/dev/null", "r") as devnull:
-            env = {
-                "LANG": "C",
-                "DEBIAN_FRONTEND": "noninteractive",
-                "TTY": "unknown",
-                }
-            apt_get = "/usr/bin/apt-get"
-            update_args = [apt_get, "-uy", "update"]
-            try:
-                self.chroot(update_args, env=env, stdin=devnull)
-            except subprocess.CalledProcessError:
-                print(
-                    "Waiting 15 seconds and trying again ...", file=sys.stderr)
-                time.sleep(15)
-                self.chroot(update_args, env=env, stdin=devnull)
-            upgrade_args = [
-                apt_get, "-o", "DPkg::Options::=--force-confold", "-uy",
-                "--purge", "dist-upgrade",
-                ]
-            self.chroot(upgrade_args, env=env, stdin=devnull)
-
-    def override_sources_list(self, archives):
-        with tempfile.NamedTemporaryFile() as sources_list:
-            for archive in archives:
-                print(archive, file=sources_list)
-            sources_list.flush()
-            self.insert_file(sources_list.name, "/etc/apt/sources.list")
-
-    def add_trusted_keys(self, input_file):
-        """Add trusted keys from an input file."""
-        self.chroot(["apt-key", "add", "-"], stdin=input_file)
-        self.chroot(["apt-key", "list"])

=== added file 'lpbuildd/target/operation.py'
--- lpbuildd/target/operation.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/operation.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,65 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+from argparse import ArgumentParser
+import logging
+import sys
+
+from lpbuildd.target.backend import Backend
+
+
+class Operation:
+    """An operation to perform on the target environment."""
+
+    def __init__(self, args=None):
+        self.parse_args(args=args)
+        self.backend = Backend.get(
+            self.args.backend, self.args.build_id,
+            series=self.args.series, arch=self.args.arch)
+
+    @property
+    def description(self):
+        """A description of this operation, passed to the argument parser."""
+        raise NotImplementedError
+
+    def make_parser(self):
+        parser = ArgumentParser(description=self.description)
+        parser.add_argument(
+            "--backend", choices=["chroot", "lxd"],
+            help="use this type of backend")
+        parser.add_argument(
+            "--series", metavar="SERIES", help="operate on series SERIES")
+        parser.add_argument(
+            "--arch", metavar="ARCH", help="operate on architecture ARCH")
+        parser.add_argument(
+            "build_id", metavar="ID", help="operate on build ID")
+        return parser
+
+    def parse_args(self, args=None):
+        self.args = self.make_parser().parse_args(args=args)
+
+    def run(self):
+        raise NotImplementedError
+
+
+def configure_logging():
+    class StdoutFilter(logging.Filter):
+        def filter(self, record):
+            return record.levelno <= logging.WARNING
+
+    class StderrFilter(logging.Filter):
+        def filter(self, record):
+            return record.levelno >= logging.ERROR
+
+    logger = logging.getLogger()
+    stdout_handler = logging.StreamHandler(stream=sys.stdout)
+    stdout_handler.addFilter(StdoutFilter())
+    stderr_handler = logging.StreamHandler(stream=sys.stderr)
+    stderr_handler.addFilter(StderrFilter())
+    for handler in (stdout_handler, stderr_handler):
+        logger.addHandler(handler)
+    logger.setLevel(logging.INFO)

=== added file 'lpbuildd/target/override_sources_list.py'
--- lpbuildd/target/override_sources_list.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/override_sources_list.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,35 @@
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import logging
+import tempfile
+
+from lpbuildd.target.operation import Operation
+
+
+logger = logging.getLogger(__name__)
+
+
+class OverrideSourcesList(Operation):
+
+    description = "Override sources.list in the target environment."
+
+    def make_parser(self):
+        parser = super(OverrideSourcesList, self).make_parser()
+        parser.add_argument(
+            "archives", metavar="ARCHIVE", nargs="+",
+            help="sources.list lines")
+        return parser
+
+    def run(self):
+        logger.info("Overriding sources.list in build-%s", self.args.build_id)
+        with tempfile.NamedTemporaryFile() as sources_list:
+            for archive in self.args.archives:
+                print(archive, file=sources_list)
+            sources_list.flush()
+            self.backend.copy_in(sources_list.name, "/etc/apt/sources.list")
+        return 0

=== added file 'lpbuildd/target/tests/test_add_trusted_keys.py'
--- lpbuildd/target/tests/test_add_trusted_keys.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/tests/test_add_trusted_keys.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,35 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import io
+
+from fixtures import (
+    EnvironmentVariable,
+    MockPatchObject,
+    )
+from testtools import TestCase
+
+from lpbuildd.target.add_trusted_keys import AddTrustedKeys
+
+
+class TestAddTrustedKeys(TestCase):
+
+    def test_add_trusted_keys(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
+        input_file = io.BytesIO()
+        add_trusted_keys = AddTrustedKeys(args=args, input_file=input_file)
+        # XXX cjwatson 2017-07-29: With a newer version of fixtures we could
+        # mock this at the subprocess level instead, but at the moment doing
+        # that wouldn't allow us to test stdin.
+        mock_backend_run = self.useFixture(
+            MockPatchObject(add_trusted_keys.backend, "run")).mock
+        add_trusted_keys.run()
+
+        self.assertEqual(2, len(mock_backend_run.mock_calls))
+        mock_backend_run.assert_has_calls([
+            ((["apt-key", "add", "-"],), {"stdin": input_file}),
+            ((["apt-key", "list"],), {}),
+            ])

=== modified file 'lpbuildd/target/tests/test_chroot.py'
--- lpbuildd/target/tests/test_chroot.py	2017-07-28 23:14:48 +0000
+++ lpbuildd/target/tests/test_chroot.py	2017-08-04 01:13:06 +0000
@@ -3,50 +3,29 @@
 
 __metaclass__ = type
 
-import io
-import sys
-import time
-from textwrap import dedent
+import os.path
 
 from fixtures import (
     EnvironmentVariable,
-    MockPatch,
-    MockPatchObject,
-    )
-from systemfixtures import (
-    FakeProcesses,
-    FakeTime,
-    )
+    TempDir,
+    )
+from systemfixtures import FakeProcesses
 from testtools import TestCase
 
-from lpbuildd.target.chroot import ChrootSetup
-from lpbuildd.tests.fakeslave import FakeMethod
-
-
-class MockInsertFile(FakeMethod):
-
-    def __init__(self, *args, **kwargs):
-        super(MockInsertFile, self).__init__(*args, **kwargs)
-        self.source_bytes = None
-
-    def __call__(self, source_path, *args, **kwargs):
-        with open(source_path, "rb") as source:
-            self.source_bytes = source.read()
-        return super(MockInsertFile, self).__call__(
-            source_path, *args, **kwargs)
-
-
-class TestChrootSetup(TestCase):
-
-    def test_chroot(self):
+from lpbuildd.target.chroot import Chroot
+
+
+class TestChroot(TestCase):
+
+    def test_run(self):
         self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
         processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="/usr/bin/sudo")
-        ChrootSetup("1", "xenial", "amd64").chroot(
+        processes_fixture.add(lambda _: {}, name="sudo")
+        Chroot("1", "xenial", "amd64").run(
             ["apt-get", "update"], env={"LANG": "C"})
 
         expected_args = [
-            ["/usr/bin/sudo", "/usr/sbin/chroot",
+            ["sudo", "/usr/sbin/chroot",
              "/expected/home/build-1/chroot-autobuild",
              "linux64", "env", "LANG=C", "apt-get", "update"],
             ]
@@ -54,120 +33,24 @@
             expected_args,
             [proc._args["args"] for proc in processes_fixture.procs])
 
-    def test_insert_file(self):
+    def test_copy_in(self):
         self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        source_dir = self.useFixture(TempDir()).path
         processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="/usr/bin/sudo")
-        source_path = "/path/to/source"
+        processes_fixture.add(lambda _: {}, name="sudo")
+        source_path = os.path.join(source_dir, "source")
+        with open(source_path, "w"):
+            pass
+        os.chmod(source_path, 0o644)
         target_path = "/path/to/target"
-        ChrootSetup("1", "xenial", "amd64").insert_file(
-            source_path, target_path)
+        Chroot("1", "xenial", "amd64").copy_in(source_path, target_path)
 
         expected_target_path = (
             "/expected/home/build-1/chroot-autobuild/path/to/target")
         expected_args = [
-            ["/usr/bin/sudo", "install",
-             "-o", "root", "-g", "root", "-m", "644",
+            ["sudo", "install", "-o", "root", "-g", "root", "-m", "644",
              source_path, expected_target_path],
             ]
         self.assertEqual(
             expected_args,
             [proc._args["args"] for proc in processes_fixture.procs])
-
-    def test_update_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="/usr/bin/sudo")
-        self.useFixture(FakeTime())
-        start_time = time.time()
-        ChrootSetup("1", "xenial", "amd64").update()
-
-        apt_get_args = [
-            "/usr/bin/sudo", "/usr/sbin/chroot",
-            "/expected/home/build-1/chroot-autobuild",
-            "linux64", "env",
-            "LANG=C",
-            "DEBIAN_FRONTEND=noninteractive",
-            "TTY=unknown",
-            "/usr/bin/apt-get",
-            ]
-        expected_args = [
-            apt_get_args + ["-uy", "update"],
-            apt_get_args + [
-                "-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
-                "dist-upgrade",
-                ],
-            ]
-        self.assertEqual(
-            expected_args,
-            [proc._args["args"] for proc in processes_fixture.procs])
-        self.assertEqual(start_time, time.time())
-
-    def test_update_first_run_fails(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        apt_get_proc_infos = iter([{"returncode": 1}, {}, {}])
-        processes_fixture.add(
-            lambda _: next(apt_get_proc_infos), name="/usr/bin/sudo")
-        mock_print = self.useFixture(MockPatch("__builtin__.print")).mock
-        self.useFixture(FakeTime())
-        start_time = time.time()
-        ChrootSetup("1", "xenial", "amd64").update()
-
-        apt_get_args = [
-            "/usr/bin/sudo", "/usr/sbin/chroot",
-            "/expected/home/build-1/chroot-autobuild",
-            "linux64", "env",
-            "LANG=C",
-            "DEBIAN_FRONTEND=noninteractive",
-            "TTY=unknown",
-            "/usr/bin/apt-get",
-            ]
-        expected_args = [
-            apt_get_args + ["-uy", "update"],
-            apt_get_args + ["-uy", "update"],
-            apt_get_args + [
-                "-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
-                "dist-upgrade",
-                ],
-            ]
-        self.assertEqual(
-            expected_args,
-            [proc._args["args"] for proc in processes_fixture.procs])
-        mock_print.assert_called_once_with(
-            "Waiting 15 seconds and trying again ...", file=sys.stderr)
-        self.assertEqual(start_time + 15, time.time())
-
-    def test_override_sources_list(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        setup = ChrootSetup("1")
-        mock_insert_file = self.useFixture(
-            MockPatchObject(setup, "insert_file", new=MockInsertFile())).mock
-        setup.override_sources_list([
-            "deb http://archive.ubuntu.com/ubuntu xenial main",
-            "deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main",
-            ])
-
-        self.assertEqual(dedent("""\
-            deb http://archive.ubuntu.com/ubuntu xenial main
-            deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main
-            """).encode("UTF-8"), mock_insert_file.source_bytes)
-        self.assertEqual(
-            ("/etc/apt/sources.list",),
-            mock_insert_file.extract_args()[0][1:])
-
-    def test_add_trusted_keys(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        setup = ChrootSetup("1")
-        # XXX cjwatson 2017-07-29: With a newer version of fixtures we could
-        # mock this at the subprocess level instead, but at the moment doing
-        # that wouldn't allow us to test stdin.
-        mock_chroot = self.useFixture(MockPatchObject(setup, "chroot")).mock
-        input_file = io.BytesIO()
-        setup.add_trusted_keys(input_file)
-
-        self.assertEqual(2, len(mock_chroot.mock_calls))
-        mock_chroot.assert_has_calls([
-            ((["apt-key", "add", "-"],), {"stdin": input_file}),
-            ((["apt-key", "list"],), {}),
-            ])

=== added file 'lpbuildd/target/tests/test_override_sources_list.py'
--- lpbuildd/target/tests/test_override_sources_list.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/tests/test_override_sources_list.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,49 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+from textwrap import dedent
+
+from fixtures import (
+    EnvironmentVariable,
+    MockPatchObject,
+    )
+from testtools import TestCase
+
+from lpbuildd.target.override_sources_list import OverrideSourcesList
+from lpbuildd.tests.fakeslave import FakeMethod
+
+
+class MockCopyIn(FakeMethod):
+
+    def __init__(self, *args, **kwargs):
+        super(MockCopyIn, self).__init__(*args, **kwargs)
+        self.source_bytes = None
+
+    def __call__(self, source_path, *args, **kwargs):
+        with open(source_path, "rb") as source:
+            self.source_bytes = source.read()
+        return super(MockCopyIn, self).__call__(source_path, *args, **kwargs)
+
+
+class TestOverrideSourcesList(TestCase):
+
+    def test_succeeds(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        args = [
+            "--backend=chroot", "--series=xenial", "--arch=amd64", "1",
+            "deb http://archive.ubuntu.com/ubuntu xenial main",
+            "deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main",
+            ]
+        override_sources_list = OverrideSourcesList(args=args)
+        mock_copy_in = self.useFixture(MockPatchObject(
+            override_sources_list.backend, "copy_in", new=MockCopyIn())).mock
+        override_sources_list.run()
+
+        self.assertEqual(dedent("""\
+            deb http://archive.ubuntu.com/ubuntu xenial main
+            deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main
+            """).encode("UTF-8"), mock_copy_in.source_bytes)
+        self.assertEqual(
+            ("/etc/apt/sources.list",), mock_copy_in.extract_args()[0][1:])

=== added file 'lpbuildd/target/tests/test_update.py'
--- lpbuildd/target/tests/test_update.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/tests/test_update.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,88 @@
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import time
+
+from fixtures import (
+    EnvironmentVariable,
+    FakeLogger,
+    )
+from systemfixtures import (
+    FakeProcesses,
+    FakeTime,
+    )
+from testtools import TestCase
+
+from lpbuildd.target.update import Update
+
+
+class TestUpdate(TestCase):
+
+    def test_succeeds(self):
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        processes_fixture.add(lambda _: {}, name="sudo")
+        self.useFixture(FakeTime())
+        start_time = time.time()
+        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
+        Update(args=args).run()
+
+        apt_get_args = [
+            "sudo", "/usr/sbin/chroot",
+            "/expected/home/build-1/chroot-autobuild",
+            "linux64", "env",
+            "LANG=C",
+            "DEBIAN_FRONTEND=noninteractive",
+            "TTY=unknown",
+            "/usr/bin/apt-get",
+            ]
+        expected_args = [
+            apt_get_args + ["-uy", "update"],
+            apt_get_args + [
+                "-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
+                "dist-upgrade",
+                ],
+            ]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])
+        self.assertEqual(start_time, time.time())
+
+    def test_first_run_fails(self):
+        logger = self.useFixture(FakeLogger())
+        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
+        processes_fixture = self.useFixture(FakeProcesses())
+        apt_get_proc_infos = iter([{"returncode": 1}, {}, {}])
+        processes_fixture.add(lambda _: next(apt_get_proc_infos), name="sudo")
+        self.useFixture(FakeTime())
+        start_time = time.time()
+        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
+        Update(args=args).run()
+
+        apt_get_args = [
+            "sudo", "/usr/sbin/chroot",
+            "/expected/home/build-1/chroot-autobuild",
+            "linux64", "env",
+            "LANG=C",
+            "DEBIAN_FRONTEND=noninteractive",
+            "TTY=unknown",
+            "/usr/bin/apt-get",
+            ]
+        expected_args = [
+            apt_get_args + ["-uy", "update"],
+            apt_get_args + ["-uy", "update"],
+            apt_get_args + [
+                "-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
+                "dist-upgrade",
+                ],
+            ]
+        self.assertEqual(
+            expected_args,
+            [proc._args["args"] for proc in processes_fixture.procs])
+        self.assertEqual(
+            "Updating target for build 1\n"
+            "Waiting 15 seconds and trying again ...\n",
+            logger.output)
+        self.assertEqual(start_time + 15, time.time())

=== added file 'lpbuildd/target/update.py'
--- lpbuildd/target/update.py	1970-01-01 00:00:00 +0000
+++ lpbuildd/target/update.py	2017-08-04 01:13:06 +0000
@@ -0,0 +1,43 @@
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import print_function
+
+__metaclass__ = type
+
+import logging
+import subprocess
+import time
+
+from lpbuildd.target.operation import Operation
+
+
+logger = logging.getLogger(__name__)
+
+
+class Update(Operation):
+
+    description = "Update the target environment."
+
+    def run(self):
+        logger.info("Updating target for build %s", self.args.build_id)
+        with open("/dev/null", "r") as devnull:
+            env = {
+                "LANG": "C",
+                "DEBIAN_FRONTEND": "noninteractive",
+                "TTY": "unknown",
+                }
+            apt_get = "/usr/bin/apt-get"
+            update_args = [apt_get, "-uy", "update"]
+            try:
+                self.backend.run(update_args, env=env, stdin=devnull)
+            except subprocess.CalledProcessError:
+                logger.warning("Waiting 15 seconds and trying again ...")
+                time.sleep(15)
+                self.backend.run(update_args, env=env, stdin=devnull)
+            upgrade_args = [
+                apt_get, "-o", "DPkg::Options::=--force-confold", "-uy",
+                "--purge", "dist-upgrade",
+                ]
+            self.backend.run(upgrade_args, env=env, stdin=devnull)
+        return 0

=== modified file 'lpbuildd/tests/test_debian.py'
--- lpbuildd/tests/test_debian.py	2017-07-28 13:57:47 +0000
+++ lpbuildd/tests/test_debian.py	2017-08-04 01:13:06 +0000
@@ -126,7 +126,9 @@
         self.assertEqual(DebianBuildState.SOURCES, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/override-sources-list',
-              'override-sources-list', self.buildid,
+              'override-sources-list',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
+              self.buildid,
               'deb http://ppa.launchpad.dev/owner/name/ubuntu xenial main'],
              None),
             self.buildmanager.commands[-1])
@@ -137,7 +139,8 @@
         self.assertEqual(DebianBuildState.UPDATE, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/update-debian-chroot',
-              'update-debian-chroot', '--series', 'xenial', '--arch', 'amd64',
+              'update-debian-chroot',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
               self.buildid],
              None),
             self.buildmanager.commands[-1])
@@ -225,7 +228,9 @@
         self.assertEqual(DebianBuildState.SOURCES, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/override-sources-list',
-              'override-sources-list', self.buildid,
+              'override-sources-list',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
+              self.buildid,
               'deb http://ppa.launchpad.dev/owner/name/ubuntu xenial main'],
              None),
             self.buildmanager.commands[-1])
@@ -236,6 +241,7 @@
         self.assertEqual(DebianBuildState.KEYS, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/add-trusted-keys', 'add-trusted-keys',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
               self.buildid],
              b'key material'),
             self.buildmanager.commands[-1])
@@ -246,7 +252,8 @@
         self.assertEqual(DebianBuildState.UPDATE, self.getState())
         self.assertEqual(
             (['sharepath/slavebin/update-debian-chroot',
-              'update-debian-chroot', '--series', 'xenial', '--arch', 'amd64',
+              'update-debian-chroot',
+              '--backend=chroot', '--series=xenial', '--arch=amd64',
               self.buildid],
              None),
             self.buildmanager.commands[-1])


Follow ups