fix(core): running testcontainer inside container #714

CarliJoy · 2024-10-11T14:25:11Z

closes #475

One step closer to solve #517

Replaces #622

According to the data collected this should fix issues running testcontainers inside a container in almost all cases.

There is still an issue when running it within docker desktop, that is probably easily solved which checking for the existence of host.docker.internal.

But I have to recollect the data to ensure this, so this will be added at later point in time, as with with setting -e TESTCONTAINERS_HOST_OVERRIDE=host.docker.internal an easy workaround exists as well.

codecov · 2024-10-11T14:32:33Z

Codecov Report

Attention: Patch coverage is 95.83333% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@f958cf9). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
core/testcontainers/core/container.py	86.66%	1 Missing and 1 partial ⚠️
core/testcontainers/core/docker_client.py	96.15%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #714   +/-   ##
=======================================
  Coverage        ?   85.62%           
=======================================
  Files           ?       12           
  Lines           ?      654           
  Branches        ?      102           
=======================================
  Hits            ?      560           
  Misses          ?       72           
  Partials        ?       22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

also use utils for platform detection

see testcontainers#475 (comment) for a summary

required to finally figure out how github actions actually work

CarliJoy · 2024-10-18T21:20:51Z

I am happy to announce that I was able to add some DinD and DooD directly into the test suite that work even within the github action here.

I think this is a good starting point to tackle more issues arising from running testonctainer within a container itself.

Following the discussion with @alexanderankin on #720 I am aware that testing non core thing inside the core test is not really wanted.
But I don't know of another method that wouldn't increase the running time by a multitude.

alexanderankin · 2024-10-22T22:57:39Z

core/tests/conftest.py

+def pytest_configure(config: pytest.Config) -> None:
+    """
+    Add configuration for custom pytest markers.
+    """
+    config.addinivalue_line(
+        "markers",
+        "inside_docker_check: test used to validate DinD/DooD are working as expected",
+    )


is this just for filtering in/out dind/dod tests?

That is to filter out tests that run within the test_dind and test_dood tests.
Inside this tests now the docker container is build and executed in a dind and dood environment.
This way we have proper test that dind and dood actually works.

