Remove proxy/Dockerfile-deps #279

olix0r · 2018-02-05T23:06:40Z

The current proxy Dockerfile configuration does not cache dependencies well, which can increase build times substantially.

By carefully splitting proxy/Dockerfile into several stages that mock parts of the project, dependencies may be built and cached in Docker such that changes to the proxy only require building the conduit-proxy crate.

Furthermore, proxy/Dockerfile now runs the proxy's tests before producing an artifact, unless the SKIP_TESTS build-arg is set and not-empty.

The BUILD_UNOPTIMIZED build-arg has been added to support quicker, debug-friendly builds.

bin/docker-build-proxy has been changed to admit arbitrary command-line arguments that are passed to docker.

siggy

By my reading of this, my default build command will become:

DOCKER_TRACE=1 BUILD_UNOPTIMIZED=1 SKIP_TESTS=1 bin/mkube bin/docker-build

...BUILD_UNOPTIMIZED and SKIP_TESTS are both nice build improvements. Can we consider making bin/docker-build leverage these build behaviors by default? CI and more official release scripts seem like better places to explicitly flip switches like these with more verbose commands.

Got a build error around conduit-grpc, perhaps something broke in the PR split:

$ DOCKER_TRACE=1 time bin/docker-build
...
Removing intermediate container 90b45479bf82
Step 38/54 : RUN if [ -n "$BUILD_UNOPTIMIZED" ];     then cargo build -p conduit-grpc --features=arbitrary --frozen ;     else cargo build -p conduit-grpc --features=arbitrary --frozen --release ;     fi
 ---> Running in 5c9e6110f2ab
error: package id specification `conduit-grpc` matched no packages
The command '/bin/sh -c if [ -n "$BUILD_UNOPTIMIZED" ];     then cargo build -p conduit-grpc --features=arbitrary --frozen ;     else cargo build -p conduit-grpc --features=arbitrary --frozen --release ;     fi' returned a non-zero code: 101

siggy · 2018-02-05T23:39:33Z

bin/docker-build

+
+bin/docker-build-proxy \
+    --build-arg="SKIP_TESTS=${SKIP_TESTS:-}" \
+    --build-arg="BUILD_UNOPTIMIZED=${PROXY_UNOPTIMIZED:-}"


can we settle on one of BUILD_UNOPTIMIZED / PROXY_UNOPTIMIZED. i'd prefer the latter as it more explicitly affects the proxy build.

in the context of proxy/Dockerfile, it's weird to have PROXY_UNOPTIMIZED, as it relates to much more than just the proxy. In the context of bin/docker-build, BUILD_UNOPTIMIZED is too generic. I suppose we can just use PROXY_UNOPTIMIZED but i think it's weird...

I agree with siggy.

siggy · 2018-02-05T23:42:03Z

proxy/Dockerfile

+#
+# Mocks out all local code and fetch external dependencies to ensure that external sources
+# are cached.
+FROM $RUST_IMAGE as fetch


not sure how actionable this is, or how much it will affect build performance, but these multistage builds come at some cost to disk space:

$ docker images REPOSITORY TAG IMAGE ID CREATED SIZE gcr.io/runconduit/proxy git-03261aaf c0a0a95e20f9 6 minutes ago 109 MB <none> <none> aa25ba002caa 6 minutes ago 2.02 GB <none> <none> a59dda926066 44 minutes ago 1.93 GB <none> <none> 064aba0af483 46 minutes ago 1.88 GB <none> <none> ee978ff534a7 50 minutes ago 1.78 GB

At least we're no longer trying to ship these intermediates around the network ;)

It does seem unfortunate that there is so much disk space being used but I think we can improve that later.

olix0r · 2018-02-06T00:18:04Z

BUILD_UNOPTIMIZED and SKIP_TESTS are both nice build improvements. Can we consider making bin/docker-build leverage these build behaviors by default?

I'm wary of using BUILD_UNOPTIMIZED by default. It results in much worse proxy performance. It'd be terrible for one of these builds to get used unintentionally. I'm specifically worried about having to set additional flags when doing a release...

briansmith · 2018-02-06T00:16:11Z

.travis.yml


      script:
-        - bin/docker-build
+        - SKIP_TESTS=1 bin/docker-build


In the previous version of this PR I asked why we're skipping the tests here and you said we're skipping them because the tests have already been run at this point. At least let's add a comment mentioning that here.

I also filed #280 to clean this up, as I don't think we should continue to maintain without-Docker and with-Docker build scripts going forward.

briansmith · 2018-02-06T00:16:48Z

.travis.yml


      after_success:
        - bin/docker-push-deps
        - bin/docker-push $CONDUIT_TAG
        - bin/docker-retag-all $CONDUIT_TAG master && bin/docker-push master
