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

Make it possible to turn off Ryuk with an environment variable #1181

Merged
merged 3 commits into from
Jan 27, 2019

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Jan 25, 2019

To turn it off, set TESTCONTAINERS_RYUK_DISABLED environment variable to true.

Closes #1023 #700.

if (ryukContainerId != null) {
checkDiskSpace(client, ryukContainerId);
} else {
runInsideDocker(
Copy link
Member

Choose a reason for hiding this comment

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

Nice that we can still run the checks without a long-lived Ryuk container!

@rnorth
Copy link
Member

rnorth commented Jan 25, 2019

This looks pretty much fine to me overall, thank you.

Just a few more things:

  • what's the best way to test this? Is there something efficient we could do in CI to check that things overall work without Ryuk, and that Ryuk doesn't get run when it's not supposed to?

  • It'd probably be worth a small mention in the 'Custom configuration' page of the docs

  • Probably one for another ticket, but if we had a Bitbucket Pipelines page in the docs this would be a natural home for helping make sure Bitbucket users are aware of the setting. WDYT?

@bsideup
Copy link
Member Author

bsideup commented Jan 25, 2019

@rnorth I think the most essential way would be to run Testcontainers tests on Bitbucket :D
Another idea is to run the tests in non-privileged container on Travis, or whatever they use.
I will check if it is feasible

@rnorth
Copy link
Member

rnorth commented Jan 26, 2019

@bsideup absolutely, I was thinking the same! Just not wanting to escalate this into something too big. Whatever is ‘efficient’ so that we can release soon 😄

Sent with GitHawk

@testcontainers testcontainers deleted a comment Jan 26, 2019
@testcontainers testcontainers deleted a comment Jan 26, 2019
@kiview
Copy link
Member

kiview commented Jan 26, 2019

I'd be fine with merging this and add an issue for adding a CI test for this, so we can give the Bitbucket guys a solution quickly 🙂

@bsideup bsideup merged commit 40cdd58 into master Jan 27, 2019
@delete-merged-branch delete-merged-branch bot deleted the ryuk_opt_out branch January 27, 2019 14:43
@trevorgowing
Copy link

Just tried this out in bitbucket pipelines with the snapshot build. Works great. Thanks for this @bsideup.

@bsideup
Copy link
Member Author

bsideup commented Jan 29, 2019

Released in 1.10.6

/cc @trevorgowing @schrieveslaach @raulgomis @nathanburrell

@mariusneo
Copy link

@bsideup i was struggling at work to make the build running on a remote DOCKER_HOST and couldn't bring it to run until i used the environment variable TESTCONTAINERS_RYUK_DISABLED set to false.

Now i am wondering why in ResourceReaper#70 i see the bind hardcoded

binds.add(new Bind("//var/run/docker.sock", new Volume("/var/run/docker.sock")));

In this way when running on a remote host, the ryuk container can't be spawned (since it tries to connect to /var/run/docker.sock when the docker container exposes only a tcp://x.x.x.x:2375 socket.

Wouldn't it make more sense to take the volume to be used from an environment variable (instead of hardcoding it) ?

BTW what are the consequences of not using ryuk in the test which depend on testcontainers containers?

@kiview
Copy link
Member

kiview commented Feb 13, 2019

Hi @mariusneo,
the Ryuk container will be spawned on the remote host as well, therefore being able to mount the local Docker Unix socket.

When not using Ryuk, resources created by Testcontainers (containers, volumes, networks) might not be cleaned up in case of JVM crash or termination with e.g. SIGKILL.

@mariusneo
Copy link

Thanks for the heads-up @kiview.

Does it make sense to have a configurable binding for ryuk (as I've mentioned over a DOCKER_HOST environment variable when present) instead of having a hard coded binding to /var/run/docker.sock ?

If yes, I could do a pull request for such a change . I'll try this scenario on my workstation tomorrow and if it works I'll make a corresponding PR for it.

@mariusneo
Copy link

@kiview i had a look on this issue again in ResourceReaper.java

public static String start(String hostIpAddress, DockerClient client) {
        String ryukImage = TestcontainersConfiguration.getInstance().getRyukImage();
        DockerClientFactory.instance().checkAndPullImage(client, ryukImage);

        List<Bind> binds = new ArrayList<>();
        binds.add(new Bind("//var/run/docker.sock", new Volume("/var/run/docker.sock")));

Is there an elegant way to receive the docker host programmatically to use it in the above referenced binding ?

Through debugging i came across

((DockerClientImpl) ((AuditLoggingDockerClient) client).wrappedClient).dockerClientConfig.getDockerHost()

which is obviously not good enough.

I'd like to get the docker host before making the ResourceReaper.start call in org.testcontainers.DockerClientFactory#client

Could anybody give me a hint in this direction?

@bsideup
Copy link
Member Author

bsideup commented Feb 25, 2019

@mariusneo the host you get (on your machine) might not be (and most probably will not be) the same as inside a container started by the daemon (the same applies to DOCKER_HOST environment variable).

@mariusneo
Copy link

@bsideup i've tested the following scenario:
For building the project a docker image is created which retrieves all its dependencies (jdk, mvn, jar dependencies) and finally builds and tests the project .

The project uses for testing testcontainers reason why I am sending the DOCKER_HOST to be used for test purposes as a --build-arg.

The docker host is exposing a tcp interface (and not a sock one) reason why I am trying to overwrite the binding in the ResourceReaper with a dynamic value.
I'll post a sample project to illustrate my problem.

I guess that by dynamically setting the binding volume to the dockerHost from the dockerClientConfig I wouldn't receive anymore the errors that I currently receive if I don't set TESTCONTAINERS_RYUK_DISABLED to true

@kiview
Copy link
Member

kiview commented Feb 25, 2019

Are you talking about using Testcontainers in running container, based on your image, or are you referring to using Testcontainers as part of your docker build?

@mariusneo
Copy link

@kiview and @bsideup as mentioned yesterday, I'm posting a simple project that can be used to showcase the problem I'm pointing to.

I've made a thorough documentation of the problem that I'm facing in the following project (a proof of concept containing one select 1 postgres testcontainer test):

https://github.com/mariusneo/dockerized-maven-build

TLDR: ryuk can't be accessed when TESTCONTAINERS_RYUK_DISABLED is set to false or is not present.

2019-02-25 16:06:46 WARN  ResourceReaper:137 - Can not connect to Ryuk at my.docker.host:32866
java.net.ConnectException: Connection refused (Connection refused)
	at java.base/java.net.PlainSocketImpl.socketConnect(Native Method)
	at java.base/java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:399)

Thanks in advance for taking the time to investigate this problem.

@kiview
Copy link
Member

kiview commented Feb 26, 2019

Thanks for the very detailed example @mariusneo.
Would you like to create an issue (as a feature request) regarding using Testcontainers as part of docker build (meaning inside a Dockerfile RUN command) and reference your example project there?

I think this use case has never been a top priority for us, but seeing that it's already achievable in the current state without using Ryuk, makes me consider it again.

@mariusneo
Copy link

@kiview i've just created #1274

I'm eager to help with the changes required for the pull request if you would give me some pointers where to get started.

@bsideup
Copy link
Member Author

bsideup commented Feb 26, 2019

@mariusneo could you please explain why the remote Docker does not expose the unix socket?

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.

None yet

5 participants