Implement CRI metrics collection #7796

Mo-Fatah · 2024-02-19T16:08:18Z

What type of PR is this?

/kind api-change
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Related to https://issues.redhat.com/browse/OCPNODE-1008

Special notes for your reviewer:

This is a continuation of Sohan's effort in #7256 based on the recent cgroup manager changes in #7658 .

Does this PR introduce a user-facing change?

Implement CRI container and sandbox metrics collection

openshift-ci · 2024-02-19T16:08:35Z

Hi @Mo-Fatah. Thanks for your PR.

I'm waiting for a cri-o 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.

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.

codecov · 2024-02-19T16:19:29Z

Codecov Report

Attention: Patch coverage is 35.48387% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 49.63%. Comparing base (b40648e) to head (67ba58c).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7796      +/-   ##
==========================================
- Coverage   49.68%   49.63%   -0.06%     
==========================================
  Files         153      153              
  Lines       16826    16884      +58     
==========================================
+ Hits         8360     8380      +20     
- Misses       7423     7459      +36     
- Partials     1043     1045       +2

kwilczynski · 2024-02-28T08:04:27Z

/ok-to-test

internal/lib/stats/metrics.go

haircommander · 2024-03-06T21:45:06Z

internal/lib/stats/metrics.go

+}
+
+func sandboxBaseLabelValues(sb *sandbox.Sandbox) []string {
+	// TODO FIXME: image?


(I wrote this TODO) What does cadvisor report as the image for the POD container?

I tried to look it up but couldn't find what they are using exactly as image name for POD containers. They seem to use the image name from the container info (here and here but not sure how they differentiate between a container and a pod, and what image name exactly they are using for pods.

my suggestion would be using the sandbox name defined in the sandbox metadata (e.g. here)

internal/lib/stats/metrics.go

haircommander · 2024-03-06T21:50:10Z

great work here @Mo-Fatah ! I am leaning towards dropping the optimzations where we replace a slice in place. I think there will be an advantage to doing so in the future, but let's keep it simple to start. I also would love to see some unit and integration tests here to verify contents. If you don't have the bandwidth to add those let us know!

kwilczynski · 2024-03-07T07:25:22Z

/hold

So I can do a proper review.

Mo-Fatah · 2024-03-15T13:48:42Z

@haircommander I simplified the slices updates and added an integration test for the memory metrics as a starter. the tests mainly reads the cgroup value and compare it to the value returned from metricsp . let me know if you have a better testing approach or other comments in general.

openshift-ci · 2024-04-30T14:41:34Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-e2e-evented-pleg

In response to this:

/override ci/prow/ci-e2e-evented-pleg
/lgtm

Thanks @Mo-Fatah ! Great work here

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.

saschagrunert · 2024-05-02T06:59:31Z

/retest

Mo-Fatah · 2024-05-02T07:28:46Z

/retest

- Add collection_period and included_pod_metrics to StatsConfig and shell completions Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com> Co-authored-by: Sohan Kunkerkar <sohank2602@gmail.com>

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com> Co-authored-by: Sohan Kunkerkar <sohank2602@gmail.com>

- Update StatsServer to collect sandbox metrics in the update cycle Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com> Co-authored-by: Sohan Kunkerkar <sohank2602@gmail.com>

- typo fix Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

- Metrics tests improvement - Clean up Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

- internal/stats: Use the internal logging package - Tiny refactoring for better error handling - lint, shfmt, shellcheck Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

haircommander · 2024-05-02T16:28:29Z

/lgtm
/approve

🎉

openshift-ci · 2024-05-02T16:29:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski, Mo-Fatah, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [haircommander,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

haircommander · 2024-05-02T18:50:47Z

/retest

haircommander · 2024-05-02T19:49:19Z

/retest

kwilczynski · 2024-05-02T22:41:10Z

/test e2e-gcp-ovn

Mo-Fatah · 2024-05-03T07:22:26Z

/retest

haircommander · 2024-05-03T11:48:23Z

/override ci/prow/e2e-gcp-ovn

openshift-ci · 2024-05-03T11:48:29Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-gcp-ovn

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.

Mo-Fatah marked this pull request as ready for review February 24, 2024 11:34

Mo-Fatah requested review from fidencio and mrunalp as code owners February 24, 2024 11:34

openshift-ci bot requested review from klihub and kwilczynski February 24, 2024 11:34

Mo-Fatah force-pushed the cri-stats-impl branch 3 times, most recently from c1d5799 to e8ac4da Compare February 24, 2024 12:08

Mo-Fatah changed the title ~~WIP: Implement CRI stats collection~~ Implement CRI metrics collection Feb 27, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 27, 2024

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 28, 2024

haircommander reviewed Mar 6, 2024

View reviewed changes

internal/lib/stats/metrics.go Outdated Show resolved Hide resolved

haircommander reviewed Mar 6, 2024

View reviewed changes

internal/lib/stats/metrics.go Outdated Show resolved Hide resolved

haircommander reviewed Mar 6, 2024

View reviewed changes

internal/lib/stats/metrics.go Outdated Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2024

Mo-Fatah force-pushed the cri-stats-impl branch from d6d83cb to a83d366 Compare March 15, 2024 13:44

haircommander removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2024

saschagrunert approved these changes May 2, 2024

View reviewed changes

openshift-ci bot assigned saschagrunert May 2, 2024

Mo-Fatah and others added 12 commits May 2, 2024 19:02

- Add cri-metrics implementations

38e296b

- Add collection_period and included_pod_metrics to StatsConfig and shell completions Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com> Co-authored-by: Sohan Kunkerkar <sohank2602@gmail.com>

Add metrics decscriptors list

40ea851

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Add file_mapped and failcnt fields to memory stats

352afed

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Replace libcontainer.Stats with cgmgr.CgroupStats

e851caf

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com> Co-authored-by: Sohan Kunkerkar <sohank2602@gmail.com>

- Implement ListPodSandboxMetrics

274a509

- Update StatsServer to collect sandbox metrics in the update cycle Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com> Co-authored-by: Sohan Kunkerkar <sohank2602@gmail.com>

- Add missing fields and functions to unsupported stats

d7fe9a3

- typo fix Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Add metrics integration tests

b4ac9a9

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

New slice for each metric collection

2ba4de4

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

- Better naming for ContainerStats struct

33d2f47

- Metrics tests improvement - Clean up Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

- internal/stats: Pass the container_server context down to statsserver

820522d

- internal/stats: Use the internal logging package - Tiny refactoring for better error handling - lint, shfmt, shellcheck Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

internal/criocli: Update criocli with new added metrics flags

1955ce5

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

test/cri-metrics.bats: skip test for kata VMs

97384f9

Signed-off-by: Mohamed Abdelfatah <mohamedabdelfatah2027@gmail.com>

Mo-Fatah force-pushed the cri-stats-impl branch from 67ba58c to 97384f9 Compare May 2, 2024 16:02

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 2, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2024

openshift-merge-bot bot merged commit 175cc34 into cri-o:main May 3, 2024
66 of 69 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CRI metrics collection #7796

Implement CRI metrics collection #7796

Mo-Fatah commented Feb 19, 2024 •

edited

openshift-ci bot commented Feb 19, 2024

codecov bot commented Feb 19, 2024 •

edited

kwilczynski commented Feb 28, 2024

haircommander Mar 6, 2024

Mo-Fatah Mar 15, 2024

haircommander commented Mar 6, 2024

kwilczynski commented Mar 7, 2024

Mo-Fatah commented Mar 15, 2024

openshift-ci bot commented Apr 30, 2024

saschagrunert commented May 2, 2024

Mo-Fatah commented May 2, 2024

haircommander commented May 2, 2024

openshift-ci bot commented May 2, 2024

haircommander commented May 2, 2024

haircommander commented May 2, 2024

kwilczynski commented May 2, 2024

Mo-Fatah commented May 3, 2024

haircommander commented May 3, 2024

openshift-ci bot commented May 3, 2024

Implement CRI metrics collection #7796

Implement CRI metrics collection #7796

Conversation

Mo-Fatah commented Feb 19, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci bot commented Feb 19, 2024

codecov bot commented Feb 19, 2024 • edited

Codecov Report

kwilczynski commented Feb 28, 2024

haircommander Mar 6, 2024

Choose a reason for hiding this comment

Mo-Fatah Mar 15, 2024

Choose a reason for hiding this comment

haircommander commented Mar 6, 2024

kwilczynski commented Mar 7, 2024

Mo-Fatah commented Mar 15, 2024

openshift-ci bot commented Apr 30, 2024

saschagrunert commented May 2, 2024

Mo-Fatah commented May 2, 2024

haircommander commented May 2, 2024

openshift-ci bot commented May 2, 2024

haircommander commented May 2, 2024

haircommander commented May 2, 2024

kwilczynski commented May 2, 2024

Mo-Fatah commented May 3, 2024

haircommander commented May 3, 2024

openshift-ci bot commented May 3, 2024

Mo-Fatah commented Feb 19, 2024 •

edited

codecov bot commented Feb 19, 2024 •

edited