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

feat(termination)!: make container termination timeout configurable #2926

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 2 additions & 32 deletions cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package testcontainers

import (
"context"
"errors"
"fmt"
"reflect"
"time"
Expand Down Expand Up @@ -54,41 +53,12 @@ func TerminateContainer(container Container, options ...TerminateOption) error {
return nil
}

c := &terminateOptions{
ctx: context.Background(),
}

for _, opt := range options {
opt(c)
}

// TODO: Add a timeout when terminate supports it.
err := container.Terminate(c.ctx)
err := container.Terminate(context.Background(), options...)
stevenh marked this conversation as resolved.
Show resolved Hide resolved
if !isCleanupSafe(err) {
return fmt.Errorf("terminate: %w", err)
}

// Remove additional volumes if any.
if len(c.volumes) == 0 {
return nil
}

client, err := NewDockerClientWithOpts(c.ctx)
if err != nil {
return fmt.Errorf("docker client: %w", err)
}

defer client.Close()

// Best effort to remove all volumes.
var errs []error
for _, volume := range c.volumes {
if errRemove := client.VolumeRemove(c.ctx, volume, true); errRemove != nil {
errs = append(errs, fmt.Errorf("volume remove %q: %w", volume, errRemove))
}
}

return errors.Join(errs...)
return nil
}

// isNil returns true if val is nil or an nil instance false otherwise.
Expand Down
2 changes: 1 addition & 1 deletion container.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ type Container interface {
Stop(context.Context, *time.Duration) error // stop the container

// Terminate stops and removes the container and its image if it was built and not flagged as kept.
Terminate(ctx context.Context) error
Terminate(ctx context.Context, opts ...TerminateOption) error
moogacs marked this conversation as resolved.
Show resolved Hide resolved

Logs(context.Context) (io.ReadCloser, error) // Get logs of the container
FollowOutput(LogConsumer) // Deprecated: it will be removed in the next major release
Expand Down
53 changes: 47 additions & 6 deletions docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,19 +296,41 @@ func (c *DockerContainer) Stop(ctx context.Context, timeout *time.Duration) erro
return nil
}

// WithTerminateTimeout is a functional option that sets the timeout for the container termination.
func WithTerminateTimeout(timeout time.Duration) TerminateOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

bug: this is actually the stop timeout and these methods aren't docker specific so should really be moved to cleanup.go.

return func(c *terminateOptions) {
c.timeout = &timeout
}
}

// WithTerminateVolumes is a functional option that sets the volumes for the container termination.
func WithTerminateVolumes(volumes ...string) TerminateOption {
return func(opts *terminateOptions) {
opts.volumes = volumes
}
}

