[cri] Implement k8s 1.23 CRI Pod Sandbox and Container Stats

[bobbypage]

• Implement new CRI RPCs - ListPodSandboxStats and PodSandboxStats
  • ListPodSandboxStats and PodSandboxStats which return stats about
    pod sandbox. To obtain pod sandbox stats, underlying metrics are
    read from the pod sandbox cgroup parent.
  • Process info is obtained by calling into the underlying task
  • Network stats are taken by looking up network metrics based on the
    pod sandbox network namespace path
• Return more detailed stats for cpu and memory for existing container
  stats. These metrics use the underlying task's metrics to obtain
  stats.

Fixes #6112
xref: kubernetes/kubernetes#102789

[k8s-ci-robot]

Hi @bobbypage. 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.

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.

[theopenlab-ci]

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

[bobbypage]

/cc @haircommander @mrunalp @qiutongs @Random-Liu @mikebrow @dims

[k8s-ci-robot]

@bobbypage: GitHub didn't allow me to request PR reviews from the following users: qiutongs, haircommander, mrunalp.

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

In response to this:

/cc @haircommander @mrunalp @qiutongs @Random-Liu @mikebrow @dims

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.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 23s (non-voting)
• containerd-test-arm64 : SUCCESS in 6m 02s (non-voting)
• containerd-integration-test-arm64 : SUCCESS in 24m 11s (non-voting)

[AkihiroSuda]

Can we use the latest commit vishvananda/netlink@187053b (20210925) ?

[bobbypage]

I actually wanted to use the tagged version - github.com/vishvananda/netlink @ v1.1.0. However, if I add

github.com/vishvananda/netlink v1.1.0

and do a go mod tidy it will change to github.com/vishvananda/netlink v1.1.1-0.20201029203352-d40f9887b852.

I think this is because it's already included in the main go.sum:

containerd/go.sum

Line 595 in 18d483b

github.com/vishvananda/netlink v1.1.1-0.20201029203352-d40f9887b852/go.mod h1:twkDnbuQxJYemMlGd4JFIcuhgX83tXhKS2B/PRMpOho=

It looks like some other deps that containerd depends on included this library such as hcsshim:

$ go mod graph | grep "d40f9887b852"
github.com/Microsoft/[email protected] github.com/vishvananda/[email protected]
github.com/containernetworking/[email protected] github.com/vishvananda/[email protected]
github.com/vishvananda/[email protected] github.com/vishvananda/[email protected]
github.com/vishvananda/[email protected] golang.org/x/[email protected]

[AkihiroSuda]

If you specify github.com/vishvananda/netlink master in go.mod and run go mod tidy, it will pick up the latest commit from the master branch

[bobbypage]

got it, thanks for tip. Do we want actually want to update github.com/vishvananda/netlink to master version though?

I ask since I'm a bit concerned since it looks like netlink is used by those other libraries and I'm not sure if master version has any breaking / unsupported changes. For the usage included in this PR, we don't need to make use of any of new functionality not pressent d40f9887b852 as far as I understand.

[AkihiroSuda]

Can be bumped up in another PR

[AkihiroSuda]

/ok-to-test

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 50s (non-voting)
• containerd-test-arm64 : SUCCESS in 6m 00s (non-voting)
• containerd-integration-test-arm64 : SUCCESS in 24m 15s (non-voting)

[mikebrow]

looking good, and thanks for all the hard work!

Needs some sort of lifecycle to remove the added container stats..

[qiutongs]

To make it easier to review, I wish you could split this PR: (1) vendor netlink (2) vendor cri-api (3) really code changes for CRI stats.

By the way, will vendoring cri-api cause circular dependency problem?

[qiutongs]

Is this set to the wrong value? After all, I suppose it should be some nano seconds.

[bobbypage]

ah, this should be set to currentUsageCoreNanoSeconds thanks, for pointing it out.

[qiutongs]

Just confirm we don't need to worry about v1alpha, right?

[bobbypage]

Yes. #5619 added support for v1. v1alpha will get converted to v1. pkg/cri/server/instrumented_service.go is responsible for v1alpha <-> v1 conversion.

[mikebrow]

nod... our CRI clients (kubelet/crictl etc..) can use either the v1alpha or v1 api, containerd v1.6 (main) exposes both sets of endpoints. We convert the v1alpha CRI calls (with the built in CRI marshalling apis) to v1 calls. Kubelet will connect to the latest version on kubelet init.. on reconnect kubelet will prefer to connect to the version is was previously connected to.

[qiutongs]

Sounds good. Thanks

[qiutongs]

Just a thought. I wonder if page cache can be added to this. I know it is not included in MemoryUsage currently but it can be useful for certain I/O heavy workload.

[bobbypage]

agree that sounds useful. That's something we can add to the CRI API, and not something we decided to add for initial implementation. But we can always add in future!

[mikebrow]

nod

[mikebrow]

if you like can add that as a // TODO (): .

