-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding chaincode-as-a-service external builder to peer's docker image #2990
Conversation
It would be really really nice to get an out of the box external builder, although that's only part of the story... e.g. |
Candidate update to the fabric-samples to use these builders. hyperledger/fabric-samples#507 |
Reference for Running Chaincode-as-a-service node.js
java
Note - this should be added to the java libraries properly golang
|
ext_ccs_builder/go.mod
Outdated
@@ -0,0 +1,5 @@ | |||
module hyperledgendary/fabric-ext-builder/v2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong module name ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes you're correct
I really think that this should support non chaincode-as-a-service as well to support existing users with existing chaincode implementations. I feel the initial benefit of this is the removal of dependencies on the internal Docker builder and launcher, plus there are an awful lot of existing chaincode implementations that would need to be changed over to chaincode as a service |
Sorry I'd misrepresented the scope of this suggestion in the title. this is only about supporting the issue #2884 - that is chaincode-as-a-service. The aim is to provide, in the Fabric Peer as standard, an external builder for processing chaincode-as-a-service. rather than have to configure the peer specifically to add this. @jt-nti @davidkel both very correct in your points. The ways an external builder (not using chaincode-as-a-service) are vast, though not directly called from the peer, code needs to be running in the same container as the peer to build/create the chaincode. This is now responsible for orchestrating the starting of the chaincode. It may no longer be docker, but it would need to be something. This specific PR is exploring one of the ways that an external builder could be provided as standard - that is via including it preconfigured in the built docker image. This is a simple, and effective solution. It does not however mean a 'standalone' peer binary would know, though that could be configured to use the same if you wished. |
437e4fb
to
04fb585
Compare
6e282ac
to
a956e72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this approach. The compromise of installing a chaincode-as-a-service external builder into the peer image strikes the right balance of simplicity vs flexibility. Even with some rough edges around the docker build, it seems infinitely better than the alternative approach of altering the peer golang routines for cc lifecycle.
Using the new default builder, it was very easy to get up and running with external chaincode services running on Kubernetes. Matthew's (fabric-samples) PR 527 shows:
- An update to core.yaml to enable the builder:
externalBuilders:
- name: ccaas_builder
propagateEnvironment:
- EXTERNAL_CHAINCODE_BUILDER_CONFIG
path: /opt/hyperledger/ccaas_builder
-
yaml updates to launch the chaincode containers (compiled out of band)
-
template substitution within connection.json
In contrast with the previous approach for registering an external chaincode builder, this is very, very straightforward. This PR reduces the config burden for switching to external chaincode-as-a-service significantly. I approve!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbwhite , I added some comments here. please take a look at.
e91c480
to
35c4301
Compare
@hyperledger/fabric-core-maintainers this is ready for review.. thankyou. |
35c4301
to
89e557f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new approach works really well but there are a couple of issues in this PR:
-
ccass
->ccaas
(to match target of metadata.json) -
detect is detecting
ccass
but build/main.go is still checking forexternal
-
The default core.yaml should propagate the updated env var name
CHAINCODE_AS_A_SERVICE_BUILDER_CONFIG
After making these updates in the core images it was smooth sailing.
89e557f
to
cb280eb
Compare
thank you @jkneubuh; I've grepped the code as well looking for the words external ccaas ccaass ccas etc make sure there's nothing hidden.. should be good now |
👍 |
ab0e576
to
2d411a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good now, I'll go through the code in some more detail, but looks like it is structured well. Just a few nits for now...
# propagateEnvironment: | ||
# - ENVVAR_NAME_TO_PROPAGATE_FROM_PEER | ||
# - GOPROXY | ||
externalBuilders: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above, it would be worthwhile to comment about the new out-of-the-box ccaas, in addition to the default Docker builder.
|
||
FROM peer-base | ||
ENV FABRIC_CFG_PATH /etc/hyperledger/fabric | ||
VOLUME /etc/hyperledger/fabric | ||
VOLUME /var/hyperledger | ||
COPY --from=peer /go/src/github.com/hyperledger/fabric/build/bin /usr/local/bin | ||
COPY --from=peer /go/src/github.com/hyperledger/fabric/sampleconfig/msp ${FABRIC_CFG_PATH}/msp | ||
COPY --from=peer /go/src/github.com/hyperledger/fabric/sampleconfig/core.yaml ${FABRIC_CFG_PATH} | ||
COPY --from=peer /go/src/github.com/hyperledger/fabric/sampleconfig/core.yaml ${FABRIC_CFG_PATH}/core.yaml | ||
COPY --from=peer /go/src/github.com/hyperledger/fabric/release/linux-amd64/bin/ccaas_builder/bin/ /opt/hyperledger/ccaas_builder/bin/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you build the image locally and confirm it has the correct content and can be used?
e.g. make docker-clean docker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done; and will repeat the exercise before final merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me although it seems a bit odd that ccaas_builder is a completely separate go module whereas there are existing commands under /cmd
which are part of the top level module. What are the pros and cons of doing it each way?
@jt-nti yes a good question; I don't have the experience of knowledge of the build/codebase structure philosophy to be able to definitively respond. Will take the direction of the maintainers here, as to what is the most approraite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed Detect first. Many of the comments there apply to Build and Release as well.
cd ccaas_builder && go test -v ./cmd/detect && go build -o ../$(MARCH:%=release/%)/bin/ccaas_builder/bin/ ./cmd/detect/ | ||
cd ccaas_builder && go test -v ./cmd/build && go build -o ../$(MARCH:%=release/%)/bin/ccaas_builder/bin/ ./cmd/build/ | ||
cd ccaas_builder && go test -v ./cmd/release && go build -o ../$(MARCH:%=release/%)/bin/ccaas_builder/bin/ ./cmd/release/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need go test
here.
Is there a reason you'd put the built executables in $(BUILD_DIR)/bin with the others?
e.g. /build/bin/ccaas_builder/bin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to assume that the make unit-test
will find all the tests needed?
The path you provide in the core.yaml
needs to point to a directory that contains a bin
directory with the apps in it. Hence the ccaas_builder/bin
but as to where exactly the source lives and the binaries built to. happy to take guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I ran the make unit-tests
and it didn't pick up these tests automatically? Should it be updated to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected all unit tests under /fabric to get picked up automatically.
"strings" | ||
) | ||
|
||
var logger = log.New(os.Stderr, "", 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everywhere else in Fabric Go code we use github.com/hyperledger/fabric/common/flogging, e.g.
var logger = flogging.MustGetLogger("kvledger")
But since this doesn't pull in any Fabric dependencies, I guess it's ok to use the built in logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes -as you say these aren't part of say the Peer, Orderer etc. they are external applications; all the examples in the docs use bash; but they could be anything.
They could pull in the Fabric dependencies such as the flogging
if you wish.
ccaas_builder/cmd/detect/main.go
Outdated
chaincodeMetaData := os.Args[2] | ||
mdbytes, err := ioutil.ReadFile(filepath.Join(chaincodeMetaData, "metadata.json")) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically add context to the error so the user knows what was going on instead of just returning the underlying Go error message. Typically errors.WithMessage()
is used for this:
https://pkg.go.dev/github.com/pkg/errors#WithMessage
Do you want to check file existence like you do in build program?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding context is very useful thanks. that's for pointing out the withMessage
. Will update across the board with this.
Though (a) if these files aren't there then the peer's probably gone wrong and (b) the peer doesn't really report much back to if one these programs fails..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of a scenario where the user forgot to include metadata.json in the chaincode package.
fileMetadata.WriteString(`{ | ||
"type":"ccaas" | ||
}`) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add negative test for no metadata.json and no connection.json.
if err := json.Unmarshal(connectionFileContents, &connectionData); err != nil { | ||
return err | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check for required properties that may not have gotten set in the JSON? Or does the template do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the error if missing
key option to the template for exactly this case.
Signed-off-by: Matthew B White <[email protected]>
2d411a8
to
1a26117
Compare
Since these are completely standalone programs with no Fabric dependencies I think it is appropriate to keep them separate. |
metadataFile := filepath.Join(chaincodeMetaData, "metadata.json") | ||
_, err := os.Stat(metadataFile) | ||
if err != nil { | ||
return fmt.Errorf("%s not found ", metadataFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even for these we usually use errors.New().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just been updating these, but now have VSCode warnings...
should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (S1028) go-staticcheck
So a follow on question is whether it would be better for the new external builder module to live in a separate repo. This PR would essentially convert the fabric repository into a monorepo. What are the implications of that in Go? |
Since users wouldn't import the new programs, I don't see an impact to users. |
For the record, I completely agree with the desire to keep things slim/pure; Fabric is a now a product to be used in production environments, so it's consumability is of paramount importance. This PR is intended to make this easier, and make migration to a K8S environment significantly easier. (agree this is not being disputed). However I don't think this PR is the place to refactor the repository; the question is it worth not having this consumeability improvement until such a refactor takes place? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the current precedent is to keep everything the image depends on in a single repo, it makes sense to get this PR in and discuss whether we want to revisit that strategy and refactor the repository later.
I would prefer a minor change to follow the precedent of the existing commands being part of the same Fabric module, rather than introducing a new separate module just for the builder, but also happy for the PR to go in as is if that's the consensus.
Hi, I'm interested in this. So just to be clear, where does this leave external Java chaincode? Does this PR cover that? Is there any chance of a working image we can use on our project as it's pretty critical for us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Matthew. This is a great leap in consumability for chaincode-as-a-service. It does muddy the waters of the Fabric repo a bit, but we can always move it out later if it becomes a problem or if we decide on another strategy for building and publishing the Fabric peer docker image.
@Mergifyio backport release-2.4 |
✅ Backports have been created
|
Well done!!!
Chris Gabriel, Founder
Hyperchain Labs
***@***.***
918-397-4078
hyperchainlabs.com
Twitter @HyperchainLabs
… On Dec 9, 2021, at 8:50 AM, Dave Enyeart ***@***.***> wrote:
@Mergifyio backport release-2.4
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
✅ Backports have been created
|
is it possible to use ccaas_builder in fabric v2.2? @mbwhite |
@uqix yes; that should be possible. The binaries can be used as is, but you will need to edit the config.yaml used by the peer inline with the Fabric 2.4 version. So they pick up the required ccaas binaries. |
For v2.2, we needed to build custom peer image: Dockerfile: ARG fabric_version
FROM hyperledger/fabric-peer:2.4.3
FROM hyperledger/fabric-peer:$fabric_version
COPY --from=0 /opt/ /opt/
RUN sed -i 's/externalBuilders: \[\]/externalBuilders:\n - name: ccaas_builder\n path: \/opt\/hyperledger\/ccaas_builder/' /etc/hyperledger/fabric/core.yaml Build: dockerfile_version=1.0.1
fabric_version=2.2.2
image_name=uqix/hyperledger-fabric-peer:$dockerfile_version-$fabric_version
docker build \
--build-arg fabric_version=$fabric_version \
-t $image_name \
. \
&& docker push $image_name |
Signed-off-by: Matthew B White [email protected]
Type of change
Description
Additional details
Related issues
Resolves #2884