Merge pull request #24467 from thenewwazoo/fix-dockerio-bound-volumes

Fix dockerio bound volumes

salt/modules/dockerio.py

@@ -563,7 +563,8 @@ def create_container(image,
                      volumes_from=None,
                      name=None,
                      cpu_shares=None,
-                     cpuset=None):
+                     cpuset=None,
+                     binds=None):
     '''
     Create a new container
 
@@ -582,10 +583,26 @@ def create_container(image,
     ports
         port redirections ``({'222': {}})``
     volumes
-        list of volume mappings::
+        list of volume mappings in either local volume, bound volume, or read-only
+        bound volume form::
 
-            (['/mountpoint/in/container:/guest/foo', '/same/path/mounted/point'])
+            (['/var/lib/mysql/', '/usr/local/etc/ssl:/etc/ssl', '/etc/passwd:/etc/passwd:ro'])
+    binds
+        complete dictionary of bound volume mappings::
 
+            { '/usr/local/etc/ssl/certs/internal.crt': {
+                'bind': '/etc/ssl/certs/com.example.internal.crt',
+                'ro': True
+                },
+              '/var/lib/mysql': {
+                'bind': '/var/lib/mysql/',
+                'ro': False
+                }
+            }
+
+        This dictionary is suitable for feeding directly into the Docker API, and all
+        keys are required.
+        (see http://docker-py.readthedocs.org/en/latest/volumes/)
     tty
         attach ttys, Default is ``False``
     stdin_open
@@ -604,23 +621,31 @@ def create_container(image,
         salt '*' docker.create_container o/ubuntu volumes="['/s','/m:/f']"
 
     '''
+    log.trace("modules.dockerio.create_container() called for image " + image)
     status = base_status.copy()
     client = _get_client()
+
+    # In order to permit specification of bind volumes in the volumes field,
+    #  we'll look through it for bind-style specs and move them. This is purely
+    #  for CLI convenience and backwards-compatibility, as states.dockerio
+    #  should parse volumes before this, and the binds argument duplicates this.
+    # N.B. this duplicates code in states.dockerio._parse_volumes()
+    if isinstance(volumes, list):
+        for volume in volumes:
+            if ':' in volume:
+                volspec = volume.split(':')
+                source = volspec[0]
+                target = volspec[1]
+                ro = False
+                try:
+                    if len(volspec) > 2:
+                        ro = volspec[2] == "ro"
+                except IndexError:
+                    pass
+                binds[source] = {'bind': target, 'ro': ro}
+                volumes.remove(volume)
+
     try:
-        mountpoints = {}
-        binds = {}
-        # create empty mountpoints for them to be
-        # editable
-        # either we have a list of guest or host:guest
-        if isinstance(volumes, list):
-            for mountpoint in volumes:
-                mounted = mountpoint
-                if ':' in mountpoint:
-                    parts = mountpoint.split(':')
-                    mountpoint = parts[1]
-                    mounted = parts[0]
-                mountpoints[mountpoint] = {}
-                binds[mounted] = mountpoint
         container_info = client.create_container(
             image=image,
             command=command,
@@ -633,12 +658,14 @@ def create_container(image,
             ports=ports,
             environment=environment,
             dns=dns,
-            volumes=mountpoints,
+            volumes=volumes,
             volumes_from=volumes_from,
             name=name,
             cpu_shares=cpu_shares,
-            cpuset=cpuset
+            cpuset=cpuset,
+            host_config=docker.utils.create_host_config(binds=binds)
         )
+        log.trace("docker.client.create_container returned: " + str(container_info))
         container = container_info['Id']
         callback = _valid
         comment = 'Container created'
@@ -648,8 +675,9 @@ def create_container(image,
         }
         __salt__['mine.send']('docker.get_containers', host=True)
         return callback(status, id_=container, comment=comment, out=out)
-    except Exception:
+    except Exception, e:
         _invalid(status, id_=image, out=traceback.format_exc())
+        raise e
     __salt__['mine.send']('docker.get_containers', host=True)
     return status
 

salt/states/dockerio.py

@@ -184,6 +184,103 @@ def _invalid(exec_status=None, name='', comment='', changes=None):
                        result=False)
 
 
