Merge pull request #8860 from wojtek-t/replace_deep_copy

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

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

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
 			}

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
 }
 

pkg/conversion/deep_copy.go

-/*
-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
-	}
-}

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

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")
 	}

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

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)
 	}

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