KEP-2371: update to beta

[haircommander]

• One-line PR description:

• Issue link: cAdvisor-less, CRI-full Container and Pod Stats #2371

• Other comments:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haircommander
Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS
• keps/sig-node/OWNERS

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

[kannon92]

Will these be done before feature gate is turned on?

The requirements for PRR have changed so ideally most of the work is complete when this is promoted to beta.

[haircommander]

yeah that's my hope. The containerd side of the verification may need to be done on a prerelease version, but since a lot of the testing will be manual I think that will be okay cc @akhilerm

[akhilerm]

Yes @haircommander . Will have to manually verify and I am trying to get it merged in before the next beta of containerd 2.2

[kannon92]

• Conformance tests for the fields in /metrics/cadvisor should be created.

That sounds like the tests will be automated.

[kannon92]

• Conformance tests for the fields in /metrics/cadvisor should be created.
• Validate performance impact of this feature is within allowable margin (or non-existent, ideally).
  • The CRI stats implementation should perform better than they did with CRI+cAdvisor.
• cAdvisor stats provider will be marked as deprecated, as well as the cAdvisor providing the metrics endpoint /metrics/cadvisor.
• Write migration documentation for entities relying on metrics from /metrics/cadvisor.
• Windows stats and metrics will be added.

This seems like a lot to do for beta promotion. Is the plan to do all of this in 1.35 cycle?

It sounds like performance impact of this is also container runtime dependent so I'd expect performance numbers for CRI-O / Containerd.

[haircommander]

btw we do have e2e_node test for it now https://github.com/kubernetes/kubernetes/blob/b393d87d16f225f873f72a79734b3409323b4a05/test/e2e_node/container_metrics_test.go#L39 so we'd have to find a way to transfer to conformance
I'm dropping "Write migration documentation for entities relying on metrics from /metrics/cadvisor." as we have changed the implementation to use /metrics/cadvisor still
I am also dropping windows piece, SIG windows can do a follow-up KEP for that

[kannon92]

I guess its not clear to me what this means actually.

https://github.com/kubernetes/kubernetes/blob/b393d87d16f225f873f72a79734b3409323b4a05/test/e2e_node/container_metrics_test.go#L36

This test is marked as "NodeConformance" in test/e2e_node. Are you proposing that we move this test to test/e2e/ and move it to the conformance tests for a k8s distribution?

[kannon92]

out of diff but is this still an open question?

https://github.com/kubernetes/enhancements/blob/f04c6991969c25f117686f065bd761493e404d08/keps/sig-node/2371-cri-pod-container-stats/README.md#open-questions

[kannon92]

If there are issues with kubelet metrics provider do you think its worth exposing this in the metric?

[haircommander]

I would advocate the specific providers should report their own error metrics

[kannon92]

Commented above but if kubelet provider is not working, should we expose a metric or something?

If Kubelet is unable to post metrics on a node, it seems difficult to find this out currently.

[haircommander]

I think if the admin attempted to roll out the feature and it failed, the metric saying provider is 'cadvisor' unexpectedly would be the signal that the fallback happened

[kannon92]

That makes sense and the metric is exposed per node?

[haircommander]

yup!

[kannon92]

IMO this is still a very difficult thing for someone to detect.

The lack of any metrics reported for pods and containers is the worst case scenerio here, and would require either a rollback or for the feature gate to be disabled.

So the only way someone who find this out is if a kubelet on a node stopped posting metrics and that pod/container on that node was not found in prometheus.

That seems very complicated to tell if I had 5000 nodes.

Its worth calling out that the rollback failing would be cadvisor but if the metrics are not being posted then what is the best way to find that out? How does one find the bad node via metrics or monitoring?

[kannon92]

Please address the verify job failure.

PRR shadow:

I left some comments but overall I think it is close.

[haircommander]

thanks @kannon92 updated!

[SergeyKanzhelev]

+1 to @kannon92 comments. We need a clear transition docs that indicate how to transition. Also there should be graceful period of supporting both equally well, maybe even simultaneously. This was the goal of transition documentation goal for beta - make sure it is reviewed and we understand if we need to announce deprecation or can just suggest an easy migration for each metric

[haircommander]

I have updated based on comments. I have made a couple of things explicity:

• Windows support is out of scope
• GA of this feature will drop support for the partial CRI and cadvisor stats providers
  • there's some included motivation on this, but basically there isn't a good way to configure it today, and it was never meant to be configured. Instead, the kubelet is opinionated on what stats provider to use
  • as such, GA will be blocked on containerd 2.2 or above being the only supported containerd, and there being at least 3 releases of delay
• Sometime in the alpha, we changed the approach of /metrics/cadvisor, now having kubelet translate CRI information to that endpoint. I have tried to update the KEP to reflect this, but this means we're not moving away from metrics/cadvisor endpoint, instead changing its source

[haircommander]

Note: we chatted about this in SIG Node and agreed that the stats provider (cadvisor or partial CRI) is an implementation detail, and doesn't currently have any configuration. Introducing configuration to allow an admin to toggle whether we turn on full cri stats just to remove it in a handful of releases doesn't seem worth it. We decided to announce deprecation in beta, and move forward with it in GA when we decide to drop support for containerd < 2.2.

We also chatted with @marosset and agreed that windows support wouldn't block this KEP going to beta but we'd best-effort try to include it because this has been a sore spot for windows for a long time.

[SergeyKanzhelev]

why not have critest for metrics we want to be collected? We need some way confirming that users transition will be seamless