From 9131fbd4463956c2235d09eb9c1a4e3c25ab9624 Mon Sep 17 00:00:00 2001 From: Hongchao Deng Date: Thu, 25 Aug 2016 20:41:58 -0700 Subject: [PATCH] refactor destroy func in unit testing --- .../experimental/controller/etcd/etcd_test.go | 23 ++-- .../generic/registry/storage_factory.go | 3 +- pkg/registry/generic/registry/store_test.go | 114 ++++++------------ pkg/registry/generic/storage_decorator.go | 6 +- .../service/ipallocator/etcd/etcd_test.go | 30 ++--- pkg/storage/storagebackend/factory/etcd2.go | 2 +- pkg/storage/storagebackend/factory/etcd3.go | 2 +- pkg/storage/storagebackend/factory/factory.go | 5 +- 8 files changed, 75 insertions(+), 110 deletions(-) diff --git a/pkg/registry/experimental/controller/etcd/etcd_test.go b/pkg/registry/experimental/controller/etcd/etcd_test.go index a96b55f4ec0..dfb6e2d2ccd 100644 --- a/pkg/registry/experimental/controller/etcd/etcd_test.go +++ b/pkg/registry/experimental/controller/etcd/etcd_test.go @@ -28,12 +28,17 @@ import ( "k8s.io/kubernetes/pkg/storage" "k8s.io/kubernetes/pkg/storage/etcd/etcdtest" etcdtesting "k8s.io/kubernetes/pkg/storage/etcd/testing" + "k8s.io/kubernetes/pkg/storage/storagebackend/factory" ) -func newStorage(t *testing.T) (*ScaleREST, *etcdtesting.EtcdTestServer, storage.Interface, func()) { +func newStorage(t *testing.T) (*ScaleREST, *etcdtesting.EtcdTestServer, storage.Interface, factory.DestroyFunc) { etcdStorage, server := registrytest.NewEtcdStorage(t, "") restOptions := generic.RESTOptions{StorageConfig: etcdStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: 1, ResourcePrefix: "controllers"} - s, destroyFunc := generic.NewRawStorage(etcdStorage) + s, d := generic.NewRawStorage(etcdStorage) + destroyFunc := func() { + d() + server.Terminate(t) + } return NewStorage(restOptions).Scale, server, s, destroyFunc } @@ -83,11 +88,8 @@ var validScale = extensions.Scale{ } func TestGet(t *testing.T) { - storage, server, si, destroyFunc := newStorage(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + storage, _, si, destroyFunc := newStorage(t) + defer destroyFunc() ctx := api.WithNamespace(api.NewContext(), "test") key := etcdtest.AddPrefix("/controllers/test/foo") @@ -105,11 +107,8 @@ func TestGet(t *testing.T) { } func TestUpdate(t *testing.T) { - storage, server, si, destroyFunc := newStorage(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + storage, _, si, destroyFunc := newStorage(t) + defer destroyFunc() ctx := api.WithNamespace(api.NewContext(), "test") key := etcdtest.AddPrefix("/controllers/test/foo") diff --git a/pkg/registry/generic/registry/storage_factory.go b/pkg/registry/generic/registry/storage_factory.go index 53cb46d3bca..4d006836a87 100644 --- a/pkg/registry/generic/registry/storage_factory.go +++ b/pkg/registry/generic/registry/storage_factory.go @@ -23,6 +23,7 @@ import ( "k8s.io/kubernetes/pkg/storage" etcdstorage "k8s.io/kubernetes/pkg/storage/etcd" "k8s.io/kubernetes/pkg/storage/storagebackend" + "k8s.io/kubernetes/pkg/storage/storagebackend/factory" ) // Creates a cacher based given storageConfig. @@ -33,7 +34,7 @@ func StorageWithCacher( resourcePrefix string, scopeStrategy rest.NamespaceScopedStrategy, newListFunc func() runtime.Object, - triggerFunc storage.TriggerPublisherFunc) (storage.Interface, func()) { + triggerFunc storage.TriggerPublisherFunc) (storage.Interface, factory.DestroyFunc) { s, d := generic.NewRawStorage(storageConfig) // TODO: we would change this later to make storage always have cacher and hide low level KV layer inside. diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index e77212bdcb8..871a20ad51c 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -40,6 +40,7 @@ import ( etcdstorage "k8s.io/kubernetes/pkg/storage/etcd" "k8s.io/kubernetes/pkg/storage/etcd/etcdtest" etcdtesting "k8s.io/kubernetes/pkg/storage/etcd/testing" + "k8s.io/kubernetes/pkg/storage/storagebackend/factory" storagetesting "k8s.io/kubernetes/pkg/storage/testing" "k8s.io/kubernetes/pkg/util/sets" "k8s.io/kubernetes/pkg/util/validation/field" @@ -88,7 +89,7 @@ func (t *testRESTStrategy) ValidateUpdate(ctx api.Context, obj, old runtime.Obje } func (t *testRESTStrategy) Canonicalize(obj runtime.Object) {} -func NewTestGenericStoreRegistry(t *testing.T) (*etcdtesting.EtcdTestServer, func(), *Store) { +func NewTestGenericStoreRegistry(t *testing.T) (factory.DestroyFunc, *Store) { return newTestGenericStoreRegistry(t, false) } @@ -173,7 +174,7 @@ func TestStoreList(t *testing.T) { if item.context != nil { ctx = item.context } - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) + destroyFunc, registry := NewTestGenericStoreRegistry(t) if item.in != nil { if err := storagetesting.CreateList("/pods", registry.Storage, item.in); err != nil { @@ -192,7 +193,6 @@ func TestStoreList(t *testing.T) { t.Errorf("%v: Expected %#v, got %#v", name, e, a) } destroyFunc() - server.Terminate(t) } } @@ -209,11 +209,8 @@ func TestStoreListResourceVersion(t *testing.T) { } ctx := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := newTestGenericStoreRegistry(t, true) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := newTestGenericStoreRegistry(t, true) + defer destroyFunc() obj, err := registry.Create(ctx, fooPod) if err != nil { @@ -274,11 +271,8 @@ func TestStoreCreate(t *testing.T) { } testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() // create the object objA, err := registry.Create(testContext, podA) @@ -337,11 +331,8 @@ func TestStoreUpdate(t *testing.T) { } testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() // Test1 try to update a non-existing node _, _, err := registry.Update(testContext, podA.Name, rest.DefaultUpdatedObjectInfo(podA, api.Scheme)) @@ -378,11 +369,8 @@ func TestStoreUpdate(t *testing.T) { } func TestNoOpUpdates(t *testing.T) { - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() newPod := func() *api.Pod { return &api.Pod{ @@ -463,11 +451,8 @@ func TestStoreCustomExport(t *testing.T) { Spec: api.PodSpec{NodeName: "machine"}, } - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() registry.ExportStrategy = testPodExport{} @@ -512,11 +497,8 @@ func TestStoreBasicExport(t *testing.T) { Status: api.PodStatus{HostIP: "1.2.3.4"}, } - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() testContext := api.WithNamespace(api.NewContext(), "test") registry.UpdateStrategy.(*testRESTStrategy).allowCreateOnUpdate = true @@ -547,11 +529,8 @@ func TestStoreGet(t *testing.T) { } testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() _, err := registry.Get(testContext, podA.Name) if !errors.IsNotFound(err) { @@ -571,11 +550,8 @@ func TestStoreDelete(t *testing.T) { } testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() // test failure condition _, err := registry.Delete(testContext, podA.Name, nil) @@ -612,11 +588,8 @@ func TestStoreHandleFinalizers(t *testing.T) { } testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() // create pod _, err := registry.Create(testContext, podWithFinalizer) if err != nil { @@ -911,11 +884,8 @@ func TestStoreDeleteWithOrphanDependents(t *testing.T) { } testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() for _, tc := range testcases { registry.DeleteStrategy = tc.strategy @@ -961,11 +931,8 @@ func TestStoreDeleteCollection(t *testing.T) { podB := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "bar"}} testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() if _, err := registry.Create(testContext, podA); err != nil { t.Errorf("Unexpected error: %v", err) @@ -993,11 +960,8 @@ func TestStoreDeleteCollection(t *testing.T) { } func TestStoreDeleteCollectionNotFound(t *testing.T) { - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() testContext := api.WithNamespace(api.NewContext(), "test") @@ -1042,11 +1006,8 @@ func TestStoreDeleteCollectionWithWatch(t *testing.T) { podA := &api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}} testContext := api.WithNamespace(api.NewContext(), "test") - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() objCreated, err := registry.Create(testContext, podA) if err != nil { @@ -1112,7 +1073,7 @@ func TestStoreWatch(t *testing.T) { Spec: api.PodSpec{NodeName: "machine"}, } - server, destroyFunc, registry := NewTestGenericStoreRegistry(t) + destroyFunc, registry := NewTestGenericStoreRegistry(t) wi, err := registry.WatchPredicate(ctx, m.selectPred, "0") if err != nil { t.Errorf("%v: unexpected error: %v", name, err) @@ -1131,18 +1092,19 @@ func TestStoreWatch(t *testing.T) { wi.Stop() } destroyFunc() - server.Terminate(t) } } -func newTestGenericStoreRegistry(t *testing.T, hasCacheEnabled bool) (*etcdtesting.EtcdTestServer, func(), *Store) { +func newTestGenericStoreRegistry(t *testing.T, hasCacheEnabled bool) (factory.DestroyFunc, *Store) { podPrefix := "/pods" server := etcdtesting.NewEtcdTestClientServer(t) strategy := &testRESTStrategy{api.Scheme, api.SimpleNameGenerator, true, false, true} codec := testapi.Default.StorageCodec() s := etcdstorage.NewEtcdStorage(server.Client, codec, etcdtest.PathPrefix(), false, etcdtest.DeserializationCacheSize) - destroyFunc := func() {} + destroyFunc := func() { + server.Terminate(t) + } if hasCacheEnabled { config := storage.CacherConfig{ CacheCapacity: 10, @@ -1155,11 +1117,15 @@ func newTestGenericStoreRegistry(t *testing.T, hasCacheEnabled bool) (*etcdtesti Codec: codec, } cacher := storage.NewCacherFromConfig(config) + d := destroyFunc s = cacher - destroyFunc = cacher.Stop + destroyFunc = func() { + cacher.Stop() + d() + } } - return server, destroyFunc, &Store{ + return destroyFunc, &Store{ NewFunc: func() runtime.Object { return &api.Pod{} }, NewListFunc: func() runtime.Object { return &api.PodList{} }, QualifiedResource: api.Resource("pods"), diff --git a/pkg/registry/generic/storage_decorator.go b/pkg/registry/generic/storage_decorator.go index 9012c495202..0d1511cab25 100644 --- a/pkg/registry/generic/storage_decorator.go +++ b/pkg/registry/generic/storage_decorator.go @@ -34,7 +34,7 @@ type StorageDecorator func( resourcePrefix string, scopeStrategy rest.NamespaceScopedStrategy, newListFunc func() runtime.Object, - trigger storage.TriggerPublisherFunc) (storage.Interface, func()) + trigger storage.TriggerPublisherFunc) (storage.Interface, factory.DestroyFunc) // Returns given 'storageInterface' without any decoration. func UndecoratedStorage( @@ -44,14 +44,14 @@ func UndecoratedStorage( resourcePrefix string, scopeStrategy rest.NamespaceScopedStrategy, newListFunc func() runtime.Object, - trigger storage.TriggerPublisherFunc) (storage.Interface, func()) { + trigger storage.TriggerPublisherFunc) (storage.Interface, factory.DestroyFunc) { return NewRawStorage(config) } // NewRawStorage creates the low level kv storage. This is a work-around for current // two layer of same storage interface. // TODO: Once cacher is enabled on all registries (event registry is special), we will remove this method. -func NewRawStorage(config *storagebackend.Config) (storage.Interface, func()) { +func NewRawStorage(config *storagebackend.Config) (storage.Interface, factory.DestroyFunc) { s, d, err := factory.Create(*config) if err != nil { glog.Fatalf("Unable to create storage backend: config (%v), err (%v)", config, err) diff --git a/pkg/registry/service/ipallocator/etcd/etcd_test.go b/pkg/registry/service/ipallocator/etcd/etcd_test.go index d309c4cc058..824b386f064 100644 --- a/pkg/registry/service/ipallocator/etcd/etcd_test.go +++ b/pkg/registry/service/ipallocator/etcd/etcd_test.go @@ -30,11 +30,12 @@ import ( "k8s.io/kubernetes/pkg/storage" "k8s.io/kubernetes/pkg/storage/etcd/etcdtest" etcdtesting "k8s.io/kubernetes/pkg/storage/etcd/testing" + "k8s.io/kubernetes/pkg/storage/storagebackend/factory" "golang.org/x/net/context" ) -func newStorage(t *testing.T) (*etcdtesting.EtcdTestServer, ipallocator.Interface, allocator.Interface, storage.Interface, func()) { +func newStorage(t *testing.T) (*etcdtesting.EtcdTestServer, ipallocator.Interface, allocator.Interface, storage.Interface, factory.DestroyFunc) { etcdStorage, server := registrytest.NewEtcdStorage(t, "") _, cidr, err := net.ParseCIDR("192.168.1.0/24") if err != nil { @@ -48,7 +49,11 @@ func newStorage(t *testing.T) (*etcdtesting.EtcdTestServer, ipallocator.Interfac etcd := allocatoretcd.NewEtcd(mem, "/ranges/serviceips", api.Resource("serviceipallocations"), etcdStorage) return etcd }) - s, destroyFunc := generic.NewRawStorage(etcdStorage) + s, d := generic.NewRawStorage(etcdStorage) + destroyFunc := func() { + d() + server.Terminate(t) + } return server, storage, backing, s, destroyFunc } @@ -65,33 +70,24 @@ func key() string { } func TestEmpty(t *testing.T) { - server, storage, _, _, destroyFunc := newStorage(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + _, storage, _, _, destroyFunc := newStorage(t) + defer destroyFunc() if err := storage.Allocate(net.ParseIP("192.168.1.2")); !strings.Contains(err.Error(), "cannot allocate resources of type serviceipallocations at this time") { t.Fatal(err) } } func TestErrors(t *testing.T) { - server, storage, _, _, destroyFunc := newStorage(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + _, storage, _, _, destroyFunc := newStorage(t) + defer destroyFunc() if err := storage.Allocate(net.ParseIP("192.168.0.0")); err != ipallocator.ErrNotInRange { t.Fatal(err) } } func TestStore(t *testing.T) { - server, storage, backing, si, destroyFunc := newStorage(t) - defer func() { - destroyFunc() - server.Terminate(t) - }() + _, storage, backing, si, destroyFunc := newStorage(t) + defer destroyFunc() if err := si.Create(context.TODO(), key(), validNewRangeAllocation(), nil, 0); err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/storage/storagebackend/factory/etcd2.go b/pkg/storage/storagebackend/factory/etcd2.go index 01a7ec4f48a..8a572460339 100644 --- a/pkg/storage/storagebackend/factory/etcd2.go +++ b/pkg/storage/storagebackend/factory/etcd2.go @@ -30,7 +30,7 @@ import ( utilnet "k8s.io/kubernetes/pkg/util/net" ) -func newETCD2Storage(c storagebackend.Config) (storage.Interface, func(), error) { +func newETCD2Storage(c storagebackend.Config) (storage.Interface, DestroyFunc, error) { tr, err := newTransportForETCD2(c.CertFile, c.KeyFile, c.CAFile) if err != nil { return nil, nil, err diff --git a/pkg/storage/storagebackend/factory/etcd3.go b/pkg/storage/storagebackend/factory/etcd3.go index eca73112127..ee5d1d93263 100644 --- a/pkg/storage/storagebackend/factory/etcd3.go +++ b/pkg/storage/storagebackend/factory/etcd3.go @@ -26,7 +26,7 @@ import ( "golang.org/x/net/context" ) -func newETCD3Storage(c storagebackend.Config) (storage.Interface, func(), error) { +func newETCD3Storage(c storagebackend.Config) (storage.Interface, DestroyFunc, error) { tlsInfo := transport.TLSInfo{ CertFile: c.CertFile, KeyFile: c.KeyFile, diff --git a/pkg/storage/storagebackend/factory/factory.go b/pkg/storage/storagebackend/factory/factory.go index d7ee038128f..647f4f9f5b1 100644 --- a/pkg/storage/storagebackend/factory/factory.go +++ b/pkg/storage/storagebackend/factory/factory.go @@ -23,8 +23,11 @@ import ( "k8s.io/kubernetes/pkg/storage/storagebackend" ) +// DestroyFunc is to destroy any resources used by the storage returned in Create() together. +type DestroyFunc func() + // Create creates a storage backend based on given config. -func Create(c storagebackend.Config) (storage.Interface, func(), error) { +func Create(c storagebackend.Config) (storage.Interface, DestroyFunc, error) { switch c.Type { case storagebackend.StorageTypeUnset, storagebackend.StorageTypeETCD2: return newETCD2Storage(c)