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

Add multi-stage builds support #495

Closed
wants to merge 3 commits into from

Conversation

bertinatto
Copy link
Contributor

@bertinatto bertinatto commented Feb 24, 2018

This PR adds multi-stage builds support to buildah.

Addresses #440.

@rhatdan
Copy link
Member

rhatdan commented Feb 24, 2018

@nalind PTAL

@TomSweeneyRedHat
Copy link
Member

Travis CI isn't happy @bertinatto

not ok 13 bud-from-multiple-files-two-froms
# (in test file ./bud.bats, line 51)
#   `cmp $root/Dockerfile1 ${TESTSDIR}/bud/from-multiple-files/Dockerfile1.scratch' failed with status 2

@bertinatto
Copy link
Contributor Author

Right, I have a question about this failing test (bud-from-multiple-files-two-froms).

In a multi-stage build, each FROM statement will result in a different stage (i.e., container). I believe this specific test made sense so far, but from now on I'd assume that files added to a stage shouldn't end up in the final stage.

What do you think about this?

@bertinatto bertinatto force-pushed the multi_stage branch 2 times, most recently from f807c99 to ecec5e2 Compare February 25, 2018 07:56
@bertinatto bertinatto changed the title [WIP] Add multi-stage builds support Add multi-stage builds support Feb 25, 2018
@rhatdan
Copy link
Member

rhatdan commented Feb 26, 2018

I am no expert on this. but isn't multi-stage the equivalent of doing two different Dockerfile, where the second Dockerfile from is from the artifact of the first Dockerfile. Anything that is in the image created by the first Dockerfile would end up in the second unless it was at some point removed?

@kbaegis
Copy link

kbaegis commented Feb 26, 2018

@rhatdan Here's an example:
https://github.com/gentoo/gentoo-docker-images/blob/master/portage.Dockerfile

This is all based around the flexibility of needing to use one images utilities to bootstrap the toolchain for another (i.e. - using alpine gpg to validate or busybox to copy/untar).

@bertinatto
Copy link
Contributor Author

@rhatdan, AFAIU the idea is that we can selectively copy artifacts from one stage to another, leaving the final image without unnecessary artifacts.

Using the example of #440, the first FROM starts a new stage with the base image golang where the binary grafeas-server is built. The second stage is built using alpine as base image and only the file grafeas-server is copied to it, leaving behind the Go SDK.

@rhatdan
Copy link
Member

rhatdan commented Feb 26, 2018

Do these workloads work with this patch?

@bertinatto
Copy link
Contributor Author

Yup, but unfortunately multi-stage builds support changes the way buildah (and imagebuilder) work with multiple Dockerfiles (i.e., multiple FROM statements).

@rhatdan
Copy link
Member

rhatdan commented Mar 8, 2018

@nalind PTAL

@rhatdan
Copy link
Member

rhatdan commented Mar 18, 2018

@ipbabble @TomSweeneyRedHat Could you try out a couple of Dockerfiles against this to make sure it works fairly well.

I would like to get this merged along with SHELL Support so we can cut a new version of Buildah.

@TomSweeneyRedHat
Copy link
Member

FWIW, I tested using both of the below Dockerfiles and these worked. I tried a couple of other more complex Dockerfiles that I found on the net and they had failures, but the same failure happened in Docker too. I don't believe it was a Buildah issue. If you've a pointer to other multi-stage Dockerfiles, happy to try them.

FROM ubuntu
RUN whoami
FROM ubuntu
RUN echo hi

and

FROM golang:1.7.3
WORKDIR /go/src/github.com/alexellis/href-counter/
RUN go get -d -v golang.org/x/net/html  
COPY app.go .
RUN CGO_ENABLED=0 GOOS=linux go build -a -installsuffix cgo -o app .

FROM alpine:latest  
RUN apk --no-cache add ca-certificates
WORKDIR /root/
COPY --from=0 /go/src/github.com/alexellis/href-counter/app .
CMD ["./app"]  

@@ -113,6 +113,8 @@ type BuildOptions struct {
// Executor is a buildah-based implementation of the imagebuilder.Executor
// interface.
type Executor struct {
name string
Copy link
Member

Choose a reason for hiding this comment

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

Please add a field for the numeric index of the stage which this Executor is responsible for building. We'll need that information to ensure that COPY instructions that try to reference this stage or a later stage, can be rejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, although I think it was rejecting invalid stages before.

@@ -329,6 +343,12 @@ func (b *Executor) Copy(excludes []string, copies ...imagebuilder.Copy) error {
for _, src := range copy.Src {
if strings.HasPrefix(src, "http://") || strings.HasPrefix(src, "https://") {
sources = append(sources, src)
} else if len(copy.From) > 0 {
if other, ok := b.named[copy.From]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to check that the stage number for other is less than the one for b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
var stageExecutor *Executor
for _, stage := range stages {
stageExecutor = b.withName(stage.Name)
Copy link
Member

Choose a reason for hiding this comment

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

This should take the index of the stage as an argument, since whether a name for the stage is specified with AS or not, its contents still need to be accessible by referring to the stage using its numeric index, and we want to know where a given Executor is in the list of them. Converting the number to a string in withName() and using it as another key for adding copied to the named map, since the values are pointers, should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for catching this.

I checked the behaviour of docker and it does accept both indexes and names. On the other hand, imagebuilder does not. We might report this.

@nalind
Copy link
Member

nalind commented Apr 3, 2018

Apologies for the delay. A couple of notes about stage ordering and numbers, but the rest looks really good. Thanks!

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably 5ce8009) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Member

rhatdan commented Apr 4, 2018

@bertinatto Needs a rebase.

@bertinatto
Copy link
Contributor Author

@nalind, thanks for the review. I'll apply your suggestions as soon as I have time (probably over the weekend, if that's not a problem).

@rhatdan, will do, thanks!

@ipbabble
Copy link
Contributor

ipbabble commented Apr 5, 2018 via email

@bertinatto bertinatto force-pushed the multi_stage branch 5 times, most recently from 64a78ab to 847aff2 Compare April 10, 2018 17:56
@nalind
Copy link
Member

nalind commented Apr 10, 2018

LGTM pending CI.

Signed-off-by: Fabio Bertinatto <fbertina@redhat.com>
Signed-off-by: Fabio Bertinatto <fbertina@redhat.com>
Signed-off-by: Fabio Bertinatto <fbertina@redhat.com>
@bertinatto
Copy link
Contributor Author

I had to close/reopen to retest. All green now.

@nalind
Copy link
Member

nalind commented Apr 10, 2018

Thanks!
@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit b5ef4be has been approved by nalind

@rh-atomic-bot
Copy link
Collaborator

⚡ Test exempted: pull fully rebased and already tested.

rh-atomic-bot pushed a commit that referenced this pull request Apr 10, 2018
Signed-off-by: Fabio Bertinatto <fbertina@redhat.com>

Closes: #495
Approved by: nalind
rh-atomic-bot pushed a commit that referenced this pull request Apr 10, 2018
Signed-off-by: Fabio Bertinatto <fbertina@redhat.com>

Closes: #495
Approved by: nalind
@rhatdan
Copy link
Member

rhatdan commented Apr 11, 2018

Time for a new build? I think this could be buildah 1.0? @nalind @TomSweeneyRedHat WDYT

@bertinatto
Copy link
Contributor Author

My vote doesn't count, but it would be awesome to have buildah 1.0!

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Apr 11, 2018 via email

@boaz0
Copy link
Collaborator

boaz0 commented Apr 11, 2018

/me being excited! 😀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants