Support for experimental BuildKit #1111

tiborvass · 2018-06-05T00:25:08Z

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

simonferquel

Small nits, but overall seem ok :)

simonferquel · 2018-06-05T08:13:11Z

Makefile

@@ -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'


Is this change related to the PR?

simonferquel · 2018-06-05T08:34:55Z

cli/command/image/build/context.go

+// 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


Comment does not seem in sync

simonferquel · 2018-06-05T08:37:34Z

cli/command/image/build/context.go

+// 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-")


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 ?

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 · 2018-06-06T00:30:55Z

@simonferquel updated. Appreciate your review!

simonferquel · 2018-06-06T07:09:28Z

cli/command/image/build/context.go

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


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.

AkihiroSuda · 2018-06-09T04:54:25Z

cli/command/image/build.go

@@ -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")


Maybe better to add "BuildKit mode only"?

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

AkihiroSuda · 2018-06-09T05:17:32Z

cli/command/image/build.go

@@ -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)


Any reason not to use CLI flag?

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

vdemeester · 2018-06-05T16:15:43Z

cli/command/formatter/disk_usage.go

@@ -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 should be only visible when the daemon is experimental right ?

Signed-off-by: Tonis Tiigi <[email protected]>

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

…ile and context Signed-off-by: Tibor Vass <[email protected]>

vdemeester

Design LGTM 👼

tiborvass · 2018-06-12T21:29:28Z

Ping @simonferquel if you have any further comments.

simonferquel · 2018-06-12T21:43:44Z

Lgtm :)

ijc · 2018-06-13T08:33:38Z

cli/command/image/build/context.go

 	}
+	return dockerfileDir, rc.Close()


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.

@ijc Addressed.

ijc · 2018-06-13T08:37:22Z

cli/command/image/build_buildkit.go

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


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

@ijc addressed in commit message.

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 <[email protected]>

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 <[email protected]>

…string enables the use of buildkit Signed-off-by: Tibor Vass <[email protected]>

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

…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 <[email protected]>

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

Signed-off-by: Tonis Tiigi <[email protected]>

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

andrewhsu

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 <[email protected]>

tiborvass · 2018-06-13T21:23:59Z

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

thaJeztah

left some questions / comments

thaJeztah · 2018-06-13T21:15:04Z

cli/command/image/build_buildkit.go

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


Should this be removed before merging?

thaJeztah · 2018-06-13T21:18:12Z

cli/command/image/build_buildkit.go

+		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


typo: s/let is/let it/

thaJeztah · 2018-06-13T21:20:56Z

cli/command/image/build_buildkit.go

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


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

thaJeztah · 2018-06-13T21:22:40Z

cli/command/formatter/disk_usage.go

 }

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

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


var inUseBytes int64 ?

thaJeztah · 2018-06-13T21:25:44Z

cli/command/image/build/context.go

+// 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-")


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)

thaJeztah · 2018-06-13T21:34:53Z

cli/command/image/build_buildkit.go

+		return errors.Errorf("buildkit not supported by daemon")
+	}
+
+	buildID := stringid.GenerateRandomID()


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

thaJeztah · 2018-06-13T21:42:02Z

cli/command/image/build_buildkit.go

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


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

thaJeztah · 2018-06-13T21:43:38Z

cli/command/image/build_buildkit.go

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


is the type conversion needed here? (just wondering)

thaJeztah · 2018-06-13T21:46:07Z

cli/command/image/build_buildkit.go

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


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)

thaJeztah · 2018-06-13T21:49:27Z

cli/command/image/build_buildkit.go

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


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 · 2018-06-13T21:56:52Z

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

tiborvass · 2018-06-13T22:35:32Z

@thaJeztah updated

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

thaJeztah

visually LGTM, but away from keyboard 😅

we'll need some documentation follow up likely

thaJeztah · 2018-06-13T23:59:18Z

cli/command/image/build/context.go

+// 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.


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

tonistiigi · 2018-06-14T01:20:00Z

LGTM

GordonTheTurtle added the status/0-triage label Jun 5, 2018

tiborvass force-pushed the experimental-buildkit branch from fc87eac to 61295de Compare June 5, 2018 00:48

vdemeester added status/1-design-review area/builder and removed status/0-triage labels Jun 5, 2018

