Skip to content
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.

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

Support for experimental BuildKit #1111

Merged
merged 20 commits into from Jun 14, 2018
Merged

Conversation

@tiborvass
Copy link
Contributor

tiborvass commented Jun 5, 2018

This PR makes docker build use the experimental support in moby/moby#37151 when the DOCKER_BUILDKIT environment variable is set.

This is still a work in progress.

To test it, build the docker CLI from this PR, and use it with the engine built from moby/moby#37151.
Mainly looking for UX feedback and bad bugs.

The output of the builder is changing (using that of buildkit for interactive mode). Non-interactive output is currently JSON stream for now but it will likely evolve to a more human-readable output (the challenge being handling output of parallel RUN commands).

Known issues:

  • no support for --iidfile (coming soon)
  • no support for docker content trust (out of scope for this PR)

Ping @tonistiigi @thaJeztah @vdemeester @garethr

Copy link
Contributor

simonferquel left a comment

Small nits, but overall seem ok :)

Makefile Outdated
@@ -52,7 +52,7 @@ watch: ## monitor file changes and run go test

vendor: vendor.conf ## check that vendor matches vendor.conf
rm -rf vendor
bash -c 'vndr |& grep -v -i clone'
bash -c 'vndr -verbose'

This comment has been minimized.

Copy link
@simonferquel

simonferquel Jun 5, 2018

Contributor

Is this change related to the PR?

// If an archive is detected in the input stream, rc stream is non-nil and ready
// to be consumed in lieu of the input stream, and dockerfileDir is empty.
//
// If a Dockerfile is detected, rc is set to nil and dockerfileDir is set to a

This comment has been minimized.

Copy link
@simonferquel

simonferquel Jun 5, 2018

Contributor

Comment does not seem in sync