I left the old make test-dind (which is not correct because it's acutall DooD) untouched.
It would actually be better to remove it not only because of the wrong name.
It also doesn't succeed because some things as some auth tests seem to have troubles with TLS settings.
I don't think the complete test suit has to pass inside a dind/dood env.
In the end these test are integration tests and should be treated as such.

lets remove the things that don't make sense

perhaps in a separate PR maybe to make the history clearer

alexanderankin

This is great! I still have a couple things I want to figure out before merging because its a lot. in particular I don't like the breaking change with the returning int instead of string because if I'm not mixing it up there is a method somewhere in there that returns str but actually returns int or str but i dont want to do that without a version bump (4.9 or just v5).

CarliJoy · 2024-10-23T15:21:37Z

in particular I don't like the breaking change with the returning int instead of string because if I'm not mixing it up there is a method somewhere in there that returns str but actually returns int or str but i dont want to do that without a version bump (4.9 or just v5).

To be frank, that doesn't matter as all, as long the packages is not marked as typed (#305), it is not considered by type checkers from end users anyway.
At least this is true for mypy.
A port can be only int so I am quite sure this is only a mistake/bug.
And I have seen other packages fix such mistakes/bug in typing without introducing a new major number.

CarliJoy · 2024-10-23T16:19:14Z

core/testcontainers/core/container.py

    def get_container_host_ip(self) -> str:
-        # infer from docker host
-        host = self.get_docker_client().host()
-        if not host:
-            return "localhost"
-        # see https://github.com/testcontainers/testcontainers-python/issues/415
-        if host == "localnpipe" and system() == "Windows":
-            return "localhost"
-
-        # # check testcontainers itself runs inside docker container
-        # if inside_container() and not os.getenv("DOCKER_HOST") and not host.startswith("http://"):
-        #     # If newly spawned container's gateway IP address from the docker
-        #     # "bridge" network is equal to detected host address, we should use
-        #     # container IP address, otherwise fall back to detected host
-        #     # address. Even it's inside container, we need to double check,
-        #     # because docker host might be set to docker:dind, usually in CI/CD environment
-        #     gateway_ip = self.get_docker_client().gateway_ip(self._container.id)
-
-        #     if gateway_ip == host:
-        #         return self.get_docker_client().bridge_ip(self._container.id)
-        #     return gateway_ip
-        return host
+        connection_mode: ConnectionMode
+        connection_mode = self.get_docker_client().get_connection_mode()
+        if connection_mode == ConnectionMode.docker_host:
+            return self.get_docker_client().host()


Btw: one could argue that this is incorrect.

self.get_docker_client().host() does return a DNS name instead of an IP especially localhost.
Indeed for some containers this has a negative effect: I tried to create a MariaDB container that failed. Using localhost instead of 127.0.0.1 it was trying to connect through the unix socket.

But this is the old behaviour so I kept it the way it is.
We might want to change this in the future.

ah yes, as it says "container_ip" in the method name but it returns a domain name instead, hm.

they both have been replaced by proper `pytest` tests follow up of #714

🤖 I have created a release *beep* *boop* --- ## [4.9.0](testcontainers-v4.8.2...testcontainers-v4.9.0) (2024-11-26) ### Features * **compose:** support for setting profiles ([#738](#738)) ([3e00e71](3e00e71)) * **core:** Support working with env files ([#737](#737)) ([932ee30](932ee30)) ### Bug Fixes * allow running all tests ([#721](#721)) ([f958cf9](f958cf9)) * **core:** Avoid hanging upon bad docker host connection ([#742](#742)) ([4ced198](4ced198)) * **core:** running testcontainer inside container ([#714](#714)) ([85a6666](85a6666)) * **generic:** Also catch URLError waiting for ServerContainer ([#743](#743)) ([24e354f](24e354f)) * update wait_for_logs to not throw on 'created', and an optimization ([#719](#719)) ([271ca9a](271ca9a)) * Vault health check ([#734](#734)) ([79434d6](79434d6)) ### Documentation * Documentation fix for ServerContainer ([#671](#671)) ([0303d47](0303d47)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Tranquility2 · 2024-12-14T18:26:18Z

core/tests/conftest.py

+    py_version = ".".join(map(str, sys.version_info[:2]))
+    image_name = f"testcontainers-python:{py_version}"
+    subprocess.run(
+        [*("docker", "build"), *("--build-arg", f"PYTHON_VERSION={py_version}"), *("-t", image_name), "."],


@CarliJoy @alexanderankin
I know this is already in, but I have to wonder doesn't this breaks anywhere that dosn't uses sudoless docker?

Then is still as good as before, as the Makefile also was using sudoless docker.

Only difference people will know that there is an issue because before they probably never cared to run make test-dind.
So I would say dosn't matter.
Either people will be so smart and see that if subprocess fails that it is there setup in which they required sudo and locally adopt the test or they will simply ignore the failing test and still create an MR that run properly in the pipeline.

Or they will create an Issue at which point we can take care of this.

cool, let me try it on a vm :)

Or put in other words: This is only relevant for testing and as long as people who want to contribute don't run into problems and it works in the pipeline I would not invest time into this.
There are many more pressuring open issues inside the project.

I'm asking because I had some unrelated issues that made me rerun all the tests and now (once I fixed the other stuff) I'm left with this, in any case I'll try and recreate all the env as sudoless.
So sadly I assume this does break :( and currently I'm effected.

Sorry to hear.
So on your dev machince you only can run docker with sudo docker ?
Does is brake also in the pipeline here in gitlab?

ok, so updating if anyone needs in the future, the workaround is very trivial just run with

[*("sudo", "docker", "build"), *("--build-arg", f"PYTHON_VERSION={py_version}"), *("-t", image_name), "."],

once and you should be good :) as the image will be created and available.

We should consider adding some remark in the docs about assuming all devs are using sudoless docker.

No not once -> everytime you update the code.
As the code is included in the docker image.

If you already have the error in front of you, why not catch it and rerun it with sudo?
I assume there is permission error or similar?

First try without sudo and when running into the troubles, just add it.

What do you think of this idea?

Created an issue to document this #749

Just to clarify, once is just to get the tests working, if I don't change anything dind related I tend to trust the CI for those kind of changes, but as a safety net I'll try and move my env to use sudoless.

The correct approach in my options is to stop using subprocess.run for docker related activity, as we have the docker SDK :) I also noted this in the ticket for the long run.