vdemeester requested review from vdemeester, tonistiigi, simonferquel, thaJeztah and silvin-lubecki June 5, 2018 06:55

simonferquel suggested changes Jun 5, 2018

View reviewed changes

tiborvass force-pushed the experimental-buildkit branch from 61295de to b30095b Compare June 6, 2018 00:30

simonferquel reviewed Jun 6, 2018

View reviewed changes

tiborvass force-pushed the experimental-buildkit branch 2 times, most recently from b3864cd to 9961958 Compare June 9, 2018 03:34

AkihiroSuda reviewed Jun 9, 2018

View reviewed changes

vdemeester reviewed Jun 9, 2018

View reviewed changes

tiborvass force-pushed the experimental-buildkit branch from 9961958 to ddc1763 Compare June 9, 2018 17:25

tonistiigi and others added 7 commits June 9, 2018 20:51

build: add experimental buildkit base

656fe85

Signed-off-by: Tonis Tiigi <[email protected]>

build: basic buildkit progress support

0f97642

Signed-off-by: Tonis Tiigi <[email protected]>

system: add buildcache formatting

b19294e

Signed-off-by: Tonis Tiigi <[email protected]>

build: use a separate upload request for early progress

8cf213b

Signed-off-by: Tonis Tiigi <[email protected]>

build: Add support for using context from stdin with buildkit

e0b3921

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

build: Add support for using dockerfile from stdin with buildkit

5314a8f

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

build: error out if buildkit is on and stdin is used for both dockerf…

89e1024

…ile and context Signed-off-by: Tibor Vass <[email protected]>

tiborvass force-pushed the experimental-buildkit branch from ddc1763 to 9939444 Compare June 9, 2018 20:52

vdemeester reviewed Jun 11, 2018

View reviewed changes

AkihiroSuda approved these changes Jun 12, 2018

View reviewed changes

ijc reviewed Jun 13, 2018

View reviewed changes

tiborvass force-pushed the experimental-buildkit branch from f08a015 to eb91ac0 Compare June 13, 2018 18:30

Tibor Vass and others added 11 commits June 13, 2018 18:32

build: setting DOCKER_BUILDKIT environment variable to any non-empty …

b2b3f9c

…string enables the use of buildkit Signed-off-by: Tibor Vass <[email protected]>

formatter: fix TestDiskUsageContextFormatWrite expected output

584d59d

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

build: simplify Close logic in WriteTempDockerfile

15674d9

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

build: fix lint issues + refactor

5919e8a

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

build: skip moby.buildkit.trace Aux message to be future proof

aef4209

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

vendor: update buildkit and fsutil

8945270

Signed-off-by: Tonis Tiigi <[email protected]>

vendor: update docker/docker to c752b0991e31ba9869ab6a0661af57e9423874fb

6c60bb4

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

build: ensure temporary folder is removed in error case

00792d1

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

tiborvass force-pushed the experimental-buildkit branch from eb91ac0 to 00792d1 Compare June 13, 2018 18:32

andrewhsu approved these changes Jun 13, 2018

View reviewed changes

build: change --no-console to --console=[true|false|auto]

5a103e1

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

tiborvass force-pushed the experimental-buildkit branch from b1f3b4b to 5a103e1 Compare June 13, 2018 21:07

thaJeztah requested changes Jun 13, 2018

View reviewed changes

build: address some review nits

b3a5c15

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

tiborvass force-pushed the experimental-buildkit branch from 913405f to b3a5c15 Compare June 13, 2018 22:40

thaJeztah approved these changes Jun 14, 2018

View reviewed changes

andrewhsu merged commit 2daec78 into docker:master Jun 14, 2018

GordonTheTurtle added this to the 18.06.0 milestone Jun 14, 2018

Support for experimental BuildKit #1111

Support for experimental BuildKit #1111

Conversation

tiborvass commented Jun 5, 2018

simonferquel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tiborvass commented Jun 12, 2018

simonferquel commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhsu left a comment • edited Loading

Choose a reason for hiding this comment

tiborvass commented Jun 13, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhsu commented Jun 13, 2018

tiborvass commented Jun 13, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Jun 14, 2018

andrewhsu left a comment •

edited

Loading