Skip to content
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: support parallel execution of reusable containers #593

Merged
merged 4 commits into from Oct 28, 2022

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Oct 27, 2022

What does this PR do?

It adds a sync.Mutex lock/unlock when a reusable container is created, including a test that demonstrates the bug

In the original bug report (#592), a repro snippet was added, although I preferred a different test that leverages our testcontainers.ParallelContainers API, adding code coverage to the library.

Why is it important?

Thanks to @mhatch-nxcr, who reported this bug in #592, we demonstrated that the implementation of the reusable containers was not complete in terms of a parallel execution of tests creating containers with the library. The bug report found a race condition while checking the Docker client to find containers with the same name when called from a goroutine (i.e. running multiple Test functions including t.Parallel() or our ParallelContainers function).

Internally, when we are checking if we need to reuse a container by its name, the library calls the Docker client with a findContainerByName function of the DockerProvider. At the moment this findContainerByName function was called by any goroutine, no container was already created with the reusable container name, and in consequence the Docker client responded with a list of zero containers, which was expected. Therefore we had to look up the stack call to identify what code was calling that method, until we found that we needed synchronization at the moment the first container was created. That's why we are adding the lock/unlock only for the req.Reuse if/else branch.

Before the changes

graph TD
    B(Test execution)
    B --> C{Parallel creation}
    C -->|ReuseContainer| D[Name: reusable]
    C -->|ReuseContainer| E[Name: reusable]
    C -->|ReuseContainer| F[Name: reusable]
    D -->|findContainerByName| G[Docker client]
    E -->|findContainerByName| G[Docker client]
    F -->|findContainerByName| G[Docker client]
    G -->|filter:name=reusable| H["len(List)"]

After the changes

graph TD
    B(Test execution)
    B --> C{Parallel creation}
    C --> I[sync.Mutex]
    I -->|ReuseContainer| D[Name: reusable]
    I -->|ReuseContainer| E[Name: reusable]
    I -->|ReuseContainer| F[Name: reusable]
    D -->|findContainerByName| G[Docker client]
    E -->|findContainerByName| G[Docker client]
    F -->|findContainerByName| G[Docker client]
    G -->|filter:name=reusable| H["len(List)"]

How to test this

Run the first commit of this PR with and without the second commit.

Another way to test this fix is using what @mhatch-nxcr proposed in the original bug report: because the two tests uses t.Parallel(), they will be run in different goroutines, being affected by the bug. Please run all tests in the reusable package, not only one of them.

package reusable
​
import (
	"context"
	"fmt"
	"testing"
	"time""github.com/docker/go-connections/nat"
	testcontainers "github.com/testcontainers/testcontainers-go"
	"github.com/testcontainers/testcontainers-go/wait"
)
​
const (
	postgresPort     = 5432
	postgresPassword = "test"
	postgresUser     = "test"
	postgresDb       = "test"
)
​
func TestSomething(t *testing.T) {
	t.Parallel()
​
	_, _ = mustSetupPostgresDB(t)
}
​
func TestSomething2(t *testing.T) {
	t.Parallel()
​
	_, _ = mustSetupPostgresDB(t)
}
​
func mustSetupPostgresDB(t *testing.T) (string, testcontainers.Container) {
	natPort := fmt.Sprintf("%d/tcp", postgresPort)
	pg, err := testcontainers.GenericContainer(
		context.Background(),
		testcontainers.GenericContainerRequest{
			ContainerRequest: testcontainers.ContainerRequest{
				Image:        "postgis/postgis",
				Name:         "test-postgres",
				ExposedPorts: []string{natPort},
				Env: map[string]string{
					"POSTGRES_PASSWORD": postgresPassword,
					"POSTGRES_USER":     postgresUser,
					"POSTGRES_DATABASE": postgresDb,
				},
				WaitingFor: wait.ForLog("database system is ready to accept connections").
					WithPollInterval(100 * time.Millisecond).
					WithOccurrence(2),
			},
			Started: true,
			Reuse:   true,
		},
	)
	if err != nil {
		t.Fatal(err)
	}
​
	// Even after log message found Postgres needs a touch more...
	time.Sleep(200 * time.Millisecond)
​
	// Get the container info needed
	port, err := pg.MappedPort(context.Background(), nat.Port(natPort))
	if err != nil {
		t.Fatal(err)
	}
​
	host, err := pg.Host(context.Background())
	if err != nil {
		t.Fatal(err)
	}
​
	return fmt.Sprintf(
		"postgresql://%s:%s@%s:%d/%s?sslmode=disable",
		postgresUser,
		postgresPassword,
		host,
		port.Int(),
		postgresDb,
	), pg
}

Related issues

@mdelapenya mdelapenya requested a review from a team as a code owner October 27, 2022 22:49
@mdelapenya mdelapenya added the bug An issue with the library label Oct 27, 2022
@mdelapenya mdelapenya self-assigned this Oct 27, 2022
generic.go Outdated Show resolved Hide resolved
@kiview
Copy link
Member

kiview commented Oct 28, 2022

Going ahead, we already know that the approach of reusing by name is not ideal and we want to change it. I assume this bug, and accordingly the fix, is independent of the mechanism of how the lookup works, correct?

kiview
kiview approved these changes Oct 28, 2022
@mdelapenya
Copy link
Collaborator Author

Going ahead, we already know that the approach of reusing by name is not ideal and we want to change it. I assume this bug, and accordingly the fix, is independent of the mechanism of how the lookup works, correct?

Exactly, and we have an issue to refactor the lookup mechanism here: #568

@mhatch-nxcr
Copy link

simple, effective. 👍

@mdelapenya mdelapenya merged commit 53f6ee9 into testcontainers:main Oct 28, 2022
14 checks passed
@mdelapenya mdelapenya deleted the fix-reusable-parallel branch October 31, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Container reuse fails with "The container name X is already in use by container"
3 participants