-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bring gzip compression for API server to GA #1115
Conversation
@liggitt @kubernetes/sig-api-machinery-api-reviews @kubernetes/sig-scalability-api-reviews Implementation in kubernetes/kubernetes#77449 addresses previous issues, this summarizes the choices. The implementation mitigates all previously known issues, but has a potential impact on scale at the very largest clusters which we can tune by choosing a higher limit if necessary. |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
rationale and approach seem reasonable. just one (mostly semantic) question around GA vs beta->GA, then lgtm |
This KEP proposes how we can move the API server alpha apicompression feature to GA by correcting the previous implementation.
0b0bea4
to
e2c6651
Compare
Updated |
Haven't read this yet, a quick thought before I forget-- How does this work with watches? It'd be pretty great if we did the gzip compression once and then dropped those bytes on the wire directly for every watcher of that resource, like the next level version of @wojtek-t's #924. I'm not sure if that is feasible off the top of my head but it'd be awesome if we could make it work. |
At least initially we decided to only use compression for large-enough list requests. Compressing everything was too expensive with not that huge gain. And I wouldn't like to block this feature on ensuring that this will work for sure. |
Based on recent tests gzip on small responses is a significant CPU and p99 regression. So I think watches are probably not going to be good candidates for compression in any near future, especially given that without more clever I can add that to this - "Experimentation with small gzip behavior on GET requests has shown significant p99 latency and CPU use increases, and given that watch gzip requires a more complex client implementation in order to realize delta benefits, we have no near term plans to extend this to watches. Nothing in the current approach limits our ability to explore that function in the future." |
/lgtm |
@lavalamp can you approve / give last comments? |
Ping |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, liggitt, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This KEP proposes how we can move the API server alpha api compression feature to GA by correcting the previous implementation and preventing performance regression.