[Builder] Expose GetImage interface for builder by dnephin · Pull Request #33054 · moby/moby

dnephin · 2017-05-05T17:52:06Z

Implement the second task in #32904

This PR removes the previous 3 Image functions from the builder.Backend interface and replaces them with a single function. Also refactors the interfaces between imageMount, imageContects, and dispatchers.from

tonistiigi · 2017-05-05T18:03:37Z

Design LGTM

dnephin · 2017-05-05T22:22:40Z

I think I fixed the test failures. The problem was that releaseableLayer was being returned with a closer over the original image/imageID. This meant that updates to the base layer were not reflected in the layer.

The change was to add an imageID to ReleasableLayer.Mount(). This works, but it makes the interface a bit weird. Now the first call returns an object that really has nothing to do with the image that it returns, it's more of an uninitialized object that happens to have a reference to the daemon.

My concern is that it wouldn't really be possible to model this with a remote API. We might want to consider a separate MountImage() like we had better. Will this interface make more sense later? WDYT?

tonistiigi · 2017-05-05T22:50:15Z

As discussed privately the need for Mount() parameter should go away and releaseableLayer should be always backed by layer.Layer. Putting back to design-review until that is resolved.

dnephin · 2017-05-06T00:17:26Z

What do you mean by "backed by layer.Layer" ? There is a RWLayer underlying the releaseableLayer

tonistiigi · 2017-05-06T00:23:44Z

@dnephin Every instance of releasableLayer needs to be an instance of(or keep a reference to) layer.Layer that is the rootfs of the image returned. That makes sure that the data is protected until builder runs. When Mount() for that layer is called, RWLayer is created on top of that layer.Layer and mountpoint for that is made available. releasableLayer.Release() calls layerstore.Release(layer) in the implementation.

tonistiigi · 2017-05-08T18:57:30Z

daemon/build.go

roLayer still needs to be released. Might be simpler to add a ref to roLayer in releaseableLayer instead of defining these callback methods.

For extra safety, maybe makes sense to define a finalizer for this type.

Oops, right. Thanks.

I don't really want to expose the daemon , so I will still need some closures. They are not really callbacks.

I guess I could use the layerStore reference

tonistiigi · 2017-05-08T18:58:30Z

daemon/build.go

roLayer.ChainID is bit safer

dnephin · 2017-05-09T04:04:10Z

The build is green!

I believe I've addressed the issues with roLayer in releaseableLayer as well as the bug described in #33090

Ready for another design review

dnephin · 2017-05-09T04:06:31Z

daemon/build.go

When an image is created from scratch and doesn't include any copy/add this chainID is empty. I'll add a comment about this with the next round of changes after design review.

I guess ill also need to add a check to Mount() and provide a better error when roLayer is nil

Yes, Mount() seems unprotected atm. Other way would be return nil(or not call GetImage for scratch) as there is no point for keeping a reference for scratch.

GetImage() is never called with a scratch object. But if you do FROM someimage and that image doesn't have any COPY/ADD, then this ChainID() is still the empty string.

So we have to check for that condition here. We could either return a nop implementation of ReleasableLayer, but I decided against that because many of these nil checks are required anyway. So i thought it was be easier to just add a few more instead of creating another struct.

What about just returning nil. It could be better to detect this case here than to call Mount() and get the error. Also if we add Exec() then this takes layer as a parameter, so it would be better to detect this case before instead of passing the empty struct.

Oh ya, that is a lot better. The case where layer is nil is already handled.

I'll leave the nil checks in mount anyway for safety

tonistiigi

Just some comments, overall looks good

tonistiigi · 2017-05-09T19:15:07Z

builder/builder.go

If you don't want to call this GetImage then maybe ImageAndRootFS.

It can't be GetImage because it conflicts with another daemon method already called GetImage.

I called it Layer, since the second return is named ReleaseableLayer. Should we rename that object as well? I think they should match.

Or should I make this GetImageAndReleasableLayer() ?

Not really that important, we could also call it a snapshot and use the containerd terms.

Ok, I'll use GetImageAndReleasableLayer() for now. It's easy to rename later when we have the ability to use snapshots. I think it will be confusing to readers if we introduce the term snapshot right now.

tonistiigi · 2017-05-09T19:16:28Z

builder/dockerfile/dispatchers.go

imageMounter is bit confusing in here as regular from never mounts anything.

That's true, but the purpose of the struct is still to manage the unmounts required because of the API we are using. I don't know what else to call it

tonistiigi · 2017-05-09T20:11:31Z

builder/dockerfile/imagecontext.go

nit: what's the benefit of callback instead of using the interface dependency

The advantage is that we don't have to store all the opts that are only used in this one place. We only need them to construct the object. By storing just the function type that relationship is clear. If we were to store all the opts it wouldn't be clear.

tonistiigi · 2017-05-09T20:16:50Z

daemon/build.go

Yes, Mount() seems unprotected atm. Other way would be return nil(or not call GetImage for scratch) as there is no point for keeping a reference for scratch.

tonistiigi · 2017-05-09T23:04:06Z

builder/builder.go

