[Builder] Move file coping from the daemon to the builder by dnephin · Pull Request #33454 · moby/moby

dnephin · 2017-05-31T17:11:12Z

Implements items 4, 5, 6 in #32904

Removes Daemon.CopyOnBuild() and moves the copy logic to the builder

tonistiigi · 2017-06-01T20:47:53Z

image/image.go

I think we can just stop using that field. I don't think it is used for anything, conflicts with #30611 etc.

This is also used by commit. In that case it seems more relevant because it would be the ID of the source container.

What do you think about waiting to do this in a follow up? That way we can draw more attention to the change without the distraction of these other unrelated changes.

Yes, I didn't notice that builder doesn't set it. It can be follow up.

tonistiigi · 2017-06-01T20:53:21Z

builder/builder.go

This should be a conversion function AddHistory(image, history) image to make it explicit that this does not create any parent/child relationship.

We could make a package that just contains the schema of the image type and directly use that pkg here instead of defining an image interface. We should not have a dependency on imagestore/layerstore though like the current image pkg has. This part could be a follow-up with a todo.

I'm happy to remove this from the Image interface, I think that's a good change.

Isn't there a parent/child relationship that is established by the history?

I'm not really clear how this becomes AddHistory() though. In addition to history, the parent image is used for:

img.RootFS

img.OSFeatures / img.OSVersion

OSFeature/Version doesn't seem to be used for much. Do you think we could drop those as well?

Image.RootFS would have to be exposed somewhere if we don't pass in both images to this new function.

AddHistory wasn't very clear name. I meant a function that appends to both history and rootfs array of the base image config. img.OSFeatures/OSVersion come from the base input image that is passed in. It is also missing Config/ContainerConfig that could be either added by this function on set manually by the caller as they carry no extra logic other than just setting a propery on a struct.

tonistiigi · 2017-06-01T21:03:32Z

builder/dockerfile/mockbackend_test.go

tonistiigi · 2017-06-01T21:08:43Z

daemon/build.go

This should probably return releasableLayer. Otherwise it complicated the reference tracking as caller will need to get an image with this layer to release the current layer. You should not need to call the imagestore between steps in the future.

Ok, I thought layer.DiffID was probably a bad return value, but wanted to keep it as simple as possible for now.

How should the DiffID of the new layer be exposed? Should releaseableLayer has a method DiffID() ?

Should releaseableLayer has a method DiffID()

Yes

tonistiigi · 2017-06-01T21:09:33Z

daemon/build.go

Do the empty layer detection in here.

I'm not really sure what that looks like. The empty layer check is layer.IsEmpty() ? How can we do that check here and use the value in the new image?

The point was to check if the layer was empty in here and discard it if it was. Then you do not need to access the layer package when you are adding to the diffID array in CreateImage. Empty commit means that rootfs part of the image config does not require updates.

tonistiigi · 2017-06-01T21:17:12Z

builder/dockerfile/internals.go

line 102: should avoid creating a container

Oops, good catch. I think I resolved a merge conflict incorrectly

tonistiigi · 2017-06-05T21:39:18Z

image/image.go

tonistiigi · 2017-06-07T22:17:07Z

LGTM, with cleaning up the dependency graph in a follow-up.

Add CreateImage() to the daemon Refactor daemon.Comit() and expose a Image.NewChild() Update copy to use IDMappings. Signed-off-by: Daniel Nephin <[email protected]>

Signed-off-by: Daniel Nephin <[email protected]>

Commit the rwLayer to get the correct DiffID Refacator copy in thebuilder move more code into exportImage cleanup some windows tests Release the newly commited layer. Set the imageID on the buildStage after exporting a new image. Move archiver to BuildManager. Have ReleaseableLayer.Commit return a layer and store the Image from exportImage in the local imageSources cache Remove NewChild from image interface. Signed-off-by: Daniel Nephin <[email protected]>

dnephin · 2017-06-08T19:09:48Z

rebased, and squashed down to 3 commits

dnephin · 2017-06-19T19:16:00Z

This need one more LGTM, PTAL

vdemeester

LGTM 🐮

GordonTheTurtle added the status/0-triage label May 31, 2017

dnephin added status/1-design-review and removed status/0-triage labels May 31, 2017

dnephin force-pushed the refactor-builder-remove-copy-on-build branch 3 times, most recently from 0219fe7 to 20688cf Compare May 31, 2017 17:42

tonistiigi reviewed Jun 1, 2017

View reviewed changes

tonistiigi added status/2-code-review and removed status/1-design-review labels Jun 1, 2017

dnephin force-pushed the refactor-builder-remove-copy-on-build branch 6 times, most recently from 0c30c9c to 23f4222 Compare June 5, 2017 18:15

dnephin added the rebuild/z label Jun 5, 2017

GordonTheTurtle removed the rebuild/z label Jun 5, 2017

tonistiigi reviewed Jun 5, 2017

View reviewed changes

image/image.go Outdated

Copy link

Member

tonistiigi Jun 5, 2017

Choose a reason for hiding this comment

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

== ""

dnephin force-pushed the refactor-builder-remove-copy-on-build branch from b81495b to 23f4222 Compare June 5, 2017 22:06

tonistiigi added the area/builder Build label Jun 6, 2017

dnephin force-pushed the refactor-builder-remove-copy-on-build branch 2 times, most recently from aeb87d2 to 5aaeeaf Compare June 7, 2017 15:46

dnephin added 3 commits June 8, 2017 15:06

Remove CopyOnBuild from the daemon.

bd5f92d

Add CreateImage() to the daemon Refactor daemon.Comit() and expose a Image.NewChild() Update copy to use IDMappings. Signed-off-by: Daniel Nephin <[email protected]>

Rename LazyContext to LazySource.

ecd44d2

Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the refactor-builder-remove-copy-on-build branch from 5aaeeaf to 5136096 Compare June 8, 2017 19:07

tonistiigi mentioned this pull request Jun 8, 2017

builder: clean up builder dependancy interface #32904

Closed

8 tasks

dnephin requested a review from vdemeester June 8, 2017 19:09

dnephin requested a review from cpuguy83 June 8, 2017 19:09

dnephin added the rebuild/windowsRS1 label Jun 12, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 12, 2017

GordonTheTurtle assigned tonistiigi Jun 15, 2017

vdemeester approved these changes Jun 20, 2017

View reviewed changes

vdemeester merged commit 99c72eb into moby:master Jun 20, 2017

lowenna mentioned this pull request Jun 20, 2017

LCOW: Support most operations excluding remote filesystem #33241

Merged

dnephin deleted the refactor-builder-remove-copy-on-build branch June 29, 2017 20:45

dnephin mentioned this pull request Jun 29, 2017

COPY can add files to an existing VOLUME but RUN can't #33842

Open

tonistiigi mentioned this pull request Jul 6, 2017

This multistage Dockerfile almost always results in layer does not exist error #33974

Closed

This was referenced Oct 3, 2018

Problem with "deep" multistage builds: layer does not exist error #37340

Closed

COPY fail in multistage build: layer does not exist #37965

Open

Conversation

dnephin commented May 31, 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!

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!

tonistiigi Jun 1, 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!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Jun 7, 2017

Uh oh!

dnephin commented Jun 8, 2017

Uh oh!

dnephin commented Jun 19, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnephin commented May 31, 2017 •

edited

Loading

tonistiigi Jun 1, 2017 •

edited

Loading