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

On Wed, Sep 09, 2015 at 10:41:10AM +0200, Cedric Bosdonnat wrote: > 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: IIUC, we shouldn't really look at the HostConfig section at all - that reflects the host-specific configuration of the image when it was deployed on the original build host. We should work entirely from the Config section, to formulate our own host-specific configuration as needed by our tools.

> > "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. I think my confusion was due to this code block handling both the user supplied command line arguments, which can include the ':', and the image supplied mounts which don't include the ':' When I reposted this series, I put this patch last of all, so my inclination at this point is to not merge it at all yet. We'll just focus on the rest of the series, and investigate the volume stuff some more before considering it for merge.