-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
vendor buildkit v0.10.0 #43239
vendor buildkit v0.10.0 #43239
Conversation
d1e159c
to
013bbff
Compare
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.
some quick comments (my browser doesn't like reviewing the diff, so let me post some of them already; will have a closer look later)
) | ||
|
||
const ( | ||
keyImageName = "name" | ||
keyImageName = "name" |
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.
Are these options unique to the integration in moby, or would conversion / definition of these be something we should move to buildkit at some point?
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.
Yeah while working on this PR I thought about moving the moby exporter to https://github.com/moby/buildkit/tree/master/exporter. Maybe we could expose some bits on BuildKit in the meantime.
449f779
to
25bb0ff
Compare
Ok we have the same issues with swarm tests: https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/11/pipeline/346#step-349-log-1332 like #42983 (comment)
Will add a commit using my fork of swarmkit (even with failed raft test) to see how it goes. |
This comment was marked as outdated.
This comment was marked as outdated.
This needs the additional patches for MergeOp/DiffOp from @sipsma . If we want to test sooner and reduce patch size we can first vendor the commit before MergeOp was merged. |
Commits needed to support changes made during mergeop: Same but for DiffOp: The changes needed to get the hardlink optimization working are here but are rough, need some cleanup: Let me know if I can help out cleaning the hardlink optimization commit up. |
Thanks @sipsma! |
1739240
to
d408439
Compare
86baed0
to
7eb4ef6
Compare
d6ed26e
to
a3a3b75
Compare
Seems like a flaky test on win-2022-c8d: https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/PR-43239/45/pipeline#step-271-log-1702 Are you aware of this one @thaJeztah?
|
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
a3a3b75
to
e4193ba
Compare
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.
Make sure the leftover mergeop/hardlink issue is tracked in moby with 22.x milestone so we can get this in for early testing.
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
e4193ba
to
ff35785
Compare
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 - left some notes about things we should look into in follow-ups
if description := layer.GetDescription(); description != "" { | ||
meta.description = description | ||
} else { | ||
meta.description = "created by buildkit" // shouldn't be shown but don't fail build |
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.
Was wondering if we use this information anywhere (and if so, if this at some point should include the buildkit version)
|
||
ulimits, err := toBuildkitUlimits(opt.Options.Ulimits) | ||
if err != nil { | ||
return nil, err |
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.
These are also the ones we need to start looking at to return a correct error-type (InvalidParam()); per my other comment; there's a lot of those already, so let's make an effort to clean that up in some sweeps.
@@ -558,6 +570,18 @@ func toBuildkitExtraHosts(inp []string) (string, error) { | |||
return strings.Join(hosts, ","), nil | |||
} | |||
|
|||
// toBuildkitUlimits converts ulimits from docker type=soft:hard format to buildkit's csv format | |||
func toBuildkitUlimits(inp []*units.Ulimit) (string, 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.
oh... hmm... so actually looks like this never return an error? ok to fix later (somewhat surprised there's no longer complaining)
if err != nil { | ||
return nil, errors.Wrap(err, "failed to marshal cache") |
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.
Somewhat curious why you removed the wrapping here (seems like the additional context could be useful); not a blocker, but add it to the list of things to look at
tracked in #43415 |
Let's get this one in! |
closes #42968
closes #42983
closes #40023
fixes #42261
moby/buildkit@9f254e1...8d45bd6
cc @thaJeztah @tonistiigi
Signed-off-by: CrazyMax crazy-max@users.noreply.github.com