Merge pull request #1366 from benallard/docker_better

Docker better

master/buildbot/buildslave/docker.py

@@ -16,6 +16,8 @@
 # Needed so that this module name don't clash with docker-py on older python.
 from __future__ import absolute_import
 
+import json
+
 from io import BytesIO
 
 from twisted.internet import defer
@@ -27,90 +29,123 @@
 from buildbot.buildslave import AbstractLatentBuildSlave
 
 try:
+    import docker
     from docker import client
-    _hush_pyflakes = [client]
+    _hush_pyflakes = [docker, client]
 except ImportError:
     client = None
 
 
+def handle_stream_line(line):
+    """\
+    Input is the json representation of: {'stream': "Content\ncontent"}
+    Output is a generator yield "Content", and then "content"
+    """
+    # XXX This necessary processing is probably a bug from docker-py,
+    # hence, might break if the bug is fixed, i.e. we should get decoded JSON
+    # directly from the API.
+    line = json.loads(line)
+    if 'error' in line:
+        content = "ERROR: " + line['error']
+    else:
+        content = line.get('stream', '')
+    for streamline in content.split('\n'):
+        if streamline:
+            yield streamline
+
+
 class DockerLatentBuildSlave(AbstractLatentBuildSlave):
     instance = None
 
     def __init__(self, name, password, docker_host, image=None, command=None,
-                 max_builds=None, notify_on_missing=None,
-                 missing_timeout=(60 * 20), build_wait_timeout=(60 * 10),
-                 properties={}, locks=None, volumes=None, dockerfile=None):
+                 volumes=None, dockerfile=None, version=None, tls=None,
+                 **kwargs):
 
         if not client:
-            config.error("The python module 'docker-py' is needed "
-                         "to use a DockerLatentBuildSlave")
-        if not image:
-            config.error("DockerLatentBuildSlave: You need to specify an"
-                         " image name")
-
-        AbstractLatentBuildSlave.__init__(self, name, password, max_builds,
-                                          notify_on_missing or [],
-                                          missing_timeout, build_wait_timeout,
-                                          properties, locks)
-
-        self.docker_host = docker_host
+            config.error("The python module 'docker-py' is needed to use a"
+                         " DockerLatentBuildSlave")
+        if not image and not dockerfile:
+            config.error("DockerLatentBuildSlave: You need to specify at least"
+                         " an image name, or a dockerfile")
+
+        self.volumes = []
+        self.binds = {}
+        for volume_string in (volumes or []):
+            try:
+                volume, bind = volume_string.split(":", 1)
+            except ValueError:
+                config.error("Invalid volume definition for docker "
+                             "{0}. Skipping...".format(volume_string))
+            self.volumes.append(volume)
+
+            ro = False
+            if bind.endswith(':ro') or bind.endswith(':rw'):
+                ro = bind[-2:] == 'ro'
+                bind = bind[:-3]
+            self.binds[volume] = {'bind': bind, 'ro': ro}
+
+        # Set build_wait_timeout to 0 if not explicitely set: Starting a
+        # container is almost immediate, we can affort doing so for each build.
+        if 'build_wait_timeout' not in kwargs:
+            kwargs['build_wait_timeout'] = 0
+        AbstractLatentBuildSlave.__init__(self, name, password, **kwargs)
+
         self.image = image
         self.command = command or []
-
-        self.volumes = volumes or []
         self.dockerfile = dockerfile
 
+        # Prepare the parameters for the Docker Client object.
+        self.client_args = {'base_url': docker_host}
+        if version is not None:
+            self.client_args['version'] = version
+        if tls is not None:
+            self.client_args['tls'] = tls
+
     def start_instance(self, build):
         if self.instance is not None:
             raise ValueError('instance active')
         return threads.deferToThread(self._thd_start_instance)
 
-    def _image_exists(self, client):
-        # Make sure the container exists
+    def _image_exists(self, client, name=None):
+        if name is None:
+            name = self.image
+        # Make sure the image exists
         for image in client.images():
             for tag in image['RepoTags']:
-                if ':' in self.image and tag == self.image:
+                if ':' in name and tag == name:
                     return True
-                if tag.startswith(self.image + ':'):
+                if tag.startswith(name + ':'):
                     return True
         return False
 
     def _thd_start_instance(self):
-        docker_client = client.Client(base_url=self.docker_host)
-
-        found = self._image_exists(docker_client)
+        docker_client = client.Client(**self.client_args)
+
+        found = False
+        if self.image is not None:
+            found = self._image_exists(docker_client)
+            image = self.image
+        else:
+            image = '%s_%s_image' % (self.slavename, id(self))
         if (not found) and (self.dockerfile is not None):
             log.msg("Image '%s' not found, building it from scratch" %
-                    self.image)
+                    image)
             for line in docker_client.build(fileobj=BytesIO(self.dockerfile.encode('utf-8')),
-                                            tag=self.image):
-                log.msg(line.rstrip())
+                                            tag=image):
+                for streamline in handle_stream_line(line):
+                    log.msg(streamline)
 