Not really that important, we could also call it a snapshot and use the containerd terms.

tonistiigi · 2017-05-10T14:23:57Z

daemon/build.go

What about just returning nil. It could be better to detect this case here than to call Mount() and get the error. Also if we add Exec() then this takes layer as a parameter, so it would be better to detect this case before instead of passing the empty struct.

tonistiigi · 2017-05-10T14:33:51Z

builder/dockerfile/dispatchers.go

re: previous comment
It isn't really that it manages unmounts. It more managers keeping the layer references. Unmount currently also happens on release but that will probably be updated. imageRefs, images, imageSources, imageCache ?

I think we already have imageCache so I'll go with imageSources

dnephin · 2017-05-10T20:48:11Z

I've made those changes, and CI seems to be good except for some flaky tests

tonistiigi

LGTM

Removes 3 methods from the builder.Backend interface Remove the coupling between imageContexts, imageMounts and the builder. Signed-off-by: Daniel Nephin <[email protected]>

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

buildStages now tracks the imageID and runConfig for a build stage imageMounter tracks image mounts so they can released when the build ends. Signed-off-by: Daniel Nephin <[email protected]>

vdemeester

LGTM 🦁

tonistiigi mentioned this pull request May 5, 2017

builder: clean up builder dependancy interface #32904

Closed

8 tasks

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

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

tonistiigi added the area/builder Build label May 5, 2017

dnephin added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/1-design-review labels May 5, 2017

dnephin force-pushed the refactor-builder-named-contexts-interface branch from a5b6bd9 to 8c35320 Compare May 5, 2017 22:18

dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 5, 2017

dnephin force-pushed the refactor-builder-named-contexts-interface branch from 8c35320 to efa5d8d Compare May 5, 2017 22:24

dnephin mentioned this pull request May 5, 2017

[Builder] Refactor builder probe cache and container backend #33061

Merged

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

tonistiigi reviewed May 8, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-named-contexts-interface branch 2 times, most recently from 516249d to b4363ad Compare May 8, 2017 22:20

dnephin commented May 9, 2017

View reviewed changes

dnephin requested review from cpuguy83, tonistiigi and vdemeester May 9, 2017 14:40

tonistiigi reviewed May 9, 2017

View reviewed changes

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

dnephin force-pushed the refactor-builder-named-contexts-interface branch from 91b13fc to 73f1462 Compare May 9, 2017 22:17

dnephin added the rebuild/powerpc label May 10, 2017

GordonTheTurtle removed the rebuild/powerpc label May 10, 2017

tonistiigi reviewed May 10, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-named-contexts-interface branch 3 times, most recently from 5062290 to 1c16f09 Compare May 10, 2017 19:39

dnephin added the rebuild/powerpc label May 10, 2017

GordonTheTurtle removed the rebuild/powerpc label May 10, 2017

dnephin added the rebuild/janky label May 10, 2017

GordonTheTurtle removed the rebuild/janky label May 10, 2017

tonistiigi approved these changes May 10, 2017

View reviewed changes

dnephin added 3 commits May 10, 2017 17:58

Expose a smaller interface for the Builder retrieving images from daemon

b3bc7b2

Removes 3 methods from the builder.Backend interface Remove the coupling between imageContexts, imageMounts and the builder. Signed-off-by: Daniel Nephin <[email protected]>

Refactor interaction between dispatcher.from and dispatchState

ab3a037

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

Refactor imageContexts into two different structs.

6c28e8e

buildStages now tracks the imageID and runConfig for a build stage imageMounter tracks image mounts so they can released when the build ends. Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the refactor-builder-named-contexts-interface branch from 1c16f09 to 6c28e8e Compare May 10, 2017 22:00

dnephin added the rebuild/powerpc label May 11, 2017

GordonTheTurtle removed the rebuild/powerpc label May 11, 2017

vdemeester added the rebuild/powerpc label May 11, 2017

GordonTheTurtle removed the rebuild/powerpc label May 11, 2017

vdemeester approved these changes May 11, 2017

View reviewed changes

vdemeester added status/4-merge rebuild/powerpc and removed status/2-code-review labels May 11, 2017

GordonTheTurtle removed the rebuild/powerpc label May 11, 2017

dnephin merged commit 974cec9 into moby:master May 11, 2017

GordonTheTurtle added this to the 17.06.0 milestone May 11, 2017

dnephin deleted the refactor-builder-named-contexts-interface branch May 11, 2017 15:03

tonistiigi mentioned this pull request May 22, 2017

Add ReferenceCount for Image to solve image operation competition #33323

Closed

Conversation

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

Uh oh!

Uh oh!

tonistiigi commented May 5, 2017

Uh oh!

dnephin commented May 5, 2017

Uh oh!

tonistiigi commented May 5, 2017

Uh oh!

dnephin commented May 6, 2017

Uh oh!

tonistiigi commented May 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

tonistiigi 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!

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 commented May 10, 2017

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

dnephin commented May 5, 2017 •

edited

Loading

dnephin May 8, 2017 •

edited

Loading

dnephin commented May 9, 2017 •

edited

Loading