Disable Requesting Compression by default on restclient #48477
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @ilackarms. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ilackarms Assign the PR to them by writing Associated issue: 46966 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
size/M
ilackarms
force-pushed
the
configure-client-compression
branch
from
b4f3905 to
9d72a26
Compare
k8s-github-robot
added
size/S
|
@deads2k cc |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
ilackarms
force-pushed
the
configure-client-compression
branch
4 times, most recently
from
00f4396 to
6fb79fd
Compare
| @@ -131,6 +131,7 @@ func (config *DirectClientConfig) ClientConfig() (*restclient.Config, error) { | |||
|
|
|||
| clientConfig := &restclient.Config{} | |||
| clientConfig.Host = configClusterInfo.Server | |||
| clientConfig.DisableCompression = true | |||
There was a problem hiding this comment.
this seems odd... this is setting a non-zero value for a field in the restclient.Config that does not come from the provided or overridden config inputs... that is unique in this function, and I don't think we want it here
There was a problem hiding this comment.
the restclient.Config passes through quite a few functions before it is used to configure the transport. Is there a function you think is more appropriate for setting this in the restclient.Config?
| return http.DefaultTransport, nil | ||
| tr := http.DefaultTransport | ||
| if defaultTransport, ok := tr.(*http.Transport); ok { | ||
| defaultTransport.DisableCompression = config.DisableCompression |
There was a problem hiding this comment.
this is changing the DisableCompression setting in the shared http.DefaultTransport, which affects all users of that transport (and possibly http.DefaultClient)
There was a problem hiding this comment.
maybe instead of using http.DefaulTransport here it makes more sense to create a "Default Client" (copy the fields of http.DefaultTransport into a new *http.Transport)? then apply config.DisableCompression
ilackarms
force-pushed
the
configure-client-compression
branch
from
6fb79fd to
f7d2d07
Compare
k8s-github-robot
added
size/M
ilackarms
force-pushed
the
configure-client-compression
branch
from
f7d2d07 to
0b7a676
Compare
k8s-github-robot
added
size/L
ilackarms
force-pushed
the
configure-client-compression
branch
from
0b7a676 to
ebd0e51
Compare
k8s-github-robot
added
the
needs-rebase
ilackarms
force-pushed
the
configure-client-compression
branch
from
ebd0e51 to
69fcc76
Compare
k8s-github-robot
removed
the
needs-rebase
ilackarms
force-pushed
the
configure-client-compression
branch
from
69fcc76 to
5f41b1e
Compare
|
/retest |
|
were there any client-level issues we found with compression or did it "just work" from the client side with the default go transports? I'd like to avoid making clients care about this if possible. The main cpu hit would be on the apiserver, so the existing flag there seems sufficient to me. |
|
enabling API compression did not cause any problems for internal clients. an increased number of test flakes occurred as a result of #45666 due to increased CPU load on the master, which led to reverting #45666 in favor of #46966 which added API Compression with default-to-off feature gating. the goal of this change is to open the possibility of enabling API compression by default on the server side without causing the test flakes seen previously. |
Enabling a feature without exercising it consistently is likely to put us in the position of having a feature enabled without actually understanding the impact. I don't think pushing the burden of opting in onto every client is a great idea. There was work done on the test that started flaking that brought it back from the timeout thresholds it was dancing around. Instead of this change, I'd rather discuss enabling by default, with use by internal clients, and run the CI job enough times prior to merge to understand the runtime impact, and coordinate with the scale tests (kubemark) after merge to test it there as well. |
@liggitt i agree. in that case, closing this PR in favor #51508 where we can test the effect of enabling Compression by default. thanks for the update |
Background:
The HTTP2 Transport used by internal HTTP Clients in Kubernetes by default adds the header
Accept-Encoding: gzipto all HTTP requests (and then attempts to decompress responses that contain the headerContent-Encoding: gzip) in a way that is transparent to the caller. This behavior can be disabled using theDisableCompressionfield of thehttp.Transport. Since #46966 the kube apiserver now supports gzipping HTTP responses as per client request. Enabling this feature causes increased CPU usage as all internal clients are consuming it by default.What this PR does / why we need it:
This PR exposes
DisableCompressionand makes it configurable via the new field*restclient.Config.DisableCompression(which is set totrueby default). This should have the benefit of enabling API Compression support (e.g. for external clients) without causing unwanted CPU overhead. Internal clients that wish to request compression should be made to do so explicitly.Release note: