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

Allow users to use multistage Docker builds based on Docker version #782

Closed
wants to merge 43 commits into from

Conversation

estroz
Copy link
Member

@estroz estroz commented Nov 27, 2018

Description of the change: if docker version >= 17.05 is present, allow users to generate a multistage Dockerfile build/multistage.Dockerfile using operator-sdk build --gen-multistage and use it in building. The SDK will detect whether both docker version >=17.05 is available and a multistage Dockerfile is present, and if the former is true but not the latter will generate a warning to upgrade using --gen-multistage. The warning directs users to move rename multistage.Dockerfile to Dockerfile to avoid the warning in the future. Multistage Dockerfiles are used in tests too.

Motivation for the change: some users might not be restricted by RHEL docker versioning, so they should be able to build Go binaries in a consistent environment. The only way to do so currently is with multistage builds. We should push users with docker version >= 17.05 towards using a multistage build with a warning, but not necessarily require it. This solution creates multistage Dockerfiles for both operator and test binaries.

Closes #167

@estroz estroz added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 27, 2018
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 27, 2018
@estroz estroz force-pushed the choose-multistage branch 2 times, most recently from 8da432c to c07f59a Compare November 27, 2018 22:52
@estroz
Copy link
Member Author

estroz commented Nov 27, 2018

While this PR passes CI, multistage builds cause the memcached e2e test to run for 400 seconds, which normally run in 250 seconds. I tested these changes locally and the result is 220 vs 200 seconds for multistage and non-multistage, respectively. With an increased timeout, the total build time for Go e2e CI with these changes is ~17 minutes, ~3 minutes more than non-multistage. I originally thought this was a buildah issue but it appears to affect both buildah and docker multistage builds in travis VM's. Locally there are no issues.

.travis.yml Outdated Show resolved Hide resolved
@estroz estroz changed the title [WIP] Allow users to use multistage Docker builds based on Docker version Allow users to use multistage Docker builds based on Docker version Nov 27, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2018
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM overall. Just a few typos to fix and some nits.

commands/operator-sdk/cmd/build.go Outdated Show resolved Hide resolved
commands/operator-sdk/cmd/build.go Outdated Show resolved Hide resolved
commands/operator-sdk/cmd/build.go Outdated Show resolved Hide resolved
commands/operator-sdk/cmd/build.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2018
@estroz estroz force-pushed the choose-multistage branch 2 times, most recently from c1f786e to c610b03 Compare November 28, 2018 01:27
@hasbro17
Copy link
Contributor

/hold

There is some more discussion that we need to have around managing multiple layers of Dockerfiles #431 and how we update the Dockerfile for a hybrid operator #670 before we see how a multistage Dockerfile fits into that solution.

We definitely want multistage Dockerfiles but before we open that door and push users to multistage, we need to be clear on how we support multiple base images(single vs multiple Dockerfiles) and how the SDK can control the Dockerfile for any changes that it needs while still allowing the user to add custom build steps.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2018
@estroz estroz force-pushed the choose-multistage branch 2 times, most recently from 74fa600 to 5f252b2 Compare November 30, 2018 18:45
@openshift-bot
Copy link

@estroz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2018
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2019
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 18, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2019
if repo == "" {
for _, dir := range []string{"pkg", "internal"} {
for _, dir := range []string{filepath.Join("internal", "pkg")} {
Copy link
Member Author

Choose a reason for hiding this comment

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

I only copy internal/pkg here because internal/util has dependencies that memcached-operator does not satisfy, which causes a build error. This is a temporary workaround until #1001 is merged, since go mod can resolve non-master commits.

@estroz
Copy link
Member Author

estroz commented Mar 29, 2019

This PR works locally but for some reason e2e tests experience a SIGQUIT timeout in Travis.

@AlexNPavel
Copy link
Contributor

@estroz The default go test timeout is 10 minutes and it's taking more than that do run the tests. In a dual core minikube instance on my laptop it takes 7.5 minutes. The oc cluster is slower than minishift and travis is slower than my laptop's minikube vm, so these tests take too long.

@estroz estroz requested a review from joelanford March 29, 2019 21:37
@lilic
Copy link
Member

lilic commented Apr 3, 2019

If that is the only problem can't we just set the timeout e.g. go test -timeout 20m?

@estroz
Copy link
Member Author

estroz commented Apr 3, 2019

@lilic travis will send a SIGQUIT if there's no test output for more than 10 minutes. I added a travis_wait 20m to the travis config and still no luck.

I'm going to put off this PR until we can figure out a CI solution for slow containerized binary builds.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019
@chrsoo
Copy link

chrsoo commented Apr 3, 2019

@lilic travis will send a SIGQUIT if there's no test output for more than 10 minutes.

@estroz there is no way to refactor the tests to produce output during their execution? Or is this due to a limitation of travic/go test? Can the go test be split in several invocations?

@estroz
Copy link
Member Author

estroz commented Apr 3, 2019

@chrsoo limitations are in image build speed and go test logging. Travis VM's are fairly slow, which combined with slow building of binaries in docker containers results in much longer build times. As of now there's no log streaming from go tests, but they're working on it. Hopefully it'll be in go 1.13 or 1.14.

A workaround in the mean time is to copy the multistage builder Dockerfile into build/Dockerfile (and the test Dockerfile into build/test-framework/Dockerfile if you're running e2e tests). Then operator-sdk build will build your operator images using multistage builds, assuming you have docker 17.05+.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2019
@openshift-ci-robot
Copy link

@estroz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@estroz estroz closed this May 31, 2019
@estroz estroz deleted the choose-multistage branch April 1, 2020 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use multi-stage Dockerfile instead of building using host's Go binary
9 participants