Verify Reaper state to create new or return existing instance #782

mdonkers · 2023-01-25T16:49:14Z

What does this PR do?

Resolves #764 and also makes #258 (PR) obsolete.

As the issue describes, the Reaper container might exit if there's no connection for a longer time. For example when running multiple integration tests, some not using TestContainers. When that happens the next test will fail as Reaper is created as singleton but no connection can be created.

Why is it important?

Have reliable tests

Related issues

netlify · 2023-01-25T16:49:24Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`fb64671`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/63d8d81b169a4a0007561126
😎 Deploy Preview	https://deploy-preview-782--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mdonkers · 2023-01-25T16:58:59Z

@mdelapenya this is the PR for better Reaper consistency.

I opted to use the Container.State() for checking if running, instead of using a connection, because it seems nicer using Container functions. The caveat is in some scenarios, there is a very slight chance the container exits between checking its state and running the Connect() function.

Fix linting errors

mdonkers · 2023-01-27T09:40:23Z

@mdelapenya it seems like this PR is awaiting approval to run tests (again?). Maybe because I fixed a linting error and then rebased and pushed again?

reaper_test.go

reaper.go

mdelapenya · 2023-01-30T23:47:45Z

Sorry for the merge conflicts 😞 , we needed to extract the calculation of the docker host to an internal package as part of certain refactors (#796). Do you want me to resolve them?

…n-failure-or-exit * origin/main: docs: add intel as user (testcontainers#798) chore: bump containerd in examples (testcontainers#797) chore(deps): bump github.com/containerd/containerd from 1.6.15 to 1.6.16 (testcontainers#793) chore: extract docker host calculation to an internal package (testcontainers#796) chore: run "go mod tidy" automatically when creating examples (testcontainers#794) chore: build images with backoff retries (testcontainers#792)

mdonkers · 2023-01-31T08:58:56Z

Sorry for the merge conflicts disappointed , we needed to extract the calculation of the docker host to an internal package as part of certain refactors (#796). Do you want me to resolve them?

Conflicts resolved. Wasn't that bad luckily :-)

mdelapenya · 2023-01-31T09:30:20Z

reaper.go

+	// If reaper already exists and healthy, re-use it
+	if reaperInstance != nil {
+		// Verify this instance is still running by checking state.
+		// Can't use Container.IsRunning because the bool is not updated when Reaper is terminated


Meanwhile the CI runs (tests will pass) can you elaborate a bit on this comment? Do you think that bool should be updated in consequence (probably here? in a follow-up?

So the Container.IsRunning is simply a boolean flag (

testcontainers-go/docker.go

Line 63 in fb64671

isRunning bool

) that gets updated when a container is stopped / terminated (

testcontainers-go/docker.go

Line 233 in fb64671

c.isRunning = false

). And while the Reaper is a 'regular' container, it is never terminated via the Stop() or Terminate() methods. Instead, it terminates automatically by itself by closing the connection to it (

testcontainers-go/docker.go

Line 241 in fb64671

case c.terminationSignal <- true:

).
Which makes the IsRunning() unreliable for the Reaper container as it would never return false.

That's why I chose to poll Docker and get the state from there, as it should be reliable.

What I think would be better, is if the IsRunning() method (

testcontainers-go/docker.go

Line 89 in fb64671

func (c *DockerContainer) IsRunning() bool {

) gets updated to what I'm doing below; checking from Docker if the state is running or not. That would make it reliable and not depend on a boolean flag internally updated.
I did not want to touch this code in the PR because I don't know the history behind it. Perhaps IsRunning is regularly checked and would be relatively heavy on the Docker calls?

I see, thanks for such a clear explanation, now I see what you mean.

I think checking the state from Docker would be more accurate than the one we have now, and I understand your concerns about performance if that method is called many times. I've checked their usages and are just a few of them at this moment (particularly worried on the GenericContainer function calling it).

Adding that code, we could also take the situation to a place where multiple goroutines in tests are trying to read the container state at the same time, and one decides to see it as not running, and the next one as running, which could be worse.

I'd keep this simple as in your submission, and evaluate if we need to check for the state more frequently.

mdelapenya

Shaun approves 😎

This comment does not affect the review, so will eventually merge this one today

Thank you very much for your support!

mdelapenya · 2023-01-31T10:11:47Z

Mmm I've checkout out the PR locally to verify one weird behaviour that I'm seeing in another issue, which happens when there are multiple packages in the Go project running the tests. How go test works is that it runs each package separately, creating as many Ryuk containers as packages in the execution. I've verified that even with this PR, there are multiple Ryuk containers. Let me do some verifications on accessing the mutex and will let you know asap

mdelapenya · 2023-01-31T13:07:26Z

I've verified that even with this PR, there are multiple Ryuk containers. Let me do some verifications on accessing the mutex and will let you know asap

This is the expected behaviour of go test ./...: creates a test binary for each package, therefore no synchronisation across them will happen and, effectively, as many reaper instances will be running for each package in the test execution.

Said that, if you agree, we can merge this one, as it covers the scenario of running just one reaper instance per test binary.

mdonkers · 2023-01-31T13:46:37Z

All good from my side. Would be happy to have it merged.

* main: chore: update Docker labels for containers (testcontainers#813) fix: nil pointer dereference in HealthStrategy (testcontainers#802) fix: Synchronise writes to containers map (testcontainers#812) chore(deps): bump google.golang.org/api from 0.108.0 to 0.109.0 in /examples (testcontainers#810) chore(deps): bump cloud.google.com/go/spanner in /examples/spanner (testcontainers#806) chore: restructure Docker helper methods (testcontainers#799) Verify Reaper state to create new or return existing instance (testcontainers#782) docs: add intel as user (testcontainers#798) chore: bump containerd in examples (testcontainers#797) chore(deps): bump github.com/containerd/containerd from 1.6.15 to 1.6.16 (testcontainers#793) chore: extract docker host calculation to an internal package (testcontainers#796) chore: run "go mod tidy" automatically when creating examples (testcontainers#794) chore: build images with backoff retries (testcontainers#792) fix: use right import package for compose in docs (testcontainers#791) chore(deps): bump google.golang.org/grpc from 1.52.1 to 1.52.3 in /examples (testcontainers#790) Add devcontainer file (testcontainers#765) chore: check dependabot dependencies weekly (testcontainers#789) chore(deps): bump google.golang.org/grpc from 1.52.0 to 1.52.1 in /examples (testcontainers#783) chore: support for titles in examples (testcontainers#775)

emetsger · 2024-04-22T02:52:07Z

Mmm I've checkout out the PR locally to verify one weird behaviour that I'm seeing in another issue, which happens when there are multiple packages in the Go project running the tests.

@mdelapenya I believe this is because the reaper container can be exposed by the Docker API before the reaper is ready to accept connections.

I've opened the following PRs to address this:

Add a HEALTHCHECK to the reaper Docker image (Add HEALTHCHECK to Linux Docker image. moby-ryuk#128)
Update testcontainers-go to consider the health status of the reaper container before returning it (Fix race condition when looking up reaper (ryuk) container #2508)

mdonkers requested a review from a team as a code owner January 25, 2023 16:49

mdelapenya added the bug An issue with the library label Jan 25, 2023

mdelapenya self-assigned this Jan 25, 2023

Verify Reaper state to create new or return existing instance

02a5664

Fix linting errors

mdonkers force-pushed the 764-reaper-recreate-on-failure-or-exit branch from 409d431 to 02a5664 Compare January 25, 2023 17:01

mdonkers added 3 commits January 26, 2023 09:25

Merge branch 'main' into 764-reaper-recreate-on-failure-or-exit

d7bb01c

Merge branch 'main' into 764-reaper-recreate-on-failure-or-exit

f880f40

Merge branch 'main' into 764-reaper-recreate-on-failure-or-exit

7ce945e

Play nice with other integration tests

9a660dc

mdelapenya reviewed Jan 30, 2023

View reviewed changes

reaper_test.go Outdated Show resolved Hide resolved

reaper.go Outdated Show resolved Hide resolved

Rename reaperSingleton to reaperInstance

983e517

Miel Donkers added 2 commits January 31, 2023 09:39

Don't skip for short tests, always run

82b8372

mdelapenya reviewed Jan 31, 2023

View reviewed changes

mdelapenya approved these changes Jan 31, 2023

View reviewed changes

mdelapenya merged commit 10cdf3a into testcontainers:main Jan 31, 2023

mdonkers deleted the 764-reaper-recreate-on-failure-or-exit branch January 31, 2023 14:32

mdonkers mentioned this pull request Jan 31, 2023

Don't re-use reaper if it failed to initialize #258

Closed

emetsger mentioned this pull request Apr 22, 2024

Add HEALTHCHECK to Linux Docker image. testcontainers/moby-ryuk#128

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify Reaper state to create new or return existing instance #782

Verify Reaper state to create new or return existing instance #782

mdonkers commented Jan 25, 2023

netlify bot commented Jan 25, 2023 •

edited

mdonkers commented Jan 25, 2023

mdonkers commented Jan 27, 2023

mdelapenya commented Jan 30, 2023

mdonkers commented Jan 31, 2023

mdelapenya Jan 31, 2023

mdonkers Jan 31, 2023

mdelapenya Jan 31, 2023

mdelapenya left a comment

mdelapenya commented Jan 31, 2023

mdelapenya commented Jan 31, 2023

mdonkers commented Jan 31, 2023

emetsger commented Apr 22, 2024

Verify Reaper state to create new or return existing instance #782

Verify Reaper state to create new or return existing instance #782

Conversation

mdonkers commented Jan 25, 2023

What does this PR do?

Why is it important?

Related issues

netlify bot commented Jan 25, 2023 • edited

✅ Deploy Preview for testcontainers-go ready!

mdonkers commented Jan 25, 2023

mdonkers commented Jan 27, 2023

mdelapenya commented Jan 30, 2023

mdonkers commented Jan 31, 2023

mdelapenya Jan 31, 2023

Choose a reason for hiding this comment

mdonkers Jan 31, 2023

Choose a reason for hiding this comment

mdelapenya Jan 31, 2023

Choose a reason for hiding this comment

mdelapenya left a comment

Choose a reason for hiding this comment

mdelapenya commented Jan 31, 2023

mdelapenya commented Jan 31, 2023

mdonkers commented Jan 31, 2023

emetsger commented Apr 22, 2024

netlify bot commented Jan 25, 2023 •

edited