fix(reaper): refactor to allow retries and fix races #2728
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Depends on #2738 |
|
@mdelapenya reminder this is dependent on #2728 which is why it has missing components still. I'll rebase once #2738 is merged. |
|
Rebased and picking up again, hopefully wont require many fixes. Will convert from draft when ready. |
There was a problem hiding this comment.
I only added a few super minor comments, other than that, my only concern is about the implications of these changes with the reuse mode we are proposing in #2768
I'm fine merging this, although I'd like to discuss with you about reuse more in depth, as it could apply to the reaper code as well.
|
@stevenh is there anything I can do to move forward with this PR? |
|
Blocked on #2786 |
|
There are failing tests for the reaper. Could you take a look? |
Thanks for the poke I was expecting some issues here, but hadn't got round to checking yet. |
|
Any progress on this? We're having issues with flaky tests and would love for this to get merged. |
|
ryuk side was merged last week, just fixing a few compatibility issue with tc-java so once that's done, this is next on the list @erikmansson |
Refactor how the reaper is created to allow for proper retries of temporary errors such as container not found issues during startup or shutdown races. This eliminates the use of sync.Once which wasn't solving the problem at hand and replaces it with a singleton spawner with locked access. Wrap reaper errors so we can determine the cause of failures more easily. Fix race condition in port wait when loading from container by always waiting for the port first. Remove unnecessary use of buffering and invalid retry logic in reaper connection handling which could never recover correctly from a partial read. Move the reaper creation just before connections are established in compose to ensure its still running when the Connect calls are made. Previously the reaper was requested in NewDockerComposeWith which means it could have already shutdown before connections are made during the later sections of Up if the startup took over 1 minute. This was causing consistent failures for: TestDockerComposeAPIWithWaitLogStrategy Ensure that resource labels are correct so that resources aren't reaped when the reaper is disabled by excluding session id when reaper is disabled. Error when creating a reaper when the config says it's disabled so that we avoid hard to debug issues because a reaper is running when it shouldn't be. Set org.testcontainers.reap label for containers which should be reaped by the reaper, to prevent containers which disable the reaper from being incorrectly reaped.
* main: fix(reaper): refactor to allow retries and fix races (testcontainers#2728) chore: update ryuk to 0.10.2 (testcontainers#2833)
* main: fix(reaper): refactor to allow retries and fix races (testcontainers#2728) chore: update ryuk to 0.10.2 (testcontainers#2833) feat: add yugabytedb module (testcontainers#2825) fix: update module container struct name and missing imports (testcontainers#2831) chore: replace 'assert' with 'require' (testcontainers#2827) chore: replace 'assert' with 'require' for critical checks (testcontainers#2824) chore: bump ryuk to latest release (testcontainers#2818) feat: add require for critical checks (testcontainers#2812)
Refactor how the reaper is created to allow for proper retries of temporary errors such as container not found issues during startup or shutdown races.
This eliminates the use of sync.Once which wasn't solving the problem at hand and replaces it with a singleton spawner with locked access.
Wrap reaper errors so we can determine the cause of failures more easily.
Fix race condition in port wait when loading from container by always waiting for the port first.
Remove unnecessary use of buffering and invalid retry logic in reaper connection handling which could never recover correctly from a partial read.
Move the reaper creation just before connections are established in compose to ensure its still running when the Connect calls are made.
Previously the reaper was requested in NewDockerComposeWith which means it could have already shutdown before connections are made during the later sections of Up if the startup took over 1 minute.
This was causing consistent failures for:
TestDockerComposeAPIWithWaitLogStrategy
Ensure that resource labels are correct so that resources aren't reaped when the reaper is disabled by excluding session id when reaper is disabled.
Error when creating a reaper when the config says it's disabled so that we avoid hard to debug issues because a reaper is running when it shouldn't be.