// Terminate calls stops and then removes the container including its volumes.
// If its image was built it and all child images are also removed unless
// the [FromDockerfile.KeepImage] on the [ContainerRequest] was set to true.
//
// The following hooks are called in order:
// - [ContainerLifecycleHooks.PreTerminates]
// - [ContainerLifecycleHooks.PostTerminates]
func (c *DockerContainer) Terminate(ctx context.Context) error {
// ContainerRemove hardcodes stop timeout to 3 seconds which is too short
// to ensure that child containers are stopped so we manually call stop.
// TODO: make this configurable via a functional option.
timeout := 10 * time.Second
err := c.Stop(ctx, &timeout)
//
// Default: timeout is 10 seconds.
func (c *DockerContainer) Terminate(ctx context.Context, opts ...TerminateOption) error {
defaultTimeout := 10 * time.Second
defaultOptions := &terminateOptions{
moogacs marked this conversation as resolved.
Show resolved Hide resolved
timeout: &defaultTimeout,
ctx: ctx,
}

for _, opt := range opts {
opt(defaultOptions)
}

err := c.Stop(defaultOptions.ctx, defaultOptions.timeout)
if err != nil && !isCleanupSafe(err) {
return fmt.Errorf("stop: %w", err)
}
Expand Down Expand Up @@ -343,6 +365,25 @@ func (c *DockerContainer) Terminate(ctx context.Context) error {
c.sessionID = ""
c.isRunning = false

// Remove additional volumes if any.
moogacs marked this conversation as resolved.
Show resolved Hide resolved
if len(defaultOptions.volumes) == 0 {
return nil
}

client, err := NewDockerClientWithOpts(ctx)
if err != nil {
return fmt.Errorf("docker client: %w", err)
}

defer client.Close()

// Best effort to remove all volumes.
for _, volume := range defaultOptions.volumes {
if errRemove := client.VolumeRemove(ctx, volume, true); errRemove != nil {
errs = append(errs, fmt.Errorf("volume remove %q: %w", volume, errRemove))
}
}

return errors.Join(errs...)
}

Expand Down
71 changes: 71 additions & 0 deletions docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,27 @@ func TestContainerStateAfterTermination(t *testing.T) {
require.Nil(t, state, "expected nil container inspect.")
})

t.Run("Nil State after termination with timeout", func(t *testing.T) {
moogacs marked this conversation as resolved.
Show resolved Hide resolved
ctx := context.Background()
nginx, err := createContainerFn(ctx)
require.NoError(t, err)

err = nginx.Start(ctx)
require.NoError(t, err, "expected no error from container start.")

state, err := nginx.State(ctx)
require.NoError(t, err, "expected no error from container inspect.")
require.NotNil(t, state, "expected non nil container inspect.")
moogacs marked this conversation as resolved.
Show resolved Hide resolved
require.True(t, state.Running, "expected container state to be running.")

err = nginx.Terminate(ctx, WithTerminateTimeout(5*time.Microsecond))
require.NoError(t, err)

state, err = nginx.State(ctx)
require.Error(t, err, "expected error from container inspect.")
require.Nil(t, state, "expected nil container inspect.")
})

t.Run("Nil State after termination if raw as already set", func(t *testing.T) {
ctx := context.Background()
nginx, err := createContainerFn(ctx)
Expand Down Expand Up @@ -1087,6 +1108,56 @@ func TestContainerCreationWithVolumeAndFileWritingToIt(t *testing.T) {
require.NoError(t, err)
}

func TestContainerCreationWithVolumeCleaning(t *testing.T) {
absPath, err := filepath.Abs(filepath.Join(".", "testdata", "hello.sh"))
require.NoError(t, err)
stevenh marked this conversation as resolved.
Show resolved Hide resolved
ctx, cnl := context.WithTimeout(context.Background(), 30*time.Second)
defer cnl()

// Create the volume.
volumeName := "volumeName"

// Create the container that writes into the mounted volume.
bashC, err := GenericContainer(ctx, GenericContainerRequest{
ProviderType: providerType,
ContainerRequest: ContainerRequest{
Image: "bash:5.2.26",
Files: []ContainerFile{
{
HostFilePath: absPath,
ContainerFilePath: "/hello.sh",
moogacs marked this conversation as resolved.
Show resolved Hide resolved
},
},
Mounts: Mounts(VolumeMount(volumeName, "/data")),
Cmd: []string{"bash", "/hello.sh"},
WaitingFor: wait.ForLog("done"),
},
Started: true,
})
require.NoError(t, err)
moogacs marked this conversation as resolved.
Show resolved Hide resolved
err = bashC.Terminate(ctx, WithTerminateVolumes(volumeName))
require.NoError(t, err)
}

func TestContainerTerminationOptions(t *testing.T) {
volumeName := "volumeName"
definedVolumeOpt := &terminateOptions{}
volumeOpt := WithTerminateVolumes(volumeName)
volumeOpt(definedVolumeOpt)
require.Equal(t, definedVolumeOpt.volumes, []string{volumeName})

defaultTimeout := 10 * time.Second
definedTimeoutOpt := &terminateOptions{
timeout: &defaultTimeout,
}

configuredTimeout := 1 * time.Second

timeoutOpt := WithTerminateTimeout(1 * time.Second)
timeoutOpt(definedTimeoutOpt)
require.Equal(t, *definedTimeoutOpt.timeout, configuredTimeout)
}

func TestContainerWithTmpFs(t *testing.T) {
ctx := context.Background()
req := ContainerRequest{
Expand Down
4 changes: 2 additions & 2 deletions modules/etcd/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ type EtcdContainer struct {

// Terminate terminates the etcd container, its child nodes, and the network in which the cluster is running
// to communicate between the nodes.
func (c *EtcdContainer) Terminate(ctx context.Context) error {
func (c *EtcdContainer) Terminate(ctx context.Context, opts ...testcontainers.TerminateOption) error {
var errs []error

// child nodes has no other children
for i, child := range c.childNodes {
if err := child.Terminate(ctx); err != nil {
if err := child.Terminate(ctx, opts...); err != nil {
moogacs marked this conversation as resolved.
Show resolved Hide resolved
errs = append(errs, fmt.Errorf("terminate child node(%d): %w", i, err))
}
}
Expand Down
4 changes: 2 additions & 2 deletions port_forwarding.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,10 @@ type sshdContainer struct {
}

// Terminate stops the container and closes the SSH session
func (sshdC *sshdContainer) Terminate(ctx context.Context) error {
func (sshdC *sshdContainer) Terminate(ctx context.Context, opts ...TerminateOption) error {
return errors.Join(
sshdC.closePorts(),
sshdC.Container.Terminate(ctx),
sshdC.Container.Terminate(ctx, opts...),
)
}

Expand Down
Loading