Fix volumes-from/bind-mounts passed in on start

Fixes #9628
Slightly reverts #8683, HostConfig on start is _not_ deprecated.

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

daemon/volumes.go

@@ -24,6 +24,7 @@ type Mount struct {
 	volume      *volumes.Volume
 	Writable    bool
 	copyData    bool
+	from        *Container
 }
 
 func (mnt *Mount) Export(resource string) (io.ReadCloser, error) {
@@ -42,9 +43,6 @@ func (container *Container) prepareVolumes() error {
 	if container.Volumes == nil || len(container.Volumes) == 0 {
 		container.Volumes = make(map[string]string)
 		container.VolumesRW = make(map[string]bool)
-		if err := container.applyVolumesFrom(); err != nil {
-			return err
-		}
 	}
 
 	return container.createVolumes()
@@ -73,14 +71,28 @@ func (container *Container) createVolumes() error {
 		}
 	}
 
-	return nil
+	// On every start, this will apply any new `VolumesFrom` entries passed in via HostConfig, which may override volumes set in `create`
+	return container.applyVolumesFrom()
 }
 
 func (m *Mount) initialize() error {
 	// No need to initialize anything since it's already been initialized
-	if _, exists := m.container.Volumes[m.MountToPath]; exists {
+	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.from == nil {
 			return nil
 		}
+		if m.volume.Path == hostPath {
+			return nil
+		}
+
+		// 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
+		v := m.container.daemon.volumes.Get(hostPath)
+		v.RemoveContainer(m.container.ID)
+		m.container.daemon.volumes.Delete(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)
@@ -217,6 +229,7 @@ func (container *Container) applyVolumesFrom() error {
 
 	for _, mounts := range mountGroups {
 		for _, mnt := range mounts {
+			mnt.from = mnt.container
 			mnt.container = container
 			if err := mnt.initialize(); err != nil {
 				return err

docs/sources/reference/api/docker_remote_api.md

@@ -61,12 +61,6 @@ You can set the new container's MAC address explicitly.
 **New!**
 Volumes are now initialized when the container is created.
 
-`POST /containers/(id)/start`
-
-**New!**
-Passing the container's `HostConfig` on start is now deprecated.  You should
-set this when creating the container.
-
 `POST /containers/(id)/copy`
 
 **New!**

integration-cli/docker_api_containers_test.go

@@ -4,7 +4,10 @@ import (
 	"bytes"
 	"encoding/json"
 	"io"
+	"io/ioutil"
+	"os"
 	"os/exec"
+	"strings"
 	"testing"
 
 	"github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar"
@@ -120,3 +123,131 @@ func TestContainerApiGetChanges(t *testing.T) {
 
 	logDone("container REST API - check GET containers/changes")
 }
+
+func TestContainerApiStartVolumeBinds(t *testing.T) {
+	defer deleteAllContainers()
+	name := "testing"
+	config := map[string]interface{}{
+		"Image":   "busybox",
+		"Volumes": map[string]struct{}{"/tmp": {}},
+	}
+
+	if _, err := sockRequest("POST", "/containers/create?name="+name, config); err != nil && !strings.Contains(err.Error(), "201 Created") {
+		t.Fatal(err)
+	}
+
+	bindPath, err := ioutil.TempDir(os.TempDir(), "test")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	config = map[string]interface{}{
+		"Binds": []string{bindPath + ":/tmp"},
+	}
+	if _, err := sockRequest("POST", "/containers/"+name+"/start", config); err != nil && !strings.Contains(err.Error(), "204 No Content") {
+		t.Fatal(err)
+	}
+
+	pth, err := inspectFieldMap(name, "Volumes", "/tmp")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if pth != bindPath {
+		t.Fatalf("expected volume host path to be %s, got %s", bindPath, pth)
+	}
+
+	logDone("container REST API - check volume binds on start")
+}
+
+func TestContainerApiStartVolumesFrom(t *testing.T) {
+	defer deleteAllContainers()
+	volName := "voltst"
+	volPath := "/tmp"
+
+	if out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "run", "-d", "--name", volName, "-v", volPath, "busybox")); err != nil {
+		t.Fatal(out, err)
+	}
+
+	name := "testing"
+	config := map[string]interface{}{
+		"Image":   "busybox",
+		"Volumes": map[string]struct{}{volPath: {}},
+	}
+
+	if _, err := sockRequest("POST", "/containers/create?name="+name, config); err != nil && !strings.Contains(err.Error(), "201 Created") {
+		t.Fatal(err)
+	}
+
+	config = map[string]interface{}{
+		"VolumesFrom": []string{volName},
+	}
+	if _, err := sockRequest("POST", "/containers/"+name+"/start", config); err != nil && !strings.Contains(err.Error(), "204 No Content") {
+		t.Fatal(err)
+	}
+
+	pth, err := inspectFieldMap(name, "Volumes", volPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pth2, err := inspectFieldMap(volName, "Volumes", volPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if pth != pth2 {
+		t.Fatalf("expected volume host path to be %s, got %s", pth, pth2)
+	}
+
+	logDone("container REST API - check VolumesFrom on start")
+}
+
+// Ensure that volumes-from has priority over binds/anything else
+// This is pretty much the same as TestRunApplyVolumesFromBeforeVolumes, except with passing the VolumesFrom and the bind on start
+func TestVolumesFromHasPriority(t *testing.T) {
+	defer deleteAllContainers()
+	volName := "voltst"
+	volPath := "/tmp"
+
+	if out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "run", "-d", "--name", volName, "-v", volPath, "busybox")); err != nil {
+		t.Fatal(out, err)
+	}
+
+	name := "testing"
+	config := map[string]interface{}{
+		"Image":   "busybox",
+		"Volumes": map[string]struct{}{volPath: {}},
+	}
+
+	if _, err := sockRequest("POST", "/containers/create?name="+name, config); err != nil && !strings.Contains(err.Error(), "201 Created") {
+		t.Fatal(err)
+	}
+
+	bindPath, err := ioutil.TempDir(os.TempDir(), "test")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	config = map[string]interface{}{
+		"VolumesFrom": []string{volName},
+		"Binds":       []string{bindPath + ":/tmp"},
+	}
+	if _, err := sockRequest("POST", "/containers/"+name+"/start", config); err != nil && !strings.Contains(err.Error(), "204 No Content") {
+		t.Fatal(err)
+	}
+
+	pth, err := inspectFieldMap(name, "Volumes", volPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pth2, err := inspectFieldMap(volName, "Volumes", volPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if pth != pth2 {
+		t.Fatalf("expected volume host path to be %s, got %s", pth, pth2)
+	}
+
+	logDone("container REST API - check VolumesFrom has priority")
+}