feat: reusable containers #636

matthiasschaub · 2024-07-03T09:28:17Z

adresses #109

Todo:

Write documentation about how to reuse containers.
Warn the user if with_reuse in use but ryuk is disabled.

Open questions:

Should reuse_enable also be configurable via environment variable?
Currently to make reuse work ryuk needs to be disabled. Should the user do this manually? -> if so warn the user if with_reuse in use but ryuk is disabled.
Current tests are sensitive to users ~/.testcontainers.properties. This file should not be present during test run.
Should each container class have the possibility to state if this container can be reused? (DockerContainer.reusable: bool = True)

matthiasschaub · 2024-07-03T11:36:36Z

index.rst

+    ...     second_id == container._container.id
+    >>> print(first_id == second_id)
+    True
+


Should the user be warned that by using this feature, containers need to be removed manually? (That this feature should not be used in a CI)

Also, do we need to make clear how this feature works (explaining the hash in use). -> If a container's run configuration changes, the hash changes and a new container will be used.

seems like you have added these comments to the doc, i think that is fine. the hash would be great to add as users would benefit from knowing exactly what is hashed.

self.image,

self._command,

self.env,

self.ports,

self._name,

self.volumes,

str(tuple(sorted(self._kwargs.items()))), - this may fail and why i want to have this be tucked away inside an obviously readable if block

core/testcontainers/core/config.py

adresses testcontainers#109 Co-authored-by: Levi Szamek <[email protected]>

codecov · 2024-07-08T01:04:51Z

Codecov Report

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

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

Files	Patch %	Lines
core/testcontainers/core/config.py	80.00%	2 Missing ⚠️
core/testcontainers/core/container.py	96.55%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #636   +/-   ##
=======================================
  Coverage        ?   77.27%           
=======================================
  Files           ?       11           
  Lines           ?      616           
  Branches        ?       93           
=======================================
  Hits            ?      476           
  Misses          ?      113           
  Partials        ?       27

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

alexanderankin · 2024-07-08T01:10:12Z

will review and merge if you have no other plans for changes

matthiasschaub · 2024-07-08T06:53:02Z

@alexanderankin we would welcome a review from you, thanks. They are no further plans right now except addressing what ever comes up in the code review.

alexanderankin · 2024-07-31T18:18:54Z

core/testcontainers/core/container.py

+        args = (
+            self.image,
+            self._command,
+            self.env,
+            self.ports,
+            self._name,
+            self.volumes,
+            str(tuple(sorted(self._kwargs.items()))),
+        )
+        hash_ = hashlib.sha256(bytes(str(args), encoding="utf-8")).hexdigest()
+
+        if self._reuse and (not c.tc_properties_testcontainers_reuse_enable or not c.ryuk_disabled):
+            logging.warning(
+                "Reuse was requested (`with_reuse`) but the environment does not "
+                + "support the reuse of containers. To enable container reuse, add "
+                + "the 'testcontainers.reuse.enable=true' to "
+                + "'~/.testcontainers.properties' and disable ryuk by setting the "
+                + "environment variable 'TESTCONTAINERS_RYUK_DISABLED=true'"
+            )
+
+        if self._reuse and c.tc_properties_testcontainers_reuse_enable:
+            docker_client = self.get_docker_client()
+            container = docker_client.find_container_by_hash(hash_)
+            if container:
+                if container.status != "running":
+                    container.start()
+                    logger.info("Existing container started: %s", container.id)
+                logger.info("Container is already running: %s", container.id)
+                self._container = container
+            else:
+                self._start(hash_)
+        else:
+            self._start(hash_)


can we refactor this so that is is obvious where the if clause is that triggers this?

want to make sure

we are doing the hash inside the clause

want to make it more readable - not in general but specifically for ensuring correcteness of logic that disables or enables reuse

Thanks for the feedback! I will revisit this part next week and try to improve upon it.

@alexanderankin I moved the generation of the hash inside the if-clause and removed passing the hash_ to start if reuse is not in use. I think that makes it better readable in general.

alexanderankin · 2024-07-31T18:20:21Z

core/testcontainers/core/container.py

-            self._container.remove(force=force, v=delete_volume)
+            if self._reuse and c.tc_properties_testcontainers_reuse_enable:
+                self._container.stop()
+            else:
+                self._container.remove(force=force, v=delete_volume)


hm, isnt the point to not even stop it so it is warm for next run? i guess if people are using the explicit api then whatever. I do see a bit of a mirror with start so i guess it will just have to be consistent and maybe clear in docs.

In other languages, having a reusable container does not change the contract of the stop() method. This is obviously something that needs to be considered to make this a full fledged use case, but as of now, I would suggest we start with an experimental reusable implementation, that mirrors the Java implementation.

Okay, I see. I updated the code and documentation (how to use reusable containers) to not change the contract of the stop() method.

alexanderankin · 2024-07-31T18:22:58Z

index.rst

+Reusable Containers (Experimental)
+----------------------------------
+
+Containers can be reused across consecutive test runs. To reuse a container, the container configuration must be the same.
+
+Containers that are set up for reuse will not be automatically removed. Thus, those containers need to be removed manually.


"...removed manually."

maybe add:
"In re-usable mode, the 'stop' api on a container will now 'stop' a container, rather than 'remove' it"

After this discussion, the stop method has not been changed.

alexanderankin · 2024-07-31T18:23:37Z

index.rst

+    ...     second_id == container._container.id
+    >>> print(first_id == second_id)
+    True
+


seems like you have added these comments to the doc, i think that is fine. the hash would be great to add as users would benefit from knowing exactly what is hashed.

