Skip to content

Commit d304c94

Browse files
author
Jason Yellick
committed
FAB-9950 Assorted deferred review cleanup
These are a few cleanups deferred from code review of: gerrit.hyperledger.org/r/c/21089/8/core/container/controller.go#39 gerrit.hyperledger.org/r/c/21093/8/core/chaincode/container_runtime.go#70 gerrit.hyperledger.org/r/c/21095/8/core/chaincode/container_runtime_test.go#217 gerrit.hyperledger.org/r/c/21095/8/core/chaincode/container_runtime_test.go#295 gerrit.hyperledger.org/r/c/21211/5/core/container/container_test.go gerrit.hyperledger.org/r/c/21097/12/core/container/container_test.go#146 gerrit.hyperledger.org/r/c/21165/8/core/chaincode/container_runtime.go#66 gerrit.hyperledger.org/r/c/21169/9/core/chaincode/container_runtime_test.go#204 gerrit.hyperledger.org/r/c/21169/9/core/chaincode/container_runtime_test.go#204 Because fixing this would have required rebasing, and re-chasing +2s, simply fixing them here. Change-Id: I73be76ad2a13e39346423d55050b2a8119e42a12 Signed-off-by: Jason Yellick <jyellick@us.ibm.com>
1 parent 90281f4 commit d304c94

File tree

5 files changed

+112
-102
lines changed

5 files changed

+112
-102
lines changed

‎core/chaincode/container_runtime.go

