cmd/ctr/commands/tasks: build metrics with target platform#8234
Closed
fuweid wants to merge 1 commit intocontainerd:mainfrom
Closed
cmd/ctr/commands/tasks: build metrics with target platform#8234fuweid wants to merge 1 commit intocontainerd:mainfrom
fuweid wants to merge 1 commit intocontainerd:mainfrom
Conversation
e7852cf to
52ab6f8
Compare
52ab6f8 to
24097cd
Compare
The windows container is using containerd/cgroups@v1.1.0 for metrics while the linux container is using containerd/cgroups@v3.0.1. They are two different packages but the metric fields in proto share the same name. For instance, the `Metrics` type: * in cgroups@v1.1.0, it is named by io.containerd.cgroups.v1.Metrics[1]. * in cgroups@v3.0.1, it's also named by io.containerd.cgroups.v1.Metrics[2]. It's hard to tell the real type, even if both types are compatible. According to typeurl/v2@v2.1.0's behavior[3], it will check the type registed by the `RegisterType` first. Let's see a case. If the data is marshaled by containerd/cgroups@v3.0.1 and the typeurl is `io.containerd.cgroups.v1.Metrics`, ideally the receiver should use the same type to unmarshal it. However, if the receiver has a dependency using the cgroups@v1.1.0, the cgroups@v1.1.0 package will register the type with `io.containerd.cgroups.v1.Metrics` by `gogo.RegisterType`. And the receiver will cgroups@v1.1.0's type to unmarshal the data. It's compatible but it is unexpected because the receiver will use the `cgroups@v3.0.1` to do the type assertion, like what nerdctl pr[4] does. They will fail to do assertion. Currently, the windows container metrics is using containerd/cgroups@v1.1.0, which impacts nerdctl to do release. So I would like to file this pr to build metrics with target platform. REF: [1]: https://github.com/containerd/cgroups/blob/v1.1.0/stats/v1/metrics.pb.go#L705 [2]: https://github.com/containerd/cgroups/blob/v3.0.1/cgroup1/stats/metrics.pb.go#L1705 [3]: https://github.com/containerd/typeurl/blob/v2.1.0/types.go#L238-L264 [4]: containerd/nerdctl#1952 Signed-off-by: Wei Fu <fuweid89@gmail.com>
24097cd to
3f14890
Compare
Member
|
Is this intended as a temporary fix until the Windows container metrics or cgroups package can be updated? Splitting apart the compiled Windows/Linux logic like this is the opposite direction we have been trying go in to unify our cross platform logic. |
Member
Author
@dmcgowan it is temporary fix for nerdctl release. No sure that when windows container can be updated... |
Member
|
For nerdctl release I think we can just copy |
Member
Author
|
@AkihiroSuda sounds 👍. I close this. Thanks |
Member
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
3 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.
The windows container is using containerd/cgroups@v1.1.0 for metrics while the linux container is using containerd/cgroups@v3.0.1. They are two different packages but the metric fields in proto share the same name. For instance, the
Metricstype:It's hard to tell the real type, even if both types are compatible. According to typeurl/v2@v2.1.0's behavior[3], it will check the type registed by the
RegisterTypefirst. Let's see a case.If the data is marshaled by containerd/cgroups@v3.0.1 and the typeurl is
io.containerd.cgroups.v1.Metrics, ideally the receiver should use the same type to unmarshal it. However, if the receiver has a dependency using the cgroups@v1.1.0, the cgroups@v1.1.0 package will register the type withio.containerd.cgroups.v1.Metricsbygogo.RegisterType. And the receiver will cgroups@v1.1.0's type to unmarshal the data. It's compatible but it is unexpected because the receiver will use thecgroups@v3.0.1to do the type assertion, like what nerdctl pr[4] does. They will fail to do assertion.Currently, the windows container metrics is using
containerd/cgroups@v1.1.0, which impacts nerdctl to do release. So I would like to file this pr to build metrics with target platform.
REF:
[1]: https://github.com/containerd/cgroups/blob/v1.1.0/stats/v1/metrics.pb.go#L705
[2]: https://github.com/containerd/cgroups/blob/v3.0.1/cgroup1/stats/metrics.pb.go#L1705
[3]: https://github.com/containerd/typeurl/blob/v2.1.0/types.go#L238-L264
[4]: containerd/nerdctl#1952