From 02a5664089b0694fa89417b281b14851b23d9d32 Mon Sep 17 00:00:00 2001 From: Miel Donkers Date: Wed, 25 Jan 2023 17:44:42 +0100 Subject: [PATCH 1/4] Verify Reaper state to create new or return existing instance Fix linting errors --- docker.go | 6 +++--- reaper.go | 37 ++++++++++++++++++++++++++++--------- reaper_test.go | 38 ++++++++++++++++++++++++++++++-------- 3 files changed, 61 insertions(+), 20 deletions(-) diff --git a/docker.go b/docker.go index 0767406a8c..e49af34f69 100644 --- a/docker.go +++ b/docker.go @@ -972,7 +972,7 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque // the reaper does not need to start a reaper for itself isReaperContainer := strings.EqualFold(req.Image, reaperImage(reaperOpts.ImageName)) if !req.SkipReaper && !isReaperContainer { - r, err := newReaper(context.WithValue(ctx, dockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...) + r, err := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...) if err != nil { return nil, fmt.Errorf("%w: creating reaper failed", err) } @@ -1189,7 +1189,7 @@ func (p *DockerProvider) ReuseOrCreateContainer(ctx context.Context, req Contain sessionID := sessionID() var termSignal chan bool if !req.SkipReaper { - r, err := newReaper(context.WithValue(ctx, dockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...) + r, err := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...) if err != nil { return nil, fmt.Errorf("%w: creating reaper failed", err) } @@ -1344,7 +1344,7 @@ func (p *DockerProvider) CreateNetwork(ctx context.Context, req NetworkRequest) var termSignal chan bool if !req.SkipReaper { sessionID := sessionID() - r, err := newReaper(context.WithValue(ctx, dockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...) + r, err := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...) if err != nil { return nil, fmt.Errorf("%w: creating network reaper failed", err) } diff --git a/reaper.go b/reaper.go index c9cce215b6..2878241f02 100644 --- a/reaper.go +++ b/reaper.go @@ -28,7 +28,7 @@ type reaperContextKey string var ( dockerHostContextKey = reaperContextKey("docker_host") - reaper *Reaper // We would like to create reaper only once + reaperSingleton *Reaper // We would like to create reaper only once mutex sync.Mutex ) @@ -42,22 +42,39 @@ type ReaperProvider interface { // NewReaper creates a Reaper with a sessionID to identify containers and a provider to use // Deprecated: it's not possible to create a reaper anymore. func NewReaper(ctx context.Context, sessionID string, provider ReaperProvider, reaperImageName string) (*Reaper, error) { - return newReaper(ctx, sessionID, provider, WithImageName(reaperImageName)) + return reuseOrCreateReaper(ctx, sessionID, provider, WithImageName(reaperImageName)) } -// newReaper creates a Reaper with a sessionID to identify containers and a provider to use -func newReaper(ctx context.Context, sessionID string, provider ReaperProvider, opts ...ContainerOption) (*Reaper, error) { +// reuseOrCreateReaper returns an existing Reaper instance if it exists and is running. Otherwise, a new Reaper instance +// will be created with a sessionID to identify containers and a provider to use +func reuseOrCreateReaper(ctx context.Context, sessionID string, provider ReaperProvider, opts ...ContainerOption) (*Reaper, error) { mutex.Lock() defer mutex.Unlock() - // If reaper already exists re-use it - if reaper != nil { - return reaper, nil + // If reaper already exists and healthy, re-use it + if reaperSingleton != nil { + // Verify this instance is still running by checking state. + // Can't use Container.IsRunning because the bool is not updated when Reaper is terminated + state, err := reaperSingleton.container.State(ctx) + if err == nil && state.Running { + return reaperSingleton, nil + } } + r, err := newReaper(ctx, sessionID, provider, opts...) + if err != nil { + return nil, err + } + + reaperSingleton = r + return reaperSingleton, nil +} + +// newReaper creates a Reaper with a sessionID to identify containers and a provider to use +// Should only be used internally and instead use reuseOrCreateReaper to prefer reusing an existing Reaper instance +func newReaper(ctx context.Context, sessionID string, provider ReaperProvider, opts ...ContainerOption) (*Reaper, error) { dockerHost := extractDockerHost(ctx) - // Otherwise create a new one - reaper = &Reaper{ + reaper := &Reaper{ Provider: provider, SessionID: sessionID, } @@ -108,6 +125,7 @@ func newReaper(ctx context.Context, sessionID string, provider ReaperProvider, o if err != nil { return nil, err } + reaper.container = c endpoint, err := c.PortEndpoint(ctx, "8080", "") if err != nil { @@ -123,6 +141,7 @@ type Reaper struct { Provider ReaperProvider SessionID string Endpoint string + container Container } // Connect runs a goroutine which can be terminated by sending true into the returned channel diff --git a/reaper_test.go b/reaper_test.go index d17d905b79..cfeb23b33b 100644 --- a/reaper_test.go +++ b/reaper_test.go @@ -56,8 +56,6 @@ func createContainerRequest(customize func(ContainerRequest) ContainerRequest) C } func Test_NewReaper(t *testing.T) { - defer func() { reaper = nil }() - type cases struct { name string req ContainerRequest @@ -104,8 +102,6 @@ func Test_NewReaper(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - // make sure we re-initialize the singleton - reaper = nil provider := &mockReaperProvider{ config: test.config, } @@ -124,8 +120,6 @@ func Test_NewReaper(t *testing.T) { } func Test_ExtractDockerHost(t *testing.T) { - defer func() { reaper = nil }() - t.Run("Docker Host as environment variable", func(t *testing.T) { t.Setenv("TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE", "/path/to/docker.sock") host := extractDockerHost(context.Background()) @@ -165,8 +159,6 @@ func Test_ExtractDockerHost(t *testing.T) { } func Test_ReaperForNetwork(t *testing.T) { - defer func() { reaper = nil }() - ctx := context.Background() networkName := "test-network-with-custom-reaper" @@ -193,3 +185,33 @@ func Test_ReaperForNetwork(t *testing.T) { assert.Equal(t, "reaperImage", provider.req.Image) assert.Equal(t, "reaperImage", provider.req.ReaperImage) } + +func Test_ReaperReusedIfHealthy(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test") + } + SkipIfProviderIsNotHealthy(&testing.T{}) + + defer func() { reaperSingleton = nil }() + + ctx := context.Background() + // Make sure there is no existing Reaper instance before running the test + if reaperSingleton != nil { + _ = reaperSingleton.container.Terminate(ctx) + reaperSingleton = nil + } + + provider, _ := ProviderDocker.GetProvider() + reaper, err := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider) + terminateContainerOnEnd(t, ctx, reaper.container) + assert.NoError(t, err, "creating the Reaper should not error") + + reaperReused, _ := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider) + assert.Same(t, reaper, reaperReused, "expecting the same reaper instance is returned if running and healthy") + + terminate, err := reaper.Connect() + defer func(term chan bool) { + term <- true + }(terminate) + assert.NoError(t, err, "connecting to Reaper should be successful") +} From 9a660dcfca50a55ed1fb958095d5864258415c11 Mon Sep 17 00:00:00 2001 From: Miel Donkers Date: Fri, 27 Jan 2023 16:41:26 +0100 Subject: [PATCH 2/4] Play nice with other integration tests --- reaper_test.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/reaper_test.go b/reaper_test.go index cfeb23b33b..30d99729db 100644 --- a/reaper_test.go +++ b/reaper_test.go @@ -192,18 +192,12 @@ func Test_ReaperReusedIfHealthy(t *testing.T) { } SkipIfProviderIsNotHealthy(&testing.T{}) - defer func() { reaperSingleton = nil }() - ctx := context.Background() - // Make sure there is no existing Reaper instance before running the test - if reaperSingleton != nil { - _ = reaperSingleton.container.Terminate(ctx) - reaperSingleton = nil - } + // As other integration tests run with the (shared) Reaper as well, re-use the instance to not interrupt other tests + wasReaperRunning := reaperSingleton != nil provider, _ := ProviderDocker.GetProvider() reaper, err := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider) - terminateContainerOnEnd(t, ctx, reaper.container) assert.NoError(t, err, "creating the Reaper should not error") reaperReused, _ := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider) @@ -214,4 +208,8 @@ func Test_ReaperReusedIfHealthy(t *testing.T) { term <- true }(terminate) assert.NoError(t, err, "connecting to Reaper should be successful") + + if !wasReaperRunning { + terminateContainerOnEnd(t, ctx, reaper.container) + } } From 983e5178dd82304b8f281d344df74507c9632bce Mon Sep 17 00:00:00 2001 From: Miel Donkers Date: Mon, 30 Jan 2023 09:57:11 +0100 Subject: [PATCH 3/4] Rename reaperSingleton to reaperInstance --- reaper.go | 12 ++++++------ reaper_test.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/reaper.go b/reaper.go index 2878241f02..e92e0f82b2 100644 --- a/reaper.go +++ b/reaper.go @@ -28,7 +28,7 @@ type reaperContextKey string var ( dockerHostContextKey = reaperContextKey("docker_host") - reaperSingleton *Reaper // We would like to create reaper only once + reaperInstance *Reaper // We would like to create reaper only once mutex sync.Mutex ) @@ -51,12 +51,12 @@ func reuseOrCreateReaper(ctx context.Context, sessionID string, provider ReaperP mutex.Lock() defer mutex.Unlock() // If reaper already exists and healthy, re-use it - if reaperSingleton != nil { + if reaperInstance != nil { // Verify this instance is still running by checking state. // Can't use Container.IsRunning because the bool is not updated when Reaper is terminated - state, err := reaperSingleton.container.State(ctx) + state, err := reaperInstance.container.State(ctx) if err == nil && state.Running { - return reaperSingleton, nil + return reaperInstance, nil } } @@ -65,8 +65,8 @@ func reuseOrCreateReaper(ctx context.Context, sessionID string, provider ReaperP return nil, err } - reaperSingleton = r - return reaperSingleton, nil + reaperInstance = r + return reaperInstance, nil } // newReaper creates a Reaper with a sessionID to identify containers and a provider to use diff --git a/reaper_test.go b/reaper_test.go index 30d99729db..6c61498866 100644 --- a/reaper_test.go +++ b/reaper_test.go @@ -194,7 +194,7 @@ func Test_ReaperReusedIfHealthy(t *testing.T) { ctx := context.Background() // As other integration tests run with the (shared) Reaper as well, re-use the instance to not interrupt other tests - wasReaperRunning := reaperSingleton != nil + wasReaperRunning := reaperInstance != nil provider, _ := ProviderDocker.GetProvider() reaper, err := reuseOrCreateReaper(context.WithValue(ctx, dockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider) From 82b837232fd351837966b730dae70643b8430725 Mon Sep 17 00:00:00 2001 From: Miel Donkers Date: Tue, 31 Jan 2023 09:39:17 +0100 Subject: [PATCH 4/4] Don't skip for short tests, always run --- reaper_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/reaper_test.go b/reaper_test.go index 6c61498866..98f26fdcd8 100644 --- a/reaper_test.go +++ b/reaper_test.go @@ -187,9 +187,6 @@ func Test_ReaperForNetwork(t *testing.T) { } func Test_ReaperReusedIfHealthy(t *testing.T) { - if testing.Short() { - t.Skip("Skipping integration test") - } SkipIfProviderIsNotHealthy(&testing.T{}) ctx := context.Background()