[bobbypage]

Found a small issue, moving to draft...

[bobbypage]

Updated the PR, the main change is that we need to track container stats for both the sandbox and individual containers to calculate UsageNanoCores and sandbox/containers have different stores. ContainerStats need to be stored under pkg/cri/store/sandbox/sandbox.go for sandbox stats, and pkg/cri/store/container/container.go for containers.

[bobbypage]

see a couple of minor comments..

We could probably use a TODO for an integration test in here, or open an issue for same.. or maybe add a new cri-tools test? or maybe you have some output you can show from kubectl/kubelet log/k8s e2e bucket that might be wip? (for the new stats with existing api, and new apis..)

Thanks @mikebrow for reviewing. Regarding testing, agree there is need for integration tests. Me and @haircommander are planning to add K8S E2E tests that will exercise this functionality. Good idea regarding cri-tools, we should add functionality there to get pod stats and it's also a good place to add a test. I created issue in CRI-tools kubernetes-sigs/cri-tools#841

For now, I have done a manual test by starting a pod and calling both existing and new APIs. Here is the tests:

https://gist.github.com/bobbypage/b38219530a3061cc5ade218488614ab8

Let me know if that is sufficient for now. We will definitely followup with k8s E2E tests as part of this feature.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 19s (non-voting)
• containerd-test-arm64 : SUCCESS in 6m 08s (non-voting)
• containerd-integration-test-arm64 : SUCCESS in 24m 17s (non-voting)

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 53s (non-voting)
• containerd-test-arm64 : SUCCESS in 6m 00s (non-voting)
• containerd-integration-test-arm64 : SUCCESS in 24m 40s (non-voting)

[mikebrow]

see comment on the list if filter is nil question

[mikebrow]

this doesn't feel right .. normally on these CRI list functions if there is no filter they get the whole list... pls double check

[bobbypage]

I actually added this to be consistent with existing ListContainerStats implementation. For ListContainerStats, in the current implementation:

1. We create a metrics request
   

   containerd/pkg/cri/server/container_stats_list.go

   Line 34 in 11ed340

   request, containers, err := c.buildTaskMetricsRequest(in)

2. If the request is nil, we return req, nil, nil

containerd/pkg/cri/server/container_stats_list.go

Lines 79 to 85 in 11ed340

	func (c *criService) buildTaskMetricsRequest(
	r *runtime.ListContainerStatsRequest,
	) (tasks.MetricsRequest, []containerstore.Container, error) {
	var req tasks.MetricsRequest
	if r.GetFilter() == nil {
	return req, nil, nil
	}

3. We call toCRIContainerStats passing nil for containers

containerd/pkg/cri/server/container_stats_list.go

Line 42 in 11ed340

criStats, err := c.toCRIContainerStats(resp.Metrics, containers)

4. Since containers is nil, we will ultimately skip the for loop and return an empty response

containerd/pkg/cri/server/container_stats_list.go

Lines 57 to 63 in 11ed340

	containerStats := new(runtime.ListContainerStatsResponse)
	for _, cntr := range containers {
	cs, err := c.containerMetrics(cntr.Metadata, statsMap[cntr.ID])
	if err != nil {
	return nil, errors.Wrapf(err, "failed to decode container metrics for %q", cntr.ID)
	}
	containerStats.Stats = append(containerStats.Stats, cs)

As a result, I added same logic in SandboxStatsList. Let me know if that makes sense. I agree it's a bit confusing, but I thought it would be good to be consistent... let me know what you think.

[mikebrow]

nod.. thx for the discussion on slack..

nut of the discussion .. let's sync with cri-o on the provided behavior for nil and empty filter list for these type requests. Don't want to spam kubelet.. also don't want the filter behavior to be different between the container runtimes.

[bobbypage]

Thanks @mikebrow, pushed up latest revision to remove this behavior. Also filed #6198 to followup on ListContainerStats behavior.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 51s (non-voting)
• containerd-test-arm64 : SUCCESS in 6m 03s (non-voting)
• containerd-integration-test-arm64 : SUCCESS in 25m 20s (non-voting)

[mikebrow]

LGTM
windows timeout fail seems random

[bobbypage]

Thanks @mikebrow for reviewing! Created #6200 to help address the windows integration test timeout issue.

[bobbypage]

^ pushed rebase to pick up some of the test fixes

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 4m 49s (non-voting)
• containerd-test-arm64 : SUCCESS in 6m 03s (non-voting)
• containerd-integration-test-arm64 : FAILURE in 26m 46s (non-voting)

[brandond]

Are there any plans to backport this to the 1.5 branch?

[AkihiroSuda]

backport

Unlikely, as this is a new feature, not a bug fix.

[brandond]

Fair enough. I'm currently struggling with getting Kubernetes 1.23 into K3s. Between the CRI API changes and the fact that containerd, Kubernetes, and etcd all want different versions of opentracing, it's not fun.