vendor buildkit 6861f17f15364de0fe1fd1e6e8da07598a485123 by tiborvass · Pull Request #41601 · moby/moby

tiborvass · 2020-10-28T17:42:31Z

Signed-off-by: Tibor Vass [email protected]

thaJeztah · 2020-10-28T23:17:39Z

Failures look related;

2020-10-28T17:53:45.190Z] === Errors
[2020-10-28T17:53:45.190Z] builder/dockerfile/dispatchers_test.go:142:37: unknown field 'KeyValuePairOptional' in struct literal of type instructions.ArgCommand
[2020-10-28T17:53:45.190Z] builder/dockerfile/dispatchers_test.go:398:34: unknown field 'KeyValuePairOptional' in struct literal of type instructions.ArgCommand

thaJeztah · 2020-10-28T23:55:29Z

Created a list of diffs per dependency:

moby/buildkit@4d1f260...212a0b1
tonistiigi/fsutil@ae3a8d7...c3ed55f
opentracing/opentracing-go@1361b9c...v1.2.0
google/shlex@e7afc7f...6f45313
- this is downgrading to an older version: google/shlex@6f45313...e7afc7f
opentracing-contrib/go-stdlib@b1a47cf...v1.0.0
mitchellh/hashstructure@2bca23e...v1.0.0
containerd/containerd@v1.4.1...d4e7820
- raw diff (as it's different branches); https://github.com/containerd/containerd/compare/v1.4.1..d4e78200d6da62480c85bf6f26b7221ea938f396
- I think the relevant commits we need for buildkit are docker: split private token helper functions to reusable pkg containerd/containerd#4445; how realistic would it be to get those backported to the v1.4 branch?
containerd/go-runc@7016d3c...16b287b
https://github.com/containerd/stargz-snapshotter (new dependency; Apache2 License, so should be ok)

thaJeztah · 2020-10-28T23:55:56Z

this is downgrading to an older version; could you update the version in buildkit instead?

thaJeztah · 2020-10-28T23:58:44Z

I know containerd doesn't have a stable Go API (yet); still trying to see what chances we have to stay on release branches / released versions, so that we can have bug- and security fixes backported upstream, and to make our dependency graph more go modules friendly; I think the relevant commits we need for buildkit are containerd/containerd#4445; how realistic would it be to get that backported to the v1.4 branch?

tiborvass · 2020-10-29T01:03:02Z

Sorry this is not ready, I need to fix things still.

AkihiroSuda · 2020-10-29T12:47:48Z

https://github.com/containerd/stargz-snapshotter (new dependency; Apache2 License, so should be ok)

@ktock Could we reorganize the package structure not to require this dependency? (Not a blocker for merging this PR)

thaJeztah · 2020-10-29T15:27:12Z

@ktock Could we reorganize the package structure not to require this dependency? (Not a blocker for merging this PR)

👍 yes, if we don't need it in this code, that would be better of course!

thaJeztah · 2020-10-30T11:21:03Z

Some upstream buildit PRs were opened;

Delay decoding stargz config for avoiding extra dependency for config pkg buildkit#1762 was opened to get rid of the stargz-snapshotter dependency
vendor: github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 buildkit#1767 to update google/shlex in buildkit to the same version as is already used in this repo
Migrate github.com/docker/docker/builder/dockerignore to BuildKit repository buildkit#1766 to move the dockerignore package to buildkit; we should deprecate the package in this repository (remove the code, and replace with aliases with a Deprecated: comment, so that the buildkit is the canonical place for that

tiborvass · 2020-11-13T05:45:36Z

@thaJeztah @tonistiigi updated. I will open a separate PR for sync-ing vendoring hashes.

thaJeztah · 2020-11-13T07:01:57Z

Can you combine the "use buildkit dockerignore" with the vendor commit? (edit: so that it's clear these files moved?)
Also should add a "deprecated.go" that deprecated it, but aliases it to to the new location (to help the transition); there's quite some consumers of it; https://grep.app/search?current=2&q=github.com/docker/docker/builder/dockerignore

This is not pkg/ but fine

Does it make a difference? afaik the pkg/ prefix was a convention, but now discouraged and deprecated

thaJeztah · 2020-11-13T15:36:22Z

This failure looks related;

=== RUN   TestDockerSuite/TestBuildContChar
    --- FAIL: TestDockerSuite/TestBuildContChar (2.41s)
        docker_cli_build_test.go:5627: assertion failed: expression is false: strings.Contains(result.Combined(), "Step 2/2 : RUN echo hi \\\n")

I suspect my changes in moby/buildkit#1559 broke that test. Looks like it was added in eaf0b57 (#27873), but AFAICS that was testing a bare (non-escaped) \ at the end of the line; #27545

Signed-off-by: Tibor Vass <[email protected]>

This old test is failing after an edge-case change in dockerfile parsing considered a bugfix: moby/buildkit#1559 Instead of fixing the test, I suggest removing it as there are already tests for it in BuildKit. Signed-off-by: Tibor Vass <[email protected]>

tiborvass · 2020-11-14T06:32:38Z

@thaJeztah @tonistiigi it's updated and the s390x failure seems unrelated. --cache-from works for both inline cache local, inline-cache remote, and with remote cache exported with mode=max.

thaJeztah · 2020-11-14T07:09:17Z

Thanks!

…

On 14 Nov 2020, at 07:32, Tibor Vass ***@***.***> wrote: @thaJeztah @tonistiigi it's updated and the s390x failure seems unrelated. --cache-from works for both inline cache local, inline-cache remote, and with remote cache exported with mode=max. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

tonistiigi · 2020-11-16T06:55:23Z

Building buildkit:master with this PR:

root@81f0e16cc830:/tmp/buildkit# docker build .
[+] Building 7.0s (15/30)
 => [internal] load build definition from Dockerfile                                                                                                    0.0s
 => => transferring dockerfile: 13.03kB                                                                                                                 0.0s
 => [internal] load .dockerignore                                                                                                                       0.0s
 => => transferring context: 56B                                                                                                                        0.0s
 => resolve image config for docker.io/docker/dockerfile:1.1-experimental                                                                               1.9s
 => docker-image://docker.io/docker/dockerfile:1.1-experimental@sha256:de85b2f3a3e8a2f7fe48e8e84a65f6fdd5cd5183afa6412fff9caa6871649c44                 0.8s
 => => resolve docker.io/docker/dockerfile:1.1-experimental@sha256:de85b2f3a3e8a2f7fe48e8e84a65f6fdd5cd5183afa6412fff9caa6871649c44                     0.0s
 => => sha256:de85b2f3a3e8a2f7fe48e8e84a65f6fdd5cd5183afa6412fff9caa6871649c44 1.69kB / 1.69kB                                                          0.0s
 => => sha256:8c69d118cfcd040a222bea7f7d57c6156faa938cb61b47657cd65343babc3664 521B / 521B                                                              0.0s
 => => sha256:08fb2b2ca3d58e19d791e73dea16126df37608115532e748d63525688df457a5 897B / 897B                                                              0.0s
 => => sha256:61261561661960014533790f5d6c42f5b88362db7e005f35248c20f075900a61 8.88MB / 8.88MB                                                          0.5s
 => => extracting sha256:61261561661960014533790f5d6c42f5b88362db7e005f35248c20f075900a61                                                               0.2s
 => [internal] load metadata for docker.io/library/alpine:3.12                                                                                          1.3s
 => [internal] load metadata for docker.io/tonistiigi/xx:golang@sha256:6f7d999551dd471b58f70716754290495690efa8421e0a1fcf18eb11d0c0a537                 1.5s
 => [internal] load metadata for docker.io/library/golang:1.13-buster                                                                                   1.5s
 => CANCELED [internal] load build context                                                                                                              1.0s
 => => transferring context: 38.34MB                                                                                                                    1.0s
 => [alpine 1/1] FROM docker.io/library/alpine:3.12@sha256:c0e9560cda118f9ec63ddefb4a173a2b2a0347082d7dff7dc14272e7841a5b5a                             1.6s
 => [gobuild-minimal 1/3] FROM docker.io/library/golang:1.13-buster@sha256:66a3f6817c129f48c9cc9b96816a1c0b6dcb399393196e3581d9367b55ab29f2             1.6s
 => CANCELED FROM docker.io/tonistiigi/binfmt:buildkit                                                                                                  1.0s
 => => resolve docker.io/tonistiigi/binfmt:buildkit                                                                                                     1.6s
 => => sha256:c43f2553830c3fe4846f88e6ea796237c83ebda9198896312bb1154795965696 7.78MB / 7.78MB                                                          0.6s
 => => sha256:9846723457345f5bc7247fd9681b51868c5b635ea77036d282ab774d2fbc0893 2.63MB / 2.63MB                                                          0.5s
 => => sha256:9da471824dab89882c995081b262df183c0d4522d4cef3656c28e54883c04df1 2.00kB / 2.00kB                                                          0.0s
 => => sha256:eee96be38e5e33c36b4c70128eeb781adc144ea342dc0dabc61ea3234416ec41 739B / 739B                                                              0.0s
 => => sha256:ceae2fe9603c9985f8874fda667cbea184ce0837ba156ad53873668ad5c9e81e 6.06kB / 6.06kB                                                          0.0s
 => => extracting sha256:c43f2553830c3fe4846f88e6ea796237c83ebda9198896312bb1154795965696                                                               0.4s
 => [xgo 1/1] FROM docker.io/tonistiigi/xx:golang@sha256:6f7d999551dd471b58f70716754290495690efa8421e0a1fcf18eb11d0c0a537                               0.3s
 => => resolve docker.io/tonistiigi/xx:golang@sha256:6f7d999551dd471b58f70716754290495690efa8421e0a1fcf18eb11d0c0a537                                   0.0s
 => => sha256:6f7d999551dd471b58f70716754290495690efa8421e0a1fcf18eb11d0c0a537 3.29kB / 3.29kB                                                          0.0s
 => => sha256:3265075f008d7c003eea3e5c4da277cd5049c0aa31d39d1ec35272168f8bf9dd 523B / 523B                                                              0.0s
 => => sha256:ebffcbb79133382ea62829f690a0273304a3f4d83cd4876b58af645356512348 466B / 466B                                                              0.0s
 => => sha256:a104d17248520be6cb06ad88875c391c27c5e9e2a7f474d8ad6222d6bbe02445 616B / 616B                                                              0.1s
 => => extracting sha256:a104d17248520be6cb06ad88875c391c27c5e9e2a7f474d8ad6222d6bbe02445                                                               0.0s
 => ERROR [git 1/1] RUN apk add --no-cache git xz                                                                                                       0.7s
 => [gobuild-minimal 2/3] COPY --from=xgo / /                                                                                                           0.1s
 => ERROR [gobuild-minimal 3/3] RUN apt-get update && apt-get install --no-install-recommends -y libseccomp-dev file                                    0.5s
------
 > [git 1/1] RUN apk add --no-cache git xz:
#9 0.707 container_linux.go:370: starting container process caused: exec: "/bin/sh": stat /bin/sh: no such file or directory
------
------
 > [gobuild-minimal 3/3] RUN apt-get update && apt-get install --no-install-recommends -y libseccomp-dev file:
#16 0.437 container_linux.go:370: starting container process caused: exec: "/bin/sh": stat /bin/sh: no such file or directory
------
executor failed running [/bin/sh -c apt-get update && apt-get install --no-install-recommends -y libseccomp-dev file]: exit code: 1

From output you can see apt-get update runs without the base image being pulled.

tonistiigi · 2020-11-16T07:22:22Z

diff --git a/builder/builder-next/adapters/containerimage/pull.go b/builder/builder-next/adapters/containerimage/pull.go
index d0891cb598..62977675bd 100644
--- a/builder/builder-next/adapters/containerimage/pull.go
+++ b/builder/builder-next/adapters/containerimage/pull.go
@@ -93,7 +93,11 @@ func (is *Source) resolveRemote(ctx context.Context, ref string, platform *ocisp
 		dgst digest.Digest
 		dt   []byte
 	}
-	res, err := is.g.Do(ctx, ref, func(ctx context.Context) (interface{}, error) {
+	p := platforms.DefaultSpec()
+	if platform != nil {
+		p = *platform
+	}
+	res, err := is.g.Do(ctx, "getconfig::"+ref+"::"+platforms.Format(p), func(ctx context.Context) (interface{}, error) {
 		res := resolver.DefaultPool.GetResolver(is.RegistryHosts, ref, "pull", sm, g)
 		dgst, dt, err := imageutil.Config(ctx, ref, res, is.ContentStore, nil, platform)
 		if err != nil {
@@ -247,7 +251,7 @@ func (p *puller) resolveLocal() {
 }
 
 func (p *puller) resolve(ctx context.Context, g session.Group) error {
-	_, err := p.is.g.Do(ctx, "", func(ctx context.Context) (_ interface{}, err error) {
+	_, err := p.is.g.Do(ctx, "resolve::"+p.ref+"::"+platforms.Format(p.platform), func(ctx context.Context) (_ interface{}, err error) {
 		resolveProgressDone := oneOffProgress(ctx, "resolve "+p.src.Reference.String())
 		defer func() {
 			resolveProgressDone(err)

tiborvass · 2020-11-16T19:19:14Z

@tonistiigi updated

tiborvass · 2020-11-16T20:38:29Z

I don't see how TestInspect failure would be related to this PR. Probably a flakey test.

thaJeztah

LGTM

cpuguy83

Is there a summary of changes in this bump?

cpuguy83 · 2020-11-16T21:07:23Z


 }

-func (s *DockerSuite) TestBuildContChar(c *testing.T) {


I don't like that we are removing the test.
Even if it is tested in buildkit, this is more of an end-to-end test, and we make sure it doesn't break regardless of builder.

@cpuguy83 who writes a \ as the last character of a Dockerfile? I am of the opinion that it's such an edge case that I'm prioritizing removing legacy tests that are useless. But if you feel strongly about this I can fix the test.

cpuguy83

LGTM

Signed-off-by: Tibor Vass <[email protected]>

tiborvass requested a review from tonistiigi October 28, 2020 17:42

thaJeztah added area/builder Build status/2-code-review labels Oct 28, 2020

thaJeztah reviewed Oct 28, 2020

View reviewed changes

tiborvass marked this pull request as draft October 29, 2020 01:02

ktock mentioned this pull request Oct 29, 2020

Delay decoding stargz config for avoiding extra dependency for config pkg moby/buildkit#1762

Merged

thaJeztah mentioned this pull request Oct 30, 2020

vendor: github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 moby/buildkit#1767

Merged

thaJeztah mentioned this pull request Oct 30, 2020

Migrate github.com/docker/docker/builder/dockerignore to BuildKit repository moby/buildkit#1766

Merged

tiborvass force-pushed the bk_vendor branch 4 times, most recently from 1a08597 to cd50e5b Compare November 13, 2020 05:43

tiborvass changed the title ~~vendor buildkit 212a0b14394dd09e9ec5e112469c526a14a93b2a~~ vendor buildkit ce283f39b5651c84ec237eb16d509ffa64c515b9 Nov 13, 2020

tiborvass marked this pull request as ready for review November 13, 2020 05:43

tiborvass force-pushed the bk_vendor branch 2 times, most recently from 945d99b to b12df87 Compare November 13, 2020 06:37

thaJeztah reviewed Nov 13, 2020

View reviewed changes

tiborvass force-pushed the bk_vendor branch from b12df87 to 6be1653 Compare November 14, 2020 03:53

Tibor Vass added 3 commits November 14, 2020 03:57

vendor buildkit 6861f17

de56a90

Signed-off-by: Tibor Vass <[email protected]>

builder-next: Refactor using buildkit's resolver pool

3756668

Signed-off-by: Tibor Vass <[email protected]>

builder-next: simple refactor

1b1eb09

Signed-off-by: Tibor Vass <[email protected]>

Tibor Vass added 3 commits November 14, 2020 03:57

builder-next: refactor to use flighcontrol and remove resolveOnce

8f464ae

Signed-off-by: Tibor Vass <[email protected]>

builder-next: fix --cache-from panic

39f9c1b

Signed-off-by: Tibor Vass <[email protected]>

tiborvass force-pushed the bk_vendor branch from 6be1653 to beff0a5 Compare November 14, 2020 03:58

tiborvass changed the title ~~vendor buildkit ce283f39b5651c84ec237eb16d509ffa64c515b9~~ vendor buildkit 6861f17f15364de0fe1fd1e6e8da07598a485123 Nov 14, 2020

tonistiigi requested a review from AkihiroSuda November 16, 2020 03:41

tonistiigi approved these changes Nov 16, 2020

View reviewed changes

thaJeztah approved these changes Nov 16, 2020

View reviewed changes

cpuguy83 requested changes Nov 16, 2020

View reviewed changes

tiborvass force-pushed the bk_vendor branch from 0e22a26 to a3438c1 Compare November 17, 2020 01:27

cpuguy83 approved these changes Nov 17, 2020

View reviewed changes

tiborvass added status/4-merge and removed status/2-code-review labels Nov 17, 2020

builder-next: fix platform-specific behavior

dbffbe8

Signed-off-by: Tibor Vass <[email protected]>

tiborvass force-pushed the bk_vendor branch from a3438c1 to dbffbe8 Compare November 17, 2020 02:33

tiborvass merged commit cf0ce96 into moby:master Nov 17, 2020

thaJeztah added this to the 20.10.0 milestone Nov 19, 2020

This was referenced Feb 19, 2021

builder: fix pull synchronization regression #42035

Merged

[20.10 backport] builder: fix pull synchronization regression #42049

Merged

thaJeztah mentioned this pull request May 19, 2025

Replace platforms.Format with platforms.FormatAll in user-visible messages and logs. #49973

Merged

Conversation

tiborvass commented Oct 28, 2020

Uh oh!

thaJeztah commented Oct 28, 2020

Uh oh!

thaJeztah commented Oct 28, 2020

Uh oh!

thaJeztah Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 28, 2020

Uh oh!

tiborvass commented Oct 29, 2020

Uh oh!

AkihiroSuda commented Oct 29, 2020

Uh oh!

thaJeztah commented Oct 29, 2020

Uh oh!

thaJeztah commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiborvass commented Nov 13, 2020

Uh oh!

thaJeztah Nov 13, 2020 • 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!

tiborvass Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 13, 2020

Uh oh!

tiborvass commented Nov 14, 2020

Uh oh!

thaJeztah commented Nov 14, 2020 via email

Uh oh!

tonistiigi commented Nov 16, 2020

Uh oh!

tonistiigi commented Nov 16, 2020 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiborvass commented Nov 16, 2020

Uh oh!

tiborvass commented Nov 16, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

tiborvass Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thaJeztah commented Oct 30, 2020 •

edited

Loading

thaJeztah Nov 13, 2020 •

edited

Loading

tonistiigi commented Nov 16, 2020 •

edited by thaJeztah

Loading