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

Remove withPublishAllPorts from Ryuk and stabilize containerInfo content on start #4263

Merged
merged 20 commits into from
Jul 20, 2021

Conversation

kiview
Copy link
Member

@kiview kiview commented Jul 2, 2021

Changes the start behavior of GenericContainer to ensure, that containerInfo is populated with all mapped ports.
In addition, withPublishAllPorts is removed from Ryuk, since the method is deprecated and can lead to non-usable ports on Docker for Windows.

This PR contains quite duplicate code with regards to polling containerInfo until the response stabilizes. I was not sure where to best extract this code to. Therefore, I submitted the PR in order to discuss this as well.

This PR should improve Windows stability substantially and fix the issues described in #3609, originating upstream from docker/for-win#3171.
In addition, the non-idempotent behavior of InspectContainerCmd, as also observed in testcontainers/testcontainers-go#294 is handled, as it was also done in testcontainers/testcontainers-go#295.

I have further created an issue upstream at docker/for-win#11584.

Using publish-all on Windows might lead to the usage of ports in the excluded port range.
@kiview
Copy link
Member Author

kiview commented Jul 2, 2021

Not sure why HttpWaitStrategyTest on CI would fail:
https://github.com/testcontainers/testcontainers-java/runs/2971856438#step:6:1753

Works fine locally.

.stream().filter(Objects::nonNull).count();

if (mappedExposedPorts != exposedPorts.size()) {
throw new ContainerLaunchException("Inspect container response did not contain mapped ports");
Copy link
Member

Choose a reason for hiding this comment

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

Should we include a small sleep to give the API results time to update?

Copy link
Member Author

@kiview kiview Jul 5, 2021

Choose a reason for hiding this comment

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

Oh, I thought sleeping is built into Unreliables, it is not? Then I probably confused this with Awaitility. 100ms?

In case of no mapped exposed ports, the Unreliables lambda should finish after first execution after all.
@kiview
Copy link
Member Author

kiview commented Jul 5, 2021

Interestingly, the last commit made ReusabilityUnitTests fail. I have a feeling, that the test has some wrong assumptions with regards to the immutability of the docker inspect statement response. Not sure why implementing the suggestion by @rnorth made the test fail.

@bsideup
Copy link
Member

bsideup commented Jul 5, 2021

@kiview ReusabilityUnitTests makes use of mocks, so please double check that there isn't something like "expected 1 call but received 3" :)

@kiview
Copy link
Member Author

kiview commented Jul 5, 2021

Oh, I lost the context over the weekend. I added the if-clause specifically for the ReusabilityUnitTests, since the return value of InspectContainerResponse.getNetworkSettings() will be null. But of course, it was not that ideal to change production code only to make the test work again (unless there might be a situation, where getNetworkSettings() can be null for a valid reason and the container should nevertheless proceed in starting.

Is there an alternative to creating an InspectContainerResponse with all required data, except deep stubbing or reflection?

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

As discussed on Slack, we should handle withCreateContainerCmdModifier-like case, where additional ports are exposed without an entry in exposedPorts

@kiview
Copy link
Member Author

kiview commented Jul 19, 2021

In addition, containers from images that have ports defined using EXPOSE, will also report those in the API resonse.

@bsideup
Copy link
Member

bsideup commented Jul 19, 2021

@kiview even without PublishAll? o_O

@kiview
Copy link
Member Author

kiview commented Jul 19, 2021

Yes, because they are still exposed, just not published. I added those cases into a test, to increase the chance of failure in case of a regression on our side.

…nerTest.java

Co-authored-by: Sergei Egorov <bsideup@gmail.com>
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

3 participants