Incrementally sending build context #231

tonistiigi · 2017-06-22T22:18:58Z

Adds support for incremental context updates to docker build moby/moby#32677 . The feature is behind a --stream=true flag atm, mostly for a testing period and in case there is some reason to use the old method. When we feel confident enough we can just switch the default value and users shouldn't see any changes except things being faster.

I also noticed that it vendors in chrootarchive and reexec package. These are not really needed but require some refactoring. Either splitting the receiver and sender part to a separate package, or passing an archiver instance into the transfer function. I can do that in a follow-up with next update.

@dnephin @vieux

codecov-io · 2017-06-22T23:07:42Z

Codecov Report

Merging #231 into master will decrease coverage by 0.61%.
The diff coverage is 25.44%.

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
- Coverage   47.19%   46.58%   -0.62%     
==========================================
  Files         171      172       +1     
  Lines       11556    11631      +75     
==========================================
- Hits         5454     5418      -36     
- Misses       5791     5902     +111     
  Partials      311      311

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

dnephin

Some tests would be great.

We don't have any in master yet, but I wrote on in #233 which uses a fakeClient. You could grab my changes to the fake from that PR.

Splitting out some functions might make it possible to test some in isolation as well (like the build shared key functionality).

dnephin · 2017-06-23T19:07:56Z

cli/command/cli.go

@@ -200,7 +200,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error {

 		// if server version is lower than the current cli, downgrade
 		if versions.LessThan(ping.APIVersion, cli.client.ClientVersion()) {
-			cli.client.UpdateClientVersion(ping.APIVersion)
+			cli.client.NegotiateAPIVersionPing(ping)


NegotiateAPIVersionPing() does the minimum version check as well, so the previous if ping.APIVersion == "" can be removed.

Oh, it also does this LessThan check, so this if can be removed as well.

dnephin · 2017-06-23T19:15:39Z

cli/command/image/build.go

@@ -133,6 +146,10 @@ func NewBuildCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.SetAnnotation("squash", "experimental", nil)
 	flags.SetAnnotation("squash", "version", []string{"1.25"})

+	flags.BoolVar(&options.stream, "stream", false, "Stream attaches to server to negotiate build context")
+	flags.SetAnnotation("stream", "experimental", nil)
+	flags.SetAnnotation("stream", "version", []string{"1.31"})


I think we normally don't set the version until it's out of experimental. I'm not sure how it works if we set both. Maybe it just works?

squash option above seems to set it

dnephin · 2017-06-23T19:20:07Z

cli/command/image/build.go

+			// Dockerfile which uses trusted pulls.
+			buildCtx = replaceDockerfileTarWrapper(ctx, buildCtx, relDockerfile, translator, &resolvedTags)
+		} else if dockerfileCtx != nil {
+			newDockerfile, _, err := rewriteDockerfileFrom(context.Background(), dockerfileCtx, translator)


Shouldn't this use ctx for the context?

dnephin · 2017-06-23T19:23:11Z

cli/command/image/build.go

@@ -250,16 +272,34 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 		}
 	}

-	ctx := context.Background()
+	if options.stream && dockerfileCtx == nil {


Is dockerfileCtx == nil the same as !options.dockerfileFromStdin() ? Could we use the latter so it's more clear what this means?

I'll add a comment. In this case they are equal as nothing has modified dockerfileCtx yet but this will change in other comparisons. dockerfileCtx doesn't really mean that it has to be from stdin, it means that custom Dockerfile source has been set.

dnephin · 2017-06-23T19:24:13Z

cli/command/image/build.go

@@ -250,16 +272,34 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 		}
 	}

-	ctx := context.Background()
+	if options.stream && dockerfileCtx == nil {
+		f, err := os.Open(relDockerfile)


minor nit: you can assign directly to dockerfileCtx here, no need to create a temporary variable.

dnephin · 2017-06-23T19:37:38Z

cli/command/image/build.go

+		buildCtx = dockerfileCtx
+	}
+	var s *session.Session
+	if isSessionSupported(dockerCli) {


This function is already way too large. There's no way we can probably unit test most of it. We really should not be adding any significant code to it.

Could we make the rest of this (up to line 362, and the if s != nil block) into a few new functions? Even if the arg list is long, at this point I think it's still an improvement. We can work on refactoring args into structs in a follow up.

Maybe even a new file along with sizeProgress and bufferedWriter ? That way all the session handling stuff is together and not mixed up with other build things.

dnephin · 2017-06-23T19:39:54Z

cli/command/image/build.go

 	}

 	response, err := dockerCli.Client().ImageBuild(ctx, body, buildOptions)
 	if err != nil {
 		if options.quiet {
 			fmt.Fprintf(dockerCli.Err(), "%s", progBuff)
 		}
+		cancel()


Isn't this already handled by the defer cancel() ?

There is a defer that makes sure we have always written out the progress before returning from this function. This cancel is for unblocking that. I'll add a comment.

dnephin · 2017-06-23T19:42:19Z

cli/command/image/build.go

+		}()
+		buildBuff = buf
+
+		remote = "client-session"


dnephin · 2017-06-23T19:46:07Z

cli/command/image/build.go

+
+func getBuildSharedKey(dir string) (string, error) {
+	dt := []byte(cliconfig.Dir()) // if no access to config dir then only use path
+	if err := os.MkdirAll(cliconfig.Dir(), 0700); err == nil {


Shouldn't "no access" be an error? This seems like it could result in some pretty surprising errors, and is also likely to be rare, so not really a big inconvenience.

Maybe there should be a flag to set a filepath or directory for this?

Currently it would still use the configdir+dir combination as sharedKey what should work well for 99% of the cases. And even if there is a collision it just means that performance will not be ideal. Maybe print a warning?

dnephin · 2017-06-23T19:54:50Z

cli/command/image/build.go

+			}
+		}
+
+		dt, err = ioutil.ReadFile(sessionFile)


I think this function would be easier to read as a struct, something like

type SharedKey struct { ... } func (...) Read() ([]byte, error) {...} func (...) create() (error) {...}

Then getBuildSharedKey() could just call Read() and would only be concerned with hashing the value.

There will never be multiple instances of sharedkey and create is only called if read fails. Maybe helps to split this to getSessionSharedKey(dir) and getNodeIdentifier() as some hashing is always needed. Getting node identifier can be skipped if it has already run once.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi · 2017-06-23T23:34:00Z

@dnephin Updated. Added more comments for the conditions and moved the helper functions to a separate file where possible.

vieux · 2017-06-26T18:39:45Z

LGTM

tiborvass · 2017-06-26T21:03:20Z

cli/command/image/build.go

+			if err := ioutil.WriteFile(sessionFile, []byte(hex.EncodeToString(b)), 0600); err != nil {
+				return "", err
+			}
+		} else {


small nit: not needed. Don't push a fix only for this small nit.

tiborvass · 2017-06-26T21:03:44Z

cli/command/image/build.go

+func getBuildSharedKey(dir string) (string, error) {
+	// build session is hash of build dir with node based randomness
+	sessionFile := filepath.Join(cliconfig.Dir(), ".buildsharedkey")
+	if _, err := os.Lstat(sessionFile); err != nil {


Curious: why Lstat and not simply Stat ?

No particular reason. We are not creating symlinks to this file so I don't think we should handle them as well.

thaJeztah

left some questions/comments

thaJeztah · 2017-06-26T20:58:14Z

cli/command/image/build.go

@@ -373,6 +414,36 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 	return nil
 }

+func getBuildSharedKey(dir string) (string, error) {
+	// build session is hash of build dir with node based randomness
+	sessionFile := filepath.Join(cliconfig.Dir(), ".buildsharedkey")


Should this be in a tmp-dir? Also, what happens if multiple clients perform a build in parallel?

No, it needs to persist. This is only written once and correctness of this value only affects performance.

thaJeztah · 2017-06-26T21:01:51Z

cli/command/formatter/disk_usage_test.go

@@ -23,6 +23,7 @@ func TestDiskUsageContextFormatWrite(t *testing.T) {
 Images              0                   0                   0B                  0B
 Containers          0                   0                   0B                  0B
 Local Volumes       0                   0                   0B                  0B
+Build Cache                                                 0B                  0B


Wonder if we should have something (-) in the first two columns, but we can tweak that later

thaJeztah · 2017-06-26T21:09:22Z

cli/command/image/build.go

 	}

-	var body io.Reader = progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon")
+	// add context stream to the session
+	if options.stream && s != nil {


This function (runBuild()) is getting awfully long now; we should have a look at refactoring it a bit, and splitting parts to separate functions where possible

I already moved some of it to the session file (and actually think it became less readable). This is synchronized with the build runner so needs to be in the same function. We could, of course, split things up like opts setting of context preparing but this is already large enough.

Definitely doesn't have to be part of this PR; I opened an issue for it; #242

thaJeztah · 2017-06-26T21:17:11Z

cli/command/image/build_session.go

+
+const clientSessionRemote = "client-session"
+
+func isSessionSupported(dockerCli *command.DockerCli) bool {


commit 0c780c8 adds isSessionSupported() and getBuildSharedKey(), which are moved in 9776fb5, perhaps those commits should be squashed, or the change moved from one to the other.

I can squash but that merges session+filesync. The move was requested from review. I think splitting up would take too much time.

thaJeztah · 2017-06-26T21:17:42Z

cli/command/image/build_session.go

+func tryNodeIdentifier() (out string) {
+	out = cliconfig.Dir() // return config dir as default on permission error
+	if err := os.MkdirAll(cliconfig.Dir(), 0700); err == nil {
+		sessionFile := filepath.Join(cliconfig.Dir(), ".buildsharedkey")


Should this be in a tmp-dir? What happens if multiple clients perform a build in parallel?

tiborvass · 2017-06-26T22:01:09Z

cli/command/image/build.go

+
+	// add context stream to the session
+	if options.stream && s != nil {
+		syncDone := make(chan error)


Please add comment on why this chan error is not used to retrieve errors but as a syncing mechanism.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Add incremental context send support Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi · 2017-06-26T23:30:59Z

@thaJeztah Updated the filename and squashed.

@tiborvass Added comment.

thaJeztah

LGTM

tiborvass · 2017-06-27T00:09:46Z

LGTM

dnephin · 2017-07-04T18:36:35Z

cli/command/image/build.go

+			buildCtx = replaceDockerfileTarWrapper(ctx, buildCtx, relDockerfile, translator, &resolvedTags)
+		} else if dockerfileCtx != nil {
+			// if there was not archive context still do the possible replacements in Dockerfile
+			newDockerfile, _, err := rewriteDockerfileFrom(ctx, dockerfileCtx, translator)


@tonistiigi I notice that resolvedTags is not being assigned here. Isn't this necessary for content trust?

dnephin · 2017-07-04T22:02:00Z

cli/command/image/build.go

+			case <-ctx.Done():
+			}
+		}()
+		buildBuff = buf


If options.quiet is true then buildBuff is a bytes.Buffer. Later on it is expected that this remains a bytes.Buffer() so that its contents can be printed.

I think this breaks with --quiet

agh, sprintf of io.Writer

cli/cli/command/image/build.go

Line 436 in 30933b5

imageID = fmt.Sprintf("%s", buildBuff)

🤦‍♂️

I have this fixed in #294, it only sets buf if !quiet

GordonTheTurtle added the status/0-triage label Jun 22, 2017

tonistiigi changed the title ~~Client session fssession~~ Incrementally sending build context Jun 22, 2017

tonistiigi force-pushed the client-session-fssession branch from c2469cb to 8278298 Compare June 22, 2017 23:07

vdemeester added status/2-code-review and removed status/0-triage labels Jun 23, 2017

vdemeester added this to the 17.07.0 milestone Jun 23, 2017

dnephin and others added 2 commits June 23, 2017 11:34

Fix cyclomatic complexity of two formatters

c60d13e

Signed-off-by: Daniel Nephin <dnephin@docker.com>

vendor: update moby

4adf701

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the client-session-fssession branch from b07f52d to 5dafb6c Compare June 23, 2017 18:34

dnephin reviewed Jun 23, 2017

View reviewed changes

fix build issue with updated moby

ae8d049

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the client-session-fssession branch 2 times, most recently from bf5a8c0 to 9776fb5 Compare June 23, 2017 23:24

tiborvass reviewed Jun 26, 2017

View reviewed changes

thaJeztah requested changes Jun 26, 2017

View reviewed changes

tiborvass reviewed Jun 26, 2017

View reviewed changes

Use long running session in builder

b95638a

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Add incremental context send support Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the client-session-fssession branch from 9776fb5 to b95638a Compare June 26, 2017 23:30

thaJeztah mentioned this pull request Jun 26, 2017

Refactor build/runBuild() #242

Closed

thaJeztah approved these changes Jun 26, 2017

View reviewed changes

vieux merged commit 0133e13 into docker:master Jun 27, 2017

thaJeztah added impact/changelog impact/documentation labels Jun 30, 2017

dnephin reviewed Jul 4, 2017

View reviewed changes

tonistiigi mentioned this pull request Jul 5, 2017

build: follow up fixes for build stream #300

Closed

thaJeztah mentioned this pull request Jul 27, 2017

The build and copy steps take an absurdly long time moby/moby#30414

Closed

albers mentioned this pull request Nov 8, 2017

Add bash completion for experimental build --stream #678

Merged

This was referenced Oct 2, 2019

build: remove --stream (was experimental) #2105

Merged

builder: remove legacy build's session handling moby/moby#39983

Merged

thaJeztah mentioned this pull request Jul 8, 2021

Update /system/df example swagger response moby/moby#42605

Closed


		const clientSessionRemote = "client-session"

		func isSessionSupported(dockerCli *command.DockerCli) bool {

Incrementally sending build context #231

Incrementally sending build context #231

Conversation

tonistiigi commented Jun 22, 2017

codecov-io commented Jun 22, 2017 • edited Loading

Codecov Report

dnephin 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

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

tonistiigi commented Jun 23, 2017

vieux commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

tonistiigi commented Jun 26, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

tiborvass commented Jun 27, 2017

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

codecov-io commented Jun 22, 2017 •

edited

Loading