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.

Sign up for GitHub

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 --disable-compression flag to kubectl #112580

Merged
merged 2 commits into from Sep 21, 2022
Merged

Add --disable-compression flag to kubectl #112580

merged 2 commits into from Sep 21, 2022

Conversation

shyamjvs
Copy link
Member

@shyamjvs shyamjvs commented Sep 19, 2022 •
edited

What this PR does / why we need it:

Follow-up of #112309, and part-3 of the proposal in #112296.

Does this PR introduce a user-facing change?

A new --disable-compression flag has been added to kubectl (default = false). When true, it opts 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.

/sig cli
/kind feature
/assign @liggitt @seans3

@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 19, 2022
@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. 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. labels Sep 19, 2022
@k8s-ci-robot
Copy link
Contributor

@shyamjvs: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Sep 19, 2022
@shyamjvs
Copy link
Member Author

/test pull-kubernetes-e2e-kind-ipv6

liggitt
liggitt reviewed Sep 20, 2022
View reviewed changes
test/cmd/get.sh Outdated
# Commands to create 3 configmaps each of size 50KB. Sum of their sizes should be >128KB (defaultGzipThresholdBytes)
# for apiserver to allow gzip compression of the response. This is required to test the disable-compression option
# from client-side
some_string=$(openssl rand -base64 50000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is there a way to get random content that doesn't require openssl? I'm not 100% sure that is accessible in all testing envs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point and I had this same question. But looks like it's already used by some auth-related tests -

kubernetes/test/cmd/authentication.sh

Lines 57 to 61 in 19935de

### Provided --token should take precedence, thus not triggering the (invalid) exec credential plugin
# Pre-condition: Client certificate authentication enabled on the API server
kube::util::test_client_certificate_authentication_enabled
# Command
output=$(kubectl "${kube_flags_with_token[@]:?}" --kubeconfig="${TMPDIR:-/tmp}"/invalid_exec_plugin.yaml get namespace kube-system -o name || true)

And we check there that openssl should be installed - https://github.com/kubernetes/kubernetes/blob/master/hack/lib/util.sh#L474

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like dd status=none if=/dev/random bs=1K count=50 | base64 might still be lighter weight

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks for the lead.

Also I used /dev/urandom instead of /dev/random as it seems like reading the latter could be a blocking operation on some OS and output of the former suffices our purpose - https://stackoverflow.com/questions/23712581/differences-between-random-and-urandom

@shyamjvs
Copy link
Member Author

/retest

@liggitt
Copy link
Member

liggitt commented Sep 21, 2022

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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 21, 2022
@k8s-ci-robot k8s-ci-robot merged commit b8e740f into kubernetes:master Sep 21, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Sep 21, 2022
@shyamjvs shyamjvs deleted the disable-compression branch September 21, 2022 15:45
@sftim
Copy link
Contributor

sftim commented Oct 10, 2022

For the release note, I recommend this text:

A new `--disable-compression` flag has been added to kubectl (default value is `false`). When `true`, kubectl opts out of response compression for all requests to the API server. This can help improve list call latencies significantly when client-server network bandwidth is ample (>30MB/s), or if the server is CPU-constrained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@brianpursley brianpursley Awaiting requested review from brianpursley

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@liggitt liggitt

@seans3 seans3

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test 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. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shyamjvs @k8s-ci-robot @liggitt @sftim @seans3