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

support buildah and operator multi-stage builds #563

Closed
wants to merge 17 commits into from

Conversation

estroz
Copy link
Member

@estroz estroz commented Oct 2, 2018

buildah is a tool that builds OCI images, from its CLI interface or by interpreting Dockerfiles. RHEL does not support docker above v1.13, preventing the SDK from leveraging the multi-stage build Dockerfile feature; buildah supports this feature, and will continue to be supported in RHEL. See #431 for relevant discussion.

This PR also adds CI support for buildah by building the latest version in an image and copying the binary, as well as that of runc which is required to run buildah bud ... in our case, into the build VM.

Relevant issues: #167

@estroz estroz added kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. documentation labels Oct 2, 2018
@estroz estroz force-pushed the buildah-crio-prototype branch 2 times, most recently from b5e00b0 to 9bdd8cd Compare October 2, 2018 23:13
@shawn-hurley
Copy link
Member

@estroz Looks like this forces users to use buildah. I wonder if there is some way to tell the build command which tool(buildah, docker) to use instead. It does not appear that I can install buildah on mac os x without installing from source, which I think is a pretty big barrier of entry IMO.

@mhrivnak
Copy link
Member

mhrivnak commented Oct 3, 2018

Note: this code change will break operator-sdk build if buildah v1.3+ is not installed, as the rootless option was added in v1.3. RHEL currently supports v1.2; a v1.3 binary should be available soon.

I assume this only "breaks" setups where a user has enabled running docker as non-root, which is horribly unsafe. I don't think the SDK should feel any obligation to preserve that experience.

That said, using buildah in general is a nice improvement, and its rootless feature will be a great benefit to SDK users. Thanks for adding support.

Copy link
Contributor

@spahl spahl left a comment

Choose a reason for hiding this comment

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

Using buildah is nice but as Shawn said we don't want to force the dependency. I would prefer the code to detect it and use it if present; otherwise fallback to docker. Also if nothing is present let's be nice and fail with an error message instead of deep in the code.

@AlexNPavel
Copy link
Contributor

Looking a bit more at buildah, it seems that 1 feature they are missing that we need is ARG-before-FROM functionality, which is in docker-ce 17+ (which technically means it is also not supported in fedora or RHEL without using docker's own rpm repos either). We need the ARG-before-FROM feature for the --enable-tests flag of build.

There is an issue tracking this, but there hasn't been much activity: containers/buildah#581

@estroz
Copy link
Member Author

estroz commented Oct 12, 2018

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2018
@estroz estroz force-pushed the buildah-crio-prototype branch 2 times, most recently from c90530f to 76be138 Compare October 25, 2018 23:51
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 25, 2018
@estroz estroz requested review from mhrivnak and removed request for fanminshi October 25, 2018 23:57
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 26, 2018
@estroz estroz force-pushed the buildah-crio-prototype branch 2 times, most recently from b596193 to 9cfdf7d Compare October 26, 2018 01:03
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2018
Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

LGTM other than some small changes to the dockerfiles

pkg/scaffold/build_dockerfile.go Outdated Show resolved Hide resolved
pkg/scaffold/build_dockerfile.go Outdated Show resolved Hide resolved
pkg/scaffold/test_framework_dockerfile.go Outdated Show resolved Hide resolved
@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 21, 2018
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 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 Jan 23, 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 Jan 23, 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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2019
shawn-hurley pushed a commit that referenced this pull request Apr 17, 2019
**Description of the change:**
This PR adds buildah support.

**Motivation for the change:**
operator-sdk should support building image by using buildah. 
There is an existing discussion for motivation in #563 .
@estroz
Copy link
Member Author

estroz commented Apr 30, 2019

Closing as this doesn't add much past what #1311 does. Multi-stage support will be addressed in the future, see #782 for details.

@estroz estroz closed this Apr 30, 2019
@estroz estroz deleted the buildah-crio-prototype 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
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants