Skip to content

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

Merged
fuweid merged 1 commit intocontainerd:mainfrom
AkihiroSuda:containerd-cgroups-v3
May 17, 2023
Merged

go.mod: github.com/containerd/cgroups/v3 v3.0.1, github.com/docker/docker v24.0.0#1952
fuweid merged 1 commit intocontainerd:mainfrom
AkihiroSuda:containerd-cgroups-v3

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Jan 30, 2023 •
edited
Loading

@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
Copy Markdown
Member

djdongjin commented Jan 30, 2023

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
Copy Markdown
Member Author

AkihiroSuda commented Jan 30, 2023 •
edited
Loading

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
Copy Markdown
Member Author

AkihiroSuda commented Jan 30, 2023

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
Copy Markdown
Member

djdongjin commented Jan 30, 2023

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 force-pushed the containerd-cgroups-v3 branch from b46c83e to a7647cd Compare February 17, 2023 05:15
@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
fuweid reviewed Mar 8, 2023
View reviewed changes
Comment thread cmd/nerdctl/container_stats_linux.go
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 8, 2023
Merged
@AkihiroSuda AkihiroSuda force-pushed the containerd-cgroups-v3 branch from a7647cd to 39d6847 Compare March 8, 2023 10:50
fuweid added a commit to fuweid/containerd that referenced this pull request Mar 8, 2023
@fuweid
cmd/ctr/commands/tasks: build metrics with target platform
e7852cf 
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 fuweid mentioned this pull request Mar 8, 2023
Closed
fuweid added a commit to fuweid/containerd that referenced this pull request Mar 8, 2023
@fuweid
cmd/ctr/commands/tasks: build metrics with target platform
52ab6f8 
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
@fuweid
cmd/ctr/commands/tasks: build metrics with target platform
24097cd 
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
@fuweid
cmd/ctr/commands/tasks: build metrics with target platform
3f14890 
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 force-pushed the containerd-cgroups-v3 branch from 39d6847 to 25c832c Compare March 9, 2023 04:09
@AkihiroSuda AkihiroSuda force-pushed the containerd-cgroups-v3 branch from 25c832c to 939cdd1 Compare April 3, 2023 05:09
@AkihiroSuda AkihiroSuda force-pushed the containerd-cgroups-v3 branch from 939cdd1 to c816df8 Compare May 16, 2023 17:54
@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
@AkihiroSuda AkihiroSuda force-pushed the containerd-cgroups-v3 branch from c816df8 to 79181b7 Compare May 16, 2023 18:03
@AkihiroSuda AkihiroSuda requested a review from fuweid May 16, 2023 19:10
fuweid
fuweid approved these changes May 17, 2023
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

dependencies Pull requests that update a dependency file go Pull requests that update Go code

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@AkihiroSuda @djdongjin @fuweid