add default req timeout for caching #60434

juanvallejo · 2018-02-26T17:45:45Z

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

Release note:

NONE

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.

cc @mfojtik @jpeeler @pmorie @kubernetes/sig-cli-pr-reviews

hzxuzhonghu · 2018-02-27T03:01:57Z

I donot think this is needed, see staging/src/k8s.io/client-go/rest/config.go func UnversionedRESTClientFor()

juanvallejo · 2018-02-27T16:05:21Z

@hzxuzhonghu thanks, but isn't the value from config.Timeout that is set there specified through the --request-timeout flag? This PR would default discovery client requests to a shorter amount of time if a user does not explicitly provide a request timeout value.

hzxuzhonghu · 2018-02-28T01:32:07Z

@juanvallejo as far as i see, you set a default timeout when no timeout explicitly set. But maybe the intention of timeout=0 is to not timeout. Here you change this.

@sttts @liggitt @cheftako

juanvallejo · 2018-03-15T14:25:09Z

cc @sttts

deads2k · 2018-03-15T20:02:28Z

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

@@ -41,6 +42,8 @@ const (
 	defaultRetries = 2
 	// protobuf mime type
 	mimePb = "application/com.github.proto-openapi.spec.v2@v1.0+protobuf"
+	// defaultRetries is the number of times a resource discovery is repeated if an api group disappears on the fly (e.g. ThirdPartyResources).


doc is wrong . It's not retries

deads2k · 2018-03-15T20:03:30Z

/ok-to-test

I like this in principle. We need to think about what a default should be. 32 seconds (make sure you're different than something else's timeout) to start is a lot better than infinity, but long enough to avoid failing "normal" levels of latency.

soltysh

lgtm, but you need to fix the nits David pointed out

juanvallejo · 2018-03-16T14:16:12Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

juanvallejo · 2018-03-16T17:23:27Z

@soltysh @deads2k thanks, review comments addressed

deads2k · 2018-03-16T18:01:55Z

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

@@ -170,7 +187,7 @@ func (d *DiscoveryClient) ServerResourcesForGroupVersion(groupVersion string) (r
 	resources = &metav1.APIResourceList{
 		GroupVersion: groupVersion,
 	}
-	err = d.restClient.Get().AbsPath(url.String()).Do().Into(resources)
+	err = d.restClient.Get().Timeout(timeout).AbsPath(url.String()).Do().Into(resources)


wait, isn't this the wrong spot. You need to set it for the context timeout/deadline, right?

@deads2k Thanks, I believe the pieces needed to set a context timeout/deadline are being added in https://github.com/kubernetes/kubernetes/pull/53076/files#diff-063e73bab84834a4187b1ad4865050adR217

Can go ahead and pick the relevant pieces from that PR into this one.

soltysh

One nit still ;)

soltysh · 2018-03-19T09:24:23Z

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.
+	// we enforce a timeout for every request here in order to avoid hanging indefinitely
+	defaultTimeout = 12 * time.Second


You're saying 32 in the comment but I see 12 here, typo?

thanks for catching that, will update godoc - @deads2k I lowered the time to 12s as it is still distinguishable from other timeouts, long enough to avoid failing on "normal" levels of latency, yet still not as long such that if every discovery request times out, the overall time that a command hangs is reduced.

long enough to avoid failing on "normal" levels of latency

the openapi docs are quite large... on low-bandwidth connections, this seems too low

+1 on leaving this at the previous 32.

@soltysh @liggitt thanks, switched this back to 32s

soltysh

/lgtm

juanvallejo · 2018-03-19T17:22:13Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce

juanvallejo · 2018-03-19T20:36:20Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce

deads2k · 2018-03-20T12:35:42Z

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

@@ -158,6 +169,12 @@ func (d *DiscoveryClient) ServerGroups() (apiGroupList *metav1.APIGroupList, err

 // ServerResourcesForGroupVersion returns the supported resources for a group and version.
 func (d *DiscoveryClient) ServerResourcesForGroupVersion(groupVersion string) (resources *metav1.APIResourceList, err error) {
+	// set the default timeout if a timeout value for this request has not already been set
+	timeout := d.restClient.Get().GetTimeout()


This interaction looks wrong

deads2k · 2018-03-20T12:36:37Z

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

@@ -363,6 +367,19 @@ func (r *Request) Timeout(d time.Duration) *Request {
 	return r
 }

+func (r *Request) RequestTimeout(d time.Duration) *Request {


If I set a timeout and one isn't set, set it.

If I set a timeout and a shorter one is set, don't do anything.

If I set a timeout and a longer one is set, reset it to the shorter one.

If I set a timeout and a longer one is set, reset it to the shorter one.

Why would you reset it to shorter one, if someone explicitly asks for longer - use it.

deads2k · 2018-03-20T12:36:45Z

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

+}
+
+// GetTimeout retrieves the current value of the "timeout" parameter.
+func (r *Request) GetTimeout() time.Duration {


The need for this isn't clear.

deads2k · 2018-03-20T12:38:08Z

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

 		if r.ctx != nil {
+			r.ctx, _ = context.WithTimeout(r.ctx, r.requestTimeout)


Why would you use a zero value timeout? Seems like this should be gated.

Why would you use a zero value timeout? Seems like this should be gated.

Definitely needs gating. You'll be messing with behavior you don't own otherwise. Is there a getter for the current timeout? If not, you don't get to touch it after the context is set.

soltysh · 2018-03-20T12:55:45Z

/lgtm cancel
@juanvallejo we need to discuss this approach once more :)

k8s-ci-robot · 2018-03-20T12:55:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juanvallejo
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: krousey

Assign the PR to them by writing /assign @krousey in a comment when ready.

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

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

jpeeler · 2018-04-11T16:44:11Z

Did the sync up ever happen here? @soltysh @juanvallejo There's a downstream issue dependent on this PR.

soltysh · 2018-04-17T10:02:22Z

@jpeeler I'm just looking into it...

soltysh · 2018-04-17T15:03:09Z

Alternative solution in #62733

soltysh · 2018-04-19T12:45:07Z

Closing in favor of the alternative solution.

k8s-ci-robot requested review from caesarxuchao and mwielgus February 26, 2018 17:46

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2018

sttts mentioned this pull request Mar 15, 2018

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

Closed

juanvallejo force-pushed the jvallejo/add-default-discovery-timeout branch from 0b1e8d6 to f09b6bd Compare March 15, 2018 15:04

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 15, 2018

deads2k reviewed Mar 15, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 15, 2018

soltysh reviewed Mar 16, 2018

View reviewed changes

soltysh approved these changes Mar 16, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/add-default-discovery-timeout branch from f09b6bd to 6c752f2 Compare March 16, 2018 14:12

juanvallejo force-pushed the jvallejo/add-default-discovery-timeout branch from 6c752f2 to df2673e Compare March 16, 2018 17:23

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 16, 2018

deads2k reviewed Mar 16, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/add-default-discovery-timeout branch from a5dd391 to d205291 Compare March 16, 2018 22:24

soltysh approved these changes Mar 19, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/add-default-discovery-timeout branch 2 times, most recently from f54dba2 to 0cadac2 Compare March 19, 2018 14:10

soltysh approved these changes Mar 19, 2018

View reviewed changes

k8s-ci-robot assigned soltysh Mar 19, 2018

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

deads2k reviewed Mar 20, 2018

View reviewed changes

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

add default req timeout for caching

f647aa6

juanvallejo force-pushed the jvallejo/add-default-discovery-timeout branch from 0cadac2 to f647aa6 Compare March 20, 2018 18:10

soltysh closed this Apr 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add default req timeout for caching #60434

add default req timeout for caching #60434

juanvallejo commented Feb 26, 2018 •

edited

hzxuzhonghu commented Feb 27, 2018

juanvallejo commented Feb 27, 2018

hzxuzhonghu commented Feb 28, 2018

juanvallejo commented Mar 15, 2018

deads2k Mar 15, 2018

deads2k commented Mar 15, 2018

soltysh left a comment

juanvallejo commented Mar 16, 2018

juanvallejo commented Mar 16, 2018

deads2k Mar 16, 2018

juanvallejo Mar 16, 2018 •

edited

soltysh left a comment

soltysh Mar 19, 2018

juanvallejo Mar 19, 2018

liggitt Mar 19, 2018

soltysh Mar 19, 2018

juanvallejo Mar 19, 2018

soltysh left a comment

juanvallejo commented Mar 19, 2018

juanvallejo commented Mar 19, 2018

deads2k Mar 20, 2018

deads2k Mar 20, 2018

soltysh Mar 20, 2018

deads2k Mar 20, 2018

deads2k Mar 20, 2018

deads2k Mar 20, 2018

soltysh commented Mar 20, 2018

k8s-ci-robot commented Mar 20, 2018

jpeeler commented Apr 11, 2018

soltysh commented Apr 17, 2018

soltysh commented Apr 17, 2018

soltysh commented Apr 19, 2018

		if r.ctx != nil {
		r.ctx, _ = context.WithTimeout(r.ctx, r.requestTimeout)

add default req timeout for caching #60434

add default req timeout for caching #60434

Conversation

juanvallejo commented Feb 26, 2018 • edited

hzxuzhonghu commented Feb 27, 2018

juanvallejo commented Feb 27, 2018

hzxuzhonghu commented Feb 28, 2018

juanvallejo commented Mar 15, 2018

Choose a reason for hiding this comment

deads2k commented Mar 15, 2018

soltysh left a comment

Choose a reason for hiding this comment

juanvallejo commented Mar 16, 2018

juanvallejo commented Mar 16, 2018

Choose a reason for hiding this comment

juanvallejo Mar 16, 2018 • edited

Choose a reason for hiding this comment

soltysh left a 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

soltysh left a comment

Choose a reason for hiding this comment

juanvallejo commented Mar 19, 2018

juanvallejo commented Mar 19, 2018

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

soltysh commented Mar 20, 2018

k8s-ci-robot commented Mar 20, 2018

jpeeler commented Apr 11, 2018

soltysh commented Apr 17, 2018

soltysh commented Apr 17, 2018

soltysh commented Apr 19, 2018

juanvallejo commented Feb 26, 2018 •

edited

juanvallejo Mar 16, 2018 •

edited