-        if not self._image_exists(docker_client):
-            log.msg("Image '%s' not found" % self.image)
+        if (not self._image_exists(docker_client, image)):
+            log.msg("Image '%s' not found" % image)
             raise interfaces.LatentBuildSlaveFailedToSubstantiate(
-                'Image "%s" not found on docker host.' % self.image
+                'Image "%s" not found on docker host.' % image
             )
 
-        volumes = {}
-        binds = {}
-        for volume_string in self.volumes:
-            try:
-                volume = volume_string.split(":")[1]
-            except IndexError:
-                log.err("Invalid volume definition for docker "
-                        "{0}. Skipping...".format(volume_string))
-                continue
-            volumes[volume] = {}
-
-            volume, bind = volume_string.split(':', 1)
-            binds[volume] = bind
-
         instance = docker_client.create_container(
-            self.image,
+            image,
             self.command,
             name='%s_%s' % (self.slavename, id(self)),
-            volumes=volumes,
+            volumes=self.volumes,
         )
 
         if instance.get('Id') is None:
@@ -120,8 +155,10 @@ def _thd_start_instance(self):
             )
 
         log.msg('Container created, Id: %s...' % instance['Id'][:6])
+        instance['image'] = image
         self.instance = instance
-        docker_client.start(instance['Id'], binds=binds)
+        docker_client.start(instance['Id'], binds=self.binds)
+        log.msg('Container started')
         return [instance['Id'], self.image]
 
     def stop_instance(self, fast=False):
@@ -135,9 +172,14 @@ def stop_instance(self, fast=False):
         return threads.deferToThread(self._thd_stop_instance, instance, fast)
 
     def _thd_stop_instance(self, instance, fast):
-        docker_client = client.Client(self.docker_host)
+        docker_client = client.Client(**self.client_args)
         log.msg('Stopping container %s...' % instance['Id'][:6])
         docker_client.stop(instance['Id'])
         if not fast:
             docker_client.wait(instance['Id'])
         docker_client.remove_container(instance['Id'], v=True, force=True)
+        if self.image is None:
+            try:
+                docker_client.remove_image(image=instance['image'])
+            except docker.errors.APIError as e:
+                log.msg('Error while removing the image: %s', e)

master/buildbot/test/unit/test_buildslave_docker.py

@@ -34,16 +34,54 @@ def test_constructor_nodocker(self):
         self.patch(dockerbuildslave, 'client', None)
         self.assertRaises(config.ConfigErrors, self.ConcreteBuildSlave, 'bot', 'pass', 'unix://tmp.sock', 'debian:wheezy', [])
 
-    def test_constructor_noimage(self):
+    def test_constructor_noimage_nodockerfile(self):
         self.assertRaises(config.ConfigErrors, self.ConcreteBuildSlave, 'bot', 'pass', 'http://localhost:2375')
 
+    def test_constructor_noimage_dockerfile(self):
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'http://localhost:2375', dockerfile="FROM ubuntu")
+        self.assertEqual(bs.dockerfile, "FROM ubuntu")
+        self.assertEqual(bs.image, None)
+
+    def test_constructor_image_nodockerfile(self):
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'http://localhost:2375', image="myslave")
+        self.assertEqual(bs.dockerfile, None)
+        self.assertEqual(bs.image, 'myslave')
+
     def test_constructor_minimal(self):
-        bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave', ['bin/bash'])
+        # Minimal set of parameters
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave')
         self.assertEqual(bs.slavename, 'bot')
         self.assertEqual(bs.password, 'pass')
-        self.assertEqual(bs.docker_host, 'tcp://1234:2375')
+        self.assertEqual(bs.client_args, {'base_url': 'tcp://1234:2375'})
         self.assertEqual(bs.image, 'slave')
-        self.assertEqual(bs.command, ['bin/bash'])
+        self.assertEqual(bs.command, [])
+
+    def test_constructor_all_docker_parameters(self):
+        # Volumes have their own tests
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'unix:///var/run/docker.sock', 'slave_img', ['/bin/sh'], dockerfile="FROM ubuntu", version='1.9', tls=True)
+        self.assertEqual(bs.slavename, 'bot')
+        self.assertEqual(bs.password, 'pass')
+        self.assertEqual(bs.image, 'slave_img')
+        self.assertEqual(bs.command, ['/bin/sh'])
+        self.assertEqual(bs.dockerfile, "FROM ubuntu")
+        self.assertEqual(bs.volumes, [])
+        self.assertEqual(bs.binds, {})
+        self.assertEqual(bs.client_args, {'base_url': 'unix:///var/run/docker.sock', 'version': '1.9', 'tls': True})
+
+    def test_rw_volume(self):
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave', ['bin/bash'], volumes=['/src/webapp:/opt/webapp'])
+        self.assertEqual(bs.volumes, ['/src/webapp'])
+        self.assertEqual(bs.binds, {'/src/webapp': {'bind': '/opt/webapp', 'ro': False}})
+
+    def test__ro_rw_volume(self):
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave', ['bin/bash'],
+                                     volumes=['~/.bash_history:/.bash_history',
+                                              '/src/webapp:/opt/webapp:ro',
+                                              '~:/backup:rw'])
+        self.assertEqual(bs.volumes, ['~/.bash_history', '/src/webapp', '~'])
+        self.assertEqual(bs.binds, {'~/.bash_history': {'bind': '/.bash_history', 'ro': False},
+                                    '/src/webapp': {'bind': '/opt/webapp', 'ro': True},
+                                    '~': {'bind': '/backup', 'ro': False}})
 
     @defer.inlineCallbacks
     def test_start_instance_image_no_version(self):
