[Builder] Refactor builder probe cache and container backend by dnephin · Pull Request #33061 · moby/moby

dnephin · 2017-05-05T22:28:32Z

Part of the first task in #32904

Remove tmpContainers from Builder and 6 container methods from builder.Backend. Replaced with ContainerBackend which has 3 methods.

Remove cacheBusted and imageCache from Builder, replaced with an imageProber which exposes the cache to Builder.

AkihiroSuda · 2017-05-06T03:15:17Z

Is this planned to be the base of buildkit?
Or buildkit will be written from scratch?

dnephin · 2017-05-07T00:52:16Z

There are definitely pieces in docker build that could be used as the foundation for buildkit. I don't see much value in starting from scratch if there are pieces we can reuse.

tonistiigi · 2017-05-11T23:09:27Z

builder/dockerfile/builder.go

I think these should be passed with Run, there is no constraint that an exec output needs to end up in a central builder output.

Makes sense, I can pass these as run opts.

tonistiigi · 2017-05-11T23:10:35Z

builder/dockerfile/containerbackend.go

Should move implementation in this file out of /builder. To daemon or a subpkg of daemon.

There are still a lot of builder specific pieces in here. Logging is a big one.

I understand you don't consider this part of the builder, but I don't think adding to daemon is appropriate either. I see this as part of the adapter in this model.

