-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes Incorrect documentation docker volume create - Issue #47606 #47798
base: master
Are you sure you want to change the base?
Conversation
…: Anish Mankal <anish.mankal@gmail.com>
…: Anish Mankal <anish.mankal@gmail.com>
Hm.. I don't think this change is correct, but the documentation may need a more complete example; https://docs.docker.com/reference/cli/docker/volume/create/ The important part here is that the name must be unique across drivers, so no 2 volumes should be allowed with the same name across multiple drivers, but for the same driver, it's treated as an idempotent action;
I once opened a ticket to suggest it should always be an error, but that ship likely sailed, and may be too late now to change that behavior; |
…ish Mankal <anish.mankal@gmail.com>
I see, if its decided upon not to always raise an error, I changed it to only raise the error if it's not the same driver name; if it is the same driver, let the current behavior continue. Is this an acceptable implementation? |
v, err := a.proxy.Get(name) | ||
if err == nil && v.DriverName() != a.Name() { | ||
return nil, errors.New("A volume named " + name + " already exists with the " + v.DriverName() + " driver. Choose a different volume name.") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is already handled; looking at
Lines 511 to 524 in 8e14f27
// 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(ctx context.Context, name, driverName string) (volume.Volume, error) { |
And, more specifically, these lines
Lines 553 to 558 in 8e14f27
if exists { | |
if conflict { | |
return nil, errors.Wrapf(errNameConflict, "driver '%s' already has volume '%s'", vDriverName, name) | |
} | |
return v, nil | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I doing this right 👍
fixes #47606
- What I did
I changed the adapter.go file to throw an error if a container with the name already exists
- How I did it
I get the volume based on the name, and if the volume is returned (i.e. err is null) it means that a volume of the given name exists. Since err is null, this means such a volume exists, which means an error and corresponding error message according to the docker documentation outlined in the issue is thrown.
- How to verify it
Run the docker volume create [NAME] and use the name of an already existing volume.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
Signed-off-by: Anish Mankal anish.mankal@gmail.com