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

[Cedric Bosdonnat]

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