dockertools: migrate the unit tests and delete the package

pkg/kubelet/BUILD

@@ -247,7 +247,6 @@ filegroup(
         "//pkg/kubelet/container:all-srcs",
         "//pkg/kubelet/custommetrics:all-srcs",
         "//pkg/kubelet/dockershim:all-srcs",
-        "//pkg/kubelet/dockertools:all-srcs",
         "//pkg/kubelet/envvars:all-srcs",
         "//pkg/kubelet/events:all-srcs",
         "//pkg/kubelet/eviction:all-srcs",

pkg/kubelet/api/testing/BUILD

@@ -18,6 +18,7 @@ go_library(
     deps = [
         "//pkg/kubelet/api/v1alpha1/runtime:go_default_library",
         "//pkg/kubelet/util/sliceutils:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
     ],
 )
 

pkg/kubelet/api/testing/fake_image_service.go

@@ -18,6 +18,9 @@ package testing
 
 import (
 	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
 
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
 	"k8s.io/kubernetes/pkg/kubelet/util/sliceutils"
@@ -29,6 +32,8 @@ type FakeImageService struct {
 	FakeImageSize uint64
 	Called        []string
 	Images        map[string]*runtimeapi.Image
+
+	pulledImages []*pulledImage
 }
 
 func (r *FakeImageService) SetFakeImages(images []string) {
@@ -97,6 +102,7 @@ func (r *FakeImageService) PullImage(image *runtimeapi.ImageSpec, auth *runtimea
 
 	r.Called = append(r.Called, "PullImage")
 
+	r.pulledImages = append(r.pulledImages, &pulledImage{imageSpec: image, authConfig: auth})
 	// ImageID should be randomized for real container runtime, but here just use
 	// image's name for easily making fake images.
 	imageID := image.Image
@@ -128,3 +134,15 @@ func (r *FakeImageService) ImageFsInfo() (*runtimeapi.FsInfo, error) {
 
 	return nil, nil
 }
+
+func (r *FakeImageService) AssertImagePulledWithAuth(t *testing.T, image *runtimeapi.ImageSpec, auth *runtimeapi.AuthConfig, failMsg string) {
+	r.Lock()
+	defer r.Unlock()
+	expected := &pulledImage{imageSpec: image, authConfig: auth}
+	assert.Contains(t, r.pulledImages, expected, failMsg)
+}
+
+type pulledImage struct {
+	imageSpec  *runtimeapi.ImageSpec
+	authConfig *runtimeapi.AuthConfig
+}

pkg/kubelet/container/helpers.go

@@ -104,7 +104,9 @@ func HashContainer(container *v1.Container) uint64 {
 
 // HashContainerLegacy returns the hash of the container. It is used to compare
 // the running container with its desired spec.
-// TODO: Delete this function after we deprecate dockertools.
+// This is used by rktnetes and dockershim (for handling <=1.5 containers).
+// TODO: Remove this function when kubernetes version is >=1.8 AND rktnetes
+// update its hash function.
 func HashContainerLegacy(container *v1.Container) uint64 {
 	hash := adler32.New()
 	hashutil.DeepHashObject(hash, *container)

pkg/kubelet/dockershim/BUILD

@@ -56,6 +56,7 @@ go_library(
         "//pkg/util/hash:go_default_library",
         "//pkg/util/term:go_default_library",
         "//vendor/github.com/blang/semver:go_default_library",
+        "//vendor/github.com/docker/docker/pkg/jsonmessage:go_default_library",
         "//vendor/github.com/docker/engine-api/types:go_default_library",
         "//vendor/github.com/docker/engine-api/types/container:go_default_library",
         "//vendor/github.com/docker/engine-api/types/filters:go_default_library",
@@ -105,6 +106,7 @@ go_test(
         "//pkg/kubelet/util/cache:go_default_library",
         "//pkg/security/apparmor:go_default_library",
         "//vendor/github.com/blang/semver:go_default_library",
+        "//vendor/github.com/docker/docker/pkg/jsonmessage:go_default_library",
         "//vendor/github.com/docker/engine-api/types:go_default_library",
         "//vendor/github.com/docker/engine-api/types/container:go_default_library",
         "//vendor/github.com/docker/go-connections/nat:go_default_library",

pkg/kubelet/dockershim/docker_image.go

@@ -18,10 +18,12 @@ package dockershim
 
 import (
 	"fmt"
+	"net/http"
 
+	"github.com/docker/docker/pkg/jsonmessage"
 	dockertypes "github.com/docker/engine-api/types"
-	runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
 
+	runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
 	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 )
 
@@ -80,7 +82,7 @@ func (ds *dockerService) PullImage(image *runtimeapi.ImageSpec, auth *runtimeapi
 		dockertypes.ImagePullOptions{},
 	)
 	if err != nil {
-		return "", err
+		return "", filterHTTPError(err, image.Image)
 	}
 
 	return getImageRef(ds.client, image.Image)
@@ -127,3 +129,18 @@ func getImageRef(client libdocker.Interface, image string) (string, error) {
 func (ds *dockerService) ImageFsInfo() (*runtimeapi.FsInfo, error) {
 	return nil, fmt.Errorf("not implemented")
 }
+
+func filterHTTPError(err error, image string) error {
+	// docker/docker/pull/11314 prints detailed error info for docker pull.
+	// When it hits 502, it returns a verbose html output including an inline svg,
+	// which makes the output of kubectl get pods much harder to parse.
+	// Here converts such verbose output to a concise one.
+	jerr, ok := err.(*jsonmessage.JSONError)
+	if ok && (jerr.Code == http.StatusBadGateway ||
+		jerr.Code == http.StatusServiceUnavailable ||
+		jerr.Code == http.StatusGatewayTimeout) {
+		return fmt.Errorf("RegistryUnavailable: %v", err)
+	}
+	return err
+
+}

pkg/kubelet/dockershim/docker_image_test.go

@@ -17,12 +17,14 @@ limitations under the License.
 package dockershim
 
 import (
+	"fmt"
 	"testing"
 
+	"github.com/docker/docker/pkg/jsonmessage"
 	dockertypes "github.com/docker/engine-api/types"
+	"github.com/stretchr/testify/assert"
 
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
-
 	"k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker"
 )
 
@@ -44,3 +46,29 @@ func TestRemoveImageWithMultipleTags(t *testing.T) {
 		libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}),
 		libdocker.NewCalledDetail("remove_image", []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}}))
 }
+
+func TestPullWithJSONError(t *testing.T) {
+	ds, fakeDocker, _ := newTestDockerService()
+	tests := map[string]struct {
+		image         *runtimeapi.ImageSpec
+		err           error
+		expectedError string
+	}{
+		"Json error": {
+			&runtimeapi.ImageSpec{Image: "ubuntu"},
+			&jsonmessage.JSONError{Code: 50, Message: "Json error"},
+			"Json error",
+		},
+		"Bad gateway": {
+			&runtimeapi.ImageSpec{Image: "ubuntu"},
+			&jsonmessage.JSONError{Code: 502, Message: "<!doctype html>\n<html class=\"no-js\" lang=\"\">\n    <head>\n  </head>\n    <body>\n   <h1>Oops, there was an error!</h1>\n        <p>We have been contacted of this error, feel free to check out <a href=\"http://status.docker.com/\">status.docker.com</a>\n           to see if there is a bigger issue.</p>\n\n    </body>\n</html>"},
+			"RegistryUnavailable",
+		},
+	}
+	for key, test := range tests {
+		fakeDocker.InjectError("pull", test.err)
+		_, err := ds.PullImage(test.image, &runtimeapi.AuthConfig{})
+		assert.Error(t, err, fmt.Sprintf("TestCase [%s]", key))
+		assert.Contains(t, err.Error(), test.expectedError)
+	}
+}

pkg/kubelet/dockershim/helpers_test.go

@@ -139,8 +139,6 @@ func TestLoadSeccompLocalhostProfiles(t *testing.T) {
 }
 
 // TestGetApparmorSecurityOpts tests the logic of generating container apparmor options from sandbox annotations.
-// The actual profile loading logic is tested in dockertools.
-// TODO: Migrate the corresponding test to dockershim.
 func TestGetApparmorSecurityOpts(t *testing.T) {
 	makeConfig := func(profile string) *runtimeapi.LinuxContainerSecurityContext {
 		return &runtimeapi.LinuxContainerSecurityContext{