From 9a3add5c6f7cfc9e8eada34ad45f361238b55cb8 Mon Sep 17 00:00:00 2001 From: Jason Yellick Date: Thu, 26 Apr 2018 17:39:01 -0400 Subject: [PATCH 1/2] FAB-9760 Remove unused prelaunch func The Prelaunch field can never be set, and complicates our interfaces and tests. Removing. Change-Id: I2cb316550104e8b33ba8abfa5b43dd3efb27b507 Signed-off-by: Jason Yellick --- core/container/api/core.go | 3 +- core/container/controller.go | 3 +- .../dockercontroller/dockercontroller.go | 8 +--- .../dockercontroller/dockercontroller_test.go | 38 +++++-------------- .../inproccontroller/inproccontroller.go | 8 +--- .../inproccontroller/inproccontroller_test.go | 2 +- 6 files changed, 15 insertions(+), 47 deletions(-) diff --git a/core/container/api/core.go b/core/container/api/core.go index c30f90d5a58..0acaf87a74f 100644 --- a/core/container/api/core.go +++ b/core/container/api/core.go @@ -25,12 +25,11 @@ import ( ) type BuildSpecFactory func() (io.Reader, error) -type PrelaunchFunc func() error //VM is an abstract virtual image for supporting arbitrary virual machines type VM interface { Deploy(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, reader io.Reader) error - Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder BuildSpecFactory, preLaunchFunc PrelaunchFunc) error + Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder BuildSpecFactory) error Stop(ctxt context.Context, ccid ccintf.CCID, timeout uint, dontkill bool, dontremove bool) error Destroy(ctxt context.Context, ccid ccintf.CCID, force bool, noprune bool) error GetVMName(ccID ccintf.CCID, format func(string) (string, error)) (string, error) diff --git a/core/container/controller.go b/core/container/controller.go index 1117ce62d8b..141ad8e93e8 100644 --- a/core/container/controller.go +++ b/core/container/controller.go @@ -126,13 +126,12 @@ type StartContainerReq struct { Args []string Env []string FilesToUpload map[string][]byte - PrelaunchFunc api.PrelaunchFunc } func (si StartContainerReq) do(ctxt context.Context, v api.VM) VMCResp { var resp VMCResp - if err := v.Start(ctxt, si.CCID, si.Args, si.Env, si.FilesToUpload, si.Builder, si.PrelaunchFunc); err != nil { + if err := v.Start(ctxt, si.CCID, si.Args, si.Env, si.FilesToUpload, si.Builder); err != nil { resp = VMCResp{Err: err} } else { resp = VMCResp{} diff --git a/core/container/dockercontroller/dockercontroller.go b/core/container/dockercontroller/dockercontroller.go index 7779980aa2f..5434e33d144 100644 --- a/core/container/dockercontroller/dockercontroller.go +++ b/core/container/dockercontroller/dockercontroller.go @@ -229,7 +229,7 @@ func (vm *DockerVM) Deploy(ctxt context.Context, ccid ccintf.CCID, //Start starts a container using a previously created docker image func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID, - args []string, env []string, filesToUpload map[string][]byte, builder container.BuildSpecFactory, prelaunchFunc container.PrelaunchFunc) error { + args []string, env []string, filesToUpload map[string][]byte, builder container.BuildSpecFactory) error { imageName, err := vm.GetVMName(ccid, formatImageName) if err != nil { return err @@ -387,12 +387,6 @@ func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID, } } - if prelaunchFunc != nil { - if err = prelaunchFunc(); err != nil { - return err - } - } - // start container with HostConfig was deprecated since v1.10 and removed in v1.2 err = client.StartContainer(containerName, nil) if err != nil { diff --git a/core/container/dockercontroller/dockercontroller_test.go b/core/container/dockercontroller/dockercontroller_test.go index f04ce0d61ff..c3c23049178 100644 --- a/core/container/dockercontroller/dockercontroller_test.go +++ b/core/container/dockercontroller/dockercontroller_test.go @@ -110,25 +110,25 @@ func Test_Start(t *testing.T) { // case 1: getMockClient returns error dvm.getClientFnc = getMockClient getClientErr = true - err := dvm.Start(ctx, ccid, args, env, files, nil, nil) + err := dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, false) getClientErr = false // case 2: dockerClient.CreateContainer returns error createErr = true - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, false) createErr = false // case 3: dockerClient.UploadToContainer returns error uploadErr = true - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, false) uploadErr = false // case 4: dockerClient.StartContainer returns docker.noSuchImgErr noSuchImgErr = true - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, false) chaincodePath := "github.com/hyperledger/fabric/examples/chaincode/go/example01/cmd" @@ -146,53 +146,35 @@ func Test_Start(t *testing.T) { // docker.noSuchImgErr and dockerClient.Start returns error viper.Set("vm.docker.attachStdout", true) startErr = true - err = dvm.Start(ctx, ccid, args, env, files, bldr, nil) + err = dvm.Start(ctx, ccid, args, env, files, bldr) testerr(t, err, false) startErr = false // Success cases - err = dvm.Start(ctx, ccid, args, env, files, bldr, nil) + err = dvm.Start(ctx, ccid, args, env, files, bldr) testerr(t, err, true) noSuchImgErr = false // dockerClient.StopContainer returns error stopErr = true - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, true) stopErr = false // dockerClient.KillContainer returns error killErr = true - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, true) killErr = false // dockerClient.RemoveContainer returns error removeErr = true - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, true) removeErr = false - err = dvm.Start(ctx, ccid, args, env, files, nil, nil) + err = dvm.Start(ctx, ccid, args, env, files, nil) testerr(t, err, true) - - //test preLaunchFunc works correctly - preLaunchStr := "notset" - preLaunchFunc := func() error { - preLaunchStr = "set" - return nil - } - - err = dvm.Start(ctx, ccid, args, env, files, nil, preLaunchFunc) - testerr(t, err, true) - assert.Equal(t, preLaunchStr, "set") - - preLaunchFunc = func() error { - return fmt.Errorf("testing error path") - } - - err = dvm.Start(ctx, ccid, args, env, files, nil, preLaunchFunc) - testerr(t, err, false) } func Test_Stop(t *testing.T) { diff --git a/core/container/inproccontroller/inproccontroller.go b/core/container/inproccontroller/inproccontroller.go index 2cd2ec65724..1e6cf374f01 100644 --- a/core/container/inproccontroller/inproccontroller.go +++ b/core/container/inproccontroller/inproccontroller.go @@ -158,7 +158,7 @@ func (ipc *inprocContainer) launchInProc(ctxt context.Context, id string, args [ } //Start starts a previously registered system codechain -func (vm *InprocVM) Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder container.BuildSpecFactory, prelaunchFunc container.PrelaunchFunc) error { +func (vm *InprocVM) Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder container.BuildSpecFactory) error { path := ccid.ChaincodeSpec.ChaincodeId.Path ipctemplate := typeRegistry[path] @@ -186,12 +186,6 @@ func (vm *InprocVM) Start(ctxt context.Context, ccid ccintf.CCID, args []string, return fmt.Errorf("in-process communication generator not supplied") } - if prelaunchFunc != nil { - if err = prelaunchFunc(); err != nil { - return err - } - } - ipc.running = true go func() { diff --git a/core/container/inproccontroller/inproccontroller_test.go b/core/container/inproccontroller/inproccontroller_test.go index 35f99570674..9f757adcefa 100644 --- a/core/container/inproccontroller/inproccontroller_test.go +++ b/core/container/inproccontroller/inproccontroller_test.go @@ -463,7 +463,7 @@ func TestStart(t *testing.T) { typeRegistry["path"] = ipc - err := vm.Start(mockContext, ccid, args, env, files, mockBuilder, nil) + err := vm.Start(mockContext, ccid, args, env, files, mockBuilder) assert.Nil(t, err, "err should be nil") } From a1da4f8ae3ac22da29eb4ee626a942e6c9e11559 Mon Sep 17 00:00:00 2001 From: Jason Yellick Date: Fri, 27 Apr 2018 00:51:51 -0400 Subject: [PATCH 2/2] FAB-9761 Convert BuilderSpecFactory to intf The BuilderSpecFactory type is currently typed to be a function. It makes life generally easier to mock by converting this function type to an interface, this CR does so. Change-Id: Ifab0b22518eb2907ff672db4714d2ddf7e1457b6 Signed-off-by: Jason Yellick --- core/chaincode/container_runtime.go | 10 +++++++++- core/container/api/core.go | 6 ++++-- core/container/controller.go | 2 +- .../container/dockercontroller/dockercontroller.go | 4 ++-- .../dockercontroller/dockercontroller_test.go | 12 +++++++++++- .../container/inproccontroller/inproccontroller.go | 2 +- .../inproccontroller/inproccontroller_test.go | 14 +------------- 7 files changed, 29 insertions(+), 21 deletions(-) diff --git a/core/chaincode/container_runtime.go b/core/chaincode/container_runtime.go index 1deaf09f4ca..49dff566dc3 100644 --- a/core/chaincode/container_runtime.go +++ b/core/chaincode/container_runtime.go @@ -46,6 +46,14 @@ type ContainerRuntime struct { PeerNetworkID string } +type platformBuilder struct { + cds *pb.ChaincodeDeploymentSpec +} + +func (b *platformBuilder) Build() (io.Reader, error) { + return platforms.GenerateDockerBuild(b.cds) +} + // Start launches chaincode in a runtime environment. func (c *ContainerRuntime) Start(ctxt context.Context, cccid *ccprovider.CCContext, cds *pb.ChaincodeDeploymentSpec) error { cname := cccid.GetCanonicalName() @@ -59,7 +67,7 @@ func (c *ContainerRuntime) Start(ctxt context.Context, cccid *ccprovider.CCConte chaincodeLogger.Debugf("start container with args: %s", strings.Join(lc.Args, " ")) chaincodeLogger.Debugf("start container with env:\n\t%s", strings.Join(lc.Envs, "\n\t")) - builder := func() (io.Reader, error) { return platforms.GenerateDockerBuild(cds) } + builder := &platformBuilder{cds: cds} scr := container.StartContainerReq{ Builder: builder, Args: lc.Args, diff --git a/core/container/api/core.go b/core/container/api/core.go index 0acaf87a74f..b43e93b2dc9 100644 --- a/core/container/api/core.go +++ b/core/container/api/core.go @@ -24,12 +24,14 @@ import ( "github.com/hyperledger/fabric/core/container/ccintf" ) -type BuildSpecFactory func() (io.Reader, error) +type Builder interface { + Build() (io.Reader, error) +} //VM is an abstract virtual image for supporting arbitrary virual machines type VM interface { Deploy(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, reader io.Reader) error - Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder BuildSpecFactory) error + Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder Builder) error Stop(ctxt context.Context, ccid ccintf.CCID, timeout uint, dontkill bool, dontremove bool) error Destroy(ctxt context.Context, ccid ccintf.CCID, force bool, noprune bool) error GetVMName(ccID ccintf.CCID, format func(string) (string, error)) (string, error) diff --git a/core/container/controller.go b/core/container/controller.go index 141ad8e93e8..4197b4af6a6 100644 --- a/core/container/controller.go +++ b/core/container/controller.go @@ -122,7 +122,7 @@ type VMCResp struct { //StartContainerReq - properties for starting a container. type StartContainerReq struct { ccintf.CCID - Builder api.BuildSpecFactory + Builder api.Builder Args []string Env []string FilesToUpload map[string][]byte diff --git a/core/container/dockercontroller/dockercontroller.go b/core/container/dockercontroller/dockercontroller.go index 5434e33d144..adcd6d3de09 100644 --- a/core/container/dockercontroller/dockercontroller.go +++ b/core/container/dockercontroller/dockercontroller.go @@ -229,7 +229,7 @@ func (vm *DockerVM) Deploy(ctxt context.Context, ccid ccintf.CCID, //Start starts a container using a previously created docker image func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID, - args []string, env []string, filesToUpload map[string][]byte, builder container.BuildSpecFactory) error { + args []string, env []string, filesToUpload map[string][]byte, builder container.Builder) error { imageName, err := vm.GetVMName(ccid, formatImageName) if err != nil { return err @@ -261,7 +261,7 @@ func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID, dockerLogger.Debugf("start-could not find image <%s> (container id <%s>), because of <%s>..."+ "attempt to recreate image", imageName, containerName, err) - reader, err1 := builder() + reader, err1 := builder.Build() if err1 != nil { dockerLogger.Errorf("Error creating image builder for image <%s> (container id <%s>), "+ "because of <%s>", imageName, containerName, err1) diff --git a/core/container/dockercontroller/dockercontroller_test.go b/core/container/dockercontroller/dockercontroller_test.go index c3c23049178..2fba8b4e960 100644 --- a/core/container/dockercontroller/dockercontroller_test.go +++ b/core/container/dockercontroller/dockercontroller_test.go @@ -140,7 +140,9 @@ func Test_Start(t *testing.T) { t.Fatal() } cds := &pb.ChaincodeDeploymentSpec{ChaincodeSpec: spec, CodePackage: codePackage} - bldr := func() (io.Reader, error) { return platforms.GenerateDockerBuild(cds) } + bldr := &mockBuilder{ + buildFunc: func() (io.Reader, error) { return platforms.GenerateDockerBuild(cds) }, + } // case 4: start called with builder and dockerClient.CreateContainer returns // docker.noSuchImgErr and dockerClient.Start returns error @@ -283,6 +285,14 @@ func getMockClient() (dockerClient, error) { return &mockClient{noSuchImgErrReturned: false}, nil } +type mockBuilder struct { + buildFunc func() (io.Reader, error) +} + +func (m *mockBuilder) Build() (io.Reader, error) { + return m.buildFunc() +} + type mockClient struct { noSuchImgErrReturned bool } diff --git a/core/container/inproccontroller/inproccontroller.go b/core/container/inproccontroller/inproccontroller.go index 1e6cf374f01..87361f6ce78 100644 --- a/core/container/inproccontroller/inproccontroller.go +++ b/core/container/inproccontroller/inproccontroller.go @@ -158,7 +158,7 @@ func (ipc *inprocContainer) launchInProc(ctxt context.Context, id string, args [ } //Start starts a previously registered system codechain -func (vm *InprocVM) Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder container.BuildSpecFactory) error { +func (vm *InprocVM) Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, filesToUpload map[string][]byte, builder container.Builder) error { path := ccid.ChaincodeSpec.ChaincodeId.Path ipctemplate := typeRegistry[path] diff --git a/core/container/inproccontroller/inproccontroller_test.go b/core/container/inproccontroller/inproccontroller_test.go index 9f757adcefa..b87fe88c606 100644 --- a/core/container/inproccontroller/inproccontroller_test.go +++ b/core/container/inproccontroller/inproccontroller_test.go @@ -13,8 +13,6 @@ import ( "golang.org/x/net/context" - "io" - "github.com/hyperledger/fabric/core/chaincode/shim" "github.com/hyperledger/fabric/core/container/ccintf" pb "github.com/hyperledger/fabric/protos/peer" @@ -425,12 +423,6 @@ func TestLaunchprocCCSupportHandleChaincodeStreamError(t *testing.T) { assert.Nil(t, err, "err should be nil") } -type MockIOReader struct{} - -func (io MockIOReader) Read(p []byte) (int, error) { - return 0, nil -} - func TestStart(t *testing.T) { defer func() { typeRegistry = make(map[string]*inprocContainer) @@ -455,15 +447,11 @@ func TestStart(t *testing.T) { "hello": []byte("world"), } - mockBuilder := func() (io.Reader, error) { - return MockIOReader{}, nil - } - ipc := &inprocContainer{args: args, env: env, chaincode: mockInprocContainer.chaincode, stopChan: make(chan struct{})} typeRegistry["path"] = ipc - err := vm.Start(mockContext, ccid, args, env, files, mockBuilder) + err := vm.Start(mockContext, ccid, args, env, files, nil) assert.Nil(t, err, "err should be nil") }