CarliJoy force-pushed the fix_running_inside_container branch from bcfb770 to 51ec378 Compare October 11, 2024 14:29

CarliJoy changed the title ~~Fix running testcontainer inside container~~ fix(core): running testcontainer inside container Oct 11, 2024

CarliJoy force-pushed the fix_running_inside_container branch from 53f8d09 to e0d969e Compare October 11, 2024 16:59

CarliJoy added 5 commits October 18, 2024 22:18

refactor: move all docker client host detection to docker_client

8df7dc9

also use utils for platform detection

fix running testcontainer inside a container

72c7f86

see testcontainers#475 (comment) for a summary

inside_docker fix: add unit tests

94c6c74

add test for find_host_network

c3c69a5

allow determining host network by running container id

a22ebe0

CarliJoy force-pushed the fix_running_inside_container branch 3 times, most recently from d80b3b1 to 341bd03 Compare October 18, 2024 20:53

CarliJoy added 2 commits October 18, 2024 23:13

add test for DinD and DooD

9024083

add debug information for host network test

9f4d1ba

required to finally figure out how github actions actually work

CarliJoy force-pushed the fix_running_inside_container branch from 341bd03 to 9f4d1ba Compare October 18, 2024 21:13

This was referenced Oct 22, 2024

Feature: Change to another layout [src] #720

Open

DinD Tracking Issue #517

Open

alexanderankin reviewed Oct 22, 2024

View reviewed changes

alexanderankin approved these changes Oct 22, 2024

View reviewed changes

CarliJoy commented Oct 23, 2024

View reviewed changes

alexanderankin merged commit 85a6666 into testcontainers:main Oct 23, 2024
19 checks passed

github-actions bot mentioned this pull request Oct 23, 2024

chore(main): release testcontainers 4.9.0 #722

Merged

This was referenced Oct 24, 2024

chore(core): remove test-dind and image makefile entries #725

Merged

fix DinD host detection when using local socket #283

Open

alexanderankin pushed a commit that referenced this pull request Oct 24, 2024

chore(core): remove test-dind and image makefile entries (#725)

82a2e7b

they both have been replaced by proper `pytest` tests follow up of #714

Tranquility2 reviewed Dec 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): running testcontainer inside container #714

fix(core): running testcontainer inside container #714

CarliJoy commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

CarliJoy commented Oct 18, 2024

alexanderankin Oct 22, 2024

CarliJoy Oct 23, 2024

alexanderankin Oct 23, 2024

alexanderankin Oct 23, 2024

alexanderankin left a comment

CarliJoy commented Oct 23, 2024 •

edited

Loading

CarliJoy Oct 23, 2024 •

edited

Loading

alexanderankin Oct 23, 2024

Tranquility2 Dec 14, 2024

CarliJoy Dec 14, 2024

Tranquility2 Dec 14, 2024

CarliJoy Dec 14, 2024 •

edited

Loading

Tranquility2 Dec 14, 2024

CarliJoy Dec 14, 2024

Tranquility2 Dec 14, 2024

CarliJoy Dec 14, 2024

Tranquility2 Dec 14, 2024

Tranquility2 Dec 14, 2024

fix(core): running testcontainer inside container #714

fix(core): running testcontainer inside container #714

Conversation

CarliJoy commented Oct 11, 2024 • edited Loading

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

CarliJoy commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderankin left a comment

Choose a reason for hiding this comment

CarliJoy commented Oct 23, 2024 • edited Loading

CarliJoy Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarliJoy Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarliJoy commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading

CarliJoy commented Oct 23, 2024 •

edited

Loading

CarliJoy Oct 23, 2024 •

edited

Loading

CarliJoy Dec 14, 2024 •

edited

Loading