Merge pull request #1047 from lavalamp/fixApiObj

Cleanup: remove deprecated fields the proper way

pkg/api/conversion.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+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 api
+
+import (
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
+)
+
+func init() {
+	// TODO: Consider inverting dependency chain-- imagine v1beta1 package
+	// registering all of these functions. Then, if you want to be able to understand
+	// v1beta1 objects, you just import that package for its side effects.
+	AddConversionFuncs(
+		// EnvVar's Key is deprecated in favor of Name.
+		func(in *EnvVar, out *v1beta1.EnvVar) error {
+			out.Value = in.Value
+			out.Key = in.Name
+			out.Name = in.Name
+			return nil
+		},
+		func(in *v1beta1.EnvVar, out *EnvVar) error {
+			out.Value = in.Value
+			if in.Name != "" {
+				out.Name = in.Name
+			} else {
+				out.Name = in.Key
+			}
+			return nil
+		},
+
+		// Path & MountType are deprecated.
+		func(in *VolumeMount, out *v1beta1.VolumeMount) error {
+			out.Name = in.Name
+			out.ReadOnly = in.ReadOnly
+			out.MountPath = in.MountPath
+			out.Path = in.MountPath
+			out.MountType = "" // MountType is ignored.
+			return nil
+		},
+		func(in *v1beta1.VolumeMount, out *VolumeMount) error {
+			out.Name = in.Name
+			out.ReadOnly = in.ReadOnly
+			if in.MountPath == "" {
+				out.MountPath = in.Path
+			} else {
+				out.MountPath = in.MountPath
+			}
+			return nil
+		},
+	)
+
+}

pkg/api/conversion_test.go

@@ -0,0 +1,112 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+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 api
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
+)
+
+func TestEnvConversion(t *testing.T) {
+	nonCanonical := []v1beta1.EnvVar{
+		{Key: "EV"},
+		{Key: "EV", Name: "EX"},
+	}
+	canonical := []EnvVar{
+		{Name: "EV"},
+		{Name: "EX"},
+	}
+	for i := range nonCanonical {
+		var got EnvVar
+		err := Convert(&nonCanonical[i], &got)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if e, a := canonical[i], got; !reflect.DeepEqual(e, a) {
+			t.Errorf("expected %v, got %v", e, a)
+		}
+	}
+
+	// Test conversion the other way, too.
+	for i := range canonical {
+		var got v1beta1.EnvVar
+		err := Convert(&canonical[i], &got)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if e, a := canonical[i].Name, got.Key; e != a {
+			t.Errorf("expected %v, got %v", e, a)
+		}
+		if e, a := canonical[i].Name, got.Name; e != a {
+			t.Errorf("expected %v, got %v", e, a)
+		}
+	}
+}
+
+func TestVolumeMountConversionToOld(t *testing.T) {
+	table := []struct {
+		in  VolumeMount
+		out v1beta1.VolumeMount
+	}{
+		{
+			in:  VolumeMount{Name: "foo", MountPath: "/dev/foo", ReadOnly: true},
+			out: v1beta1.VolumeMount{Name: "foo", MountPath: "/dev/foo", Path: "/dev/foo", ReadOnly: true},
+		},
+	}
+	for _, item := range table {
+		got := v1beta1.VolumeMount{}
+		err := Convert(&item.in, &got)
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+			continue
+		}
+		if e, a := item.out, got; !reflect.DeepEqual(e, a) {
+			t.Errorf("Expected: %#v, got %#v", e, a)
+		}
+	}
+}
+
+func TestVolumeMountConversionToNew(t *testing.T) {
+	table := []struct {
+		in  v1beta1.VolumeMount
+		out VolumeMount
+	}{
+		{
+			in:  v1beta1.VolumeMount{Name: "foo", MountPath: "/dev/foo", ReadOnly: true},
+			out: VolumeMount{Name: "foo", MountPath: "/dev/foo", ReadOnly: true},
+		}, {
+			in:  v1beta1.VolumeMount{Name: "foo", MountPath: "/dev/foo", Path: "/dev/bar", ReadOnly: true},
+			out: VolumeMount{Name: "foo", MountPath: "/dev/foo", ReadOnly: true},
+		}, {
+			in:  v1beta1.VolumeMount{Name: "foo", Path: "/dev/bar", ReadOnly: true},
+			out: VolumeMount{Name: "foo", MountPath: "/dev/bar", ReadOnly: true},
+		},
+	}
+	for _, item := range table {
+		got := VolumeMount{}
+		err := Convert(&item.in, &got)
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+			continue
+		}
+		if e, a := item.out, got; !reflect.DeepEqual(e, a) {
+			t.Errorf("Expected: %#v, got %#v", e, a)
+		}
+	}
+}

