Command-based port check in HostPortWaitStrategy in case of Docker for Mac #236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bsideup
commented
Oct 25, 2016
| if (DockerClientFactory.instance().isUsing(ProxiedUnixSocketClientProviderStrategy.class)) { | ||
| List<Integer> exposedPorts = container.getExposedPorts(); | ||
|
|
||
| Integer exposedPort = exposedPorts.stream() |
There was a problem hiding this comment.
unfortunately, we have to do some reverse engineering to find the port originally exposed since it's not private (protected, in fact)
| return; | ||
| } | ||
|
|
||
| String[][] commands = { |
There was a problem hiding this comment.
two strategies at this moment - one without BASH because it's not available in Alpine linux for example, and another one with BASH
| public class HostPortWaitStrategy extends GenericContainer.AbstractWaitStrategy { | ||
|
|
||
| private static final String SUCCESS_MARKER = "TESTCONTAINERS_SUCCESS"; |
There was a problem hiding this comment.
we have to use string-based success marker because exit code is not available
|
Sorry for being slow to respond to this. It seems like a neat idea - while it is a little 'hacky' it would insulate our users from this nasty issue, so is a good thing in the short term. I'm having some issues with tests running locally, though - not quite sure why, though I've not done much digging yet. Any ideas? |
rnorth
reviewed
Nov 7, 2016
| checkStrategy = () -> { | ||
| for (String[] command : commands) { | ||
| try { | ||
| if (container.execInContainer(command).getStdout().contains(SUCCESS_MARKER)) { |
There was a problem hiding this comment.
This will cause a log message to come out at INFO level - might get a bit noisy when the check has to be repeated many times. I'm not quite sure of the best answer really, but maybe we should change the execInContainer method so that it logs at DEBUG instead? What do you think?
There was a problem hiding this comment.
sounds great! The INFO level is a bit verbose for it. Where should we change it?
There was a problem hiding this comment.
org/testcontainers/containers/GenericContainer.java:820 would be the place.
There was a problem hiding this comment.
I mean, in this PR or as a separate change? :)
There was a problem hiding this comment.
Aha, oops! In this PR is fine I think!
|
Ah OK - may have found part of the problem.
Integer exposedPort = exposedPorts.stream()
.map(container::getMappedPort)
.filter(port::equals)
.findFirst()
.orElse(null);Seems to start off with the internal port numbers, but maps them to the external form before checking quality with Then the commands: { "/bin/sh", "-c", "nc -vz -w 1 localhost " + exposedPort + " && echo " + SUCCESS_MARKER },
{ "/bin/bash", "-c", "</dev/tcp/localhost/" + exposedPort + " && echo " + SUCCESS_MARKER }are picking up the external port number, which is not valid when executed inside the container. Maybe I've missed something important (!), but it seems to me that commenting out these lines should work: Integer exposedPort = exposedPorts.stream()
// .map(container::getMappedPort)
// .filter(port::equals)
.findFirst()
.orElse(null); |
rnorth
added a commit
that referenced
this pull request
Nov 7, 2016
|
FYI I've put a possible fix on this branch: https://github.com/testcontainers/testcontainers-java/tree/zeroturnaround-160-docker4mac_port_check If I've completely missed the point please let me know!!! |
|
@rnorth see my note on the PR. Yes, ideally, we should use "exposedPorts", BUT. Of course we can ignore it for now and use exposed ports directly |
|
Yeah OK - it's just that I think I understand how the streams flow maybe should work now... Should it perhaps be this? For each It's just that right now, it's returning |
|
@rnorth we perform |
|
But the result of that is that we get Basically we need something like |
|
@rnorth actually, you're right. Now is the question - why it passed locally on my machine :D let me take a look :D |
|
Hehe, docker never fails to surprise me! Maybe there's some different port mapping behaviour happening or something... |
|
@rnorth fixed :) Sorry, no idea how it happened. Maybe some last minute fix I forgot to test, sorry :) |
|
Great, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #160
I did a test run on my Mac with Docker for Mac, everything is green :)
Consider it as a temporary hack until the issue with port forwarding is fixed on Docker's side