Merge pull request #40564 from Random-Liu/add-container-not-found-error

Automatic merge from submit-queue (batch tested with PRs 40126, 40565, 38777, 40564, 40572)

Add IsContainerNotFound in kube_docker_client

This PR added `IsContainerNotFound` function in kube_docker_client and changed dockershim to use it.

@yujuhong @freehan

pkg/kubelet/dockershim/helpers.go

@@ -39,8 +39,7 @@ const (
 )
 
 var (
-	conflictRE    = regexp.MustCompile(`Conflict. (?:.)+ is already in use by container ([0-9a-z]+)`)
-	noContainerRE = regexp.MustCompile(`No such container: [0-9a-z]+`)
+	conflictRE = regexp.MustCompile(`Conflict. (?:.)+ is already in use by container ([0-9a-z]+)`)
 )
 
 // apiVersion implements kubecontainer.Version interface by implementing
@@ -313,8 +312,8 @@ func recoverFromCreationConflictIfNeeded(client dockertools.DockerInterface, cre
 		return nil, err
 	} else {
 		glog.Errorf("Failed to remove the conflicting container %q: %v", id, rmErr)
-		// Return if the error is not "No such container" error.
-		if !noContainerRE.MatchString(rmErr.Error()) {
+		// Return if the error is not container not found error.
+		if !dockertools.IsContainerNotFoundError(rmErr) {
 			return nil, err
 		}
 	}

pkg/kubelet/dockershim/helpers_test.go

@@ -236,12 +236,3 @@ func TestParsingCreationConflictError(t *testing.T) {
 	require.Len(t, matches, 2)
 	require.Equal(t, matches[1], "24666ab8c814d16f986449e504ea0159468ddf8da01897144a770f66dce0e14e")
 }
-
-func TestParsingRemovalNoContainerError(t *testing.T) {
-	// Expected error message from docker.
-	match := "Error response from daemon: No such container: 96e914f31579e44fe49b239266385330a9b2125abeb9254badd9fca74580c95a"
-	notMatch := "Error response from daemon: Other errors"
-
-	assert.True(t, noContainerRE.MatchString(match))
-	assert.False(t, noContainerRE.MatchString(notMatch))
-}

pkg/kubelet/dockertools/BUILD

@@ -90,6 +90,7 @@ go_test(
         "docker_manager_test.go",
         "docker_test.go",
         "images_test.go",
+        "kube_docker_client_test.go",
         "labels_test.go",
     ],
     data = [

pkg/kubelet/dockertools/docker_manager.go

@@ -2428,7 +2428,7 @@ func (dm *DockerManager) pruneInitContainersBeforeStart(pod *v1.Pod, podStatus *
 			// TODO: we may not need aggressive pruning
 			glog.V(4).Infof("Removing init container %q instance %q %d", status.Name, status.ID.ID, count)
 			if err := dm.client.RemoveContainer(status.ID.ID, dockertypes.ContainerRemoveOptions{RemoveVolumes: true}); err != nil {
-				if _, ok := err.(containerNotFoundError); ok {
+				if IsContainerNotFoundError(err) {
 					count--
 					continue
 				}
@@ -2671,7 +2671,7 @@ func (dm *DockerManager) GetPodStatus(uid kubetypes.UID, name, namespace string)
 		}
 		result, ip, err := dm.inspectContainer(c.ID, name, namespace)
 		if err != nil {
-			if _, ok := err.(containerNotFoundError); ok {
+			if IsContainerNotFoundError(err) {
 				// https://github.com/kubernetes/kubernetes/issues/22541
 				// Sometimes when docker's state is corrupt, a container can be listed
 				// but couldn't be inspected. We fake a status for this container so

pkg/kubelet/dockertools/docker_manager_test.go

@@ -1820,7 +1820,7 @@ func TestGetPodStatusNoSuchContainer(t *testing.T) {
 			Running:    false,
 		},
 	})
-	fakeDocker.InjectErrors(map[string]error{"inspect_container": containerNotFoundError{}})
+	fakeDocker.InjectErrors(map[string]error{"inspect_container": fmt.Errorf("Error: No such container: %s", noSuchContainerID)})
 	runSyncPod(t, dm, fakeDocker, pod, nil, false)
 
 	// Verify that we will try to start new contrainers even if the inspections

pkg/kubelet/dockertools/kube_docker_client.go

@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"regexp"
 	"sync"
 	"time"
 
@@ -123,9 +124,6 @@ func (d *kubeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS
 		return nil, ctxErr
 	}
 	if err != nil {
-		if dockerapi.IsErrContainerNotFound(err) {
-			return nil, containerNotFoundError{ID: id}
-		}
 		return nil, err
 	}
 	return &containerJSON, nil
@@ -607,15 +605,12 @@ func (e operationTimeout) Error() string {
 	return fmt.Sprintf("operation timeout: %v", e.err)
 }
 
-// containerNotFoundError is the error returned by InspectContainer when container not found. We
-// add this error type for testability. We don't use the original error returned by engine-api
-// because dockertypes.containerNotFoundError is private, we can't create and inject it in our test.
-type containerNotFoundError struct {
-	ID string
-}
+// containerNotFoundErrorRegx is the regexp of container not found error message.
+var containerNotFoundErrorRegx = regexp.MustCompile(`No such container: [0-9a-z]+`)
 
-func (e containerNotFoundError) Error() string {
-	return fmt.Sprintf("no such container: %q", e.ID)
+// IsContainerNotFoundError checks whether the error is container not found error.
+func IsContainerNotFoundError(err error) bool {
+	return containerNotFoundErrorRegx.MatchString(err.Error())
 }
 
 // imageNotFoundError is the error returned by InspectImage when image not found.

pkg/kubelet/dockertools/kube_docker_client_test.go

@@ -0,0 +1,33 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package dockertools
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIsContainerNotFoundError(t *testing.T) {
+	// Expected error message from docker.
+	containerNotFoundError := fmt.Errorf("Error response from daemon: No such container: 96e914f31579e44fe49b239266385330a9b2125abeb9254badd9fca74580c95a")
+	otherError := fmt.Errorf("Error response from daemon: Other errors")
+
+	assert.True(t, IsContainerNotFoundError(containerNotFoundError))
+	assert.False(t, IsContainerNotFoundError(otherError))
+}