From ec42aa2a1b40daaeb3bf98cc1de4c579f1ca2cb2 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:32:29 +0200 Subject: [PATCH] docker: use docker errdefs instead of string comparisons when checking errors (#24075) --- drivers/docker/coordinator.go | 6 +++--- drivers/docker/docklog/docker_logger.go | 3 ++- drivers/docker/driver.go | 11 ++++++----- drivers/docker/driver_test.go | 5 +++-- drivers/docker/handle.go | 14 ++++++-------- 5 files changed, 20 insertions(+), 19 deletions(-) diff --git a/drivers/docker/coordinator.go b/drivers/docker/coordinator.go index 4df57c3b1..a89ddb603 100644 --- a/drivers/docker/coordinator.go +++ b/drivers/docker/coordinator.go @@ -9,13 +9,13 @@ import ( "fmt" "io" "regexp" - "strings" "sync" "time" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/image" "github.com/docker/docker/api/types/registry" + "github.com/docker/docker/errdefs" hclog "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/nomad/structs" ) @@ -344,11 +344,11 @@ func (d *dockerCoordinator) removeImageImpl(id string, ctx context.Context) { break } - if strings.Contains(err.Error(), "No such image") { + if errdefs.IsNotFound(err) { d.logger.Debug("unable to cleanup image, does not exist", "image_id", id) return } - if derr, ok := err.(*types.ErrorResponse); ok && strings.Contains(derr.Error(), "Conflict") { + if errdefs.IsConflict(err) { d.logger.Debug("unable to cleanup image, still in use", "image_id", id) return } diff --git a/drivers/docker/docklog/docker_logger.go b/drivers/docker/docklog/docker_logger.go index e67b51cd9..f8a463127 100644 --- a/drivers/docker/docklog/docker_logger.go +++ b/drivers/docker/docklog/docker_logger.go @@ -14,6 +14,7 @@ import ( containerapi "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/stdcopy" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-multierror" @@ -135,7 +136,7 @@ func (d *dockerLogger) Start(opts *StartOpts) error { container, err := client.ContainerInspect(ctx, opts.ContainerID) if err != nil { - if !strings.Contains(err.Error(), "No such container") { + if !errdefs.IsNotFound(err) { return } } else if !container.State.Running { diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 93e24138c..f24b0ced7 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -27,6 +27,7 @@ import ( networkapi "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/stdcopy" "github.com/hashicorp/consul-template/signals" hclog "github.com/hashicorp/go-hclog" @@ -408,7 +409,7 @@ CREATE: d.logger.Error("failed to start container", "container_id", container.ID, "error", err) dockerClient.ContainerRemove(d.ctx, container.ID, containerapi.RemoveOptions{Force: true}) // Some sort of docker race bug, recreating the container usually works - if strings.Contains(err.Error(), "OCI runtime create failed: container with id exists:") && startAttempts < 5 { + if errdefs.IsConflict(err) && startAttempts < 5 { startAttempts++ d.logger.Debug("reattempting container create/start sequence", "attempt", startAttempts, "container_id", id) goto CREATE @@ -529,7 +530,7 @@ CREATE: // If the container already exists determine whether it's already // running or if it's dead and needs to be recreated. - if strings.Contains(strings.ToLower(createErr.Error()), "conflict. the container name") { + if errdefs.IsConflict(createErr) { container, err := d.containerByName(config.Name) if err != nil { @@ -561,7 +562,7 @@ CREATE: goto CREATE } - } else if strings.Contains(strings.ToLower(createErr.Error()), "no such image") { + } else if errdefs.IsNotFound(createErr) { // There is still a very small chance this is possible even with the // coordinator so retry. return nil, nstructs.NewRecoverableError(createErr, true) @@ -588,7 +589,7 @@ func (d *Driver) startContainer(c types.ContainerJSON) error { START: startErr := dockerClient.ContainerStart(d.ctx, c.ID, containerapi.StartOptions{}) - if startErr == nil || strings.Contains(startErr.Error(), "Container already running") { + if startErr == nil || errdefs.IsConflict(err) { return nil } @@ -1641,7 +1642,7 @@ func (d *Driver) DestroyTask(taskID string, force bool) error { c, err := dockerClient.ContainerInspect(d.ctx, h.containerID) if err != nil { - if strings.Contains(err.Error(), NoSuchContainerError) { + if _, ok := err.(errdefs.ErrNotFound); ok { h.logger.Info("container was removed out of band, will proceed with DestroyTask", "error", err) } else { diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index 3bedcae54..86b766893 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -27,6 +27,7 @@ import ( networkapi "github.com/docker/docker/api/types/network" "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" + "github.com/docker/docker/errdefs" "github.com/docker/go-connections/nat" hclog "github.com/hashicorp/go-hclog" "github.com/hashicorp/nomad/ci" @@ -376,7 +377,7 @@ func TestDockerDriver_Start_StoppedContainer(t *testing.T) { must.NoError(t, err) if _, err := client.ContainerCreate(context.Background(), opts, nil, nil, nil, containerName); err != nil { - if !strings.Contains(err.Error(), "Conflict") { + if !errdefs.IsConflict(err) { t.Fatalf("error creating initial container: %v", err) } } @@ -2890,7 +2891,7 @@ func waitForExist(t *testing.T, client *client.Client, containerID string) { tu.WaitForResult(func() (bool, error) { container, err := client.ContainerInspect(context.Background(), containerID) if err != nil { - if !strings.Contains(err.Error(), NoSuchContainerError) { + if !errdefs.IsNotFound(err) { return false, err } } diff --git a/drivers/docker/handle.go b/drivers/docker/handle.go index a6d943d42..34d8e305d 100644 --- a/drivers/docker/handle.go +++ b/drivers/docker/handle.go @@ -8,13 +8,13 @@ import ( "fmt" "os" "runtime" - "strings" "sync" "time" "github.com/armon/circbuf" containerapi "github.com/docker/docker/api/types/container" "github.com/docker/docker/client" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/stdcopy" "github.com/hashicorp/consul-template/signals" "github.com/hashicorp/go-hclog" @@ -192,12 +192,12 @@ func (h *taskHandle) Kill(killTimeout time.Duration, signal string) error { if err := h.Signal(ctx, signal); err != nil { // Container has already been removed. - if strings.Contains(err.Error(), NoSuchContainerError) { + if errdefs.IsNotFound(err) { h.logger.Debug("attempted to signal nonexistent container") return nil } // Container has already been stopped. - if strings.Contains(err.Error(), ContainerNotRunningError) { + if errdefs.IsNotModified(err) { h.logger.Debug("attempted to signal a not-running container") return nil } @@ -218,12 +218,12 @@ func (h *taskHandle) Kill(killTimeout time.Duration, signal string) error { if err != nil { // Container has already been removed. - if strings.Contains(err.Error(), NoSuchContainerError) { + if errdefs.IsNotFound(err) { h.logger.Debug("attempted to stop nonexistent container") return nil } // Container has already been stopped. - if strings.Contains(err.Error(), ContainerNotRunningError) { + if errdefs.IsNotModified(err) { h.logger.Debug("attempted to stop an not-running container") return nil } @@ -335,9 +335,7 @@ func (h *taskHandle) run() { if err := h.dockerClient.ContainerStop(ctx, h.containerID, containerapi.StopOptions{ Timeout: pointer.Of(0), }); err != nil { - noSuchContainer := strings.Contains(err.Error(), NoSuchContainerError) - containerNotRunning := strings.Contains(err.Error(), ContainerNotRunningError) - if !containerNotRunning && !noSuchContainer { + if !errdefs.IsNotModified(err) && !errdefs.IsNotFound(err) { h.logger.Error("error stopping container", "error", err) } }