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.

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 <[email protected]>

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 <[email protected]>

Introduced in containerd/containerd@dd3eedf Signed-off-by: Bjorn Neergaard <[email protected]>

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 <[email protected]> Co-authored-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Bjorn Neergaard <[email protected]>

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 <[email protected]>

SourcePolicy was accounted for in moby/buildkit@330cf7a TODO: replace applySourcePolicies with BuildKit's implementation, which is currently unexported. Co-authored-by: Tonis Tiigi <[email protected]> Signed-off-by: Bjorn Neergaard <[email protected]>

Introduced years ago in moby/buildkit@6644e1b Signed-off-by: Bjorn Neergaard <[email protected]>

Introduced in moby/buildkit@4fc2d7b Signed-off-by: Bjorn Neergaard <[email protected]>

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 <[email protected]>

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 <[email protected]> Signed-off-by: CrazyMax <[email protected]> Signed-off-by: Bjorn Neergaard <[email protected]>

thaJeztah

LGTM

Skipping digest-related tests is no longer necessary after containerd/containerd@4065831 Signed-off-by: CrazyMax <[email protected]> Signed-off-by: Bjorn Neergaard <[email protected]>

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

neersighted force-pushed the buildkit_0.12 branch 3 times, most recently from b1572f3 to 5e9a6ee Compare September 21, 2023 19:15

neersighted requested review from AkihiroSuda and corhere September 21, 2023 20:15

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 <[email protected]>

daemon/c8d: use new containerd LabelDistributionSource constant

8cd5f04

Introduced in containerd/containerd@dd3eedf Signed-off-by: Bjorn Neergaard <[email protected]>

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

a728bd0

Introduced years ago in moby/buildkit@6644e1b Signed-off-by: Bjorn Neergaard <[email protected]>

builder-next/wrapper: use new exptypes constants

c8773e1

Introduced in moby/buildkit@4fc2d7b Signed-off-by: Bjorn Neergaard <[email protected]>

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 <[email protected]>

neersighted force-pushed the buildkit_0.12 branch from 5e9a6ee to 57eac69 Compare September 21, 2023 20:19

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 <[email protected]> Signed-off-by: Bjorn Neergaard <[email protected]>

thaJeztah added status/4-merge and removed status/2-code-review labels Sep 21, 2023

neersighted force-pushed the buildkit_0.12 branch from 57eac69 to d5b067e Compare September 21, 2023 20:26

thaJeztah merged commit 3614749 into moby:master Sep 22, 2023

neersighted deleted the buildkit_0.12 branch September 22, 2023 01:42

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"

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

neersighted commented Jul 13, 2023 •

edited

Loading

crazy-max Jul 20, 2023 •

edited

Loading

neersighted Jul 20, 2023 •

edited

Loading