Update to BuildKit 0.12 by neersighted · Pull Request #45966 · moby/moby

neersighted · 2023-07-13T22:15:41Z

Supersedes vendor: github.com/containerd/containerd v1.7.0-rc.2 #44530
Supersedes [WIP] testing buildkit 0.12 fixes #46035

- What I did
Update containerd (library) to v1.7 and BuildKit to v0.12.

Make changes to the code to adjust to the new dependency versions, and fix some TODOs that were waiting on these updates. Leave some new TODOs as well, mostly related to the builder-next code and having to re-implement some things instead of re-using them from BuildKit.

- How I did it
With help from my IDE, and with assistance from others (@tonistiigi, @crazy-max, @rumpl) where I ran out of skill and/or time.

- How to verify it
CI ✅

- Description for the changelog

Update buildkit to v0.12.2.
Update containerd to v1.7.6.

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

thaJeztah · 2023-07-20T14:05:09Z

builder/builder-next/exporter/overrides/wrapper.go

+	exporterAttrs[string(exptypes.OptKeyName)] = strings.Join(reposAndTags, ",")
+	exporterAttrs[string(exptypes.OptKeyUnpack)] = "true"


For these I was wondering if it's possible to do the reverse (cast to ImageExporterOptKey) ? Haven't looked at this code locally though, just through GitHub's ui

The type in the protobuf is string; I don't know that we can control the codegen enough to "do the right thing" here, and this is the pattern BK uses. Still, it's really clunky, and if we can invert it I'm totally in favor.

Ah... gotcha.. didn't realise we're dealing with protobuf here. Was mostly hoping "strong(ish) types" to help discoverability.

Yeah, this is just a newtype to ease discoverability, but it doesn't seem to be totally ergonomic even on the BK side due to protobufs:
https://github.com/moby/buildkit/blob/f53c1752113e1f9c0fbcc1d5c8e18a9b13452afc/control/control.go#L334-L342

cc @crazy-max @jedevc if this is something we can change

exporter attributes is map<string, string> in the proto def: https://github.com/moby/buildkit/blob/f53c1752113e1f9c0fbcc1d5c8e18a9b13452afc/api/services/control/control.proto#L64

Maybe map<string, google.protobuf.Any> would have work but the contract is already there.

Right, my thought (and specific inexperience) is that maybe we can change the protobuf definition to use the newtype, and existing clients with the old generated code will continue to interoperate as it's just a type alias to string. That breaks the Go API a little; but in theory the protobuf is the stable component and the generated Go code can change (ala containerd v1.7).

It turns out this is not possible as we have multiple types for the keys (ExporterOptKey for all exporters, ImageExporterOptKey for the image exporter).

It looks like this is a wart we'll have to live with; in the future when/if we remove Gogo, the SDK will have to change a fair bit anyway; at that point defining some conversion helper (maybe with generics) ought to be better.

thaJeztah

changes "LGTM"

left some nits about imports and "requires" going in the wrong group
commit-order and squashing
some minor change that could be moved separate

cmd/dockerd/daemon.go

libcontainerd/remote/client.go

vendor.mod

daemon/runtime_unix_test.go

builder/builder-next/adapters/containerimage/pull.go

thaJeztah · 2023-09-20T18:31:03Z

builder/builder-next/adapters/containerimage/pull.go

-func (is *Source) resolveRemote(ctx context.Context, ref string, platform *ocispec.Platform, sm *session.Manager, g session.Group) (digest.Digest, []byte, error) {
+func (is *Source) resolveRemote(ctx context.Context, ref string, platform *ocispec.Platform, sm *session.Manager, g session.Group) (string, digest.Digest, []byte, error) {


Looks like this commit is needed for the BuildKit update (otherwise it doesn't compile)

I'd rather keep this separate, but it is possible to first stub this (like I did in an earlier version of this PR), and then apply Tonis's implementation on top. That's not my preference, however, as there will have to be non-buildable commits in this PR to keep things reviewable/discrete.

builder/builder-next/adapters/containerimage/pull.go

builder/builder-next/executor_linux.go

thaJeztah · 2023-09-20T18:45:42Z

daemon/config/builder.go

 type BuilderHistoryConfig struct {
-	MaxAge     int64 `json:",omitempty"`
-	MaxEntries int64 `json:",omitempty"`
+	MaxAge     bkconfig.Duration `json:",omitempty"`


This change does not affect how the config is interpreted? (i.e. existing config still means the same?) because I think this may be part of config options set through daemon.json 😅

Existing options should work fine; you can see in moby/buildkit@e06c962 that a custom deserializer is implemented to handle the existing int64 format, but also parse a Go time.Duration string.

Ah, yes looks good. I didn't look yet, and was curious if the * times.Second was handled; I've (ahum) seen cases where that part was overlooked, and now the duration going from "seconds" to "nanoseconds" 😂

thaJeztah · 2023-09-21T08:56:42Z

@neersighted I did a quick rebase of this one after #46522 was merged;

rebased
dropped the temporary commit / replace for hcsshim (no longer needed after vendor: github.com/Microsoft/hcsshim v0.11.1 #46522)
moved the daemon-config log-format type change to a separate PR; daemon: strongly type containerd log.OutputFormat #46525
fixed some of the minor nits w.r.t. vendor.mod formatting and imports in the wrong "group"

We should probably still squash some of the commits so that the intermediate commits can compile (we can keep some of the "improvements", such as functions that can now be used from buildkit and/or containerd separate if we want them to be more visible in history).

TBBle · 2023-09-21T14:08:41Z

builder/builder-next/executor_nolinux.go

If we get a builder-next executor for Windows, will this file need to be renamed again? What about executor_notimplemented.go, since we're not relying on the GOOS filename tag anymore, and then any future executors only need to update the build-tag to exclude their newly-implemented GOOS from the not-implemented list.

Or am I misunderstanding, and this can never be implemented on non-Linux, e.g., even if we have BuildKit working for Windows.

This could be implemented for Windows, based on @gabriel-samfira and your work, as I understand it. We would need to rename the file, but I like it like this for now as _nolinux is a BuildKit-ism that we are symmetric with (when you look at the built-in executor implementations).

When BuildKit changes, I expect we'll rename this/change the build tags to once again be symmetric.

The only consideration could be to name this _unsupported.go (done that in some other cases, to be slightly more generic, and sometimes preventing some renames).

Not a blocker for me though

.github/workflows/buildkit.yml

builder/builder-next/adapters/containerimage/pull.go

builder/builder-next/executor_nolinux.go

The current executor is only tested on Linux, so let's be honest about that. Stubbing this correctly helps avoid incorrectly trying to call into Linux-only code in e.g. libnetwork. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

The DeepEqual ignore required in the daemon tests is a bit ugly, but it works given the new protoc output. We also have to ignore lints related to schema1 deprecations; these do not apply as we must continue to support this schema version. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

Introduced in containerd/containerd@dd3eedf Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

The following changes were required: * integration/build: progressui's signature changed in moby/buildkit@6b8fbed * builder-next: flightcontrol.Group has become a generic type in moby/buildkit@8ffc03b * builder-next/executor: add github.com/moby/buildkit/executor/resources types, necessitated by moby/buildkit@6e87e4b * builder-next: stub util/network/Namespace.Sample(), necessitated by moby/buildkit@963f161 Co-authored-by: CrazyMax <crazy-max@users.noreply.github.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

With BuildKit 0.12, some existing types are now required to be wrapped by new types: * containerd's LeaseManager and ContentStore have to be a (namespace-aware) BuildKit type since moby/buildkit@f044e0a * BuildKit's solver.CacheManager is used instead of bboltstorage.CacheKeyStorage since moby/buildkit@2b30693 * The MaxAge config field is a bkconfig.Duration since moby/buildkit@e06c962 Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

SourcePolicy was accounted for in moby/buildkit@330cf7a TODO: replace applySourcePolicies with BuildKit's implementation, which is currently unexported. Co-authored-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

Introduced years ago in moby/buildkit@6644e1b Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

Introduced in moby/buildkit@4fc2d7b Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

Now that this is a generic, we can define a struct type at the package level, and remove the casting logic necessary when we had to use interface{}. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

This exposes `ACTIONS_RUNTIME_TOKEN` and `ACTIONS_CACHE_URL`, which are used to skip cache exporter tests, when combined with moby/buildkit@a8789cb Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com> Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

thaJeztah

LGTM

Skipping digest-related tests is no longer necessary after containerd/containerd@4065831 Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

tianon · 2023-09-22T22:34:03Z

vendor/github.com/moby/buildkit/solver/result/result.go

 )

-type Result[T any] struct {
+type Result[T comparable] struct {


# github.com/moby/buildkit/frontend/gateway/client vendor/github.com/moby/buildkit/frontend/gateway/client/client.go:18:29: Reference does not implement comparable note: module requires Go 1.20

If we want to keep this change, we should update vendor.mod to go 1.20, right?

Ah, good catch! I should have tested with an older Go compiler.

I guess that's #46069 👀

Oh, nope, that doesn't touch the go line 😅

Ah, yes, I decided not to force go1.21 in that PR (as most of the code definitely would still work on older versions); perhaps we still should? At least setting the go.mod to go1.20 could be reasonable I guess. (with go1.19 being EOL); at least for the master / v25.0-dev branch.

I was planning on a follow-up PR (as I have some plans for the go line after Go 1.21), but I think that adding a go 1.20 line to your PR makes some sense 👍

I'll make it a separate commit, as we don't need it for the 24.0 (and other branches)

I guess in that case we might as well make it a separate PR; want to open one?

neersighted added the area/builder/buildkit Build label Jul 13, 2023

neersighted added this to the 25.0.0 milestone Jul 13, 2023

neersighted requested a review from crazy-max July 13, 2023 22:15

neersighted force-pushed the buildkit_0.12 branch 4 times, most recently from acfc1d9 to e7f85db Compare July 13, 2023 23:03

neersighted mentioned this pull request Jul 13, 2023

25.0 Release Tracking #45926

Closed

26 tasks

neersighted force-pushed the buildkit_0.12 branch 7 times, most recently from a7ff5e1 to 7886e0f Compare July 18, 2023 18:27

thaJeztah mentioned this pull request Jul 19, 2023

vendor: update dependencies in preparation of BuildKit v0.12 and containerd 1.7 update #46027

Merged

neersighted force-pushed the buildkit_0.12 branch from 7886e0f to cca7322 Compare July 19, 2023 19:07

This comment was marked as outdated.

Sign in to view

thaJeztah mentioned this pull request Jul 20, 2023

vendor: update dependencies in preparation of BuildKit v0.12 and containerd 1.7 update (part 2) #46032

Merged

This comment was marked as outdated.

Sign in to view

This was referenced Jul 20, 2023

[WIP] testing buildkit 0.12 fixes #46035

Closed

executor/resource: stub out NewSysSampler on Windows moby/buildkit#4040

Merged

This comment was marked as outdated.

Sign in to view

neersighted force-pushed the buildkit_0.12 branch from cca7322 to 040037d Compare July 20, 2023 11:51

thaJeztah mentioned this pull request Jul 20, 2023

[0.12 backport] executor/resource: stub out NewSysSampler on Windows moby/buildkit#4042

Merged

neersighted force-pushed the buildkit_0.12 branch from 040037d to 7b9d489 Compare July 20, 2023 13:42

This comment was marked as outdated.

Sign in to view

thaJeztah reviewed Jul 20, 2023

View reviewed changes

neersighted force-pushed the buildkit_0.12 branch 2 times, most recently from 1c661c8 to 21288ba Compare July 20, 2023 14:29

thaJeztah reviewed Sep 20, 2023

View reviewed changes

This was referenced Sep 21, 2023

vendor: github.com/Microsoft/hcsshim v0.11.1 #46522

Merged

daemon: strongly type containerd log.OutputFormat #46525

Merged

thaJeztah mentioned this pull request Sep 21, 2023

builder-next: make stub executor generic #46526

Merged

TBBle reviewed Sep 21, 2023

View reviewed changes

AkihiroSuda reviewed Sep 21, 2023

View reviewed changes

.github/workflows/buildkit.yml Show resolved Hide resolved

AkihiroSuda approved these changes Sep 21, 2023

View reviewed changes

corhere approved these changes Sep 21, 2023

View reviewed changes

builder/builder-next/adapters/containerimage/pull.go Outdated Show resolved Hide resolved

builder/builder-next/adapters/containerimage/pull.go Outdated Show resolved Hide resolved

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

rumpl approved these changes Sep 21, 2023

View reviewed changes

crazy-max approved these changes Sep 21, 2023

View reviewed changes

neersighted and others added 9 commits September 21, 2023 14:18

builder-next: make stub executor generic

79a4cbb

The current executor is only tested on Linux, so let's be honest about that. Stubbing this correctly helps avoid incorrectly trying to call into Linux-only code in e.g. libnetwork. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

daemon/c8d: use new containerd LabelDistributionSource constant

8cd5f04

Introduced in containerd/containerd@dd3eedf Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

builder-next/pull: use ResolveMode.String() helper

a728bd0

Introduced years ago in moby/buildkit@6644e1b Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

builder-next/wrapper: use new exptypes constants

c8773e1

Introduced in moby/buildkit@4fc2d7b Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

builder-next/a/ci/pull: strongly type flightcontrol.Group

34e71b3

Now that this is a generic, we can define a struct type at the package level, and remove the casting logic necessary when we had to use interface{}. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

corhere approved these changes Sep 21, 2023

View reviewed changes

thaJeztah approved these changes Sep 21, 2023

View reviewed changes

ci(buildkit): remove regex skipping tests with digest inconsistency

d5b067e

Skipping digest-related tests is no longer necessary after containerd/containerd@4065831 Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>

tianon reviewed Sep 22, 2023

View reviewed changes

TBBle mentioned this pull request Sep 24, 2023

c8d: Just enough Windows support to run the test suite #46539

Merged

4 tasks

neersighted mentioned this pull request Sep 28, 2023

vendor.mod: bump go line to 1.20 #46562

Merged

tianon mentioned this pull request Sep 28, 2023

ssh: fix error on commandconn close, add ping and default timeout docker/cli#4226

Merged

NiklasBeierl mentioned this pull request Oct 3, 2023

Reproducable: bake creates un-prunable files in overlay2 docker/buildx#2061

Closed

3 tasks

thaJeztah mentioned this pull request Feb 3, 2025

Flaky test (on Windows): TestContainersAPICreateMountsCreate #49382

Open

		exporterAttrs[string(exptypes.OptKeyName)] = strings.Join(reposAndTags, ",")
		exporterAttrs[string(exptypes.OptKeyUnpack)] = "true"

		func (is Source) resolveRemote(ctx context.Context, ref string, platform ocispec.Platform, sm *session.Manager, g session.Group) (digest.Digest, []byte, error) {
		func (is Source) resolveRemote(ctx context.Context, ref string, platform ocispec.Platform, sm *session.Manager, g session.Group) (string, digest.Digest, []byte, error) {

Conversation

neersighted commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazy-max Jul 20, 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!

Choose a reason for hiding this comment

Uh oh!

neersighted Jul 20, 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!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neersighted Sep 21, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neersighted commented Jul 13, 2023 •

edited

Loading

crazy-max Jul 20, 2023 •

edited

Loading

neersighted Jul 20, 2023 •

edited

Loading

neersighted Sep 21, 2023 •

edited

Loading