pkg/api/helper.go

@@ -43,10 +43,9 @@ type resourceVersioner interface {
 var Codec codec
 var ResourceVersioner resourceVersioner
 
-var conversionScheme *conversion.Scheme
+var conversionScheme = conversion.NewScheme()
 
 func init() {
-	conversionScheme = conversion.NewScheme()
 	conversionScheme.InternalVersion = ""
 	conversionScheme.ExternalVersion = "v1beta1"
 	conversionScheme.MetaInsertionFactory = metaInsertion{}
@@ -83,30 +82,6 @@ func init() {
 		v1beta1.Binding{},
 	)
 
-	// TODO: when we get more of this stuff, move to its own file. This is not a
-	// good home for lots of conversion functions.
-	// TODO: Consider inverting dependency chain-- imagine v1beta1 package
-	// registering all of these functions. Then, if you want to be able to understand
-	// v1beta1 objects, you just import that package for its side effects.
-	AddConversionFuncs(
-		// EnvVar's Key is deprecated in favor of Name.
-		func(in *EnvVar, out *v1beta1.EnvVar) error {
-			out.Value = in.Value
-			out.Key = in.Name
-			out.Name = in.Name
-			return nil
-		},
-		func(in *v1beta1.EnvVar, out *EnvVar) error {
-			out.Value = in.Value
-			if in.Name != "" {
-				out.Name = in.Name
-			} else {
-				out.Name = in.Key
-			}
-			return nil
-		},
-	)
-
 	Codec = conversionScheme
 	ResourceVersioner = NewJSONBaseResourceVersioner()
 }

pkg/api/types.go

@@ -116,13 +116,7 @@ type VolumeMount struct {
 	// Optional: Defaults to false (read-write).
 	ReadOnly bool `yaml:"readOnly,omitempty" json:"readOnly,omitempty"`
 	// Required.
-	// Exactly one of the following must be set.  If both are set, prefer MountPath.
-	// DEPRECATED: Path will be removed in a future version of the API.
 	MountPath string `yaml:"mountPath,omitempty" json:"mountPath,omitempty"`
-	Path      string `yaml:"path,omitempty" json:"path,omitempty"`
-	// One of: "LOCAL" (local volume) or "HOST" (external mount from the host). Default: LOCAL.
-	// DEPRECATED: MountType will be removed in a future version of the API.
-	MountType string `yaml:"mountType,omitempty" json:"mountType,omitempty"`
 }
 
 // EnvVar represents an environment variable present in a Container

pkg/api/validation.go

@@ -22,7 +22,6 @@ import (
 	errs "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
-	"github.com/golang/glog"
 )
 
 func validateVolumes(volumes []Volume) (util.StringSet, errs.ErrorList) {
@@ -142,17 +141,7 @@ func validateVolumeMounts(mounts []VolumeMount, volumes util.StringSet) errs.Err
 			mErrs = append(mErrs, errs.NewNotFound("name", mnt.Name))
 		}
 		if len(mnt.MountPath) == 0 {
-			// Backwards compat.
-			if len(mnt.Path) == 0 {
-				mErrs = append(mErrs, errs.NewRequired("mountPath", mnt.MountPath))
-			} else {
-				glog.Warning("DEPRECATED: VolumeMount.Path has been replaced by VolumeMount.MountPath")
-				mnt.MountPath = mnt.Path
-				mnt.Path = ""
-			}
-		}
-		if len(mnt.MountType) != 0 {
-			glog.Warning("DEPRECATED: VolumeMount.MountType will be removed. The Volume struct will handle types")
+			mErrs = append(mErrs, errs.NewRequired("mountPath", mnt.MountPath))
 		}
 		allErrs = append(allErrs, mErrs.PrefixIndex(i)...)
 	}