+3-12
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,10 @@ package chaincode
99
import (
1010
"bytes"
1111
"fmt"
12-
"io"
1312
"sort"
1413
"strings"
1514

1615
"github.com/hyperledger/fabric/core/chaincode/accesscontrol"
17-
"github.com/hyperledger/fabric/core/chaincode/platforms"
1816
"github.com/hyperledger/fabric/core/common/ccprovider"
1917
"github.com/hyperledger/fabric/core/container"
2018
"github.com/hyperledger/fabric/core/container/ccintf"
@@ -46,14 +44,6 @@ type ContainerRuntime struct {
4644
PeerAddress string
4745
}
4846

49-
type platformBuilder struct {
50-
cds *pb.ChaincodeDeploymentSpec
51-
}
52-
53-
func (b *platformBuilder) Build() (io.Reader, error) {
54-
return platforms.GenerateDockerBuild(b.cds)
55-
}
56-
5747
// Start launches chaincode in a runtime environment.
5848
func (c *ContainerRuntime) Start(ctxt context.Context, cccid *ccprovider.CCContext, cds *pb.ChaincodeDeploymentSpec) error {
5949
cname := cccid.GetCanonicalName()
@@ -67,9 +57,10 @@ func (c *ContainerRuntime) Start(ctxt context.Context, cccid *ccprovider.CCConte
6757
chaincodeLogger.Debugf("start container with args: %s", strings.Join(lc.Args, " "))
6858
chaincodeLogger.Debugf("start container with env:\n\t%s", strings.Join(lc.Envs, "\n\t"))
6959

70-
builder := &platformBuilder{cds: cds}
7160
scr := container.StartContainerReq{
72-
Builder: builder,
61+
Builder: &container.PlatformBuilder{
62+
DeploymentSpec: cds,
63+
},
7364
Args: lc.Args,
7465
Env: lc.Envs,
7566
FilesToUpload: lc.Files,

‎core/chaincode/container_runtime_test.go

+2-4
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ func TestContainerRuntimeStart(t *testing.T) {
201201
assert.Equal(t, startReq.Env, []string{"CORE_CHAINCODE_ID_NAME=context-name:context-version", "CORE_PEER_TLS_ENABLED=false"})
202202
assert.Nil(t, startReq.FilesToUpload)
203203
assert.Equal(t, startReq.CCID, ccintf.CCID{
204-
Name: cds.ChaincodeSpec.ChaincodeId.Name,
204+
Name: "chaincode-id-name",
205205
Version: "context-version",
206206
})
207207
}
@@ -215,7 +215,6 @@ func TestContainerRuntimeStartErrors(t *testing.T) {
215215
}{
216216
{pb.ChaincodeSpec_Type(999), nil, "unknown chaincodeType: 999"},
217217
{pb.ChaincodeSpec_GOLANG, errors.New("process-failed"), "error starting container: process-failed"},
218-
{pb.ChaincodeSpec_GOLANG, errors.New("error-in-response"), "error starting container: error-in-response"},
219218
}
220219

221220
for _, tc := range tests {
@@ -281,7 +280,7 @@ func TestContainerRuntimeStop(t *testing.T) {
281280
assert.Equal(t, stopReq.Timeout, uint(0))
282281
assert.Equal(t, stopReq.Dontremove, false)
283282
assert.Equal(t, stopReq.CCID, ccintf.CCID{
284-
Name: cds.ChaincodeSpec.ChaincodeId.Name,
283+
Name: "chaincode-id-name",
285284
Version: "context-version",
286285
})
287286
}
@@ -293,7 +292,6 @@ func TestContainerRuntimeStopErrors(t *testing.T) {
293292
errValue string
294293
}{
295294
{errors.New("process-failed"), "error stopping container: process-failed"},
296-
{errors.New("error-in-response-is-ignored"), "error stopping container: error-in-response-is-ignored"},
297295
}
298296

299297
for _, tc := range tests {

‎core/container/container_test.go

+91-81
Original file line numberDiff line numberDiff line change
@@ -7,110 +7,117 @@ SPDX-License-Identifier: Apache-2.0
77
package container_test
88

99
import (
10-
"fmt"
11-
1210
"github.com/hyperledger/fabric/core/container"
1311
"github.com/hyperledger/fabric/core/container/ccintf"
1412
"github.com/hyperledger/fabric/core/container/mock"
13+
"github.com/pkg/errors"
1514
"golang.org/x/net/context"
1615

1716
. "github.com/onsi/ginkgo"
1817
. "github.com/onsi/gomega"
1918
)
2019

2120
var _ = Describe("Container", func() {
22-
Describe("StartContainerReq", func() {
21+
Describe("VMCReqs", func() {
2322
var (
24-
ctxt = context.Background()
25-
startReq *container.StartContainerReq
26-
fakeVM *mock.VM
23+
fakeVM *mock.VM
24+
ctxt context.Context
2725
)
2826

2927
BeforeEach(func() {
30-
startReq = &container.StartContainerReq{
31-
CCID: ccintf.CCID{Name: "start-name"},
32-
Args: []string{"foo", "bar"},
33-
Env: []string{"Bar", "Foo"},
34-
FilesToUpload: map[string][]byte{
35-
"Foo": []byte("bar"),
36-
},
37-
Builder: &mock.Builder{},
38-
}
28+
ctxt = context.Background()
3929
fakeVM = &mock.VM{}
4030
})
4131

42-
Describe("Do", func() {
43-
It("Returns a response with no error when things go fine", func() {
44-
fakeVM.StartReturns(nil)
45-
err := startReq.Do(ctxt, fakeVM)
46-
Expect(err).NotTo(HaveOccurred())
47-
Expect(fakeVM.StartCallCount()).To(Equal(1))
48-
rctxt, ccid, args, env, filesToUpload, builder := fakeVM.StartArgsForCall(0)
49-
Expect(rctxt).To(Equal(ctxt))
50-
Expect(ccid).To(Equal(startReq.CCID))
51-
Expect(args).To(Equal(startReq.Args))
52-
Expect(env).To(Equal(startReq.Env))
53-
Expect(filesToUpload).To(Equal(startReq.FilesToUpload))
54-
Expect(builder).To(Equal(startReq.Builder))
32+
Describe("StartContainerReq", func() {
33+
var (
34+
startReq *container.StartContainerReq
35+
)
36+
37+
BeforeEach(func() {
38+
startReq = &container.StartContainerReq{
39+
CCID: ccintf.CCID{Name: "start-name"},
40+
Args: []string{"foo", "bar"},
41+
Env: []string{"Bar", "Foo"},
42+
FilesToUpload: map[string][]byte{
43+
"Foo": []byte("bar"),
44+
},
45+
Builder: &mock.Builder{},
46+
}
5547
})
5648

57-
It("Returns an error when the vm does", func() {
58-
err := fmt.Errorf("Boo")
59-
fakeVM.StartReturns(err)
60-
rerr := startReq.Do(ctxt, fakeVM)
61-
Expect(rerr).To(Equal(err))
49+
Describe("Do", func() {
50+
It("starts a vm", func() {
51+
err := startReq.Do(ctxt, fakeVM)
52+
Expect(err).NotTo(HaveOccurred())
53+
Expect(fakeVM.StartCallCount()).To(Equal(1))
54+
rctxt, ccid, args, env, filesToUpload, builder := fakeVM.StartArgsForCall(0)
55+
Expect(rctxt).To(Equal(ctxt))
56+
Expect(ccid).To(Equal(ccintf.CCID{Name: "start-name"}))
57+
Expect(args).To(Equal([]string{"foo", "bar"}))
58+
Expect(env).To(Equal([]string{"Bar", "Foo"}))
59+
Expect(filesToUpload).To(Equal(map[string][]byte{
60+
"Foo": []byte("bar"),
61+
}))
62+
Expect(builder).To(Equal(&mock.Builder{}))
63+
})
64+
65+
Context("when the vm provider fails", func() {
66+
It("returns the error", func() {
67+
fakeVM.StartReturns(errors.New("Boo"))
68+
err := startReq.Do(ctxt, fakeVM)
69+
Expect(err).To(MatchError("Boo"))
70+
})
71+
})
6272
})
63-
})
6473

65-
Describe("GetCCID", func() {
66-
It("Returns the CCID embedded in the structure", func() {
67-
Expect(startReq.GetCCID()).To(Equal(startReq.CCID))
74+
Describe("GetCCID", func() {
75+
It("Returns the CCID embedded in the structure", func() {
76+
Expect(startReq.GetCCID()).To(Equal(ccintf.CCID{Name: "start-name"}))
77+
})
6878
})
6979
})
70-
})
7180

72-
Describe("StopContainerReq", func() {
73-
var (
74-
ctxt = context.Background()
75-
stopReq *container.StopContainerReq
76-
fakeVM *mock.VM
77-
)
78-
79-
BeforeEach(func() {
80-
stopReq = &container.StopContainerReq{
81-
CCID: ccintf.CCID{Name: "stop-name"},
82-
Timeout: 283,
83-
Dontkill: true,
84-
Dontremove: false,
85-
}
86-
fakeVM = &mock.VM{}
87-
})
88-
89-
Describe("Do", func() {
90-
It("Returns a response with no error when things go fine", func() {
91-
fakeVM.StartReturns(nil)
92-
resp := stopReq.Do(ctxt, fakeVM)
93-
Expect(resp).To(BeNil())
94-
Expect(fakeVM.StopCallCount()).To(Equal(1))
95-
rctxt, ccid, timeout, dontKill, dontRemove := fakeVM.StopArgsForCall(0)
96-
Expect(rctxt).To(Equal(ctxt))
97-
Expect(ccid).To(Equal(stopReq.CCID))
98-
Expect(timeout).To(Equal(stopReq.Timeout))
99-
Expect(dontKill).To(Equal(stopReq.Dontkill))
100-
Expect(dontRemove).To(Equal(stopReq.Dontremove))
81+
Describe("StopContainerReq", func() {
82+
var (
83+
stopReq *container.StopContainerReq
84+
)
85+
86+
BeforeEach(func() {
87+
stopReq = &container.StopContainerReq{
88+
CCID: ccintf.CCID{Name: "stop-name"},
89+
Timeout: 283,
90+
Dontkill: true,
91+
Dontremove: false,
92+
}
10193
})
10294

103-
It("Returns an error when the vm does", func() {
104-
err := fmt.Errorf("Boo")
105-
fakeVM.StopReturns(err)
106-
rerr := stopReq.Do(ctxt, fakeVM)
107-
Expect(rerr).To(Equal(err))
95+
Describe("Do", func() {
96+
It("stops the vm", func() {
97+
resp := stopReq.Do(ctxt, fakeVM)
98+
Expect(resp).To(BeNil())
99+
Expect(fakeVM.StopCallCount()).To(Equal(1))
100+
rctxt, ccid, timeout, dontKill, dontRemove := fakeVM.StopArgsForCall(0)
101+
Expect(rctxt).To(Equal(ctxt))
102+
Expect(ccid).To(Equal(ccintf.CCID{Name: "stop-name"}))
103+
Expect(timeout).To(Equal(uint(283)))
104+
Expect(dontKill).To(Equal(true))
105+
Expect(dontRemove).To(Equal(false))
106+
})
107+
108+
Context("when the vm provider fails", func() {
109+
It("returns the error", func() {
110+
fakeVM.StopReturns(errors.New("Boo"))
111+
err := stopReq.Do(ctxt, fakeVM)
112+
Expect(err).To(MatchError("Boo"))
113+
})
114+
})
108115
})
109-
})
110116

111-
Describe("GetCCID", func() {
112-
It("Returns the CCID embedded in the structure", func() {
113-
Expect(stopReq.GetCCID()).To(Equal(stopReq.CCID))
117+
Describe("GetCCID", func() {
118+
It("Returns the CCID embedded in the structure", func() {
119+
Expect(stopReq.GetCCID()).To(Equal(ccintf.CCID{Name: "stop-name"}))
120+
})
114121
})
115122
})
116123
})
@@ -133,15 +140,18 @@ var _ = Describe("Container", func() {
133140
})
134141

135142
Describe("Process", func() {
136-
It("Panics if there is no underlying VM provider", func() {
137-
Expect(func() { vmController.Process(ctxt, "Unknown-Type", nil) }).To(Panic())
138-
Expect(vmProvider.NewVMCallCount()).To(Equal(0))
139-
})
140-
It("Returns no error if the underlying VM provider is successful", func() {
143+
It("completes the request using the correct vm provider", func() {
141144
err := vmController.Process(ctxt, "FakeProvider", vmcReq)
142145
Expect(vmProvider.NewVMCallCount()).To(Equal(1))
143146
Expect(err).NotTo(HaveOccurred())
144147
})
148+
149+
Context("the request is for an unknown VM provider type", func() {
150+
It("causes the system to halt as this is a serious bug", func() {
151+
Expect(func() { vmController.Process(ctxt, "Unknown-Type", nil) }).To(Panic())
152+
Expect(vmProvider.NewVMCallCount()).To(Equal(0))
153+
})
154+
})
145155
})
146156
})
147157
})

‎core/container/controller.go

+16-4
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ type refCountedLock struct {
4444
// eventually probably need fine grained management)
4545
type VMController struct {
4646
sync.RWMutex
47-
// Handlers for each chaincode
4847
containerLocks map[string]*refCountedLock
4948
vmProviders map[string]VMProvider
5049
}
@@ -120,6 +119,22 @@ type StartContainerReq struct {
120119
FilesToUpload map[string][]byte
121120
}
122121

122+
// PlatformBuilder implements the Build interface using
123+
// the platforms package GenerateDockerBuild function.
124+
// XXX This is a pretty awkward spot for the builder, it should
125+
// really probably be pushed into the dockercontroller, as it only
126+
// builds docker images, but, doing so would require contaminating
127+
// the dockercontroller package with the CDS, which is also
128+
// undesirable.
129+
type PlatformBuilder struct {
130+
DeploymentSpec *pb.ChaincodeDeploymentSpec
131+
}
132+
133+
// Build a tar stream based on the CDS
134+
func (b *PlatformBuilder) Build() (io.Reader, error) {
135+
return platforms.GenerateDockerBuild(b.DeploymentSpec)
136+
}
137+
123138
func (si StartContainerReq) Do(ctxt context.Context, v VM) error {
124139
return v.Start(ctxt, si.CCID, si.Args, si.Env, si.FilesToUpload, si.Builder)
125140
}
@@ -156,9 +171,6 @@ func (si StopContainerReq) GetCCID() ccintf.CCID {
156171
//In all cases VMCProcess will wait for the called go routine to return
157172
func (vmc *VMController) Process(ctxt context.Context, vmtype string, req VMCReq) error {
158173
v := vmc.newVM(vmtype)
159-
if v == nil {
160-
return fmt.Errorf("Unknown VM type %s", vmtype)
161-
}
162174

163175
c := make(chan error)
164176
go func() {

‎core/container/dockercontroller/dockercontroller.go

-1
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ type getClient func() (dockerClient, error)
4747

4848
//DockerVM is a vm. It is identified by an image id
4949
type DockerVM struct {
50-
id string
5150
getClientFnc getClient
5251
PeerID string
5352
NetworkID string

0 commit comments

Comments
 (0)