-        - target/cli/linux/conduit install --conduit-version=$CONDUIT_TAG |tee conduit.yml
+        - target/cli/linux/conduit install --version=$CONDUIT_TAG |tee conduit.yml


This looks like a bad merge; didn't we just rename --version to --conduit-version?

--conduit-version

briansmith · 2018-02-06T00:17:35Z

bin/docker-build

+
+bin/docker-build-proxy \
+    --build-arg="SKIP_TESTS=${SKIP_TESTS:-}" \
+    --build-arg="BUILD_UNOPTIMIZED=${PROXY_UNOPTIMIZED:-}"


I agree with siggy.

briansmith · 2018-02-06T00:19:17Z

proxy/Dockerfile

+#
+# Mocks out all local code and fetch external dependencies to ensure that external sources
+# are cached.
+FROM $RUST_IMAGE as fetch


It does seem unfortunate that there is so much disk space being used but I think we can improve that later.

briansmith · 2018-02-06T00:20:34Z

proxy/Dockerfile

-FROM gcr.io/runconduit/proxy-deps:673c53de as build
+# Mocks proxy sources to ensure that dependencies, including build-time dependencies
+# needed to generate the bindings, are cached.
+FROM $RUST_IMAGE as grpc


Please add a comment about why this is a separate named stage, instead of just being the end of the previous stage or the beginning of the next stage. (AFAICT Docker will cache things about the same whichever way is chosen.)

briansmith · 2018-02-06T00:21:36Z

proxy/Dockerfile

+## Build libraries.
+#
+# Mocks proxy sources and gRPC bindings to ensure that dependencies are cached.
+FROM $RUST_IMAGE as libs


Please add a comment about why this is a separate named stage, instead of just being the end of the previous stage.

It seems like everything should work just fine if fetch and libs were combined into a single stage. Am I overlooking something?

briansmith · 2018-02-06T00:32:33Z

To clarify my previous comments, it's unclear why our Dockerfiles here are so much more complicated than what's described at rust-lang/cargo#2644 and https://whitfin.io/speeding-up-rust-docker-builds/. I would love to see them simplified so that it's easier for Docker non-exports (i.e. myself) to understand and maintain them. However, if the extra complication actually does have significant benefits then those benefits should be documented.

siggy

hmm, still broken...

$ DOCKER_TRACE=1 time bin/docker-build

...

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests conduit-proxy

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

 ---> 3a5477cab8b3
Removing intermediate container c3b9c66e85cd
Step 50/54 : RUN if [ -z "$BUILD_OPTIMIZED" ];     then mv target/debug/conduit-proxy   target/conduit-proxy ;     else mv target/release/conduit-proxy target/conduit-proxy ;     fi
 ---> Running in 98a38b1c83cb
mv: cannot stat 'target/debug/conduit-proxy': No such file or directory
The command '/bin/sh -c if [ -z "$BUILD_OPTIMIZED" ];     then mv target/debug/conduit-proxy   target/conduit-proxy ;     else mv target/release/conduit-proxy target/conduit-proxy ;     fi' returned a non-zero code: 1
     1187.08 real        10.57 user         3.50 sys

olix0r · 2018-02-06T01:02:36Z

:; docker image ls |head -n 3                                                          
REPOSITORY                                               TAG                 IMAGE ID            CREATED             SIZE
gcr.io/runconduit/proxy                                  git-f0280775        474238e0836f        4 minutes ago       55.8MB
<none>                                                   <none>              3043b6589200        4 minutes ago       2.3GB

olix0r · 2018-02-06T17:17:01Z

To summarize an offline conversation with @siggy:

SKIP_TESTS is enabled by default because, if we have a build artifact, we generally want to trust that it passed tests.
Note that SKIP_TESTS can be enabled/disabled without impacting caching, since this is basically the last thing done in the build stage.
SKIP_TESTS should probably be renamed PROXY_SKIP_TESTS or something....
PROXY_UNOPTIMIZED should not be the default, because it produces a substantially worse (latency, size, etc) proxy. Our thought is that developers who do not work on the proxy should only have to pay the build-time penalty rarely. Developers who are working on the proxy should know (i.e. we should document in BUILD.md) to set BUILD_UNOPTIMIZED and/or SKIP_TESTS when quickly iterating on the proxy.

siggy

lgtm.

per offline convo, we should move away from explicit env vars like SKIP_TESTS and PROXY_UNOPTIMIZED, in favor of simply passing docker build args into the docker-build* scripts.

siggy · 2018-02-06T01:29:07Z

.travis.yml


      after_success:
        - bin/docker-push-deps
        - bin/docker-push $CONDUIT_TAG
        - bin/docker-retag-all $CONDUIT_TAG master && bin/docker-push master