pkg/api/validation_test.go

@@ -17,12 +17,10 @@ limitations under the License.
 package api
 
 import (
-	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
@@ -154,42 +152,6 @@ func TestValidateEnv(t *testing.T) {
 	}
 }
 
-func TestEnvConversion(t *testing.T) {
-	nonCanonical := []v1beta1.EnvVar{
-		{Key: "EV"},
-		{Key: "EV", Name: "EX"},
-	}
-	canonical := []EnvVar{
-		{Name: "EV"},
-		{Name: "EX"},
-	}
-	for i := range nonCanonical {
-		var got EnvVar
-		err := Convert(&nonCanonical[i], &got)
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		if e, a := canonical[i], got; !reflect.DeepEqual(e, a) {
-			t.Errorf("expected %v, got %v", e, a)
-		}
-	}
-
-	// Test conversion the other way, too.
-	for i := range canonical {
-		var got v1beta1.EnvVar
-		err := Convert(&canonical[i], &got)
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		if e, a := canonical[i].Name, got.Key; e != a {
-			t.Errorf("expected %v, got %v", e, a)
-		}
-		if e, a := canonical[i].Name, got.Name; e != a {
-			t.Errorf("expected %v, got %v", e, a)
-		}
-	}
-}
-
 func TestValidateVolumeMounts(t *testing.T) {
 	volumes := util.NewStringSet("abc", "123", "abc-123")
 
@@ -202,16 +164,6 @@ func TestValidateVolumeMounts(t *testing.T) {
 		t.Errorf("expected success: %v", errs)
 	}
 
-	nonCanonicalCase := []VolumeMount{
-		{Name: "abc", Path: "/foo"},
-	}
-	if errs := validateVolumeMounts(nonCanonicalCase, volumes); len(errs) != 0 {
-		t.Errorf("expected success: %v", errs)
-	}
-	if nonCanonicalCase[0].MountPath != "/foo" {
-		t.Errorf("expected canonicalized values: %+v", nonCanonicalCase[0])
-	}
-
 	errorCases := map[string][]VolumeMount{
 		"empty name":      {{Name: "", MountPath: "/foo"}},
 		"name not found":  {{Name: "", MountPath: "/foo"}},
@@ -293,7 +245,7 @@ func TestValidateManifest(t *testing.T) {
 					},
 					VolumeMounts: []VolumeMount{
 						{Name: "vol1", MountPath: "/foo"},
-						{Name: "vol1", Path: "/bar"},
+						{Name: "vol1", MountPath: "/bar"},
 					},
 				},
 			},

pkg/kubelet/kubelet.go

@@ -198,29 +198,20 @@ func makeEnvironmentVariables(container *api.Container) []string {
 	return result
 }
 
