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

Jump to bottom

Add a "DisableCompression" option to kubeconfig #112309

Merged
merged 2 commits into from Sep 12, 2022
Merged

Add a "DisableCompression" option to kubeconfig #112309

merged 2 commits into from Sep 12, 2022

Conversation

shyamjvs
Copy link
Member

@shyamjvs shyamjvs commented Sep 8, 2022 •
edited

What this PR does / why we need it:

Part-2 of the proposal here - #112296
/kind feature

Does this PR introduce a user-facing change?

A new "DisableCompression" field (default = false) has been added to kubeconfig under cluster info. When set to true, clients using the kubeconfig opt out of response compression for all requests to the apiserver. This can help improve list call latencies significantly when client-server network bandwidth is ample (>30MB/s) or if the server is CPU-constrained.

/assign @lavalamp @deads2k
/sig api-machinery

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 8, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 8, 2022
@leilajal
Copy link
Contributor

leilajal commented Sep 8, 2022

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 8, 2022
@lavalamp
Copy link
Member

lavalamp commented Sep 8, 2022

code looks fine, unit test is failing. Someone from SIG CLI should look at this also.

/assign @seans3

@shyamjvs
Copy link
Member Author

shyamjvs commented Sep 8, 2022 •
edited

I think I figured out the reason for failing unit test. There's also a clientauthentication.Cluster schema (seems to be used by exec provider plugin) that needs to be in sync with the clientcmdapi.Cluster schema. Updated it, hopefully works now.

@seans3 Lemme know if there are any gaps with my approach.

@k8s-ci-robot k8s-ci-robot added area/code-generation sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Sep 8, 2022
@shyamjvs
Copy link
Member Author

shyamjvs commented Sep 8, 2022

Besides pull-kubernetes-verify (which I'm looking into), others seem to be unrelated flakes

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-integration

@shyamjvs
Copy link
Member Author

shyamjvs commented Sep 9, 2022

/test pull-kubernetes-integration

@dims
Copy link
Member

dims commented Sep 9, 2022

@shyamjvs please make it clear in release notes and where ever else it is appropriate that this field is set to false by default (reflecting the current status quo). Other than that, looks good to me!

@lavalamp
Copy link
Member

lavalamp commented Sep 9, 2022

... honestly if it is easy to choose a non-go default, it would be more normal to name this in the positive (EnableCompression) and default to true.

@dims
Copy link
Member

dims commented Sep 9, 2022

@lavalamp yep, was my first instinct as well. (EnableCompression=true) and folks can switch it off.

@shyamjvs
Copy link
Member Author

shyamjvs commented Sep 9, 2022

@dims Good callout on mentioning default value. Updated the release note, thanks.

Regarding the field name DisableCompression, the reason I preferred it is because its the convention with both the RestConfig field and go http library. Also I expect compression is something clients would mostly enable and rarely disable, so making the latter explicit. Also using EnableCompression will need us to negate before passing to restConfig at few places, but if you feel strongly about it I can switch.

@dims
Copy link
Member

dims commented Sep 9, 2022

@shyamjvs good enough explanation for me :) ok from me to keep it the way it is.

@lavalamp
Copy link
Member

lavalamp commented Sep 9, 2022

I think we should double check w/ @deads2k or @liggitt -- I still slightly prefer naming things in the positive when user facing.

@shyamjvs
Copy link
Member Author

shyamjvs commented Sep 9, 2022

@deads2k @liggitt - Thoughts on the above?

@liggitt
Copy link
Member

liggitt commented Sep 12, 2022

if this were a new field in isolation, I'd maybe agree... but I think it's more usable to match the rest.Config field name and let the boolean zero value match the default (so we can omit it from serialization rather than having to default it to true)

@lavalamp
Copy link
Member

OK, I guess that's a good enough reason for me.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, shyamjvs

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 12, 2022
@enj enj added this to Needs Triage in SIG Auth Old Sep 12, 2022
@k8s-ci-robot k8s-ci-robot merged commit ed520f3 into kubernetes:master Sep 12, 2022
SIG Auth Old automation moved this from Needs Triage to Closed / Done Sep 12, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Sep 12, 2022
@shyamjvs shyamjvs deleted the disable-compression branch September 12, 2022 17:55
@shyamjvs shyamjvs mentioned this pull request Sep 19, 2022
Merged
aojea pushed a commit to aojea/kubernetes that referenced this pull request Jun 14, 2023
@mborsz @aojea
Disable compression for static pods running on master.
7582e46 
The goal is to increase throughput of LIST calls (currenetly limited by
compression) and reduce overhead which isn't needed for traffic over
localhost.

This kubeconfig option has been added in kubernetes#112309

BUG=240522929

Change-Id: I57377bccfcde06c38bfccd5fdf65a46d44623ef0
serathius pushed a commit to serathius/kubernetes that referenced this pull request Mar 14, 2024
@mborsz
Disable compression for static pods running on master.
0dbba16 
The goal is to increase throughput of LIST calls (currenetly limited by
compression) and reduce overhead which isn't needed for traffic over
localhost.

This kubeconfig option has been added in kubernetes#112309

BUG=240522929

Change-Id: I57377bccfcde06c38bfccd5fdf65a46d44623ef0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

@seans3 seans3

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG Auth Old
Closed / Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@shyamjvs @leilajal @lavalamp @dims @liggitt @k8s-ci-robot @seans3 @deads2k