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

Don't use docker.Client instance from context if missing attributes #39973

Merged
merged 1 commit into from Mar 11, 2017

Conversation

terminalmage
Copy link
Contributor

Under some circumstances (which I can't reliably reproduce), the
docker.Client() instance does not have a timeout attribute, causing
anything that needs to use the docker client to result in a traceback.

This fixes this corner case (later branches initialize the timeout
differently).

@ghost
Copy link

ghost commented Mar 11, 2017

@terminalmage, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ticosax, @rallytime and @tyhunt99 to be potential reviewers.

@terminalmage
Copy link
Contributor Author

Hold off on merging this, the issue might be something with docker-py itself.

Under some circumstances (which I can't reliably reproduce), the
docker.Client() instance does not have a timeout attribute, causing
anything that needs to use the docker client to result in a traceback.

This fixes this corner case (later branches initialize the timeout
differently).
@terminalmage terminalmage changed the title Don't try to access the timeout attribute before setting it Don't use docker.Client instance from context if missing attributes Mar 11, 2017
@terminalmage
Copy link
Contributor Author

OK I changed the approach on this PR and renamed it. For some reason, on one of @SaltyCharles' test boxes, the client instance when returned from the __context__ dunder has no timeout, base_url, and probably other attributes. Thing is, you can instantiate a client instance in a docker shell and it has a timeout attribute, so why the instance returned from the __context__ dunder is missing one I do not know. There may be something else interfering that I'm not seeing. Regardless, I've worked around this odd corner case by generating a new Client instance if the one in the dunder lacks the timeout attribute. It's a weird, hacky fix, but I honestly can't think of a more elegant solution since there doesn't appear to be any self-evident reason behind the weird behavior.

@cachedout cachedout merged commit cd0336e into saltstack:2016.3 Mar 11, 2017
@ticosax
Copy link
Contributor

ticosax commented Mar 11, 2017

@terminalmage Can you give some instructions so I can reproduce this behavior myself ?

@terminalmage
Copy link
Contributor Author

@ticosax Sorry, but I can't. I only ever reproduced this on one of @SaltyCharles' VMs, which he gave me access to use. I tried to reproduce on my laptop, and on a couple Vagrant boxes with Docker installed on them, and I never was able to reproduce.

The PR should be pretty straightforward though.

@ticosax
Copy link
Contributor

ticosax commented Mar 13, 2017

It might be a bigger issue than docker-py itself, or just an edge case related to local setup of packages.
In any case I'd put a comment around that line explaining what is going on.
I'll be able to submit a PR in couple of days.

@terminalmage
Copy link
Contributor Author

@ticosax See #39988, I added a comment.

terminalmage pushed a commit that referenced this pull request Mar 13, 2017
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.

None yet

3 participants