self.image,

self._command,

self.env,

self.ports,

self._name,

self.volumes,

str(tuple(sorted(self._kwargs.items()))), - this may fail and why i want to have this be tucked away inside an obviously readable if block

alexanderankin · 2024-07-31T18:25:16Z

core/testcontainers/core/container.py

+        if self._network:
+            self._network.connect(self._container.id, self._network_aliases)
+        return self


apparently this part is also fairly jank and we should remove/rework so as a note to myself i can only do that after this pr merges

kiview · 2024-08-01T06:48:08Z

Thanks for looking into this @matthiasschaub 👋
We have no clean spec for this features in Java / across languages, so for now I would suggest, we mostly mirror the Java implementation, including its limitations. We can sync on a cross language spec in the future that provides a better DX, but for now I would strongly favor an implementation that mirrors Java.

I already left some comments within the PR.

Currently to make reuse work ryuk needs to be disabled. Should the user do this manually?

Given the above, if a container has reusable set, it must not be registered with the Ryuk cleanup label (see tc-java).

kiview · 2024-08-01T06:50:11Z

core/testcontainers/core/container.py

+            self.volumes,
+            str(tuple(sorted(self._kwargs.items()))),
+        )
+        hash_ = hashlib.sha256(bytes(str(args), encoding="utf-8")).hexdigest()


Ideally we use the full container create request as the hash input. In tc-java, this is the CreateContainerCmd from docker-java, I guess we have some equivalent request object from the Docker Python SDK somewhere available?

Yeah, that would be the ideal solution. Unfortunately, I could not find an equivalent function to CreateContainerCmd in the Docker SDK for Python: Not by going through the documentation and not by browsing the code base.

do not create Ryuk cleanup instance if reuse enabled and container has been start with `with_reuse`

matthiasschaub · 2024-08-02T12:03:59Z

Thanks for looking into this @matthiasschaub 👋 We have no clean spec for this features in Java / across languages, so for now I would suggest, we mostly mirror the Java implementation, including its limitations. We can sync on a cross language spec in the future that provides a better DX, but for now I would strongly favor an implementation that mirrors Java.

I already left some comments within the PR.

Currently to make reuse work ryuk needs to be disabled. Should the user do this manually?

Given the above, if a container has reusable set, it must not be registered with the Ryuk cleanup label (see tc-java).

Thanks for the review @kiview! I agree. It is sensible to follow the Java implementation.

In commit 1ea9ed1 I do not create a Reaper instance during container start-up if reuse is enabled and container has been started with with_reuse. This works as expected as long as no other container is started without with_reuse. In that case a Reaper instances is created which will remove the reuse container as well.
Is there a way to explicitly exclude a container from the Reapers routine?

matthiasschaub commented Jul 3, 2024

View reviewed changes

core/testcontainers/core/config.py Show resolved Hide resolved

matthiasschaub force-pushed the reusable_containers branch from 44660f1 to 74c8df1 Compare July 3, 2024 12:04

matthiasschaub and others added 7 commits July 7, 2024 10:23

feat: reusable containers

a933873

adresses testcontainers#109 Co-authored-by: Levi Szamek <[email protected]>

docs: add documentation about reusable containers

f0e2bc7

test: additional testcase for reusable containers

08e33ba

test: add newlines for better readability

d2a83bc

warn user if ryuk is disabled but with_reuse used

c781606

docs: fix code highlighting

dd429e7

fix: use Union instead of | for type hint

e87e782

matthiasschaub force-pushed the reusable_containers branch from 50e6160 to e87e782 Compare July 7, 2024 08:23

matthiasschaub added 2 commits July 8, 2024 09:40

refactor: remove TODO comment

c656660

docs: update section on reusable containers

efb1265

alexanderankin reviewed Jul 31, 2024

View reviewed changes

kiview reviewed Aug 1, 2024

View reviewed changes

matthiasschaub added 2 commits August 2, 2024 11:24

feat(reuse): do not change contract of stop method

d4445d6

feat(reuse): do not create Ryuk cleanup instance

1ea9ed1

do not create Ryuk cleanup instance if reuse enabled and container has been start with `with_reuse`

refactor: move hash generation into if clause

ea6fec7

matthiasschaub requested review from alexanderankin and kiview August 3, 2024 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reusable containers #636

feat: reusable containers #636

matthiasschaub commented Jul 3, 2024 •

edited

Loading

matthiasschaub Jul 3, 2024

alexanderankin Jul 31, 2024

codecov bot commented Jul 8, 2024 •

edited

Loading

alexanderankin commented Jul 8, 2024

matthiasschaub commented Jul 8, 2024

alexanderankin Jul 31, 2024

matthiasschaub Aug 2, 2024

matthiasschaub Aug 3, 2024

alexanderankin Jul 31, 2024

kiview Aug 1, 2024

matthiasschaub Aug 2, 2024

alexanderankin Jul 31, 2024

matthiasschaub Aug 2, 2024

alexanderankin Jul 31, 2024

alexanderankin Jul 31, 2024

kiview commented Aug 1, 2024

kiview Aug 1, 2024

matthiasschaub Aug 3, 2024

matthiasschaub commented Aug 2, 2024 •

edited

Loading

feat: reusable containers #636

Are you sure you want to change the base?

feat: reusable containers #636

Conversation

matthiasschaub commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

alexanderankin commented Jul 8, 2024

matthiasschaub commented Jul 8, 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

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

kiview commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasschaub commented Aug 2, 2024 • edited Loading

matthiasschaub commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

matthiasschaub commented Aug 2, 2024 •

edited

Loading