nicolasff / webdis

A Redis HTTP interface with JSON output
https://webd.is
BSD 2-Clause "Simplified" License
2.82k stars 307 forks source link

Simplifying and reducing commands in Dockerfile #237

Closed zacbayhan closed 3 months ago

zacbayhan commented 10 months ago

This PR is ment to simplify the dockerfile by removing the apk update command, by switching it to a no-cache pull using the --no-cache flag. Then it removes the sed command to remove the parentheses, by switching to the jq raw flag -r.

nicolasff commented 10 months ago

Hey @zacbayhan, thanks for the PR! The jq -r part is simpler, but I don't actually see any difference with the apk changes. That's expected, since the main downside of using apk update is that it stores its database of packages locally, but the Dockerfile makes sure to remove it as part of the same command with an rm -f /var/cache/apk/*: https://github.com/nicolasff/webdis/blob/496ac1aa79087c3865b8fa0ec90192c013dc6c4b/Dockerfile#L14-L17

I took the current Dockerfile on master and built webdis:current from it:

git checkout master
docker build -f Dockerfile -t webdis:current --no-cache .

and then did the same with your version:

git checkout zacbayhan/master
docker build -f Dockerfile -t webdis:pr237 --no-cache .

before comparing them:

$ docker image inspect webdis:current | jq -r '.[0].Size'
19304002

$ docker image inspect webdis:pr237 | jq -r '.[0].Size'
19304002

They're exactly the same size.

nicolasff commented 10 months ago

Updated: I timed the two docker build commands.

Here are two test files, first Dockerfile.current:

FROM alpine:3.17.1
RUN apk update && apk add libevent msgpack-c openssl \
    'redis>=6.2.10' 'libssl3>=3.0.8-r0' 'libcrypto3>=3.0.8-r1' \
    'libssl1.1' 'libcrypto1.1>=1.1.1t-r0' && \
    rm -f /var/cache/apk/*

and Dockerfile.pr237:

FROM alpine:3.17.1
RUN apk add --no-cache libevent msgpack-c openssl \
    'redis>=6.2.10' 'libssl3>=3.0.8-r0' 'libcrypto3>=3.0.8-r1' \
    'libssl1.1' 'libcrypto1.1>=1.1.1t-r0' && \
    rm -f /var/cache/apk/*

Context:

cd /tmp
mkdir timing
mv Dockerfile.current Dockerfile.pr237 timing/
cd timing
For the first file, timed with: time docker build --no-cache -f Dockerfile.current -t timing:current .: Attempt time output Duration (sec)
1 0.51s user 0.63s system 39% cpu 2.854 total 2.854
2 0.52s user 0.64s system 40% cpu 2.883 total 2.833
3 0.51s user 0.63s system 39% cpu 2.886 total 2.866
4 0.52s user 0.66s system 39% cpu 3.004 total 3.004
5 0.52s user 0.66s system 40% cpu 2.937 total 2.937

Average: 2.899s.

Second file, timed with time docker build --no-cache -f Dockerfile.pr237 -t timing:pr237 .: Attempt time output Duration (sec)
1 0.52s user 0.66s system 42% cpu 2.817 total 2.817
2 0.53s user 0.67s system 46% cpu 2.603 total 2.603
3 0.53s user 0.67s system 46% cpu 2.668 total 2.668
4 0.52s user 0.65s system 45% cpu 2.672 total 2.672
5 0.52s user 0.64s system 41% cpu 2.767 total 2.767

Average: 2.705s.

So maybe 200ms faster, if that's not just lost in the noise? The entire docker build process for Webdis takes about 23 seconds on the same machine, with --no-cache. It can probably be improved, but changing this part doesn't really seem to make a difference.

zacbayhan commented 10 months ago

TL;DR, caught your update while I was writing this, seems we were going down the same rabbit hole this morning. There might be some optimization that could be made using args at run time.

Thanks for following up on the PR, I agree with you that at face value no --no-cache offers limited upside in terms of image size. However, that is only one metric to gauge a build process of a containerized build process. Also thanks for pointing out that I forgot about removing the repository cache in the final step. Another thing that might be worth looking into is the build time of a specific process. I realize not everyone is worried about this specific metric and people will prioritize what is most important for them and their specific application. So, please bear with me while I try to justify this through a series of highly unsophisticated test cases I haphazardly came up with prior to having my morning dosage of coffee.

Test 1), Create a temporary docker file to investigate if there is any reduction of time in a specific build process from a clean system docker system prune -a.

FROM alpine:latest
RUN apk update && apk add libevent msgpack-c openssl \ 
     'redis>=6.2.10' 'libssl3>=3.0.8-r0' 'libcrypto3>=3.0.8-r1' \ 
     'libssl1.1' 'libcrypto1.1>=1.1.1t-r0' && \ 
     rm -f /var/cache/apk/* /usr/bin/redis-benchmark /usr/bin/redis-cli

running the following command,

I get the real time for the build process of the simple built image taking 2.8 seconds

modifying the dockerfile to (see below) and cleaning the system again docker system prune -a and repeating the process.

FROM alpine:latest

RUN apk add --no-cache libevent msgpack-c openssl \ 
     'redis>=6.2.10' 'libssl3>=3.0.8-r0' 'libcrypto3>=3.0.8-r1' \ 
     'libssl1.1' 'libcrypto1.1>=1.1.1t-r0' && \ 
     rm -f /usr/bin/redis-benchmark /usr/bin/redis-cli

returns a real observed time of 1.9 seconds, this difference could easily be accounted for by network lag in pulling time alpine:latest image so let's repeat the process by having a cached base image.

then timing the builds respectively 1.9s and 1.4s. My guess is this difference in observed time is by reducing the 3 discrete commands down to 2 we get a slight performance in terms of build time. We should test this by looking at the other jq command

Test 2) Comparing

FROM alpine:latest
RUN apk update && apk add wget make gcc libevent-dev msgpack-c-dev musl-dev openssl-dev bsd-compat-headers jq
RUN wget -q https://api.github.com/repos/nicolasff/webdis/tags -O /dev/stdout | jq '.[] | .name' | head -1  | sed 's/"//g' > latest

6.076 Seconds

FROM alpine:latest
RUN apk add --no-cache wget make gcc libevent-dev msgpack-c-dev musl-dev openssl-dev bsd-compat-headers jq
RUN wget -q https://api.github.com/repos/nicolasff/webdis/tags -O /dev/stdout | jq '.[] | .name' | head -1  | sed 's/"//g' > latest

5.251 Seconds

FROM alpine:latest
RUN apk add --no-cache wget make gcc libevent-dev msgpack-c-dev musl-dev openssl-dev bsd-compat-headers jq
RUN wget -q https://api.github.com/repos/nicolasff/webdis/tags -O /dev/stdout | jq -r '.[0] | .name' > latest

5.3 Seconds

Test 3)

modified

FROM alpine:3.17.1 AS stage
LABEL maintainer="Nicolas Favre-Felix <n.favrefelix@gmail.com>"

RUN apk update && apk add wget make gcc libevent-dev msgpack-c-dev musl-dev openssl-dev bsd-compat-headers jq
RUN wget -q https://api.github.com/repos/nicolasff/webdis/tags -O /dev/stdout | jq '.[] | .name' | head -1  | sed 's/"//g' > latest
RUN wget https://github.com/nicolasff/webdis/archive/$(cat latest).tar.gz -O webdis-latest.tar.gz
RUN tar -xvzf webdis-latest.tar.gz
RUN cd webdis-$(cat latest) && make && make install && make clean && make SSL=1 && cp webdis /usr/local/bin/webdis-ssl && cd ..
RUN sed -i -e 's/"daemonize":.*true,/"daemonize": false,/g' /etc/webdis.prod.json

orginal

FROM alpine:3.17.1 AS stage
ARG BUILD_TAG
RUN apk add --no-cache wget make gcc libevent-dev msgpack-c-dev musl-dev openssl-dev bsd-compat-headers jq
RUN wget https://github.com/nicolasff/webdis/archive/${BUILD_TAG}.tar.gz -O webdis-${BUILD_TAG}.tar.gz
RUN tar -xvzf webdis-${BUILD_TAG}.tar.gz
RUN cd webdis-${BUILD_TAG} && make && make install && make clean && make SSL=1 && cp webdis /usr/local/bin/webdis-ssl && cd ..
RUN sed -i -e 's/"daemonize":.*true,/"daemonize": false,/g' /etc/webdis.prod.json
REPOSITORY      TAG       IMAGE ID       CREATED              SIZE
orginal-stage   latest    4630e19ea545   About a minute ago   221MB
test-stage      latest    ccca6c2a8290   2 minutes ago        215MB
<none>          <none>    551465528c08   5 minutes ago        218MB
alpine          latest    7e01a0d0a1dc   2 days ago           7.34MB
nicolasff commented 10 months ago

Sorry, I had missed this reply. Again these are really tiny improvements, barely measurable.

I tried to rebase your changes locally but hit a bunch of conflicts due to the latest release which changed the base image and some dependency versions in related lines so I stopped there, but I think the jq change in d03276f is worth cherry-picking so I'll bring that in.

All this said, your focus on docker build performance made me realize that there is in fact a trivial way to make the build much faster, simply by running make with -j. Applied to the two make commands used for building makes a huge difference. You just have to be careful not to run make -j clean all but this won't be a problem since the two are separate commands on the RUN line.

I ran the following command on the original Dockerfile, after adding -j to the first make command only, to the second make command only, and to both:

time docker build -t webdis:test --no-cache .

Here are the results:

Changes Time (sec) Delta (sec) Improvement
Original Dockerfile 16.787 0 0%
make -j on first build 12.709 4.078 24.29%
make -j SSL=1 on second build 12.404 4.383 26.11%
make -j on both 8.540 8.247 49.13%

See the change here: https://github.com/nicolasff/webdis/compare/master...docker-fast

Thoughts?