Skip to content
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

go.mod: github.com/containerd/cgroups/v3 v3.0.1, github.com/docker/docker v24.0.0 #1952

Merged
merged 1 commit into from
May 17, 2023

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Jan 30, 2023

@AkihiroSuda AkihiroSuda added dependencies Pull requests that update a dependency file go Pull requests that update Go code labels Jan 30, 2023
@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jan 30, 2023
@djdongjin
Copy link
Member

There is also https://github.com/containerd/typeurl/releases/tag/v2.0.0

Should we update it as well?

nerdctl/go.mod

Line 20 in ca4a763

github.com/containerd/typeurl v1.0.3-0.20220422153119-7f6e6d160d67

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Jan 30, 2023

Looks like nerdctl stats hangs up with this

Probably due to

@AkihiroSuda AkihiroSuda removed this from the v1.2.0 milestone Jan 30, 2023
@AkihiroSuda AkihiroSuda marked this pull request as draft January 30, 2023 15:48
@AkihiroSuda
Copy link
Member Author

There is also https://github.com/containerd/typeurl/releases/tag/v2.0.0

Should we update it as well?

nerdctl/go.mod

Line 20 in ca4a763

github.com/containerd/typeurl v1.0.3-0.20220422153119-7f6e6d160d67

Not sure how it is relevant to this PR, but I'd prefer to wait for v2.1.0 (containerd/typeurl#40)

@djdongjin
Copy link
Member

Not sure how it is relevant to this PR, but I'd prefer to wait for v2.1.0 (containerd/typeurl#40)

SGTM, it's not relevant to this PR, just for awareness since dependabot seems skip checking those dependencies with a specific commit. :)

@AkihiroSuda AkihiroSuda changed the title go.mod: github.com/containerd/cgroups/v3 v3.0.0 go.mod: github.com/containerd/cgroups/v3 v3.0.1 Feb 17, 2023
fuweid added a commit to fuweid/containerd that referenced this pull request Mar 8, 2023
The windows container is using containerd/[email protected] for metrics
while the linux container is using containerd/[email protected]. They are
two different packages but the metric fields in proto share the same
name. For instance, the `Metrics` type:

* in [email protected], it is named by io.containerd.cgroups.v1.Metrics[1].
* in [email protected], 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/[email protected]'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/[email protected] 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 [email protected], the [email protected] package will register the
type with `io.containerd.cgroups.v1.Metrics` by `gogo.RegisterType`. And
the receiver will [email protected]'s type to unmarshal the data. It's
compatible but it is unexpected because the receiver will use the
`[email protected]` 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/[email protected], 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 <[email protected]>
fuweid added a commit to fuweid/containerd that referenced this pull request Mar 8, 2023
The windows container is using containerd/[email protected] for metrics
while the linux container is using containerd/[email protected]. They are
two different packages but the metric fields in proto share the same
name. For instance, the `Metrics` type:

* in [email protected], it is named by io.containerd.cgroups.v1.Metrics[1].
* in [email protected], 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/[email protected]'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/[email protected] 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 [email protected], the [email protected] package will register the
type with `io.containerd.cgroups.v1.Metrics` by `gogo.RegisterType`. And
the receiver will [email protected]'s type to unmarshal the data. It's
compatible but it is unexpected because the receiver will use the
`[email protected]` 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/[email protected], 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 <[email protected]>
fuweid added a commit to fuweid/containerd that referenced this pull request Mar 8, 2023
The windows container is using containerd/[email protected] for metrics
while the linux container is using containerd/[email protected]. They are
two different packages but the metric fields in proto share the same
name. For instance, the `Metrics` type:

* in [email protected], it is named by io.containerd.cgroups.v1.Metrics[1].
* in [email protected], 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/[email protected]'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/[email protected] 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 [email protected], the [email protected] package will register the
type with `io.containerd.cgroups.v1.Metrics` by `gogo.RegisterType`. And
the receiver will [email protected]'s type to unmarshal the data. It's
compatible but it is unexpected because the receiver will use the
`[email protected]` 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/[email protected], 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 <[email protected]>
fuweid added a commit to fuweid/containerd that referenced this pull request Mar 8, 2023
The windows container is using containerd/[email protected] for metrics
while the linux container is using containerd/[email protected]. They are
two different packages but the metric fields in proto share the same
name. For instance, the `Metrics` type:

* in [email protected], it is named by io.containerd.cgroups.v1.Metrics[1].
* in [email protected], 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/[email protected]'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/[email protected] 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 [email protected], the [email protected] package will register the
type with `io.containerd.cgroups.v1.Metrics` by `gogo.RegisterType`. And
the receiver will [email protected]'s type to unmarshal the data. It's
compatible but it is unexpected because the receiver will use the
`[email protected]` 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/[email protected], 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 <[email protected]>
@AkihiroSuda AkihiroSuda changed the title go.mod: github.com/containerd/cgroups/v3 v3.0.1 go.mod: github.com/containerd/cgroups/v3 v3.0.1, github.com/docker/docker v24.0.0 May 16, 2023
@AkihiroSuda AkihiroSuda marked this pull request as ready for review May 16, 2023 17:54
@AkihiroSuda AkihiroSuda added this to the v1.4.0 milestone May 16, 2023
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit b7fead1 into containerd:main May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants