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

Multiple fixes to container name detection #206

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jperville
Contributor

jperville commented Aug 29, 2014

Summary

This PR fixes multiple issues with container names in the container provider; each of these issue has been encountered when playing with linked containers in #204. I have split each fix in a different changeset, feel free to cherry-pick (or merge everything if you like the PR).

Handle case when container has multiple names

Containers may have multiple names assigned to them; even if the user only assigns one name to the container, the use of features such as 'links' may append technical names to the container. The different names are displayed in the docker ps output concatenated with a comma.

Example:

vagrant@docker-ubuntu-1404:~$ docker ps | grep backend
d605ff9bfbed        docker.local/sample-docker-app:latest   "/sbin/my_init --ena   45 minutes ago      Up 45 minutes       0.0.0.0:6080->80/tcp     my-backend-app,my-test-app/link   

When a container has multiple names, the new_resource.container_name may not be found in the output of docker ps because of exact string comparison with the raw value of the NAMES column.

acc8296 should allow matching the container name with any of the different names of the container.

Made container matching more strict

When loading the current_resource, the docker_container provider iterates over each container listed in docker ps output and stops when it finds a line that matches the new_resource. However, the method that does the matching (container_matches?) is too lax and will incorrectly select the wrong container with the following docker ps output:

aad30ba67b24 docker.local/sample-docker-app:latest "/sbin/my_init --ena About a minute ago Up About a minute 0.0.0.0:8080->80/tcp my-test-app
26ba3c500199 docker.local/sample-docker-app:latest "/sbin/my_init --ena 2 hours ago Up About an hour 0.0.0.0:6080->80/tcp my-backend-app,my-test-app/link

Assuming new_resource.container_name = 'my-backend-name', the current version of container_matches? will match a container that has:

  • a different id
  • a different name
  • the same image
  • the same command
    such as my-test-app in the above example just because my-test-app is before my-backend-app in the docker ps output.

As a consequence, chef tried to recreate the my-backend-app container (with docker run) while the container already existed, and failed. This bug is very subtle because the incorrect loading of the current resource only shows up when log level is set at debug.

My fix in cac59af is to stop considering that being run from the same image with the same cmdline is enough to 'match' a container. @bflad any side effects you imagine by being stricter? Matching an incorrect container is worse than matching none IMO.

Filter out technical names

When I was investigating #204, I found that the current_resource.container_name is initialized from the value of the raw NAMES colum in docker ps output.

In the case where docker run --link is used, it results in the current_resource.container_name being set to my-backend-app,my-test-app/link instead of my-backend-app which was the container_name that I have set in my recipe.

In 50b73cc I have decided to work around the issue by hiding the technical container names (such as my-test-app/link). This is not a full fix though, because if a container has multiple non-technical names the issue will surface again.

@jperville

This comment has been minimized.

Show comment
Hide comment
@jperville

jperville Aug 29, 2014

Contributor

@bflad be careful when resolving the merge conflict, 1aac671 probably introduced a bug in container_name_matches_if_exists? (compare with the implementation of container_name_matches? which was altered in the same changeset).

Contributor

jperville commented Aug 29, 2014

@bflad be careful when resolving the merge conflict, 1aac671 probably introduced a bug in container_name_matches_if_exists? (compare with the implementation of container_name_matches? which was altered in the same changeset).

@djdefi

This comment has been minimized.

Show comment
Hide comment
@djdefi

djdefi commented Sep 10, 2014

+1

1 similar comment
@alexdeleon

This comment has been minimized.

Show comment
Hide comment
@alexdeleon

alexdeleon commented Oct 21, 2014

+1

tduffield pushed a commit that referenced this pull request Oct 24, 2014

Tom Duffield
Merge #206 - Fixed container name matching for containers with multip…
…le names.

- Filter out technical names when loading the current container resource.
- Made container name matching more strict.
- Fixed container name matching for containers with multiple names.

@tduffield tduffield closed this Oct 24, 2014

tduffield pushed a commit that referenced this pull request Oct 24, 2014

Tom Duffield
Merge #206 - Fixed container name matching for containers with multip…
…le names.

- Filter out technical names when loading the current container resource.
- Made container name matching more strict.
- Fixed container name matching for containers with multiple names.
@jperville

This comment has been minimized.

Show comment
Hide comment
@jperville

jperville Oct 25, 2014

Contributor

Thank you very much for the merge @tduffield

Contributor

jperville commented Oct 25, 2014

Thank you very much for the merge @tduffield

@jperville jperville deleted the PerfectMemory:fix-container-name-detection branch Sep 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment