Skip to content

containerd worker: use diff service instead of client-side implementation#2366

Closed
ktock wants to merge 1 commit into
moby:masterfrom
ktock:containerd-worker-differ
Closed

containerd worker: use diff service instead of client-side implementation#2366
ktock wants to merge 1 commit into
moby:masterfrom
ktock:containerd-worker-differ

Conversation

@ktock
Copy link
Copy Markdown
Collaborator

@ktock ktock commented Sep 16, 2021

Fixes: #2365

This commit fixes containerd worker to use containerd's diff service instead of client-side implementation to avoid the performance regression. Overlayfs differ will be enabled only for OCI worker.

As discussed in #2365, to allow containerd worker using overlayfs differ, we'll need to port the differ implementation to containerd diff serivce.

@ktock
Copy link
Copy Markdown
Collaborator Author

ktock commented Sep 16, 2021

Containerd's diff service doesn't support custom compresor (containerd/containerd#5735) so zstd support is limited to OCI worker.

https://github.com/moby/buildkit/pull/2366/checks?check_run_id=3623798830#step:7:1046

=== CONT  TestIntegration/TestBuildExportZstd/worker=containerd
    client_test.go:2203: 
        	Error Trace:	client_test.go:2203
        	            				run.go:176
        	Error:      	Received unexpected error:
        	            	unsupported diff media type: application/vnd.oci.image.layer.v1.tar+zstd: not implemented
        	            	github.com/moby/buildkit/util/stack.Enable
        	            		/src/util/stack/stack.go:77
        	            	github.com/moby/buildkit/util/grpcerrors.FromGRPC
        	            		/src/util/grpcerrors/grpcerrors.go:188
        	            	github.com/moby/buildkit/util/grpcerrors.UnaryClientInterceptor
        	            		/src/util/grpcerrors/intercept.go:41
        	            	google.golang.org/grpc.(*ClientConn).Invoke
        	            		/src/vendor/google.golang.org/grpc/call.go:35
        	            	github.com/moby/buildkit/api/services/control.(*controlClient).Solve
        	            		/src/api/services/control/control.pb.go:1320
        	            	github.com/moby/buildkit/client.(*Client).solve.func2
        	            		/src/client/solve.go:201
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:57
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1581
        	            	failed to solve
        	            	github.com/moby/buildkit/client.(*Client).solve.func2
        	            		/src/client/solve.go:214
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:57
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1581
        	Test:       	TestIntegration/TestBuildExportZstd/worker=containerd

@ktock
Copy link
Copy Markdown
Collaborator Author

ktock commented Sep 21, 2021

@tonistiigi WDYT about #2366 (comment) ?
Client-side differ is needed to support non-gzip compressions (zstd,esgz,etc) that aren't supported by containerd.
Should we limit the support of such compressions to the OCI worker, or have overlayfs-differ (10-20% slower than containerd differ as discussed in #2365) to enable them for containerd worker as well?

@ktock ktock force-pushed the containerd-worker-differ branch from bdc14ce to dfb0658 Compare September 27, 2021 12:46
@ktock ktock marked this pull request as ready for review September 27, 2021 13:15
Comment thread util/overlaydiffer/differ_linux.go Outdated
enableOverlay, fallback = true, true
switch d.sn.Name() {
case "overlayfs", "fuse-overlayfs", "stargz":
logWarnOnErr = true // snapshotter should support overlay diff. so print warn log on failure
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does “fuse-overlayfs” really support overlay diff?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for pointing this. No, it doesn't because fuse-overlayfs snapshotter doesn't provide overlayfs mounts. Disabled it for fuse-overlayfs snapshotter.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock force-pushed the containerd-worker-differ branch from dfb0658 to cbda4a2 Compare September 28, 2021 06:18
@tonistiigi
Copy link
Copy Markdown
Member

I'm not really sure if these hacks are worth it. If we can fix the buffering issue and performance compared to oci worker is in +10-20% range I'd be ok with just using the client-side implementation always. Some cleanup in this area might be good though.

Currently, we have exceptions for overlay-native, windows and zstd. All of them could be implemented in containerd as well if the performance parity is needed but I don't think it is very important to have it instantly.

In #buildkit while discussing mergeop with @sipsma we've come up with another exception where we want certain refs to store their changes list for upper layer directly in ref metadata. This is so that we can guarantee the diff remains persistent when it is copied to different snapshot chains. If that list exists for a ref it would be used as a source for the differ, so again something that containerd API itself does not support.

FYI @aaronlehmann

@ktock
Copy link
Copy Markdown
Collaborator Author

ktock commented Oct 1, 2021

Thank you for the comment. At least fixing the buffering issue seems to be needed instantly. I'll open a PR for the fix.

@ktock
Copy link
Copy Markdown
Collaborator Author

ktock commented Oct 4, 2021

Closing in favor of #2388 and #2390.

@ktock ktock closed this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[master] Large performance regression exporting layers with containerd worker

3 participants