8000 builder: implement PullParent option with buildkit by tiborvass · Pull Request #37613 · moby/moby · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

builder: implement PullParent option with buildkit #37613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 17, 2018
Merged

builder: implement PullParent option with buildkit #37613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 55 additions & 15 deletions builder/builder-next/adapters/containerimage/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ import (
"golang.org/x/time/rate"
)

const preferLocal = true // FIXME: make this optional from the op

// SourceOpt is options for creating the image source
type SourceOpt struct {
SessionManager *session.Manager
Expand Down Expand Up @@ -114,32 +112,61 @@ func (is *imageSource) resolveLocal(refStr string) ([]byte, error) {
return img.RawJSON(), nil
}

func (is *imageSource) ResolveImageConfig(ctx context.Context, ref string, opt gw.ResolveImageConfigOpt) (digest.Digest, []byte, error) {
if preferLocal {
dt, err := is.resolveLocal(ref)
if err == nil {
return "", dt, nil
}
}

func (is *imageSource) resolveRemote(ctx context.Context, ref string, platform *ocispec.Platform) (digest.Digest, []byte, error) {
type t struct {
dgst digest.Digest
dt []byte
}
res, err := is.g.Do(ctx, ref, func(ctx context.Context) (interface{}, error) {
dgst, dt, err := imageutil.Config(ctx, ref, is.getResolver(ctx), is.ContentStore, opt.Platform)
dgst, dt, err := imageutil.Config(ctx, ref, is.getResolver(ctx), is.ContentStore, platform)
if err != nil {
return nil, err
}
return &t{dgst: dgst, dt: dt}, nil
})
var typed *t
if err != nil {
return "", nil, err
}
typed := res.(*t)
typed = res.(*t)
return typed.dgst, typed.dt, nil
}

func (is *imageSource) ResolveImageConfig(ctx context.Context, ref string, opt gw.ResolveImageConfigOpt) (digest.Digest, []byte, error) {
resolveMode, err := source.ParseImageResolveMode(opt.ResolveMode)
if err != nil {
return "", nil, err
}
switch resolveMode {
case source.ResolveModeForcePull:
dgst, dt, err := is.resolveRemote(ctx, ref, opt.Platform)
// TODO: pull should fallback to local in case of failure to allow offline behavior
// the fallback doesn't work currently
return dgst, dt, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; could be

return is.resolveRemote(ctx, ref, opt.Platform)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah true, this was because the original (desired) code is the one commented out below in which case the assignments do make sense.

/*
if err == nil {
return dgst, dt, err
}
// fallback to local
dt, err = is.resolveLocal(ref)
return "", dt, err
*/

case source.ResolveModeDefault:
// default == prefer local, but in the future could be smarter
fallthrough
case source.ResolveModePreferLocal:
dt, err := is.resolveLocal(ref)
if err == nil {
return "", dt, err
}
// fallback to remote
return is.resolveRemote(ctx, ref, opt.Platform)
}
// should never happen
return "", nil, fmt.Errorf("builder cannot resolve image %s: invalid mode %q", ref, opt.ResolveMode)
}

func (is *imageSource) Resolve(ctx context.Context, id source.Identifier) (source.SourceInstance, error) {
imageIdentifier, ok := id.(*source.ImageIdentifier)
if !ok {
Expand Down Expand Up @@ -213,7 +240,7 @@ func (p *puller) resolveLocal() {
}
}

if preferLocal {
if p.src.ResolveMode == source.ResolveModeDefault || p.src.ResolveMode == source.ResolveModePreferLocal {
dt, err := p.is.resolveLocal(p.src.Reference.String())
if err == nil {
p.config = dt
Expand Down Expand Up @@ -257,8 +284,7 @@ func (p *puller) resolve(ctx context.Context) error {
resolveProgressDone(err)
return
}

_, dt, err := p.is.ResolveImageConfig(ctx, ref.String(), gw.ResolveImageConfigOpt{Platform: &p.platform})
_, dt, err := p.is.ResolveImageConfig(ctx, ref.String(), gw.ResolveImageConfigOpt{Platform: &p.platform, ResolveMode: resolveModeToString(p.src.ResolveMode)})
if err != nil {
p.resolveErr = err
resolveProgressDone(err)
Expand Down Expand Up @@ -732,3 +758,17 @@ func cacheKeyFromConfig(dt []byte) digest.Digest {
}
return identity.ChainID(img.RootFS.DiffIDs)
}

// resolveModeToString is the equivalent of github.com/moby/buildkit/solver/llb.ResolveMode.String()
// FIXME: add String method on source.ResolveMode
func resolveModeToString(rm source.ResolveMode) string {
switch rm {
case source.ResolveModeDefault:
return "default"
case source.ResolveModeForcePull:
return "pull"
case source.ResolveModePreferLocal:
return "local"
}
return ""
}
6 changes: 6 additions & 0 deletions builder/builder-next/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ func (b *Builder) Build(ctx context.Context, opt backend.BuildConfig) (*builder.
frontendAttrs["no-cache"] = ""
}

if opt.Options.PullParent {
frontendAttrs["image-resolve-mode"] = "pull"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking if it would be good to use the values that are / will be defined in source.ResolveMode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah actually, could do it in a follow up once buildkit has a String() method on it. I don't want to rely on llb package at this level of abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moby/buildkit#577

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup; definitely not a blocker (same for my other comment) 👍

} else {
frontendAttrs["image-resolve-mode"] = "default"
}

if opt.Options.Platform != "" {
// same as in newBuilder in builder/dockerfile.builder.go
// TODO: remove once opt.Options.Platform is of type specs.Platform
Expand Down
0