------------ -------------------- -------------- | builder | --> | builder adaptor | ---> | image API | ------------ -------------------- | -------------- | ----------------- `-> | Container API | -----------------

(from #32904 (comment))

How about I move it out of dockerfile into something like builder/containeradaptor ?

Builder doesn't need to depend on one central dependency(builder adaptor). It can just talk with "image API" and "container API" directly. In #32904 the "container API" is Exec() and Debug(). There is nothing builder specific in these methods so it is wrong to call it "builder *". That being said, there is no requirement that "container API" needs to be daemon.Daemon instance. I'd be happy if you can move it anywhere outside of builder/ dir.

For the logging. The Run() method should just return the context cancellation error.

I meant the fmt.Fprintf() stuff, but I think it's fine if it's in api/server/backend/build

dnephin · 2017-05-17T00:33:58Z

Those issues should be fixed.

Note that containerBackend does have state that can't be applied to different builds. If a build runs without --rm or --force-rm the tmpContainer still need to be cleared.

tonistiigi · 2017-05-17T00:31:16Z

api/server/backend/build/containerbackend.go

Let's avoid messages like this in here. Just return ctx.Err(), if the builder wants to detect this error as ctx. Canceled it can do that and print a message.

tonistiigi · 2017-05-17T18:52:31Z

api/server/backend/build/containerbackend.go

Holding tmpContainers in here don't seem correct. That seems to be the only reason why these loggers are passed around in rm functions. It should remain part of builder and backend exposes removal by ID without loggers. If it is really simpler like this I'd be ok with it as the whole tmpContainers logic will go away with Exec/Debug. In that case add TODO comments.

Yes, the entire purpose of this object is to manage the tmpContainers. If we remove them then we are back to the old interface.

I agree this is an incremental change which gets us closer to the final interface.

This was original part of the builder and you asked me to move it out! I can add a TODO on the struct explaining why it exists.

tonistiigi · 2017-05-17T19:06:10Z

api/server/backend/build/backend.go

This callback logic appears to exists because of the tmpContainers as well. I don't think there is a need for these objects to be stateful. It is dangerous to make such assumptions as 2 build jobs should be able to share each other's resources. ContainerBackend is a dependency on builder, not on the build definition.

Yes, this callback exists because each build should have it's own containerBackend.

I don't think there is a need for these objects to be stateful

There is a need for some object to be stateful to manage temporary contaienrs. This object is current the containerBackend, so it must be stateful.

It is dangerous to make such assumptions as 2 build jobs should be able to share each other's resources.

I think the problem is that you asked me to move this out of the builder. Previously this made a lot more sense because it was part of the builder and not a dependency.

dnephin · 2017-05-19T14:20:02Z

This is rebased, and green again.

I've moved the containerManager back to the builder since it is still handling responsibilities of the builder. When we have Exec()/Export() on the backend we should be able to modify the containerManager to use those, with almost no changes to Builder (the struct).

tonistiigi

I went through this again, and unfortunately, I'm still confused how this helps. We already discussed that this is not 3rd task of #32904 but even when ignoring that I have some concerns.

First, I don't have any issues with the imageProber part.

For the exec part, it exposes new public interface ContainerBackend with 3 methods but there is no way for the user to ever provide this interface to the builder. Everything that uses it is private to the package.

Then it introduces a concept called containerManager. I don't see why a build operation should depend on a "manager" with state and why anyone would need to implement this state handling if they want to plug custom features in the builder. Keeping track of intermediate containers is an internal feature of builder(and one that can be removed completely). If this manager concept is internal then it should not be exposed as a build option.

The dependency interface for the builder does not change with this PR, so it seems just adding to the confusion that BuildManager now takes multiple inputs. It is good to split into smaller interfaces so parts of the code can have smaller dependency but with the current code the input interface for builder should just be a combination of them as nothing has actually changed.

So my suggestions:

Remove ContainerBackend interface
Make ContainerManager completely internal helper(eg imageSources today)
Comment on containerManager that this is supposed to go away to avoid anyone adding features to it.
/s/ContainerComponent/ExecBackend that extends the Backend (same way as ImageBackend for example)
Remove changes under api/dockerd.

dnephin · 2017-05-23T18:39:13Z

Ok, I think that is a fair compromise. I will make those changes.

Extract a common function for builder.createContainer Extract imageCache for doing cache probes Removes the cacheBuested field from Builder Create a new containerManager class which reduces the interface between the builder and managing containers to 3 functions (from 6) Signed-off-by: Daniel Nephin <[email protected]>

dnephin · 2017-05-23T20:55:14Z

I've made those changes, and the build still looks to be green

tonistiigi

Thanks @dnephin

LGTM

vdemeester

LGTM 🦁

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

dnephin force-pushed the refactor-builder-probe-cache branch from 2127d05 to 9b8f017 Compare May 5, 2017 22:32

tonistiigi mentioned this pull request May 7, 2017

builder: clean up builder dependancy interface #32904

Closed

8 tasks

vdemeester added status/2-code-review and removed status/0-triage labels May 7, 2017

dnephin force-pushed the refactor-builder-probe-cache branch 2 times, most recently from eaf43ec to 15d35dd Compare May 8, 2017 21:39

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

dnephin force-pushed the refactor-builder-probe-cache branch 2 times, most recently from 1188c97 to 64279a0 Compare May 9, 2017 19:05

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

dnephin force-pushed the refactor-builder-probe-cache branch from 64279a0 to 4ea6ca5 Compare May 9, 2017 22:18

dnephin added the rebuild/experimental label May 10, 2017

GordonTheTurtle removed the rebuild/experimental label May 10, 2017

dnephin force-pushed the refactor-builder-probe-cache branch 4 times, most recently from 4f6c97b to e7e4138 Compare May 10, 2017 22:12

dnephin added the rebuild/powerpc label May 11, 2017

GordonTheTurtle removed the rebuild/powerpc label May 11, 2017

dnephin force-pushed the refactor-builder-probe-cache branch from e7e4138 to 201f762 Compare May 11, 2017 15:04

dnephin requested review from AkihiroSuda, cpuguy83 and tonistiigi May 11, 2017 15:05

tonistiigi reviewed May 11, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-probe-cache branch 3 times, most recently from d4b09ea to cb2283b Compare May 16, 2017 21:02

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 16, 2017

dnephin force-pushed the refactor-builder-probe-cache branch from 5e53d71 to f0948af Compare May 16, 2017 21:52

dnephin added the rebuild/windowsRS1 label May 17, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label May 17, 2017

dnephin force-pushed the refactor-builder-probe-cache branch from f0948af to 0261586 Compare May 17, 2017 14:12

dnephin added the rebuild/windowsRS1 label May 17, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label May 17, 2017

tonistiigi reviewed May 17, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-probe-cache branch 2 times, most recently from 6ccaba2 to 3f18cf8 Compare May 18, 2017 16:41

dnephin requested a review from vdemeester May 19, 2017 14:18

dnephin force-pushed the refactor-builder-probe-cache branch 2 times, most recently from 6a6d55a to f7f829b Compare May 19, 2017 14:35

dnephin added the rebuild/powerpc label May 19, 2017

GordonTheTurtle removed the rebuild/powerpc label May 19, 2017

dnephin added the rebuild/windowsRS1 label May 19, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label May 19, 2017

GordonTheTurtle assigned coolljt0725 May 20, 2017

tonistiigi reviewed May 23, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-probe-cache branch from f7f829b to 19f3b07 Compare May 23, 2017 19:13

tonistiigi approved these changes May 23, 2017

View reviewed changes

vdemeester approved these changes May 25, 2017

View reviewed changes

vdemeester merged commit 8d20641 into moby:master May 25, 2017

GordonTheTurtle added this to the 17.06.0 milestone May 25, 2017

dnephin deleted the refactor-builder-probe-cache branch May 25, 2017 17:13

Conversation

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

Uh oh!

Uh oh!

AkihiroSuda commented May 6, 2017

Uh oh!

dnephin commented May 7, 2017

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 17, 2017

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 19, 2017

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin commented May 23, 2017

Uh oh!

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

Uh oh!

6 participants

dnephin commented May 5, 2017 •

edited

Loading