vendor: github.com/containerd/containerd v1.7.0-rc.2#44530
Closed
thaJeztah wants to merge 3 commits intomoby:masterfrom
Closed
vendor: github.com/containerd/containerd v1.7.0-rc.2#44530thaJeztah wants to merge 3 commits intomoby:masterfrom
thaJeztah wants to merge 3 commits intomoby:masterfrom
Conversation
thaJeztah
force-pushed
the
containerd_1.7
branch
from
116c2a4 to
ca518d3
Compare
Member
Author
|
Some linting failures to fix; |
Member
Author
|
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
Member
Author
|
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(), | ||
| } |
Member
Author
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?
Member
Author
There was a problem hiding this comment.
Maybe I'm looking at proto.Clone() or proto.Merge
Member
Author
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
Member
Author
|
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); |
Member
Author
Member
Author
|
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/[email protected]
github.com/moby/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]containerd 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/[email protected]
github.com/containerd/[email protected] go.opentelemetry.io/otel/[email protected]
github.com/moby/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/internal/[email protected] go.opentelemetry.io/otel/[email protected]But 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/[email protected]
k8s.io/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
k8s.io/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]But 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/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/internal/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/exporters/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/sdk/export/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/sdk/[email protected] go.opentelemetry.io/otel/[email protected]Looks 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/[email protected]
github.com/moby/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
```
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/[email protected]
github.com/containerd/[email protected] go.opentelemetry.io/otel/[email protected]
github.com/moby/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/internal/[email protected] go.opentelemetry.io/otel/[email protected]
```
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/[email protected]
k8s.io/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
k8s.io/[email protected] go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]
```
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/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/internal/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/contrib/instrumentation/net/http/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/exporters/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/sdk/export/[email protected] go.opentelemetry.io/otel/[email protected]
go.opentelemetry.io/otel/sdk/[email protected] go.opentelemetry.io/otel/[email protected]
```
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 <[email protected]>
Member
Author
|
So, TL;DR;
|
Member
Author
|
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
Member
Can we just vendor the master branch of BuildKit (for the master branch of Moby)? |
Member
Author
|
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
Member
Author
|
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 <[email protected]>
full diff: https://github.com/containerd/containerd/compare/v1.16.19..v1.7.0 Signed-off-by: Sebastiaan van Stijn <[email protected]>
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 <[email protected]>
thaJeztah
force-pushed
the
containerd_1.7
branch
from
396c633 to
a1bf697
Compare
Contributor
|
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. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Uh oh!
There was an error while loading. Please reload this page.