From 2a62d721401600f8036173c3b8eeb2ec2f23a093 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Tue, 26 Aug 2014 17:28:36 -0700 Subject: [PATCH 1/2] Cleanup: remove deprecated fields the proper way --- pkg/api/conversion.go | 66 ++++++++++++++++++++++ pkg/api/conversion_test.go | 112 +++++++++++++++++++++++++++++++++++++ pkg/api/helper.go | 27 +-------- pkg/api/types.go | 6 -- pkg/api/validation.go | 13 +---- pkg/api/validation_test.go | 50 +---------------- 6 files changed, 181 insertions(+), 93 deletions(-) create mode 100644 pkg/api/conversion.go create mode 100644 pkg/api/conversion_test.go diff --git a/pkg/api/conversion.go b/pkg/api/conversion.go new file mode 100644 index 00000000000..6e18834ba52 --- /dev/null +++ b/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 + }, + ) + +} diff --git a/pkg/api/conversion_test.go b/pkg/api/conversion_test.go new file mode 100644 index 00000000000..bcfebe8470f --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/api/helper.go b/pkg/api/helper.go index a4a067fb2bd..db85cd32a9f 100644 --- a/pkg/api/helper.go +++ b/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() } diff --git a/pkg/api/types.go b/pkg/api/types.go index b1dfaeb3846..9e3f61845e9 100644 --- a/pkg/api/types.go +++ b/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 diff --git a/pkg/api/validation.go b/pkg/api/validation.go index 0dd02dea649..3761795b340 100644 --- a/pkg/api/validation.go +++ b/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)...) } diff --git a/pkg/api/validation_test.go b/pkg/api/validation_test.go index 50c9cf4fab5..5e599b85017 100644 --- a/pkg/api/validation_test.go +++ b/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"}, }, }, }, From 97b05619f1b96a860f7dd6990243f4475bb3c33b Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Tue, 26 Aug 2014 22:08:06 -0700 Subject: [PATCH 2/2] Remove deprecated bits from kubelet --- pkg/kubelet/kubelet.go | 32 ++++++++++------------------- pkg/kubelet/kubelet_test.go | 40 ++++++++++++++----------------------- 2 files changed, 26 insertions(+), 46 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 9691ebf7ef1..f6bc2734fcb 100644 --- a/pkg/kubelet/kubelet.go +++ b/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, }, } diff --git a/pkg/kubelet/kubelet_test.go b/pkg/kubelet/kubelet_test.go index 71ae0c83f49..bdbeeb9e886 100644 --- a/pkg/kubelet/kubelet_test.go +++ b/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) }