From e9c4c54fb9ec14401249348fa4c20d7603d9d128 Mon Sep 17 00:00:00 2001 From: Mathew-Estafanous Date: Thu, 28 Dec 2023 15:38:40 -0500 Subject: [PATCH 1/4] fix: recreate terminated repear container when needed. --- reaper.go | 14 +++++++++----- reaper_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/reaper.go b/reaper.go index b618ad3854..f9e7140465 100644 --- a/reaper.go +++ b/reaper.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "fmt" + "github.com/docker/docker/errdefs" "math/rand" "net" "regexp" @@ -141,13 +142,16 @@ func reuseOrCreateReaper(ctx context.Context, sessionID string, provider ReaperP // Can't use Container.IsRunning because the bool is not updated when Reaper is terminated state, err := reaperInstance.container.State(ctx) if err != nil { - return nil, err - } - - if state.Running { - return reaperInstance, nil + if !errdefs.IsNotFound(err) { + return nil, err + } + } else { + if state.Running { + return reaperInstance, nil + } } // else: the reaper instance has been terminated, so we need to create a new one + reaperOnce = sync.Once{} } // 2. because the reaper instance has not been created yet, look for it in the Docker daemon, which diff --git a/reaper_test.go b/reaper_test.go index d6b9cfe8f9..7471766ba5 100644 --- a/reaper_test.go +++ b/reaper_test.go @@ -491,6 +491,42 @@ func Test_ReaperReusedIfHealthy(t *testing.T) { } } +func Test_RecreateReaperIfTerminated(t *testing.T) { + config.Reset() // reset the config using the internal method to avoid the sync.Once + tcConfig := config.Read() + if tcConfig.RyukDisabled { + t.Skip("Ryuk is disabled, skipping test") + } + + mockProvider := newMockReaperProvider(t) + t.Cleanup(mockProvider.RestoreReaperState) + + SkipIfProviderIsNotHealthy(&testing.T{}) + + provider, _ := ProviderDocker.GetProvider() + ctx := context.Background() + reaper, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, provider.(*DockerProvider).host), testSessionID, provider) + assert.NoError(t, err, "creating the Reaper should not error") + + terminate, err := reaper.Connect() + assert.NoError(t, err, "connecting to Reaper should be successful") + terminate <- true + + // wait for reaper to timeout and terminate + time.Sleep(11 * time.Second) + + recreatedRepear, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, provider.(*DockerProvider).host), testSessionID, provider) + assert.NoError(t, err, "creating the Reaper should not error") + assert.NotEqual(t, reaper.container.GetContainerID(), recreatedRepear.container.GetContainerID(), "expected different container ID") + + terminate, err = recreatedRepear.Connect() + defer func(term chan bool) { + term <- true + }(terminate) + assert.NoError(t, err, "connecting to Reaper should be successful") + terminateContainerOnEnd(t, ctx, recreatedRepear.container) +} + func TestReaper_reuseItFromOtherTestProgramUsingDocker(t *testing.T) { config.Reset() // reset the config using the internal method to avoid the sync.Once tcConfig := config.Read() From d50621caeb79e6075a7b597caabbf83ab0208d6b Mon Sep 17 00:00:00 2001 From: Mathew-Estafanous Date: Tue, 9 Jan 2024 12:54:43 -0500 Subject: [PATCH 2/4] chore: rename variable name in test --- reaper_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/reaper_test.go b/reaper_test.go index 7471766ba5..6ad153092b 100644 --- a/reaper_test.go +++ b/reaper_test.go @@ -505,26 +505,26 @@ func Test_RecreateReaperIfTerminated(t *testing.T) { provider, _ := ProviderDocker.GetProvider() ctx := context.Background() - reaper, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, provider.(*DockerProvider).host), testSessionID, provider) + reaper, err := reuseOrCreateReaper(context.WithValue(ctx, core.DockerHostContextKey, provider.(*DockerProvider).host), testSessionID, provider) assert.NoError(t, err, "creating the Reaper should not error") terminate, err := reaper.Connect() assert.NoError(t, err, "connecting to Reaper should be successful") terminate <- true - // wait for reaper to timeout and terminate + // Wait for ryuk's default timeout (10s) + 1s to allow for a graceful shutdown/cleanup of the container. time.Sleep(11 * time.Second) - recreatedRepear, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, provider.(*DockerProvider).host), testSessionID, provider) + recreatedReaper, err := reuseOrCreateReaper(context.WithValue(ctx, core.DockerHostContextKey, provider.(*DockerProvider).host), testSessionID, provider) assert.NoError(t, err, "creating the Reaper should not error") - assert.NotEqual(t, reaper.container.GetContainerID(), recreatedRepear.container.GetContainerID(), "expected different container ID") + assert.NotEqual(t, reaper.container.GetContainerID(), recreatedReaper.container.GetContainerID(), "expected different container ID") - terminate, err = recreatedRepear.Connect() + terminate, err = recreatedReaper.Connect() defer func(term chan bool) { term <- true }(terminate) assert.NoError(t, err, "connecting to Reaper should be successful") - terminateContainerOnEnd(t, ctx, recreatedRepear.container) + terminateContainerOnEnd(t, ctx, recreatedReaper.container) } func TestReaper_reuseItFromOtherTestProgramUsingDocker(t *testing.T) { From ed6417a2ae6dbd353f8734ad4aaa403eb0ac4919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20de=20la=20Pe=C3=B1a?= Date: Thu, 11 Jan 2024 16:12:29 +0100 Subject: [PATCH 3/4] chore: run make lint --- reaper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reaper.go b/reaper.go index f9e7140465..676d0188b9 100644 --- a/reaper.go +++ b/reaper.go @@ -4,7 +4,6 @@ import ( "bufio" "context" "fmt" - "github.com/docker/docker/errdefs" "math/rand" "net" "regexp" @@ -16,6 +15,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/filters" + "github.com/docker/docker/errdefs" "github.com/docker/go-connections/nat" "github.com/testcontainers/testcontainers-go/internal/config" From 8594915bce8d6394b8bea8165e13a209893e014a Mon Sep 17 00:00:00 2001 From: Mathew-Estafanous Date: Thu, 11 Jan 2024 12:06:10 -0500 Subject: [PATCH 4/4] chore: simplify with else if --- reaper.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/reaper.go b/reaper.go index 676d0188b9..1d269ce899 100644 --- a/reaper.go +++ b/reaper.go @@ -145,10 +145,8 @@ func reuseOrCreateReaper(ctx context.Context, sessionID string, provider ReaperP if !errdefs.IsNotFound(err) { return nil, err } - } else { - if state.Running { - return reaperInstance, nil - } + } else if state.Running { + return reaperInstance, nil } // else: the reaper instance has been terminated, so we need to create a new one reaperOnce = sync.Once{}