+def _parse_volumes(volumes):
+    '''
+    Parse a given volumes state specification for later use in
+    modules.docker.create_container(). This produces a dict that can be directly
+    consumed by the Docker API /containers/create.
+
+    Note: this only really exists for backwards-compatibility, and because
+    modules.dockerio.start() currently takes a binds argument.
+
+    volumes
+        A structure containing information about the volumes to be included in the
+        container that will be created, either:
+            - a bare dictionary
+            - a list of dictionaries and lists
+
+        .. code-block:: yaml
+
+            # bare dict style
+            - volumes:
+                /usr/local/etc/ssl/certs/example.crt:
+                  bind: /etc/ssl/certs/com.example.internal.crt
+                  ro: True
+                /var/run:
+                  bind: /var/run/host/
+                  ro: False
+
+            # list of dicts style:
+            - volumes:
+              - /usr/local/etc/ssl/certs/example.crt:
+                  bind: /etc/ssl/certs/com.example.internal.crt
+                  ro: True
+              - /var/run: /var/run/host/ # read-write bound volume
+              - /var/lib/mysql # un-bound, container-only volume
+
+        note: bind mounts specified like "/etc/timezone:/tmp/host_tz" will fall
+        through this parser.
+
+    Returns a dict of volume specifications:
+
+        .. code-block:: yaml
+
+            {
+              'bindvols': {
+                '/usr/local/etc/ssl/certs/example.crt': {
+                  'bind': '/etc/ssl/certs/com.example.internal.crt',
+                  'ro': True
+                  },
+                '/var/run/': {
+                  'bind': '/var/run/host',
+                  'ro': False
+                },
+              },
+              'contvols': [ '/var/lib/mysql/' ]
+            }
+
+    '''
+    log.trace("Parsing given volumes dict: " + str(volumes))
+    bindvolumes = {}
+    contvolumes = []
+    if isinstance(volumes, dict):
+        # If volumes as a whole is a dict, then there's no way to specify a non-bound volume
+        # so we exit early and assume the dict is properly formed.
+        bindvolumes = volumes
+    if isinstance(volumes, list):
+        for vol in volumes:
+            if isinstance(vol, dict):
+                for volsource, voldef in vol.items():
+                    if isinstance(voldef, dict):
+                        target = voldef['bind']
+                        read_only = voldef.get('ro', False)
+                    else:
+                        target = str(voldef)
+                        read_only = False
+                    source = volsource
+            else:  # isinstance(vol, dict)
+                if ':' in vol:
+                    volspec = vol.split(':')
+                    source = volspec[0]
+                    target = volspec[1]
+                    read_only = False
+                    try:
+                        if len(volspec) > 2:
+                            read_only = volspec[2] == "ro"
+                    except IndexError:
+                        pass
+                else:
+                    contvolumes.append(str(vol))
+                    continue
+            bindvolumes[source] = {
+                    'bind': target,
+                    'ro': read_only
+            }
+    result = {'bindvols': bindvolumes, 'contvols': contvolumes}
+    log.trace("Finished parsing volumes, with result: " + str(result))
+    return result
+
+
 def mod_watch(name, sfun=None, *args, **kw):
     if sfun == 'built':
         # Needs to refresh the image
