critest: test metric descriptors #1931

haircommander · 2025-10-20T19:09:47Z

What type of PR is this?

/kind feature

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?

add tests for metric descriptors in critest

k8s-ci-robot · 2025-10-20T19:09:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander

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]

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

haircommander · 2025-10-20T19:26:14Z

cc @akhilerm

akhilerm · 2025-10-21T05:23:05Z

pkg/validate/pod.go

+	"container_fs_inodes_free",
+	"container_fs_inodes_total",
+	"container_fs_io_current",
+	"container_fs_io_time_seconds_total",


The fs metrics are currently not implemented in cri-o. ref: cri-o/cri-o#9344

yeah there's a number of these cri-o doesn't support yet. My plan is to get consensus on the list then fill in the gaps

akhilerm · 2025-10-21T05:43:37Z

pkg/validate/pod.go

+
+			By("create container in pod")
+			ic := f.CRIClient.CRIImageClient
+			containerID := framework.CreatePauseContainer(rc, ic, podID, podConfig, "container-for-metrics-")


Should we use CreateDefaultContainer rather than the pause container?

akhilerm · 2025-10-21T05:44:19Z

pkg/validate/pod.go

+	"container_fs_write_seconds_total",
+	"container_fs_writes_merged_total",
+	"container_fs_writes_total",
+	"container_last_seen",


A few of these metrics are currently not in the implementation in containerd also.

do you have a list handy of the ones you aren't yet supporting? is it because you don't plan on it or haven't gotten to it yet
I think for this KEP to go beta each should really support the full set (I say knowing CRI-O is missing some)

From containerd, the below metrics are not reported from metric descriptors

"container_cpu_load_average_10s", "container_cpu_load_d_average_10s", "container_file_descriptors", "container_last_seen", "container_oom_events_total", "container_pressure_cpu_stalled_seconds_total", "container_pressure_cpu_waiting_seconds_total", "container_pressure_io_stalled_seconds_total", "container_pressure_io_waiting_seconds_total", "container_pressure_memory_stalled_seconds_total", "container_pressure_memory_waiting_seconds_total", "container_sockets", "container_spec_cpu_period", "container_spec_cpu_shares", "container_spec_memory_limit_bytes", "container_spec_memory_reservation_limit_bytes", "container_spec_memory_swap_limit_bytes", "container_start_time_seconds", "container_tasks_state", "container_threads", "container_ulimits_soft",

These are now added as part of #12426.

Checking why the CI fails on containerd main.

Signed-off-by: Peter Hunt <[email protected]>

some context: kubernetes/kubernetes#134981 Signed-off-by: Peter Hunt <[email protected]>

… present Signed-off-by: Peter Hunt <[email protected]>

Signed-off-by: Peter Hunt <[email protected]>

haircommander · 2025-11-05T21:16:01Z

once cri-o/cri-o#9571 merges, this should pass for CRI-O. I still could use @akhilerm 's help figuring out why containerd tests are still failing

haircommander · 2025-11-06T17:57:47Z

  Nov  6 17:55:09.926: INFO: Unexpected error occurred: rpc error: code = Unknown desc = container create failed: time="2025-11-06T17:55:09Z" level=error msg="runc create failed: unable to start container process: error during container init: error closing exec fds: get handle to /proc/thread-self/fd: unsafe procfs detected: openat2 fsmount:fscontext:proc/thread-self/fd/: operation not permitted"

CRI-O failures are expected and unrelated opencontainers/runc#4968

haircommander · 2025-11-06T18:10:52Z

i've paired this down to just test present metric descriptors. CRI-O was hitting issues with diskIO metrics being tricky to reliably trigger (io.stat file doesn't get created when the container hasn't triggered block IO) and containerd is returning empty metrics for some reason. I think adding the pod metrics tests can be a follow up

saschagrunert · 2025-11-07T06:58:39Z

pkg/validate/pod.go

