Refactor Info and Version.

pkg/kubelet/dockertools/docker.go

@@ -69,8 +69,8 @@ type DockerInterface interface {
 	PullImage(opts docker.PullImageOptions, auth docker.AuthConfiguration) error
 	RemoveImage(image string) error
 	Logs(string, dockertypes.ContainerLogsOptions, StreamOptions) error
-	Version() (*docker.Env, error)
-	Info() (*docker.Env, error)
+	Version() (*dockertypes.Version, error)
+	Info() (*dockertypes.Info, error)
 	CreateExec(string, dockertypes.ExecConfig) (*dockertypes.ContainerExecCreateResponse, error)
 	StartExec(string, dockertypes.ExecStartCheck, StreamOptions) error
 	InspectExec(id string) (*dockertypes.ContainerExecInspect, error)

pkg/kubelet/dockertools/docker_test.go

@@ -157,28 +157,6 @@ func TestContainerNaming(t *testing.T) {
 	}
 }
 
-func TestVersion(t *testing.T) {
-	fakeDocker := NewFakeDockerClientWithVersion("1.1.3", "1.15")
-	manager := &DockerManager{client: fakeDocker}
-	version, err := manager.Version()
-	if err != nil {
-		t.Errorf("got error while getting docker server version - %s", err)
-	}
-	expectedVersion, _ := docker.NewAPIVersion("1.1.3")
-	if e, a := expectedVersion.String(), version.String(); e != a {
-		t.Errorf("invalid docker server version. expected: %v, got: %v", e, a)
-	}
-
-	version, err = manager.APIVersion()
-	if err != nil {
-		t.Errorf("got error while getting docker server version - %s", err)
-	}
-	expectedVersion, _ = docker.NewAPIVersion("1.15")
-	if e, a := expectedVersion.String(), version.String(); e != a {
-		t.Errorf("invalid docker server version. expected: %v, got: %v", e, a)
-	}
-}
-
 func TestParseImageName(t *testing.T) {
 	tests := []struct {
 		imageName string

pkg/kubelet/dockertools/fake_docker_client.go

@@ -50,8 +50,8 @@ type FakeDockerClient struct {
 	Stopped       []string
 	Removed       []string
 	RemovedImages sets.String
-	VersionInfo   docker.Env
-	Information   docker.Env
+	VersionInfo   dockertypes.Version
+	Information   dockertypes.Info
 	ExecInspect   *dockertypes.ContainerExecInspect
 	execCmd       []string
 	EnableSleep   bool
@@ -67,7 +67,7 @@ func NewFakeDockerClient() *FakeDockerClient {
 
 func NewFakeDockerClientWithVersion(version, apiVersion string) *FakeDockerClient {
 	return &FakeDockerClient{
-		VersionInfo:   docker.Env{fmt.Sprintf("Version=%s", version), fmt.Sprintf("ApiVersion=%s", apiVersion)},
+		VersionInfo:   dockertypes.Version{Version: version, APIVersion: apiVersion},
 		Errors:        make(map[string]error),
 		RemovedImages: sets.String{},
 		ContainerMap:  make(map[string]*dockertypes.ContainerJSON),
@@ -437,13 +437,13 @@ func (f *FakeDockerClient) PullImage(opts docker.PullImageOptions, auth docker.A
 	return err
 }
 
-func (f *FakeDockerClient) Version() (*docker.Env, error) {
+func (f *FakeDockerClient) Version() (*dockertypes.Version, error) {
 	f.Lock()
 	defer f.Unlock()
 	return &f.VersionInfo, f.popError("version")
 }
 
-func (f *FakeDockerClient) Info() (*docker.Env, error) {
+func (f *FakeDockerClient) Info() (*dockertypes.Info, error) {
 	return &f.Information, nil
 }
 

pkg/kubelet/dockertools/instrumented_docker.go

@@ -148,7 +148,7 @@ func (in instrumentedDockerInterface) Logs(id string, opts dockertypes.Container
 	return err
 }
 
-func (in instrumentedDockerInterface) Version() (*docker.Env, error) {
+func (in instrumentedDockerInterface) Version() (*dockertypes.Version, error) {
 	const operation = "version"
 	defer recordOperation(operation, time.Now())
 
@@ -157,7 +157,7 @@ func (in instrumentedDockerInterface) Version() (*docker.Env, error) {
 	return out, err
 }
 
-func (in instrumentedDockerInterface) Info() (*docker.Env, error) {
+func (in instrumentedDockerInterface) Info() (*dockertypes.Info, error) {
 	const operation = "info"
 	defer recordOperation(operation, time.Now())
 

pkg/kubelet/dockertools/kube_docker_client.go

@@ -87,19 +87,6 @@ func convertFilters(filters map[string][]string) dockerfilters.Args {
 	return args
 }
 
-// convertEnv converts data to a go-dockerclient Env
-func convertEnv(src interface{}) (*docker.Env, error) {
-	m := make(map[string]interface{})
-	if err := convertType(&src, &m); err != nil {
-		return nil, err
-	}
-	env := &docker.Env{}
-	for k, v := range m {
-		env.SetAuto(k, v)
-	}
-	return env, nil
-}
-
 func (k *kubeDockerClient) ListContainers(options dockertypes.ContainerListOptions) ([]dockertypes.Container, error) {
 	containers, err := k.client.ContainerList(getDefaultContext(), options)
 	if err != nil {
@@ -219,20 +206,20 @@ func (d *kubeDockerClient) Logs(id string, opts dockertypes.ContainerLogsOptions
 	return d.redirectResponseToOutputStream(sopts.RawTerminal, sopts.OutputStream, sopts.ErrorStream, resp)
 }
 
-func (d *kubeDockerClient) Version() (*docker.Env, error) {
+func (d *kubeDockerClient) Version() (*dockertypes.Version, error) {
 	resp, err := d.client.ServerVersion(getDefaultContext())
 	if err != nil {
 		return nil, err
 	}
-	return convertEnv(resp)
+	return &resp, nil
 }
 
-func (d *kubeDockerClient) Info() (*docker.Env, error) {
+func (d *kubeDockerClient) Info() (*dockertypes.Info, error) {
 	resp, err := d.client.Info(getDefaultContext())
 	if err != nil {
 		return nil, err
 	}
-	return convertEnv(resp)
+	return &resp, nil
 }
 
 // TODO(random-liu): Add unit test for exec and attach functions, just like what go-dockerclient did.

pkg/kubelet/dockertools/manager.go

@@ -25,13 +25,11 @@ import (
 	"os"
 	"os/exec"
 	"path"
-	"regexp"
 	"strconv"
 	"strings"
 	"sync"
 	"time"
 
-	"github.com/coreos/go-semver/semver"
 	dockertypes "github.com/docker/engine-api/types"
 	dockercontainer "github.com/docker/engine-api/types/container"
 	dockerstrslice "github.com/docker/engine-api/types/strslice"
@@ -220,7 +218,7 @@ func NewDockerManager(
 		glog.Errorf("Failed to execute Info() call to the Docker client: %v", err)
 		glog.Warningf("Using fallback default of /var/lib/docker for location of Docker runtime")
 	} else {
-		dockerRoot = dockerInfo.Get("DockerRootDir")
+		dockerRoot = dockerInfo.DockerRootDir
 		glog.Infof("Setting dockerRoot to %s", dockerRoot)
 	}
 
@@ -873,67 +871,48 @@ func getDockerNetworkMode(container *dockertypes.ContainerJSON) string {
 }
 
 // dockerVersion implementes kubecontainer.Version interface by implementing
-// Compare() and String() (which is implemented by the underlying semver.Version)
-// TODO: this code is the same as rktVersion and may make sense to be moved to
-// somewhere shared.
-type dockerVersion struct {
-	*semver.Version
+// Compare() and String(). It could contain either server version or api version.
+type dockerVersion string
+
+func (v dockerVersion) String() string {
+	return string(v)
 }
 
-// Older versions of Docker could return non-semantically versioned values (distros like Fedora
-// included partial values such as 1.8.1.fc21 which is not semver). Force those values to be semver.
-var almostSemverRegexp = regexp.MustCompile(`^(\d+\.\d+\.\d+)\.(.*)$`)
+func (v dockerVersion) Compare(other string) (int, error) {
+	return compare(string(v), other), nil
+}
 
-// newDockerVersion returns a semantically versioned docker version value
-func newDockerVersion(version string) (dockerVersion, error) {
-	sem, err := semver.NewVersion(version)
-	if err != nil {
-		matches := almostSemverRegexp.FindStringSubmatch(version)
-		if matches == nil {
-			return dockerVersion{}, err
+// compare is copied from engine-api, it compares two version strings, returns -1 if
+// v1 < v2, 1 if v1 > v2, 0 otherwise.
+// TODO(random-liu): Leveraging the version comparison in engine-api after bumping up
+// the engine-api version. See #24076
+func compare(v1, v2 string) int {
+	var (
+		currTab  = strings.Split(v1, ".")
+		otherTab = strings.Split(v2, ".")
+	)
+
+	max := len(currTab)
+	if len(otherTab) > max {
+		max = len(otherTab)
+	}
+	for i := 0; i < max; i++ {
+		var currInt, otherInt int
+
+		if len(currTab) > i {
+			currInt, _ = strconv.Atoi(currTab[i])
+		}
+		if len(otherTab) > i {
+			otherInt, _ = strconv.Atoi(otherTab[i])
+		}
+		if currInt > otherInt {
+			return 1
+		}
+		if otherInt > currInt {
+			return -1
 		}
-		sem, err = semver.NewVersion(strings.Join(matches[1:], "-"))
 	}
-	return dockerVersion{sem}, err
-}
-
-func (r dockerVersion) Compare(other string) (int, error) {
-	v, err := newDockerVersion(other)
-	if err != nil {
-		return -1, err
-	}
-
-	if r.LessThan(*v.Version) {
-		return -1, nil
-	}
-	if v.Version.LessThan(*r.Version) {
-		return 1, nil
-	}
-	return 0, nil
-}
-
-// dockerVersion implementes kubecontainer.Version interface by implementing
-// Compare() and String() on top og go-dockerclient's APIVersion. This version
-// string doesn't conform to semantic versioning, as it is only "x.y"
-type dockerAPIVersion docker.APIVersion
-
-func (dv dockerAPIVersion) String() string {
-	return docker.APIVersion(dv).String()
-}
-
-func (dv dockerAPIVersion) Compare(other string) (int, error) {
-	a := docker.APIVersion(dv)
-	b, err := docker.NewAPIVersion(other)
-	if err != nil {
-		return 0, err
-	}
-	if a.LessThan(b) {
-		return -1, nil
-	}
-	if a.GreaterThan(b) {
-		return 1, nil
-	}
-	return 0, nil
+	return 0
 }
 
 func (dm *DockerManager) Type() string {
@@ -941,33 +920,21 @@ func (dm *DockerManager) Type() string {
 }
 
 func (dm *DockerManager) Version() (kubecontainer.Version, error) {
-	env, err := dm.client.Version()
+	v, err := dm.client.Version()
 	if err != nil {
 		return nil, fmt.Errorf("docker: failed to get docker version: %v", err)
 	}
 
-	engineVersion := env.Get("Version")
-	version, err := newDockerVersion(engineVersion)
-	if err != nil {
-		glog.Errorf("docker: failed to parse docker server version %q: %v", engineVersion, err)
-		return nil, fmt.Errorf("docker: failed to parse docker server version %q: %v", engineVersion, err)
-	}
-	return version, nil
+	return dockerVersion(v.Version), nil
 }
 
 func (dm *DockerManager) APIVersion() (kubecontainer.Version, error) {
-	env, err := dm.client.Version()
+	v, err := dm.client.Version()
 	if err != nil {
 		return nil, fmt.Errorf("docker: failed to get docker version: %v", err)
 	}
 
-	apiVersion := env.Get("ApiVersion")
-	version, err := docker.NewAPIVersion(apiVersion)
-	if err != nil {
-		glog.Errorf("docker: failed to parse docker api version %q: %v", apiVersion, err)
-		return nil, fmt.Errorf("docker: failed to parse docker api version %q: %v", apiVersion, err)
-	}
-	return dockerAPIVersion(version), nil
+	return dockerVersion(v.APIVersion), nil
 }
 
 // Status returns error if docker daemon is unhealthy, nil otherwise.

pkg/kubelet/dockertools/manager_test.go

@@ -93,8 +93,13 @@ func (f *fakeRuntimeHelper) GeneratePodHostNameAndDomain(pod *api.Pod) (string,
 	return "", ""
 }
 
-func newTestDockerManagerWithHTTPClientWithVersion(fakeHTTPClient *fakeHTTP, version, apiVersion string) (*DockerManager, *FakeDockerClient) {
-	fakeDocker := NewFakeDockerClientWithVersion(version, apiVersion)
+func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerClient) (*DockerManager, *FakeDockerClient) {
+	if fakeHTTPClient == nil {
+		fakeHTTPClient = &fakeHTTP{}
+	}
+	if fakeDocker == nil {
+		fakeDocker = NewFakeDockerClient()
+	}
 	fakeRecorder := &record.FakeRecorder{}
 	containerRefManager := kubecontainer.NewRefManager()
 	networkPlugin, _ := network.InitNetworkPlugin([]network.NetworkPlugin{}, "", nettest.NewFakeHost(nil))
@@ -116,11 +121,16 @@ func newTestDockerManagerWithHTTPClientWithVersion(fakeHTTPClient *fakeHTTP, ver
 }
 
 func newTestDockerManagerWithHTTPClient(fakeHTTPClient *fakeHTTP) (*DockerManager, *FakeDockerClient) {
-	return newTestDockerManagerWithHTTPClientWithVersion(fakeHTTPClient, "1.8.1", "1.20")
+	return createTestDockerManager(fakeHTTPClient, nil)
+}
+
+func newTestDockerManagerWithVersion(version, apiVersion string) (*DockerManager, *FakeDockerClient) {
+	fakeDocker := NewFakeDockerClientWithVersion(version, apiVersion)
+	return createTestDockerManager(nil, fakeDocker)
 }
 
 func newTestDockerManager() (*DockerManager, *FakeDockerClient) {
-	return newTestDockerManagerWithHTTPClient(&fakeHTTP{})
+	return createTestDockerManager(nil, nil)
 }
 
 func matchString(t *testing.T, pattern, str string) bool {
@@ -131,35 +141,6 @@ func matchString(t *testing.T, pattern, str string) bool {
 	return match
 }
 
-func TestNewDockerVersion(t *testing.T) {
-	cases := []struct {
-		value string
-		out   string
-		err   bool
-	}{
-		{value: "1", err: true},
-		{value: "1.8", err: true},
-		{value: "1.8.1", out: "1.8.1"},
-		{value: "1.8.1.fc21", out: "1.8.1-fc21"},
-		{value: "1.8.1.fc21.other", out: "1.8.1-fc21.other"},
-		{value: "1.8.1-fc21.other", out: "1.8.1-fc21.other"},
-		{value: "1.8.1-beta.12", out: "1.8.1-beta.12"},
-	}
-	for _, test := range cases {
-		v, err := newDockerVersion(test.value)
-		switch {
-		case err != nil && test.err:
-			continue
-		case (err != nil) != test.err:
-			t.Errorf("error for %q: expected %t, got %v", test.value, test.err, err)
-			continue
-		}
-		if v.String() != test.out {
-			t.Errorf("unexpected parsed version %q for %q", v, test.value)
-		}
-	}
-}
-
 func TestSetEntrypointAndCommand(t *testing.T) {
 	cases := []struct {
 		name      string
@@ -1730,7 +1711,7 @@ func verifySyncResults(t *testing.T, expectedResults []*kubecontainer.SyncResult
 }
 
 func TestSeccompIsDisabledWithDockerV110(t *testing.T) {
-	dm, fakeDocker := newTestDockerManagerWithHTTPClientWithVersion(&fakeHTTP{}, "1.10.1", "1.22")
+	dm, fakeDocker := newTestDockerManagerWithVersion("1.10.1", "1.22")
 	pod := &api.Pod{
 		ObjectMeta: api.ObjectMeta{
 			UID:       "12345678",
@@ -1769,7 +1750,7 @@ func TestSeccompIsDisabledWithDockerV110(t *testing.T) {
 }
 
 func TestSecurityOptsAreNilWithDockerV19(t *testing.T) {
-	dm, fakeDocker := newTestDockerManagerWithHTTPClientWithVersion(&fakeHTTP{}, "1.9.1", "1.21")
+	dm, fakeDocker := newTestDockerManagerWithVersion("1.9.1", "1.21")
 	pod := &api.Pod{
 		ObjectMeta: api.ObjectMeta{
 			UID:       "12345678",
@@ -1808,10 +1789,6 @@ func TestSecurityOptsAreNilWithDockerV19(t *testing.T) {
 }
 
 func TestCheckVersionCompatibility(t *testing.T) {
-	apiVersion, err := docker.NewAPIVersion(minimumDockerAPIVersion)
-	if err != nil {
-		t.Fatalf("unexpected error %v", err)
-	}
 	type test struct {
 		version    string
 		compatible bool
@@ -1821,22 +1798,17 @@ func TestCheckVersionCompatibility(t *testing.T) {
 		{minimumDockerAPIVersion, true},
 		// Invalid apiversion
 		{"invalid_api_version", false},
-	}
-	for i := range apiVersion {
-		apiVersion[i]++
-		// Newer apiversion
-		tests = append(tests, test{apiVersion.String(), true})
-		apiVersion[i] -= 2
 		// Older apiversion
-		if apiVersion[i] >= 0 {
-			tests = append(tests, test{apiVersion.String(), false})
-		}
-		apiVersion[i]++
+		{"1.0.0", false},
+		// Newer apiversion
+		// NOTE(random-liu): We need to bump up the newer apiversion,
+		// if docker apiversion really reaches "9.9.9" someday. But I
+		// really doubt whether the test could live that long.
+		{"9.9.9", true},
 	}
-
 	for i, tt := range tests {
 		testCase := fmt.Sprintf("test case #%d test version %q", i, tt.version)
-		dm, fakeDocker := newTestDockerManagerWithHTTPClientWithVersion(&fakeHTTP{}, "", tt.version)
+		dm, fakeDocker := newTestDockerManagerWithVersion("", tt.version)
 		err := dm.checkVersionCompatibility()
 		assert.Equal(t, tt.compatible, err == nil, testCase)
 		if tt.compatible == true {
@@ -1848,6 +1820,27 @@ func TestCheckVersionCompatibility(t *testing.T) {
 	}
 }
 
+func TestVersion(t *testing.T) {
+	expectedVersion := "1.8.1"
+	expectedAPIVersion := "1.20"
+	dm, _ := newTestDockerManagerWithVersion(expectedVersion, expectedAPIVersion)
+	version, err := dm.Version()
+	if err != nil {
+		t.Errorf("got error while getting docker server version - %v", err)
+	}
+	if e, a := expectedVersion, version.String(); e != a {
+		t.Errorf("expect docker server version %q, got %q", e, a)
+	}
+
+	apiVersion, err := dm.APIVersion()
+	if err != nil {
+		t.Errorf("got error while getting docker api version - %v", err)
+	}
+	if e, a := expectedAPIVersion, apiVersion.String(); e != a {
+		t.Errorf("expect docker api version %q, got %q", e, a)
+	}
+}
+
 func TestGetPodStatusNoSuchContainer(t *testing.T) {
 	const (
 		noSuchContainerID = "nosuchcontainer"