-        - target/cli/linux/conduit install --conduit-version=$CONDUIT_TAG |tee conduit.yml
+        - target/cli/linux/conduit install --version=$CONDUIT_TAG |tee conduit.yml


--conduit-version

olix0r · 2018-02-06T17:41:54Z

@siggy I reverted my changes to docker-build-proxy so that it behaves like other docker-build scripts today. If we want to change the behavior of these later, we can revisit that. (Too large a change to make now.)

siggy

👍

$ time env PROXY_UNOPTIMIZED=1 SKIP_TESTS=1 DOCKER_TRACE=1 bin/docker-build-proxy
...
real	0m13.040s
user	0m1.465s
sys	0m0.461s

The current proxy Dockerfile configuration does not cache dependencies well, which can increase build times substantially. By carefully splitting proxy/Dockerfile into several stages that mock parts of the project, dependencies may be built and cached in Docker such that changes to the proxy only require building the conduit-proxy crate. Furthermore, proxy/Dockerfile now runs the proxy's tests before producing an artifact, unless the `SKIP_TESTS` build-arg is set and not-empty. The `BUILD_UNOPTIMIZED` build-arg has been added to support quicker, debug-friendly builds. `bin/docker-build-proxy` has been changed to admit arbitrary command-line arguments that are passed to `docker`.

This reduces build times by avoiding extra COPYing of large target dirs and results in fewer images with lesser total size.

Remove changes to docker-build-proxy, so it behaves like all other docker-builds.

olix0r requested review from klingerf, briansmith and siggy February 5, 2018 23:06

olix0r added the review/ready Issue has a reviewable PR label Feb 5, 2018

siggy assigned olix0r Feb 5, 2018

siggy reviewed Feb 5, 2018

View reviewed changes

briansmith reviewed Feb 6, 2018

View reviewed changes

siggy added the area/build label Feb 6, 2018

siggy reviewed Feb 6, 2018

View reviewed changes

olix0r force-pushed the ver/no-proxy-deps-2 branch from 261b1ae to bf1a041 Compare February 6, 2018 02:22

siggy approved these changes Feb 6, 2018

View reviewed changes

olix0r added 8 commits February 6, 2018 20:33

fixup! proxy/Dockerfile

5864983

Elide intermediate stages

ab80a56

This reduces build times by avoiding extra COPYing of large target dirs and results in fewer images with lesser total size.

move image name args to top of file to appease older dockers

e504fd8

Fix merge conflict in travis

0c496ed

Add comment in travis explaining SKIP_TESTS

e544bbe

Rename SKIP_TESTS to PROXY_SKIP_TESTS

a54424d

Remove changes to docker-build-proxy, so it behaves like all other docker-builds.

Update BUILD.md with docker-build-proxy instructions

1f2b36b

olix0r changed the base branch from ver/split-grpc-bindings to master February 6, 2018 20:33

olix0r force-pushed the ver/no-proxy-deps-2 branch from 2d75508 to 1f2b36b Compare February 6, 2018 20:34

olix0r merged commit 6a0936e into master Feb 6, 2018

olix0r deleted the ver/no-proxy-deps-2 branch February 6, 2018 21:01

olix0r removed the review/ready Issue has a reviewable PR label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove proxy/Dockerfile-deps #279

Remove proxy/Dockerfile-deps #279

olix0r commented Feb 5, 2018

siggy left a comment •

edited

Loading

siggy Feb 5, 2018

olix0r Feb 6, 2018

briansmith Feb 6, 2018

siggy Feb 5, 2018

olix0r Feb 6, 2018

briansmith Feb 6, 2018

olix0r commented Feb 6, 2018

briansmith Feb 6, 2018

briansmith Feb 6, 2018

siggy Feb 6, 2018

briansmith Feb 6, 2018

briansmith Feb 6, 2018

briansmith Feb 6, 2018

briansmith Feb 6, 2018

briansmith commented Feb 6, 2018

siggy left a comment •

edited

Loading

olix0r commented Feb 6, 2018 •

edited

Loading

olix0r commented Feb 6, 2018

siggy left a comment

siggy Feb 6, 2018

olix0r commented Feb 6, 2018

siggy left a comment

Remove proxy/Dockerfile-deps #279

Remove proxy/Dockerfile-deps #279

Conversation

olix0r commented Feb 5, 2018

siggy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith commented Feb 6, 2018

siggy left a comment • edited Loading

Choose a reason for hiding this comment

olix0r commented Feb 6, 2018 • edited Loading

olix0r commented Feb 6, 2018

siggy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r commented Feb 6, 2018

siggy left a comment

Choose a reason for hiding this comment

siggy left a comment •

edited

Loading

siggy left a comment •

edited

Loading

olix0r commented Feb 6, 2018 •

edited

Loading