+		}
+	}
+
+	Expect(missingMetrics).To(BeEmpty(), "Expected %s metrics to be present and they were not", strings.Join(missingMetrics, " "))


Suggested change

Expect(missingMetrics).To(BeEmpty(), "Expected %s metrics to be present and they were not", strings.Join(missingMetrics, " "))

Expect(missingMetrics).To(BeEmpty(), "Expected metrics missing: %s", strings.Join(missingMetrics, ", "))

akhilerm · 2025-11-11T05:50:04Z

pkg/validate/pod.go

+}
+
+// testPodSandboxMetrics verifies that metrics are present for the specified pod.
+func testPodSandboxMetrics(allMetrics []*runtimeapi.PodSandboxMetrics, podID string) {


Are still taking in this function. Only the metrics descriptors test are needed right?

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 20, 2025

k8s-ci-robot requested review from Random-Liu and mrunalp October 20, 2025 19:09

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2025

haircommander force-pushed the metrics-tests branch 2 times, most recently from 37a4117 to 5188a10 Compare October 20, 2025 19:18

akhilerm reviewed Oct 21, 2025

View reviewed changes

haircommander mentioned this pull request Oct 23, 2025

feat: Add Disk Metrics cri-o/cri-o#9344

Merged

This was referenced Oct 24, 2025

Implement CRI ListPodSandboxMetrics containerd/containerd#10691

Merged

add missing podsandbox metrics for CRI conformance containerd/containerd#12426

Merged

haircommander force-pushed the metrics-tests branch 3 times, most recently from 94b0d3b to bd53331 Compare October 28, 2025 18:24

haircommander mentioned this pull request Oct 30, 2025

features: move PodAndContainerStatsFromCRI to off by default beta kubernetes/kubernetes#134988

Open

1 task

haircommander force-pushed the metrics-tests branch from f66ce62 to e5357fd Compare November 5, 2025 20:57

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2025

haircommander added 4 commits November 5, 2025 16:13

critest: test metric and metric descriptors

7865637

Signed-off-by: Peter Hunt <[email protected]>

critest: drop cpuLoad metrics from test

7ac79fe

some context: kubernetes/kubernetes#134981 Signed-off-by: Peter Hunt <[email protected]>

critest/metrics: generate some disk usage to guarantee io metrics are…

125e9c3

… present Signed-off-by: Peter Hunt <[email protected]>

crio: update config to enable metrics

01b03eb

Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the metrics-tests branch from e5357fd to 01b03eb Compare November 5, 2025 21:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2025

haircommander changed the title ~~WIP critest: test metric and metric descriptors~~ ritest: test metric and metric descriptors Nov 5, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2025

haircommander changed the title ~~ritest: test metric and metric descriptors~~ critest: test metric and metric descriptors Nov 5, 2025

haircommander force-pushed the metrics-tests branch from 01b03eb to 12832bd Compare November 6, 2025 17:54

haircommander changed the title ~~critest: test metric and metric descriptors~~ critest: test metric descriptors Nov 6, 2025

haircommander force-pushed the metrics-tests branch from 12832bd to 01b03eb Compare November 6, 2025 18:42

saschagrunert reviewed Nov 7, 2025

View reviewed changes

akhilerm reviewed Nov 11, 2025

View reviewed changes

	Expect(missingMetrics).To(BeEmpty(), "Expected %s metrics to be present and they were not", strings.Join(missingMetrics, " "))
	Expect(missingMetrics).To(BeEmpty(), "Expected metrics missing: %s", strings.Join(missingMetrics, ", "))

critest: test metric descriptors #1931

Are you sure you want to change the base?

critest: test metric descriptors #1931

Conversation

haircommander commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

k8s-ci-robot commented Oct 20, 2025

Uh oh!

haircommander commented Oct 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haircommander commented Nov 5, 2025

Uh oh!

haircommander commented Nov 6, 2025

Uh oh!

haircommander commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

haircommander commented Oct 20, 2025 •

edited

Loading