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

Dockerfile: upgrade to go 1.12.10 and reduce layer churn around packages #986

Merged
merged 1 commit into from Oct 8, 2019

Conversation

nickvanw
Copy link
Contributor

@nickvanw nickvanw commented Sep 26, 2019

Description

This PR offers some improvements to the bundled Dockerfile for orchestrator, namely:

I did not create an issue as this is a proactive PR - I am happy to do any needed work, but I opted to move to a PR due to security constrains around the bump of Go versioning.

  • contributed code is using same conventions as original code
  • code is formatted via gofmt (please avoid goimports)
  • code is built via ./build.sh
  • code is tested via go test ./go/...

@shlomi-noach
Copy link
Collaborator

Slims down the usage of apk to avoid superflous calls to their registry, as well as the creation of extra layers

so, I did that superfluous apk calls on purpose, let me know what you make of it:

  • if I need to install a new package (change the Dockerfile), and it's a single RUN apk one liner, then the entire RUN command changes, and Docker needs to rebuild the entire list of packages.
  • if I need to install a new package, and I just append a new RUN apk line, Docker uses the cached image, leading to quicker development time.

WDYT?

@nickvanw
Copy link
Contributor Author

nickvanw commented Oct 7, 2019

Hey @shlomi-noach!

I figured it was done on purpose, here's why I think doing it the current way is better for the average case, and for the long term:

  • Installing packages in one command saves a significant number of network round trips to the Alpine Package servers - every call to add --update pulls the fresh package lists, which creates load on their systems, more work for Docker to build, and more time.

  • When package lists are static (e.g. when you're not actively changing and adding frequently), there's near-zero gain to having them in multiple commands. It creates multiple layers, which is just file system and layer churn on Docker's end. Incremental caching may be useful in development, but not for building a release asset that is deployed.

  • For the non-builder image, combining the package installation to a single command creates a single layer, which reduces image file size and pull/push time for everyone who uses these images. I believe it's a safe bet that these images are used more than they're built - optimizing for the final product should be the goal of the main Dockerfile, in my opinion, as this is what users are going to run in CI and/or push and pull frequently.

Ultimately, I think combining everything to reduce layers matches the Docker best practices, optimizes for the common case of building a release asset and not a development tool, and produces the easiest to read, digest and work with Dockerfile, as it follows the accepted Docker best practices, which result in a smaller and more efficient image.

@shlomi-noach
Copy link
Collaborator

I believe it's a safe bet that these images are used more than they're built

That makes perfect sense.

Thank you!

@nickvanw nickvanw merged commit 5c80b6a into master Oct 8, 2019
@nickvanw nickvanw deleted the nickvanw/cleanup-docker branch October 8, 2019 16:23
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.

None yet

2 participants