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

#291 Docker for Windows #297

Merged
merged 4 commits into from
Mar 3, 2017
Merged

#291 Docker for Windows #297

merged 4 commits into from
Mar 3, 2017

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Feb 22, 2017

No description provided.

@bsideup bsideup requested a review from rnorth February 22, 2017 09:52
@bsideup bsideup self-assigned this Feb 22, 2017
@bsideup bsideup added this to the 1.1.10 milestone Feb 22, 2017
@kiview
Copy link
Member

kiview commented Feb 24, 2017

Great experience using this on Windows. Just tested it on my Windows Laptop and was happy to see I didn't have to configure any additional JVM properties or ENV variables (I think the docker-java lib behaved differently).

However, docker-compose doesn't seem to work for me in the current implementation. I get the following error i.e. in DockerComposeV2FormatTest:

org.testcontainers.containers.ContainerLaunchException: Container startup failed

	at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:179)
	at org.testcontainers.containers.ContainerisedDockerCompose.start(DockerComposeContainer.java:423)
	at org.testcontainers.containers.DockerComposeContainer.pullImages(DockerComposeContainer.java:120)
	at org.testcontainers.containers.DockerComposeContainer.starting(DockerComposeContainer.java:106)
	at org.testcontainers.containers.FailureDetectingExternalResource$1.evaluate(FailureDetectingExternalResource.java:28)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:237)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Caused by: org.rnorth.ducttape.RetryCountExceededException: Retry limit hit with exception
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:83)
	at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:172)
	... 25 more
Caused by: java.lang.NullPointerException: containerId was not specified
	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:228)
	at com.github.dockerjava.core.command.LogContainerCmdImpl.withContainerId(LogContainerCmdImpl.java:78)
	at com.github.dockerjava.core.command.LogContainerCmdImpl.<init>(LogContainerCmdImpl.java:38)
	at com.github.dockerjava.core.DockerClientImpl.logContainerCmd(DockerClientImpl.java:335)
	at org.testcontainers.containers.GenericContainer.tryStart(GenericContainer.java:236)
	at org.testcontainers.containers.GenericContainer.lambda$start$0(GenericContainer.java:174)
	at org.rnorth.ducttape.unreliables.Unreliables.retryUntilSuccess(Unreliables.java:76)
	... 26 more

I've got the same error in the docker-compose tests of my spock extension.

@rnorth
Copy link
Member

rnorth commented Mar 1, 2017

Has anyone looked into this apparent problem with Compose when using this approach?

@kiview
Copy link
Member

kiview commented Mar 1, 2017

Can someone else reproduce the error I've posted?

@bsideup
Copy link
Member Author

bsideup commented Mar 1, 2017

@rnorth it seems like a problem with Docker Compose integration on Windows, not the Docker for Windows itself.
I'll test it a little bit more (I have a Windows PC to experiment), if it's not related to this change (Docker for Windows support) I would rather create the follow-up tasks

@kiview I can, fails on my machine too

@rnorth
Copy link
Member

rnorth commented Mar 1, 2017

👍 sure, we can split compose support for windows if we have to.

I was just wondering whether there's something unsafe about the approach of volume mounting the docker socket (here) on Docker for Windows. It would seem unlikely, but I can't think of many other things it could be.

@bsideup
Copy link
Member Author

bsideup commented Mar 1, 2017

@rnorth could be
or "createMinGWPath" workaround, I'm not sure that it's needed on Docker for Windows

@kiview
Copy link
Member

kiview commented Mar 1, 2017

I think the mounting does nothing or might create an empty directory somewhere. Not sure though.
+1 for integrating this branch and fixing the Docker-Compose support in another task.

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - only trivial comments

public class WindowsClientProviderStrategy extends DockerClientProviderStrategy {

private static final int PING_TIMEOUT_DEFAULT = 5;
private static final String PING_TIMEOUT_PROPERTY_NAME = "testcontainers.namedpipesocketprovider.timeout";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably change this name since we're not using named pipes

@@ -9,4 +9,4 @@
| Mac OS X - Docker Toolbox | Docker Machine v0.8.0 | |
| Mac OS X - Docker for Mac | 1.12.0 | *Support is best-efforts at present*. `getTestHostIpAddress()` is [not currently supported](https://github.com/testcontainers/testcontainers-java/issues/166) due to limitations in Docker for Mac. |
| Windows - Docker Toolbox | | *Support is limited at present and this is not currently tested on a regular basis*. |
| Windows - Docker for Windows Beta | | *Not currently supported*. |
| Windows - Docker for Windows | | *Supported, but not currently tested on a regular basis.* |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just add the note re Docker Compose not currently working, then 👍

@bsideup
Copy link
Member Author

bsideup commented Mar 2, 2017

@rnorth I did the changes you requested, please take another look

@rnorth
Copy link
Member

rnorth commented Mar 3, 2017

Looks good to me, thanks @bsideup !

@rnorth rnorth merged commit 9b8207e into master Mar 3, 2017
@rnorth rnorth deleted the 291-docker_for_windows branch March 3, 2017 08:22
rnorth added a commit that referenced this pull request Mar 12, 2017
## [1.2.0] - 2017-03-12
### Fixed
- Fix various escaping issues that may arise when paths contain spaces (#263, #279)
- General documentation fixes/improvements (#300, #303, #304)
- Improve reliability of `ResourceReaper` when there are a large number of containers returned by `docker ps -a` (#295)

### Changed
- Support Docker for Windows via TCP socket connection (#291, #297, #309). _Note that Docker Compose is not yet supported under Docker for Windows (see #306)
- Expose `docker-java`'s `CreateContainerCmd` API for low-level container tweaking (#301)
- Shade `org.newsclub` and Guava dependencies (#299, #292)
- Add `org.testcontainers` label to all containers created by Testcontainers (#294)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants