Cleanup daemon/volumes

- Mount struct now called volumeMount
- Merged volume creation for each volume type (volumes-from, binds, normal
  volumes) so this only happens in once place
- Simplified container copy of volumes (for when `docker cp` is a
  volume)

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

daemon/volumes.go

@@ -14,17 +14,14 @@ import (
 	"github.com/docker/docker/pkg/mount"
 	"github.com/docker/docker/pkg/symlink"
 	"github.com/docker/docker/pkg/system"
-	"github.com/docker/docker/volumes"
 )
 
-type Mount struct {
-	MountToPath string
-	container   *Container
-	volume      *volumes.Volume
-	Writable    bool
-	copyData    bool
-	from        *Container
-	isBind      bool
+type volumeMount struct {
+	containerPath string
+	hostPath      string
+	writable      bool
+	copyData      bool
+	from          string
 }
 
 func (container *Container) prepareVolumes() error {
@@ -33,70 +30,119 @@ func (container *Container) prepareVolumes() error {
 		container.VolumesRW = make(map[string]bool)
 	}
 
+	if len(container.hostConfig.VolumesFrom) > 0 && container.AppliedVolumesFrom == nil {
+		container.AppliedVolumesFrom = make(map[string]struct{})
+	}
 	return container.createVolumes()
 }
 
-// sortedVolumeMounts returns the list of container volume mount points sorted in lexicographic order
-func (container *Container) sortedVolumeMounts() []string {
-	var mountPaths []string
-	for path := range container.Volumes {
-		mountPaths = append(mountPaths, path)
+func (container *Container) createVolumes() error {
+	mounts := make(map[string]*volumeMount)
+
+	// get the normal volumes
+	for path := range container.Config.Volumes {
+		path = filepath.Clean(path)
+		// skip if there is already a volume for this container path
+		if _, exists := container.Volumes[path]; exists {
+			continue
+		}
+
+		realPath, err := container.getResourcePath(path)
+		if err != nil {
+			return err
+		}
+		if stat, err := os.Stat(realPath); err == nil {
+			if !stat.IsDir() {
+				return fmt.Errorf("can't mount to container path, file exists - %s", path)
+			}
+		}
+
+		mnt := &volumeMount{
+			containerPath: path,
+			writable:      true,
+			copyData:      true,
+		}
+		mounts[mnt.containerPath] = mnt
 	}
 
-	sort.Strings(mountPaths)
-	return mountPaths
-}
+	// Get all the bind mounts
+	// track bind paths separately due to #10618
+	bindPaths := make(map[string]struct{})
+	for _, spec := range container.hostConfig.Binds {
+		mnt, err := parseBindMountSpec(spec)
+		if err != nil {
+			return err
+		}
 
-func (container *Container) createVolumes() error {
-	mounts, err := container.parseVolumeMountConfig()
-	if err != nil {
-		return err
+		// #10618
+		if _, exists := bindPaths[mnt.containerPath]; exists {
+			return fmt.Errorf("Duplicate volume mount %s", mnt.containerPath)
+		}
+
+		bindPaths[mnt.containerPath] = struct{}{}
+		mounts[mnt.containerPath] = mnt
 	}
 
-	for _, mnt := range mounts {
-		if err := mnt.initialize(); err != nil {
+	// Get volumes from
+	for _, from := range container.hostConfig.VolumesFrom {
+		cID, mode, err := parseVolumesFromSpec(from)
+		if err != nil {
 			return err
 		}
+		if _, exists := container.AppliedVolumesFrom[cID]; exists {
+			// skip since it's already been applied
+			continue
+		}
+
+		c, err := container.daemon.Get(cID)
+		if err != nil {
+			return fmt.Errorf("container %s not found, impossible to mount its volumes", cID)
+		}
+
+		for _, mnt := range c.volumeMounts() {
+			mnt.writable = mnt.writable && (mode == "rw")
+			mnt.from = cID
+			mounts[mnt.containerPath] = mnt
+		}
 	}
 
-	// On every start, this will apply any new `VolumesFrom` entries passed in via HostConfig, which may override volumes set in `create`
-	return container.applyVolumesFrom()
-}
+	for _, mnt := range mounts {
+		containerMntPath, err := symlink.FollowSymlinkInScope(filepath.Join(container.basefs, mnt.containerPath), container.basefs)
+		if err != nil {
+			return err
+		}
 
-func (m *Mount) initialize() error {
-	// No need to initialize anything since it's already been initialized
-	if hostPath, exists := m.container.Volumes[m.MountToPath]; exists {
-		// If this is a bind-mount/volumes-from, maybe it was passed in at start instead of create
-		// We need to make sure bind-mounts/volumes-from passed on start can override existing ones.
-		if (!m.volume.IsBindMount && !m.isBind) && m.from == nil {
-			return nil
+		// Create the actual volume
+		v, err := container.daemon.volumes.FindOrCreateVolume(mnt.hostPath, mnt.writable)
+		if err != nil {
+			return err
 		}
-		if m.volume.Path == hostPath {
-			return nil
+
+		container.VolumesRW[mnt.containerPath] = mnt.writable
+		container.Volumes[mnt.containerPath] = v.Path
+		v.AddContainer(container.ID)
+		if mnt.from != "" {
+			container.AppliedVolumesFrom[mnt.from] = struct{}{}
 		}
 
-		// Make sure we remove these old volumes we don't actually want now.
-		// Ignore any errors here since this is just cleanup, maybe someone volumes-from'd this volume
-		if v := m.container.daemon.volumes.Get(hostPath); v != nil {
-			v.RemoveContainer(m.container.ID)
-			m.container.daemon.volumes.Delete(v.Path)
+		if mnt.writable && mnt.copyData {
+			// Copy whatever is in the container at the containerPath to the volume
+			copyExistingContents(containerMntPath, v.Path)
 		}
 	}
 
-	// This is the full path to container fs + mntToPath
-	containerMntPath, err := symlink.FollowSymlinkInScope(filepath.Join(m.container.basefs, m.MountToPath), m.container.basefs)
-	if err != nil {
-		return err
-	}
-	m.container.VolumesRW[m.MountToPath] = m.Writable
-	m.container.Volumes[m.MountToPath] = m.volume.Path
-	m.volume.AddContainer(m.container.ID)
-	if m.Writable && m.copyData {
-		// Copy whatever is in the container at the mntToPath to the volume
-		copyExistingContents(containerMntPath, m.volume.Path)
+	return nil
+}
+
+// sortedVolumeMounts returns the list of container volume mount points sorted in lexicographic order
+func (container *Container) sortedVolumeMounts() []string {
+	var mountPaths []string
+	for path := range container.Volumes {
+		mountPaths = append(mountPaths, path)
 	}
 
-	return nil
+	sort.Strings(mountPaths)
+	return mountPaths
 }
 
 func (container *Container) VolumePaths() map[string]struct{} {
@@ -139,97 +185,30 @@ func (container *Container) derefVolumes() {
 	}
 }
 
-func (container *Container) parseVolumeMountConfig() (map[string]*Mount, error) {
-	var mounts = make(map[string]*Mount)
-	// Get all the bind mounts
-	for _, spec := range container.hostConfig.Binds {
-		path, mountToPath, writable, err := parseBindMountSpec(spec)
-		if err != nil {
-			return nil, err
-		}
-		// Check if a bind mount has already been specified for the same container path
-		if m, exists := mounts[mountToPath]; exists {
-			return nil, fmt.Errorf("Duplicate volume %q: %q already in use, mounted from %q", path, mountToPath, m.volume.Path)
-		}
-		// Check if a volume already exists for this and use it
-		vol, err := container.daemon.volumes.FindOrCreateVolume(path, writable)
-		if err != nil {
-			return nil, err
-		}
-		mounts[mountToPath] = &Mount{
-			container:   container,
-			volume:      vol,
-			MountToPath: mountToPath,
-			Writable:    writable,
-			isBind:      true, // in case the volume itself is a normal volume, but is being mounted in as a bindmount here
-		}
-	}
-
-	// Get the rest of the volumes
-	for path := range container.Config.Volumes {
-		// Check if this is already added as a bind-mount
-		path = filepath.Clean(path)
-		if _, exists := mounts[path]; exists {
-			continue
-		}
-
-		// Check if this has already been created
-		if _, exists := container.Volumes[path]; exists {
-			continue
-		}
-		realPath, err := container.getResourcePath(path)
-		if err != nil {
-			return nil, fmt.Errorf("failed to evaluate the absolute path of symlink")
-		}
-		if stat, err := os.Stat(realPath); err == nil {
-			if !stat.IsDir() {
-				return nil, fmt.Errorf("file exists at %s, can't create volume there", realPath)
-			}
-		}
-
-		vol, err := container.daemon.volumes.FindOrCreateVolume("", true)
-		if err != nil {
-			return nil, err
-		}
-		mounts[path] = &Mount{
-			container:   container,
-			MountToPath: path,
-			volume:      vol,
-			Writable:    true,
-			copyData:    true,
-		}
-	}
-
-	return mounts, nil
-}
-
-func parseBindMountSpec(spec string) (string, string, bool, error) {
-	var (
-		path, mountToPath string
-		writable          bool
-		arr               = strings.Split(spec, ":")
-	)
+func parseBindMountSpec(spec string) (*volumeMount, error) {
+	arr := strings.Split(spec, ":")
 
+	mnt := &volumeMount{}
 	switch len(arr) {
 	case 2:
-		path = arr[0]
-		mountToPath = arr[1]
-		writable = true
+		mnt.hostPath = arr[0]
+		mnt.containerPath = arr[1]
+		mnt.writable = true
 	case 3:
-		path = arr[0]
-		mountToPath = arr[1]
-		writable = validMountMode(arr[2]) && arr[2] == "rw"
+		mnt.hostPath = arr[0]
+		mnt.containerPath = arr[1]
+		mnt.writable = validMountMode(arr[2]) && arr[2] == "rw"
 	default:
-		return "", "", false, fmt.Errorf("Invalid volume specification: %s", spec)
+		return nil, fmt.Errorf("Invalid volume specification: %s", spec)
 	}
 
-	if !filepath.IsAbs(path) {
-		return "", "", false, fmt.Errorf("cannot bind mount volume: %s volume paths must be absolute.", path)
+	if !filepath.IsAbs(mnt.hostPath) {
+		return nil, fmt.Errorf("cannot bind mount volume: %s volume paths must be absolute.", mnt.hostPath)
 	}
 
-	path = filepath.Clean(path)
-	mountToPath = filepath.Clean(mountToPath)
-	return path, mountToPath, writable, nil
+	mnt.hostPath = filepath.Clean(mnt.hostPath)
+	mnt.containerPath = filepath.Clean(mnt.containerPath)
+	return mnt, nil
 }
 
 func parseVolumesFromSpec(spec string) (string, string, error) {
@@ -251,54 +230,6 @@ func parseVolumesFromSpec(spec string) (string, string, error) {
 	return id, mode, nil
 }
 
-func (container *Container) applyVolumesFrom() error {
-	volumesFrom := container.hostConfig.VolumesFrom
-	if len(volumesFrom) > 0 && container.AppliedVolumesFrom == nil {
-		container.AppliedVolumesFrom = make(map[string]struct{})
-	}
-
-	mountGroups := make(map[string][]*Mount)
-
-	for _, spec := range volumesFrom {
-		id, mode, err := parseVolumesFromSpec(spec)
-		if err != nil {
-			return err
-		}
-		if _, exists := container.AppliedVolumesFrom[id]; exists {
-			// Don't try to apply these since they've already been applied
-			continue
-		}
-
-		c, err := container.daemon.Get(id)
-		if err != nil {
-			return fmt.Errorf("Could not apply volumes of non-existent container %q.", id)
-		}
-
-		var (
-			fromMounts = c.VolumeMounts()
-			mounts     []*Mount
-		)
-
-		for _, mnt := range fromMounts {
-			mnt.Writable = mnt.Writable && (mode == "rw")
-			mounts = append(mounts, mnt)
-		}
-		mountGroups[id] = mounts
-	}
-
-	for id, mounts := range mountGroups {
-		for _, mnt := range mounts {
-			mnt.from = mnt.container
-			mnt.container = container
-			if err := mnt.initialize(); err != nil {
-				return err
-			}
-		}
-		container.AppliedVolumesFrom[id] = struct{}{}
-	}
-	return nil
-}
-
 func validMountMode(mode string) bool {
 	validModes := map[string]bool{
 		"rw": true,
@@ -344,13 +275,17 @@ func (container *Container) setupMounts() error {
 	return nil
 }
 
-func (container *Container) VolumeMounts() map[string]*Mount {
-	mounts := make(map[string]*Mount)
+func (container *Container) volumeMounts() map[string]*volumeMount {
+	mounts := make(map[string]*volumeMount)
 
-	for mountToPath, path := range container.Volumes {
-		if v := container.daemon.volumes.Get(path); v != nil {
-			mounts[mountToPath] = &Mount{volume: v, container: container, MountToPath: mountToPath, Writable: container.VolumesRW[mountToPath]}
+	for containerPath, path := range container.Volumes {
+		v := container.daemon.volumes.Get(path)
+		if v == nil {
+			// This should never happen
+			logrus.Debugf("reference by container %s to non-existent volume path %s", container.ID, path)
+			continue
 		}
+		mounts[containerPath] = &volumeMount{hostPath: path, containerPath: containerPath, writable: container.VolumesRW[containerPath]}
 	}
 
 	return mounts

integration-cli/docker_cli_build_test.go

@@ -4504,7 +4504,7 @@ func (s *DockerSuite) TestBuildExoticShellInterpolation(c *check.C) {
 
 	_, err := buildImage(name, `
 		FROM busybox
-		
+
 		ENV SOME_VAR a.b.c
 
 		RUN [ "$SOME_VAR"       = 'a.b.c' ]