Change `docker volume create` to non-idempotent #27675

yongtang · 2016-10-24T04:11:48Z

- What I did

This fix tries to address the issue raised in #16068 where non-idempotency is desired for docker volume create.

- How I did it

Previously docker volume create will silently ignore in case a volume with the same name has already been created with the same driver (idempotency). This fix made the change so that an already exist error is returned together with the already created volume v in Create/CreateWithRef func.

(So a check of return error type is needed if the caller wants idempotency instead)

- How to verify it

Tests has been added to cover the chagnes.

- Description for the changelog

An error will be returned for docker volume create in case a volume with the same name has been created by the same driver.

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #16068.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

thaJeztah · 2016-10-25T00:33:12Z

Oh! I just noticed that this probably does the same as #27346 (haven't looked at the implementation yet)

Also we should take into account that we now store the options that were used when a volume was created, so perhaps volume create does not have to produce an error if all options are exactly the same

allencloud · 2016-10-25T02:57:14Z

Oh, @thaJeztah
To be honest, maybe we should be more careful on what you suggest that same name with same options.

Here is my own thought.
Since if a user executes docker volume create --name aaa twice(same name same options), probably he indeed needs two different volumes.(maybe he forgot he created before or someone else did that) If dockerd make the second action succeed, afterwards the volume may be polluted. Its damage will be exposed.

Maybe needs more discussion in this part.

cpuguy83 · 2016-10-26T22:33:51Z

volume/store/store.go

