feat(termination)!: make container termination timeout configurable (t…

…estcontainers#2926) * feat(termination): make container termination timeout configurable * revert TerminateOption to terminateOptions, seperate from DockerContainer and introduce configuration test * rename to defaultOptions and pass ctx * address comments * resolve more suggestions * don't export terminateOptions * address more comments * avoid global default * address more comments * add docs for new termination options * move docs to garbage_collector/#terminate-function * resolve doc comments * docs: typo --------- Co-authored-by: Manuel de la Peña <[email protected]>
mdelapenya · Jan 2, 2025 · 6f718ee · 6f718ee
1 parent eb5b8ed
commit 6f718ee
Show file tree

Hide file tree

Showing 7 changed files with 188 additions and 53 deletions.
diff --git a/cleanup.go b/cleanup.go
@@ -8,29 +8,74 @@ import (
 	"time"
 )
 
-// terminateOptions is a type that holds the options for terminating a container.
-type terminateOptions struct {
-	ctx     context.Context
-	timeout *time.Duration
-	volumes []string
+// TerminateOptions is a type that holds the options for terminating a container.
+type TerminateOptions struct {
+	ctx         context.Context
+	stopTimeout *time.Duration
+	volumes     []string
 }
 
 // TerminateOption is a type that represents an option for terminating a container.
-type TerminateOption func(*terminateOptions)
+type TerminateOption func(*TerminateOptions)
+
+// NewTerminateOptions returns a fully initialised TerminateOptions.
+// Defaults: StopTimeout: 10 seconds.
+func NewTerminateOptions(ctx context.Context, opts ...TerminateOption) *TerminateOptions {
+	timeout := time.Second * 10
+	options := &TerminateOptions{
+		stopTimeout: &timeout,
+		ctx:         ctx,
+	}
+	for _, opt := range opts {
+		opt(options)
+	}
+	return options
+}
+
+// Context returns the context to use during a Terminate.
+func (o *TerminateOptions) Context() context.Context {
+	return o.ctx
+}
+
+// StopTimeout returns the stop timeout to use during a Terminate.
+func (o *TerminateOptions) StopTimeout() *time.Duration {
+	return o.stopTimeout
+}
+
+// Cleanup performs any clean up needed
+func (o *TerminateOptions) Cleanup() error {
+	// TODO: simplify this when when perform the client refactor.
+	if len(o.volumes) == 0 {
+		return nil
+	}
+	client, err := NewDockerClientWithOpts(o.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 o.volumes {
+		if errRemove := client.VolumeRemove(o.ctx, volume, true); errRemove != nil {
+			errs = append(errs, fmt.Errorf("volume remove %q: %w", volume, errRemove))
+		}
+	}
+	return errors.Join(errs...)
+}
 
 // StopContext returns a TerminateOption that sets the context.
 // Default: context.Background().
 func StopContext(ctx context.Context) TerminateOption {
-	return func(c *terminateOptions) {
+	return func(c *TerminateOptions) {
 		c.ctx = ctx
 	}
 }
 
 // StopTimeout returns a TerminateOption that sets the timeout.
 // Default: See [Container.Stop].
 func StopTimeout(timeout time.Duration) TerminateOption {
-	return func(c *terminateOptions) {
-		c.timeout = &timeout
+	return func(c *TerminateOptions) {
+		c.stopTimeout = &timeout
 	}
 }
 
@@ -39,7 +84,7 @@ func StopTimeout(timeout time.Duration) TerminateOption {
 // which are not removed by default.
 // Default: nil.
 func RemoveVolumes(volumes ...string) TerminateOption {
-	return func(c *terminateOptions) {
+	return func(c *TerminateOptions) {
 		c.volumes = volumes
 	}
 }
@@ -54,41 +99,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...)
 	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.

diff --git a/container.go b/container.go
@@ -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
 
 	Logs(context.Context) (io.ReadCloser, error)                    // Get logs of the container
 	FollowOutput(LogConsumer)                                       // Deprecated: it will be removed in the next major release

diff --git a/docker.go b/docker.go
@@ -303,12 +303,11 @@ func (c *DockerContainer) Stop(ctx context.Context, timeout *time.Duration) erro
 // 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 {
+	options := NewTerminateOptions(ctx, opts...)
+	err := c.Stop(options.Context(), options.StopTimeout())
 	if err != nil && !isCleanupSafe(err) {
 		return fmt.Errorf("stop: %w", err)
 	}
@@ -343,6 +342,10 @@ func (c *DockerContainer) Terminate(ctx context.Context) error {
 	c.sessionID = ""
 	c.isRunning = false
 
+	if err = options.Cleanup(); err != nil {
+		errs = append(errs, err)
+	}
+
 	return errors.Join(errs...)
 }
 

diff --git a/docker_test.go b/docker_test.go
@@ -281,6 +281,18 @@ func TestContainerStateAfterTermination(t *testing.T) {
 		require.Nil(t, state, "expected nil container inspect.")
 	})
 
+	t.Run("termination-timeout", func(t *testing.T) {
+		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.")
+
+		err = nginx.Terminate(ctx, StopTimeout(5*time.Microsecond))
+		require.NoError(t, err)
+	})
+
 	t.Run("Nil State after termination if raw as already set", func(t *testing.T) {
 		ctx := context.Background()
 		nginx, err := createContainerFn(ctx)
@@ -1077,6 +1089,38 @@ func TestContainerCreationWithVolumeAndFileWritingToIt(t *testing.T) {
 				{
 					HostFilePath:      absPath,
 					ContainerFilePath: "/hello.sh",
+					FileMode:          700,
+				},
+			},
+			Mounts:     Mounts(VolumeMount(volumeName, "/data")),
+			Cmd:        []string{"bash", "/hello.sh"},
+			WaitingFor: wait.ForLog("done"),
+		},
+		Started: true,
+	})
+	CleanupContainer(t, bashC, RemoveVolumes(volumeName))
+	require.NoError(t, err)
+}
+
+func TestContainerCreationWithVolumeCleaning(t *testing.T) {
+	absPath, err := filepath.Abs(filepath.Join(".", "testdata", "hello.sh"))
+	require.NoError(t, err)
+	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",
+					FileMode:          700,
 				},
 			},
 			Mounts:     Mounts(VolumeMount(volumeName, "/data")),
@@ -1085,10 +1129,41 @@ func TestContainerCreationWithVolumeAndFileWritingToIt(t *testing.T) {
 		},
 		Started: true,
 	})
