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 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
48 changes: 9 additions & 39 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 All @@ -16,21 +15,21 @@ type terminateOptions struct {
}

// TerminateOption is a type that represents an option for terminating a container.
type TerminateOption func(*terminateOptions)
type TerminateOption func(*DockerContainer)
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 ties the container implementation to the concrete DockerContainer type, which would prevent other types implementing Container for example the incoming ollama change.

We could maintain the dedicated type but export it and the fields, which could then be embedded in others.

The challenge with that is you can't rely on the interface to ensure an implementation is complete so it may be better to bite the bullet and make them concrete options @mdelapenya thoughts?

Copy link
Author

@moogacs moogacs Dec 20, 2024

Choose a reason for hiding this comment

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

yeah, i had similar feeling, but wasn't sure if shall i introduce a separate type, when i was opening this PR i tried to make as minimal breaking change but seems it has to be. open for suggestions

i like the idea of maintain the dedicated type but export it and the fields

Copy link
Collaborator

Choose a reason for hiding this comment

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

I initially thought the dedicated type would work, but that wont ensure that all Container implementations support the options, which I'm not a fan of as a consumer would be left wondering why their options didn't work.

Copy link
Author

Choose a reason for hiding this comment

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

the other thought i had before i come to this was introducing it in the ContainerRequest and add getter in the container interface

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought about setters too, but I think that would make it much harder to discover for users.

I can't think of any perfect solution ATM, something mull over.

Copy link
Author

Choose a reason for hiding this comment

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

i have made a change and revert it to make sure it uses terminateOptions instead of the dependency on DockerContainer type.

also added more test to show case the usage and the changes of configuration

defaults of volumes is nil, timeout is 10 and context is passed already on Terminate() , lmk wdyt ?


// StopContext returns a TerminateOption that sets the context.
// Default: context.Background().
func StopContext(ctx context.Context) TerminateOption {
return func(c *terminateOptions) {
c.ctx = ctx
return func(c *DockerContainer) {
c.terminationOptions.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 *DockerContainer) {
c.terminationOptions.timeout = &timeout
}
}

Expand All @@ -39,8 +38,8 @@ func StopTimeout(timeout time.Duration) TerminateOption {
// which are not removed by default.
// Default: nil.
func RemoveVolumes(volumes ...string) TerminateOption {
return func(c *terminateOptions) {
c.volumes = volumes
return func(c *DockerContainer) {
c.terminationOptions.volumes = volumes
}
}

Expand All @@ -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
46 changes: 40 additions & 6 deletions docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ type DockerContainer struct {
logProductionCtx context.Context

logProductionTimeout *time.Duration
terminationOptions terminateOptions
logger Logging
lifecycleHooks []ContainerLifecycleHooks

Expand Down Expand Up @@ -296,19 +297,33 @@ 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 {
moogacs marked this conversation as resolved.
Show resolved Hide resolved
return func(c *DockerContainer) {
c.terminationOptions.timeout = &timeout
}
}

// 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 {
if len(opts) == 0 {
moogacs marked this conversation as resolved.
Show resolved Hide resolved
d := 10 * time.Second
c.terminationOptions.timeout = &d
}

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

err := c.Stop(ctx, c.terminationOptions.timeout)
if err != nil && !isCleanupSafe(err) {
return fmt.Errorf("stop: %w", err)
}
Expand Down Expand Up @@ -343,6 +358,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(c.terminationOptions.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 c.terminationOptions.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
21 changes: 21 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
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