@@ -269,14 +273,16 @@ func (s *VolumeStore) create(name, driverName string, opts, labels map[string]st
 		if v.DriverName() != driverName && driverName != "" && driverName != volume.DefaultDriverName {
 			return nil, errNameConflict
 		}
-		return v, nil
+		// In case a volume has already been created, we will return an error (non-idempotency)
+		return v, errAlreadyCreated


I'm not a huge fan of this approach.

Ultimately the reason the code is as it is was to support the legacy auto-create/use for docker run -v <name>:/foo.
Meanwhile this pattern is racey:

Get(foo) || Create(foo)

Maybe we could have Get accept a callback that gets run at the end of the fn while the store is still locked.

yongtang · 2016-11-02T01:06:23Z

@cpuguy83 @allencloud @thaJeztah I made some changes to the PR. Now instead of checking the error, the CreateWithRef will carry a arg of quiet bool. If quiet == true, then the behavior will be the same as before. If quiet == false, then an error already exist will be returned. Please take a look and let me know if this will fit the requirement.

cpuguy83 · 2016-11-02T01:10:41Z

volume/store/store.go

-	return s.CreateWithRef(name, driverName, "", opts, labels)
+// In case a volume with the name has already been created, an errAlreadyCreated error will be
+// returned along with the already created volume, if `quiet` is not true
+func (s *VolumeStore) Create(name, driverName string, opts, labels map[string]string, quiet bool) (volume.Volume, error) {


I don't think this is a good solution.
Bool parameters are never a good solution.

Thanks @cpuguy83. No sure about the best way to handle the case. Any suggestions?

@yongtang I think it would be better to have a CreateIdempotent than a bool arg... or a set of functional arguments (http://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)

yongtang · 2016-11-07T15:39:05Z

Thanks @cpuguy83. The PR has been updated with functional options. Please take a look.

cpuguy83

Thanks, this is definitely much cleaner.

Right now this is implemented more like a callback.
I wonder if it would be better to do more like a config struct:

ie, the functional argument signature would look like func(*CreateConfig) where in the argument you set options (like "Ref" and idempotency handling).

Then we always call for _, arg := range args { f(cfg) } at the beginning of Create and check the options struct in the function body as needed.

yongtang · 2016-11-14T19:38:42Z

@cpuguy83 One issue I am encountering is that, idempotency check is done at the end of Get while setting Refs is done at the end of Create.

We may need to pass a flag to indicate if the volume is obtained from the existing one or created new, before invoking an array of option functionals.

I will spend more time thinking about if there is a better way to handle the scenario.

cpuguy83 · 2016-11-30T18:59:36Z

@yongtang Maybe we can just have an CreateIdempotent for docker run -v and a normal Create for docker volume create?

This will definitely need to be versioned in the API, though... and probably go through the deprecation policy.

yongtang · 2016-12-01T19:13:43Z

Thanks @cpuguy83 for the suggestion. Let me take a look and update the PR accordingly.

yongtang · 2016-12-05T03:06:20Z

Thanks @cpuguy83. The PR has been updated to take API version into consideration. Also added the CreateIdempotent function. Please take a look.

cpuguy83 · 2016-12-05T20:50:49Z

@yongtang Thanks! Seems like we're getting closer.
Now what if volumes.Create returned something like an ErrAlreadyExists when name and driver are the same. Then at the API we can determine how to respond to the client.

Namely I think in older API's we keep sending the same response (created), in newer API's we follow deprecation policy and send a 304 (not modified) for now, which the client can pick up and use to display a warning but otherwise not error out.
Then after the deprecation period is up we can return the error.

cpuguy83 · 2016-12-05T20:51:29Z

Also, thanks for sticking with this!

yongtang · 2016-12-07T01:13:31Z

Thanks @cpuguy83. The PR has been updated with 302 response. Please take a look.

cpuguy83 · 2016-12-07T13:42:05Z

volume/store/store.go

-func (s *VolumeStore) CreateWithRef(name, driverName, ref string, opts, labels map[string]string) (volume.Volume, error) {
+// In case a volume has already been created with the same name and driver, the already
+// created volumes will be returned
+func (s *VolumeStore) CreateIdempotentWithRef(name, driverName, ref string, opts, labels map[string]string) (volume.Volume, error) {


Just a note, we don't need this Idempotent change anymore

Thanks. idempotent has been removed.

yongtang · 2017-04-07T18:00:18Z

Thanks @thaJeztah. I updated the PR. Now StatusNotModified will only write the header without the content:

                       w.WriteHeader(http.StatusNotModified)
                       return nil

The error message should be addressed. Please take a look.

cpuguy83 · 2017-05-22T14:42:02Z

volume/store/store.go

 func (s *VolumeStore) Create(name, driverName string, opts, labels map[string]string) (volume.Volume, error) {
-	return s.CreateWithRef(name, driverName, "", opts, labels)
+	return s.CreateWithRef(name, driverName, "", opts, labels, func(vol volume.Volume) error {


This is the only place we're using this "found" and it seems legit to put this in the volume conflict lookup check.

allencloud · 2017-06-20T01:10:45Z

Conflict happened, perhaps a rebase is needed. @yongtang

yongtang · 2017-06-25T22:32:28Z

The Jenkins failure are caused by recent changes in other places in Moby. Still looking into the issue.

This fix tries to address the issue raised in 16068 where non-idempotency is desired for `docker volume create`. Previously `docker volume create` will silently ignore in case a volume with the same name has already been created with the same driver (idempotency). This fix made the change so that an `already exist` error is returned together with the already created volume `v` in `Create/CreateWithRef` func. The change is to add functional options to `Create` so that caller has the option to return error if a volume has been found. Tests has been added to cover the chagnes. This fix fixes 16068. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

This commit is an update of the last commit such that an error is only returned if options are different for `docker volume create`. The change is based on the feedback: moby#27675 (comment) Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2017-06-28T20:11:44Z

@cpuguy83 The PR has been updated with "found" into volume conflict lookup check. Now all tests finally passes. (powerpc test is probably related to Jenkins server. will give it a try again later)

Please take a look and let me know if there are any issues.

vieux · 2017-07-04T13:52:33Z

ping @cpuguy83

allencloud · 2017-07-12T06:27:46Z

conflict happens, and I am afraid @yongtang needs a bottle of beer and a rebase. 🍺

cpuguy83 · 2017-07-13T16:52:36Z

volume/store/store.go

 func (s *VolumeStore) Create(name, driverName string, opts, labels map[string]string) (volume.Volume, error) {
-	return s.CreateWithRef(name, driverName, "", opts, labels)
+	return s.CreateWithRef(name, driverName, "", opts, labels, func(vol volume.Volume) error {
+		var options map[string]string


Why are we keeping this logic for CreateWithRef only?

ping @yongtang ^^

vdemeester · 2018-02-16T10:16:18Z

ping @thaJeztah @yongtang what is the status here ?

AkihiroSuda · 2018-03-22T06:06:24Z

ping @yongtang ^^

cpuguy83 · 2018-05-25T18:18:54Z

Closing as this has a number of conflicts at this point and hasn't been updated in a long time.
Also see #36688 which re-writes much of the volume handling code.

GordonTheTurtle added the status/0-triage label Oct 24, 2016

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch 5 times, most recently from acfb44c to 2d2298b Compare October 24, 2016 22:06

thaJeztah added status/1-design-review and removed status/0-triage labels Oct 24, 2016

cpuguy83 reviewed Oct 26, 2016

View reviewed changes

This was referenced Oct 26, 2016

return error when create volume with existing name #27346

Closed

Volume name conflict error message to include the driver name #27437

Closed

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from 2d2298b to 616b923 Compare November 1, 2016 23:41

cpuguy83 requested changes Nov 2, 2016

View reviewed changes

yongtang mentioned this pull request Nov 4, 2016

Windows: Error response from daemon: missing device in volume options #27792

Closed

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from 616b923 to e409074 Compare November 6, 2016 01:56

cpuguy83 reviewed Nov 7, 2016

View reviewed changes

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from e409074 to c9f2680 Compare December 5, 2016 00:07

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from c9f2680 to 86ebd69 Compare December 6, 2016 23:05

cpuguy83 reviewed Dec 7, 2016

View reviewed changes

cpuguy83 reviewed May 22, 2017

View reviewed changes

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from efb5648 to c765c2e Compare June 20, 2017 02:18

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from c765c2e to 0607ccf Compare June 22, 2017 23:42

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from 0607ccf to 097e087 Compare June 23, 2017 02:55

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from 0bd69af to 3a8c224 Compare June 24, 2017 18:39

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from 3a8c224 to 598adbd Compare June 28, 2017 01:07

yongtang added 3 commits June 28, 2017 03:24

Move found to checkConflict

8c668b2

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 16068-docker-volume-create-non-idempotency branch from 598adbd to 8c668b2 Compare June 28, 2017 03:25

yongtang added the rebuild/powerpc label Jun 28, 2017

GordonTheTurtle removed the rebuild/powerpc label Jun 28, 2017

yongtang added the rebuild/powerpc label Jun 28, 2017

GordonTheTurtle removed the rebuild/powerpc label Jun 28, 2017

cpuguy83 reviewed Jul 13, 2017

View reviewed changes

cpuguy83 closed this May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `docker volume create` to non-idempotent #27675

Change `docker volume create` to non-idempotent #27675

yongtang commented Oct 24, 2016

thaJeztah commented Oct 25, 2016

allencloud commented Oct 25, 2016

cpuguy83 Oct 26, 2016

yongtang commented Nov 2, 2016

cpuguy83 Nov 2, 2016

yongtang Nov 2, 2016

cpuguy83 Nov 4, 2016

yongtang commented Nov 7, 2016

cpuguy83 left a comment

yongtang commented Nov 14, 2016

cpuguy83 commented Nov 30, 2016

yongtang commented Dec 1, 2016

yongtang commented Dec 5, 2016

cpuguy83 commented Dec 5, 2016 •

edited

cpuguy83 commented Dec 5, 2016

yongtang commented Dec 7, 2016

cpuguy83 Dec 7, 2016

yongtang Dec 7, 2016

yongtang commented Apr 7, 2017

cpuguy83 May 22, 2017

allencloud commented Jun 20, 2017

yongtang commented Jun 25, 2017

yongtang commented Jun 28, 2017

vieux commented Jul 4, 2017

allencloud commented Jul 12, 2017

cpuguy83 Jul 13, 2017

thaJeztah Jul 20, 2017

vdemeester commented Feb 16, 2018

AkihiroSuda commented Mar 22, 2018

cpuguy83 commented May 25, 2018

Change docker volume create to non-idempotent #27675

Change docker volume create to non-idempotent #27675

Conversation

yongtang commented Oct 24, 2016

thaJeztah commented Oct 25, 2016

allencloud commented Oct 25, 2016

Choose a reason for hiding this comment

yongtang commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Nov 7, 2016

cpuguy83 left a comment

Choose a reason for hiding this comment

yongtang commented Nov 14, 2016

cpuguy83 commented Nov 30, 2016

yongtang commented Dec 1, 2016

yongtang commented Dec 5, 2016

cpuguy83 commented Dec 5, 2016 • edited

cpuguy83 commented Dec 5, 2016

yongtang commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Apr 7, 2017

Choose a reason for hiding this comment

allencloud commented Jun 20, 2017

yongtang commented Jun 25, 2017

yongtang commented Jun 28, 2017

vieux commented Jul 4, 2017

allencloud commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Feb 16, 2018

AkihiroSuda commented Mar 22, 2018

cpuguy83 commented May 25, 2018

Change `docker volume create` to non-idempotent #27675

Change `docker volume create` to non-idempotent #27675

cpuguy83 commented Dec 5, 2016 •

edited