Fix Ryuk container recursion when TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX is set - #1120
Open
mohitduhan19 wants to merge 2 commits into
Open
mohitduhan19 wants to merge 2 commits into
mohitduhan19 wants to merge 2 commits into
Conversation
…IX is set Once TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX is configured, DockerContainer.image carries that prefix but c.ryuk_image never does. The recursion guard in start() compared the two directly, so it never recognized Ryuk's own container once a prefix was set, causing Reaper._create_instance() to recurse until Python raised RecursionError. This applies the same prefix to both sides of the comparison. Closes testcontainers#1085
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1085
Problem: Once TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX is configured, DockerContainer.init applies the prefix to self.image (self.image = c.hub_image_name_prefix + image), but Reaper._create_instance() builds Ryuk's own container from the unprefixed c.ryuk_image. The recursion guard in DockerContainer.start() used to compare the two directly: if not c.ryuk_disabled and self.image != c.ryuk_image: Reaper.get_instance(). With a prefix configured, that comparison is never equal for Ryuk's own container either, so starting Ryuk triggers Reaper.get_instance(), which creates another Ryuk container, which triggers another Reaper.get_instance(), recursing until Python raises RecursionError (reported as docker.errors.DockerException: ... maximum recursion depth exceeded). This was introduced in #961, which added the prefixing but did not update this comparison.
Fix: apply the same prefix to both sides of the comparison in start(), so Ryuk's own container is still correctly recognized as Ryuk regardless of whether a hub prefix is configured: if not c.ryuk_disabled and self.image != c.hub_image_name_prefix + c.ryuk_image: Reaper.get_instance().
Tests: added two regression tests in tests/core/test_container.py. test_start_does_not_recreate_reaper_for_ryuk_container_with_hub_prefix asserts that with a hub prefix configured, starting Ryuk's own container does not request another Reaper (this fails against the old code; confirmed locally by temporarily reverting the fix). test_start_still_creates_reaper_for_regular_container_with_hub_prefix is a companion test confirming the guard still requests a Reaper for a normal container, so the fix doesn't just turn the guard into a no-op. Both tests construct a DockerContainer and drive start() to completion without a real Docker daemon, by patching the DockerClient the container module resolves at construction time.
Verification: ran the two new tests against the fix and both pass. Verified they fail against the pre-fix comparison (self.image != c.ryuk_image) by temporarily reverting the guard locally, confirming they actually catch the regression. Ran the full tests/core/test_container.py file; the only failures are pre-existing tests that require a real Docker daemon, which isn't available in my sandbox, and are unrelated to this change.