- 
                Notifications
    You must be signed in to change notification settings 
- Fork 18.8k
Experimental BuildKit support #37151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| var _ snapshot.SnapshotterBase = &snapshotter{} | ||
|  | ||
| // NewSnapshotter creates a new snapshotter | ||
| func NewSnapshotter(opt Opt) (snapshot.SnapshotterBase, error) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split this to a separate package that wraps Moby graphdriver as a containerd snapshotter?
cc @dmcgowan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the issue is that containerd mounts are stateless while some graphdrives can't produce stateless mounts. @dmcgowan has a plan for that for Moby integration that is somewhat similar to the buildkit/snapshot.Mountable abstraction.
| @@ -0,0 +1,179 @@ | |||
| package containerimage | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we dedupe writer.go with https://github.com/moby/buildkit/blob/master/exporter/containerimage/writer.go via some util pkg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I couldn't use it directly was that buildkit uses the blobsum-diffid pairs here while Moby doesn't yet have an understanding of managing layer blobs. But with a careful refactor, some parts of it can definitely be shared.
| BuilderV1 BuilderVersion = "1" | ||
| // BuilderBuildKit is builder based on moby/buildkit project | ||
| BuilderBuildKit = "2" | ||
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we just use strings rather than numbers? e.g. legacy and buildkit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AkihiroSuda I don't like the word legacy (which is what it was initially). With all the respect I have for buildkit, with time everything becomes legacy and eventually you'll have to differentiate between legacy legacy and legacy. This problem is solved with versions, so I believe it's a better tradeoff this way. Happy to hear your concerns though.
        
          
                api/types/client.go
              
                Outdated
          
        
      | SessionID string | ||
| Platform string | ||
| Version BuilderVersion | ||
| BuildID string | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add godoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On high level, definitely Design LGTM 😻
In this PR it seems it's not possible to choose a different frontend yet right ? if yes, is it something we plan to allow before going out of experimental ?
I see the gateway.v0 frontend is there so it seems it's already possible to use custom images as frontend (which is awesome 💓) but I'm not sure how to specify it from the API standpoint.
Also, should we open a PR in docker/cli to discuss the cli design in parallel ; and also easily have a docker binary built to make manual testing easier (without having to clone and build the cli)
| 
 Custom Dockerfile implementations can be used with a Dockerfile directive moby/buildkit#384 but generic frontends (or raw LLB) is not currently enabled in HTTP API. As this would be a new feature, it probably makes sense to do discuss/PR that separately. It may definitely happen in experimental is maintainers agree. 
 There's still some cleanup stuff that needs to happen there. You can expect a PR early next week. | 
| Codecov Report
 @@            Coverage Diff            @@
##             master   #37151   +/-   ##
=========================================
  Coverage          ?   35.33%           
=========================================
  Files             ?      609           
  Lines             ?    45000           
  Branches          ?        0           
=========================================
  Hits              ?    15901           
  Misses            ?    26947           
  Partials          ?     2152 | 
| Design LGTM | 
3a6c7d4    to
    71b7cd5      
    Compare
  
    4937e74    to
    b00dc69      
    Compare
  
            
          
                vendor.conf
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we can upstream? I didn't see a pull request; https://github.com/hashicorp/go-immutable-radix/pulls (looks like its this change; tonistiigi/go-immutable-radix@826af9c)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if they are interested and the project isn't very active. I can make a proper fork if needed (although we use this project for swarmkit as well). There is no functionality divergence, just an extra method that is only used by buildkit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could try if they accept the PR, then we'd be set (unless we think we'll be adding more features and a fork would help)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll make sure to do that and replace if it gets merged
        
          
                vendor.conf
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using UNLICENSE; looks like that could have some implications (but IANAL); https://softwareengineering.stackexchange.com/a/147120, docopt/docopt.rs#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the regression is fixed now.
| @AkihiroSuda yes, regression was on docker CLI. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦁
| CI for powerpc and s390x are known to have issues /cc @andrewhsu @seemethere. | 
| This has broken master on Windows:  | 
| @jhowardmsft Thanks. Sorry about that. Windows CI was green :( | 
| @tiborvass Yes, the CI servers have (and need) git installed :/ | 
| "github.com/docker/docker/api/types" | ||
| "github.com/docker/docker/api/types/backend" | ||
| "github.com/docker/docker/builder" | ||
| buildkit "github.com/docker/docker/builder/builder-next" | 
      
        
              This comment was marked as spam.
        
          
      
    
    This comment was marked as spam.
Sorry, something went wrong.
This PR adds experimental support for using moby/buildkit as a backend for
docker build. #32925 #34227The support is only enabled in the experimental daemon, and user needs to opt-in in docker/cli to start using it.
Docker CLI branch with BuildKit support enabled is in https://github.com/tonistiigi/docker-cli/tree/experimental-buildkit
In the API,
versionfield allows switching between different build backends.This enables lots of new capabilities, including parallel build steps and requests, skipping unused stages, efficient incremental builds, build context file detection, remote cache support, graceful cancellation, build cache issues, etc. It also enables using experimental Dockerfile features without requiring to update Moby by using the BuildKit frontend images.
The technical side of this integration is a temporary (and somewhat limited) solution as BuildKit is based on containerd storage and that has not been integrated into Moby yet. The current solution provides adapters to some components to provide compatibility between things like containerd snapshots and Moby layerstore/storage drivers or image store. When containerd storage support lands in Moby, all these adapters should be removed entirely, and BuildKit can use containerd directly. This temporary solution allows us to get feedback before we feel comfortable to move it out of experimental.
The tests will not currently use BuildKit in the PR as it requires opt-in and a new CLI binary. We have run the tests manually. Some of them need to be reworked for output change or a side-effect removal. We will continue addressing some of the test results that we are still investigating.
Known issues (some of them will not make it into 18.06 release):
system dfandsystem prunework)pulling schema1 images during buildautomatic path detection in dockerfile doesn't detect unused symlinks--pull(currently existing images in image store always take precedence)--iidfileis not enabled in CLI--cache-from.@tiborvass @AkihiroSuda
depends on #36895