feat: more robust Reusable containers experience #2768

mdelapenya · 2024-09-04T16:20:21Z

What does this PR do?

This PR modifies the way the Reuse mode is implemented in the project, taking tc-java as the implementation reference. For that:

instead of passing Reuse plus the container name, we don't need to pass a container name anymore, only the Reuse field. This is a breaking change in terms of semantics.
we are calculating a hash of the container request just before the creation of the container.
we use github.com/mitchellh/hashstructure/v2 for the hash of the container request, with the following exceptions:
- functions in the struct are ignored (e.g. config modifiers). Please search for hash:"ignore" to identify them all.
- if the Files field contains a reference to a directory in the host, it won't participate in the calculation of the hash, as we don't want to traverse the directory getting the hash for the directory tree.
- for the real files in the Files field, we'll read their bytes and use them get a hash, which will be added to the struct hash.
for containers marked for reuse, two labels will be added:
- org.testcontainers.hash: the hash of the container request.
- org.testcontainers.copied_files.hash: the hash of the files copied to the container using the Files field in the container request.
those two labels will be used to check if a container exists in the docker daemon: the code will do a backoff search up-to 5 seconds (not configurable, maybe desired?), and if there is no container, it will create a new one. Else, the container will be reused. This backoff search is needed because we want to support the use case where multiple and different test processes start the same container, so they can "reuse" it.
the SessionID label of the container will be removed from the container request before the container is created, to not tell Ryuk this container must be terminated alongside the test session. Therefore, the container persists across test sessions.
the ReuseOrCreateContainer from the provider struct has been deprecated for removal in the upcoming releases.
the docs for Reuse has been updated, trying to make it clear the expectations around Reuse.
the tests have been updated too, to reflect the new state for Reuse.
modules are receving a functional option for marking module containers as reusable.

Why is it important?

Create a more consistent experience around reuse, based on the container state and not forcing the user to add a container name, that can be reused by mistake in a totally different container request.

We expect this change helps the community, but at the same time warn about its usage in parallel executions, as it could be the case that two concurrent test sessions get to the container creation at the same time, which could lead to the creation of two containers with the same request.

Related issues

Docs

Render URLs:

Reuse docs: https://deploy-preview-2768--testcontainers-go.netlify.app/features/creating_container/#reusable-container
Reuse option for modules: https://deploy-preview-2768--testcontainers-go.netlify.app/modules/chroma/#withreuse

Follow-ups

Please try out this branch, and read the docs so that they can be improved if needed 🙏

…ed files

Removing the sessionID label, it won't be pruned by Ryuk

netlify · 2024-09-04T16:20:39Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`799ee15`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/66d9710ef06f0c0007ef30bf
😎 Deploy Preview	https://deploy-preview-2768--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 configuration.

mdelapenya · 2024-09-04T16:22:09Z

@stevenh as always, your wise 👀 are more than welcome!

mdelapenya · 2024-09-05T08:13:17Z

docker.go

+		// can proceed with the creation of the container.
+		// This is needed because we need to synchronize the creation of the container across
+		// different test programs.
+		c, err := p.waitContainerCreationInTimeout(ctx, hash, 5*time.Second)


I see this as a potential configuration:

a property: testcontainers.reuse.search.timeout, and

an env var: TESTCONTAINERS_REUSE_SEARCH_TIMEOUT

Since the PR description says:

We expect this change helps the community, but at the same time warn about its usage in parallel executions, as it could be the case that two concurrent test sessions get to the container creation at the same time, which could lead to the creation of two containers with the same request.

wouldn't we want to accept this limitation and allow for the race condition with the current implementation?

Indeed. Added that comment as an idea for a potential follow-up

aaomidi · 2024-09-05T20:20:52Z

I wonder if this solves this bug: #2749

I'll do a small code review here as well. Thank you for this!

stevenh

Done an initial pass

stevenh · 2024-09-07T15:02:14Z

docker.go

