Skip to content
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

New docker flow including pulling images #612

Merged
merged 6 commits into from
Aug 13, 2021

Conversation

mbrandenburger
Copy link
Contributor

@mbrandenburger mbrandenburger commented Aug 2, 2021

This PR introduces changes the docker flow as discussed in #611.

FPC docker images can now be pulled from github registry or built manually.

Closes #611

@ikegawa-koshi
Copy link
Contributor

@mbrandenburger
I just tried to run FPC following the steps in this PR.
I was able to pull that docker image without any problem.
It works correctly in my environment (Azure SGX HW).
Very nice work!

@mbrandenburger
Copy link
Contributor Author

removed automated publishing from this PR. This will come with a followup PR.

TODO: DOCKER_REGISTRY ?= ghcr.io/mbrandenburger needs to be changed to DOCKER_REGISTRY ?= ghcr.io before merging

@mbrandenburger mbrandenburger marked this pull request as ready for review August 6, 2021 10:14
@mbrandenburger mbrandenburger requested a review from a team as a code owner August 6, 2021 10:14
@mbrandenburger mbrandenburger changed the title WIP: New docker flow including image publishing New docker flow including pulling images Aug 6, 2021
@mbrandenburger mbrandenburger force-pushed the new-docker branch 2 times, most recently from 7e786e1 to aed405f Compare August 6, 2021 10:45
@munapower
Copy link
Contributor

munapower commented Aug 8, 2021

@mbrandenburger I was able to create the development environment with both methods provided. When I tried to follow the test-network sample I ran into this error:

make: Entering directory '/project/src/github.com/hyperledger/fabric-private-chaincode/utils/docker'
# ccenv
DOCKER_BUILDKIT=1 docker  pull ghcr.io/mbrandenburger/hyperledger/fabric-private-chaincode-ccenv:main
Error response from daemon: Head https://ghcr.io/v2/mbrandenburger/hyperledger/fabric-private-chaincode-ccenv/manifests/main: unauthorized
make: *** [Makefile:242: pull] Error 1
make: Leaving directory '/project/src/github.com/hyperledger/fabric-private-chaincode/utils/docker'```

@mbrandenburger
Copy link
Contributor Author

Error response from daemon: Head https://ghcr.io/v2/mbrandenburger/hyperledger/fabric-private-chaincode-ccenv/manifests/main: unauthorized

Ouch that was my fault. I did not hit the public button on github for this image. It should work now.

@munapower
Copy link
Contributor

@mbrandenburger I was able to follow de readmes for FPC dev environment and the sample test-network both pulling as well as building the images.

Copy link
Contributor

@bvavala bvavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments and one suggestion to remove dev.

utils/docker/dev/Dockerfile Outdated Show resolved Hide resolved
utils/docker/dev/Dockerfile Outdated Show resolved Hide resolved
utils/docker/Makefile Show resolved Hide resolved
@@ -0,0 +1,62 @@
# Copyright IBM Corp. All Rights Reserved.
# Copyright 2020 Intel Corporation
#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make the point that we should remove this dev image, and leave the base-dev one. This requires minor modifications, because both images can build FPC, though only dev can run all the FPC tests.

The main difference between base-dev and dev is the presence of Fabric's source code and the SGX_MODE variable in dev.
The former, in particular, does not change, since we check out a predefined version.
So we are leaving to the user to do the job (while building the dev image), but we can do this before. Additionally, Fabric's code is necessary only because of some tests under internal. Otherwise, even base-dev can run the FPC tests.
Anyway, without opening a discussion on the tests, I would leave the source code in the image.

The SGX_MODE variable instead is set/hardcoded in the image. However, the dev image is meant to be used in SIM/HW mode. So, this should not be hardcoded -- besides, we already have SIM as default value.
Additionally, any such variable can be set at docker run-time (with the -e option). Hence, there is no need to create an image just for setting it. Actually, it would be even desirable to set it at run-time, equal to the value of the host's variable.

Concluding, dev could be removed, and this would not impact the build process. However, for our tests (including local tests in HW mode) to work, base-dev must check out Fabric's code, and make run-dev (or equivalent target for running base-dev) must set the SGX_MODE variable appropriately.

Copy link
Contributor Author

@mbrandenburger mbrandenburger Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you made a good point. In general I vote for removing the dev image but I still see a good use for it.

Regarding SGX_MODE I agree with you, we do not need to set it at image build time. Having SIM as default is good and can be overwritten at container start-up.

Regarding Fabric, this can also go away once tests are "refactored".

Let me add another difference between base-dev and dev. The dev image may contain custom applications installed via apt. The user may set DOCKER_DEV_IMAGE_APT_ADD_PKGS to install whatever they need inside the dev container. For instance, I use this feature to install a few things, like tmux and vim in my dev container, which I will not miss!! :P

Moving DOCKER_DEV_IMAGE_APT_ADD_PKGS from dev to base-dev would require to rebuild it from scratch and not leveraging the new docker pull mode. Works but not nice when you just want to have tmux installed.
Installing these packages on every startup also not ideal as the container is not persistent.
Let the user create a "custom" image based on base-dev that contains additional tools they want, would lead to the solution we already have with dev. Do you have another idea how to enable custom modifications to the dev container while benefiting from docker pull mechanism?

Copy link
Contributor

@bvavala bvavala Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding Fabric, this can also go away once tests are "refactored".

"Away", when tests are refactored -- I guess you know better -- or "in base-dev" at any time to consolidate the two images.

The user may set DOCKER_DEV_IMAGE_APT_ADD_PKGS ...

True, but that's not something that impacts FPC: neither the build process, nor the testing (assuming Fabric's code is present, of course).
For this reason, the question is whether FPC should provide this feature, or leave this to the end users/developers. Expectedly, if they need anything other than an apt package, they can and will build their own images, but FPC will not provide directly any such feature.

Do you have another idea how to enable custom modifications to the dev container while benefiting from docker pull mechanism?

This is the point: FPC does not prevent to do that -- just use docker to extend the provided images with what you need -- so there is nothing to enable here. Docker can already do the job.

Also, the "custom" modification that dev now supports is "only" related to the installation of apt packages.
One of the many ways in which an image can be modified.

Anyway, fine to keep this for now to avoid any disruption. I have captured this in #616 .

Copy link
Contributor

@bvavala bvavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg, tested in hw mode

final step before merge: switch the ghcr link to the hyperledger namespace

Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
Signed-off-by: Marcus Brandenburger <[email protected]>
@mbrandenburger
Copy link
Contributor Author

@bvavala I changed the namespace and pushed the images to

Please check again that you can pull the images before (squashing) merging. Thank you!

@bvavala bvavala merged commit 84e26db into hyperledger:main Aug 13, 2021
@mbrandenburger mbrandenburger deleted the new-docker branch August 13, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New docker flow
4 participants