[libvirt] [sandbox PATCH v4 16/21] Image: Add Volume Support

[Cedric Bosdonnat]

On Tue, 2015-09-08 at 17:27 +0100, Daniel P. Berrange wrote:
> On Fri, Aug 28, 2015 at 01:47:44PM +0000, Eren Yagdiran wrote:
> > Volumes let user to map host-paths into sandbox. Docker containers
> > need volumes for data persistence.
> 
> I'm a little bit puzzelled about how this feature is supposed
> to be used. IIUC, in the Docker json file we have something
> like
> 
> {
>     "/var/my-app-data/": {},
>     "/etc/some-config.d/": {},
> }

See here for the full syntax / documentation of volume mounts:
http://docs.docker.com/userguide/dockervolumes/#volume

> > +    def getVolumes(self):
> > +        volumes = self.json_data['container_config']['Volumes']

It seems this commit needs quite some rework: the Config/Volumes section
only gets the destination of the bind mounts created by docker, while
HostConfig/Mounts gets all the details:

    "Mounts": [
        {
            "Source": "/public",
            "Destination": "/home/public",
            "Mode": "",
            "RW": true
        },
        {
            "Source": "/home",
            "Destination": "ro",
            "Mode": "",
            "RW": true
        },
        {
            "Name": "43c4b4472e74bc5d1cf0a390f8707decade340df5e10cdea468bb191cac14a76",
            "Source": "/var/lib/docker/volumes/43c4b4472e74bc5d1cf0a390f8707decade340df5e10cdea468bb191cac14a76/_data",
            "Destination": "/srv/www",
            "Driver": "local",
            "Mode": "",
            "RW": true
        }
    ],

Note in this sample, that at least some versions of docker can't
properly handle the -v /path:ro flag (documented) and try to create a
bind mount to /ro ;)

In the same config, I'm having this for the Volumes:

        "Volumes": {
            "/srv/www": {}
        },

It really seems like volumes are just bind mounts to an automatically
created image. The purpose of this feature is easy data storing outside
the rootfs image.

Volumes and Mounts are both created with the docker run -v option. The
--volumes-from will just add the required items in the Mounts section of
the docker container config.

> > +        volumelist = []
> > +        if isinstance(volumes,collections.Iterable):
> > +          for key,value in volumes.iteritems():
> > +            volumelist.append(key)
> > +        return volumelist
> 
> This will just return a python list
> 
>   ["/var/my-app-data/", "/etc/some-config.d"]
> 
> 
> > diff --git a/virt-sandbox-image/virt-sandbox-image.py b/virt-sandbox-image/virt-sandbox-image.py
> > index 058738a..79f8d8c 100755
> > --- a/virt-sandbox-image/virt-sandbox-image.py
> 
> > @@ -150,6 +151,25 @@ def run(args):
> >          if networkArgs is not None:
> >              params.append('-N')
> >              params.append(networkArgs)
> > +        allVolumes = source.get_volume(configfile)
> > +        volumeArgs = args.volume
> > +        if volumeArgs is not None:
> > +            allVolumes = allVolumes + volumeArgs
> > +        for volume in allVolumes:
> > +            volumeSplit = volume.split(":")
> 
> We don't have any ':' in our returned list from getVolumes()

Indeed there is no ':' in the Volumes list, and there will be none in
the mounts section too. I think Eren mixed up the docker parameters and
the config file values.

--
Cedric

> 
> > +            volumelen = len(volumeSplit)
> > +            if volumelen == 2:
> > +                hostPath = volumeSplit[0]
> > +                guestPath = volumeSplit[1]
> > +            elif volumelen == 1:
> > +                guestPath = volumeSplit[0]
> > +                hostPath = storage_dir + guestPath
> > +                if not os.path.exists(hostPath):
> > +                    os.makedirs(hostPath)
> > +            else:
> > +                pass
> 
> So we seem to just skip this ?
> 
> > +            params.append("--mount")
> > +            params.append("host-bind:%s=%s" %(guestPath,hostPath))
> >          params.append('--')
> >          params.append(commandToRun)
> >          cmd = cmd + params
> 
> Regards,
> Daniel