treat binary buildconfig instantiate requests as long running #12679
Conversation
|
@smarterclayton ptal @ncdc @smarterclayton implied you might be the right one to review this also, in terms of picking the "right" place to setup the long running check. |
|
[test] |
|
I'll take a look in a bit. cc @sttts |
bparees
force-pushed
the
binary_longrunning
branch
from
719e056 to
40d8994
Compare
pkg/cmd/server/origin/master.go
Outdated
| longRunningRequestCheckFunc := func(r *http.Request) bool { | ||
| return longRunningRequestCheck(r) || kc.LongRunningFunc(r) | ||
| } | ||
| handler = kgenericfilters.WithTimeoutForNonLongRunningRequests(handler, longRunningRequestCheckFunc) |
There was a problem hiding this comment.
Don't like to have this logic here. Can't we add that RE to kc.LongRunningFunc (haven't looked at the code) ?
There was a problem hiding this comment.
genericapiserver.Config can be customized. Just extend the upstream default function and set it in the config.
There was a problem hiding this comment.
Can you elaborate on why you don't like it here? This puts it as close to where we setup the handler as possible, ensuring it doesn't get skipped by an alternate codepath (e.g. test code that mocks bringing up the api server).
There was a problem hiding this comment.
It complicates the handler chain even more. There were complaints already in this direction. Moreover, the Config.LongRunningFunc exists for the very reason of customizability. Otherwise, we would have hardcoded it in the upstream code.
|
It's upstream, I don't think kube would want our openshift specific
endpoint included in their upstream regex.
Ben Parees | OpenShift
…On Jan 30, 2017 4:08 AM, "Dr. Stefan Schimanski" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/cmd/server/origin/master.go
<#12679 (review)>
:
> @@ -314,7 +319,13 @@ func (c *MasterConfig) buildHandlerChain(assetConfig *AssetConfig) (func(http.Ha
handler = kgenericfilters.WithCORS(handler, c.Options.CORSAllowedOrigins, nil, nil, nil, "true")
handler = kgenericfilters.WithPanicRecovery(handler, c.RequestContextMapper)
- handler = kgenericfilters.WithTimeoutForNonLongRunningRequests(handler, kc.LongRunningFunc)
+
+ longRunningRequestRE := regexp.MustCompile(longRunningEndpointsRE)
+ longRunningRequestCheck := kgenericfilters.BasicLongRunningRequestCheck(longRunningRequestRE, nil)
+ longRunningRequestCheckFunc := func(r *http.Request) bool {
+ return longRunningRequestCheck(r) || kc.LongRunningFunc(r)
+ }
+ handler = kgenericfilters.WithTimeoutForNonLongRunningRequests(handler, longRunningRequestCheckFunc)
Don't like to have this logic here. Can't we add that RE to
kc.LongRunningFunc (haven't looked at the code) ?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#12679 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEvl3jxRB-2ZeKc0DBkcPPvZADJhe0x1ks5rXaiQgaJpZM4Lu1kJ>
.
|
bparees
force-pushed
the
binary_longrunning
branch
from
40d8994 to
977ce29
Compare
| genericConfig.LongRunningFunc = func(r *http.Request) bool { | ||
| return longRunningRequestCheck(r) || kubeLongRunningFunc(r) | ||
| } | ||
|
|
There was a problem hiding this comment.
@sttts i don't know if this is closer to what you had in mind... but i want to retain the behavior of delegating to the kubernetes default function so that if it is changed, we are still honoring it. ptal.
|
|
||
| // request paths that match this regular expression will be treated as long running | ||
| // and not subjected to the default server timeout. | ||
| longRunningEndpointsRE = "(/|^)buildconfigs/.*?/instantiatebinary$" |
There was a problem hiding this comment.
can we call this extraLongRunningEndpointsRE or even originLongRunningEndpointsRE?
| @@ -301,6 +309,13 @@ func BuildKubernetesMasterConfig(options configapi.MasterConfig, requestContextM | |||
| } | |||
| genericConfig.ExternalAddress = url.Host | |||
|
|
|||
| longRunningRequestRE := regexp.MustCompile(longRunningEndpointsRE) | |||
| longRunningRequestCheck := kgenericfilters.BasicLongRunningRequestCheck(longRunningRequestRE, nil) | |||
There was a problem hiding this comment.
Better: originLongRunningFunc
bparees
force-pushed
the
binary_longrunning
branch
from
977ce29 to
ad60ba3
Compare
|
@sttts updated, ptal. |
|
|
||
| // request paths that match this regular expression will be treated as long running | ||
| // and not subjected to the default server timeout. | ||
| originLongRunningEndpointsRE = "(/|^)buildconfigs/.*?/instantiatebinary$" |
There was a problem hiding this comment.
Why the question mark in .*??
There was a problem hiding this comment.
I think it's probably unnecessary in this case, but i generally try to default to non-greedy matching. tends to have less surprises.
There was a problem hiding this comment.
shoudln't the MustCompile be here?
There was a problem hiding this comment.
no, but it should be a constant, not a variable, if we're going to follow the pattern kube used.
There was a problem hiding this comment.
updated to make it a constant.
|
|
||
| // request paths that match this regular expression will be treated as long running | ||
| // and not subjected to the default server timeout. | ||
| originLongRunningEndpointsRE = "(/|^)buildconfigs/.*?/instantiatebinary$" |
There was a problem hiding this comment.
shoudln't the MustCompile be here?
bparees
force-pushed
the
binary_longrunning
branch
from
ad60ba3 to
7678e97
Compare
|
@ncdc @smarterclayton bump |
|
extended image_eco passed. [testextended][extended:core(builds)] |
|
@bparees |
eh, i'll take it out. |
bparees
force-pushed
the
binary_longrunning
branch
from
7678e97 to
c083b52
Compare
|
Evaluated for origin test up to c083b52 |
|
Evaluated for origin testextended up to c083b52 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13439/) (Base Commit: ed1d332) |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1049/) (Base Commit: ed1d332) (Extended Tests: core(builds)) |
|
[merge] |
|
Evaluated for origin merge up to c083b52 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13457/) (Base Commit: 6b6610a) (Image: devenv-rhel7_5823) |
fixes #11563