@@ -86,3 +124,24 @@ def test_start_instance_noimage_gooddockerfile(self):
         bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave', dockerfile='FROM debian:wheezy')
         id, name = yield bs.start_instance(None)
         self.assertEqual(name, 'slave')
+
+
+class testDockerPyStreamLogs(unittest.TestCase):
+
+    def compare(self, result, log):
+        self.assertEquals(result,
+                          list(dockerbuildslave.handle_stream_line(log)))
+
+    def testEmpty(self):
+        self.compare([], '{"stream":"\\n"}\r\n')
+
+    def testOneLine(self):
+        self.compare([" ---> Using cache"], '{"stream":" ---\\u003e Using cache\\n"}\r\n')
+
+    def testMultipleLines(self):
+        self.compare(["Fetched 8298 kB in 3s (2096 kB/s)", "Reading package lists..."],
+                     '{"stream":"Fetched 8298 kB in 3s (2096 kB/s)\\nReading package lists..."}\r\n')
+
+    def testError(self):
+        self.compare(["ERROR: The command [/bin/sh -c apt-get update && apt-get install -y    python-dev    python-pip] returned a non-zero code: 127"],
+                     '{"errorDetail": {"message": "The command [/bin/sh -c apt-get update && apt-get install -y    python-dev    python-pip] returned a non-zero code: 127"}, "error": "The command [/bin/sh -c apt-get update && apt-get install -y    python-dev    python-pip] returned a non-zero code: 127"}\r\n')

master/docs/manual/cfg-buildslaves-docker.rst

@@ -6,7 +6,9 @@ Docker latent BuildSlave
 ========================
 
 Docker_ is an open-source project that automates the deployment of applications inside software containers.
-Using the Docker latent BuildSlave, a fresh image will be instantiated upon each build, assuring consistency of the environment between builds.
+Using the Docker latent BuildSlave, an attempt is made at instantiating a fresh image upon each build, assuring consistency of the environment between builds.
+Each image will be discarded once the slave finished processing the build queue (i.e. becomes ``idle``).
+See :ref:`build_wait_timeout <Common-Latent-Buildslaves-Options>` to change this behavior.
 
 This document will guide you through the setup of such slaves.
 
@@ -149,7 +151,7 @@ In addition to the arguments available for any :ref:`Latent-Buildslaves`, :class
     This is the adress the master will use to connect with a running Docker instance.
 
 ``image``
-    (mandatory)
+    (optional if ``dockerfile`` is given)
     This is the name of the image that will be started by the build master. It should start a buildslave.
 
 ``command``
@@ -161,13 +163,21 @@ In addition to the arguments available for any :ref:`Latent-Buildslaves`, :class
     See `Setting up Volumes`_
 
 ``dockerfile``
-    (optional)
+    (optional if ``image`` is given)
     This is the content of the Dockerfile that will be used to build the specified image if the image is not found by Docker.
     It should be a multiline string.
 
-    .. note:: This parameter will be used only once as the next times the image will already be available.
+    .. note:: In case ``image`` and ``dockerfile`` are given, no attempt is made to compare the image with the content of the Dockerfile parameter if the image is found.
 
-    .. note:: No attempt is made to compare the image with the content of the Dockerfile parameter if the image is found.
+``version``
+    (optional, default to the highest version known by docker-py)
+    This will indicates wich API version must be used to communicate with Docker.
+
+``tls``
+    (optional)
+    This allow to use TLS when connecting with the Docker socket.
+    This should be a ``docker.tls.TLSConfig`` object.
+    See `docker-py's own documentation <http://docker-py.readthedocs.org/en/latest/tls/>`_ for more details on how to initialise this object.
 
 Setting up Volumes
 ..................
@@ -177,3 +187,6 @@ Refer to Docker documentation for more information about Volumes.
 
 The format of that variable has to be an array of string.
 Each string specify a volume in the following format: :samp:`{volumename}:{bindname}`.
+The volume name has to be appended with ``:ro`` if the volume should be mounted *read-only*.
+
+.. note:: This is the same format as when specifying volumes on the command line for docker's own ``-v`` option.

master/docs/manual/cfg-buildslaves.rst

@@ -131,6 +131,8 @@ Thanks to services such as Amazon Web Services' Elastic Compute Cloud ("AWS EC2"
 The buildslaves that are started on-demand are called "latent" buildslaves.
 As of this writing, buildbot ships with an abstract base class for building latent buildslaves, and a concrete implementation for AWS EC2 and for libvirt.
 
+.. _Common-Latent-Buildslaves-Options:
+
 Common Options
 ++++++++++++++