Cgroupv2: Added CPU, Memory metrics by Zyqsempai · Pull Request #1376 · containerd/cri

Zyqsempai · 2020-01-14T10:27:41Z

Vendor updated to the last and stble versions of contqinerd and cgroups
Added cgroupv2 metrics.

Signed-off-by: Boris Popovschi zyqsempai@mail.ru

k8s-ci-robot · 2020-01-14T10:27:56Z

Hi @Zyqsempai. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/server/container_stats_list_unix.go

AkihiroSuda · 2020-01-14T10:49:43Z

pkg/server/container_stats_list_unix.go

+				cs.Memory = &runtime.MemoryUsage{
+					Timestamp: stats.Timestamp.UnixNano(),
+					WorkingSetBytes: &runtime.UInt64Value{
+						Value: metrics.Memory.Usage,


needs consideration for inactive_file as in v1 implementation? (I haven't looked into whether v2 inactive_file really corresponds to v1 total_inactive_file)

cc @Random-Liu

It's a good question actually, but it seems like they just removed prefix total from all the metrics in v2.

AkihiroSuda

/ok-to-test /lgtm

AkihiroSuda · 2020-01-14T17:30:35Z

/ok-to-test

jterry75 · 2020-01-14T21:03:49Z

pkg/server/container_stats_list_unix.go

-			cs.Cpu = &runtime.CpuUsage{
-				Timestamp:            stats.Timestamp.UnixNano(),
-				UsageCoreNanoSeconds: &runtime.UInt64Value{Value: metrics.CPU.Usage.Total},
+		switch s.(type) {


You can use switch metric := s.(type) { and then you dont need the type assertion below.

jterry75 · 2020-01-14T21:05:05Z

pkg/server/container_stats_list_unix.go

 		if err != nil {
 			return nil, errors.Wrap(err, "failed to extract container metrics")
 		}
-		metrics := s.(*v1.Metrics)


@Random-Liu - This would have previously panic'd if this wasnt the return type. Should we have a default case where we return error or maintain this?

The CRI expects failure on conversion errors. We should not panic... we should return the error.

jterry75

I like it! Minor questions

AkihiroSuda

LGTM, but please consider squashing commits

mikebrow

See comment on vndr update.

vendor.conf

AkihiroSuda · 2020-01-15T23:58:01Z

/test pull-cri-containerd-verify

AkihiroSuda · 2020-01-16T03:21:06Z

/test pull-cri-containerd-verify

mikebrow · 2020-01-16T14:30:22Z

/test pull-cri-containerd-verify

AkihiroSuda · 2020-01-17T04:01:10Z

vendor failing

I0116 14:33:35.417] These files were modified:
I0116 14:33:35.418] 
I0116 14:33:35.418]  M vendor/github.com/Microsoft/hcsshim/container.go
I0116 14:33:35.418]  M vendor/github.com/Microsoft/hcsshim/go.mod
I0116 14:33:35.418]  M vendor/github.com/Microsoft/hcsshim/hcn/hcn.go
I0116 14:33:35.418]  M vendor/github.com/Microsoft/hcsshim/hcn/hcnglobals.go
I0116 14:33:35.418]  M vendor/github.com/Microsoft/hcsshim/hcn/hcnloadbalancer.go
I0116 14:33:35.419]  M vendor/github.com/Microsoft/hcsshim/hcn/hcnsupport.go
I0116 14:33:35.419]  M vendor/github.com/Microsoft/hcsshim/internal/hcs/process.go
I0116 14:33:35.419]  M vendor/github.com/Microsoft/hcsshim/internal/hcs/system.go
I0116 14:33:35.419]  M vendor/github.com/Microsoft/hcsshim/internal/vmcompute/vmcompute.go
I0116 14:33:35.419]  M vendor/github.com/Microsoft/hcsshim/osversion/windowsbuilds.go
I0116 14:33:35.419] 
W0116 14:33:35.430] make: *** [Makefile:71: check-vendor] Error 1

Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>

AkihiroSuda · 2020-01-17T10:26:36Z

/test pull-cri-containerd-node-e2e

jterry75 · 2020-01-17T19:25:42Z

Green!

AkihiroSuda

LGTM but a typo

AkihiroSuda · 2020-01-20T04:08:03Z

pkg/server/container_stats_list_unix.go

+				}
+			}
+		default:
+			return &cs, errors.New(fmt.Sprintf("unxpected metrics type: %v", metrics))


typo: "unxpected"

Also, you can use errors.Errorf here.

AkihiroSuda · 2020-01-21T14:21:02Z

merging, typo is negligible

k8s-ci-robot added needs-ok-to-test size/XXL labels Jan 14, 2020

AkihiroSuda reviewed Jan 14, 2020

View reviewed changes

pkg/server/container_stats_list_unix.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jan 14, 2020

View reviewed changes

crosbymichael mentioned this pull request Jan 14, 2020

cgroups v2 Support containerd/containerd#3726

Closed

11 tasks

AkihiroSuda approved these changes Jan 14, 2020

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jan 14, 2020

Random-Liu self-assigned this Jan 14, 2020

jterry75 reviewed Jan 14, 2020

View reviewed changes

AkihiroSuda approved these changes Jan 15, 2020

View reviewed changes

Zyqsempai force-pushed the add-cgroups-v2-metrics branch from 292737b to e67cc43 Compare January 15, 2020 09:31

mikebrow suggested changes Jan 15, 2020

View reviewed changes

vendor.conf Outdated Show resolved Hide resolved

Zyqsempai force-pushed the add-cgroups-v2-metrics branch from e67cc43 to b515c20 Compare January 15, 2020 20:55

mikebrow approved these changes Jan 16, 2020

View reviewed changes

vendor updated + added cgroupv2 metrics

6b8846c

Signed-off-by: Boris Popovschi <zyqsempai@mail.ru>

Zyqsempai force-pushed the add-cgroups-v2-metrics branch from b515c20 to 6b8846c Compare January 17, 2020 09:55

AkihiroSuda reviewed Jan 20, 2020

View reviewed changes

AkihiroSuda merged commit 5e5960f into containerd:master Jan 21, 2020

Conversation

Zyqsempai commented Jan 14, 2020

Uh oh!

k8s-ci-robot commented Jan 14, 2020

Uh oh!

Uh oh!

AkihiroSuda Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

Zyqsempai Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jan 14, 2020

Uh oh!

jterry75 Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

jterry75 Jan 14, 2020

Choose a reason for hiding this comment

Uh oh!

mikebrow Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

jterry75 left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AkihiroSuda commented Jan 15, 2020

Uh oh!

AkihiroSuda commented Jan 16, 2020

Uh oh!

mikebrow commented Jan 16, 2020

Uh oh!

AkihiroSuda commented Jan 17, 2020

Uh oh!

AkihiroSuda commented Jan 17, 2020

Uh oh!

jterry75 commented Jan 17, 2020

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants