On Wed, 2015-09-09 at 09:54 +0100, Daniel P. Berrange wrote:
> 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.
Yes, looks like docker build files VOLUME directory only can handle the
automatically created data volume:
http://docs.docker.com/reference/builder/#volume
Thus may be better to ignore bind mounts.
> >
> > "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.
Indeed.
--
Cedric
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list