From a84c2b06fa38378253e32e88e43129f0a918553a Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 4 May 2022 20:58:19 -0700 Subject: [PATCH 1/3] frontend: make sure BuildOpts is cached Dockerfile frontend makes many calls to BuildOpts() function after named build context support was added. The assumption is that BuildOpts() is fast and does not need to be cached on the caller side. In current implementation BuildOpts() listed the workers in a way that triggered the platform emulation check to be triggered to with could take 50-100ms to complete what adds up if there are many Dockerfile stages. This commit adds caching to BuildOpts. If frontend was loaded through external image then it was already cached and this issue did not appear. Signed-off-by: Tonis Tiigi (cherry picked from commit a7c248be12997b33e85dda074161d87c17162135) --- frontend/gateway/forwarder/forward.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/frontend/gateway/forwarder/forward.go b/frontend/gateway/forwarder/forward.go index 2964ad02bd06..b866de818c93 100644 --- a/frontend/gateway/forwarder/forward.go +++ b/frontend/gateway/forwarder/forward.go @@ -26,7 +26,7 @@ import ( ) func llbBridgeToGatewayClient(ctx context.Context, llbBridge frontend.FrontendLLBBridge, opts map[string]string, inputs map[string]*opspb.Definition, w worker.Infos, sid string, sm *session.Manager) (*bridgeClient, error) { - return &bridgeClient{ + bc := &bridgeClient{ opts: opts, inputs: inputs, FrontendLLBBridge: llbBridge, @@ -35,7 +35,9 @@ func llbBridgeToGatewayClient(ctx context.Context, llbBridge frontend.FrontendLL workers: w, final: map[*ref]struct{}{}, workerRefByID: make(map[string]*worker.WorkerRef), - }, nil + } + bc.buildOpts = bc.loadBuildOpts() + return bc, nil } type bridgeClient struct { @@ -49,6 +51,7 @@ type bridgeClient struct { refs []*ref workers worker.Infos workerRefByID map[string]*worker.WorkerRef + buildOpts client.BuildOpts } func (c *bridgeClient) Solve(ctx context.Context, req client.SolveRequest) (*client.Result, error) { @@ -87,14 +90,15 @@ func (c *bridgeClient) Solve(ctx context.Context, req client.SolveRequest) (*cli return cRes, nil } -func (c *bridgeClient) BuildOpts() client.BuildOpts { - workers := make([]client.WorkerInfo, 0, len(c.workers.WorkerInfos())) - for _, w := range c.workers.WorkerInfos() { - workers = append(workers, client.WorkerInfo{ +func (c *bridgeClient) loadBuildOpts() client.BuildOpts { + wis := c.workers.WorkerInfos() + workers := make([]client.WorkerInfo, len(wis)) + for i, w := range wis { + workers[i] = client.WorkerInfo{ ID: w.ID, Labels: w.Labels, Platforms: w.Platforms, - }) + } } return client.BuildOpts{ @@ -107,6 +111,10 @@ func (c *bridgeClient) BuildOpts() client.BuildOpts { } } +func (c *bridgeClient) BuildOpts() client.BuildOpts { + return c.buildOpts +} + func (c *bridgeClient) Inputs(ctx context.Context) (map[string]llb.State, error) { inputs := make(map[string]llb.State) for key, def := range c.inputs { From 128245edb98d8aea90c6d57039b89dcd0617b7c8 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 4 May 2022 21:02:53 -0700 Subject: [PATCH 2/3] =?UTF-8?q?worker:=20don=E2=80=99t=20recheck=20emulato?= =?UTF-8?q?rs=20support=20for=20WorkerInfos?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To reload emulators after they have been changed in the kernel `ListWorkers` should be called. Signed-off-by: Tonis Tiigi (cherry picked from commit d6978f1cf2bd6c66ef942d3971f56b6d1d0f8dd1) --- worker/workercontroller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worker/workercontroller.go b/worker/workercontroller.go index 3899d5999cb6..26ca9459231c 100644 --- a/worker/workercontroller.go +++ b/worker/workercontroller.go @@ -64,7 +64,7 @@ func (c *Controller) WorkerInfos() []client.WorkerInfo { out = append(out, client.WorkerInfo{ ID: w.ID(), Labels: w.Labels(), - Platforms: w.Platforms(true), + Platforms: w.Platforms(false), }) } return out From 4d4d9cdf864d4aed2c7fb5d50004674e2b58f965 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 4 May 2022 00:59:36 +0000 Subject: [PATCH 3/3] Resolve docker-image named context config AOT This fixes an issue where `$PATH` was being overwritten while converting to LLB since we never actually passed it an image object so when it checks to see if the `PATH` env var is set PATH is indeed not set since we wind up with an empty `Image`. Signed-off-by: Brian Goff (cherry picked from commit 74eec1ee61c3cc59b730dc065d5822f03c8e44f6) Signed-off-by: Tonis Tiigi --- frontend/dockerfile/builder/build.go | 39 ++++++-- frontend/dockerfile/dockerfile2llb/convert.go | 10 +-- frontend/dockerfile/dockerfile_test.go | 90 +++++++++++++++++++ 3 files changed, 127 insertions(+), 12 deletions(-) diff --git a/frontend/dockerfile/builder/build.go b/frontend/dockerfile/builder/build.go index e0a1806901bc..f4b4a04ace82 100644 --- a/frontend/dockerfile/builder/build.go +++ b/frontend/dockerfile/builder/build.go @@ -812,8 +812,8 @@ func warnOpts(sm *llb.SourceMap, r *parser.Range, detail [][]byte, url string) c return opts } -func contextByNameFunc(c client.Client, p *ocispecs.Platform) func(context.Context, string) (*llb.State, *dockerfile2llb.Image, *binfotypes.BuildInfo, error) { - return func(ctx context.Context, name string) (*llb.State, *dockerfile2llb.Image, *binfotypes.BuildInfo, error) { +func contextByNameFunc(c client.Client, p *ocispecs.Platform) func(context.Context, string, string) (*llb.State, *dockerfile2llb.Image, *binfotypes.BuildInfo, error) { + return func(ctx context.Context, name, resolveMode string) (*llb.State, *dockerfile2llb.Image, *binfotypes.BuildInfo, error) { named, err := reference.ParseNormalizedNamed(name) if err != nil { return nil, nil, nil, errors.Wrapf(err, "invalid context name %s", name) @@ -826,7 +826,7 @@ func contextByNameFunc(c client.Client, p *ocispecs.Platform) func(context.Conte } if p != nil { name := name + "::" + platforms.Format(platforms.Normalize(*p)) - st, img, bi, err := contextByName(ctx, c, name, p) + st, img, bi, err := contextByName(ctx, c, name, p, resolveMode) if err != nil { return nil, nil, nil, err } @@ -834,11 +834,11 @@ func contextByNameFunc(c client.Client, p *ocispecs.Platform) func(context.Conte return st, img, bi, nil } } - return contextByName(ctx, c, name, p) + return contextByName(ctx, c, name, p, resolveMode) } } -func contextByName(ctx context.Context, c client.Client, name string, platform *ocispecs.Platform) (*llb.State, *dockerfile2llb.Image, *binfotypes.BuildInfo, error) { +func contextByName(ctx context.Context, c client.Client, name string, platform *ocispecs.Platform, resolveMode string) (*llb.State, *dockerfile2llb.Image, *binfotypes.BuildInfo, error) { opts := c.BuildOpts().Opts v, ok := opts["context:"+name] if !ok { @@ -854,13 +854,38 @@ func contextByName(ctx context.Context, c client.Client, name string, platform * ref := strings.TrimPrefix(vv[1], "//") imgOpt := []llb.ImageOption{ llb.WithCustomName("[context " + name + "] " + ref), - llb.WithMetaResolver(c), } if platform != nil { imgOpt = append(imgOpt, llb.Platform(*platform)) } + + named, err := reference.ParseNormalizedNamed(ref) + if err != nil { + return nil, nil, nil, err + } + + named = reference.TagNameOnly(named) + + _, data, err := c.ResolveImageConfig(ctx, named.String(), llb.ResolveImageConfigOpt{ + Platform: platform, + ResolveMode: resolveMode, + LogName: fmt.Sprintf("[context %s] load metadata for %s", name, ref), + }) + if err != nil { + return nil, nil, nil, err + } + + var img dockerfile2llb.Image + if err := json.Unmarshal(data, &img); err != nil { + return nil, nil, nil, err + } + st := llb.Image(ref, imgOpt...) - return &st, nil, nil, nil + st, err = st.WithImageConfig(data) + if err != nil { + return nil, nil, nil, err + } + return &st, &img, nil, nil case "git": st, ok := detectGitContext(v, "1") if !ok { diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index b9bff25ea744..a18e36ea5a21 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -70,16 +70,16 @@ type ConvertOpt struct { SourceMap *llb.SourceMap Hostname string Warn func(short, url string, detail [][]byte, location *parser.Range) - ContextByName func(context.Context, string) (*llb.State, *Image, *binfotypes.BuildInfo, error) + ContextByName func(ctx context.Context, name, resolveMode string) (*llb.State, *Image, *binfotypes.BuildInfo, error) } func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State, *Image, *binfotypes.BuildInfo, error) { buildInfo := &binfotypes.BuildInfo{} contextByName := opt.ContextByName - opt.ContextByName = func(ctx context.Context, name string) (*llb.State, *Image, *binfotypes.BuildInfo, error) { + opt.ContextByName = func(ctx context.Context, name, resolveMode string) (*llb.State, *Image, *binfotypes.BuildInfo, error) { if !strings.EqualFold(name, "scratch") && !strings.EqualFold(name, "context") { if contextByName != nil { - st, img, bi, err := contextByName(ctx, name) + st, img, bi, err := contextByName(ctx, name, resolveMode) if err != nil { return nil, nil, nil, err } @@ -166,7 +166,7 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State, } if st.Name != "" { - s, img, bi, err := opt.ContextByName(ctx, st.Name) + s, img, bi, err := opt.ContextByName(ctx, st.Name, opt.ImageResolveMode.String()) if err != nil { return nil, nil, nil, err } @@ -313,7 +313,7 @@ func Dockerfile2LLB(ctx context.Context, dt []byte, opt ConvertOpt) (*llb.State, d.stage.BaseName = reference.TagNameOnly(ref).String() var isScratch bool - st, img, bi, err := opt.ContextByName(ctx, d.stage.BaseName) + st, img, bi, err := opt.ContextByName(ctx, d.stage.BaseName, opt.ImageResolveMode.String()) if err != nil { return err } diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 2e5a16012a96..a1c9f548a786 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -5322,6 +5322,7 @@ COPY --from=base /out / _, err = f.Solve(sb.Context(), c, client.SolveOpt{ FrontendAttrs: map[string]string{ + // Make sure image resolution works as expected, do not add a tag or locator. "context:busybox": "docker-image://alpine", }, LocalDirs: map[string]string{ @@ -5340,6 +5341,95 @@ COPY --from=base /out / dt, err := ioutil.ReadFile(filepath.Join(destDir, "out")) require.NoError(t, err) require.True(t, len(dt) > 0) + + // Now test with an image with custom envs + dockerfile = []byte(` +FROM alpine:latest +ENV PATH=/foobar:$PATH +ENV FOOBAR=foobar +`) + + dir, err = tmpdir( + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + require.NoError(t, err) + defer os.RemoveAll(dir) + + registry, err := sb.NewRegistry() + if errors.Is(err, integration.ErrRequirements) { + t.Skip(err.Error()) + } + require.NoError(t, err) + target := registry + "/buildkit/testnamedimagecontext:latest" + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + LocalDirs: map[string]string{ + builder.DefaultLocalNameDockerfile: dir, + builder.DefaultLocalNameContext: dir, + }, + Exports: []client.ExportEntry{ + { + Type: client.ExporterImage, + Attrs: map[string]string{ + "name": target, + "push": "true", + }, + }, + }, + }, nil) + require.NoError(t, err) + + dockerfile = []byte(` +FROM busybox AS base +RUN cat /etc/alpine-release > /out +RUN env | grep PATH > /env_path +RUN env | grep FOOBAR > /env_foobar +FROM scratch +COPY --from=base /out / +COPY --from=base /env_path / +COPY --from=base /env_foobar / + `) + + dir, err = tmpdir( + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + require.NoError(t, err) + defer os.RemoveAll(dir) + + f = getFrontend(t, sb) + + destDir, err = os.MkdirTemp("", "buildkit") + require.NoError(t, err) + defer os.RemoveAll(destDir) + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + FrontendAttrs: map[string]string{ + "context:busybox": "docker-image://" + target, + }, + LocalDirs: map[string]string{ + builder.DefaultLocalNameDockerfile: dir, + builder.DefaultLocalNameContext: dir, + }, + Exports: []client.ExportEntry{ + { + Type: client.ExporterLocal, + OutputDir: destDir, + }, + }, + }, nil) + require.NoError(t, err) + + dt, err = os.ReadFile(filepath.Join(destDir, "out")) + require.NoError(t, err) + require.True(t, len(dt) > 0) + + dt, err = os.ReadFile(filepath.Join(destDir, "env_foobar")) + require.NoError(t, err) + require.Equal(t, "FOOBAR=foobar", strings.TrimSpace(string(dt))) + + dt, err = os.ReadFile(filepath.Join(destDir, "env_path")) + require.NoError(t, err) + require.Contains(t, string(dt), "/foobar:") } func testNamedLocalContext(t *testing.T, sb integration.Sandbox) {