vendor buildkit v0.10.0 by crazy-max · Pull Request #43239 · moby/moby

crazy-max · 2022-02-15T13:22:38Z

closes #42968
closes #42983
closes #40023
fixes #42261

moby/buildkit@9f254e1...8d45bd6

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

thaJeztah

some quick comments (my browser doesn't like reviewing the diff, so let me post some of them already; will have a closer look later)

builder/builder-next/exporter/writer.go

builder/builder-next/worker/worker.go

Dockerfile.windows

builder/builder-next/controller.go

thaJeztah · 2022-02-15T14:27:02Z

builder/builder-next/exporter/export.go


 const (
-	keyImageName = "name"
+	keyImageName        = "name"


Are these options unique to the integration in moby, or would conversion / definition of these be something we should move to buildkit at some point?

Yeah while working on this PR I thought about moving the moby exporter to https://github.com/moby/buildkit/tree/master/exporter. Maybe we could expose some bits on BuildKit in the meantime.

crazy-max · 2022-02-15T15:46:31Z

Ok we have the same issues with swarm tests: https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1332 like #42983 (comment)

[](https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1333)[](https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1334)[](https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1335)[](https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1336)[](https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1337)=== Failed

[2022-02-15T15:36:22.526Z] === FAIL: arm64.integration.service TestCreateServiceSecretFileMode (12.06s)

[2022-02-15T15:36:22.526Z]     create_test.go:294: assertion failed: string "\x03\x00\x00\x00\x00\x00\x00\xc2Error grabbing logs: rpc error: code = Unknown desc = warning: incomplete log stream. some logs could not be retrieved for the following reasons: node fe09fk6y4w8myh53ocb2viaoj is not available\n" does not contain "-rwxrwxrwx"

[2022-02-15T15:36:22.526Z] 

[2022-02-15T15:36:22.526Z] === FAIL: arm64.integration.service TestCreateServiceConfigFileMode (12.07s)

[2022-02-15T15:36:22.526Z]     create_test.go:351: assertion failed: string "\x03\x00\x00\x00\x00\x00\x00\xc2Error grabbing logs: rpc error: code = Unknown desc = warning: incomplete log stream. some logs could not be retrieved for the following reasons: node qrbt4wmhhxymcpydrnjg7aubv is not available\n" does not contain "-rwxrwxrwx"

Will add a commit using my fork of swarmkit (even with failed raft test) to see how it goes.

tonistiigi · 2022-02-15T17:21:14Z

This needs the additional patches for MergeOp/DiffOp from @sipsma . If we want to test sooner and reduce patch size we can first vendor the commit before MergeOp was merged.

builder/builder-next/exporter/export.go

sipsma · 2022-02-15T17:28:26Z

Commits needed to support changes made during mergeop:

Same but for DiffOp:

sipsma@3a5a263

The changes needed to get the hardlink optimization working are here but are rough, need some cleanup:

sipsma@e1dacf0

Let me know if I can help out cleaning the hardlink optimization commit up.

crazy-max · 2022-02-15T17:29:38Z

Thanks @sipsma!

crazy-max · 2022-03-22T15:10:12Z

Seems like a flaky test on win-2022-c8d: https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/45/pipeline#step-271-log-1702

Are you aware of this one @thaJeztah?

[2022-03-22T14:41:24.835Z] === Failed
[2022-03-22T14:41:24.835Z] === FAIL: github.com/docker/docker/integration-cli TestDockerSuite/TestRunContainerWithRmFlagCannotStartContainer (2.65s)
[2022-03-22T14:41:24.835Z]     docker_cli_run_test.go:2770: Expected not to have containers 918947e84a9f
[2022-03-22T14:41:24.835Z]         
[2022-03-22T14:41:24.835Z]     --- FAIL: TestDockerSuite/TestRunContainerWithRmFlagCannotStartContainer (2.65s)
[2022-03-22T14:41:24.835Z] 
[2022-03-22T14:41:24.835Z] === FAIL: github.com/docker/docker/integration-cli TestDockerSuite (3009.20s)

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

tonistiigi

Make sure the leftover mergeop/hardlink issue is tracked in moby with 22.x milestone so we can get this in for early testing.

builder/builder-next/adapters/snapshot/snapshot.go

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

thaJeztah

LGTM - left some notes about things we should look into in follow-ups

thaJeztah · 2022-03-24T16:04:15Z

builder/builder-next/exporter/writer.go

+		if description := layer.GetDescription(); description != "" {
+			meta.description = description
+		} else {
+			meta.description = "created by buildkit" // shouldn't be shown but don't fail build


Was wondering if we use this information anywhere (and if so, if this at some point should include the buildkit version)

thaJeztah · 2022-03-24T16:13:18Z

builder/builder-next/builder.go

+
+	ulimits, err := toBuildkitUlimits(opt.Options.Ulimits)
+	if err != nil {
+		return nil, err


These are also the ones we need to start looking at to return a correct error-type (InvalidParam()); per my other comment; there's a lot of those already, so let's make an effort to clean that up in some sweeps.

thaJeztah · 2022-03-24T16:14:18Z

builder/builder-next/builder.go

 }

+// toBuildkitUlimits converts ulimits from docker type=soft:hard format to buildkit's csv format
+func toBuildkitUlimits(inp []*units.Ulimit) (string, error) {


oh... hmm... so actually looks like this never return an error? ok to fix later (somewhat surprised there's no longer complaining)

thaJeztah · 2022-03-24T16:16:07Z

builder/builder-next/exporter/writer.go

-		dt, err = json.Marshal(cache)
+		dt, err := json.Marshal(cache)
 		if err != nil {
-			return nil, errors.Wrap(err, "failed to marshal cache")


Somewhat curious why you removed the wrapping here (seems like the additional context could be useful); not a blocker, but add it to the list of things to look at

thaJeztah · 2022-03-24T16:24:45Z

Make sure the leftover mergeop/hardlink issue is tracked in moby with 22.x milestone so we can get this in for early testing.

tracked in #43415

thaJeztah · 2022-03-24T16:25:33Z

Let's get this one in!

crazy-max force-pushed the buildkit-0.10 branch 6 times, most recently from d1e159c to 013bbff Compare February 15, 2022 14:21

thaJeztah reviewed Feb 15, 2022

View reviewed changes

crazy-max force-pushed the buildkit-0.10 branch 4 times, most recently from 449f779 to 25bb0ff Compare February 15, 2022 15:17

This comment was marked as outdated.

Sign in to view

tonistiigi reviewed Feb 15, 2022

View reviewed changes

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

This was referenced Feb 23, 2022

vendor: update various dependencies in preparation of BuildKit update #43287

Merged

vendor: github.com/aws/aws-sdk-go v1.31.6 #43288

Merged

vendor: github.com/Microsoft/hcsshim v0.9.2 #43289

Merged

crazy-max changed the title ~~vendor buildkit v0.10.0-rc1~~ vendor buildkit v0.10.0 Feb 25, 2022

crazy-max force-pushed the buildkit-0.10 branch 3 times, most recently from 1739240 to d408439 Compare March 3, 2022 18:49

thaJeztah mentioned this pull request Mar 4, 2022

remove unneeded "digest" alias for "go-digest" #43323

Merged

crazy-max force-pushed the buildkit-0.10 branch 4 times, most recently from 86baed0 to 7eb4ef6 Compare March 13, 2022 15:50

crazy-max mentioned this pull request Mar 14, 2022

ci: simple gha workflow to build docker daemon #43375

Merged

crazy-max force-pushed the buildkit-0.10 branch 2 times, most recently from d6ed26e to a3a3b75 Compare March 22, 2022 13:15

crazy-max marked this pull request as ready for review March 22, 2022 15:10

crazy-max requested a review from tianon as a code owner March 22, 2022 15:10

crazy-max requested review from thaJeztah and tonistiigi March 22, 2022 15:10

vendor buildkit v0.10.0

a2aaf4c

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max force-pushed the buildkit-0.10 branch from a3a3b75 to e4193ba Compare March 22, 2022 17:54

tonistiigi reviewed Mar 23, 2022

View reviewed changes

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

crazy-max mentioned this pull request Mar 23, 2022

[22.06] Hardlink optimization support for BuildKit #43415

Open

crazy-max added 5 commits March 23, 2022 16:07

builder: changes needed since buildkit 0.10.0

aadb3bf

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

builder: enable shm-size and ulimit for buildkit

b899db6

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

builder: add buildinfo for buildkit

fda0226

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

ci: buildkit integration test

32f0561

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

vendor buildkit 8d45bd6 that fixes dockerd worker integration tests

ff35785

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max force-pushed the buildkit-0.10 branch from e4193ba to ff35785 Compare March 23, 2022 15:07

tonistiigi approved these changes Mar 23, 2022

View reviewed changes

thaJeztah approved these changes Mar 24, 2022

View reviewed changes

thaJeztah merged commit 56ea588 into moby:master Mar 24, 2022

crazy-max deleted the buildkit-0.10 branch March 26, 2022 12:01

bobbypage mentioned this pull request Aug 2, 2022

vendor: Bump cAdvisor to v0.45.0 kubernetes/kubernetes#111647

Merged

This was referenced Sep 6, 2022

(WSL2) docker build "output clipped, log limit 1MiB reached" #42261

Closed

Update uses of Image platform fields in OCI image-spec #44125

Merged

petrzjunior mentioned this pull request Jan 3, 2023

Wrong dockerfile used when building two images in the same context folder moby/buildkit#1368

Closed

vvoland mentioned this pull request Mar 2, 2023

builder: Remove buildkit's build information #45097

Merged

thaJeztah mentioned this pull request Apr 14, 2023

docs: update deprecation status of BuildKit "buildInfo" for v24.0.0 docker/cli#4206

Merged

cahillsf mentioned this pull request Jan 7, 2024

Investigate how we ended up with a test extension binary in the CAPD image kubernetes-sigs/cluster-api#9752

Closed

4 tasks

Conversation

crazy-max commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

crazy-max Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

crazy-max commented Feb 15, 2022

Uh oh!

This comment was marked as outdated.

tonistiigi commented Feb 15, 2022

Uh oh!

Uh oh!

sipsma commented Feb 15, 2022

Uh oh!

crazy-max commented Feb 15, 2022

Uh oh!

crazy-max commented Mar 22, 2022

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 24, 2022

Uh oh!

thaJeztah commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crazy-max commented Feb 15, 2022 •

edited

Loading