From 3bd73a96333e011738136f6a9eda23642cc204ab Mon Sep 17 00:00:00 2001
From: Greg Thornton <xdissent@me.com>
Date: Sat, 10 Aug 2013 04:55:23 +0000
Subject: [PATCH] Apply volumes-from before creating volumes

Copies the volumes from the container specified in `Config.VolumesFrom` before
creating volumes from `Config.Volumes`. Skips any preexisting volumes when
processing `Config.Volumes`. Fixes #1351
---
 container.go      | 60 ++++++++++++++++++++++++-----------------------
 container_test.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 29 deletions(-)

diff --git a/container.go b/container.go
index 8721d45a5..44797e672 100644
--- a/container.go
+++ b/container.go
@@ -574,40 +574,12 @@ func (container *Container) Start(hostConfig *HostConfig) error {
 		binds[path.Clean(dst)] = bindMap
 	}
 
-	// FIXME: evaluate volumes-from before individual volumes, so that the latter can override the former.
-	// Create the requested volumes volumes
 	if container.Volumes == nil || len(container.Volumes) == 0 {
 		container.Volumes = make(map[string]string)
 		container.VolumesRW = make(map[string]bool)
-
-		for volPath := range container.Config.Volumes {
-			volPath = path.Clean(volPath)
-			// If an external bind is defined for this volume, use that as a source
-			if bindMap, exists := binds[volPath]; exists {
-				container.Volumes[volPath] = bindMap.SrcPath
-				if strings.ToLower(bindMap.Mode) == "rw" {
-					container.VolumesRW[volPath] = true
-				}
-				// Otherwise create an directory in $ROOT/volumes/ and use that
-			} else {
-				c, err := container.runtime.volumes.Create(nil, container, "", "", nil)
-				if err != nil {
-					return err
-				}
-				srcPath, err := c.layer()
-				if err != nil {
-					return err
-				}
-				container.Volumes[volPath] = srcPath
-				container.VolumesRW[volPath] = true // RW by default
-			}
-			// Create the mountpoint
-			if err := os.MkdirAll(path.Join(container.RootfsPath(), volPath), 0755); err != nil {
-				return nil
-			}
-		}
 	}
 
+	// Apply volumes from another container if requested
 	if container.Config.VolumesFrom != "" {
 		c := container.runtime.Get(container.Config.VolumesFrom)
 		if c == nil {
@@ -627,6 +599,36 @@ func (container *Container) Start(hostConfig *HostConfig) error {
 		}
 	}
 
+	// Create the requested volumes if they don't exist
+	for volPath := range container.Config.Volumes {
+		volPath = path.Clean(volPath)
+		// If an external bind is defined for this volume, use that as a source
+		if _, exists := container.Volumes[volPath]; exists {
+			// Skip existing mounts
+		} else if bindMap, exists := binds[volPath]; exists {
+			container.Volumes[volPath] = bindMap.SrcPath
+			if strings.ToLower(bindMap.Mode) == "rw" {
+				container.VolumesRW[volPath] = true
+			}
+			// Otherwise create an directory in $ROOT/volumes/ and use that
+		} else {
+			c, err := container.runtime.volumes.Create(nil, container, "", "", nil)
+			if err != nil {
+				return err
+			}
+			srcPath, err := c.layer()
+			if err != nil {
+				return err
+			}
+			container.Volumes[volPath] = srcPath
+			container.VolumesRW[volPath] = true // RW by default
+		}
+		// Create the mountpoint
+		if err := os.MkdirAll(path.Join(container.RootfsPath(), volPath), 0755); err != nil {
+			return nil
+		}
+	}
+
 	if err := container.generateLXCConfig(); err != nil {
 		return err
 	}
diff --git a/container_test.go b/container_test.go
index aca53e5eb..c4f219373 100644
--- a/container_test.go
+++ b/container_test.go
@@ -1276,6 +1276,65 @@ func TestRestartWithVolumes(t *testing.T) {
 	}
 }
 
+// Test for #1351
+func TestVolumesFromWithVolumes(t *testing.T) {
+	runtime := mkRuntime(t)
+	defer nuke(runtime)
+
+	container, err := NewBuilder(runtime).Create(&Config{
+		Image:   GetTestImage(runtime).ID,
+		Cmd:     []string{"sh", "-c", "echo -n bar > /test/foo"},
+		Volumes: map[string]struct{}{"/test": {}},
+	},
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer runtime.Destroy(container)
+
+	for key := range container.Config.Volumes {
+		if key != "/test" {
+			t.Fail()
+		}
+	}
+
+	_, err = container.Output()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expected := container.Volumes["/test"]
+	if expected == "" {
+		t.Fail()
+	}
+
+	container2, err := NewBuilder(runtime).Create(
+		&Config{
+			Image:       GetTestImage(runtime).ID,
+			Cmd:         []string{"cat", "/test/foo"},
+			VolumesFrom: container.ID,
+			Volumes:     map[string]struct{}{"/test": {}},
+		},
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer runtime.Destroy(container2)
+
+	output, err := container2.Output()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if string(output) != "bar" {
+		t.Fail()
+	}
+
+	if container.Volumes["/test"] != container2.Volumes["/test"] {
+		t.Fail()
+	}
+}
+
 func TestOnlyLoopbackExistsWhenUsingDisableNetworkOption(t *testing.T) {
 	runtime := mkRuntime(t)
 	defer nuke(runtime)