[serathius]

/kind feature

Implements proposal from #129304 where streaming json respones reduced the memory used by apiserver 3-6x times. Validated on configmaps and pods objects with 5 concurrent list each returning 1.5GB of data.

Improved how the API server responds to **list** requests where the response format negotiates to JSON. List responses in JSON are marshalled one element at the time, drastically reducing memory needed to serve large collections. Streaming list responses can be disabled via the `StreamingJSONListEncoding` feature gate.

/cc @mborsz @wojtek-t @jpbetz @deads2k @p0lyn0mial

[sftim]

💭 If we wanted to do streaming encoding to YAML or CBOR, is there anything in this code making the YAML / CBOR option hard to do?

[sftim]

Changelog suggestion

-List responses in JSON are marshalled one element at the time, drastically reducing memory needed to serve large responses. Can be disabled via StreamingJSONListEncoding feature flag
+Improved how the API server responds to **list** requests where the response format negotiates to JSON. List responses in JSON are marshalled one element at the time, drastically reducing memory needed to serve large collections. Streaming list responses can be disabled via the `StreamingJSONListEncoding` feature gate.

(will need adjusting if the feature gate name changes per my opinion / suggestion)

[serathius]

If we wanted to do streaming encoding to YAML or CBOR, is there anything in this code making the YAML / CBOR option hard to do?

Don't know, my focus is on JSON for CRDs and than Proto for built in types. I would need to get some help for the others.

[jpbetz]

Would it be possible to backport this to supported versions?

+1 for this since kube-apiserver's memory issue triggers a lot of incidents in different companies as I heard. (e.g. https://status.openai.com/incidents/ctrsv3lwd797)

This does not meet backport criteria. See also #129334 (comment)

[sftim]

Anyone who wants to do their own backport could do that, right? As a project we'd not object to someone patching their own API server code.

[anson627]

@serathius this change is going to solve a lot API server OOMKilled for many k8s use cases, cannot wait to adopt this for millions of clusters

I'm trying to address the comments on unit test but cannot create PR based off your PR, so I opened a new one: 5f8c1c8

[serathius]

@anson627 @haosdent Thanks for taking initiative, however as described in #129334 (comment) the work has grown beyond just this PR.

The work for streaming encoding was upgraded from a single PR to a full KEP. To understand the next steps you can read the design https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/5116-streaming-response-encoding/README.md and follow the implementation plan in kubernetes/enhancements#5116

[anson627]

thanks @serathius! I will take a closer look at the plan, and see where and how I can help

[serathius]

Updated PR, still need to wait for merging:

• Implement chunking for gzip encoder in deferredResponseWriter #130281
• Add tests for encoding collections in JSON for KEP-5116 #130220

[serathius]

The PR is ready for re-review @liggitt

[serathius]

/retest

[liggitt]

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: 12bd3e248c3fe40c2d548ba8821c72b363229964

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fuweid, liggitt, serathius

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/features/OWNERS [liggitt]
• pkg/registry/OWNERS [liggitt]
• staging/src/k8s.io/apiextensions-apiserver/OWNERS [liggitt]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [liggitt]
• staging/src/k8s.io/apiserver/OWNERS [liggitt]
• test/featuregates_linter/test_data/OWNERS [liggitt]

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

[serathius]

https://perf-dash.k8s.io/#/?jobname=benchmark%20list&metriccategoryname=E2E&metricname=Resources&Resource=memory&PodName=kube-apiserver-benchmark-list-master%2Fkube-apiserver###

[songminglong]

we tested in our env: list 6000 configmaps with size 150kb

Conclusion： under json list streaming conditions, gzip may be more time-consuming

before the mr #130281

//total durations
Trace[1274966822]: ["cacher list" audit-id:3c8cb522-d8b6-4a0b-ad1a-265dd3e5ecb3,type:configmaps 91429ms

// for example, one of steam
size:150373,firstWrite:false 8ms

after the mr #130281

//total durations:
Trace[574860878]: ["cacher list" audit-id:70a49c16-fd5c-4ddb-aec4-35eebd5249f7,type:configmaps 104139ms

// and one of each per stream
size:150373,firstWrite:false 13ms

[serathius]

10% difference seems ok. Our goal was to reduce memory usage, doing so is already a tradeoff that increases CPU usage.

[haosdent]

10% difference seems ok. Our goal was to reduce memory usage, doing so is already a tradeoff that increases CPU usage.

Agree, users could toggle off this if they don't want to compromise on latency

[songminglong]

10% difference seems ok. Our goal was to reduce memory usage, doing so is already a tradeoff that increases CPU usage.

I agree with this tradeoff