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

Client-side frontend leaks local source snapshots. #581

Closed
ijc opened this issue Aug 20, 2018 · 0 comments
Closed

Client-side frontend leaks local source snapshots. #581

ijc opened this issue Aug 20, 2018 · 0 comments
Labels
kind/bug

Comments

@ijc
Copy link
Collaborator

ijc commented Aug 20, 2018

I first noticed this with #552. With that PR it can be reproduced with:

make test TESTFLAGS="-v -test.run TestDockerfile.*Integration/TestNoSnapshotLeak" TESTPKGS=./frontend/dockerfile/...

The result includes:

$ grep -E PASS:\|FAIL: test.log 
--- PASS: TestDockerfileBuiltinIntegration (0.32s)
    --- PASS: TestDockerfileBuiltinIntegration/TestNoSnapshotLeak/worker=oci (5.64s)
    --- PASS: TestDockerfileBuiltinIntegration/TestNoSnapshotLeak/worker=containerd (5.94s)
    --- PASS: TestDockerfileBuiltinIntegration/TestNoSnapshotLeak/worker=containerd-1.0 (6.16s)
--- PASS: TestDockerfileMasterGatewayIntegration (0.46s)
    --- PASS: TestDockerfileMasterGatewayIntegration/TestNoSnapshotLeak/worker=containerd (10.37s)
    --- PASS: TestDockerfileMasterGatewayIntegration/TestNoSnapshotLeak/worker=oci (10.45s)
    --- PASS: TestDockerfileMasterGatewayIntegration/TestNoSnapshotLeak/worker=containerd-1.0 (11.01s)
--- FAIL: TestDockerfileClientIntegration (0.51s)
    --- FAIL: TestDockerfileClientIntegration/TestNoSnapshotLeak/worker=oci (5.41s)
    --- FAIL: TestDockerfileClientIntegration/TestNoSnapshotLeak/worker=containerd-1.0 (6.16s)
    --- FAIL: TestDockerfileClientIntegration/TestNoSnapshotLeak/worker=containerd (6.73s)

A slightly easier reproducer is to use examples/build-using-dockerfile with --clientside-frontend:

$ go build ./examples/build-using-dockerfile
$ cat Dockerfile.test 
FROM scratch
COPY foo /foo
$ ./build-using-dockerfile --clientside-frontend -f Dockerfile.test -t foo .
[...]
$ buildctl du --verbose | sed -E -e 's/((Usage count|Last used):\s+).*/\1«elided»/g' > du0.log
$ ./build-using-dockerfile --clientside-frontend -f Dockerfile.test -t foo .
[...]
$ buildctl du --verbose | sed -E -e 's/((Usage count|Last used):\s+).*/\1«elided»/g' > du1.log

And the resulting diff

--- du0.log	2018-08-20 10:18:05.775130507 +0100
+++ du1.log	2018-08-20 10:18:09.523190354 +0100
@@ -74,6 +74,16 @@
 Usage count:	«elided»
 Type:		regular
 
+ID:		up4bcbsn7q2a8nqvamx0qomwn
+Created at:	2018-08-20 09:18:07.234560484 +0000 UTC
+Mutable:	false
+Reclaimable:	false
+Shared:		false
+Size:		4.12kB
+Description:	local source for dockerfile
+Usage count:	«elided»
+Type:		regular
+
 ID:		tx5zxtr4ybe6z6ww925anf17i
 Created at:	2018-08-20 09:18:02.6588382 +0000 UTC
 Mutable:	false
@@ -84,6 +94,16 @@
 Usage count:	«elided»
 Type:		regular
 
+ID:		dwz52rjthzctf503u7e72jg0n
+Created at:	2018-08-20 09:18:07.258977609 +0000 UTC
+Mutable:	false
+Reclaimable:	false
+Shared:		false
+Size:		4.11kB
+Description:	local source for context
+Usage count:	«elided»
+Type:		regular
+
 ID:		fb7ooahft8rnfvfetavf5okxr
 Created at:	2018-08-20 09:18:02.712236577 +0000 UTC
 Mutable:	true
@@ -96,4 +116,4 @@
 Type:		source.local
 
 Reclaimable:	3.48MB
-Total:		3.49MB
+Total:		3.50MB

I think the issue is the Type: regular. It should be source.local I think (and there are others with that type in the list). Each run leaks another pair.

I can repro with both oci and containerd worker. Without --clientside-frontend the is no leak.

I'll investigate today, just filing since I'm away from Wednesday.
ijc pushed a commit to ijc/buildkit that referenced this issue Aug 20, 2018
clientside-frontend: release non-result refs after completion
cb2fb4f 
Tests such as TestNoSnapshotLeak were failing in client mode (e.g. using moby#522)
because we weren't releasing the intermediate refs.

Resolve this by refactoring the existing code which frees the intermediate refs
from `gatewayFrontend.Solve` into a method on `llbBridgeForwarder` and as well
as the original site also call from the solver when the top-level solve (in
clientside frontend mode) completes. The original call (which is via a defer)
could likely sensibly be moved either earlier or later if desired but leave it
here it is to minimise the scope of the change.

The previous code used the `retErr` named return but the code between that
point and the end of the function already ensured that `lbf.err` is the same as
`retErr`. Note that the `res` named return was previously unused by name.

Fixes moby#581.

Signed-off-by: Ian Campbell <ijc@docker.com>
ijc pushed a commit to ijc/buildkit that referenced this issue Aug 20, 2018
clientside-frontend: release non-result refs after completion
c8344e3 
Tests such as TestNoSnapshotLeak were failing in client mode (e.g. using moby#522)
because we weren't releasing the intermediate refs.

Resolve this by refactoring the existing code which frees the intermediate refs
from `gatewayFrontend.Solve` into a method on `llbBridgeForwarder` and as well
as the original site also call from the solver when the top-level solve (in
clientside frontend mode) completes. The original call (which is via a defer)
could likely sensibly be moved either earlier or later if desired but leave it
here it is to minimise the scope of the change.

The previous code used the `retErr` named return but the code between that
point and the end of the function already ensured that `lbf.err` is the same as
`retErr`, thus the only change in the code which has moved is
`s/retErr/lbf.err/`. Note that the `res` named return was previously unused by
name.

Fixes moby#581.

Signed-off-by: Ian Campbell <ijc@docker.com>
@ijc ijc mentioned this issue Aug 20, 2018
Merged
ijc pushed a commit to ijc/buildkit that referenced this issue Aug 30, 2018
clientside-frontend: release non-result refs after completion
59391d1 
Tests such as TestNoSnapshotLeak were failing in client mode (e.g. using moby#522)
because we weren't releasing the intermediate refs.

Resolve this by refactoring the existing code which frees the intermediate refs
from `gatewayFrontend.Solve` into a method on `llbBridgeForwarder` and as well
as the original site also call from the solver when the top-level solve (in
clientside frontend mode) completes. The original call (which is via a defer)
could likely sensibly be moved either earlier or later if desired but leave it
here it is to minimise the scope of the change.

The previous code used the `retErr` named return but the code between that
point and the end of the function already ensured that `lbf.err` is the same as
`retErr`, thus the only change in the code which has moved is
`s/retErr/lbf.err/`. Note that the `res` named return was previously unused by
name.

Fixes moby#581.

Signed-off-by: Ian Campbell <ijc@docker.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tonistiigi @ijc