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
Update to BuildKit 0.12 #45966
Update to BuildKit 0.12 #45966
Conversation
acfc1d9
to
e7f85db
Compare
a7ff5e1
to
7886e0f
Compare
7886e0f
to
cca7322
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
cca7322
to
040037d
Compare
040037d
to
7b9d489
Compare
This comment was marked as outdated.
This comment was marked as outdated.
exporterAttrs[string(exptypes.OptKeyName)] = strings.Join(reposAndTags, ",") | ||
exporterAttrs[string(exptypes.OptKeyUnpack)] = "true" |
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.
For these I was wondering if it's possible to do the reverse (cast to ImageExporterOptKey
) ? Haven't looked at this code locally though, just through GitHub's ui
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 type in the protobuf is string
; I don't know that we can control the codegen enough to "do the right thing" here, and this is the pattern BK uses. Still, it's really clunky, and if we can invert it I'm totally in favor.
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.
Ah... gotcha.. didn't realise we're dealing with protobuf here. Was mostly hoping "strong(ish) types" to help discoverability.
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, this is just a newtype to ease discoverability, but it doesn't seem to be totally ergonomic even on the BK side due to protobufs:
https://github.com/moby/buildkit/blob/f53c1752113e1f9c0fbcc1d5c8e18a9b13452afc/control/control.go#L334-L342
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.
cc @crazy-max @jedevc if this is something we can change
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.
exporter attributes is map<string, string>
in the proto def: https://github.com/moby/buildkit/blob/f53c1752113e1f9c0fbcc1d5c8e18a9b13452afc/api/services/control/control.proto#L64
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.
Maybe map<string, google.protobuf.Any>
would have work but the contract is already there.
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.
Right, my thought (and specific inexperience) is that maybe we can change the protobuf definition to use the newtype, and existing clients with the old generated code will continue to interoperate as it's just a type alias to string. That breaks the Go API a little; but in theory the protobuf is the stable component and the generated Go code can change (ala containerd v1.7).
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.
It turns out this is not possible as we have multiple types for the keys (ExporterOptKey
for all exporters, ImageExporterOptKey
for the image exporter).
It looks like this is a wart we'll have to live with; in the future when/if we remove Gogo, the SDK will have to change a fair bit anyway; at that point defining some conversion helper (maybe with generics) ought to be better.
1c661c8
to
21288ba
Compare
b1572f3
to
5e9a6ee
Compare
The current executor is only tested on Linux, so let's be honest about that. Stubbing this correctly helps avoid incorrectly trying to call into Linux-only code in e.g. libnetwork. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
The DeepEqual ignore required in the daemon tests is a bit ugly, but it works given the new protoc output. We also have to ignore lints related to schema1 deprecations; these do not apply as we must continue to support this schema version. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Introduced in containerd/containerd@dd3eedf Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
The following changes were required: * integration/build: progressui's signature changed in moby/buildkit@6b8fbed * builder-next: flightcontrol.Group has become a generic type in moby/buildkit@8ffc03b * builder-next/executor: add github.com/moby/buildkit/executor/resources types, necessitated by moby/buildkit@6e87e4b * builder-next: stub util/network/Namespace.Sample(), necessitated by moby/buildkit@963f161 Co-authored-by: CrazyMax <crazy-max@users.noreply.github.com> Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
With BuildKit 0.12, some existing types are now required to be wrapped by new types: * containerd's LeaseManager and ContentStore have to be a (namespace-aware) BuildKit type since moby/buildkit@f044e0a * BuildKit's solver.CacheManager is used instead of bboltstorage.CacheKeyStorage since moby/buildkit@2b30693 * The MaxAge config field is a bkconfig.Duration since moby/buildkit@e06c962 Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
SourcePolicy was accounted for in moby/buildkit@330cf7a TODO: replace applySourcePolicies with BuildKit's implementation, which is currently unexported. Co-authored-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Introduced years ago in moby/buildkit@6644e1b Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Introduced in moby/buildkit@4fc2d7b Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Now that this is a generic, we can define a struct type at the package level, and remove the casting logic necessary when we had to use interface{}. Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
5e9a6ee
to
57eac69
Compare
This exposes `ACTIONS_RUNTIME_TOKEN` and `ACTIONS_CACHE_URL`, which are used to skip cache exporter tests, when combined with moby/buildkit@a8789cb Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com> Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
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
Skipping digest-related tests is no longer necessary after containerd/containerd@4065831 Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
57eac69
to
d5b067e
Compare
"sync" | ||
|
||
"github.com/pkg/errors" | ||
) | ||
|
||
type Result[T any] struct { | ||
type Result[T comparable] struct { |
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.
# github.com/moby/buildkit/frontend/gateway/client
vendor/github.com/moby/buildkit/frontend/gateway/client/client.go:18:29: Reference does not implement comparable
note: module requires Go 1.20
If we want to keep this change, we should update vendor.mod
to go 1.20
, right?
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.
Ah, good catch! I should have tested with an older Go compiler.
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.
I guess that's #46069 👀
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, nope, that doesn't touch the go
line 😅
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.
Ah, yes, I decided not to force go1.21 in that PR (as most of the code definitely would still work on older versions); perhaps we still should? At least setting the go.mod
to go1.20 could be reasonable I guess. (with go1.19 being EOL); at least for the master / v25.0-dev branch.
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.
I was planning on a follow-up PR (as I have some plans for the go
line after Go 1.21), but I think that adding a go 1.20
line to your PR makes some sense 👍
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.
I'll make it a separate commit, as we don't need it for the 24.0 (and other branches)
I guess in that case we might as well make it a separate PR; want to open one?
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.
- What I did
Update containerd (library) to v1.7 and BuildKit to v0.12.
Make changes to the code to adjust to the new dependency versions, and fix some TODOs that were waiting on these updates. Leave some new TODOs as well, mostly related to the builder-next code and having to re-implement some things instead of re-using them from BuildKit.
- How I did it
With help from my IDE, and with assistance from others (@tonistiigi, @crazy-max, @rumpl) where I ran out of skill and/or time.
- How to verify it
CI ✅
- Description for the changelog
v0.12.2
.v1.7.6
.- A picture of a cute animal (not mandatory but encouraged)