Bug 1433244 - allow imageimports to be long runing requests #13458

soltysh · 2017-03-20T14:18:10Z

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

soltysh · 2017-03-20T14:19:50Z

pkg/client/imagestreams.go

@@ -109,7 +110,7 @@ func (c *imageStreams) UpdateStatus(stream *imageapi.ImageStream) (result *image
 // will be returned if no actual import was requested (the to fields were not set), or an ImageStream if import was requested.
 func (c *imageStreams) Import(isi *imageapi.ImageStreamImport) (*imageapi.ImageStreamImport, error) {
 	result := &imageapi.ImageStreamImport{}
-	if err := c.r.Post().Namespace(c.ns).Resource("imageStreamImports").Body(isi).Do().Into(result); err != nil {
+	if err := c.r.Post().Namespace(c.ns).Resource("imageStreamImports").Timeout(2 * time.Minute).Body(isi).Do().Into(result); err != nil {


@smarterclayton this 2m is wrong, we should use request-timeout value when specified in client, imho.

Isn't request timeout already set into the value we send to the server? If not, why not?

Yes, request timeout is set and verified inside WithTimeoutForNonLongRunningRequests filter, but resthandler.go has additional timeout which needs to be set here explicitly. Personally, I'd prefer the value from request-timeout flag would be applied to this, as well.

mfojtik · 2017-03-21T10:09:12Z

I'm pretty much against adding more logic to handle "wait for" (like --follow, etc). Either we need some solution that will work for all cases where users want to "wait for something to happen" or have some consistent flag (like --follow). I would much prefer the first one...

And I think we actually have something like that... oc observe can serve as 'wait for tags to be imported'. Maybe we just need to provide more examples on how to use it, or make some derivate of oc observe that will explicitely allow users to 'wait for field XYZ is populated or get value XYZ'

How about a wrapper for oc observe like:

$ oc wait-for is/foo --to-contain=.status.tags["bar"]
$ oc wait-for dc/registry --to-be=running
$ oc wait-for secret/foo --to-exists
...

I think this might actually help the ansible code as well. Also 50% of our extended tests and test-cmd are just derivation of "wait for XYZ to happen". With a simple DSL that we can extended if we find a case we can simplify all that.

@fabianofranz @smarterclayton ^

smarterclayton · 2017-03-22T21:07:20Z

I'm fine with request-timeout being applied there - that's the effective request timeout

…

On Mon, Mar 20, 2017 at 4:07 PM, Maciej Szulik ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/client/imagestreams.go <#13458 (comment)>: > @@ -109,7 +110,7 @@ func (c *imageStreams) UpdateStatus(stream *imageapi.ImageStream) (result *image // will be returned if no actual import was requested (the to fields were not set), or an ImageStream if import was requested. func (c *imageStreams) Import(isi *imageapi.ImageStreamImport) (*imageapi.ImageStreamImport, error) { result := &imageapi.ImageStreamImport{} - if err := c.r.Post().Namespace(c.ns).Resource("imageStreamImports").Body(isi).Do().Into(result); err != nil { + if err := c.r.Post().Namespace(c.ns).Resource("imageStreamImports").Timeout(2 * time.Minute).Body(isi).Do().Into(result); err != nil { Yes, request timeout is set and verified inside WithTimeoutForNonLongRunningRequests filter, but resthandler.go <https://github.com/openshift/origin/blob/612dcfb20d42755959895cc19285b429864ddeba/vendor/k8s.io/kubernetes/pkg/apiserver/resthandler.go#L350> has additional timeout which needs to be set here explicitly. Personally, I'd prefer the value from request-timeout flag would be applied to this, as well. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#13458 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p3UDltPEQI_riSgZsU91qZ3gz3Uyks5rntxogaJpZM4MifVv> .

mfojtik · 2017-03-27T12:24:59Z

as discussed on arch call, i'm fine with this for now.

mfojtik · 2017-03-27T12:25:14Z

[test]

mfojtik · 2017-03-31T08:56:07Z

[test]

mfojtik · 2017-03-31T08:58:20Z

@soltysh

POST /oapi/v1/namespaces/integration/imagestreamimports?timeout=2m0s: (2m0.000878996s) 504

this does not seems right?

soltysh · 2017-04-06T11:03:26Z

@smarterclayton ptal if the 2nd commit passing request-timeout value all the way down to Request object makes sense, if so I'll open an upstream PR for that.

smarterclayton · 2017-04-07T23:26:40Z

vendor/k8s.io/kubernetes/pkg/client/restclient/client.go

 	}
-	return NewRequest(c.Client, verb, c.base, c.versionedAPIPath, c.contentConfig, c.serializers, backoff, c.Throttle)
+	return NewRequest(c.Client, verb, c.base, c.versionedAPIPath, c.contentConfig,


Timeout should be a helper on Request, vs adding something to the constructor. There are lots of reasons to set timeouts per request type.

Spawned #13701

smarterclayton · 2017-04-07T23:27:28Z

We can merge imagestreamimport separately - let's split the other issue out. There's enough wrinkles we need to deal with there that doing two is better than one.

openshift-bot · 2017-04-10T14:13:30Z

Evaluated for origin test up to 078e6ea

soltysh · 2017-04-10T15:05:18Z

@smarterclayton this is only the fix that allows extended timeout for image imports.

openshift-bot · 2017-04-10T15:29:39Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/673/) (Base Commit: 585b160)

smarterclayton · 2017-04-10T16:43:11Z

LGTM [merge]

openshift-bot · 2017-04-10T16:46:34Z

Evaluated for origin merge up to 078e6ea

openshift-bot · 2017-04-10T17:53:40Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/304/) (Base Commit: d128020) (Image: devenv-rhel7_6133)

@smarterclayton

Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515). UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down This was split from #13458. @smarterclayton let's continue the discussion here.

@smarterclayton

Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515). UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down This was split from openshift/origin#13458. @smarterclayton let's continue the discussion here. Origin-commit: 0fd7db928c80fe528c51848eebb85b9544beb72f

