GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Experimental BuildKit support by tonistiigi · Pull Request #37151 · moby/moby Conversation
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var _ snapshot.SnapshotterBase = &snapshotter{} | ||
|
|
||
| // NewSnapshotter creates a new snapshotter | ||
| func NewSnapshotter(opt Opt) (snapshot.SnapshotterBase, error) { |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,179 @@ | |||
| package containerimage | |||
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| BuilderV1 BuilderVersion = "1" | ||
| // BuilderBuildKit is builder based on moby/buildkit project | ||
| BuilderBuildKit = "2" | ||
| ) |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| SessionID string | ||
| Platform string | ||
| Version BuilderVersion | ||
| BuildID string |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Could you add godoc?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Codecov Report
@@ Coverage Diff @@ ## master #37151 +/- ## ========================================= Coverage ? 35.33% ========================================= Files ? 609 Lines ? 45000 Branches ? 0 ========================================= Hits ? 15901 Misses ? 26947 Partials ? 2152 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Design LGTM |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
3a6c7d4 to
71b7cd5
Compare
4937e74 to
b00dc69
Compare
There was a problem 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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
removed
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the regression is fixed now.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@AkihiroSuda yes, regression was on docker CLI. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦁
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
CI for powerpc and s390x are known to have issues /cc @andrewhsu @seemethere. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This has broken master on Windows: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@jhowardmsft Thanks. Sorry about that. Windows CI was green :( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@tiborvass Yes, the CI servers have (and need) git installed :/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| "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" |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.