+	require.NoError(t, err)
+	err = bashC.Terminate(ctx, RemoveVolumes(volumeName))
 	CleanupContainer(t, bashC, RemoveVolumes(volumeName))
 	require.NoError(t, err)
 }
 
+func TestContainerTerminationOptions(t *testing.T) {
+	t.Run("volumes", func(t *testing.T) {
+		var options TerminateOptions
+		RemoveVolumes("vol1", "vol2")(&options)
+		require.Equal(t, TerminateOptions{
+			volumes: []string{"vol1", "vol2"},
+		}, options)
+	})
+	t.Run("stop-timeout", func(t *testing.T) {
+		var options TerminateOptions
+		timeout := 11 * time.Second
+		StopTimeout(timeout)(&options)
+		require.Equal(t, TerminateOptions{
+			stopTimeout: &timeout,
+		}, options)
+	})
+
+	t.Run("all", func(t *testing.T) {
+		var options TerminateOptions
+		timeout := 9 * time.Second
+		StopTimeout(timeout)(&options)
+		RemoveVolumes("vol1", "vol2")(&options)
+		require.Equal(t, TerminateOptions{
+			stopTimeout: &timeout,
+			volumes:     []string{"vol1", "vol2"},
+		}, options)
+	})
+}
+
 func TestContainerWithTmpFs(t *testing.T) {
 	ctx := context.Background()
 	req := ContainerRequest{

diff --git a/docs/features/garbage_collector.md b/docs/features/garbage_collector.md
@@ -17,6 +17,47 @@ The primary method is to use the `Terminate(context.Context)` function that is
 available when a container is created. Use `defer` to ensure that it is called
 on test completion.
 
+The `Terminate` function can be customised with termination options to determine how a container is removed: termination timeout, and the ability to remove container volumes are supported at the moment. You can build the default options using the `testcontainers.NewTerminationOptions` function.
+
+#### NewTerminateOptions
+
+- Not available until the next release of testcontainers-go <a href="https://github.com/testcontainers/testcontainers-go"><span class="tc-version">:material-tag: main</span></a>
+
+If you want to attach option to container termination, you can use the `testcontainers.NewTerminateOptions(ctx context.Context, opts ...TerminateOption) *TerminateOptions` option, which receives a TerminateOption as parameter, creating custom termination options to be passed on the container termination.
+
+##### Terminate Options
+
+###### [StopContext](../../cleanup.go)
+Sets the context for the Container termination.
+
+- **Function**: `StopContext(ctx context.Context) TerminateOption`
+- **Default**: The context passed in `Terminate()`
+- **Usage**:
+```go
+err := container.Terminate(ctx,StopContext(context.Background()))
+```
+
+###### [StopTimeout](../../cleanup.go)
+Sets the timeout for stopping the Container.
+
+- **Function**: ` StopTimeout(timeout time.Duration) TerminateOption`
+- **Default**:  10 seconds
+- **Usage**:
+```go
+err := container.Terminate(ctx, StopTimeout(20 * time.Second))
+```
+
+###### [RemoveVolumes](../../cleanup.go)
+Sets the volumes to be removed during Container termination.
+
+- **Function**: ` RemoveVolumes(volumes ...string) TerminateOption`
+- **Default**:  Empty (no volumes removed)
+- **Usage**:
+```go
+err := container.Terminate(ctx, RemoveVolumes("vol1", "vol2"))
+```
+
+
 !!!tip
 
     Remember to `defer` as soon as possible so you won't forget. The best time

diff --git a/modules/etcd/etcd.go b/modules/etcd/etcd.go
@@ -29,18 +29,18 @@ 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 {
 			errs = append(errs, fmt.Errorf("terminate child node(%d): %w", i, err))
 		}
 	}
 
 	if c.Container != nil {
-		if err := c.Container.Terminate(ctx); err != nil {
+		if err := c.Container.Terminate(ctx, opts...); err != nil {
 			errs = append(errs, fmt.Errorf("terminate cluster node: %w", err))
 		}
 	}

diff --git a/port_forwarding.go b/port_forwarding.go
@@ -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...),
 	)
 }