Skip to content
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

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jun 25, 2019

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2019
@k8s-ci-robot k8s-ci-robot requested review from deads2k and jdumars June 25, 2019 16:40
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jun 25, 2019
@smarterclayton
Copy link
Contributor Author

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

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. labels Jun 25, 2019
@fejta-bot
Copy link

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.

@liggitt
Copy link
Member

liggitt commented Jun 26, 2019

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.
@smarterclayton
Copy link
Contributor Author

Updated

@lavalamp
Copy link
Member

lavalamp commented Jul 2, 2019

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.

@wojtek-t
Copy link
Member

wojtek-t commented Jul 3, 2019

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.

@smarterclayton
Copy link
Contributor Author

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 transfer-encoding: gzip behavior (which requires client side changes) to preserve a gzip dictionary across individual events we aren't going to see much benefit.

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."

@liggitt
Copy link
Member

liggitt commented Jul 9, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2019
@smarterclayton
Copy link
Contributor Author

@lavalamp can you approve / give last comments?

@smarterclayton
Copy link
Contributor Author

Ping

@lavalamp
Copy link
Member

lavalamp commented Sep 3, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2019
@k8s-ci-robot k8s-ci-robot merged commit 811466d into kubernetes:master Sep 3, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants