Set a default request timeout for discovery client #62733

soltysh · 2018-04-17T15:02:54Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1546117

Adds a default request timeout to requests made by the discovery client.
This prevents a command from hanging indefinitely due to one or multiple calls
to the apiserver taking longer than usual when when a --request-timeout flag value
has not been set.

/assign @deads2k @juanvallejo

Release note:

NONE

deads2k · 2018-04-17T15:09:35Z

staging/src/k8s.io/client-go/rest/request.go

@@ -649,6 +648,12 @@ func (r *Request) request(fn func(*http.Request, *http.Response)) error {
 		if err != nil {
 			return err
 		}
+		if r.timeout > 0 {


update doc on the Timeout method.

deads2k · 2018-04-17T15:10:09Z

staging/src/k8s.io/client-go/discovery/discovery_client.go

+	// 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.
+	// It's a variable to be able to change it in tests.
+	DefaultTimeout = 32 * time.Second


can be private, right?

I'd wish, the tests are explicitly using discover_test package to separate from this package, so I had to make public. I don't know the reasoning behind it, but I didn't want to change it without syncing with you about it.

liggitt · 2018-04-17T15:16:38Z

staging/src/k8s.io/client-go/rest/request.go

+			if r.ctx == nil {
+				r.ctx = context.Background()
+			}
+			r.ctx, _ = context.WithTimeout(r.ctx, r.timeout)


this looks like it's applying the timeout per attempt... does this mean a timeout of 10 seconds could get repeated 10 times, resulting in 100 seconds before this returned? I'm not sure that's what the "Change to a timeout based approach." TODO above intended.

ah, WithTimeout -> WithDeadline, which doesn't lengthen the deadline, so that's ok.

I don't think it's correct to discard the cancelfunc though:

Canceling this context releases resources associated with it, so code should call cancel as soon as the operations running in this Context complete

cancel must be called.

cancel must be called.

Hmm... I had some doubts about that, but looking at it again it does make sense to have it there.

hzxuzhonghu · 2018-04-18T07:20:33Z

when when a --request-timeout flag value
has not been set.

IIRC, the default value is 60s.

soltysh · 2018-04-19T09:46:45Z

IIRC, the default value is 60s.

Nope default is 0 (see here), so not set. This PR defaults it ONLY for discovery.

soltysh · 2018-04-19T13:07:06Z

Comments addressed.

deads2k · 2018-04-19T13:11:04Z

staging/src/k8s.io/client-go/rest/request.go

+			}
+			var cancelFn context.CancelFunc
+			r.ctx, cancelFn = context.WithTimeout(r.ctx, r.timeout)
+			defer cancelFn()


I think this works, but it will wait until all the retries are done. I'm fine with it since it eventually cleans up. I'll give some time if people really want to argue that it all ends up as an anonymous function to clean up sooner in exceptional cases. I can't see it mattering much.

deads2k · 2018-04-19T21:47:57Z

lgtm, but get green

/approve

soltysh · 2018-04-20T15:11:41Z

/retest

deads2k · 2018-04-20T16:40:18Z

/lgtm

k8s-ci-robot · 2018-04-20T16:40:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, soltysh

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-04-20T20:39:08Z

Automatic merge from submit-queue (batch tested with PRs 62876, 62733, 62827). If you want to cherry-pick this change to another branch, please follow the instructions here.

sttts · 2018-04-24T14:25:16Z

staging/src/k8s.io/client-go/discovery/discovery_client_test.go

+	client := NewDiscoveryClientForConfigOrDie(&restclient.Config{Host: server.URL})
+	_, err := client.ServerGroups()
+	if err == nil || strings.Contains(err.Error(), "deadline") {
+		t.Fatalf("unexpected error: %v", err)


this flakes, especially it broke the publishing bot:

+ go test k8s.io/client-go/discovery k8s.io/client-go/discovery/cached k8s.io/client-go/discovery/fake k8s.io/client-go/dynamic k8s.io/client-go/dynamic/fake k8s.io/client-go/examples/create-update-delete-deployment k8s.io/client-go/examples/in-cluster-client-configuration k8s.io/client-go/examples/out-of-cluster-client-configuration k8s.io/client-go/examples/workqueue k8s.io/client-go/informers k8s.io/client-go/informers/admissionregistration k8s.io/client-go/informers/admissionregistration/v1alpha1 k8s.io/client-go/informers/admissionregistration/v1beta1 k8s.io/client-go/informers/apps k8s.io/client-go/informers/apps/v1 k8s.io/client-go/informers/apps/v1beta1 k8s.io/client-go/informers/apps/v1beta2 k8s.io/client-go/informers/autoscaling k8s.io/client-go/informers/autoscaling/v1 k8s.io/client-go/informers/autoscaling/v2beta1 k8s.io/client-go/informers/batch k8s.io/client-go/informers/batch/v1 k8s.io/client-go/informers/batch/v1beta1 k8s.io/client-go/informers/batch/v2alpha1 k8s.io/client-go/informers/certificates k8s.io/client-go/informers/certificates/v1beta1 k8s.io/client-go/informers/core k8s.io/client-go/informers/core/v1 k8s.io/client-go/informers/events k8s.io/client-go/informers/events/v1beta1 k8s.io/client-go/informers/extensions k8s.io/client-go/informers/extensions/v1beta1 k8s.io/client-go/informers/internalinterfaces k8s.io/client-go/informers/networking k8s.io/client-go/informers/networking/v1 k8s.io/client-go/informers/policy k8s.io/client-go/informers/policy/v1beta1 k8s.io/client-go/informers/rbac k8s.io/client-go/informers/rbac/v1 k8s.io/client-go/informers/rbac/v1alpha1 k8s.io/client-go/informers/rbac/v1beta1 k8s.io/client-go/informers/scheduling k8s.io/client-go/informers/scheduling/v1alpha1 k8s.io/client-go/informers/settings k8s.io/client-go/informers/settings/v1alpha1 k8s.io/client-go/informers/storage k8s.io/client-go/informers/storage/v1 k8s.io/client-go/informers/storage/v1alpha1 k8s.io/client-go/informers/storage/v1beta1 k8s.io/client-go/kubernetes k8s.io/client-go/kubernetes/fake k8s.io/client-go/kubernetes/scheme k8s.io/client-go/kubernetes/typed/admissionregistration/v1alpha1 k8s.io/client-go/kubernetes/typed/admissionregistration/v1alpha1/fake k8s.io/client-go/kubernetes/typed/admissionregistration/v1beta1 k8s.io/client-go/kubernetes/typed/admissionregistration/v1beta1/fake k8s.io/client-go/kubernetes/typed/apps/v1 k8s.io/client-go/kubernetes/typed/apps/v1/fake k8s.io/client-go/kubernetes/typed/apps/v1beta1 k8s.io/client-go/kubernetes/typed/apps/v1beta1/fake k8s.io/client-go/kubernetes/typed/apps/v1beta2 k8s.io/client-go/kubernetes/typed/apps/v1beta2/fake k8s.io/client-go/kubernetes/typed/authentication/v1 k8s.io/client-go/kubernetes/typed/authentication/v1/fake k8s.io/client-go/kubernetes/typed/authentication/v1beta1 k8s.io/client-go/kubernetes/typed/authentication/v1beta1/fake k8s.io/client-go/kubernetes/typed/authorization/v1 k8s.io/client-go/kubernetes/typed/authorization/v1/fake k8s.io/client-go/kubernetes/typed/authorization/v1beta1 k8s.io/client-go/kubernetes/typed/authorization/v1beta1/fake k8s.io/client-go/kubernetes/typed/autoscaling/v1 k8s.io/client-go/kubernetes/typed/autoscaling/v1/fake k8s.io/client-go/kubernetes/typed/autoscaling/v2beta1 k8s.io/client-go/kubernetes/typed/autoscaling/v2beta1/fake k8s.io/client-go/kubernetes/typed/batch/v1 k8s.io/client-go/kubernetes/typed/batch/v1/fake k8s.io/client-go/kubernetes/typed/batch/v1beta1 k8s.io/client-go/kubernetes/typed/batch/v1beta1/fake k8s.io/client-go/kubernetes/typed/batch/v2alpha1 k8s.io/client-go/kubernetes/typed/batch/v2alpha1/fake k8s.io/client-go/kubernetes/typed/certificates/v1beta1 k8s.io/client-go/kubernetes/typed/certificates/v1beta1/fake k8s.io/client-go/kubernetes/typed/core/v1 k8s.io/client-go/kubernetes/typed/core/v1/fake k8s.io/client-go/kubernetes/typed/events/v1beta1 k8s.io/client-go/kubernetes/typed/events/v1beta1/fake k8s.io/client-go/kubernetes/typed/extensions/v1beta1 k8s.io/client-go/kubernetes/typed/extensions/v1beta1/fake k8s.io/client-go/kubernetes/typed/networking/v1 k8s.io/client-go/kubernetes/typed/networking/v1/fake k8s.io/client-go/kubernetes/typed/policy/v1beta1 k8s.io/client-go/kubernetes/typed/policy/v1beta1/fake k8s.io/client-go/kubernetes/typed/rbac/v1 k8s.io/client-go/kubernetes/typed/rbac/v1/fake k8s.io/client-go/kubernetes/typed/rbac/v1alpha1 k8s.io/client-go/kubernetes/typed/rbac/v1alpha1/fake k8s.io/client-go/kubernetes/typed/rbac/v1beta1 k8s.io/client-go/kubernetes/typed/rbac/v1beta1/fake k8s.io/client-go/kubernetes/typed/scheduling/v1alpha1 k8s.io/client-go/kubernetes/typed/scheduling/v1alpha1/fake k8s.io/client-go/kubernetes/typed/settings/v1alpha1 k8s.io/client-go/kubernetes/typed/settings/v1alpha1/fake k8s.io/client-go/kubernetes/typed/storage/v1 k8s.io/client-go/kubernetes/typed/storage/v1/fake k8s.io/client-go/kubernetes/typed/storage/v1alpha1 k8s.io/client-go/kubernetes/typed/storage/v1alpha1/fake k8s.io/client-go/kubernetes/typed/storage/v1beta1 k8s.io/client-go/kubernetes/typed/storage/v1beta1/fake k8s.io/client-go/listers/admissionregistration/v1alpha1 k8s.io/client-go/listers/admissionregistration/v1beta1 k8s.io/client-go/listers/apps/v1 k8s.io/client-go/listers/apps/v1beta1 k8s.io/client-go/listers/apps/v1beta2 k8s.io/client-go/listers/authentication/v1 k8s.io/client-go/listers/authentication/v1beta1 k8s.io/client-go/listers/authorization/v1 k8s.io/client-go/listers/authorization/v1beta1 k8s.io/client-go/listers/autoscaling/v1 k8s.io/client-go/listers/autoscaling/v2beta1 k8s.io/client-go/listers/batch/v1 k8s.io/client-go/listers/batch/v1beta1 k8s.io/client-go/listers/batch/v2alpha1 k8s.io/client-go/listers/certificates/v1beta1 k8s.io/client-go/listers/core/v1 k8s.io/client-go/listers/events/v1beta1 k8s.io/client-go/listers/extensions/v1beta1 k8s.io/client-go/listers/imagepolicy/v1alpha1 k8s.io/client-go/listers/networking/v1 k8s.io/client-go/listers/policy/v1beta1 k8s.io/client-go/listers/rbac/v1 k8s.io/client-go/listers/rbac/v1alpha1 k8s.io/client-go/listers/rbac/v1beta1 k8s.io/client-go/listers/scheduling/v1alpha1 k8s.io/client-go/listers/settings/v1alpha1 k8s.io/client-go/listers/storage/v1 k8s.io/client-go/listers/storage/v1alpha1 k8s.io/client-go/listers/storage/v1beta1 k8s.io/client-go/pkg/apis/clientauthentication k8s.io/client-go/pkg/apis/clientauthentication/install k8s.io/client-go/pkg/apis/clientauthentication/v1alpha1 k8s.io/client-go/pkg/version k8s.io/client-go/plugin/pkg/client/auth k8s.io/client-go/plugin/pkg/client/auth/azure k8s.io/client-go/plugin/pkg/client/auth/exec k8s.io/client-go/plugin/pkg/client/auth/gcp k8s.io/client-go/plugin/pkg/client/auth/oidc k8s.io/client-go/plugin/pkg/client/auth/openstack k8s.io/client-go/rest k8s.io/client-go/rest/fake k8s.io/client-go/rest/watch k8s.io/client-go/scale k8s.io/client-go/scale/fake k8s.io/client-go/scale/scheme k8s.io/client-go/scale/scheme/appsint k8s.io/client-go/scale/scheme/appsv1beta1 k8s.io/client-go/scale/scheme/appsv1beta2 k8s.io/client-go/scale/scheme/autoscalingv1 k8s.io/client-go/scale/scheme/extensionsint k8s.io/client-go/scale/scheme/extensionsv1beta1 k8s.io/client-go/testing k8s.io/client-go/third_party/forked/golang/template k8s.io/client-go/tools/auth k8s.io/client-go/tools/bootstrap/token/api k8s.io/client-go/tools/bootstrap/token/util k8s.io/client-go/tools/cache k8s.io/client-go/tools/cache/testing k8s.io/client-go/tools/clientcmd k8s.io/client-go/tools/clientcmd/api k8s.io/client-go/tools/clientcmd/api/latest k8s.io/client-go/tools/clientcmd/api/v1 k8s.io/client-go/tools/leaderelection k8s.io/client-go/tools/leaderelection/resourcelock k8s.io/client-go/tools/metrics k8s.io/client-go/tools/pager k8s.io/client-go/tools/portforward k8s.io/client-go/tools/record k8s.io/client-go/tools/reference k8s.io/client-go/tools/remotecommand k8s.io/client-go/transport k8s.io/client-go/transport/spdy k8s.io/client-go/util/buffer k8s.io/client-go/util/cert k8s.io/client-go/util/cert/triple k8s.io/client-go/util/certificate k8s.io/client-go/util/certificate/csr k8s.io/client-go/util/exec k8s.io/client-go/util/flowcontrol k8s.io/client-go/util/homedir k8s.io/client-go/util/integer k8s.io/client-go/util/jsonpath k8s.io/client-go/util/retry k8s.io/client-go/util/testing k8s.io/client-go/util/workqueue --- FAIL: TestGetServerGroupsWithTimeout (2.00s) discovery_client_test.go:144: unexpected error: Get http://127.0.0.1:42428/api?timeout=1s: context deadline exceeded

#56876

#63086 should fix the problem

aarondav · 2018-05-10T15:48:30Z

Quick question -- before this patch, what prevented a blackholed TCP connection from causing the client to hang indefinitely? Did we set an SO_TIMEOUT/socketTimeout in some way?

soltysh · 2018-05-11T19:53:20Z

It wasn't an indefinite hang, rather a long one, which lead to requests longing 10min+, which from a UX point of view was too long.

bjhaid · 2019-04-01T19:21:10Z

FWIW today we observed the behavior this should have addressed with kubectl 1.13.2

k8s-ci-robot assigned deads2k and juanvallejo Apr 17, 2018

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 17, 2018

k8s-ci-robot requested review from brendandburns and luxas April 17, 2018 15:03

This was referenced Apr 17, 2018

add default req timeout for caching #60434

Closed

If an aggregated API server is down, kubectl may take up to 10 minutes to retrieve server resources openshift/origin#17159

Closed

deads2k reviewed Apr 17, 2018

View reviewed changes

liggitt reviewed Apr 17, 2018

View reviewed changes

soltysh force-pushed the discovery_timeout branch from 7029932 to c10197f Compare April 19, 2018 13:03

deads2k reviewed Apr 19, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2018

Set a default request timeout for discovery client

7bd48a7

soltysh force-pushed the discovery_timeout branch from c10197f to 7bd48a7 Compare April 20, 2018 14:23

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2018

k8s-github-robot merged commit 6da4355 into kubernetes:master Apr 20, 2018

soltysh deleted the discovery_timeout branch April 23, 2018 10:38

sttts reviewed Apr 24, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set a default request timeout for discovery client #62733

Set a default request timeout for discovery client #62733

soltysh commented Apr 17, 2018

deads2k Apr 17, 2018 •

edited

deads2k Apr 17, 2018

soltysh Apr 19, 2018

liggitt Apr 17, 2018

liggitt Apr 17, 2018

hzxuzhonghu Apr 18, 2018

soltysh Apr 19, 2018

hzxuzhonghu commented Apr 18, 2018

soltysh commented Apr 19, 2018

soltysh commented Apr 19, 2018

deads2k Apr 19, 2018

deads2k commented Apr 19, 2018

soltysh commented Apr 20, 2018

deads2k commented Apr 20, 2018

k8s-ci-robot commented Apr 20, 2018

k8s-github-robot commented Apr 20, 2018

sttts Apr 24, 2018

soltysh Apr 24, 2018

sttts Apr 24, 2018

aarondav commented May 10, 2018

soltysh commented May 11, 2018

bjhaid commented Apr 1, 2019

Set a default request timeout for discovery client #62733

Set a default request timeout for discovery client #62733

Conversation

soltysh commented Apr 17, 2018

deads2k Apr 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Apr 18, 2018

soltysh commented Apr 19, 2018

soltysh commented Apr 19, 2018

Choose a reason for hiding this comment

deads2k commented Apr 19, 2018

soltysh commented Apr 20, 2018

deads2k commented Apr 20, 2018

k8s-ci-robot commented Apr 20, 2018

k8s-github-robot commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarondav commented May 10, 2018

soltysh commented May 11, 2018

bjhaid commented Apr 1, 2019

deads2k Apr 17, 2018 •

edited