-
Notifications
You must be signed in to change notification settings - Fork 191
build: multi-stage builds and services configuration #1811
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
base: master
Are you sure you want to change the base?
Conversation
Dockerfile
Outdated
| && apt-get clean \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| FROM bitcoin/bitcoin:29.2 AS bitcoin |
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.
As tests use compose anyway, why not remove/split out the bitcoin and test stages from the production Dockerfile? I don't want to redundantly have to pull the bitcoin image every time I build in a clean environment.
If a bundled image for testing is desired, I suggest putting that under test/Dockerfiles/.
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 test or bitcoin layer are not included in the final one.
$ docker build .
[+] Building 9.0s (12/12) FINISHED docker:desktop-linux
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 717B 0.0s
=> [internal] load metadata for docker.io/library/python:3.11-slim-trixie 0.9s
=> [internal] load .dockerignore 0.0s
=> => transferring context: 67B 0.0s
=> [joinmarket 1/7] FROM docker.io/library/python:3.11-slim-trixie@sha256:8eb5fc663972b871c528fef04be4eaa9ab8ab4539a5316c4b8c133771214a617 0.0s
=> [internal] load build context 0.1s
=> => transferring context: 1.70MB 0.1s
=> CACHED [joinmarket 2/7] WORKDIR /jm/clientserver 0.0s
=> CACHED [joinmarket 3/7] COPY ./pubkeys ./pubkeys 0.0s
=> CACHED [joinmarket 4/7] COPY ./install.sh ./install.sh 0.0s
=> CACHED [joinmarket 5/7] RUN apt-get update && apt-get install -y --no-install-recommends gnupg ca-certificates curl && ./install.sh --docker-install --deps-only && 0.0s
=> [joinmarket 6/7] COPY . . 0.1s
=> [joinmarket 7/7] RUN apt-get update && ./install.sh --docker-install --no-deps && apt-get clean && rm -rf /var/lib/apt/lists/* 7.7s
=> exporting to image 0.3s
=> => exporting layers 0.3s
=> => writing image sha256:17292e6798277f17d6542eb9370083c031add6e477739e2938767f3bb8135263
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.
Suggestion: to minimize confusion, indicate that this is intended for tests by e.g. moving to test/Dockerfiles/compose.yml or test/docker-compose.yml.
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.
This is meant to be extended at some point, should not be restricted to test imho. Further to that, developer experience feels better to me if you can run all from root folder
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.
Extension can still happen in separate Dockerfiles, no?
Docker is generally "smart" enough to reuse cache layers even if you build from a separate Dockerfile.
Further extending the production Dockerfile and making it more complex without benefiting the runtime image is an antipattern IMO. You can still build and run it from the root directory even if the Dockerfile is in a subdir.
Dockerfile
Outdated
| gnupg \ | ||
| ca-certificates \ | ||
| curl \ | ||
| && ./install.sh --docker-install --deps-only \ |
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.
| && ./install.sh --docker-install --deps-only \ | |
| && apt-get purge -y --autoremove python3-pip build-essential python3-dev python3-pip automake pkg-config libffi-dev libssl-dev \ |
these packages are not necessary at runtime so can be removed to save space.
(feel free to ignore; I can make a separate PR for 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 can only agree. Install dependencies optimization deserves a PR on its own imo, we could further optimize layers with a more granular script.
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.
Dockerfile
Outdated
| COPY --from=bitcoin /opt/bitcoin-29.2/bin /usr/local/bin/ | ||
| RUN ./install.sh --docker-install --no-deps --develop | ||
|
|
||
| FROM joinmarket AS final |
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.
IMO a major benefit of multi-stage builds is to do the build in a separate branch from the final image, basing the final image on a clean base such as debian:slim or python:3.12-slim, and copy over the necessary artefacts from the build image (like you are doing with bitcoin here). Since build scripts have never been run in the context of the final image this way, this can have both security and space-efficiency benefits.
Did you consider this approach? What's the benefit of this approach of multi-stage where the final image is simply a copy of the build step?
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.
For now, this multi layer approach allows to extend the main layer with test dependencies, no more. Ideally we indeed want a thin final layer, which requires a refactoring of the install script imo, in a subsequent PR.
1c13e06 to
8ebbdef
Compare
|
nACK on colocating bitcoin and joinmarket in the same image. This gives joinmarket full (root) access to the bitcoin daemon (and vice versa). Resolving this is possible but would make the Dockerfile a lot more complex for little benefit when the idiomatic approach is using separate containers and link them in the compose file. We don't want users to be lured into running such a weakened setup in prod. Having a more representative setup for tests is good. I do think joinmarket could benefit from a multi-stage build that separates the build- and runtime steps for joinmarket-clientserver itself but that's not what this is. BTW, this results in a final production image size increase of 14% (without bitcoind). |
|
a couple minor updates, most notably a fix introducing an entrypoint to final image allowing command execution as before refactoring, despite
|
bitcoin binaries are not included in the final image. $ docker build -t jm .
[...]
$ docker run -it --rm jm which bitcoind || echo "bitcoind not found"
bitcoind not found |
3439e37 to
91875d3
Compare
|
a couple minor updates:
|
2dd8928 to
852dee9
Compare
|
|
my previous iteration on this pull request, adding other than leveraging this proposed feature, this pr is amended to consistently use editable install, and implement a base service that others can simply extend, avoiding repetition. |
- Updated `Dockerfile` to support multi-stage builds - Introduced a new `compose.yml` file for service definition - Updated documentation build: consistently use editable install build: extend base service feat: obwatcher config from environment variable revert: obwatcher
|
removed |
compose.ymlfile for running tests in an isolated Docker environment.Dockerfileto support multi-stage builds, including a dedicated test stage with Bitcoin Core.Enhancedinstall.shto allow installation of dependencies only, without building JoinMarket.