@smarterclayton

Automatic merge from submit-queue (batch tested with PRs 18503, 18399, 13701, 18513, 18515). UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down This was split from openshift/origin#13458. @smarterclayton let's continue the discussion here. Origin-commit: 0fd7db928c80fe528c51848eebb85b9544beb72f Kubernetes-commit: c6df867ec232478651d96abce6d459be747defed

soltysh commented Mar 20, 2017

View reviewed changes

soltysh assigned smarterclayton Mar 20, 2017

Bug 1433244 - allow imageimports to be long runing requests

078e6ea

soltysh force-pushed the bug1433244 branch from 7e52677 to a8751db Compare April 6, 2017 11:02

soltysh force-pushed the bug1433244 branch from a8751db to 1089875 Compare April 6, 2017 13:17

smarterclayton reviewed Apr 7, 2017

View reviewed changes

soltysh mentioned this pull request Apr 10, 2017

UPSTREAM: 51042: Allow passing request-timeout from NewRequest all the way down #13701

Merged

soltysh force-pushed the bug1433244 branch from 1089875 to 078e6ea Compare April 10, 2017 14:12

openshift-bot merged commit 5129a2d into openshift:master Apr 10, 2017

soltysh deleted the bug1433244 branch April 11, 2017 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1433244 - allow imageimports to be long runing requests #13458

Bug 1433244 - allow imageimports to be long runing requests #13458

soltysh commented Mar 20, 2017

soltysh Mar 20, 2017

smarterclayton Mar 20, 2017

soltysh Mar 20, 2017

mfojtik commented Mar 21, 2017 •

edited

Loading

smarterclayton commented Mar 22, 2017 via email

mfojtik commented Mar 27, 2017

mfojtik commented Mar 27, 2017

mfojtik commented Mar 31, 2017

mfojtik commented Mar 31, 2017

soltysh commented Apr 6, 2017

smarterclayton Apr 7, 2017

soltysh Apr 10, 2017

smarterclayton commented Apr 7, 2017

openshift-bot commented Apr 10, 2017

soltysh commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

smarterclayton commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

openshift-bot commented Apr 10, 2017 •

edited

Loading

Bug 1433244 - allow imageimports to be long runing requests #13458

Bug 1433244 - allow imageimports to be long runing requests #13458

Conversation

soltysh commented Mar 20, 2017

soltysh Mar 20, 2017

Choose a reason for hiding this comment

smarterclayton Mar 20, 2017

Choose a reason for hiding this comment

soltysh Mar 20, 2017

Choose a reason for hiding this comment

mfojtik commented Mar 21, 2017 • edited Loading

smarterclayton commented Mar 22, 2017 via email

mfojtik commented Mar 27, 2017

mfojtik commented Mar 27, 2017

mfojtik commented Mar 31, 2017

mfojtik commented Mar 31, 2017

soltysh commented Apr 6, 2017

smarterclayton Apr 7, 2017

Choose a reason for hiding this comment

soltysh Apr 10, 2017

Choose a reason for hiding this comment

smarterclayton commented Apr 7, 2017

openshift-bot commented Apr 10, 2017

soltysh commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

smarterclayton commented Apr 10, 2017

openshift-bot commented Apr 10, 2017

openshift-bot commented Apr 10, 2017 • edited Loading

mfojtik commented Mar 21, 2017 •

edited

Loading

openshift-bot commented Apr 10, 2017 •

edited

Loading