containerd integration: Make build work by thaJeztah · Pull Request #44079 · moby/moby

thaJeztah · 2022-09-01T12:39:44Z

Upstreaming

Make build work rumpl/moby#28 Make build and buildx work
Only use the image exporter in build if we don't use containerd rumpl/moby#36 Only use the image exporter in build if we don't use containerd
let buildx know we support containerd snapshotter rumpl/moby#50 let buildx know we support containerd snapshotter
Pass the current snapshotter to the buildkit worker rumpl/moby#52 Pass the current snapshotter to the buildkit worker
builder-next: enable more cache backends rumpl/moby#58 builder-next: enable more cache backends
builder-next: reenable runc executor rumpl/moby#57 builder-next: reenable runc executor
builder: Emit image id when using image exporter rumpl/moby#125 builder: Emit image id when using image exporter
builder: containerimage exporter with store option rumpl/moby#99
builder: Emit image id when using image exporter rumpl/moby#125

No conflicts, except for vendoring / vendor.sum

- A picture of a cute animal (not mandatory but encouraged)

cmd/dockerd/daemon.go

thaJeztah · 2022-09-01T16:03:55Z

I squashed a couple of commits as some of them were fix-ups in previous commits; kept the last two separate (but possibly the "runc" one should be squashed.

thaJeztah · 2022-09-06T15:28:56Z

@tonistiigi @rumpl PTAL (also at #44079 (comment))

rumpl

LGTM 👍

thaJeztah · 2022-09-08T15:16:14Z

Ah, dang; conflict in vendor.sum; I'll rebase

thaJeztah · 2022-09-08T15:20:20Z

done 👍

thaJeztah · 2022-09-08T17:30:50Z

Not sure I've seen this before;


[2022-09-08T15:21:21.530Z]  > [crun 1/2] RUN --mount=type=cache,sharing=locked,id=moby-crun-aptlib,target=/var/lib/apt     --mount=type=cache,sharing=locked,id=moby-crun-aptcache,target=/var/cache/apt         apt-get update && apt-get install -y --no-install-recommends             autoconf             automake             build-essential             libcap-dev             libprotobuf-c-dev             libseccomp-dev             libsystemd-dev             libtool             libudev-dev             libyajl-dev             python3             ;:
[2022-09-08T15:21:21.530Z] #0 1.373 runc run failed: unable to start container process: error during container init: error mounting "/var/lib/docker/buildkit/executor/resolv.conf" to rootfs at "/etc/resolv.conf": possibly malicious path detected -- refusing to operate on /var/lib/docker/buildkit/executor/qj8f5s2o4ep3euuoz99hyp7a0/rootfs/etc/resolv.conf (deleted)

thaJeztah · 2022-09-08T18:54:37Z

/cc @tonistiigi

thaJeztah · 2022-09-08T23:08:29Z

Alright; this time it passed without the weird error; looks like a race indeed?

@tonistiigi PTAL if this PR looks good to you 🙏

crazy-max · 2022-09-09T00:37:29Z

Alright; this time it passed without the weird error; looks like a race indeed?

@thaJeztah fyi BuildKit tests in this workflow https://github.com/moby/moby/blob/master/.github/workflows/buildkit.yml don't run with containerd snapshotter atm.

As discussed with @tonistiigi, this matrix should run both with dockerd worker and containerd integration.

Basically we should change the worker being tested to make it based on a new matrix entry and also update docker daemon config accordingly. I can do it on @rumpl fork so you can cherry pick here. LGTY?

cpuguy83

Code looks mostly good.
Also tested this out and it seems to work pretty well, even supports multi-platform output.

Logs are quite verbose after a build and looks like there's some transient error that occurs:

INFO[2022-11-01T21:53:03.707692329Z] [core] Subchannel Connectivity change to CONNECTING  module=grpc
INFO[2022-11-01T21:53:03.707771334Z] [core] Subchannel picks a new address "localhost" to connect  module=grpc
INFO[2022-11-01T21:53:03.707832438Z] [core] Channel Connectivity change to CONNECTING  module=grpc
WARN[2022-11-01T21:53:03.707856239Z] [core] grpc: addrConn.createTransport failed to connect to {localhost localhost <nil> <nil> 0 <nil>}. Err: connection error: desc = "transport: Error while dialing only one connection allowed"  module=grpc
INFO[2022-11-01T21:53:03.707904743Z] [core] Subchannel Connectivity change to TRANSIENT_FAILURE  module=grpc
INFO[2022-11-01T21:53:03.707934945Z] [core] Channel Connectivity change to TRANSIENT_FAILURE  module=grpc
ERRO[2022-11-01T21:53:03.708072154Z] healthcheck failed fatally                   
INFO[2022-11-01T21:53:03.708146258Z] [core] Channel Connectivity change to SHUTDOWN  module=grpc

Not sure if that's related to this change or just the containerd snapshotter use-case as a whole.

builder/builder-next/control/control.go

builder/builder-next/controller.go

builder/builder-next/worker/containerdworker.go

builder/builder-next/builder.go

thaJeztah · 2023-03-02T14:59:26Z

was to add a new worker feature (like FeatureCacheExportS3)

Yeah, for that specific case, thinking if (ideally) exporters should have a mechanism to "register" themselves, and some way to get that list. Similar to how (e.g.) graph drivers register themselves 🤔

vvoland · 2023-03-02T15:00:59Z

However, I think in the end we still will need to support the s3 cache backend anyway, so adding the extra compat checks to buildkit tests is a bit wasteful work.

We already have the aws-sdk vendored it, so hopefully for now we can just vendor the s3 bits until we have a way to push these out of moby (see Brian's suggestion: #44079 (comment))

vvoland · 2023-03-02T15:06:18Z

Opened a vendor PR: #45095
Also included the vendor here to re-run the tests.
Thanks, I didn't consider vendoring non-released version 😅

vvoland · 2023-03-03T15:53:06Z

Rebased and included the s3 cache support. Now all buildkit tests should pass.
Warning: it vendors 88k lines of code (s3 related code from aws-sdk-go-v2). Do we want to squash it, or should I open a separate PR for it?

thaJeztah · 2023-03-06T11:29:41Z

builder/builder-next/controller.go

 			"inline":   inlineremotecache.ResolveCacheExporterFunc(),
 			"local":    localremotecache.ResolveCacheExporterFunc(opt.SessionManager),
 			"gha":      gha.ResolveCacheExporterFunc(),
+			"s3":       s3remotecache.ResolveCacheExporterFunc(),


Comparing these two, I noticed that these are not "symmetrical";

❓ is it expected that the inline cache is included in Exporters, but not in Importers ?

(nit): perhaps we should sort both lists have the exporters/importers in the same order (we could just sort them alphabetically, then at least there's no bike-shedding about what order they should appear in).

Sorting would definitely make sense!

Regarding inline cache - according to the buildkit's docs it embeds the cache into the image and is expected to be imported with the registry cache: https://github.com/moby/buildkit#inline-push-image-and-cache-together

Yes, I was wondering if it was something like that 🤔 it was confusing me a bit though (we could even consider including a comment to describe that, if what you linked to is indeed the expected behaviour). /cc @crazy-max @jedevc (if you know)

Yeah this is expected - they are assymetrical. Inline cache works fundamentally differently than the other types of cache though they expose the same interface (but we can import them in the same way).

I would really prefer to keep s3/azure caching out of the main dockerd binary.

thaJeztah · 2023-03-06T11:35:31Z

Rebased and included the s3 cache support. Now all buildkit tests should pass.
Warning: it vendors 88k lines of code (s3 related code from aws-sdk-go-v2). Do we want to squash it, or should I open a separate PR for it?

I'm tempted to say; open as a separate PR, as there's other caching that we may want to(?) add, but IIUC, it was currently needed to make the BuildKit CI pass (as it runs some tests that expect s3 to be supported?)

For the caching, I was wondering how authentication is handled, and it looks like they use environment-variables that are supported by the SDKs. If we add support for those (guess this would be "automatically?"), at least we need to update the documentation for the daemon to mention the environment variables it may be using;

https://docs.docker.com/build/cache/backends/s3/#authentication

access_key_id, secret_access_key, and session_token, if left unspecified, are read from environment variables on the BuildKit server following the scheme for the AWS Go SDK. The environment variables are read from the server, not the Buildx client.
https://docs.docker.com/build/cache/backends/gha/#authentication

If the url or token parameters are left unspecified, the gha cache backend will fall back to using environment variables.
https://docs.docker.com/build/cache/backends/azblob/#authentication

The secret_access_key, if left unspecified, is read from environment variables on the BuildKit server following the scheme for the Azure Go SDK. The environment variables are read from the server, not the Buildx client.

That said, perhaps we need to look at those;

Do we want them to automatically look at environment variables? (such env-vars may be present for other purposes; perhaps it's ok to use them, but at least users should be aware that dockerd now may be using those (so implicitly gets access to their s3/azure/github account)
Do we want configuration options in daemon.json to allow users to specify (default) accounts for caching? (which may be separate from environment variables on the host)?

thaJeztah · 2023-03-06T20:32:58Z

As discussed in the call;

We removed the s3 caching from this PR (for follow-up discussion)
Temporarily removed the BuildKit CI changes, because those currently fail without the s3 caching (as they run some tests that expect it to be present)

BuildKit CI was green before those patches were removed (but they allowed us to verify that everything worked)

thaJeztah

LGTM (can't approve as this PR was originally opened by me)

@cpuguy83 @neersighted @rumpl PTAL if you're okay with the last iteration)

- Only use the image exporter in build if we don't use containerd Without this "docker build" fails with: Error response from daemon: exporter "image" could not be found - let buildx know we support containerd snapshotter - Pass the current snapshotter to the buildkit worker If buildkit uses a different snapshotter we can't list the images any more because we can't find the snapshot. builder/builder-next: make ContainerdWorker a minimal wrapper Note that this makes "Worker" a public field, so technically one could overwrite it. builder-next: reenable runc executor Currently, without special CNI config the builder would only create host network containers that is a security issue. Using runc directly instead of shim is faster as well as builder doesn’t need anything from shim. The overhead of setting up network sandbox is much slower of course. builder/builder-next: simplify options handling Trying to simplify the logic; - Use an early return if multiple outputs are provided - Only construct the list of tags if we're using an image (or moby) exporter - Combine some logic for snapshotter and non-snapshotter handling Create a constant for the moby exporter Pass a context when creating a router The context has a 10 seconds timeout which should be more than enough to get the answer from containerd. Signed-off-by: Djordje Lukic <djordje.lukic@docker.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Co-authored-by: Tonis Tiigi <tonistiigi@gmail.com> Co-authored-by: Nicolas De Loof <nicolas.deloof@gmail.com> Co-authored-by: Paweł Gronowski <pawel.gronowski@docker.com> Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah · 2023-03-07T10:22:36Z

For transparency; this PR does add gha as caching backend, which also implicitly adds support in the daemon for reading GHA authentication information from env vars (as mentioned above #44079 (comment))

https://docs.docker.com/build/cache/backends/gha/#authentication
If the url or token parameters are left unspecified, the gha cache backend will fall back to using environment variables.

I think it's okay to keep that change in, but I'll open a tracking ticket for further discussion on how we want to go forward with configuring these.

rumpl · 2023-03-07T10:34:22Z

Yay!

thaJeztah added area/builder Build status/2-code-review area/builder/buildkit Build containerd-integration Issues and PRs related to containerd integration labels Sep 1, 2022

thaJeztah added this to the v-next milestone Sep 1, 2022

thaJeztah commented Sep 1, 2022

View reviewed changes

cmd/dockerd/daemon.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the c8d_build branch 2 times, most recently from 04ce54d to ddc621e Compare September 1, 2022 16:03

thaJeztah marked this pull request as ready for review September 1, 2022 16:03

thaJeztah requested a review from tonistiigi as a code owner September 1, 2022 16:03

thaJeztah requested a review from rumpl September 6, 2022 12:46

This comment was marked as resolved.

Sign in to view

thaJeztah force-pushed the c8d_build branch from ddc621e to c8d9aa1 Compare September 7, 2022 19:59

rumpl approved these changes Sep 8, 2022

View reviewed changes

thaJeztah force-pushed the c8d_build branch from c8d9aa1 to c6b47c4 Compare September 8, 2022 15:20

thaJeztah mentioned this pull request Sep 8, 2022

"possibly malicious path detected" during build moby/buildkit#3096

Open

cpuguy83 reviewed Nov 1, 2022

View reviewed changes

builder/builder-next/control/control.go Outdated Show resolved Hide resolved

builder/builder-next/controller.go Outdated Show resolved Hide resolved

tiborvass reviewed Nov 13, 2022

View reviewed changes

builder/builder-next/controller.go Outdated Show resolved Hide resolved

tiborvass reviewed Nov 13, 2022

View reviewed changes

builder/builder-next/worker/containerdworker.go Outdated Show resolved Hide resolved

tiborvass reviewed Nov 13, 2022

View reviewed changes

builder/builder-next/builder.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the c8d_build branch 2 times, most recently from b4d8b10 to e0e2066 Compare November 14, 2022 19:12

vvoland force-pushed the c8d_build branch from 889b1fd to 1148433 Compare March 2, 2023 18:53

thaJeztah mentioned this pull request Mar 3, 2023

vendor: buildkit v0.11.4-0.20230228113103-218e934edfba #45095

Merged

vvoland force-pushed the c8d_build branch from 1148433 to 738e492 Compare March 3, 2023 15:34

thaJeztah commented Mar 6, 2023

View reviewed changes

vvoland force-pushed the c8d_build branch from 43344e0 to f42ac01 Compare March 6, 2023 19:53

thaJeztah commented Mar 6, 2023

View reviewed changes

cpuguy83 approved these changes Mar 6, 2023

View reviewed changes

neersighted requested review from corhere and rumpl March 6, 2023 21:27

tonistiigi mentioned this pull request Mar 6, 2023

[23.0 backport] builder-next: temporarily disable mergeop and diffop #45112

Merged

rumpl approved these changes Mar 7, 2023

View reviewed changes

vvoland force-pushed the c8d_build branch from f42ac01 to e5a34e4 Compare March 7, 2023 09:02

builder-next: enable more cache backends

f8b468f

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the c8d_build branch from e5a34e4 to f8b468f Compare March 7, 2023 09:33

thaJeztah added impact/changelog docs/revisit labels Mar 7, 2023

thaJeztah merged commit b3428bc into moby:master Mar 7, 2023

thaJeztah deleted the c8d_build branch March 7, 2023 10:26

vvoland mentioned this pull request Mar 7, 2023

c8d/builder: Enable s3 cache backend #45114

Closed

crazy-max mentioned this pull request Mar 13, 2023

integration: enforce features compat through env vars moby/buildkit#3711

Merged

crazy-max mentioned this pull request Sep 30, 2024

build history: orphan records when switching between graphdriver and containerd store #48562

Closed

Conversation

thaJeztah commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Sep 1, 2022

Uh oh!

thaJeztah commented Sep 6, 2022

Uh oh!

This comment was marked as resolved.

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 8, 2022

Uh oh!

thaJeztah commented Sep 8, 2022

Uh oh!

thaJeztah commented Sep 8, 2022

Uh oh!

thaJeztah commented Sep 8, 2022

Uh oh!

thaJeztah commented Sep 8, 2022

Uh oh!

crazy-max commented Sep 9, 2022

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Mar 2, 2023

Uh oh!

vvoland commented Mar 2, 2023

Uh oh!

vvoland commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland commented Mar 3, 2023

Uh oh!

thaJeztah Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

vvoland Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

jedevc Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 6, 2023

Uh oh!

thaJeztah commented Mar 6, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 7, 2023

Uh oh!

rumpl commented Mar 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

thaJeztah commented Sep 1, 2022 •

edited

Loading

vvoland commented Mar 2, 2023 •

edited

Loading

vvoland Mar 6, 2023 •

edited

Loading