-func makeVolumesAndBinds(pod *Pod, container *api.Container, podVolumes volumeMap) (map[string]struct{}, []string) {
-	volumes := map[string]struct{}{}
+func makeBinds(pod *Pod, container *api.Container, podVolumes volumeMap) []string {
 	binds := []string{}
-	for _, volume := range container.VolumeMounts {
-		var basePath string
-		if vol, ok := podVolumes[volume.Name]; ok {
-			// Host volumes are not Docker volumes and are directly mounted from the host.
-			basePath = fmt.Sprintf("%s:%s", vol.GetPath(), volume.MountPath)
-		} else if volume.MountType == "HOST" {
-			// DEPRECATED: VolumeMount.MountType will be handled by the Volume struct.
-			basePath = fmt.Sprintf("%s:%s", volume.MountPath, volume.MountPath)
-		} else {
-			// TODO(jonesdl) This clause should be deleted and an error should be thrown. The default
-			// behavior is now supported by the EmptyDirectory type.
-			volumes[volume.MountPath] = struct{}{}
-			basePath = fmt.Sprintf("/exports/%s/%s:%s", GetPodFullName(pod), volume.Name, volume.MountPath)
+	for _, mount := range container.VolumeMounts {
+		vol, ok := podVolumes[mount.Name]
+		if !ok {
+			continue
 		}
-		if volume.ReadOnly {
-			basePath += ":ro"
+		b := fmt.Sprintf("%s:%s", vol.GetPath(), mount.MountPath)
+		if mount.ReadOnly {
+			b += ":ro"
 		}
-		binds = append(binds, basePath)
+		binds = append(binds, b)
 	}
-	return volumes, binds
+	return binds
 }
 
 func makePortsAndBindings(container *api.Container) (map[docker.Port]struct{}, map[docker.Port][]docker.PortBinding) {
@@ -294,7 +285,7 @@ func (kl *Kubelet) mountExternalVolumes(manifest *api.ContainerManifest) (volume
 // Run a single container from a pod. Returns the docker container ID
 func (kl *Kubelet) runContainer(pod *Pod, container *api.Container, podVolumes volumeMap, netMode string) (id DockerID, err error) {
 	envVariables := makeEnvironmentVariables(container)
-	volumes, binds := makeVolumesAndBinds(pod, container, podVolumes)
+	binds := makeBinds(pod, container, podVolumes)
 	exposedPorts, portBindings := makePortsAndBindings(container)
 
 	opts := docker.CreateContainerOptions{
@@ -307,7 +298,6 @@ func (kl *Kubelet) runContainer(pod *Pod, container *api.Container, podVolumes v
 			Image:        container.Image,
 			Memory:       int64(container.Memory),
 			CpuShares:    int64(milliCPUToShares(container.CPU)),
-			Volumes:      volumes,
 			WorkingDir:   container.WorkingDir,
 		},
 	}

pkg/kubelet/kubelet_test.go

@@ -671,17 +671,10 @@ func TestMakeVolumesAndBinds(t *testing.T) {
 				Name:      "disk",
 				ReadOnly:  false,
 			},
-			{
-				MountPath: "/mnt/path2",
-				Name:      "disk2",
-				ReadOnly:  true,
-				MountType: "LOCAL",
-			},
 			{
 				MountPath: "/mnt/path3",
-				Name:      "disk3",
-				ReadOnly:  false,
-				MountType: "HOST",
+				Name:      "disk",
+				ReadOnly:  true,
 			},
 			{
 				MountPath: "/mnt/path4",
@@ -701,24 +694,21 @@ func TestMakeVolumesAndBinds(t *testing.T) {
 		Namespace: "test",
 	}
 
-	podVolumes := make(volumeMap)
-	podVolumes["disk4"] = &volume.HostDirectory{"/mnt/host"}
-	podVolumes["disk5"] = &volume.EmptyDirectory{"disk5", "podID", "/var/lib/kubelet"}
-
-	volumes, binds := makeVolumesAndBinds(&pod, &container, podVolumes)
-
-	expectedVolumes := []string{"/mnt/path", "/mnt/path2"}
-	expectedBinds := []string{"/exports/pod.test/disk:/mnt/path", "/exports/pod.test/disk2:/mnt/path2:ro", "/mnt/path3:/mnt/path3",
-		"/mnt/host:/mnt/path4", "/var/lib/kubelet/podID/volumes/empty/disk5:/mnt/path5"}
-
-	if len(volumes) != len(expectedVolumes) {
-		t.Errorf("Unexpected volumes. Expected %#v got %#v.  Container was: %#v", expectedVolumes, volumes, container)
+	podVolumes := volumeMap{
+		"disk":  &volume.HostDirectory{"/mnt/disk"},
+		"disk4": &volume.HostDirectory{"/mnt/host"},
+		"disk5": &volume.EmptyDirectory{"disk5", "podID", "/var/lib/kubelet"},
 	}
-	for _, expectedVolume := range expectedVolumes {
-		if _, ok := volumes[expectedVolume]; !ok {
-			t.Errorf("Volumes map is missing key: %s. %#v", expectedVolume, volumes)
-		}
+
+	binds := makeBinds(&pod, &container, podVolumes)
+
+	expectedBinds := []string{
+		"/mnt/disk:/mnt/path",
+		"/mnt/disk:/mnt/path3:ro",
+		"/mnt/host:/mnt/path4",
+		"/var/lib/kubelet/podID/volumes/empty/disk5:/mnt/path5",
 	}
+
 	if len(binds) != len(expectedBinds) {
 		t.Errorf("Unexpected binds: Expected %#v got %#v.  Container was: %#v", expectedBinds, binds, container)
 	}