Browse Source

Fix volume creates blocked by stale cache entries

When a conflict is found in the volume cache, check with the driver if
that volume still actually exists.
If the volume doesn't exist, purge it from the cache and allow the
create to happen.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Brian Goff 8 years ago
parent
commit
6a0bdffc1a

+ 95 - 57
integration-cli/docker_cli_external_volume_driver_unix_test.go

@@ -17,10 +17,13 @@ import (
 
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/pkg/integration/checker"
 	"github.com/docker/docker/pkg/integration/checker"
+	"github.com/docker/docker/pkg/stringid"
 	"github.com/docker/docker/volume"
 	"github.com/docker/docker/volume"
 	"github.com/go-check/check"
 	"github.com/go-check/check"
 )
 )
 
 
+const volumePluginName = "test-external-volume-driver"
+
 func init() {
 func init() {
 	check.Suite(&DockerExternalVolumeSuite{
 	check.Suite(&DockerExternalVolumeSuite{
 		ds: &DockerSuite{},
 		ds: &DockerSuite{},
@@ -40,10 +43,9 @@ type eventCounter struct {
 }
 }
 
 
 type DockerExternalVolumeSuite struct {
 type DockerExternalVolumeSuite struct {
-	server *httptest.Server
-	ds     *DockerSuite
-	d      *Daemon
-	ec     *eventCounter
+	ds *DockerSuite
+	d  *Daemon
+	*volumePlugin
 }
 }
 
 
 func (s *DockerExternalVolumeSuite) SetUpTest(c *check.C) {
 func (s *DockerExternalVolumeSuite) SetUpTest(c *check.C) {
@@ -57,8 +59,29 @@ func (s *DockerExternalVolumeSuite) TearDownTest(c *check.C) {
 }
 }
 
 
 func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
+	s.volumePlugin = newVolumePlugin(c, volumePluginName)
+}
+
+type volumePlugin struct {
+	ec *eventCounter
+	*httptest.Server
+	vols map[string]vol
+}
+
+type vol struct {
+	Name       string
+	Mountpoint string
+	Ninja      bool // hack used to trigger a null volume return on `Get`
+	Status     map[string]interface{}
+}
+
+func (p *volumePlugin) Close() {
+	p.Server.Close()
+}
+
+func newVolumePlugin(c *check.C, name string) *volumePlugin {
 	mux := http.NewServeMux()
 	mux := http.NewServeMux()
-	s.server = httptest.NewServer(mux)
+	s := &volumePlugin{Server: httptest.NewServer(mux), ec: &eventCounter{}, vols: make(map[string]vol)}
 
 
 	type pluginRequest struct {
 	type pluginRequest struct {
 		Name string
 		Name string
@@ -71,14 +94,6 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 		Err        string `json:",omitempty"`
 		Err        string `json:",omitempty"`
 	}
 	}
 
 
-	type vol struct {
-		Name       string
-		Mountpoint string
-		Ninja      bool // hack used to trigger a null volume return on `Get`
-		Status     map[string]interface{}
-	}
-	var volList []vol
-
 	read := func(b io.ReadCloser) (pluginRequest, error) {
 	read := func(b io.ReadCloser) (pluginRequest, error) {
 		defer b.Close()
 		defer b.Close()
 		var pr pluginRequest
 		var pr pluginRequest
@@ -115,14 +130,14 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 		}
 		}
 		_, isNinja := pr.Opts["ninja"]
 		_, isNinja := pr.Opts["ninja"]
 		status := map[string]interface{}{"Hello": "world"}
 		status := map[string]interface{}{"Hello": "world"}
-		volList = append(volList, vol{Name: pr.Name, Ninja: isNinja, Status: status})
+		s.vols[pr.Name] = vol{Name: pr.Name, Ninja: isNinja, Status: status}
 		send(w, nil)
 		send(w, nil)
 	})
 	})
 
 
 	mux.HandleFunc("/VolumeDriver.List", func(w http.ResponseWriter, r *http.Request) {
 	mux.HandleFunc("/VolumeDriver.List", func(w http.ResponseWriter, r *http.Request) {
 		s.ec.lists++
 		s.ec.lists++
-		vols := []vol{}
-		for _, v := range volList {
+		vols := make([]vol, 0, len(s.vols))
+		for _, v := range s.vols {
 			if v.Ninja {
 			if v.Ninja {
 				continue
 				continue
 			}
 			}
@@ -139,19 +154,19 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 			return
 			return
 		}
 		}
 
 
-		for _, v := range volList {
-			if v.Name == pr.Name {
-				if v.Ninja {
-					send(w, map[string]vol{})
-					return
-				}
+		v, exists := s.vols[pr.Name]
+		if !exists {
+			send(w, `{"Err": "no such volume"}`)
+		}
 
 
-				v.Mountpoint = hostVolumePath(pr.Name)
-				send(w, map[string]vol{"Volume": v})
-				return
-			}
+		if v.Ninja {
+			send(w, map[string]vol{})
+			return
 		}
 		}
-		send(w, `{"Err": "no such volume"}`)
+
+		v.Mountpoint = hostVolumePath(pr.Name)
+		send(w, map[string]vol{"Volume": v})
+		return
 	})
 	})
 
 
 	mux.HandleFunc("/VolumeDriver.Remove", func(w http.ResponseWriter, r *http.Request) {
 	mux.HandleFunc("/VolumeDriver.Remove", func(w http.ResponseWriter, r *http.Request) {
@@ -162,16 +177,17 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 			return
 			return
 		}
 		}
 
 
-		for i, v := range volList {
-			if v.Name == pr.Name {
-				if err := os.RemoveAll(hostVolumePath(v.Name)); err != nil {
-					send(w, &pluginResp{Err: err.Error()})
-					return
-				}
-				volList = append(volList[:i], volList[i+1:]...)
-				break
-			}
+		v, ok := s.vols[pr.Name]
+		if !ok {
+			send(w, nil)
+			return
+		}
+
+		if err := os.RemoveAll(hostVolumePath(v.Name)); err != nil {
+			send(w, &pluginResp{Err: err.Error()})
+			return
 		}
 		}
+		delete(s.vols, v.Name)
 		send(w, nil)
 		send(w, nil)
 	})
 	})
 
 
