Browse code

Fix error handling for kill/process not found

With the contianerd 1.0 migration we now have strongly typed errors that
we can check for process not found.
We also had some bad error checks looking for `ESRCH` which would only
be returned from `unix.Kill` and never from containerd even though we
were checking containerd responses for it.

Fixes some race conditions around process handling and our error checks
that could lead to errors that propagate up to the user that should not.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Brian Goff authored on 2017/12/16 00:00:15
Showing 2 changed files
... ...
@@ -4,10 +4,10 @@ import (
4 4
 	"context"
5 5
 	"fmt"
6 6
 	"runtime"
7
-	"strings"
8 7
 	"syscall"
9 8
 	"time"
10 9
 
10
+	"github.com/docker/docker/api/errdefs"
11 11
 	containerpkg "github.com/docker/docker/container"
12 12
 	"github.com/docker/docker/libcontainerd"
13 13
 	"github.com/docker/docker/pkg/signal"
... ...
@@ -97,15 +97,11 @@ func (daemon *Daemon) killWithSignal(container *containerpkg.Container, sig int)
97 97
 	}
98 98
 
99 99
 	if err := daemon.kill(container, sig); err != nil {
100
-		err = errors.Wrapf(err, "Cannot kill container %s", container.ID)
101
-		// if container or process not exists, ignore the error
102
-		// TODO: we shouldn't have to parse error strings from containerd
103
-		if strings.Contains(err.Error(), "container not found") ||
104
-			strings.Contains(err.Error(), "no such process") {
105
-			logrus.Warnf("container kill failed because of 'container not found' or 'no such process': %s", err.Error())
100
+		if errdefs.IsNotFound(err) {
106 101
 			unpause = false
102
+			logrus.WithError(err).WithField("container", container.ID).WithField("action", "kill").Debug("container kill failed because of 'container not found' or 'no such process'")
107 103
 		} else {
108
-			return err
104
+			return errors.Wrapf(err, "Cannot kill container %s", container.ID)
109 105
 		}
110 106
 	}
111 107
 
... ...
@@ -171,7 +167,7 @@ func (daemon *Daemon) Kill(container *containerpkg.Container) error {
171 171
 // killPossibleDeadProcess is a wrapper around killSig() suppressing "no such process" error.
172 172
 func (daemon *Daemon) killPossiblyDeadProcess(container *containerpkg.Container, sig int) error {
173 173
 	err := daemon.killWithSignal(container, sig)
174
-	if err == syscall.ESRCH {
174
+	if errdefs.IsNotFound(err) {
175 175
 		e := errNoSuchProcess{container.GetPID(), sig}
176 176
 		logrus.Debug(e)
177 177
 		return e
... ...
@@ -27,6 +27,7 @@ import (
27 27
 	"github.com/containerd/containerd/archive"
28 28
 	"github.com/containerd/containerd/cio"
29 29
 	"github.com/containerd/containerd/content"
30
+	"github.com/containerd/containerd/errdefs"
30 31
 	"github.com/containerd/containerd/images"
31 32
 	"github.com/containerd/containerd/linux/runctypes"
32 33
 	"github.com/containerd/typeurl"
... ...
@@ -317,7 +318,7 @@ func (c *client) SignalProcess(ctx context.Context, containerID, processID strin
317 317
 	if err != nil {
318 318
 		return err
319 319
 	}
320
-	return p.Kill(ctx, syscall.Signal(signal))
320
+	return wrapError(p.Kill(ctx, syscall.Signal(signal)))
321 321
 }
322 322
 
323 323
 func (c *client) ResizeTerminal(ctx context.Context, containerID, processID string, width, height int) error {
... ...
@@ -816,12 +817,19 @@ func (c *client) writeContent(ctx context.Context, mediaType, ref string, r io.R
816 816
 }
817 817
 
818 818
 func wrapError(err error) error {
819
-	if err != nil {
820
-		msg := err.Error()
821
-		for _, s := range []string{"container does not exist", "not found", "no such container"} {
822
-			if strings.Contains(msg, s) {
823
-				return wrapNotFoundError(err)
824
-			}
819
+	if err == nil {
820
+		return nil
821
+	}
822
+
823
+	switch {
824
+	case errdefs.IsNotFound(err):
825
+		return wrapNotFoundError(err)
826
+	}
827
+
828
+	msg := err.Error()
829
+	for _, s := range []string{"container does not exist", "not found", "no such container"} {
830
+		if strings.Contains(msg, s) {
831
+			return wrapNotFoundError(err)
825 832
 		}
826 833
 	}
827 834
 	return err