-
-
Notifications
You must be signed in to change notification settings - Fork 549
fix: handle stopped containers more gracefully when reuse is enabled #3062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle stopped containers more gracefully when reuse is enabled #3062
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
b3305a6
to
20e85ba
Compare
20e85ba
to
fd826b9
Compare
fd826b9
to
1666dee
Compare
* main: feat(postgres): add WithOrderedInitScripts for Postgres testcontainers (testcontainers#3121) feat(redis): add TLS support (testcontainers#3115) feat: add Docker Model Runner module (testcontainers#3106) feat: add toxiproxy module (testcontainers#3092) chore(ci): run core tests on Testcontainers Cloud (testcontainers#3117) deps(aerospike): replace core module in go.mod (testcontainers#3116)
* main: chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3119) chore(deps): bump github.com/magiconair/properties from 1.8.9 to 1.8.10 (testcontainers#3118) chore(ci): exclude more files for a full-blown build (testcontainers#3122) feat(influxdb): support for influxdbv2 (testcontainers#3072)
@georgespalding thanks for submitting this PR. I'm trying to reproduce it locally with this test: func TestWithReuse(t *testing.T) {
containerName := "my-postgres"
opts := []testcontainers.ContainerCustomizer{
postgres.BasicWaitStrategies(),
testcontainers.CustomizeRequestOption(func(req *testcontainers.GenericContainerRequest) error {
req.Name = containerName
req.Reuse = true
return nil
}),
}
ctr, err := postgres.Run(context.Background(), "postgres:14-bookworm", opts...)
if err != nil {
t.Logf("run postgres testcontainer: %s", err.Error())
} else {
t.Logf("postgres testcontainer is running: %t", ctr.IsRunning())
}
require.NoError(t, err)
require.NotNil(t, ctr)
err = ctr.Stop(context.Background(), nil)
require.NoError(t, err)
ctr1, err := postgres.Run(context.Background(), "postgres:14-bookworm", opts...)
if err != nil {
t.Logf("run postgres testcontainer: %s", err.Error())
} else {
t.Logf("postgres testcontainer is running: %t", ctr1.IsRunning())
}
require.NoError(t, err)
require.NotNil(t, ctr1)
cli, err := testcontainers.NewDockerClientWithOpts(context.Background())
require.NoError(t, err)
require.NotNil(t, cli)
err = cli.ContainerPause(context.Background(), ctr1.GetContainerID())
require.NoError(t, err)
ctr2, err := postgres.Run(context.Background(), "postgres:14-bookworm", opts...)
if err != nil {
t.Logf("run postgres testcontainer: %s", err.Error())
} else {
t.Logf("postgres testcontainer is running: %t", ctr2.IsRunning())
}
require.NoError(t, err)
require.NotNil(t, ctr2)
} This test fails for I tested it against docker.go:L1367 - if p.config.RyukDisabled {
- // ryuk disabled, ensure the container we are about to reuse is running
- if c.State != "running" {
- // found container to reuse, but it is not in state running
- if err := dc.Start(ctx); err != nil {
- return dc, fmt.Errorf("start container %s in state %s: %w", req.Name, c.State, err)
- }
- }
- }
+ // Ensure the container we are about to reuse is running,
+ // but not if it is paused.
+ if c.State != "running" && c.State != "paused" {
+ // found container to reuse, but it is not in state running
+ if err := dc.Start(ctx); err != nil {
+ return dc, fmt.Errorf("start container %s in state %s: %w", req.Name, c.State, err)
+ }
+ } OTOH, testcontainers-go does not provide native support for Pausing/Unpausing containers, unless using the underlying Docker client we expose (see my test). Said that, I'm not sure how to proceed with this PR |
@georgespalding I added a commit with the related fix that I found |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few clarifying questions
* main: chore(ci): close PR if it was sent from main (testcontainers#3123) feat: add `WithReuseByName` for modifying Generic Container Requests (testcontainers#3064) chore(deps): bump github/codeql-action from 3.28.15 to 3.28.16 (testcontainers#3120)
* main: (76 commits) chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3137) chore(deps): bump github.com/shirou/gopsutil/v4 from 4.25.1 to 4.25.4 (testcontainers#3133) chore(deps): bump github.com/docker/docker from 28.0.1+incompatible to 28.1.1+incompatible (testcontainers#3152) feat(memcached): add memcached module (testcontainers#3132) fix(etcd): single node etcd cluster access (testcontainers#3149) feat(valkey): add TLS support for Valkey (testcontainers#3131) fix(dockermodelrunner): wait for the model to be pulled (testcontainers#3125) fix(localstack): remove checksum before parsing version (testcontainers#3130) fix(dockermodelrunner): dependency with socat chore: prepare for next minor development cycle (0.38.0) chore: use new version (v0.37.0) in modules and examples fix: handle stopped containers more gracefully when reuse is enabled (testcontainers#3062) feat(gcloud): add option to run firestore in datastore mode (testcontainers#3009) feat: support for mounting images (testcontainers#3044) chore(ci): close PR if it was sent from main (testcontainers#3123) feat: add `WithReuseByName` for modifying Generic Container Requests (testcontainers#3064) chore(deps): bump github/codeql-action from 3.28.15 to 3.28.16 (testcontainers#3120) chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3119) chore(deps): bump github.com/magiconair/properties from 1.8.9 to 1.8.10 (testcontainers#3118) chore(ci): exclude more files for a full-blown build (testcontainers#3122) ...
* main: (302 commits) chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3137) chore(deps): bump github.com/shirou/gopsutil/v4 from 4.25.1 to 4.25.4 (testcontainers#3133) chore(deps): bump github.com/docker/docker from 28.0.1+incompatible to 28.1.1+incompatible (testcontainers#3152) feat(memcached): add memcached module (testcontainers#3132) fix(etcd): single node etcd cluster access (testcontainers#3149) feat(valkey): add TLS support for Valkey (testcontainers#3131) fix(dockermodelrunner): wait for the model to be pulled (testcontainers#3125) fix(localstack): remove checksum before parsing version (testcontainers#3130) fix(dockermodelrunner): dependency with socat chore: prepare for next minor development cycle (0.38.0) chore: use new version (v0.37.0) in modules and examples fix: handle stopped containers more gracefully when reuse is enabled (testcontainers#3062) feat(gcloud): add option to run firestore in datastore mode (testcontainers#3009) feat: support for mounting images (testcontainers#3044) chore(ci): close PR if it was sent from main (testcontainers#3123) feat: add `WithReuseByName` for modifying Generic Container Requests (testcontainers#3064) chore(deps): bump github/codeql-action from 3.28.15 to 3.28.16 (testcontainers#3120) chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3119) chore(deps): bump github.com/magiconair/properties from 1.8.9 to 1.8.10 (testcontainers#3118) chore(ci): exclude more files for a full-blown build (testcontainers#3122) ...
What does this PR do?
Fixes a bug that causes testcontainers to hang for a really long time waiting on a stopped container when reuse is enabled.
The fix in short is:
When a container is being created with reuse enabled:
The alternative cases above should work same as before
Why is it important?
The way the library works today, if a reused container exists, but is not running, it freezes for a long time, which is a really bad developer experience.
This fix is not watertight (ses the
docker pause
scenario below), but instantly tells you that something is wrong, giving fast feedback.Related issues
How to test this PR
Create a little program that runs a testcontainer:
docker ps
-> container is not therego run ./
-> printspostgres testcontainer is running: true
docker ps
-> container is runningdocker stop my-postgres
go run ./
-> printspostgres testcontainer is running: true
docker pause my-postgres
go run ./
-> printsrun postgres testcontainer: generic container: create container: start container my-postgres in state paused: container start: Error response from daemon: cannot start a paused container, try unpause instead