From 33318f0162e3c3f10aae7b249883587ad6e1b269 Mon Sep 17 00:00:00 2001 From: Wojciech Tyczynski Date: Wed, 13 May 2015 15:52:53 +0200 Subject: [PATCH] Use generated DeepCopy methods. --- hooks/pre-commit | 12 ++ pkg/api/copy_test.go | 3 +- .../framework/fake_controller_source.go | 5 +- pkg/conversion/cloner.go | 8 +- pkg/conversion/deep_copy.go | 114 ------------------ pkg/conversion/deep_copy_test.go | 8 +- pkg/registry/service/rest_test.go | 3 +- pkg/runtime/deep_copy_generator.go | 5 + pkg/runtime/embedded_test.go | 4 +- pkg/tools/etcd_helper.go | 5 +- 10 files changed, 36 insertions(+), 131 deletions(-) delete mode 100644 pkg/conversion/deep_copy.go diff --git a/hooks/pre-commit b/hooks/pre-commit index 56a5cff2fbb2d..a9855c5480ee6 100755 --- a/hooks/pre-commit +++ b/hooks/pre-commit @@ -111,4 +111,16 @@ else fi echo "${reset}" +echo -ne "Checking for deep-copies that need updating... " +if ! hack/verify-generated-deep-copies.sh > /dev/null; then + echo "${red}ERROR!" + echo "Some deep-copy functions need regeneration." + echo "To regenerate deep-copies, run:" + echo " hack/update-generated-deep-copies.sh" + exit_code=1 +else + echo "${green}OK" +fi +echo "${reset}" + exit $exit_code diff --git a/pkg/api/copy_test.go b/pkg/api/copy_test.go index 08e4a7ab5eff5..347d20e2bc670 100644 --- a/pkg/api/copy_test.go +++ b/pkg/api/copy_test.go @@ -24,7 +24,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi" apitesting "github.com/GoogleCloudPlatform/kubernetes/pkg/api/testing" - "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" ) func TestDeepCopyApiObjects(t *testing.T) { @@ -37,7 +36,7 @@ func TestDeepCopyApiObjects(t *testing.T) { t.Fatalf("Could not create a %s: %s", kind, err) } f.Fuzz(item) - itemCopy, err := conversion.DeepCopy(item) + itemCopy, err := api.Scheme.DeepCopy(item) if err != nil { t.Errorf("Could not deep copy a %s: %s", kind, err) continue diff --git a/pkg/controller/framework/fake_controller_source.go b/pkg/controller/framework/fake_controller_source.go index f2b2da1276d16..dcbaff9cbf4da 100644 --- a/pkg/controller/framework/fake_controller_source.go +++ b/pkg/controller/framework/fake_controller_source.go @@ -23,7 +23,6 @@ import ( "sync" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/types" "github.com/GoogleCloudPlatform/kubernetes/pkg/watch" @@ -130,7 +129,7 @@ func (f *FakeControllerSource) List() (runtime.Object, error) { // Otherwise, if they make a change and write it back, they // will inadvertently change the our canonical copy (in // addition to racing with other clients). - objCopy, err := conversion.DeepCopy(obj) + objCopy, err := api.Scheme.DeepCopy(obj) if err != nil { return nil, err } @@ -167,7 +166,7 @@ func (f *FakeControllerSource) Watch(resourceVersion string) (watch.Interface, e // it back, they will inadvertently change the our // canonical copy (in addition to racing with other // clients). - objCopy, err := conversion.DeepCopy(c.Object) + objCopy, err := api.Scheme.DeepCopy(c.Object) if err != nil { return nil, err } diff --git a/pkg/conversion/cloner.go b/pkg/conversion/cloner.go index d42922159dcce..3afafef05d10a 100644 --- a/pkg/conversion/cloner.go +++ b/pkg/conversion/cloner.go @@ -43,8 +43,12 @@ func NewCloner() *Cloner { // Prevent recursing into every byte... func byteSliceDeepCopy(in []byte, out *[]byte, c *Cloner) error { - *out = make([]byte, len(in)) - copy(*out, in) + if in != nil { + *out = make([]byte, len(in)) + copy(*out, in) + } else { + *out = nil + } return nil } diff --git a/pkg/conversion/deep_copy.go b/pkg/conversion/deep_copy.go deleted file mode 100644 index 4927ecfba8805..0000000000000 --- a/pkg/conversion/deep_copy.go +++ /dev/null @@ -1,114 +0,0 @@ -/* -Copyright 2015 The Kubernetes Authors 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 conversion - -import ( - "fmt" - "reflect" -) - -// DeepCopy makes a deep copy of source or returns an error. -func DeepCopy(source interface{}) (interface{}, error) { - v, err := deepCopy(reflect.ValueOf(source)) - return v.Interface(), err -} - -func deepCopy(src reflect.Value) (reflect.Value, error) { - switch src.Kind() { - case reflect.Chan, reflect.Func, reflect.UnsafePointer, reflect.Uintptr: - return src, fmt.Errorf("cannot deep copy kind: %s", src.Kind()) - - case reflect.Array: - dst := reflect.New(src.Type()) - for i := 0; i < src.Len(); i++ { - copyVal, err := deepCopy(src.Index(i)) - if err != nil { - return src, err - } - dst.Elem().Index(i).Set(copyVal) - } - return dst.Elem(), nil - - case reflect.Interface: - if src.IsNil() { - return src, nil - } - return deepCopy(src.Elem()) - - case reflect.Map: - if src.IsNil() { - return src, nil - } - dst := reflect.MakeMap(src.Type()) - for _, k := range src.MapKeys() { - copyVal, err := deepCopy(src.MapIndex(k)) - if err != nil { - return src, err - } - dst.SetMapIndex(k, copyVal) - } - return dst, nil - - case reflect.Ptr: - if src.IsNil() { - return src, nil - } - dst := reflect.New(src.Type().Elem()) - copyVal, err := deepCopy(src.Elem()) - if err != nil { - return src, err - } - dst.Elem().Set(copyVal) - return dst, nil - - case reflect.Slice: - if src.IsNil() { - return src, nil - } - dst := reflect.MakeSlice(src.Type(), 0, src.Len()) - for i := 0; i < src.Len(); i++ { - copyVal, err := deepCopy(src.Index(i)) - if err != nil { - return src, err - } - dst = reflect.Append(dst, copyVal) - } - return dst, nil - - case reflect.Struct: - dst := reflect.New(src.Type()) - for i := 0; i < src.NumField(); i++ { - if !dst.Elem().Field(i).CanSet() { - // Can't set private fields. At this point, the - // best we can do is a shallow copy. For - // example, time.Time is a value type with - // private members that can be shallow copied. - return src, nil - } - copyVal, err := deepCopy(src.Field(i)) - if err != nil { - return src, err - } - dst.Elem().Field(i).Set(copyVal) - } - return dst.Elem(), nil - - default: - // Value types like numbers, booleans, and strings. - return src, nil - } -} diff --git a/pkg/conversion/deep_copy_test.go b/pkg/conversion/deep_copy_test.go index 9c4d78d0a3b49..a1cd65308ad50 100644 --- a/pkg/conversion/deep_copy_test.go +++ b/pkg/conversion/deep_copy_test.go @@ -40,7 +40,7 @@ func TestDeepCopy(t *testing.T) { }{}, } for _, obj := range table { - obj2, err := DeepCopy(obj) + obj2, err := NewCloner().DeepCopy(obj) if err != nil { t.Errorf("Error: couldn't copy %#v", obj) continue @@ -51,7 +51,7 @@ func TestDeepCopy(t *testing.T) { obj3 := reflect.New(reflect.TypeOf(obj)).Interface() f.Fuzz(obj3) - obj4, err := DeepCopy(obj3) + obj4, err := NewCloner().DeepCopy(obj3) if err != nil { t.Errorf("Error: couldn't copy %#v", obj) continue @@ -64,7 +64,7 @@ func TestDeepCopy(t *testing.T) { } func copyOrDie(t *testing.T, in interface{}) interface{} { - out, err := DeepCopy(in) + out, err := NewCloner().DeepCopy(in) if err != nil { t.Fatalf("DeepCopy failed: %#q: %v", in, err) } @@ -154,7 +154,7 @@ func BenchmarkDeepCopy(b *testing.B) { var r interface{} for i := 0; i < b.N; i++ { for j := range table { - r, _ = DeepCopy(table[j]) + r, _ = NewCloner().DeepCopy(table[j]) } } result = r diff --git a/pkg/registry/service/rest_test.go b/pkg/registry/service/rest_test.go index 4ab1310b99498..ec658d349bb4b 100644 --- a/pkg/registry/service/rest_test.go +++ b/pkg/registry/service/rest_test.go @@ -25,7 +25,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest/resttest" - "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" "github.com/GoogleCloudPlatform/kubernetes/pkg/fields" "github.com/GoogleCloudPlatform/kubernetes/pkg/labels" "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/registrytest" @@ -60,7 +59,7 @@ func makeIPNet(t *testing.T) *net.IPNet { } func deepCloneService(svc *api.Service) *api.Service { - value, err := conversion.DeepCopy(svc) + value, err := api.Scheme.DeepCopy(svc) if err != nil { panic("couldn't copy service") } diff --git a/pkg/runtime/deep_copy_generator.go b/pkg/runtime/deep_copy_generator.go index 1c35633c2e895..7be7af6bd86c0 100644 --- a/pkg/runtime/deep_copy_generator.go +++ b/pkg/runtime/deep_copy_generator.go @@ -27,6 +27,11 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) +// TODO(wojtek-t): As suggested in #8320, we should consider the strategy +// to first do the shallow copy and then recurse into things that need a +// deep copy (maps, pointers, slices). That sort of copy function would +// need one parameter - a pointer to the thing it's supposed to expand, +// and it would involve a lot less memory copying. type DeepCopyGenerator interface { // Adds a type to a generator. // If the type is non-struct, it will return an error, otherwise deep-copy diff --git a/pkg/runtime/embedded_test.go b/pkg/runtime/embedded_test.go index 6f7ea92dd36fb..8341fd9d666fe 100644 --- a/pkg/runtime/embedded_test.go +++ b/pkg/runtime/embedded_test.go @@ -21,7 +21,7 @@ import ( "reflect" "testing" - "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" ) @@ -224,7 +224,7 @@ func TestDeepCopyOfEmbeddedObject(t *testing.T) { } t.Logf("originalRole = %v\n", string(originalData)) - copyOfOriginal, err := conversion.DeepCopy(original) + copyOfOriginal, err := api.Scheme.DeepCopy(original) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/tools/etcd_helper.go b/pkg/tools/etcd_helper.go index 35351807a0441..7446d889d203a 100644 --- a/pkg/tools/etcd_helper.go +++ b/pkg/tools/etcd_helper.go @@ -27,6 +27,7 @@ import ( "strings" "time" + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/conversion" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" @@ -238,7 +239,7 @@ func (h *EtcdHelper) getFromCache(index uint64) (runtime.Object, bool) { if found { // We should not return the object itself to avoid poluting the cache if someone // modifies returned values. - objCopy, err := conversion.DeepCopy(obj) + objCopy, err := api.Scheme.DeepCopy(obj) trace.Step("Deep copied") if err != nil { glog.Errorf("Error during DeepCopy of cached object: %q", err) @@ -256,7 +257,7 @@ func (h *EtcdHelper) addToCache(index uint64, obj runtime.Object) { defer func() { cacheAddLatency.Observe(float64(time.Since(startTime) / time.Microsecond)) }() - objCopy, err := conversion.DeepCopy(obj) + objCopy, err := api.Scheme.DeepCopy(obj) if err != nil { glog.Errorf("Error during DeepCopy of cached object: %q", err) return