Merge pull request #43248 from thaJeztah/cleanup_translateContainerdStartErr

daemon: refactor translateContainerdStartErr() and remove unused argument

daemon/errors.go

@@ -132,35 +132,70 @@ func (e startInvalidConfigError) Error() string {
 
 func (e startInvalidConfigError) InvalidParameter() {} // Is this right???
 
-func translateContainerdStartErr(cmd string, setExitCode func(int), err error) error {
+// exitStatus is the exit-code as set by setExitCodeFromError
+type exitStatus = int
+
+const (
+	exitEaccess     exitStatus = 126 // container cmd can't be invoked (permission denied)
+	exitCmdNotFound exitStatus = 127 // container cmd not found/does not exist or invalid bind-mount
+	exitUnknown     exitStatus = 128 // unknown error
+)
+
+// setExitCodeFromError converts the error returned by containerd
+// when starting a container, and applies the corresponding exitStatus to the
+// container. It returns an errdefs error (either errdefs.ErrInvalidParameter
+// or errdefs.ErrUnknown).
+func setExitCodeFromError(setExitCode func(exitStatus), err error) error {
+	if err == nil {
+		return nil
+	}
 	errDesc := status.Convert(err).Message()
 	contains := func(s1, s2 string) bool {
 		return strings.Contains(strings.ToLower(s1), s2)
 	}
-	var retErr = errdefs.Unknown(errors.New(errDesc))
-	// if we receive an internal error from the initial start of a container then lets
-	// return it instead of entering the restart loop
-	// set to 127 for container cmd not found/does not exist)
-	if contains(errDesc, "executable file not found") ||
-		contains(errDesc, "no such file or directory") ||
-		contains(errDesc, "system cannot find the file specified") ||
-		contains(errDesc, "failed to run runc create/exec call") {
-		setExitCode(127)
-		retErr = startInvalidConfigError(errDesc)
-	}
+
 	// set to 126 for container cmd can't be invoked errors
 	if contains(errDesc, syscall.EACCES.Error()) {
-		setExitCode(126)
-		retErr = startInvalidConfigError(errDesc)
+		setExitCode(exitEaccess)
+		return startInvalidConfigError(errDesc)
 	}
 
 	// attempted to mount a file onto a directory, or a directory onto a file, maybe from user specified bind mounts
 	if contains(errDesc, syscall.ENOTDIR.Error()) {
 		errDesc += ": Are you trying to mount a directory onto a file (or vice-versa)? Check if the specified host path exists and is the expected type"
-		setExitCode(127)
-		retErr = startInvalidConfigError(errDesc)
+		setExitCode(exitCmdNotFound)
+		return startInvalidConfigError(errDesc)
+	}
+
+	// if we receive an internal error from the initial start of a container then lets
+	// return it instead of entering the restart loop
+	// set to 127 for container cmd not found/does not exist.
+	if isInvalidCommand(errDesc) {
+		setExitCode(exitCmdNotFound)
+		return startInvalidConfigError(errDesc)
 	}
 
 	// TODO: it would be nice to get some better errors from containerd so we can return better errors here
-	return retErr
+	setExitCode(exitUnknown)
+	return errdefs.Unknown(errors.New(errDesc))
+}
+
+// isInvalidCommand tries to detect if the reason the container failed to start
+// was due to an invalid command for the container (command not found, or not
+// a valid executable).
+func isInvalidCommand(errMessage string) bool {
+	errMessage = strings.ToLower(errMessage)
+	errMessages := []string{
+		"executable file not found",
+		"no such file or directory",
+		"system cannot find the file specified",
+		"failed to run runc create/exec call",
+	}
+
+	for _, msg := range errMessages {
+		if strings.Contains(errMessage, msg) {
+			return true
+		}
+	}
+	return false
 }

daemon/exec.go

@@ -288,7 +288,7 @@ func (daemon *Daemon) ContainerExecStart(ctx context.Context, name string, optio
 	close(ec.Started)
 	if err != nil {
 		defer ec.Unlock()
-		return translateContainerdStartErr(ec.Entrypoint, ec.SetExitCode, err)
+		return setExitCodeFromError(ec.SetExitCode, err)
 	}
 	ec.Unlock()
 

daemon/start.go

@@ -99,7 +99,7 @@ func (daemon *Daemon) ContainerStart(ctx context.Context, name string, hostConfi
 // container needs, such as storage and networking, as well as links
 // between containers. The container is left waiting for a signal to
 // begin running.
-func (daemon *Daemon) containerStart(ctx context.Context, container *container.Container, checkpoint string, checkpointDir string, resetRestartManager bool) (err error) {
+func (daemon *Daemon) containerStart(ctx context.Context, container *container.Container, checkpoint string, checkpointDir string, resetRestartManager bool) (retErr error) {
 	start := time.Now()
 	container.Lock()
 	defer container.Unlock()
@@ -120,11 +120,11 @@ func (daemon *Daemon) containerStart(ctx context.Context, container *container.C
 	// if we encounter an error during start we need to ensure that any other
 	// setup has been cleaned up properly
 	defer func() {
-		if err != nil {
-			container.SetError(err)
+		if retErr != nil {
+			container.SetError(retErr)
 			// if no one else has set it, make sure we don't leave it at zero
 			if container.ExitCode() == 0 {
-				container.SetExitCode(128)
+				container.SetExitCode(exitUnknown)
 			}
 			if err := container.CheckpointTo(daemon.containersReplica); err != nil {
 				logrus.Errorf("%s: failed saving state on start failure: %v", container.ID, err)
@@ -179,7 +179,7 @@ func (daemon *Daemon) containerStart(ctx context.Context, container *container.C
 
 	ctr, err := libcontainerd.ReplaceContainer(ctx, daemon.containerd, container.ID, spec, shim, createOptions)
 	if err != nil {
-		return translateContainerdStartErr(container.Path, container.SetExitCode, err)
+		return setExitCodeFromError(container.SetExitCode, err)
 	}
 
 	// TODO(mlaventure): we need to specify checkpoint options here
@@ -191,7 +191,7 @@ func (daemon *Daemon) containerStart(ctx context.Context, container *container.C
 			logrus.WithError(err).WithField("container", container.ID).
 				Error("failed to delete failed start container")
 		}
-		return translateContainerdStartErr(container.Path, container.SetExitCode, err)
+		return setExitCodeFromError(container.SetExitCode, err)
 	}
 
 	container.HasBeenManuallyRestarted = false

daemon/start_unix.go

@@ -17,7 +17,7 @@ func (daemon *Daemon) getLibcontainerdCreateOptions(container *container.Contain
 
 	rt, err := daemon.getRuntime(container.HostConfig.Runtime)
 	if err != nil {
-		return "", nil, translateContainerdStartErr(container.Path, container.SetExitCode, err)
+		return "", nil, setExitCodeFromError(container.SetExitCode, err)
 	}
 
 	return rt.Shim.Binary, rt.Shim.Opts, nil