// name specified by DefaultDockerfileName and returns the path to the
// temporary directory containing the Dockerfile.
func WriteTempDockerfile(rc io.ReadCloser) (dockerfileDir string, err error) {
dockerfileDir, err = ioutil.TempDir("", "docker-build-tempdockerfile-")

This comment has been minimized.

Copy link
@simonferquel

simonferquel Jun 5, 2018

Contributor

error management in this function seem more complicated than necessary. As the err output parameter is named, why not use a single defer block to centralize error reporting ?

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

nit: consider renaming the err output var, to prevent it from being confused with local vars (we ran into a couple of cases where this was overlooked in code changes)

@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from 61295de to b30095b Jun 6, 2018
@tiborvass

This comment has been minimized.

Copy link
Contributor Author

tiborvass commented Jun 6, 2018

@simonferquel updated. Appreciate your review!

}
_, err = io.Copy(f, buf)
_, err = io.Copy(f, rc)
if err != nil {
f.Close()

This comment has been minimized.

Copy link
@simonferquel

simonferquel Jun 6, 2018

Contributor
defer func (){
  er := f.Close ()
  if err == nil {
    err = er
  }
}

Not sure about it, but I dislike having duplicated Close call. Seem error prone if/when new code paths are introduced in the future.

@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch 2 times, most recently from b3864cd to 9961958 Jun 9, 2018
@@ -149,6 +152,9 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
flags.SetAnnotation("stream", "experimental", nil)
flags.SetAnnotation("stream", "version", []string{"1.31"})

flags.BoolVar(&options.noConsole, "no-console", false, "Show non-console output")

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Jun 9, 2018

Contributor

Maybe better to add "BuildKit mode only"?

This comment has been minimized.

Copy link
@tiborvass

tiborvass Jun 9, 2018

Author Contributor

Sure. BTW, the main reason we have this flag, is because the interactive output of buildkit doesn't show outputs of RUN commands. If it did, we might not need this flag. Which makes me wonder whether we could remove this flag in the future as it graduates from experimental. cc @tonistiigi @vdemeester

@@ -170,6 +176,10 @@ func (out *lastProgressOutput) WriteProgress(prog progress.Progress) error {

// nolint: gocyclo
func runBuild(dockerCli command.Cli, options buildOptions) error {
if os.Getenv("DOCKER_BUILDKIT") != "" {
return runBuildBuildKit(dockerCli, options)

This comment has been minimized.

Copy link
@AkihiroSuda

AkihiroSuda Jun 9, 2018

Contributor

Any reason not to use CLI flag?

This comment has been minimized.

Copy link
@tiborvass

tiborvass Jun 9, 2018

Author Contributor

@AkihiroSuda because in the future I'd like to remove it.

@@ -184,6 +195,13 @@ func (ctx *DiskUsageContext) verboseWrite() error {

// And build cache
fmt.Fprintf(ctx.Output, "\nBuild cache usage: %s\n\n", units.HumanSize(float64(ctx.BuilderSize)))

t := template.Must(template.New("buildcache").Parse(defaultBuildCacheVerboseFormat))

This comment has been minimized.

Copy link
@vdemeester

vdemeester Jun 9, 2018

Collaborator

This should be only visible when the daemon is experimental right ?

@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from 9961958 to ddc1763 Jun 9, 2018
tonistiigi and others added 7 commits Apr 18, 2018
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
…ile and context

Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from ddc1763 to 9939444 Jun 9, 2018
Copy link
Collaborator

vdemeester left a comment

Design LGTM 👼

@simonferquel

This comment has been minimized.

Copy link
Contributor

simonferquel commented Jun 12, 2018

Lgtm :)

}
return dockerfileDir, rc.Close()

This comment has been minimized.

Copy link
@ijc

ijc Jun 13, 2018

Contributor

If rc.Close() returns an error then the callers will leak tmpDir, since they don't clean it up in the error case (and it would be a little odd to expect them to do so).

The dir is also leaked on errors in in the os.Create above too.

This comment has been minimized.

Copy link
@tiborvass

tiborvass Jun 13, 2018

Author Contributor

@ijc Addressed.

remote = uploadRequestRemote
} else {
if options.dockerfileName != "" {
// This is new behavior, but @tiborvass believes it's correct.

This comment has been minimized.

Copy link
@ijc

ijc Jun 13, 2018

Contributor

This looks like a discussion for the commit message rather than a code comment.

This comment has been minimized.

Copy link
@tiborvass

tiborvass Jun 13, 2018

Author Contributor

@ijc addressed in commit message.

@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from f08a015 to eb91ac0 Jun 13, 2018
tiborvass and others added 11 commits May 23, 2018
This commit brings a more pedantic change in the following ambiguous case:
cat Dockerfile | docker build -f otherDockerfile -

The legacy builder does not error out and prefers the Dockerfile from stdin
while the buildkit-based one errors out.

Note that this is only in the case where stdin is a Dockerfile (not an archive)

Signed-off-by: Tibor Vass <tibor@docker.com>
With this patch the following become true even with buildkit enabled:
1. `docker build -q .` only outputs the created image's sha256 ID.
2. `docker build -q .` outputs as if no `-q` was specified, if error occurred
3. `docker build . &> out` outputs JSON (instead of TTY characters)

Signed-off-by: Tibor Vass <tibor@docker.com>
…string enables the use of buildkit

Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
…adable output with buildkit

Unfortunately, this is for now the only way to see the output of RUN commands when using buildkit.
It is equivalent to `DOCKER_BUILDKIT=1 docker build . 2>&1 | cat`

Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from eb91ac0 to 00792d1 Jun 13, 2018
Copy link
Contributor

andrewhsu left a comment

LGTM

I created a local build using moby commit 692df46 for the engine and this PR's cli commit 00792d1. Stuff works when testing a build of docker-dev image from moby/moby repo:

$ DOCKER_BUILDKIT=1 docker build -t docker-dev .
[+] Building 124.1s (58/58) FINISHED

One issue to address for future before this goes out of experimental is to make sure the error message is nicer if the cli is talking to an older version of an engine that does not have buildkit:

$ DOCKER_BUILDKIT=1 docker build -t docker-dev .
Error response from daemon: failed to copy to /var/lib/docker/builder/e9f592f09a03f2584b07a250ec1dc3e46b0e580b9da7e311021d55a6c0b35f86: rpc error: code = Unknown desc = no access allowed to dir ""
Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from b1f3b4b to 5a103e1 Jun 13, 2018
@tiborvass

This comment has been minimized.

Copy link
Contributor Author

tiborvass commented Jun 13, 2018

FYI I changed --no-console to --console=[true|false|auto].

Copy link
Member

thaJeztah left a comment

left some questions / comments


// if span := opentracing.SpanFromContext(ctx); span != nil {
// statusContext = opentracing.ContextWithSpan(statusContext, span)
// }

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

Should this be removed before merging?

if cons, err := console.ConsoleFromFile(out); err == nil && (consoleOpt == nil || *consoleOpt) {
c = cons
}
// not using shared context to not disrupt display but let is finish reporting errors

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

typo: s/let is/let it/

}
// if options.quiet {
// fmt.Fprintf(dockerCli.Err(), "%s%s", progBuff, buildBuff)
// }

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

should this be removed before merging? (or a TODO added)

}

func (c *diskUsageBuilderContext) Size() string {
return units.HumanSize(float64(c.builderSize))
}

func (c *diskUsageBuilderContext) Reclaimable() string {
return c.Size()
inUseBytes := int64(0)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

var inUseBytes int64 ?

// name specified by DefaultDockerfileName and returns the path to the
// temporary directory containing the Dockerfile.
func WriteTempDockerfile(rc io.ReadCloser) (dockerfileDir string, err error) {
dockerfileDir, err = ioutil.TempDir("", "docker-build-tempdockerfile-")

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

nit: consider renaming the err output var, to prevent it from being confused with local vars (we ran into a couple of cases where this was overlooked in code changes)

return errors.Errorf("buildkit not supported by daemon")
}

buildID := stringid.GenerateRandomID()

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

looks like this isn't used until quite some lines below; could you move this closer to where it's used?

// }
return cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code}
}
return err

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

this is redundant if you change the return below to return err.

perhaps changing the if err != nil to if err == nil above (and return early), could make the logic a bit less complicated


func resetUIDAndGID(s *fsutil.Stat) bool {
s.Uid = uint32(0)
s.Gid = uint32(0)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

is the type conversion needed here? (just wondering)

func resetUIDAndGID(s *fsutil.Stat) bool {
s.Uid = uint32(0)
s.Gid = uint32(0)
return true

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

was wondering why the boolean return was here, but guess this is to satisfy an interface? (reading from my phone so couldn't check easily)

if err := json.Unmarshal(*msg.Aux, &dt); err != nil {
return
}
if err := (&resp).Unmarshal(dt); err != nil {

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 13, 2018

Member

it's ok to hide them actual errors? May be good to document that (as a comment here and/or godoc on the function)

@andrewhsu

This comment has been minimized.

Copy link
Contributor

andrewhsu commented Jun 13, 2018

Stuff still works with 5a103e1 with my manual build. I also ran a docker-ce integration test suite and stuff is green.

@tiborvass

This comment has been minimized.

Copy link
Contributor Author

tiborvass commented Jun 13, 2018

@thaJeztah updated

Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the tiborvass:experimental-buildkit branch from 913405f to b3a5c15 Jun 13, 2018
Copy link
Member

thaJeztah left a comment

visually LGTM, but away from keyboard 😅

we'll need some documentation follow up likely

// name specified by DefaultDockerfileName and returns the path to the
// temporary directory containing the Dockerfile.
func WriteTempDockerfile(rc io.ReadCloser) (dockerfileDir string, err error) {
// err is a named return value, due to the defer call below.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jun 14, 2018

Member

was thinking naming it retErr (eg), but just a nit; no need to change

@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Jun 14, 2018

LGTM

@andrewhsu andrewhsu merged commit 2daec78 into docker:master Jun 14, 2018
7 checks passed
7 checks passed
ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed
@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.