-	err = req.creatingHook(ctx)
-	if err != nil {
-		return nil, err
+	if req.Reuse {


question: Not sure about this, I would expect reused containers to still only persist while in use, is that the intent?

The edge case for shutdown while still in use, is addressed by reaper rework PRs, so it should be safe to remove this if the desired behaviour is to share between test runs that are within a reasonable window.

stevenh · 2024-09-07T15:06:03Z

docker.go

+		reuseContainerMx.Lock()
+		defer reuseContainerMx.Unlock()


question: This will lock for the duration of rest of the call, is that the intention?

stevenh · 2024-09-07T15:06:35Z

docker.go

+		req.Labels[core.LabelContainerHash] = fmt.Sprintf("%d", hash.Hash)
+		req.Labels[core.LabelCopiedFilesHash] = fmt.Sprintf("%d", hash.FilesHash)


question: do we need multiple hashes?

This is aligned with the Java implementation. We need these two in order to keep track of the added files before the container is started (Files attribute in the container request)

stevenh · 2024-09-07T15:07:03Z

docker.go

+		c, err := p.waitContainerCreationInTimeout(ctx, hash, 5*time.Second)
+		if err != nil && !errdefs.IsNotFound(err) {
+			// another error occurred different from not found, so we return the error
+			return nil, err


suggestion: Can we wrap so we know where it came from

stevenh · 2024-09-07T15:08:48Z

docker.go

+	}
+
+	var resp container.CreateResponse
+	if req.Reuse {


suggestion: combine the two if's if the above isn't removed, based on previous question

stevenh · 2024-09-07T15:53:10Z

request_hash.go

+				if !ok {
+					// Calculate the hash of the file content only if it is a file.
+					fileContent, err = os.ReadFile(f.HostFilePath)
+					if err != nil {


bug: this will lead to unexplained issues.

stevenh · 2024-09-07T15:53:21Z

request_hash.go

+			// the hash will be zero to avoid breaking the hash calculation.
+			if f.Reader != nil {
+				fileContent, err = io.ReadAll(f.Reader)
+				if err != nil {


bug: this will lead to unexplained issues.

stevenh · 2024-09-07T15:53:25Z

request_hash.go

+				}
+			} else {
+				ok, err := isDir(f.HostFilePath)
+				if err != nil {


bug: this will lead to unexplained issues.

stevenh · 2024-09-07T15:55:41Z

request_hash.go

+)
+
+// containerHash represents the hash of the container configuration
+type containerHash struct {


question: are two independent hashes needed.

stevenh · 2024-09-07T15:57:28Z

request_hash.go

+			// Read the file content to calculate the hash, if there is an error reading the file,
+			// the hash will be zero to avoid breaking the hash calculation.
+			if f.Reader != nil {
+				fileContent, err = io.ReadAll(f.Reader)


suggest: Use streaming hash instead of reading files into memory.

mdelapenya · 2024-09-10T10:06:32Z

@kiview I'm converting to draft until we discuss internally about the implications of switching from the container name-based approach to a hash-based approach. Will share here the discussion once it takes place.

mdelapenya · 2024-09-24T08:20:35Z

docker.go

+	if req.Reuse {
+		// Remove the SessionID label from the request, as we don't want Ryuk to control
+		// the container lifecycle in the case of reusing containers.
+		delete(req.Labels, core.LabelSessionID)


We discussed to not remove the sessionId but an specific label when ryuk is enabled. TBD

mdelapenya added 21 commits September 3, 2024 14:37

feat: calculate hash of a container request

4902c76

chore: run mod tidy

10daab5

feat: calculate the hash of a container request, considering the copi…

ed3e1fa

…ed files

chore: add labels for the container hashes

442dd4a

feat: reuse containers based on hashes

969fe59

docs: document reuse

c2f5e83

chore: add more tests for not hashing dir contents

dcbb467

fix: wait for the container to be created

c07b5dc

chore: simplify deprecation

d1d40ce

docs: improve docs for reuse

7aa26da

feat: do not remove the container with Ryuk

35da30a

Removing the sessionID label, it won't be pruned by Ryuk

docs: enrich reuse docs

0ffec34

feat: support modules to be reused

7a505ba

docs: refinement

a544bc0

chore: adjust tests

c9356ed

chore: adapt test to flakiness

1f66181

chore: calculate hash at the right time

31e54b9

chore: improve the subprocess tests

44b1b73

chore: make lint

781ade7

chore: no need to ignore Reuse

e975ae9

docs: warn about flakiness in concurrent executions

dd91901

mdelapenya requested a review from a team as a code owner September 4, 2024 16:20

mdelapenya added the breaking change Causing compatibility issues. label Sep 4, 2024

mdelapenya self-assigned this Sep 4, 2024

mdelapenya requested review from eddumelendez and kiview September 4, 2024 16:20

This was referenced Sep 4, 2024

[Bug]: Ryuk might shutdown reused Container while connected to it #2445

Open

[Feature]: Containers with Reuse should not be killed by Ryuk #2654

Open

[Enhancement]: Reuse flag to start exited or paused containers #2470

Open

mdelapenya mentioned this pull request Sep 4, 2024

[Enhancement]: Reuse flag for module Postgres #2726

Open

mdelapenya commented Sep 5, 2024

View reviewed changes

docs: do not use postgres in funcitonal options

799ee15

stevenh requested changes Sep 7, 2024

View reviewed changes

mdelapenya marked this pull request as draft September 10, 2024 10:05

mdelapenya mentioned this pull request Sep 13, 2024

fix(reaper): refactor to allow retries and fix races #2728

Merged

mdelapenya commented Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: more robust Reusable containers experience #2768

feat: more robust Reusable containers experience #2768

mdelapenya commented Sep 4, 2024 •

edited

Loading

netlify bot commented Sep 4, 2024 •

edited

Loading

mdelapenya commented Sep 4, 2024

mdelapenya Sep 5, 2024

kiview Sep 9, 2024

mdelapenya Sep 10, 2024

aaomidi commented Sep 5, 2024

stevenh left a comment

stevenh Sep 7, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

mdelapenya Sep 8, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

stevenh Sep 7, 2024

mdelapenya commented Sep 10, 2024

mdelapenya Sep 24, 2024

		req.Labels[core.LabelContainerHash] = fmt.Sprintf("%d", hash.Hash)
		req.Labels[core.LabelCopiedFilesHash] = fmt.Sprintf("%d", hash.FilesHash)

feat: more robust Reusable containers experience #2768

Are you sure you want to change the base?

feat: more robust Reusable containers experience #2768

Conversation

mdelapenya commented Sep 4, 2024 • edited Loading

What does this PR do?

Why is it important?

Related issues

Docs

Follow-ups

netlify bot commented Sep 4, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

mdelapenya commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaomidi commented Sep 5, 2024

stevenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdelapenya commented Sep 10, 2024

Choose a reason for hiding this comment

mdelapenya commented Sep 4, 2024 •

edited

Loading

netlify bot commented Sep 4, 2024 •

edited

Loading