builder: Refactor remote context by tonistiigi · Pull Request #31984 · moby/moby

tonistiigi · 2017-03-21T21:45:59Z

~~depends on #31257~~

~~only new commit: tonistiigi/docker@nested-build...remote-context~~

The builder currently seems to depend on Context interface that could be reimplemented to provide different types of sources. In reality, there is only one context: tarsum. That is because the real functionality for context is finding hashes for paths for cache and tarsum is the only context that can do that. Others are just helpers for making tarsum context from other inputs.

Context interface hides direct access to the paths with .Path() method in builder.FileInfo. Actual copy only happens through real paths and doesn't have anything to do with context methods.

File hash capability is the main reason Context exists but that capability is completely missing from the interface. The implementation just recasts builder.FileInfo to builder.Hashed and would panic if they don't match.

Context and Dockerfile are mixed together and builder tries to separate them while it is running. That means recasts to ModifiableContext and DockerIgnorContext. Every context passed to builder is always DockerIgnoreContext but there is nothing predicting that from the function's signature. This has a TODO but there is no way to fix that without separating these concerns.

There are also actual bugs that can't be fixed like hardlink handling because the implementation is fixed to Tarsum.

This PR changes the dependent interface to something that better matches the reality. Source is a path from a host and a capability to calculate hashes for paths.

It also renames Context to Source so that it doesn't collide with context.Context.

This enables following features:

Cache hashing can be separated from the tarsum package. Raw tar headers are not the correct way calculate file hashes. For example hardlinks are completely broken because of that https://gist.github.com/tonistiigi/775cb15d3918958020bdd0165f776005 . This does not mean breaking the compatibility in non-critical places (yet) as we don't want to invalidate people's cache.
Alternative implementations can be used for defining the remotes. Previous implememtation was centric to specific tar behavior.
No .dockerignore magic needs to be implemented for the other implementations. For example Proposal: Incremental build context sending #31829 does not need to worry about this at all.

Follow up:

Hashes for directories shouldn't be calculated by the builder. They should be provided same way as they are for the files.
~~Move all Remote implementations (and detection) into a separate package.~~ Done
~~Remove ModifiableContext. This is now only needed on source detection phase and Remove() function can be directly called on the remote's implementation.~~ Done

tonistiigi · 2017-03-22T00:42:46Z

integration-cli/docker_api_build_test.go

-RUN find / -xdev -name ba*
-RUN find /tmp/`
-	}
+	testD := `FROM busybox


This is the only UI change in this PR. It seems that the test checks an invalid side-effect caused by the previous implementation. I could make a fake context for this case but don't see a practical reason for it, especially after all the work that was needed in #31236 to hide the Dockerfile if no such file actually exists.

Could also add a second case that pulls a tar. Then it will have the same behavior as the previous test.

If I understand this correctly the old test expected a dockerfile to exist at Dockerfile and no file named baz to exist.

But the test fixture seems to be just a remote server with a single file at /testD. Where do these extra files come from?

Previous implementation created a fake context tar after pulling the Dockerfile as having a tar was the only way to start the builder(the implementation didn't bother to create .dockerignore as well so the Dockerfile would not show up). File baz does not exist but is mentioned in the URL. I'm not sure how it would leak, but I don't actually remember why I decided to change the Dockerfile itself. I'll try to change it back and put back the baz assertion.

tiborvass · 2017-03-23T22:19:28Z

builder/remotecontext/lazycontext.go

+	return sum, nil
+}
+
+func (c *lazyContext) normalize(path string) (cleanPath, fullPath string, err error) {


Just a reminder from #31257 (comment)

Also I checked, once you refactored normalize, it is fine to remove the lstat. Any action that was using normalize was doing an Open, or an Lstat, or Remove after it anyways.

simonferquel

I am not sure if I am best suited for such a review. The changes are quite pervasive over the builder code base and I am not familiar enough yet with the builder code to make constructive feedback

dnephin

I like it, just a few minor comments and questions

dnephin · 2017-04-07T15:56:02Z

builder/builder.go

minor: Comment suggests that maybe Source (namespaced it would be builder.Source) might be a more appropriate name?

dnephin · 2017-04-07T16:07:24Z

builder/dockerfile/detectcontext.go

Looks like bm is not used here, so this could be a function instead of an instance method on buildManager.

dnephin · 2017-04-07T16:17:19Z

builder/dockerfile/detectcontext.go

I often find that using a switch/case here makes the intent a little more obvious, without needing a comment:

switch { case os.IsNotExist(err): return nil case err != nil: return err

dnephin · 2017-04-07T16:17:49Z

builder/dockerfile/detectcontext.go

No error on invalid .dockerignore format?

dnephin · 2017-04-07T16:20:17Z

builder/dockerfile/detectcontext_test.go

comment still relevant?

s/testRemote/stubRemote/

?

dnephin · 2017-04-07T16:37:17Z

integration-cli/docker_api_build_test.go

-RUN find / -xdev -name ba*
-RUN find /tmp/`
-	}
+	testD := `FROM busybox


If I understand this correctly the old test expected a dockerfile to exist at Dockerfile and no file named baz to exist.

But the test fixture seems to be just a remote server with a single file at /testD. Where do these extra files come from?

dnephin · 2017-04-07T16:39:28Z

builder/tarsum.go

New unit test for this?

dnephin · 2017-04-21T14:06:56Z

builder/dockerfile/builder.go

I don't think we need this field, dockerfile can be just an arg.

dnephin · 2017-04-21T14:08:47Z

builder/dockerfile/builder.go

Can we pass dockerfile this to build() instead? That way it doesn't need to be a field in the struct.

dnephin · 2017-04-21T14:09:19Z

builder/dockerfile/builder.go

This was just removed in a recent PR. I think with the two suggestions above it wont be necessary anymore

dnephin · 2017-04-21T14:10:11Z

builder/dockerfile/imagecontext.go

rename the field here as well? s/ctx/source

dnephin · 2017-04-21T14:16:29Z

builder/dockerfile/internals.go

b here is only used to get the Dockerfile name now, so this could be:

func readAndParseDockerfile(src io.ReadCloser, filename string) (*parser.Result, error { ... }

dnephin · 2017-04-21T14:18:57Z

builder/dockerfile/internals_test.go

With the suggestion above this could just call readAndParseDockerfile() directly I think.

dnephin · 2017-04-21T14:22:29Z

builder/remotecontext/detect.go

I believe I got most of your changes from #27270 into #32580, which is merged.

However this funtion looks pretty good as is. What's the advantage to returning the parser.Result{} here?

Redefine a better interface for remote context dependency. Separate Dockerfile build instruction from remote context. Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi · 2017-04-26T00:13:44Z

All green. PTAL

dnephin

I like this, nice cleanup.

Just a question about the test change, otherwise I think this looks good.

dnephin · 2017-04-26T15:23:23Z

builder/dockerfile/internals.go

 	progressOutput := stdoutFormatter.StreamFormatter.NewProgressOutput(stdoutFormatter.Writer, true)
 	progressReader := progress.NewProgressReader(resp.Body, progressOutput, resp.ContentLength, "", "Downloading")
 	// Download and dump result to tmp file
+	// TODO: add filehash directly


Not sure what this means, I just want to make sure that it's not necessary for this PR.

Because this is a single file there isn't really a need for LazyContext and reading file back from the disk. We could just create a single hash object and pipe data through it. But that is an optimization for edge case so not currently implemented.

dnephin · 2017-04-26T15:24:23Z

builder/dockerfile/internals.go

+		return nil, err
 	}
+
+	// TODO: remove, handle dirs in Hash()


this is ok for now?

Yes, currently Hash() only returns hash for a single file so to get the correct hash for a dir we still need to recursively walk it. The todo is about moving the walker code inside Hash so the values from that func don't need post-processing.

dnephin · 2017-04-26T15:31:31Z

integration-cli/docker_api_build_test.go

 	var testD string
 	if testEnv.DaemonPlatform() == "windows" {
 		testD = `FROM busybox
-COPY * /tmp/


Does this not work anymore?

Yes, see #31984 (review)

Ahh right, I forgot about this. Thanks!

dnephin

LGTM

cpuguy83 · 2017-04-27T13:03:22Z

builder/remotecontext/detect.go

@@ -0,0 +1,172 @@
+package remotecontext


Elsewhere you renamed Context to Source. I like this, do you think we can do the same for these new *context packages?

I did not rename LazyContext/TarsumContext/modifiableContext inside this pkg for now. Only in the builder where the conflict with context.Context was very clear. More renames could probably be follow up as this is already quite big.

cpuguy83 · 2017-04-27T13:07:44Z

builder/dockerfile/internals.go

 	if allowWildcards && containsWildcards(origPath) {
 		var copyInfos []copyInfo
-		if err := context.Walk("", func(path string, info builder.FileInfo, err error) error {
+		if err := filepath.Walk(source.Root(), func(path string, info os.FileInfo, err error) error {


Are we expecting that this will always be some object on disk?

Yes, the previous implementation expected it as well. The Path() method was just hidden from the interface but the actual copy could never work if it was not implemented.

cpuguy83

LGTM

GordonTheTurtle added the status/0-triage label Mar 21, 2017

tonistiigi changed the title ~~Remote context~~ Refactor remote context parsing Mar 21, 2017

tonistiigi commented Mar 22, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Mar 22, 2017

tonistiigi mentioned this pull request Mar 23, 2017

build: add multi-stage build support #31257

Merged

tiborvass reviewed Mar 23, 2017

View reviewed changes

tonistiigi force-pushed the remote-context branch from 6a02f6b to 75c6ebb Compare March 24, 2017 18:46

tonistiigi changed the title ~~Refactor remote context parsing~~ builder: Refactor remote context Mar 25, 2017

vdemeester requested review from simonferquel, tiborvass and vdemeester April 1, 2017 18:56

GordonTheTurtle assigned justincormack Apr 5, 2017

simonferquel reviewed Apr 7, 2017

View reviewed changes

vdemeester requested a review from dnephin April 7, 2017 13:37

dnephin reviewed Apr 7, 2017

View reviewed changes

tonistiigi force-pushed the remote-context branch 3 times, most recently from cbe6539 to e415e86 Compare April 14, 2017 21:57

tonistiigi mentioned this pull request Apr 18, 2017

Implement long running interactive session and sending build context incrementally #32677

Merged

tonistiigi force-pushed the remote-context branch from e415e86 to ea52a3d Compare April 20, 2017 20:34

dnephin reviewed Apr 21, 2017

View reviewed changes

dnephin added rebuild/janky labels Apr 21, 2017

GordonTheTurtle removed rebuild/janky labels Apr 21, 2017

tonistiigi force-pushed the remote-context branch from ea52a3d to a1662af Compare April 25, 2017 21:09

Refactor remote context parsing

d1faf3d

Redefine a better interface for remote context dependency. Separate Dockerfile build instruction from remote context. Signed-off-by: Tonis Tiigi <[email protected]>

tonistiigi force-pushed the remote-context branch from a1662af to d1faf3d Compare April 25, 2017 21:35

dnephin reviewed Apr 26, 2017

View reviewed changes

dnephin approved these changes Apr 26, 2017

View reviewed changes

tonistiigi requested a review from cpuguy83 April 26, 2017 23:05

cpuguy83 reviewed Apr 27, 2017

View reviewed changes

cpuguy83 approved these changes Apr 27, 2017

View reviewed changes

cpuguy83 merged commit ae0f8c7 into moby:master Apr 27, 2017

tonistiigi mentioned this pull request May 3, 2017

Fix an issue on Windows, where dockerfile is not removed from daemon-side build context tonistiigi/docker#3

Open

thaJeztah mentioned this pull request Aug 22, 2022

contrib: update Windows busybox to FRP-4716-g31467ddfc #44000

Draft

Conversation

tonistiigi commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonferquel left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Apr 26, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Mar 21, 2017 •

edited

Loading

dnephin Apr 21, 2017 •

edited

Loading

dnephin Apr 21, 2017 •

edited

Loading