Skip to content

[release/1.7] Fix "invalid metric type" error message for cgroup v1#10814

Merged
dmcgowan merged 1 commit into
containerd:release/1.7from
Iceber:release-1.7/unmarshal-metrics-to-type
Oct 16, 2024
Merged

[release/1.7] Fix "invalid metric type" error message for cgroup v1#10814
dmcgowan merged 1 commit into
containerd:release/1.7from
Iceber:release-1.7/unmarshal-metrics-to-type

Conversation

@Iceber

@Iceber Iceber commented Oct 11, 2024

Copy link
Copy Markdown
Member

This pr comes from not updating #9195, and it only works for release/1.7, it didn't have this problem in 1.6 on 2.x


Unmarshal io.containerd.cgroups.v1.Metrics with UnmarshalAny in 1.7+ (don't track which version it started with, but the latest version still has this issue) will first be parsed as github.com/containerd/cgroups/stats/v1.Metrics.

If UnmarshalAny in a unit test it shouldn't have this problem because it's not importing the wrong Metrics

This is due to hcsshim causing the dependency to have both github.com/containerd/cgroups/stats/v1/ and . /vendor/github.com/containerd/cgroups/v3/cgroup1/stats

How to reproduce this issue:
env:

  • containerd release/1.7 or 1.7.19
  • cgroup v1

steps:

  1. configure /etc/containerd/config.toml to enable metrics
[metrics]
  address = "127.0.0.1:7000"
  1. curl 127.0.0.1:7000/v1/metrics | grep ^container_
  2. You will notice that there is no container_* indicator.
  3. Check the logs of containerd, error log -> {"error":null,"level":"error","msg":"invalid metric type for ...

Copy from #9195 (comment)

data, err := typeurl.UnmarshalAny(stats)
if err != nil {
s := &v2.Metrics{}
if err := typeurl.UnmarshalTo(stats, s); err != nil {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It just guarantees the same style of code as v1/metrics.go.

@samuelkarp

Copy link
Copy Markdown
Member

it didn't have this problem in 1.6 on 2.x

Even if 2.x doesn't have this problem, can we do the code change on main first and then backport as normal?

One other note: there's a blank line between the Co-authored-by trailer and Signed-off-by which is causing GitHub to not read it properly.

@Iceber

Iceber commented Oct 11, 2024

Copy link
Copy Markdown
Member Author

it didn't have this problem in 1.6 on 2.x

Even if 2.x doesn't have this problem, can we do the code change on main first and then backport as normal?

yeah, thanks for the suggestion, I wasn't sure whether to change him on 2.x first.
Now I'll create a new pr on main.

Co-authored-by: Sam Lockart <sam.lockart@zendesk.com>
Signed-off-by: Iceber Gu <caiwei95@hotmail.com>
@Iceber Iceber force-pushed the release-1.7/unmarshal-metrics-to-type branch from 4e6e97d to d6f5778 Compare October 11, 2024 07:17
@estesp

estesp commented Oct 16, 2024

Copy link
Copy Markdown
Member

main PR is now merged (#10815)

@dmcgowan dmcgowan merged commit 7d2d0c6 into containerd:release/1.7 Oct 16, 2024
@samuelkarp samuelkarp changed the title [release/1.7] metrics: Use UnmarshalTo instead of UnmarshalAny [release/1.7] fix "invalid metric type" for cgroup v1 Nov 20, 2024
@samuelkarp samuelkarp changed the title [release/1.7] fix "invalid metric type" for cgroup v1 [release/1.7] fix "invalid metric type" error message for cgroup v1 Nov 20, 2024
@samuelkarp samuelkarp changed the title [release/1.7] fix "invalid metric type" error message for cgroup v1 [release/1.7] Fix "invalid metric type" error message for cgroup v1 Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants