volume create error on conflict option #16009

azurezk · 2015-09-02T06:46:27Z

fixes #15998
Signed-off-by: Kun Zhang zkazure@gmail.com

runcom · 2015-09-02T06:51:34Z

Can you add a test case for this change just to make sure the command errors out correctly?

Btw the linked issue seems to have a broader scope in making sure conflicting options errors out, does that only apply to driver names?

azurezk · 2015-09-02T08:37:12Z

added test. :)
I m not sure about other conflict cases. @runcom @thaJeztah @cpuguy83

runcom · 2015-09-02T08:49:56Z

integration-cli/docker_cli_volume_test.go

@@ -18,6 +18,18 @@ func (s *DockerSuite) TestVolumeCliCreate(c *check.C) {
 	c.Assert(name, check.Equals, "test")
 }

+func (s *DockerSuite) TestVolumeCliCreateOptionConflict(c *check.C) {
+	dockerCmd(c, "volume", "create")


is this really needed?

runcom · 2015-09-02T08:52:15Z

just two small comments, ping @cpuguy83 @thaJeztah

cpuguy83 · 2015-09-02T13:31:46Z

daemon/volumes.go

@@ -136,6 +136,9 @@ func (s *volumeStore) Create(name, driverName string, opts map[string]string) (v
 	if vc, exists := s.vols[name]; exists {
 		v := vc.Volume
 		s.mu.Unlock()
+		if driverName != v.DriverName() {


This should go in daemon.VolumeCreate (in daemon/create.go) instead of here.
In cases where you have docker run -v some_name:/foo --volume-driver other_driver and some_name is an existing volume but on a different driver, this should work.

thaJeztah · 2015-09-02T15:53:04Z

Btw the linked issue seems to have a broader scope in making sure conflicting options errors out, does that only apply to driver names?

At this moment, yes, only the driver: Docker doesn't keep track of other options (e.g. driver-options used to create the volume), so it's not possible to check if a user is trying to create a volume with the same name, but different options.

runcom · 2015-09-02T15:57:52Z

@thaJeztah gotcha thanks

azurezk · 2015-09-06T05:40:31Z

change to daemon/create.go :) @cpuguy83

cpuguy83 · 2015-09-07T11:21:14Z

daemon/create.go

@@ -139,6 +139,15 @@ func (daemon *Daemon) VolumeCreate(name, driverName string, opts map[string]stri
 		name = stringid.GenerateNonCryptoID()
 	}

+	daemon.volumes.mu.Lock()


Sorry, this is not quite correct.
We can remove all this low-level check here and just check if the volume returned by daemon.volumes.Create has the same driver as driverName.

@cpuguy83 I guess the problem with that is that you'd still end up creating the volume, and we want to avoid it.
I agree though that this might not be the right place to put this code. What do you think about adding this inside volume.Create and returning an "already created" error?

Well we do want it to be created if it doesn't exist, which would be the only case it would actually be created.

The thing with having it return an error when it already exists is it would break named volumes on run.

azurezk · 2015-09-08T07:33:55Z

I misunderstood your meaning. Updated! @cpuguy83

runcom · 2015-09-08T12:35:58Z

daemon/create.go

@@ -144,5 +144,9 @@ func (daemon *Daemon) VolumeCreate(name, driverName string, opts map[string]stri
 	if err != nil {
 		return nil, err
 	}
+
+	if driverName != "" && v.DriverName() != driverName {


is the emtpy string check needed here?

@runcom , yes , because if user don' t give a driver name when create a volume the driverName will be "" and the volume will be given a default driver name "local", no matter the volume is exist or not, it' s not a conflict case

Yep, definitely needed but we need to get the default volume driver if it is "" instead of ignoring.

azurezk · 2015-09-09T07:11:37Z

have updated! @cpuguy83

calavera · 2015-09-09T15:23:57Z

Change LGTM, but it needs be be squashed into a single commit.

azurezk · 2015-09-10T02:10:25Z

Forgot to squash , updated!

cpuguy83 · 2015-09-10T15:24:43Z

Cool, LGTM, moving to docs review.

cpuguy83 · 2015-09-10T15:25:39Z

@azurezk Actually if you can update the commit message to just a single message and a single sign-off that would be amazing. Thanks!

azurezk · 2015-09-25T09:34:31Z

@calavera have updated

calavera · 2015-09-25T18:14:32Z

integration-cli/docker_cli_volume_test.go

+func (s *DockerSuite) TestVolumeCliCreateOptionConflict(c *check.C) {
+	dockerCmd(c, "volume", "create", "--name=test")
+	_, _, err := dockerCmdWithError("volume", "create", "--name", "test", "--driver", "nosuchdriver")
+	c.Assert(err, check.NotNil, check.Commentf("volume create excepted to be failed on creating an existed volume with a different driver name"))


can we test that the error we get is the right error we expect? I'm not completely sure this command fails where we expect it to fail.

jessfraz · 2015-10-02T22:42:32Z

needs rebase

azurezk · 2015-10-05T11:17:38Z

rebased

jessfraz · 2015-10-08T21:40:23Z

ping @cpuguy83

icecrime · 2015-10-10T01:00:44Z

Moving to docs review, thanks!

thaJeztah · 2015-10-10T10:46:14Z

docs/reference/commandline/volume_create.md

@@ -29,6 +29,8 @@ The mount is created inside the container's `/src` directory. Docker does not su

 Multiple containers can use the same volume in the same time period. This is useful if two containers need access to shared data. For example, if one container writes and the other reads the data.

+You cannot create a volume if there is a volume already registered with the same name in other driver. Docker returns the existent volume ID if the volume name conflicts and you don't specify the driver.


Slightly confused by the description here; if I understand correctly, the new behavior is;

if I try to create a volume with the same name but a different driver, I receive an error

if I try to create a volume with the same name, but don't specify a driver, it is assumed I want to re-use the existing volume, so I don't get an error

Correct?

Thanks! I'll see if I can come with a better description, unless @moxiegirl beats me to it :-)

@azurezk Thank you for the contribution. Couple of changes to text and error message.

---> https://gist.github.com/moxiegirl/77c1f117d3080d1c95c6

Volume names must be unique among drivers. This means you cannot use the same volume name with two different drivers. If you attempt this docker returns an error:

A volume named %s already exists with the %s driver. Choose a different volume name.

If you specify a volume name already in use on the current driver, Docker assumes you want to re-use the existing volume and does not return an error.

Signed-off-by: Kun Zhang <zkazure@gmail.com>

azurezk · 2015-10-12T07:51:34Z

updated @moxiegirl

thaJeztah · 2015-10-12T17:33:15Z

LGTM, thanks @azurezk !

icecrime · 2015-10-12T20:13:26Z

Ping @moxiegirl: all good? ;-)

moxiegirl · 2015-10-12T22:27:10Z

LGTM @icecrime I'll let you coders merge the GO

volume create error on conflict option

cpuguy83 · 2015-10-29T20:56:24Z

docs/reference/commandline/volume_create.md

+Volume names must be unique among drivers.  This means you cannot use the same volume name with two different drivers.  If you attempt this `docker` returns an error:
+
+```
+A volume named  %s  already exists with the %s driver. Choose a different volume name.


Oops, %s is not what we want here :)

Oh, I open #17515 to fix it

runcom added the status/2-code-review label Sep 2, 2015

azurezk force-pushed the vol-create-conflict-option branch 2 times, most recently from c564cd9 to 1e9a6a2 Compare September 2, 2015 08:05

runcom reviewed Sep 2, 2015
View reviewed changes

azurezk force-pushed the vol-create-conflict-option branch from 1e9a6a2 to 1a1dfbe Compare September 2, 2015 09:44

cpuguy83 reviewed Sep 2, 2015
View reviewed changes

azurezk force-pushed the vol-create-conflict-option branch from 1a1dfbe to 825ecb8 Compare September 6, 2015 04:28

cpuguy83 reviewed Sep 7, 2015
View reviewed changes

azurezk force-pushed the vol-create-conflict-option branch 2 times, most recently from 8af1a3e to 02adcb1 Compare September 8, 2015 06:59

runcom reviewed Sep 8, 2015
View reviewed changes

azurezk force-pushed the vol-create-conflict-option branch 2 times, most recently from 2dcdfdd to 48583ee Compare September 9, 2015 03:21

azurezk force-pushed the vol-create-conflict-option branch from 15efff4 to cb3ea7c Compare September 9, 2015 16:10

cpuguy83 added status/3-docs-review and removed status/2-code-review labels Sep 10, 2015

azurezk force-pushed the vol-create-conflict-option branch from 7c16419 to 1f46bdc Compare September 25, 2015 07:45

azurezk force-pushed the vol-create-conflict-option branch from 1f46bdc to db73f95 Compare September 25, 2015 10:25

calavera reviewed Sep 25, 2015
View reviewed changes

azurezk force-pushed the vol-create-conflict-option branch 2 times, most recently from bd49a4a to 1c26244 Compare September 28, 2015 14:45

azurezk force-pushed the vol-create-conflict-option branch from 1c26244 to 62762a0 Compare October 4, 2015 12:12

icecrime added status/3-docs-review and removed status/2-code-review labels Oct 10, 2015

icecrime added this to the 1.9.0 milestone Oct 10, 2015

thaJeztah reviewed Oct 10, 2015
View reviewed changes

volume create error on conflict option

0ff3123

Signed-off-by: Kun Zhang <zkazure@gmail.com>

azurezk force-pushed the vol-create-conflict-option branch from 62762a0 to 0ff3123 Compare October 12, 2015 03:18

icecrime self-assigned this Oct 12, 2015

jessfraz pushed a commit that referenced this pull request Oct 12, 2015

Merge pull request #16009 from azurezk/vol-create-conflict-option

df79536

volume create error on conflict option

jessfraz merged commit df79536 into moby:master Oct 12, 2015

icecrime added the impact/changelog label Oct 12, 2015

cpuguy83 reviewed Oct 29, 2015
View reviewed changes

		@@ -29,6 +29,8 @@ The mount is created inside the container's `/src` directory. Docker does not su

		Multiple containers can use the same volume in the same time period. This is useful if two containers need access to shared data. For example, if one container writes and the other reads the data.

		You cannot create a volume if there is a volume already registered with the same name in other driver. Docker returns the existent volume ID if the volume name conflicts and you don't specify the driver.

volume create error on conflict option #16009

volume create error on conflict option #16009

Conversation

azurezk commented Sep 2, 2015

runcom commented Sep 2, 2015

azurezk commented Sep 2, 2015

Choose a reason for hiding this comment

runcom commented Sep 2, 2015

Choose a reason for hiding this comment

thaJeztah commented Sep 2, 2015

runcom commented Sep 2, 2015

azurezk commented Sep 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azurezk commented Sep 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azurezk commented Sep 9, 2015

calavera commented Sep 9, 2015

azurezk commented Sep 10, 2015

cpuguy83 commented Sep 10, 2015

cpuguy83 commented Sep 10, 2015

azurezk commented Sep 25, 2015

Choose a reason for hiding this comment

jessfraz commented Oct 2, 2015

azurezk commented Oct 5, 2015

jessfraz commented Oct 8, 2015

icecrime commented Oct 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azurezk commented Oct 12, 2015

thaJeztah commented Oct 12, 2015

icecrime commented Oct 12, 2015

moxiegirl commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment