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: Recreate Ryuk container if terminated #2084

Merged

Conversation

Mathew-Estafanous
Copy link
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
Copy link

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.

reaper_test.go Outdated Show resolved Hide resolved
reaper_test.go Outdated Show resolved Hide resolved
reaper_test.go Outdated Show resolved Hide resolved
reaper_test.go Outdated Show resolved Hide resolved
reaper_test.go Outdated Show resolved Hide resolved
Copy link
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
reaper.go Outdated Show resolved Hide resolved
Copy link
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
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