Skip to content

fix: Recreate Ryuk container if terminated#2084

Merged
mdelapenya merged 5 commits intotestcontainers:mainfrom
Mathew-Estafanous:me/recreate-terminated-reaper
Jan 12, 2024
Merged

fix: Recreate Ryuk container if terminated#2084
mdelapenya merged 5 commits intotestcontainers:mainfrom
Mathew-Estafanous:me/recreate-terminated-reaper

Conversation

@Mathew-Estafanous
Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes ryuk recreation such that a new container is spun up if the previous container has terminated and been removed. This involves checking for ErrNotFound during the Ryuk state check, triggering the creation of a new ryuk container if this error is returned.

It is essential to reset reaperOnce. Without resetting, no attempt to create a new reaper will be made.

Why is it important?

  • Test suites that exceed the ryuk timeout fail because a new container is not created.
  • Relying on clients to extend timeout as a workaround adds unnecessary work for end users.

Related issues

How to test this PR

Run test Test_RecreateReaperIfTerminated to validate changes.

Or run the following test suite with env variable SLEEP=11s. Without changes made in this PR TestC would fail, but with them all tests pass.

package foo

import (
	"context"
	"os"
	"testing"
	"time"

	"github.com/testcontainers/testcontainers-go/modules/redpanda"
)

func TestA(t *testing.T) {
	ctx := context.Background()
	tc, err := redpanda.RunContainer(ctx)
	if err != nil {
		t.Fatal(err)
	}
	t.Cleanup(func() { tc.Terminate(ctx) })
}

func TestB(t *testing.T) {
	d := os.Getenv("SLEEP")
	if d == "" {
		return
	}
	dur, err := time.ParseDuration(d)
	if err != nil {
		t.Fatal(err)
	}
	time.Sleep(dur)
}

func TestC(t *testing.T) {
	ctx := context.Background()
	tc, err := redpanda.RunContainer(ctx)
	if err != nil {
		t.Fatal(err)
	}
	t.Cleanup(func() { tc.Terminate(ctx) })
}

@Mathew-Estafanous Mathew-Estafanous requested a review from a team as a code owner January 9, 2024 16:01
@netlify
Copy link
Copy Markdown

netlify bot commented Jan 9, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 42f1f29
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65a072573a9c9a00085b1e4f
😎 Deploy Preview https://deploy-preview-2084--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link
Copy Markdown
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments, once resolved, I think we are good to go, Thanks!

@mdelapenya mdelapenya self-assigned this Jan 9, 2024
@mdelapenya mdelapenya added the bug An issue with the library label Jan 9, 2024
@Mathew-Estafanous Mathew-Estafanous force-pushed the me/recreate-terminated-reaper branch from 964b47b to 605ca7b Compare January 10, 2024 16:02
Copy link
Copy Markdown
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@Mathew-Estafanous
Copy link
Copy Markdown
Contributor Author

Great. Could you run the test suite one more time for me? I had to update the branch. Thanks!

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]: ryuk restart/create after first instance has timed out fails

2 participants