Merge pull request #5918 from crosbymichael/volumes-commit

Do not commit host bind mounts into image

daemon/daemon.go

@@ -607,15 +607,18 @@ func (daemon *Daemon) Commit(container *Container, repository, tag, comment, aut
 		containerID, containerImage string
 		containerConfig             *runconfig.Config
 	)
+
 	if container != nil {
 		containerID = container.ID
 		containerImage = container.Image
 		containerConfig = container.Config
 	}
+
 	img, err := daemon.graph.Create(rwTar, containerID, containerImage, comment, author, containerConfig, config)
 	if err != nil {
 		return nil, err
 	}
+
 	// Register the image if needed
 	if repository != "" {
 		if err := daemon.repositories.Set(repository, tag, img.ID, true); err != nil {

daemon/volumes.go

@@ -162,115 +162,17 @@ func createVolumes(container *Container) error {
 		return err
 	}
 
-	volumesDriver := container.daemon.volumes.Driver()
 	// Create the requested volumes if they don't exist
 	for volPath := range container.Config.Volumes {
-		volPath = filepath.Clean(volPath)
-		volIsDir := true
-		// Skip existing volumes
-		if _, exists := container.Volumes[volPath]; exists {
-			continue
-		}
-		var srcPath string
-		var isBindMount bool
-		srcRW := false
-		// If an external bind is defined for this volume, use that as a source
-		if bindMap, exists := binds[volPath]; exists {
-			isBindMount = true
-			srcPath = bindMap.SrcPath
-			if !filepath.IsAbs(srcPath) {
-				return fmt.Errorf("%s must be an absolute path", srcPath)
-			}
-			if strings.ToLower(bindMap.Mode) == "rw" {
-				srcRW = true
-			}
-			if stat, err := os.Stat(bindMap.SrcPath); err != nil {
-				return err
-			} else {
-				volIsDir = stat.IsDir()
-			}
-			// Otherwise create an directory in $ROOT/volumes/ and use that
-		} else {
-
-			// Do not pass a container as the parameter for the volume creation.
-			// The graph driver using the container's information ( Image ) to
-			// create the parent.
-			c, err := container.daemon.volumes.Create(nil, "", "", "", "", nil, nil)
-			if err != nil {
-				return err
-			}
-			srcPath, err = volumesDriver.Get(c.ID, "")
-			if err != nil {
-				return fmt.Errorf("Driver %s failed to get volume rootfs %s: %s", volumesDriver, c.ID, err)
-			}
-			srcRW = true // RW by default
-		}
-
-		if p, err := filepath.EvalSymlinks(srcPath); err != nil {
-			return err
-		} else {
-			srcPath = p
-		}
-
-		// Create the mountpoint
-		rootVolPath, err := symlink.FollowSymlinkInScope(filepath.Join(container.basefs, volPath), container.basefs)
-		if err != nil {
+		if err := initializeVolume(container, volPath, binds); err != nil {
 			return err
 		}
+	}
 
-		newVolPath, err := filepath.Rel(container.basefs, rootVolPath)
-		if err != nil {
+	for volPath := range binds {
+		if err := initializeVolume(container, volPath, binds); err != nil {
 			return err
 		}
-		newVolPath = "/" + newVolPath
-
-		if volPath != newVolPath {
-			delete(container.Volumes, volPath)
-			delete(container.VolumesRW, volPath)
-		}
-
-		container.Volumes[newVolPath] = srcPath
-		container.VolumesRW[newVolPath] = srcRW
-
-		if err := createIfNotExists(rootVolPath, volIsDir); err != nil {
-			return err
-		}
-
-		// Do not copy or change permissions if we are mounting from the host
-		if srcRW && !isBindMount {
-			volList, err := ioutil.ReadDir(rootVolPath)
-			if err != nil {
-				return err
-			}
-			if len(volList) > 0 {
-				srcList, err := ioutil.ReadDir(srcPath)
-				if err != nil {
-					return err
-				}
-				if len(srcList) == 0 {
-					// If the source volume is empty copy files from the root into the volume
-					if err := archive.CopyWithTar(rootVolPath, srcPath); err != nil {
-						return err
-					}
-				}
-			}
-
-			var stat syscall.Stat_t
-			if err := syscall.Stat(rootVolPath, &stat); err != nil {
-				return err
-			}
-			var srcStat syscall.Stat_t
-			if err := syscall.Stat(srcPath, &srcStat); err != nil {
-				return err
-			}
-			// Change the source volume's ownership if it differs from the root
-			// files that were just copied
-			if stat.Uid != srcStat.Uid || stat.Gid != srcStat.Gid {
-				if err := os.Chown(srcPath, int(stat.Uid), int(stat.Gid)); err != nil {
-					return err
-				}
-			}
-		}
 	}
 	return nil
 }
@@ -296,3 +198,130 @@ func createIfNotExists(path string, isDir bool) error {
 	}
 	return nil
 }
+
+func initializeVolume(container *Container, volPath string, binds map[string]BindMap) error {
+	volumesDriver := container.daemon.volumes.Driver()
+	volPath = filepath.Clean(volPath)
+	// Skip existing volumes
+	if _, exists := container.Volumes[volPath]; exists {
+		return nil
+	}
+
+	var (
+		srcPath     string
+		isBindMount bool
+		volIsDir    = true
+
+		srcRW = false
+	)
+
+	// If an external bind is defined for this volume, use that as a source
+	if bindMap, exists := binds[volPath]; exists {
+		isBindMount = true
+		srcPath = bindMap.SrcPath
+		if !filepath.IsAbs(srcPath) {
+			return fmt.Errorf("%s must be an absolute path", srcPath)
+		}
+		if strings.ToLower(bindMap.Mode) == "rw" {
+			srcRW = true
+		}
+		if stat, err := os.Stat(bindMap.SrcPath); err != nil {
+			return err
+		} else {
+			volIsDir = stat.IsDir()
+		}
+		// Otherwise create an directory in $ROOT/volumes/ and use that
+	} else {
+		// Do not pass a container as the parameter for the volume creation.
+		// The graph driver using the container's information ( Image ) to
+		// create the parent.
+		c, err := container.daemon.volumes.Create(nil, "", "", "", "", nil, nil)
+		if err != nil {
+			return err
+		}
+		srcPath, err = volumesDriver.Get(c.ID, "")
+		if err != nil {
+			return fmt.Errorf("Driver %s failed to get volume rootfs %s: %s", volumesDriver, c.ID, err)
+		}
+		srcRW = true // RW by default
+	}
+
+	if p, err := filepath.EvalSymlinks(srcPath); err != nil {
+		return err
+	} else {
+		srcPath = p
+	}
+
+	// Create the mountpoint
+	rootVolPath, err := symlink.FollowSymlinkInScope(filepath.Join(container.basefs, volPath), container.basefs)
+	if err != nil {
+		return err
+	}
+
+	newVolPath, err := filepath.Rel(container.basefs, rootVolPath)
+	if err != nil {
+		return err
+	}
+	newVolPath = "/" + newVolPath
+
+	if volPath != newVolPath {
+		delete(container.Volumes, volPath)
+		delete(container.VolumesRW, volPath)
+	}
+
+	container.Volumes[newVolPath] = srcPath
+	container.VolumesRW[newVolPath] = srcRW
+
+	if err := createIfNotExists(rootVolPath, volIsDir); err != nil {
+		return err
+	}
+
+	// Do not copy or change permissions if we are mounting from the host
+	if srcRW && !isBindMount {
+		if err := copyExistingContents(rootVolPath, srcPath); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func copyExistingContents(rootVolPath, srcPath string) error {
+	volList, err := ioutil.ReadDir(rootVolPath)
+	if err != nil {
+		return err
+	}
+
+	if len(volList) > 0 {
+		srcList, err := ioutil.ReadDir(srcPath)
+		if err != nil {
+			return err
+		}
+
+		if len(srcList) == 0 {
+			// If the source volume is empty copy files from the root into the volume
+			if err := archive.CopyWithTar(rootVolPath, srcPath); err != nil {
+				return err
+			}
+		}
+	}
+
+	var (
+		stat    syscall.Stat_t
+		srcStat syscall.Stat_t
+	)
+
+	if err := syscall.Stat(rootVolPath, &stat); err != nil {
+		return err
+	}
+	if err := syscall.Stat(srcPath, &srcStat); err != nil {
+		return err
+	}
+	// Change the source volume's ownership if it differs from the root
+	// files that were just copied
+	if stat.Uid != srcStat.Uid || stat.Gid != srcStat.Gid {
+		if err := os.Chown(srcPath, int(stat.Uid), int(stat.Gid)); err != nil {
+			return err
+		}
+	}
+	return nil
+}

graph/graph.go

@@ -2,12 +2,6 @@ package graph
 
 import (
 	"fmt"
-	"github.com/dotcloud/docker/archive"
-	"github.com/dotcloud/docker/daemon/graphdriver"
-	"github.com/dotcloud/docker/dockerversion"
-	"github.com/dotcloud/docker/image"
-	"github.com/dotcloud/docker/runconfig"
-	"github.com/dotcloud/docker/utils"
 	"io"
 	"io/ioutil"
 	"os"
@@ -17,6 +11,13 @@ import (
 	"strings"
 	"syscall"
 	"time"
+
+	"github.com/dotcloud/docker/archive"
+	"github.com/dotcloud/docker/daemon/graphdriver"
+	"github.com/dotcloud/docker/dockerversion"
+	"github.com/dotcloud/docker/image"
+	"github.com/dotcloud/docker/runconfig"
+	"github.com/dotcloud/docker/utils"
 )
 
 // A Graph is a store for versioned filesystem images and the relationship between them.
@@ -141,11 +142,13 @@ func (graph *Graph) Create(layerData archive.ArchiveReader, containerID, contain
 		Architecture:  runtime.GOARCH,
 		OS:            runtime.GOOS,
 	}
+
 	if containerID != "" {
 		img.Parent = containerImage
 		img.Container = containerID
 		img.ContainerConfig = *containerConfig
 	}
+
 	if err := graph.Register(nil, layerData, img); err != nil {
 		return nil, err
 	}

integration-cli/docker_cli_commit_test.go

@@ -83,3 +83,28 @@ func TestCommitTTY(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestCommitWithHostBindMount(t *testing.T) {
+	cmd := exec.Command(dockerBinary, "run", "--name", "bind-commit", "-v", "/dev/null:/winning", "busybox", "true")
+	if _, err := runCommand(cmd); err != nil {
+		t.Fatal(err)
+	}
+
+	cmd = exec.Command(dockerBinary, "commit", "bind-commit", "bindtest")
+	imageId, _, err := runCommandWithOutput(cmd)
+	if err != nil {
+		t.Fatal(err)
+	}
+	imageId = strings.Trim(imageId, "\r\n")
+
+	cmd = exec.Command(dockerBinary, "run", "bindtest", "true")
+
+	if _, err := runCommand(cmd); err != nil {
+		t.Fatal(err)
+	}
+
+	deleteAllContainers()
+	deleteImages(imageId)
+
+	logDone("commit - commit bind mounted file")
+}

runconfig/config_test.go

@@ -1,9 +1,10 @@
 package runconfig
 
 import (
-	"github.com/dotcloud/docker/nat"
 	"strings"
 	"testing"
+
+	"github.com/dotcloud/docker/nat"
 )
 
 func parse(t *testing.T, args string) (*Config, *HostConfig, error) {
@@ -93,32 +94,20 @@ func TestParseRunVolumes(t *testing.T) {
 		t.Fatalf("Error parsing volume flags, `-v /var` is missing from volumes. Received %v", config.Volumes)
 	}
 
-	if config, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp"); hostConfig.Binds == nil || hostConfig.Binds[0] != "/hostTmp:/containerTmp" {
+	if _, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp"); hostConfig.Binds == nil || hostConfig.Binds[0] != "/hostTmp:/containerTmp" {
 		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp` should mount-bind /hostTmp into /containeTmp. Received %v", hostConfig.Binds)
-	} else if _, exists := config.Volumes["/containerTmp"]; !exists {
-		t.Fatalf("Error parsing volume flags, `-v /tmp` is missing from volumes. Received %v", config.Volumes)
 	}
 
-	if config, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp -v /hostVar:/containerVar"); hostConfig.Binds == nil || hostConfig.Binds[0] != "/hostTmp:/containerTmp" || hostConfig.Binds[1] != "/hostVar:/containerVar" {
+	if _, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp -v /hostVar:/containerVar"); hostConfig.Binds == nil || hostConfig.Binds[0] != "/hostTmp:/containerTmp" || hostConfig.Binds[1] != "/hostVar:/containerVar" {
 		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp -v /hostVar:/containerVar` should mount-bind /hostTmp into /containeTmp and /hostVar into /hostContainer. Received %v", hostConfig.Binds)
-	} else if _, exists := config.Volumes["/containerTmp"]; !exists {
-		t.Fatalf("Error parsing volume flags, `-v /containerTmp` is missing from volumes. Received %v", config.Volumes)
-	} else if _, exists := config.Volumes["/containerVar"]; !exists {
-		t.Fatalf("Error parsing volume flags, `-v /containerVar` is missing from volumes. Received %v", config.Volumes)
 	}
 
-	if config, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp:ro -v /hostVar:/containerVar:rw"); hostConfig.Binds == nil || hostConfig.Binds[0] != "/hostTmp:/containerTmp:ro" || hostConfig.Binds[1] != "/hostVar:/containerVar:rw" {
+	if _, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp:ro -v /hostVar:/containerVar:rw"); hostConfig.Binds == nil || hostConfig.Binds[0] != "/hostTmp:/containerTmp:ro" || hostConfig.Binds[1] != "/hostVar:/containerVar:rw" {
 		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp:ro -v /hostVar:/containerVar:rw` should mount-bind /hostTmp into /containeTmp and /hostVar into /hostContainer. Received %v", hostConfig.Binds)
-	} else if _, exists := config.Volumes["/containerTmp"]; !exists {
-		t.Fatalf("Error parsing volume flags, `-v /containerTmp` is missing from volumes. Received %v", config.Volumes)
-	} else if _, exists := config.Volumes["/containerVar"]; !exists {
-		t.Fatalf("Error parsing volume flags, `-v /containerVar` is missing from volumes. Received %v", config.Volumes)
 	}
 
 	if config, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp -v /containerVar"); hostConfig.Binds == nil || len(hostConfig.Binds) > 1 || hostConfig.Binds[0] != "/hostTmp:/containerTmp" {
 		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp -v /containerVar` should mount-bind only /hostTmp into /containeTmp. Received %v", hostConfig.Binds)
-	} else if _, exists := config.Volumes["/containerTmp"]; !exists {
-		t.Fatalf("Error parsing volume flags, `-v /containerTmp` is missing from volumes. Received %v", config.Volumes)
 	} else if _, exists := config.Volumes["/containerVar"]; !exists {
 		t.Fatalf("Error parsing volume flags, `-v /containerVar` is missing from volumes. Received %v", config.Volumes)
 	}

runconfig/merge.go

@@ -1,9 +1,10 @@
 package runconfig
 
 import (
+	"strings"
+
 	"github.com/dotcloud/docker/nat"
 	"github.com/dotcloud/docker/utils"
-	"strings"
 )
 
 func Merge(userConf, imageConf *Config) error {
@@ -82,6 +83,7 @@ func Merge(userConf, imageConf *Config) error {
 			}
 		}
 	}
+
 	if userConf.Cmd == nil || len(userConf.Cmd) == 0 {
 		userConf.Cmd = imageConf.Cmd
 	}

runconfig/parse.go

@@ -135,8 +135,8 @@ func parseRun(cmd *flag.FlagSet, args []string, sysInfo *sysinfo.SysInfo) (*Conf
 			if arr[0] == "/" {
 				return nil, nil, cmd, fmt.Errorf("Invalid bind mount: source can't be '/'")
 			}
-			dstDir := arr[1]
-			flVolumes.Set(dstDir)
+			// after creating the bind mount we want to delete it from the flVolumes values because
+			// we do not want bind mounts being committed to image configs
 			binds = append(binds, bind)
 			flVolumes.Delete(bind)
 		} else if bind == "/" {

server/server.go

@@ -1050,8 +1050,12 @@ func (srv *Server) ContainerCommit(job *engine.Job) engine.Status {
 	if container == nil {
 		return job.Errorf("No such container: %s", name)
 	}
-	var config = container.Config
-	var newConfig runconfig.Config
+
+	var (
+		config    = container.Config
+		newConfig runconfig.Config
+	)
+
 	if err := job.GetenvJson("config", &newConfig); err != nil {
 		return job.Error(err)
 	}