kubectl: fix timeout=32s for some rest APIs when --request-timeout=0 #103619
Conversation
According to `kubectl options`, for `--request-timeout`, a value of zero means don't timeout requests. This PR fixes the wrong behavior.
k8s-ci-robot
added
release-note
|
Welcome @BoleynSu! |
|
Hi @BoleynSu. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
needs-priority
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BoleynSu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This patch is to correct the current behavior. However, as it has been there for 3 years, it is very likely that someone is already depending on the current behavior. If we do not want to change the current behavior, maybe we can change the usage message instead? |
k8s-ci-robot
added
the
triage/accepted
k8s-ci-robot
removed
the
needs-triage
| @@ -462,9 +459,6 @@ func withRetries(maxRetries int, f func() ([]*metav1.APIGroup, []*metav1.APIReso | |||
| func setDiscoveryDefaults(config *restclient.Config) error { | |||
| config.APIPath = "" | |||
| config.GroupVersion = nil | |||
| if config.Timeout == 0 { | |||
There was a problem hiding this comment.
We shouldn't remove the default timeout to preserve backwards compatibility.
What if we add -1 for no timeout instead?
There was a problem hiding this comment.
Actually I think this is already the current behavior?
kubectl get nodes --request-timeout=-1s -v10
...
I0719 14:20:47.145520 41053 round_trippers.go:435] curl ... foo.com/api/v1/nodes?limit=500'
There was a problem hiding this comment.
it looks like it's an issue specific to the kubectl documentation. Unless there is a mismatch between the client-go documentation and client-go behavior, we shouldn't change client-go.
There was a problem hiding this comment.
I believe this is a doc-code dismatch. Other places have the correct behavior while only this one misbehaves.
There was a problem hiding this comment.
kubectl options shows the following.
--request-timeout='0': The length of time to wait before giving up on a single server request.
Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means
don't timeout requests.
Note that I did not really check if other places work as documented. Only that when searching the codebase for this particular flag I did not find any other code using it wrongly.
it looks like it's an issue specific to the kubectl documentation. Unless there is a mismatch between the client-go documentation and client-go behavior, we shouldn't change client-go.
In staging/src/k8s.io/client-gorest/config.go, we also have
// The maximum length of time to wait before giving up on a server request. A value of zero means no timeout.
Timeout time.Duration
There was a problem hiding this comment.
+1 to @roycaihw's comment. If we want to respect the --request-timeout documentation in kubectl (which seems reasonable) we should do it by passing in a value to client-go to tell it to not timeout. We cannot delete (or otherwise change) the default in client-go (which is used by more clients than just kubectl), since that would be a breaking change to the other clients.
There was a problem hiding this comment.
I agree with the comments about driving behavior from the kubectl side, rather than here
even in kubectl, I'm not sure removing timeout entirely makes sense... the server will still time out eventually, regardless of what the client does (and at around 30 seconds for short-lived requests, I think, at least for REST API requests)
|
/sig api-machinery |
|
@jpbetz Please check the last paragraph in the comment above yours.
Sent from https://boleyn.su/phone
…On Tue, Jul 27, 2021, 03:37 Joe Betz ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In staging/src/k8s.io/client-go/discovery/discovery_client.go
<#103619 (comment)>
:
> @@ -462,9 +459,6 @@ func withRetries(maxRetries int, f func() ([]*metav1.APIGroup, []*metav1.APIReso
func setDiscoveryDefaults(config *restclient.Config) error {
config.APIPath = ""
config.GroupVersion = nil
- if config.Timeout == 0 {
+1 to @roycaihw <https://github.com/roycaihw>'s comment. If we want to
respect the --request-timeout documentation in kubectl (which seems
reasonable) we should do it by passing in a value to client-go to tell it
to not timeout. We cannot change the default in client-go (which is used by
more clients than just kubectl), since that would be a breaking change to
the other clients.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103619 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXVSKBF6Q6XJAMZJX3YZEDTZW2QXANCNFSM5ADGO4JA>
.
|
|
I ran through this in a debugger and get what is happening now. @BoleynSu apologies for not understanding earlier. To summarize: When running The callstack looks like this:
As it stands now we aren't able to set discovery clients (created by We aren't able to remove the default or make it a pointer but maybe we can pass metadata that the timeout was set in the config (like @jpbetz suggested above) or change the expectation that a -1 one be used for no timeout. kubernetes/staging/src/k8s.io/client-go/rest/config.go Lines 131 to 132 in d92b788 |
|
IIUC, they are only for create/patch/... which change the data, not for
retrieving data. @liggitt
Sent from https://boleyn.su/phone
…On Thu, Jul 29, 2021, 03:19 Jordan Liggitt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In staging/src/k8s.io/client-go/discovery/discovery_client.go
<#103619 (comment)>
:
> @@ -462,9 +459,6 @@ func withRetries(maxRetries int, f func() ([]*metav1.APIGroup, []*metav1.APIReso
func setDiscoveryDefaults(config *restclient.Config) error {
config.APIPath = ""
config.GroupVersion = nil
- if config.Timeout == 0 {
I'm not sure removing timeout entirely makes sense... the server will
still time out eventually, regardless of what the client does (and at
around 30 seconds for short-lived requests
<https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go#L57>,
I think, at least for REST API requests)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103619 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXVSKA3TSS7JY6OBRIPCRTT2BJ5JANCNFSM5ADGO4JA>
.
|
get requests still have a server-side upper bound timeout (defaulting to 60 seconds, I think) |
|
@liggitt I think we only set a timeout for watch requests in get.go.
My request to the API server over a very slow connection should timeout
whatever parameters I use for the kubectl cli if what you think is true.
But it did not.
Sent from https://boleyn.su/phone
…On Thu, Jul 29, 2021, 21:44 Jordan Liggitt ***@***.***> wrote:
IIUC, they are only for create/patch/... which change the data, not for
retrieving data. @liggitt <https://github.com/liggitt>
get requests still have a server-side upper bound timeout (defaulting to
60 seconds, I think)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103619 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXVSKDKX4IWLUU6A5OEFMDT2FLMTANCNFSM5ADGO4JA>
.
|
|
Friendly ping. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
Friendly ping. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
@fedebongio this probably needs an additional assignee? |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
According to
kubectl options, for--request-timeout, a valueof zero means don't timeout requests. This PR fixes the wrong
behavior.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Please check #103618
Which issue(s) this PR fixes:
Fixes #103618
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: