Implement CRI ListPodSandboxMetrics by akhilerm · Pull Request #10691 · containerd/containerd

akhilerm · 2024-09-10T16:19:39Z

Implement the following CRI APIs

ListPodSandboxMetrics
ListMetricDescriptors

Fixes: #10506

TESTING

crictl metricsp command can be used to test the pod sandbox metrics returned by the runtime.

Output

Ref: https://gist.github.com/akhilerm/625d12b805d482cd577311be3a4f7551

Part of kubernetes/enhancements#2371

* **Pod Sandbox Metrics** ([#10691](https://github.com/containerd/containerd/pull/10691))
  
  Full implementation of Kubernetes CRI pod-level metrics API
  * **ListPodSandboxMetrics**: Query metrics for  running pods/sandboxes
  * **ListMetricsDescriptors**: Discover available metrics and their descriptions

k8s-ci-robot · 2024-09-10T16:19:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akhilerm · 2024-10-22T13:25:30Z

/cc @mikebrow

zvonkok · 2024-11-28T02:12:15Z

@akhilerm You still working on this?

akhilerm · 2024-11-28T03:30:24Z

@akhilerm You still working on this?

Yepp. I am working on it. Couldnt focus for sometime, but will pickup from next week onwards.

zvonkok · 2024-11-28T03:49:48Z

/cc @zvonkok

k8s-ci-robot · 2024-11-28T03:49:50Z

@zvonkok: GitHub didn't allow me to request PR reviews from the following users: zvonkok.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @zvonkok

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-sigs/prow repository.

zvonkok · 2024-12-16T19:20:46Z

@akhilerm Is this still on your roadmap?

zvonkok · 2024-12-20T21:14:16Z

AFAIC some metrics are missing if we want to be on par with cAdvisor.

akhilerm · 2024-12-22T05:06:08Z

@akhilerm Is this still on your roadmap?

Yes

AFAIC some metrics are missing if we want to be on par with cAdvisor.

The ListMetricDescriptors should have every metrics and this was added over from cadvisor. I will cross check if I missed something while adding from the cadvisor code.

zvonkok · 2025-01-01T20:22:49Z

@akhilerm I mostly referring to this table: https://github.com/google/cadvisor/blob/master/docs/storage/prometheus.md AFAIR the FS were missing?

zvonkok · 2025-01-09T21:59:58Z

This is the e2e test which metrics we will need: kubernetes/kubernetes#126213

AkihiroSuda · 2025-09-26T06:39:10Z

+	var mu sync.Mutex
+	var wg sync.WaitGroup
+
+	semaphore := make(chan struct{}, 10) // Limit to 10 concurrent goroutines


nit: https://pkg.go.dev/golang.org/x/sync/errgroup#Group.SetLimit might be easier to use

@AkihiroSuda can you take a look again?

AkihiroSuda · 2025-09-26T06:40:07Z

+					Name:        "container_network_receive_bytes_total",
+					Timestamp:   timestamp,
+					MetricType:  runtime.MetricType_COUNTER,
+					LabelValues: append(podLabels, "eth0"),


Is the network device name guaranteed to be "eth0" ?
Also, don't we need to care about other network devices?

@aojea isn't the kubernetes convention to default the pod network device interface name to eth0

Copilot

Pull Request Overview

This PR implements the CRI ListPodSandboxMetrics and ListMetricDescriptors APIs in containerd, enabling monitoring systems to collect pod and container metrics from the CRI runtime. The implementation provides comprehensive metrics for CPU, memory, network, disk I/O, filesystem, and process statistics.

Implements ListPodSandboxMetrics with concurrent collection of metrics for pod sandboxes and their containers
Implements ListMetricDescriptors to provide metadata about available metrics
Enhances network statistics collection to include packet counts and dropped packets

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/cri/server/sandbox_stats_linux.go	Enhanced getContainerNetIO function to collect additional network statistics (packets, dropped packets)
internal/cri/server/list_pod_sandbox_metrics_other.go	Platform stub for non-Linux systems returning unimplemented error
internal/cri/server/list_pod_sandbox_metrics_linux.go	Main implementation of ListPodSandboxMetrics with comprehensive metrics collection
internal/cri/server/list_pod_sandbox_metrics.go	Removed generic stub implementation, now handled by platform-specific files
internal/cri/server/list_metric_descriptors_other.go	Platform stub for non-Linux systems returning unimplemented error
internal/cri/server/list_metric_descriptors_linux.go	Implementation of ListMetricDescriptors with metric definitions and descriptions
internal/cri/server/list_metric_descriptors.go	Defined metric category constants for organizing metric types
go.mod	Moved golang.org/x/time from indirect to direct dependency for rate limiting

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-10T05:09:50Z

+)
+
+// Rate limiter to prevent overwhelming the system with concurrent requests
+var limiter = rate.NewLimiter(rate.Limit(10), 10) // Allow 10 concurrent requests with burst of 10


The rate limiter is a global variable with hardcoded values. Consider making this configurable or using per-service instance variables to avoid global state.

Copilot · 2025-10-10T05:09:51Z

+	var mu sync.Mutex
+	var wg sync.WaitGroup
+
+	semaphore := make(chan struct{}, 10) // Limit to 10 concurrent goroutines


The semaphore size is hardcoded to 10. This should match the rate limiter configuration or be made configurable to maintain consistency.

mikebrow · 2025-10-17T19:46:06Z

all green ... but needs rebase..

akhilerm · 2025-10-19T14:12:18Z

all green ... but needs rebase..

I will rebase and also push the change to move to errGroup

akhilerm · 2025-10-20T09:42:32Z

Rebased and made all the requested changes.

mxpv · 2025-10-20T23:17:39Z


 		if sandbox.NetNSPath != "" {
-			rxBytes, rxErrors, txBytes, txErrors := getContainerNetIO(ctx, sandbox.NetNSPath)
+			rxBytes, rxErrors, txBytes, txErrors, rxPackets, rxDropped, txPackets, txDropped := getContainerNetIO(ctx, sandbox.NetNSPath)


This function returns 8 values. Should we introduce a struct instead?

since the getContainerNetIO was used in the container_stats also, I didnt want to introduce a change there. Also, in the initial implementation, we had used a struct , ref: #10691 (comment), but was later removed based on the comments

mxpv · 2025-10-20T23:24:20Z

+	}
+
+	// Use a default namespace if we can't determine it
+	namespace := "default"


What if it's a different namespace?

We can default to k8s.io namespace as that will be the one used by CRI service.

Have updated to use k8s.io always.

Hardcoding namespace might not be the best approach here.
You probably can retrieve the root directory path via shim manager.

type ShimInstance interface { ID() string Namespace() string Bundle() string // Returns bundle path // ... } shim, err := shimManager.Get(ctx, containerID) if err != nil { return err } bundlePath := shim.Bundle() rootPath := filepath.Join(bundlePath, "rootfs")

Or another way is to expose it in tasks's state:

state, err := task.State(ctx) if err != nil { return err } state.BundlePath

We need to find a way to get the rootfs path without leaking the runtime specific details into the cri service.

Signed-off-by: Akhil Mohan <akhilerm@gmail.com> Signed-off-by: Davanum Srinivas <davanum@gmail.com>

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

Signed-off-by: Akhil Mohan <akhilerm@gmail.com> Signed-off-by: Davanum Srinivas <davanum@gmail.com>

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

mikebrow

LGTM

dims

LGTM

akhilerm · 2025-10-24T10:28:24Z

A few more additional metrics will be added as mentioned in kubernetes-sigs/cri-tools#1931 for the conformance

mxpv · 2025-10-24T17:21:49Z

@akhilerm could you pls create a follow up issue to address #10691 (comment)

k8s-ci-robot added do-not-merge/work-in-progress size/XS labels Sep 10, 2024

akhilerm force-pushed the list-pod-sandbox-metrics branch from 2981bb8 to 3b7a1b7 Compare September 24, 2024 03:13

k8s-ci-robot added size/S size/L and removed size/XS size/S labels Sep 24, 2024

akhilerm force-pushed the list-pod-sandbox-metrics branch 2 times, most recently from e07f859 to 55a5d80 Compare October 21, 2024 18:06

k8s-ci-robot requested a review from mikebrow October 22, 2024 13:25

akhilerm force-pushed the list-pod-sandbox-metrics branch from b0830f6 to 44e5695 Compare October 25, 2024 06:27

mengjiao-liu mentioned this pull request Nov 27, 2024

[CRI] The ListPodSandboxMetrics method in containerd is not yet implemented #10506

Closed

haircommander mentioned this pull request Dec 18, 2024

cAdvisor-less, CRI-full Container and Pod Stats kubernetes/enhancements#2371

Open

16 tasks

akhilerm force-pushed the list-pod-sandbox-metrics branch from 44e5695 to 0b4a872 Compare December 31, 2024 09:22

k8s-ci-robot added size/S and removed size/L labels Dec 31, 2024

akhilerm force-pushed the list-pod-sandbox-metrics branch from 0b4a872 to 41203bc Compare January 7, 2025 15:25

k8s-ci-robot added size/L and removed size/S labels Jan 7, 2025

AkihiroSuda reviewed Sep 26, 2025

View reviewed changes

chrishenzie reviewed Sep 26, 2025

View reviewed changes

Comment thread internal/cri/server/list_pod_sandbox_metrics_linux.go Outdated

k8s-ci-robot added the needs-rebase label Oct 8, 2025

akhilerm force-pushed the list-pod-sandbox-metrics branch from 52fe271 to 268e529 Compare October 10, 2025 02:22

k8s-ci-robot removed the needs-rebase label Oct 10, 2025

Copilot AI reviewed Oct 10, 2025

View reviewed changes

mxpv reviewed Oct 20, 2025

View reviewed changes

akhilerm added 7 commits October 22, 2025 09:40

Implement CRI ListPodSandboxMetrics

942d7af

Signed-off-by: Akhil Mohan <akhilerm@gmail.com> Signed-off-by: Davanum Srinivas <davanum@gmail.com>

add cpu metrics

1bd3b45

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

implement CRI ListMetricDescriptors

63eca8f

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

add memory metrics

68beb81

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

add network metrics

2e58d4c

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

add disk and diskIO metrics

7da6a9c

Signed-off-by: Akhil Mohan <akhilerm@gmail.com> Signed-off-by: Davanum Srinivas <davanum@gmail.com>

add process metrics

a418e28

Signed-off-by: Akhil Mohan <akhilerm@gmail.com>

mikebrow approved these changes Oct 22, 2025

View reviewed changes

dmcgowan approved these changes Oct 23, 2025

View reviewed changes

dims approved these changes Oct 23, 2025

View reviewed changes

This was referenced Oct 26, 2025

Use shimManager or task to get the bundle root directory path #12414

Open

Prepare release notes for v2.2.0-rc.0 #12413

Merged

add missing podsandbox metrics for CRI conformance #12426

Merged

dmcgowan mentioned this pull request Nov 5, 2025

Prepare release notes for v2.2.0 #12473

Merged

fionera mentioned this pull request Nov 29, 2025

fix: refactor ListPodSandboxMetrics #12594

Merged

dims mentioned this pull request Dec 4, 2025

[WIP] Re-enable ResourceMetrics for node-e2e tests #12620

Closed

ringerc mentioned this pull request Dec 31, 2025

container_oom_events_total always returns 0 google/cadvisor#3015

Closed

Conversation

akhilerm commented Sep 10, 2024 • edited by dmcgowan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TESTING

Output

Uh oh!

k8s-ci-robot commented Sep 10, 2024

Uh oh!

akhilerm commented Oct 22, 2024

Uh oh!

zvonkok commented Nov 28, 2024

Uh oh!

akhilerm commented Nov 28, 2024

Uh oh!

zvonkok commented Nov 28, 2024

Uh oh!

k8s-ci-robot commented Nov 28, 2024

Uh oh!

zvonkok commented Dec 16, 2024

Uh oh!

zvonkok commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akhilerm commented Dec 22, 2024

Uh oh!

zvonkok commented Jan 1, 2025

Uh oh!

zvonkok commented Jan 9, 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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Oct 17, 2025

Uh oh!

akhilerm commented Oct 19, 2025

Uh oh!

akhilerm 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

dims left a comment

Choose a reason for hiding this comment

akhilerm commented Sep 10, 2024 •

edited by dmcgowan

Loading

zvonkok commented Dec 20, 2024 •

edited

Loading

akhilerm Oct 22, 2025 •

edited

Loading

mxpv Oct 22, 2025 •

edited

Loading