@@ -479,7 +576,7 @@ def installed(name,
             - a port to map
             - a mapping of mapping portInHost : PortInContainer
     volumes
-        List of volumes
+        List of volumes (see notes for the running function)
 
     For other parameters, see absolutely first the salt.modules.dockerio
     execution module and the docker-py python bindings for docker
@@ -502,7 +599,7 @@ def installed(name,
     # if container exists but is not started, try to start it
     if already_exists:
         return _valid(comment='image {0!r} already exists'.format(name))
-    dports, dvolumes, denvironment = {}, [], {}
+    dports, denvironment = {}, {}
     if not ports:
         ports = []
     if not volumes:
@@ -521,15 +618,13 @@ def installed(name,
         else:
             for k in p:
                 dports[str(p)] = {}
-    for p in volumes:
-        vals = []
-        if not isinstance(p, dict):
-            vals.append('{0}'.format(p))
-        else:
-            for k in p:
-                vals.append('{0}:{1}'.format(k, p[k]))
-        dvolumes.extend(vals)
+
+    parsed_volumes = _parse_volumes(volumes)
+    bindvolumes = parsed_volumes['bindvols']
+    contvolumes = parsed_volumes['contvols']
+
     a, kw = [image], dict(
+        binds=bindvolumes,
         command=command,
         hostname=hostname,
         user=user,
@@ -540,7 +635,7 @@ def installed(name,
         ports=dports,
         environment=denvironment,
         dns=dns,
-        volumes=dvolumes,
+        volumes=contvolumes,
         volumes_from=volumes_from,
         name=name,
         cpu_shares=cpu_shares,
@@ -799,44 +894,47 @@ def running(name,
     volumes
         List of volumes to mount or create in the container (like ``-v`` of ``docker run`` command),
         mapping host directory to container directory.
-        To create a volume in the container:
+
+        To specify a volume in the container in terse list format:
 
         .. code-block:: yaml
 
             - volumes:
-              - "/var/log/service"
+              - "/var/log/service" # container-only volume
+              - "/srv/timezone:/etc/timezone" # bound volume
+              - "/usr/local/etc/passwd:/etc/passwd:ro" # read-only bound volume
 
-        For read-write mounting, use the short form (note that the notion of
+        You can also use the short dictionary form (note that the notion of
         source:target from docker is preserved):
 
         .. code-block:: yaml
 
             - volumes:
-              - /var/log/service: /var/log/service
+              - /var/log/service: /var/log/service # mandatory read-write implied
 
-        Or, to specify read-only mounting, use the extended form:
+        Or, alternatively, to specify read-only mounting, use the extended form:
 
         .. code-block:: yaml
 
             - volumes:
               - /home/user1:
-                bind: /mnt/vol2
-                ro: true
+                  bind: /mnt/vol2
+                  ro: True
               - /var/www:
-                bind: /mnt/vol1
-                ro: false
+                  bind: /mnt/vol1
+                  ro: False
 
-        Or (mostly for backwards compatibility) a dict style
+        Or (for backwards compatibility) another dict style:
 
         .. code-block:: yaml
 
             - volumes:
-              /home/user1:
-                bind: /mnt/vol2
-                ro: true
-              /var/www:
-                bind: /mnt/vol1
-                ro: false
+                /home/user1:
+                  bind: /mnt/vol2
+                  ro: True
+                /var/www:
+                  bind: /mnt/vol1
+                  ro: False
 
     volumes_from
         List of containers to share volumes with
@@ -934,27 +1032,6 @@ def running(name,
             if isinstance(var, dict):
                 for key in var:
                     denvironment[six.text_type(key)] = six.text_type(var[key])
-    if isinstance(volumes, dict):
-        bindvolumes = volumes
-    if isinstance(volumes, list):
-        for vol in volumes:
-            if isinstance(vol, dict):
-                # get source as the dict key
-                source = list(vol.keys())[0]
-                # then find target
-                if isinstance(vol[source], dict):
-                    target = vol[source]['bind']
-                    read_only = vol[source].get('ro', False)
-                else:
-                    target = str(vol[source])
-                    read_only = False
-                bindvolumes[source] = {
-                'bind': target,
-                'ro': read_only
-                }
-            else:
-                # assume just an own volumes
-                contvolumes.append(str(vol))
     if isinstance(ports, dict):
         bindports = ports
         # in dict form all ports bind, so no need for exposeports
@@ -976,6 +1053,11 @@ def running(name,
             else:
                 #assume just a port to expose
                 exposeports.append(str(port))
+
+    parsed_volumes = _parse_volumes(volumes)
+    bindvolumes = parsed_volumes['bindvols']
+    contvolumes = parsed_volumes['contvols']
+
     if not already_exists:
         args, kwargs = [image], dict(
                         command=command,
@@ -988,6 +1070,7 @@ def running(name,
                         ports=exposeports,
                         environment=denvironment,
                         dns=dns,
+                        binds=bindvolumes,
                         volumes=contvolumes,
                         name=name,
                         cpu_shares=cpu_shares,