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.

Sign up for GitHub

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

Jump to bottom

[dockerfile2llb] fix daemon crash: check for circular dependency in convert #999

Merged
merged 4 commits into from
May 12, 2019
Merged

[dockerfile2llb] fix daemon crash: check for circular dependency in convert #999

merged 4 commits into from
May 12, 2019

Conversation

Blyschak
Copy link
Contributor

Signed-off-by: Stepan Blyshchak stepanblischak@gmail.com

By creating a simple dockerfile like this:

FROM alpine:latest AS stage0
FROM alpine:latest AS stage1
COPY --from=stage1 bin /

and building using following command:
DOCKER_BUILDKIT=1 /usr/bin/docker build -t test .

dockerd crashes (part of log):

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x1d33169, 0xe)
        /usr/local/go/src/runtime/panic.go:617 +0x74
runtime.newstack()
        /usr/local/go/src/runtime/stack.go:1041 +0x6f4
runtime.morestack()
        /usr/local/go/src/runtime/asm_amd64.s:429 +0x84

goroutine 677 [running]:
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0x0, 0xc0008b8900, 0xffffffffffffffff)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1118 +0x15c fp=0xc024000360 sp=0xc024000358 pc=0x161990c
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000400)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1122 +0x128 fp=0xc0240003e8 sp=0xc024000360 pc=0x16198d8
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000488)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1126 +0xcd fp=0xc024000470 sp=0xc0240003e8 pc=0x161987d
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000510)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1126 +0xcd fp=0xc0240004f8 sp=0xc024000470 pc=0x161987d
github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.isReachable(0xc0008b8c00, 0xc0008b8900, 0xc024000598)
        /go/src/github.com/docker/docker/vendor/github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1126 +0xcd fp=0xc024000580 sp=0xc0240004f8 pc=0x161987d

Interestingly that dockerfile:

FROM alpine:latest AS stage1
COPY --from=stage2 bin /

FROM alpine:latest AS stage2
COPY --from=stage1 bin /

does not crashes that daemon however there is a cyclic dependency on stages and IMO resulted image is undefined.
Lagacy builder with same dockerfile fails with a another error:

root@819191141f53:~/test# /usr/bin/docker build -t test .
Sending build context to Docker daemon  2.048kB
Step 1/4 : FROM alpine:latest AS stage1
 ---> cdf98d1859c1
Step 2/4 : COPY --from=stage2 bin /
invalid from flag value stage2: pull access denied for stage2, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

I have added a check for circular dependency after convert fills all dependencies in, so now I get:

root@819191141f53:~/test# cat Dockerfile
FROM alpine:latest AS stage1
COPY --from=stage2 bin /

FROM alpine:latest AS stage2
COPY --from=stage1 bin /

root@819191141f53:~/test# DOCKER_BUILDKIT=1 /usr/bin/docker build -t test .
[+] Building 0.2s (2/2) FINISHED
 => [internal] load build definition from Dockerfile                                                           0.1s
 => => transferring dockerfile: 31B                                                                            0.0s
 => [internal] load .dockerignore                                                                              0.1s
 => => transferring context: 2B                                                                                0.0s
failed to create LLB definition: circular dependency detected on stage: stage1

@AkihiroSuda
Copy link
Member 

#12 [4/4] RUN --mount=target=/go/src/github.com/moby/buildkit 	gometalinter ...
#12 0.785 frontend/dockerfile/dockerfile2llb/convert.go:1::warning: file is not gofmted with -s (gofmt)
#12 1.955 frontend/dockerfile/dockerfile2llb/convert.go:1::warning: file is not goimported (goimports)
#12 ERROR: executor failed running [/bin/sh -c gometalinter --config=gometalinter.json ./...]: exit code: 1

AkihiroSuda
AkihiroSuda reviewed May 11, 2019
View reviewed changes
frontend/dockerfile/dockerfile2llb/convert.go
@@ -1130,6 +1134,41 @@ func isReachable(from, to *dispatchState) (ret bool) {
return false
}

func hasCircularDependency(states []*dispatchState) (bool, *dispatchState) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have UT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, added "TestDockerfileCircularDependencies"

AkihiroSuda
AkihiroSuda reviewed May 11, 2019
View reviewed changes
frontend/dockerfile/dockerfile2llb/convert.go Outdated
visited := make(map[*dispatchState]struct{})
path := make(map[*dispatchState]struct{})

visit = func (state *dispatchState) bool {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can be visit := ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

visit is called recursively, I get "undefined: visit" when using " := "; this is the go's order of evaluation

Blyschak added 4 commits May 11, 2019 18:25
@Blyschak
[dockerfile2llb] check for circular dependency in convert
ad4ae59 
Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>
@Blyschak
apply gofmt
98a6ff4 
Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>
@Blyschak
[convert] move 'stageName = ""' in covert after checking for circular…
981ee38 
… dependency

To have stageName in error output in case one stage depends on itself

Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>
@Blyschak
[convert_test] add ut for circular dependency cases in dockerfile
ea4bb02 
Signed-off-by: Stepan Blyshchak <stepanblischak@gmail.com>
@Blyschak Blyschak force-pushed the fix-inf-recursion branch from 1e6367b to ea4bb02 Compare May 11, 2019 16:49
@tonistiigi tonistiigi merged commit 255defe into moby:master May 12, 2019
@tonistiigi
Copy link
Member

Thanks @Blyschak

@Blyschak Blyschak deleted the fix-inf-recursion branch May 12, 2019 10:43
@AkihiroSuda
Copy link
Member

This should be back ported to Docker 18.09?

@tonistiigi tonistiigi mentioned this pull request May 13, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request May 14, 2019
Merged
@Blyschak
Copy link
Contributor Author

@AkihiroSuda I have no specific wish to back port it to 18.09, so only if you want

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Blyschak @AkihiroSuda @tonistiigi