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

kubectl: fix timeout=32s for some rest APIs when --request-timeout=0 #103619

Closed
wants to merge 1 commit into from
Closed

kubectl: fix timeout=32s for some rest APIs when --request-timeout=0 #103619

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 0 additions & 6 deletions staging/src/k8s.io/client-go/discovery/discovery_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ const (
defaultRetries = 2
// protobuf mime type
mimePb = "application/com.github.proto-openapi.spec.v2@v1.0+protobuf"
// defaultTimeout is the maximum amount of time per request when no timeout has been set on a RESTClient.
// Defaults to 32s in order to have a distinguishable length of time, relative to other timeouts that exist.
defaultTimeout = 32 * time.Second
)

// DiscoveryInterface holds the methods that discover server-supported API groups,
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't remove the default timeout to preserve backwards compatibility.

What if we add -1 for no timeout instead?

Copy link
Member

Choose a reason for hiding this comment

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

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'

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I believe this is a doc-code dismatch. Other places have the correct behavior while only this one misbehaves.

Copy link
Author

@BoleynSu BoleynSu Jul 20, 2021 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jpbetz jpbetz Jul 26, 2021 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Member

@liggitt liggitt Jul 28, 2021 •
edited

Choose a reason for hiding this comment

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

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)

config.Timeout = defaultTimeout
}
if config.Burst == 0 && config.QPS < 100 {
// discovery is expected to be bursty, increase the default burst
// to accommodate looking up resource info for many API groups.
Expand Down