From 2c9010f393ff20cc8a837bcea003c5f605c0c42b Mon Sep 17 00:00:00 2001 From: Erdem Toraman Date: Sun, 16 Feb 2020 15:23:13 +0200 Subject: [PATCH 1/3] #153 propagate ctx cancellation to image pull and not found errors are permanent no need to retry --- docker.go | 40 ++++++++++++++++++++++++++-------------- docker_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/docker.go b/docker.go index de6ac93e26..eb823db955 100644 --- a/docker.go +++ b/docker.go @@ -5,6 +5,8 @@ import ( "context" "encoding/binary" "fmt" + "github.com/cenkalti/backoff" + "github.com/docker/docker/errdefs" "io" "io/ioutil" "log" @@ -14,7 +16,6 @@ import ( "strings" "time" - "github.com/cenkalti/backoff" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/mount" @@ -500,22 +501,11 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque if req.RegistryCred != "" { pullOpt.RegistryAuth = req.RegistryCred } - var pull io.ReadCloser - err := backoff.Retry(func() error { - var err error - pull, err = p.client.ImagePull(ctx, tag, pullOpt) - return err - }, backoff.NewExponentialBackOff()) - if err != nil { - return nil, err - } - defer pull.Close() - // download of docker image finishes at EOF of the pull request - _, err = ioutil.ReadAll(pull) - if err != nil { + if err := p.attemptToPullImage(ctx, tag, pullOpt); err != nil { return nil, err } + } else { return nil, err } @@ -592,6 +582,28 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque return c, nil } +func (p *DockerProvider) attemptToPullImage(ctx context.Context, tag string, pullOpt types.ImagePullOptions) error { + var ( + err error + pull io.ReadCloser + ) + err = backoff.Retry(func() error { + pull, err = p.client.ImagePull(ctx, tag, pullOpt) + if _, ok := err.(errdefs.ErrNotFound); ok { + return backoff.Permanent(err) + } + return err + }, backoff.WithContext(backoff.NewExponentialBackOff(), ctx)) + if err != nil { + return err + } + defer pull.Close() + + // download of docker image finishes at EOF of the pull request + _, err = ioutil.ReadAll(pull) + return err +} + // RunContainer takes a RequestContainer as input and it runs a container via the docker sdk func (p *DockerProvider) RunContainer(ctx context.Context, req ContainerRequest) (Container, error) { c, err := p.CreateContainer(ctx, req) diff --git a/docker_test.go b/docker_test.go index 49ac840642..b298f9e2af 100644 --- a/docker_test.go +++ b/docker_test.go @@ -2,7 +2,9 @@ package testcontainers import ( "context" + "errors" "fmt" + "github.com/docker/docker/errdefs" "net/http" "path/filepath" "testing" @@ -1180,3 +1182,34 @@ func TestContainerWithTmpFs(t *testing.T) { t.Fatalf("File %s should exist, expected return code 0, got %v", path, c) } } + +func TestContainerNonExistentImage(t *testing.T) { + t.Run("if the image not found don't propagate the error", func(t *testing.T) { + _, err := GenericContainer(context.Background(), GenericContainerRequest{ + ContainerRequest: ContainerRequest{ + Image: "postgres:nonexistent-version", + }, + Started: true, + }) + + var nf errdefs.ErrNotFound + if !errors.As(err, &nf) { + t.Fatalf("the error should have bee an errdefs.ErrNotFound: %v", err) + } + }) + + t.Run("the context cancellation is propagated to container creation", func(t *testing.T) { + ctx, _ := context.WithTimeout(context.Background(), time.Millisecond*100) + _, err := GenericContainer(ctx, GenericContainerRequest{ + ContainerRequest: ContainerRequest{ + Image: "postgres:latest", + WaitingFor: wait.ForLog("log"), + }, + Started: true, + }) + if !errors.Is(err, ctx.Err()) { + t.Fatalf("err should be a ctx cancelled error %v", err) + } + }) + +} From c1c32769430bae4f0517a7e7b4c60e1794151dcf Mon Sep 17 00:00:00 2001 From: Erdem Toraman Date: Sun, 16 Feb 2020 15:26:34 +0200 Subject: [PATCH 2/3] #153 add a comment --- docker.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docker.go b/docker.go index eb823db955..38256366d0 100644 --- a/docker.go +++ b/docker.go @@ -582,6 +582,8 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque return c, nil } +//attemptToPullImage tries to pull the image while respecting the ctx cancellations. +// Besides, if the image cannot be pulled due to ErrorNotFound then no need to retry but terminate immediately. func (p *DockerProvider) attemptToPullImage(ctx context.Context, tag string, pullOpt types.ImagePullOptions) error { var ( err error From 5279bbbeb696ccd27a0031ac9da866c976de0add Mon Sep 17 00:00:00 2001 From: Erdem Toraman Date: Mon, 17 Feb 2020 17:04:16 +0200 Subject: [PATCH 3/3] Increase context timeout and manage cancellation for TestContainerNonExistentImage Signed-off-by: Gianluca Arbezzano --- docker_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docker_test.go b/docker_test.go index b298f9e2af..afdde1eeb5 100644 --- a/docker_test.go +++ b/docker_test.go @@ -4,12 +4,13 @@ import ( "context" "errors" "fmt" - "github.com/docker/docker/errdefs" "net/http" "path/filepath" "testing" "time" + "github.com/docker/docker/errdefs" + "github.com/docker/docker/api/types/volume" "database/sql" @@ -38,7 +39,7 @@ func TestContainerAttachedToNewNetwork(t *testing.T) { networkName, }, NetworkAliases: map[string][]string{ - networkName: []string{ + networkName: { "alias1", "alias2", "alias3", }, }, @@ -1187,7 +1188,8 @@ func TestContainerNonExistentImage(t *testing.T) { t.Run("if the image not found don't propagate the error", func(t *testing.T) { _, err := GenericContainer(context.Background(), GenericContainerRequest{ ContainerRequest: ContainerRequest{ - Image: "postgres:nonexistent-version", + Image: "postgres:nonexistent-version", + SkipReaper: true, }, Started: true, }) @@ -1199,11 +1201,13 @@ func TestContainerNonExistentImage(t *testing.T) { }) t.Run("the context cancellation is propagated to container creation", func(t *testing.T) { - ctx, _ := context.WithTimeout(context.Background(), time.Millisecond*100) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() _, err := GenericContainer(ctx, GenericContainerRequest{ ContainerRequest: ContainerRequest{ Image: "postgres:latest", WaitingFor: wait.ForLog("log"), + SkipReaper: true, }, Started: true, })