docker rename fix to address the issue of renaming with the same name#23360
docker rename fix to address the issue of renaming with the same name#23360vdemeester merged 1 commit intomoby:masterfrom
Conversation
There was a problem hiding this comment.
We should probably error out instead of returning nil.
|
Thanks for working on this! |
sainath14
force-pushed
the
rename_error
branch
2 times, most recently
from
1ac36c9 to
fefced8
Compare
|
@cpuguy83 regarding returning nil, it is safe to silently ignore renaming with same name. Silently ignoring would become a problem in case a user tries using old name and new name that are very close and makes a typo to make them same. He will not realize that he made a typo :) if we decide on how to handle this, i would proceed writing a test case accordingly |
|
No we should return an error. |
|
Agree with @cpuguy83, we should return an error. Client is then free to handle or ignore it 😇 |
|
hm, still slightly in favor of ignoring (there is no error, because the operation basically succeeded), but looks like I'm the minority 😅 |
|
What is the justification for returning an error? I really think it should return success. |
|
Because it's odd to rename something to the same name and is most likely a mistake. |
|
I don't think this PR is actually addressing the underlying issue, as the rename is currently also failing if I use the container id not the name: |
|
This is because the name registrar doesn't return an error if the name is already reserved to the passed in reference. Could update it to go ahead and return an error there or just explicitly check if the new name matches the container's ID/Name... |
|
I agree @cpuguy83. Only other places where they call Reserve API from registrar package are in the new container creation flow. So it might not have made sense to check the name against the passed in id. |
sainath14
force-pushed
the
rename_error
branch
2 times, most recently
from
6a246ca to
3e8c16e
Compare
… issue moby#23319 Signed-off-by: Sainath Grandhi <[email protected]>
|
LGTM |
|
LGTM 🐯 |
|
@sainath14 could you update the "changelog" part of the PR description so that it matches the current behaviour ? 👼 |
|
@vdemeester change log updated. |
| container.Lock() | ||
| defer container.Unlock() | ||
|
|
||
| if oldName == newName { |
There was a problem hiding this comment.
Should we move this before the container.Lock()?
There was a problem hiding this comment.
Just discussed this with @vdemeester and we saw some other things that may need to be improved (not related to this PR), so he's going to do a follow up
During the renaming of a container, no need to call `container.Lock()` if `oldName == newName`. This is a follow-up from moby#23360 (commit 88d1ee6) Signed-off-by: Vincent Demeester <[email protected]>
During the renaming of a container, no need to call `container.Lock()` if `oldName == newName`. This is a follow-up from moby#23360 (commit 88d1ee6) Signed-off-by: Vincent Demeester <[email protected]> (cherry picked from commit 7e1ec8d)
… issue moby#23319 ---- Fix DTS2017050408229 Cherry-pick from moby#23360 Issue: moby#23319 Signed-off-by: Sainath Grandhi <[email protected]> Signed-off-by: majiuyue 00385406 <[email protected]>
8 participants
Fixes #23319
- What I did
docker rename cont_name cont_name , using same name to rename creates an inconsistent nameIndex for daemon. Added a check in daemon rename code to see if oldname is same as newname. If so return an error
- How to verify it
Before the fix
docker run -itd --name=old busybox
docker rename old old - successful without any error
docker rename old old - should complain there is no container with name old
After the fix
docker run -itd --name=old busybox
docker rename old old - error "Renaming a container with the same name as its current name"
docker rename old old - error "Renaming a container with the same name as its current name"
- Description for the changelog
A check in the docker daemon rename function to see if the current name of the container is same as the passed in new name. If the names match, daemon returns an error saying "names matched"
Signed-off-by: Sainath Grandhi [email protected]