Skip to content
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

Fix removing of old docker container #12597

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kv-y
Copy link
Contributor

@kv-y kv-y commented Nov 3, 2022

Why I did it

scripts/collect_docker_version_files.sh doesn't remove existing docker container.
It's possible to get "Error response from daemon: Conflict. The container name ... is already in use by container ...".

How I did it

Use $DOCKER_CONTAINER instead of $DOCKER_IMAGE to remove existing docker container.

How to verify it

Manually create docker container and run scripts/collect_docker_version_files.sh
Existing container will be successfully removed in this case.

Signed-off-by: Konstantin Vasin k.vasin@yadro.com

Signed-off-by: Konstantin Vasin <k.vasin@yadro.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kv-y / name: Konstantin Vasin (1bef8f1)

@kv-y
Copy link
Contributor Author

kv-y commented Nov 8, 2022

@xumia
Could you please review this PR?

You added scripts/collect_docker_version_files.sh for Repr build in 2020 and this bug exists since then.
So if we really need to remove existing container before creating new one then this PR fixes this bug.
If we want to get an error message when container already exists then we can remove this code.

But now in this code we try to inspect and remove docker image with docker container command like this;
docker container inspect sonic-slave-bullseye-user:20d23eb25b2 > /dev/null 2>&1
It's not correct anyway. This error was not detected cause you redirect all stderr/stdout to /dev/null.

@kv-y
Copy link
Contributor Author

kv-y commented Dec 13, 2022

@xumia or @lguohan

Could you please review this PR?
There were many fixes in scripts/collect_docker_version_files.sh last week for vcache, but this incorrect code for removing old docker container is still here.

lguohan
lguohan previously approved these changes Dec 14, 2022
@lguohan lguohan enabled auto-merge (squash) December 14, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants