vendor: github.com/containerd/containerd v1.7.0-rc.2 #44530
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
thaJeztah
commented
Nov 24, 2022
•
thaJeztah
commented
- https://github.com/containerd/containerd/compare/v1.6.12..v1.7.0-beta.0
- containerd/containerd@v1.7.0-beta.0...v1.7.0-beta.1
thaJeztah
force-pushed
the
containerd_1.7
branch
from
116c2a4
to
ca518d3
Compare
|
Some linting failures to fix; |
|
Hm... looks like that's in the vendor code? (perhaps we can work around it, but could also be it needs to be fixed in containerd?) |
thaJeztah
force-pushed
the
containerd_1.7
branch
2 times, most recently
from
6d34a86
to
55f08ef
Compare
|
thaJeztah
force-pushed
the
containerd_1.7
branch
2 times, most recently
from
2886b06
to
e0fc5eb
Compare
thaJeztah
commented
Nov 25, 2022
libcontainerd/remote/client.go
Outdated
Comment on lines
203
to
218
| opts := *c.v2runcoptions | ||
| opts.IoUid = uint32(uid) | ||
| opts.IoGid = uint32(gid) | ||
| info.Options = &opts | ||
| // libcontainerd/remote/client.go:204:13: copylocks: assignment copies lock value to opts: github.com/docker/docker/vendor/github.com/containerd/containerd/runtime/v2/runc/options.Options contains github.com/docker/docker/vendor/google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet) | ||
| // opts := *c.v2runcoptions | ||
| // ^ | ||
| info.Options = &v2runcoptions.Options{ | ||
| NoPivotRoot: c.v2runcoptions.GetNoPivotRoot(), | ||
| NoNewKeyring: c.v2runcoptions.GetNoNewKeyring(), | ||
| ShimCgroup: c.v2runcoptions.GetShimCgroup(), | ||
| IoUid: uint32(uid), | ||
| IoGid: uint32(gid), | ||
| BinaryName: c.v2runcoptions.GetBinaryName(), | ||
| Root: c.v2runcoptions.GetRoot(), | ||
| SystemdCgroup: c.v2runcoptions.GetSystemdCgroup(), | ||
| CriuImagePath: c.v2runcoptions.GetCriuImagePath(), | ||
| CriuWorkPath: c.v2runcoptions.GetCriuWorkPath(), | ||
| } |
There was a problem hiding this comment.
Does anyone have a good approach for these kind of things? So the issue is that these types embed a mutex (through the state (protoimpl.MessageState); the linter (correctly) flags the old code because we're copying the mutex. https://github.com/containerd/containerd/blob/v1.7.0-beta.0/runtime/v2/runc/options/oci.pb.go#L23-L31
type Options struct {
state protoimpl.MessageState
sizeCache protoimpl.SizeCache
unknownFields protoimpl.UnknownFields
// disable pivot root when creating a container
NoPivotRoot bool `protobuf:"varint,1,opt,name=no_pivot_root,json=noPivotRoot,proto3" json:"no_pivot_root,omitempty"`
// create a new keyring for the container
NoNewKeyring bool `protobuf:"varint,2,opt,name=no_new_keyring,json=noNewKeyring,proto3" json:"no_new_keyring,omitempty"`The reason we copy is that we want to have the existing options, but update some fields; So my naive approach is to construct a new option, and copy all the fields. While that works; it's brittle, as (e.g.) if a field is added or removed, this code will not copy those new fields.
I wish these types had a .Copy() method to get a copy of them (to prevent the mutex being copied), but perhaps there's a better approach for this.
Related discussion on StackOverflow; https://stackoverflow.com/a/64183968/1811501, which seems to indicate this is on purpose, and the MessageState has a pragma.DoNotCopy (and in this case pragma.DoNotCompare) to indicate it should not be copied; https://github.com/protocolbuffers/protobuf-go/blob/v1.28.1/internal/impl/message_reflect.go#L358-L364
type MessageState struct {
pragma.NoUnkeyedLiterals
pragma.DoNotCompare
pragma.DoNotCopySo, perhaps we're doing it wrong? Or should the type be changed?
There was a problem hiding this comment.
Maybe I'm looking at proto.Clone() or proto.Merge
There was a problem hiding this comment.
^^ updated to use proto.Clone()
thaJeztah
force-pushed
the
containerd_1.7
branch
from
e0fc5eb
to
703cd63
Compare
thaJeztah
force-pushed
the
containerd_1.7
branch
3 times, most recently
from
9e63ad5
to
94ee16f
Compare
thaJeztah
force-pushed
the
containerd_1.7
branch
from
94ee16f
to
0dd9837
Compare
thaJeztah
changed the title
thaJeztah
force-pushed
the
containerd_1.7
branch
from
0dd9837
to
581ed06
Compare
thaJeztah
force-pushed
the
containerd_1.7
branch
from
581ed06
to
3eedc19
Compare
|
Looks like changes may be needed in BuildKit to make it compatible with some of the newer OTEL dependencies from containerd, or maybe some dependencies weren't updated (but need to); |
|
Looking at
We're missing
Removed between v0.27.0 and v0.28.0; open-telemetry/opentelemetry-go@/metric/v0.27.0.../metric/v0.28.0
😂 yeah; removing actual code from your package, and implementing a new API is what I would expect "fix tests and examples" to be 😂 |
|
So, who specified the wrong versions? The problem here is that try to fix go.opentelemetry.io dependency hell docker ("indirect") and buildkit ("direct") specify go mod graph --modfile=vendor.mod | grep ' go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp'
github.com/docker/docker go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0
github.com/moby/buildkit@v0.11.4 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0containerd specified v0.34.0 for go mod graph --modfile=vendor.mod | grep ' go.opentelemetry.io/otel/metric@'
github.com/docker/docker go.opentelemetry.io/otel/metric@v0.34.0
github.com/containerd/containerd@v1.7.0-rc.2 go.opentelemetry.io/otel/metric@v0.34.0
github.com/moby/buildkit@v0.11.4 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc@v0.37.0 go.opentelemetry.io/otel/metric@v0.34.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/otel/internal/metric@v0.27.0 go.opentelemetry.io/otel/metric@v0.27.0But BuildKit uses; git checkout v0.11.4
go mod graph | grep ' go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp'
github.com/moby/buildkit go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0
k8s.io/apiserver@v0.22.5 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.20.0
k8s.io/component-base@v0.22.5 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.20.0But looks like BuildKit has older versions of the other packages installed; go mod graph | grep ' go.opentelemetry.io/otel/metric@'
github.com/moby/buildkit go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/otel/internal/metric@v0.27.0 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/exporters/otlp@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/oteltest@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/sdk/export/metric@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/sdk/metric@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0Looks like Changed in open-telemetry/opentelemetry-go-contrib@a587520 (open-telemetry/opentelemetry-go-contrib#1977) Unfortunately, updating these to v0.31.0 works around the other issues, but breaks BuildKit, which doesn't support the newer API; Building static bundles/binary-daemon/dockerd (linux/arm64)...
# github.com/docker/docker/vendor/github.com/moby/buildkit/util/tracing/transform
vendor/github.com/moby/buildkit/util/tracing/transform/span.go:34:26: sd.InstrumentationLibrarySpans undefined (type *"github.com/docker/docker/vendor/go.opentelemetry.io/proto/otlp/trace/v1".ResourceSpans has no field or method InstrumentationLibrarySpans)
vendor/github.com/moby/buildkit/util/tracing/transform/span.go:56:17: undefined: v11.InstrumentationLibrary
vendor/github.com/moby/buildkit/util/tracing/transform/instrumentation.go:9:42: undefined: commonpb.InstrumentationLibrary
# github.com/docker/docker/vendor/github.com/moby/buildkit/worker/containerd
vendor/github.com/moby/buildkit/worker/containerd/containerd.go:56:74: cannot use &gogoptypes.Empty{} (value of type *"github.com/docker/docker/vendor/github.com/gogo/protobuf/types".Empty) as *emptypb.Empty value in argument to client.IntrospectionService().Server |
thaJeztah
added a commit
to thaJeztah/docker
that referenced
this pull request
Mar 8, 2023
…x` is "unstable", and as it specifieds a minimum version, that will include "any breaking change after".
try to fix go.opentelemetry.io dependency hell
docker ("indirect") and buildkit ("direct") specify `v0.29.0`;
```bash
go mod graph --modfile=vendor.mod | grep ' go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp'
github.com/docker/docker go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0
github.com/moby/buildkit@v0.11.4 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0
```
containerd specified v0.34.0 for `go.opentelemetry.io/otel/metric`;
```bash
go mod graph --modfile=vendor.mod | grep ' go.opentelemetry.io/otel/metric@'
github.com/docker/docker go.opentelemetry.io/otel/metric@v0.34.0
github.com/containerd/containerd@v1.7.0-rc.2 go.opentelemetry.io/otel/metric@v0.34.0
github.com/moby/buildkit@v0.11.4 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc@v0.37.0 go.opentelemetry.io/otel/metric@v0.34.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/otel/internal/metric@v0.27.0 go.opentelemetry.io/otel/metric@v0.27.0
```
But BuildKit uses;
```bash
git checkout v0.11.4
go mod graph | grep ' go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp'
github.com/moby/buildkit go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0
k8s.io/apiserver@v0.22.5 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.20.0
k8s.io/component-base@v0.22.5 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.20.0
```
But looks like BuildKit has older versions of the other packages installed;
```bash
go mod graph | grep ' go.opentelemetry.io/otel/metric@'
github.com/moby/buildkit go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.29.0 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/otel/internal/metric@v0.27.0 go.opentelemetry.io/otel/metric@v0.27.0
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/exporters/otlp@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/oteltest@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/sdk/export/metric@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
go.opentelemetry.io/otel/sdk/metric@v0.20.0 go.opentelemetry.io/otel/metric@v0.20.0
```
Looks like `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.31.0` is the first version taking the new metrics into account;
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/instrumentation/net/http/otelhttp/v0.31.0/instrumentation/net/http/otelhttp/handler.go#L52-L53
Changed in open-telemetry/opentelemetry-go-contrib@a587520 (open-telemetry/opentelemetry-go-contrib#1977)
Unfortunately, updating these to v0.31.0 works around the other issues, but breaks BuildKit, which doesn't support the newer API;
go.opentelemetry.io/contrib/instrumentation/net/http/httptrace/otelhttptrace v0.31.0 // indirect // updated to v0.31+ to account for moby#44530 (comment)
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.31.0 // indirect // updated v0.31+ to account for moby#44530 (comment)
```bash
Building static bundles/binary-daemon/dockerd (linux/arm64)...
vendor/github.com/moby/buildkit/util/tracing/transform/span.go:34:26: sd.InstrumentationLibrarySpans undefined (type *"github.com/docker/docker/vendor/go.opentelemetry.io/proto/otlp/trace/v1".ResourceSpans has no field or method InstrumentationLibrarySpans)
vendor/github.com/moby/buildkit/util/tracing/transform/span.go:56:17: undefined: v11.InstrumentationLibrary
vendor/github.com/moby/buildkit/util/tracing/transform/instrumentation.go:9:42: undefined: commonpb.InstrumentationLibrary
vendor/github.com/moby/buildkit/worker/containerd/containerd.go:56:74: cannot use &gogoptypes.Empty{} (value of type *"github.com/docker/docker/vendor/github.com/gogo/protobuf/types".Empty) as *emptypb.Empty value in argument to client.IntrospectionService().Server
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
So, TL;DR;
|
|
Looks like the update in containerd came in
So currently we cannot update containerd until BuildKit v0.12.0 🤔 @AkihiroSuda @tonistiigi any suggestions? (would there be some hack we can apply to make v0.11.x work with both "old" and "new" OTEL? |
AkihiroSuda
changed the title
Can we just vendor the master branch of BuildKit (for the master branch of Moby)? |
|
Yeah, so I wanted to prevent already depending on an unreleased version of buildkit, so that we could do a v24.0.0 release when we wanted to.
I want to try if I can use a |
thaJeztah
force-pushed
the
containerd_1.7
branch
from
b3bedf1
to
829779f
Compare
|
Hm... right, so I can make OTEL working, but looks like there's a breaking change in containerd that is backward incompatible with BuildKit v0.11;
|
thaJeztah
force-pushed
the
containerd_1.7
branch
3 times, most recently
from
9423877
to
396c633
Compare
…240-3a7f492d3f1b full diff: opencontainers/image-spec@02efb9a...3a7f492 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: https://github.com/containerd/containerd/compare/v1.16.19..v1.7.0 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Building static bundles/binary-daemon/dockerd (linux/arm64)...
# github.com/docker/docker/vendor/github.com/moby/buildkit/worker/containerd
vendor/github.com/moby/buildkit/worker/containerd/containerd.go:56:74: cannot use &gogoptypes.Empty{} (value of type *"github.com/docker/docker/vendor/github.com/gogo/protobuf/types".Empty) as *emptypb.Empty value in argument to client.IntrospectionService().Server
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
containerd_1.7
branch
from
396c633
to
a1bf697
Compare
|
Was this working last time you looked? I was playing with container-image-store for Windows on a rare quiet evening, and that will require vendoring containerd 1.7.2 (which contains backported Windows Snapshotter work that the tests turn out to need, apparently). So I tried adapting this draft to current master as a starting point, and fixed a few conflicts in the tests, but still hit a compile issue. I wasn't sure if that's "changes since this last worked" or if it never reached a working state, as CI logs have expired. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.