@@ -202,7 +218,7 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 			return
 			return
 		}
 		}
 
 
-		if err := ioutil.WriteFile(filepath.Join(p, "test"), []byte(s.server.URL), 0644); err != nil {
+		if err := ioutil.WriteFile(filepath.Join(p, "test"), []byte(s.Server.URL), 0644); err != nil {
 			send(w, err)
 			send(w, err)
 			return
 			return
 		}
 		}
@@ -242,12 +258,13 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
 	err := os.MkdirAll("/etc/docker/plugins", 0755)
 	err := os.MkdirAll("/etc/docker/plugins", 0755)
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
-	err = ioutil.WriteFile("/etc/docker/plugins/test-external-volume-driver.spec", []byte(s.server.URL), 0644)
+	err = ioutil.WriteFile("/etc/docker/plugins/"+name+".spec", []byte(s.Server.URL), 0644)
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
+	return s
 }
 }
 
 
 func (s *DockerExternalVolumeSuite) TearDownSuite(c *check.C) {
 func (s *DockerExternalVolumeSuite) TearDownSuite(c *check.C) {
-	s.server.Close()
+	s.volumePlugin.Close()
 
 
 	err := os.RemoveAll("/etc/docker/plugins")
 	err := os.RemoveAll("/etc/docker/plugins")
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
@@ -257,9 +274,9 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverNamed(c *check.C) {
 	err := s.d.StartWithBusybox()
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
-	out, err := s.d.Cmd("run", "--rm", "--name", "test-data", "-v", "external-volume-test:/tmp/external-volume-test", "--volume-driver", "test-external-volume-driver", "busybox:latest", "cat", "/tmp/external-volume-test/test")
+	out, err := s.d.Cmd("run", "--rm", "--name", "test-data", "-v", "external-volume-test:/tmp/external-volume-test", "--volume-driver", volumePluginName, "busybox:latest", "cat", "/tmp/external-volume-test/test")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
-	c.Assert(out, checker.Contains, s.server.URL)
+	c.Assert(out, checker.Contains, s.Server.URL)
 
 
 	_, err = s.d.Cmd("volume", "rm", "external-volume-test")
 	_, err = s.d.Cmd("volume", "rm", "external-volume-test")
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
@@ -280,9 +297,9 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnnamed(c *check.C)
 	err := s.d.StartWithBusybox()
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
-	out, err := s.d.Cmd("run", "--rm", "--name", "test-data", "-v", "/tmp/external-volume-test", "--volume-driver", "test-external-volume-driver", "busybox:latest", "cat", "/tmp/external-volume-test/test")
+	out, err := s.d.Cmd("run", "--rm", "--name", "test-data", "-v", "/tmp/external-volume-test", "--volume-driver", volumePluginName, "busybox:latest", "cat", "/tmp/external-volume-test/test")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
-	c.Assert(out, checker.Contains, s.server.URL)
+	c.Assert(out, checker.Contains, s.Server.URL)
 
 
 	c.Assert(s.ec.activations, checker.Equals, 1)
 	c.Assert(s.ec.activations, checker.Equals, 1)
 	c.Assert(s.ec.creations, checker.Equals, 1)
 	c.Assert(s.ec.creations, checker.Equals, 1)
@@ -295,7 +312,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverVolumesFrom(c *check
 	err := s.d.StartWithBusybox()
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
-	out, err := s.d.Cmd("run", "--name", "vol-test1", "-v", "/foo", "--volume-driver", "test-external-volume-driver", "busybox:latest")
+	out, err := s.d.Cmd("run", "--name", "vol-test1", "-v", "/foo", "--volume-driver", volumePluginName, "busybox:latest")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 
 
 	out, err = s.d.Cmd("run", "--rm", "--volumes-from", "vol-test1", "--name", "vol-test2", "busybox", "ls", "/tmp")
 	out, err = s.d.Cmd("run", "--rm", "--volumes-from", "vol-test1", "--name", "vol-test2", "busybox", "ls", "/tmp")
@@ -315,7 +332,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverDeleteContainer(c *c
 	err := s.d.StartWithBusybox()
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
-	out, err := s.d.Cmd("run", "--name", "vol-test1", "-v", "/foo", "--volume-driver", "test-external-volume-driver", "busybox:latest")
+	out, err := s.d.Cmd("run", "--name", "vol-test1", "-v", "/foo", "--volume-driver", volumePluginName, "busybox:latest")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 
 
 	out, err = s.d.Cmd("rm", "-fv", "vol-test1")
 	out, err = s.d.Cmd("rm", "-fv", "vol-test1")
@@ -388,7 +405,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverRetryNotImmediatelyE
 		// wait for a retry to occur, then create spec to allow plugin to register
 		// wait for a retry to occur, then create spec to allow plugin to register
 		time.Sleep(2000 * time.Millisecond)
 		time.Sleep(2000 * time.Millisecond)
 		// no need to check for an error here since it will get picked up by the timeout later
 		// no need to check for an error here since it will get picked up by the timeout later
-		ioutil.WriteFile(specPath, []byte(s.server.URL), 0644)
+		ioutil.WriteFile(specPath, []byte(s.Server.URL), 0644)
 	}()
 	}()
 
 
 	select {
 	select {
@@ -409,7 +426,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverRetryNotImmediatelyE
 }
 }
 
 
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverBindExternalVolume(c *check.C) {
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverBindExternalVolume(c *check.C) {
-	dockerCmd(c, "volume", "create", "-d", "test-external-volume-driver", "foo")
+	dockerCmd(c, "volume", "create", "-d", volumePluginName, "foo")
 	dockerCmd(c, "run", "-d", "--name", "testing", "-v", "foo:/bar", "busybox", "top")
 	dockerCmd(c, "run", "-d", "--name", "testing", "-v", "foo:/bar", "busybox", "top")
 
 
 	var mounts []struct {
 	var mounts []struct {
@@ -420,18 +437,18 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverBindExternalVolume(c
 	c.Assert(json.NewDecoder(strings.NewReader(out)).Decode(&mounts), checker.IsNil)
 	c.Assert(json.NewDecoder(strings.NewReader(out)).Decode(&mounts), checker.IsNil)
 	c.Assert(len(mounts), checker.Equals, 1, check.Commentf(out))
 	c.Assert(len(mounts), checker.Equals, 1, check.Commentf(out))
 	c.Assert(mounts[0].Name, checker.Equals, "foo")
 	c.Assert(mounts[0].Name, checker.Equals, "foo")
-	c.Assert(mounts[0].Driver, checker.Equals, "test-external-volume-driver")
+	c.Assert(mounts[0].Driver, checker.Equals, volumePluginName)
 }
 }
 
 
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverList(c *check.C) {
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverList(c *check.C) {
-	dockerCmd(c, "volume", "create", "-d", "test-external-volume-driver", "abc3")
+	dockerCmd(c, "volume", "create", "-d", volumePluginName, "abc3")
 	out, _ := dockerCmd(c, "volume", "ls")
 	out, _ := dockerCmd(c, "volume", "ls")
 	ls := strings.Split(strings.TrimSpace(out), "\n")
 	ls := strings.Split(strings.TrimSpace(out), "\n")
 	c.Assert(len(ls), check.Equals, 2, check.Commentf("\n%s", out))
 	c.Assert(len(ls), check.Equals, 2, check.Commentf("\n%s", out))
 
 
 	vol := strings.Fields(ls[len(ls)-1])
 	vol := strings.Fields(ls[len(ls)-1])
 	c.Assert(len(vol), check.Equals, 2, check.Commentf("%v", vol))
 	c.Assert(len(vol), check.Equals, 2, check.Commentf("%v", vol))
-	c.Assert(vol[0], check.Equals, "test-external-volume-driver")
+	c.Assert(vol[0], check.Equals, volumePluginName)
 	c.Assert(vol[1], check.Equals, "abc3")
 	c.Assert(vol[1], check.Equals, "abc3")
 
 
 	c.Assert(s.ec.lists, check.Equals, 1)
 	c.Assert(s.ec.lists, check.Equals, 1)
@@ -440,10 +457,10 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverList(c *check.C) {
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverGet(c *check.C) {
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverGet(c *check.C) {
 	out, _, err := dockerCmdWithError("volume", "inspect", "dummy")
 	out, _, err := dockerCmdWithError("volume", "inspect", "dummy")
 	c.Assert(err, check.NotNil, check.Commentf(out))
 	c.Assert(err, check.NotNil, check.Commentf(out))
-	c.Assert(s.ec.gets, check.Equals, 1)
 	c.Assert(out, checker.Contains, "No such volume")
 	c.Assert(out, checker.Contains, "No such volume")
+	c.Assert(s.ec.gets, check.Equals, 1)
 
 
-	dockerCmd(c, "volume", "create", "test", "-d", "test-external-volume-driver")
+	dockerCmd(c, "volume", "create", "test", "-d", volumePluginName)
 	out, _ = dockerCmd(c, "volume", "inspect", "test")
 	out, _ = dockerCmd(c, "volume", "inspect", "test")
 
 
 	type vol struct {
 	type vol struct {
@@ -458,7 +475,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverGet(c *check.C) {
 }
 }
 
 
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverWithDaemonRestart(c *check.C) {
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverWithDaemonRestart(c *check.C) {
-	dockerCmd(c, "volume", "create", "-d", "test-external-volume-driver", "abc1")
+	dockerCmd(c, "volume", "create", "-d", volumePluginName, "abc1")
 	err := s.d.Restart()
 	err := s.d.Restart()
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
@@ -466,7 +483,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverWithDaemonRestart(c
 	var mounts []types.MountPoint
 	var mounts []types.MountPoint
 	inspectFieldAndMarshall(c, "test", "Mounts", &mounts)
 	inspectFieldAndMarshall(c, "test", "Mounts", &mounts)
 	c.Assert(mounts, checker.HasLen, 1)
 	c.Assert(mounts, checker.HasLen, 1)
-	c.Assert(mounts[0].Driver, checker.Equals, "test-external-volume-driver")
+	c.Assert(mounts[0].Driver, checker.Equals, volumePluginName)
 }
 }
 
 
 // Ensures that the daemon handles when the plugin responds to a `Get` request with a null volume and a null error.
 // Ensures that the daemon handles when the plugin responds to a `Get` request with a null volume and a null error.
@@ -474,7 +491,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverWithDaemonRestart(c
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverGetEmptyResponse(c *check.C) {
 func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverGetEmptyResponse(c *check.C) {
 	c.Assert(s.d.Start(), checker.IsNil)
 	c.Assert(s.d.Start(), checker.IsNil)
 
 
-	out, err := s.d.Cmd("volume", "create", "-d", "test-external-volume-driver", "abc2", "--opt", "ninja=1")
+	out, err := s.d.Cmd("volume", "create", "-d", volumePluginName, "abc2", "--opt", "ninja=1")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 
 
 	out, err = s.d.Cmd("volume", "inspect", "abc2")
 	out, err = s.d.Cmd("volume", "inspect", "abc2")
@@ -505,7 +522,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverMountID(c *check.C)
 	err := s.d.StartWithBusybox()
 	err := s.d.StartWithBusybox()
 	c.Assert(err, checker.IsNil)
 	c.Assert(err, checker.IsNil)
 
 
-	out, err := s.d.Cmd("run", "--rm", "-v", "external-volume-test:/tmp/external-volume-test", "--volume-driver", "test-external-volume-driver", "busybox:latest", "cat", "/tmp/external-volume-test/test")
+	out, err := s.d.Cmd("run", "--rm", "-v", "external-volume-test:/tmp/external-volume-test", "--volume-driver", volumePluginName, "busybox:latest", "cat", "/tmp/external-volume-test/test")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 	c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
 	c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
 }
 }
@@ -516,7 +533,7 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverCapabilities(c *chec
 	c.Assert(s.ec.caps, checker.Equals, 0)
 	c.Assert(s.ec.caps, checker.Equals, 0)
 
 
 	for i := 0; i < 3; i++ {
 	for i := 0; i < 3; i++ {
-		out, err := s.d.Cmd("volume", "create", "-d", "test-external-volume-driver", fmt.Sprintf("test%d", i))
+		out, err := s.d.Cmd("volume", "create", "-d", volumePluginName, fmt.Sprintf("test%d", i))
 		c.Assert(err, checker.IsNil, check.Commentf(out))
 		c.Assert(err, checker.IsNil, check.Commentf(out))
 		c.Assert(s.ec.caps, checker.Equals, 1)
 		c.Assert(s.ec.caps, checker.Equals, 1)
 		out, err = s.d.Cmd("volume", "inspect", "--format={{.Scope}}", fmt.Sprintf("test%d", i))
 		out, err = s.d.Cmd("volume", "inspect", "--format={{.Scope}}", fmt.Sprintf("test%d", i))
@@ -524,3 +541,24 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverCapabilities(c *chec
 		c.Assert(strings.TrimSpace(out), checker.Equals, volume.GlobalScope)
 		c.Assert(strings.TrimSpace(out), checker.Equals, volume.GlobalScope)
 	}
 	}
 }
 }
+
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverOutOfBandDelete(c *check.C) {
+	driverName := stringid.GenerateNonCryptoID()
+	p := newVolumePlugin(c, driverName)
+	defer p.Close()
+
+	c.Assert(s.d.StartWithBusybox(), checker.IsNil)
+
+	out, err := s.d.Cmd("volume", "create", "-d", driverName, "--name", "test")
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+
+	out, err = s.d.Cmd("volume", "create", "-d", "local", "--name", "test")
+	c.Assert(err, checker.NotNil, check.Commentf(out))
+	c.Assert(out, checker.Contains, "volume named test already exists")
+
+	// simulate out of band volume deletion on plugin level
+	delete(p.vols, "test")
+
+	out, err = s.d.Cmd("volume", "create", "-d", "local", "--name", "test")
+	c.Assert(err, checker.IsNil, check.Commentf(out))
+}

+ 4 - 2
volume/store/errors.go

@@ -1,8 +1,9 @@
 package store
 package store
 
 
 import (
 import (
-	"errors"
 	"strings"
 	"strings"
+
+	"github.com/pkg/errors"
 )
 )
 
 
 var (
 var (
@@ -64,11 +65,12 @@ func IsNameConflict(err error) bool {
 }
 }
 
 
 func isErr(err error, expected error) bool {
 func isErr(err error, expected error) bool {
+	err = errors.Cause(err)
 	switch pe := err.(type) {
 	switch pe := err.(type) {
 	case nil:
 	case nil:
 		return false
 		return false
 	case *OpErr:
 	case *OpErr:
-		err = pe.Err
+		err = errors.Cause(pe.Err)
 	}
 	}
 	return err == expected
 	return err == expected
 }
 }

+ 90 - 19
volume/store/store.go

@@ -3,6 +3,7 @@ package store
 import (
 import (
 	"bytes"
 	"bytes"
 	"encoding/json"
 	"encoding/json"
+	"net"
 	"os"
 	"os"
 	"path/filepath"
 	"path/filepath"
 	"sync"
 	"sync"
@@ -117,6 +118,15 @@ func (s *VolumeStore) setNamed(v volume.Volume, ref string) {
 	s.globalLock.Unlock()
 	s.globalLock.Unlock()
 }
 }
 
 
+// getRefs gets the list of refs for a given name
+// Callers of this function are expected to hold the name lock.
+func (s *VolumeStore) getRefs(name string) []string {
+	s.globalLock.Lock()
+	refs := s.refs[name]
+	s.globalLock.Unlock()
+	return refs
+}
+
 // Purge allows the cleanup of internal data on docker in case
 // Purge allows the cleanup of internal data on docker in case
 // the internal data is out of sync with volumes driver plugins.
 // the internal data is out of sync with volumes driver plugins.
 func (s *VolumeStore) Purge(name string) {
 func (s *VolumeStore) Purge(name string) {
@@ -251,9 +261,77 @@ func (s *VolumeStore) Create(name, driverName string, opts, labels map[string]st
 	return s.CreateWithRef(name, driverName, "", opts, labels)
 	return s.CreateWithRef(name, driverName, "", opts, labels)
 }
 }
 
 
+// checkConflict checks the local cache for name collisions with the passed in name,
+// for existing volumes with the same name but in a different driver.
+// This is used by `Create` as a best effort to prevent name collisions for volumes.
+// If a matching volume is found that is not a conflict that is returned so the caller
+// does not need to perform an additional lookup.
+// When no matching volume is found, both returns will be nil
+//
+// Note: This does not probe all the drivers for name collisions because v1 plugins
+// are very slow, particularly if the plugin is down, and cause other issues,
+// particularly around locking the store.
+// TODO(cpuguy83): With v2 plugins this shouldn't be a problem. Could also potentially
+// use a connect timeout for this kind of check to ensure we aren't blocking for a
+// long time.
+func (s *VolumeStore) checkConflict(name, driverName string) (volume.Volume, error) {
+	// check the local cache
+	v, _ := s.getNamed(name)
+	if v != nil {
+		vDriverName := v.DriverName()
+		if driverName != "" && vDriverName != driverName {
+			// we have what looks like a conflict
+			// let's see if there are existing refs to this volume, if so we don't need
+			// to go any further since we can assume the volume is legit.
+			if len(s.getRefs(name)) > 0 {
+				return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name)
+			}
+
+			// looks like there is a conflict, but nothing is referencing it...
+			// let's check if the found volume ref
+			// is stale by checking with the driver if it still exists
+			vd, err := volumedrivers.GetDriver(vDriverName)
+			if err != nil {
+				// play it safe and return the error
+				// TODO(cpuguy83): maybe when when v2 plugins are ubiquitous, we should
+				// just purge this from the cache
+				return nil, errors.Wrapf(errNameConflict, "found reference to volume '%s' in driver '%s', but got an error while checking the driver: %v", name, vDriverName, err)
+			}
+
+			// now check if it still exists in the driver
+			v2, err := vd.Get(name)
+			err = errors.Cause(err)
+			if err != nil {
+				if _, ok := err.(net.Error); ok {
+					// got some error related to the driver connectivity
+					// play it safe and return the error
+					// TODO(cpuguy83): When when v2 plugins are ubiquitous, maybe we should
+					// just purge this from the cache
+					return nil, errors.Wrapf(errNameConflict, "found reference to volume '%s' in driver '%s', but got an error while checking the driver: %v", name, vDriverName, err)
+				}
+
+				// a driver can return whatever it wants, so let's make sure this is nil
+				if v2 == nil {
+					// purge this reference from the cache
+					s.Purge(name)
+					return nil, nil
+				}
+			}
+			if v2 != nil {
+				return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name)
+			}
+		}
+		return v, nil
+	}
+
+	return nil, nil
+}
+
 // create asks the given driver to create a volume with the name/opts.
 // create asks the given driver to create a volume with the name/opts.
 // If a volume with the name is already known, it will ask the stored driver for the volume.
 // If a volume with the name is already known, it will ask the stored driver for the volume.
-// If the passed in driver name does not match the driver name which is stored for the given volume name, an error is returned.
+// If the passed in driver name does not match the driver name which is stored
+//  for the given volume name, an error is returned after checking if the reference is stale.
+// If the reference is stale, it will be purged and this create can continue.
 // It is expected that callers of this function hold any necessary locks.
 // It is expected that callers of this function hold any necessary locks.
 func (s *VolumeStore) create(name, driverName string, opts, labels map[string]string) (volume.Volume, error) {
 func (s *VolumeStore) create(name, driverName string, opts, labels map[string]string) (volume.Volume, error) {
 	// Validate the name in a platform-specific manner
 	// Validate the name in a platform-specific manner
@@ -265,14 +343,12 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st
 		return nil, &OpErr{Err: errInvalidName, Name: name, Op: "create"}
 		return nil, &OpErr{Err: errInvalidName, Name: name, Op: "create"}
 	}
 	}
 
 
-	if v, exists := s.getNamed(name); exists {
-		if v.DriverName() != driverName && driverName != "" && driverName != volume.DefaultDriverName {
-			return nil, errNameConflict
-		}
-		// check exist in driver
-		if driverName == "" || driverName == volume.DefaultDriverName {
-			return v, nil
-		}
+	v, err := s.checkConflict(name, driverName)
+	if err != nil {
+		return nil, err
+	}
+	if v != nil {
+		return v, nil
 	}
 	}
 
 
 	// Since there isn't a specified driver name, let's see if any of the existing drivers have this volume name
 	// Since there isn't a specified driver name, let's see if any of the existing drivers have this volume name
@@ -294,7 +370,7 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st
 	if v, _ := vd.Get(name); v != nil {
 	if v, _ := vd.Get(name); v != nil {
 		return v, nil
 		return v, nil
 	}
 	}
-	v, err := vd.Create(name, opts)
+	v, err = vd.Create(name, opts)
 	if err != nil {
 	if err != nil {
 		return nil, err
 		return nil, err
 	}
 	}
@@ -435,7 +511,8 @@ func (s *VolumeStore) Remove(v volume.Volume) error {
 	s.locks.Lock(name)
 	s.locks.Lock(name)
 	defer s.locks.Unlock(name)
 	defer s.locks.Unlock(name)
 
 
-	if refs, exists := s.refs[name]; exists && len(refs) > 0 {
+	refs := s.getRefs(name)
+	if len(refs) > 0 {
 		return &OpErr{Err: errVolumeInUse, Name: v.Name(), Op: "remove", Refs: refs}
 		return &OpErr{Err: errVolumeInUse, Name: v.Name(), Op: "remove", Refs: refs}
 	}
 	}
 
 
@@ -476,13 +553,7 @@ func (s *VolumeStore) Refs(v volume.Volume) []string {
 	s.locks.Lock(v.Name())
 	s.locks.Lock(v.Name())
 	defer s.locks.Unlock(v.Name())
 	defer s.locks.Unlock(v.Name())
 
 
-	s.globalLock.Lock()
-	defer s.globalLock.Unlock()
-	refs, exists := s.refs[v.Name()]
-	if !exists {
-		return nil
-	}
-
+	refs := s.getRefs(v.Name())
 	refsOut := make([]string, len(refs))
 	refsOut := make([]string, len(refs))
 	copy(refsOut, refs)
 	copy(refsOut, refs)
 	return refsOut
 	return refsOut
@@ -514,7 +585,7 @@ func (s *VolumeStore) FilterByDriver(name string) ([]volume.Volume, error) {
 func (s *VolumeStore) FilterByUsed(vols []volume.Volume, used bool) []volume.Volume {
 func (s *VolumeStore) FilterByUsed(vols []volume.Volume, used bool) []volume.Volume {
 	return s.filter(vols, func(v volume.Volume) bool {
 	return s.filter(vols, func(v volume.Volume) bool {
 		s.locks.Lock(v.Name())
 		s.locks.Lock(v.Name())
-		l := len(s.refs[v.Name()])
+		l := len(s.getRefs(v.Name()))
 		s.locks.Unlock(v.Name())
 		s.locks.Unlock(v.Name())
 		if (used && l > 0) || (!used && l == 0) {
 		if (used && l > 0) || (!used && l == 0) {
 			return true
 			return true

+ 0 - 1
volume/volume.go

@@ -270,7 +270,6 @@ func ParseMountSpec(cfg mounttypes.Mount, options ...func(*validateOpts)) (*Moun
 		}
 		}
 		mp.CopyData = DefaultCopyMode
 		mp.CopyData = DefaultCopyMode
 
 
-		mp.Driver = DefaultDriverName
 		if cfg.VolumeOptions != nil {
 		if cfg.VolumeOptions != nil {
 			if cfg.VolumeOptions.DriverConfig != nil {
 			if cfg.VolumeOptions.DriverConfig != nil {
 				mp.Driver = cfg.VolumeOptions.DriverConfig.Name
 				mp.Driver = cfg.VolumeOptions.DriverConfig.Name

+ 7 - 7
volume/volume_test.go

@@ -163,8 +163,8 @@ func TestParseMountRawSplit(t *testing.T) {
 			{`name:d::rw`, "local", `d:`, ``, `name`, "local", true, false},
 			{`name:d::rw`, "local", `d:`, ``, `name`, "local", true, false},
 			{`name:d:`, "local", `d:`, ``, `name`, "local", true, false},
 			{`name:d:`, "local", `d:`, ``, `name`, "local", true, false},
 			// TODO Windows post TP5 - Add readonly support {`name:d::ro`, "local", `d:`, ``, `name`, "local", false, false},
 			// TODO Windows post TP5 - Add readonly support {`name:d::ro`, "local", `d:`, ``, `name`, "local", false, false},
-			{`name:c:`, "", ``, ``, ``, DefaultDriverName, true, true},
-			{`driver/name:c:`, "", ``, ``, ``, DefaultDriverName, true, true},
+			{`name:c:`, "", ``, ``, ``, "", true, true},
+			{`driver/name:c:`, "", ``, ``, ``, "", true, true},
 		}
 		}
 	} else {
 	} else {
 		cases = []testParseMountRaw{
 		cases = []testParseMountRaw{
@@ -172,10 +172,10 @@ func TestParseMountRawSplit(t *testing.T) {
 			{"/tmp:/tmp2:ro", "", "/tmp2", "/tmp", "", "", false, false},
 			{"/tmp:/tmp2:ro", "", "/tmp2", "/tmp", "", "", false, false},
 			{"/tmp:/tmp3:rw", "", "/tmp3", "/tmp", "", "", true, false},
 			{"/tmp:/tmp3:rw", "", "/tmp3", "/tmp", "", "", true, false},
 			{"/tmp:/tmp4:foo", "", "", "", "", "", false, true},
 			{"/tmp:/tmp4:foo", "", "", "", "", "", false, true},
-			{"name:/named1", "", "/named1", "", "name", DefaultDriverName, true, false},
+			{"name:/named1", "", "/named1", "", "name", "", true, false},
 			{"name:/named2", "external", "/named2", "", "name", "external", true, false},
 			{"name:/named2", "external", "/named2", "", "name", "external", true, false},
 			{"name:/named3:ro", "local", "/named3", "", "name", "local", false, false},
 			{"name:/named3:ro", "local", "/named3", "", "name", "local", false, false},
-			{"local/name:/tmp:rw", "", "/tmp", "", "local/name", DefaultDriverName, true, false},
+			{"local/name:/tmp:rw", "", "/tmp", "", "local/name", "", true, false},
 			{"/tmp:tmp", "", "", "", "", "", true, true},
 			{"/tmp:tmp", "", "", "", "", "", true, true},
 		}
 		}
 	}
 	}
@@ -207,7 +207,7 @@ func TestParseMountRawSplit(t *testing.T) {
 			t.Fatalf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind)
 			t.Fatalf("Expected name '%s', was '%s' for spec '%s'", c.expName, m.Name, c.bind)
 		}
 		}
 
 
-		if (m.Driver != c.expDriver) || (m.Driver == DefaultDriverName && c.expDriver == "") {
+		if m.Driver != c.expDriver {
 			t.Fatalf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind)
 			t.Fatalf("Expected driver '%s', was '%s', for spec '%s'", c.expDriver, m.Driver, c.bind)
 		}
 		}
 
 
@@ -233,8 +233,8 @@ func TestParseMountSpec(t *testing.T) {
 		{mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, RW: true}},
 		{mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath, RW: true}},
 		{mount.Mount{Type: mount.TypeBind, Source: testDir + string(os.PathSeparator), Target: testDestinationPath, ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath}},
 		{mount.Mount{Type: mount.TypeBind, Source: testDir + string(os.PathSeparator), Target: testDestinationPath, ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath}},
 		{mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath + string(os.PathSeparator), ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath}},
 		{mount.Mount{Type: mount.TypeBind, Source: testDir, Target: testDestinationPath + string(os.PathSeparator), ReadOnly: true}, MountPoint{Type: mount.TypeBind, Source: testDir, Destination: testDestinationPath}},
-		{mount.Mount{Type: mount.TypeVolume, Target: testDestinationPath}, MountPoint{Type: mount.TypeVolume, Destination: testDestinationPath, RW: true, Driver: DefaultDriverName, CopyData: DefaultCopyMode}},
-		{mount.Mount{Type: mount.TypeVolume, Target: testDestinationPath + string(os.PathSeparator)}, MountPoint{Type: mount.TypeVolume, Destination: testDestinationPath, RW: true, Driver: DefaultDriverName, CopyData: DefaultCopyMode}},
+		{mount.Mount{Type: mount.TypeVolume, Target: testDestinationPath}, MountPoint{Type: mount.TypeVolume, Destination: testDestinationPath, RW: true, CopyData: DefaultCopyMode}},
+		{mount.Mount{Type: mount.TypeVolume, Target: testDestinationPath + string(os.PathSeparator)}, MountPoint{Type: mount.TypeVolume, Destination: testDestinationPath, RW: true, CopyData: DefaultCopyMode}},
 	}
 	}
 
 
 	for i, c := range cases {
 	for i, c := range cases {