KEP-5825: update CRI List Streaming for beta graduation in v1.38#6146
KEP-5825: update CRI List Streaming for beta graduation in v1.38#6146bitoku wants to merge 1 commit into
Conversation
Update KEP metadata, PRR answers, and metrics strategy. Replace proposed streaming-specific metrics with existing kubelet CRI metrics that cover streaming transparently.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bitoku The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| An unexpected increase in list operation latency after enabling the feature | ||
| gate may indicate streaming overhead. | ||
|
|
||
| ###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested? |
There was a problem hiding this comment.
Can you do something like https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/3085-pod-conditions-for-starting-completition-of-sandbox-creation#were-upgrade-and-rollback-tested-was-the-upgrade-downgrade-upgrade-path-tested?
We want to confirm that you are testing upgrades of this feature even if manually.
| - `kubelet_runtime_operations_duration_seconds` for list operations stable: no | ||
| latency regression from streaming. | ||
|
|
||
| ###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service? |
There was a problem hiding this comment.
We have https://github.com/kubernetes/community/blob/main/sig-scalability/slos/pod_startup_latency.md#pod-startup-latency-slislo-details but I'm not sure if CRI list will impact pod startup?
I guess it shouldn't but maybe we should include that as a SLO as increased pod startup time would be concerning operationally.
| @@ -369,7 +369,8 @@ kubelet does not have integration tests. | |||
| - Feature gate enabled by default | |||
| - Both containerd and CRI-O implement streaming RPCs | |||
There was a problem hiding this comment.
Do we have containerd support for this?
There was a problem hiding this comment.
Not yet, the PR is pending.
containerd/containerd#13187
There was a problem hiding this comment.
So the requirement for beta is to have both implement this? Should we push this feature to 1.38?
I'm not as clear on CRI support for containerd skew. Even if this merges will we have a production release in time for 1.37?
There was a problem hiding this comment.
So the requirement for beta is to have both implement this?
Yes.
Should we push this feature to 1.38?
I'm not as clear on CRI support for containerd skew. Even if this merges will we have a production release in time for 1.37?
I was thinking we could merge the KEP while we're waiting for the PR merged.
However, as you said, containerd 2.4 release is scheduled to August, which is prior to 1.37 Code Freeze (end of July). We can't write test with the new containerd version, right?
I guess we may want to push to 1.38.
ref: https://containerd.io/releases/#current-state-of-containerd-releases
There was a problem hiding this comment.
if the PR is staged for a release candidate before test freeze then I think it's acceptable. But we also could defer if needed, I just think this feature is gaining in importance as we look at extending the number of pods per node cc @SergeyKanzhelev
There was a problem hiding this comment.
I disagree. We won't be able to have testing for containerd before code freeze. THere is also no test freeze anymore.
So we would be effectively going beta without testing containerd in CI.
There was a problem hiding this comment.
I think we need to wait for containerd support. It is important feature, but not urgent. I am a bit sad we didn't merge the PR to containerd in-time for 2.3
There was a problem hiding this comment.
Sure, thank you all for taking a look. I'll keep the PR open to discuss more about the design, but I'll update the KEP issue to tell it'll be delayed to 1.38.
| - Mid-stream failures already propagate as errors to the instrumented | ||
| layer and are captured by `kubelet_runtime_operations_errors_total`. | ||
| - Fallback to unary is cached after the first occurrence (via | ||
| `atomic.Bool`) and only happens a few times at startup. Kubelet logs |
There was a problem hiding this comment.
Logs are a poor way to support a feature rollout.
I did an AI review and it has this to say:
- No streaming-specific observability — this is the biggest concern
The alpha KEP originally promised dedicated streaming metrics
(kubelet_cri_list_streaming_failure_total,
kubelet_cri_list_streaming_fallback_total). The beta update removes these and
argues existing metrics are sufficient. The justification (avoiding metrics in
the cri-client staging package) is reasonable from a code-cleanliness
perspective, but operationally this means:
- An operator cannot tell whether streaming is active or the kubelet silently
fell back to unary. The only signal is a kubelet Info-level log line. At
scale, log-based detection is fragile — operators rely on metrics and
dashboards.
- You cannot distinguish a streaming mid-stream failure from any other CRI
error. kubelet_runtime_operations_errors_total is a catch-all. If streaming
introduces a new failure mode, it's invisible in the existing metric without
label enrichment.
- Recommend at minimum: a log-based metric or a label on the existing metric
(path=streaming|unary) to let operators confirm code path. If the
architectural argument holds (no metrics in cri-client), consider emitting a
kubelet-level event or condition that's scrapeable.
I still thinking something more than logs to detect fall back is important. How would you detect if this feature is misheaving on a 10000 node cluster?
There was a problem hiding this comment.
There's some difficulty having the streaming specific metrics because cri-client is a reusable client library with no metrics. Registering metrics via init() would inject kubelet-specific side effects into every importer (tests, third-party tools).
I'm exploring the possible solutions for it, and found we could do something like
kubernetes/kubernetes#139500
It doesn't affect other importers and can emit metrics from kubelet.
I'd like to hear opinions about it.
There was a problem hiding this comment.
I think this approach is reasonable and similar to other instrumented services
kannon92
left a comment
There was a problem hiding this comment.
Out of diff:
Can you fill out the checkbox for https://github.com/bitoku/kubernetes-enhancements/blob/def4672e67e92e65dc510ec5def8738c0a9df4bd/keps/sig-node/5825-cri-pagination/README.md#release-signoff-checklist
|
/hold |
Assisted-by: Claude