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

Use host port 0 when setting up PortBindings #4396

Closed
wants to merge 2 commits into from
Closed

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Aug 26, 2021

Fixes #4395

See also docker/for-mac#5588

@rnorth
Copy link
Member Author

rnorth commented Aug 26, 2021

I'm running all Testcontainers tests using this branch, using the Docker wormhole pattern on a Mac.

One slightly unexpected failure is this in the gcloud module - I doubt it's anything to do with this PR, but it seems odd that it's happening and perhaps indicates a gap in our testing of docker-wormhole usage:

org.testcontainers.containers.DatastoreEmulatorContainerTest > testSimple FAILED
    java.lang.IllegalArgumentException: Project endpoint "172.17.0.1:55070/v1/projects/test-project" must include scheme.
        at com.google.datastore.v1.client.DatastoreOptions$Builder.projectEndpoint(DatastoreOptions.java:148)
        at com.google.cloud.datastore.spi.v1.HttpDatastoreRpc.<init>(HttpDatastoreRpc.java:71)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreRpcFactory.create(DatastoreOptions.java:60)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreRpcFactory.create(DatastoreOptions.java:54)
        at com.google.cloud.ServiceOptions.getRpc(ServiceOptions.java:561)
        at com.google.cloud.datastore.DatastoreOptions.getDatastoreRpcV1(DatastoreOptions.java:169)
        at com.google.cloud.datastore.DatastoreImpl.<init>(DatastoreImpl.java:57)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreFactory.create(DatastoreOptions.java:50)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreFactory.create(DatastoreOptions.java:44)
        at com.google.cloud.ServiceOptions.getService(ServiceOptions.java:541)
        at org.testcontainers.containers.DatastoreEmulatorContainerTest.testSimple(DatastoreEmulatorContainerTest.java:33)

@rnorth
Copy link
Member Author

rnorth commented Aug 27, 2021

OK, having run all the tests on a Mac, using the wormhole pattern, I'd say the result is fine. The Mac in question is an x86 machine using Docker Desktop 3.6.0.

There were a few failures, but I think as a direct result of tests/modules that don't play well with a remote docker host rather than anything else.

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Works fine on Windows and WSL as well.

@rnorth rnorth marked this pull request as ready for review September 7, 2021 12:17
@rnorth rnorth requested a review from bsideup as a code owner September 7, 2021 12:17
@bsideup
Copy link
Member

bsideup commented Sep 7, 2021

@rnorth Have you heard anything from the Docker team on this one? I am seriously worried thwt this may break unexpectedly...

@rnorth
Copy link
Member Author

rnorth commented Sep 7, 2021

@bsideup not yet.

@rnorth
Copy link
Member Author

rnorth commented Sep 20, 2021

I'm going to move this back to draft as we still don't have a clear steer on whether this is safer. We're still broken in some local DinD use cases, which is a bad thing, and I'm wondering if there's another way to tackle this (e.g. an opt-in configuration setting that applies this workaround).

@kiview
Copy link
Member

kiview commented Sep 20, 2021

I would say, this fix does not make it worse. Instead of increasing our own complexity, I'd opt for implementing it in a straightforward way and be prepared to revert this in the future in case Docker would break compatibility. But I would not design directly for this eventuality.

We had to deal with certain breaking changes in Docker, especially in their desktop distributions, in the past already and it will happen again in the future, but this should not paralyze us or make us overly defensive.

@rnorth
Copy link
Member Author

rnorth commented Oct 8, 2021

To round this off, from docker/for-mac#5588:

I can't guarantee anything but I think there's no imminent danger of it stopping working: I'll add a test case which checks that it continues to work and linking to this issue so can avoid breaking it if possible / give you advance warning

I think this should give us the confidence to (a) use this workaround, and (b) know that if the workaround is ever on the way to being broken we'll be informed ahead of time.

The other comments give me hope that a longer term solution can be developed, but we'll have to wait on Docker to make some changes.

@kiview
Copy link
Member

kiview commented Oct 8, 2021

This PR fixes the currently failing ImagePullPolicyTest and AuthenticatedImagePullTest on Docker Desktop for Windows with WSL backend.

@smurfy
Copy link

smurfy commented Apr 22, 2022

Any update on this? Still failing for me on mac with docker desktop 4.7.1 (77678)

Tried testcontainers 1.16.3 and 1.17.1

@kiview
Copy link
Member

kiview commented Jan 3, 2023

The correct way is now document through #6383.

@kiview kiview closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't connect to Ryuk when using when running in container and using Docker-Socket-Mounting on Docker Desktop
4 participants