builder: clean up builder dependancy interface

[tonistiigi]

The builder currently uses an API that was designed more around the existing functions in daemon package.

The main problems with this interface are:

• It is bloated, contains 17 functions that are very specific to docker.
• It is not isolated. The builder uses image and containers by IDs directly it means that it conflicts with other API users. For example deleting an image or container while the builder is running would cause an undefined behavior. Deleting a container while the copy operation is running even causes EBUSY and leaks data.
• It is inefficient, creating containers and mounting filesystems for every operation, while many commands like ENV just change a single value in a configuration struct.
• It can't be used for zero-copy commit because docker commit requires duplicating data.

The proposed new interface would be:

// Backend abstracts a dependency interface for the builder.
type Backend interface {
    // GetImage returns an image config and rootFS. Used by `FROM` and `--from`
    GetImage(ctx context.Context, refOrID string, opt ImageGetOpt) (config []byte, l releaseableLayer, error)

    // Execute a process in a sandbox environment. Used by `RUN`
    Exec(ctx context.Context, config types.RunConfig, layer releaseableLayer, opt_stdio stdio) (releaseableLayer, error)

    // Register image in imagestore
    Export(ctx context.Context, config []byte, legacyParentID string) (id string, err error)

    // Create a debug container. This is called when the build fails instead of releasing the layer.
    Debug(ctx context.Context, config types.RunConfig, layer releaseableLayer) (string, error)
}

type releaseableLayer interface {
  Release() error
  Mount() (string, error)
  Unmount() (string, error)
}

type ImageGetOpt struct {
  ForcePull bool
  ConfigOnly bool
  Auth map[]
}

releaseableLayer maps directly to the layer.Layer. Builder holds a reference to a layer and snapshot of image config until it runs. That makes sure that if images are deleted it doesn't affect the builder and everything would be cleaned up after the builder has finished.

Export is separated from commit so no hacks are needed to make sure that config of a container is in a correct state for committing. COPY/ADD would be implemented directly with layer access.

This is a big change and would need to be split up into multiple PRs. I propose following intermediate steps(open for discussion of course):

• General refactoring in builder to split different features and make further work easier(@dnephin is working on this)
• Implement GetImage and replace current pull methods (PR open [Builder] Expose GetImage interface for builder #33054)
• Combine Create/Attach/Start/Wait into same function, don't change Commit logic yet. (PR open [Builder] Refactor builder probe cache and container backend #33061)
• Implement Export, don’t try to integrate yet (@dnephin [Builder] Move file coping from the daemon to the builder #33454)
• Temporarily use GetImage to access image config committed with Commit so that Export and Commit can be used in parallel (@dnephin [Builder] Move file coping from the daemon to the builder #33454)
• Expose releasableLayer into builder. Remove CopyOnBuild, commit with Export (@dnephin [Builder] Move file coping from the daemon to the builder #33454)
• Change commands that update metadata to use Export
• Implement Exec/Debug and remove current Commit functions

@dnephin @vdemeester @duglin @AkihiroSuda

[duglin]

@tonistiigi I wonder if it wouldn't make more sense to look at this slightly differently. Rather than continuing to try to have specialized builder APIs, what if we extended the normal engine APIs such that you could do all builder actions via normal docker APIs/cli commands? Then the build just becomes a user of the normal APIs. It might require some new APIs, or even a new way to achieve the same net result from a "docker build" perspective, but I think in the long run it would make a much cleaner design, and it'll be easier to maintain. Not to mention, it makes it easier for alternative builders to be created.

[tonistiigi]

@duglin Each component should do what it is designed to do and define the minimum requirements it needs for other components. By defining current builder subfeatures in remote API we just make them a standard forever and freeze development for better builders. It doesn't provide easier alternatives but just defines that you can only build one type of builder.

That being said, I don't see why the interface proposed couldn't be implemented with the current remote API. Having safe layer reference can either be ignored(like it is today) or implemented with docker save/load endpoints.

[dnephin]

Then the build just becomes a user of the normal APIs.

Yes, I think we should be able to accomplish that with the proposal.

I expect what we'll get initially is something like this:

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

The Backend interface described above will be provided by the "builder adaptor", which will be a thin layer between the general purpose APIs and the builder.

That gives us time to validate the general purpose APIs with other consumers of those APIs while still having a clean API for the builder. Eventually the adaptor may disappear, or it might stick around with some minimal functionality that doesn't make sense in an image or container component, but can be re-used by any builder that requires that same functionality.

[AkihiroSuda]

SGTM (Debug would be really useful)

How does this proposal relate to BuildKit proposal (#32550 (comment)) and monolith split #32872 ?

[tonistiigi]

@AkihiroSuda This is a more short term to fix the current quality issues while Buildkit is for providing customization, extendability and alternative frontends. They should converge in the end. You can already see similarities though, GetImage -> Sources, Exec() -> Worker , Export() -> Exporter, releasableLayer ->Snapshot.

[tonistiigi]

superseded by buildkit integration

[Talhasaleem110]

The builder currently uses an API that was designed more around the existing functions in daemon package.

The main problems with this interface are:

• It is bloated, contains 17 functions that are very specific to docker.
• It is not isolated. The builder uses image and containers by IDs directly it means that it conflicts with other API users. For example deleting an image or container while the builder is running would cause an undefined behavior. Deleting a container while the copy operation is running even causes EBUSY and leaks data.
• It is inefficient, creating containers and mounting filesystems for every operation, while many commands like ENV just change a single value in a configuration struct.
• It can't be used for zero-copy commit because docker commit requires duplicating data.

The proposed new interface would be:

// Backend abstracts a dependency interface for the builder.
type Backend interface {
    // GetImage returns an image config and rootFS. Used by `FROM` and `--from`
    GetImage(ctx context.Context, refOrID string, opt ImageGetOpt) (config []byte, l releaseableLayer, error)

    // Execute a process in a sandbox environment. Used by `RUN`
    Exec(ctx context.Context, config types.RunConfig, layer releaseableLayer, opt_stdio stdio) (releaseableLayer, error)

    // Register image in imagestore
    Export(ctx context.Context, config []byte, legacyParentID string) (id string, err error)

    // Create a debug container. This is called when the build fails instead of releasing the layer.
    Debug(ctx context.Context, config types.RunConfig, layer releaseableLayer) (string, error)
}

type releaseableLayer interface {
  Release() error
  Mount() (string, error)
  Unmount() (string, error)
}

type ImageGetOpt struct {
  ForcePull bool
  ConfigOnly bool
  Auth map[]
}

releaseableLayer maps directly to the layer.Layer. Builder holds a reference to a layer and snapshot of image config until it runs. That makes sure that if images are deleted it doesn't affect the builder and everything would be cleaned up after the builder has finished.

Export is separated from commit so no hacks are needed to make sure that config of a container is in a correct state for committing. COPY/ADD would be implemented directly with layer access.

This is a big change and would need to be split up into multiple PRs. I propose following intermediate steps(open for discussion of course):

• General refactoring in builder to split different features and make further work easier(@dnephin is working on this)
• Implement GetImage and replace current pull methods (PR open [Builder] Expose GetImage interface for builder #33054)
• Combine Create/Attach/Start/Wait into same function, don't change Commit logic yet. (PR open [Builder] Refactor builder probe cache and container backend #33061)
• Implement Export, don’t try to integrate yet (@dnephin [Builder] Move file coping from the daemon to the builder #33454)
• Temporarily use GetImage to access image config committed with Commit so that Export and Commit can be used in parallel (@dnephin [Builder] Move file coping from the daemon to the builder #33454)
• Expose releasableLayer into builder. Remove CopyOnBuild, commit with Export (@dnephin [Builder] Move file coping from the daemon to the builder #33454)
• Change commands that update metadata to use Export
• Implement Exec/Debug and remove current Commit functions

@dnephin @vdemeester @duglin @AkihiroSuda