allow for configurable server side timeouts on routes #9671

JacobTanenbaum · 2016-07-01T14:18:38Z

using annotations configure a route to have it's own server side timeout

--beginning of route.yaml
apiVersion: v1
kind: Route
metadata:
annotations:
hello-again.timeout: 5s
creationTimestamp: 2016-06-20T22:43:44Z
name: hello-again
namespace: default
resourceVersion: "461544"
selfLink: /oapi/v1/namespaces/default/routes/hello-again
uid: 717b8a38-3738-11e6-8b7f-525400d6577c
...

matching against [routename].timeout: [time] is what creates the timeout

JacobTanenbaum · 2016-07-06T14:04:04Z

@ramr @rajatchopra @knobunc please review?

rajatchopra · 2016-07-06T18:41:58Z

Since the timeout is per route, maybe we should follow the existing convention of the annotation name: router.openshift.io/haproxy.timeout

ramr · 2016-07-06T21:43:53Z

Yeah, agree with @rajatchopra that it would better to just use a standard "name" ala: router.openshift.io/haproxy.timeout rather than a custom one per route (makes copy-pasta editing easier as well as if you want to change the host name).

And we should also do this for the other be_tcp_* and be_secure_* backends.

rajatchopra · 2016-07-08T18:13:15Z

LGTM
@JacobTanenbaum squash the commits maybe.

knobunc · 2016-07-08T19:24:36Z

Is the annotation value validated somewhere? Or can they sneak in a string and do naughty things?

rajatchopra · 2016-07-08T20:08:55Z

Good point @knobunc
There is a function call implemented already to check if the value is an integer, we should use that call.

@JacobTanenbaum see https://github.com/openshift/origin/blob/master/pkg/router/template/router.go#L176

JacobTanenbaum · 2016-07-12T18:22:00Z

@rajatchopra @knobunc If I use the isInteger() function then the timeout can only be specified in milliseconds. Most of the other timeouts in the haproxy router template are in seconds, should I make another function for isTime that includes the units so users can input a timeout in any unit that is valid in haproxy?

knobunc · 2016-07-12T18:31:37Z

@rajatchopra added a regex validator... we can just use that.

On Tue, Jul 12, 2016 at 2:22 PM, Jacob Tanenbaum [email protected]
wrote:

@rajatchopra https://github.com/rajatchopra @knobunc
https://github.com/knobunc If I use the isInteger() function then the
timeout can only be specified in milliseconds. Most of the other timeouts
in the haproxy router template are in seconds, should I make another
function for isTime that includes the units so users can input a timeout in
any unit that is valid in haproxy?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9671 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AJkavcJYdgVVnP3zfhRfTZF16VkVLlD1ks5qU9tPgaJpZM4JDLz9
.

rajatchopra · 2016-07-12T20:10:48Z

Yes matchString(pattern, str). [0-9]+[su] for the pattern or something like that.

knobunc · 2016-07-14T19:10:04Z

images/router/haproxy/conf/haproxy-config.template

@@ -226,6 +226,11 @@ backend be_edge_http_{{$cfgIdx}}
    {{ else }}
  balance leastconn
    {{ end }}
+    {{ with $value := index $cfg.Annotations "router.openshift.io/haproxy.timeout"}}
+      {{if (matchString "[0-9]*(us|ms|s|m|h|d)?" $value) }}


I assume matchString has an implicit anchor to the start and end of the string? But can we check that "test 9ms test" doesn't match.

Also * matches 0+ characters... I assume you want [0-9]+ to make it match 1+. Also \d is usually a synonym for [0-9], so let's see if "\d+(us|ms|s|m|h|d)?" works.

'rm -f *; 90ms test' will match. the regex needs to be "^[1-9][0-9]*(us|ms|s|m|h|d)?$"

Should we make anchoring the front and back the default in matchString? i.e. wrap the match they give us with ^(...)$

If we want to match a substr, then we can say .*thing.*

It feels like we will bite people with this behavior.

rajatchopra · 2016-07-18T17:44:10Z

Looks good. Squash?

knobunc · 2016-07-19T13:21:46Z

pkg/router/template/router.go

-	status, err := regexp.MatchString(pattern, s)
+func matchPattern(pattern, s string) bool {
+	glog.V(4).Infof("matchPattern called with %s and %s", pattern, s)
+	status, err := regexp.MatchString("^"+pattern+"$", s)


This needs parens, or 'a|b' would match 'anaughty'

JacobTanenbaum · 2016-07-19T13:43:36Z

@knobunc squashed the commits

knobunc · 2016-07-19T13:47:14Z

pkg/router/template/plugin.go

@@ -99,7 +99,7 @@ func NewTemplatePlugin(cfg TemplatePluginConfig) (*TemplatePlugin, error) {
 	globalFuncs := template.FuncMap{
 		"endpointsForAlias": endpointsForAlias,
 		"env":               env,
-		"matchString":       matchString,
+		"matchPattern":      matchPattern,


Are there any docs for this that should change? I couldn't find any. So at the least can we put a comment over in the original PR that added matchString to say that it changed (and what the new behavior is).

using annotations configure a route to have it's own server side timeout --beginning of route.yaml apiVersion: v1 kind: Route metadata: annotations: hello-again.timeout: 5s creationTimestamp: 2016-06-20T22:43:44Z name: hello-again namespace: default resourceVersion: "461544" selfLink: /oapi/v1/namespaces/default/routes/hello-again uid: 717b8a38-3738-11e6-8b7f-525400d6577c ... matching against [routename].timeout: [time] is what creates the timeout changelog: - change the annotation name to "router.openshift.io/haproxy.timeout" to conform to naming convention - adding timeout annotations to be_tcp_* and be_secure_* - added a regex to validate input - corrected the regular expression - changed matchString to matchPattern and made the default to anchor the pattern at the beginning and end of lines - nit, changed info prompt to correct function name - inline description FuncMap functions for the router template

knobunc · 2016-07-20T13:32:13Z

LGTM [merge]

openshift-bot · 2016-07-20T13:35:57Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6563/) (Image: devenv-rhel7_4632)

openshift-bot · 2016-07-20T13:35:57Z

Evaluated for origin merge up to c1db34b

openshift-bot · 2016-07-20T13:35:58Z

[Test]ing while waiting on the merge queue

openshift-bot · 2016-07-20T13:40:48Z

Evaluated for origin test up to c1db34b

openshift-bot · 2016-07-20T14:55:39Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6563/)

JacobTanenbaum force-pushed the RouterTimeouts branch from 786f5af to 1055ae0 Compare July 7, 2016 02:39

knobunc reviewed Jul 14, 2016
View reviewed changes

knobunc reviewed Jul 19, 2016
View reviewed changes

JacobTanenbaum force-pushed the RouterTimeouts branch from 0edc1a3 to 3893c95 Compare July 19, 2016 13:42

knobunc reviewed Jul 19, 2016
View reviewed changes

JacobTanenbaum force-pushed the RouterTimeouts branch from 3893c95 to 3eb05ca Compare July 19, 2016 13:52

JacobTanenbaum mentioned this pull request Jul 19, 2016

Ab testing redone #9119

Merged

JacobTanenbaum force-pushed the RouterTimeouts branch from 3eb05ca to c1db34b Compare July 19, 2016 19:51

openshift-bot merged commit b0d347c into openshift:master Jul 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow for configurable server side timeouts on routes #9671

allow for configurable server side timeouts on routes #9671

JacobTanenbaum commented Jul 1, 2016

JacobTanenbaum commented Jul 6, 2016

rajatchopra commented Jul 6, 2016

ramr commented Jul 6, 2016

rajatchopra commented Jul 8, 2016

knobunc commented Jul 8, 2016

rajatchopra commented Jul 8, 2016

JacobTanenbaum commented Jul 12, 2016

knobunc commented Jul 12, 2016

rajatchopra commented Jul 12, 2016 •

edited

Loading

knobunc Jul 14, 2016

rajatchopra Jul 14, 2016 •

edited

Loading

knobunc Jul 14, 2016 •

edited

Loading

rajatchopra commented Jul 18, 2016

knobunc Jul 19, 2016

JacobTanenbaum commented Jul 19, 2016

knobunc Jul 19, 2016

knobunc commented Jul 20, 2016

openshift-bot commented Jul 20, 2016 •

edited

Loading

openshift-bot commented Jul 20, 2016

openshift-bot commented Jul 20, 2016

openshift-bot commented Jul 20, 2016

openshift-bot commented Jul 20, 2016

allow for configurable server side timeouts on routes #9671

allow for configurable server side timeouts on routes #9671

Conversation

JacobTanenbaum commented Jul 1, 2016

JacobTanenbaum commented Jul 6, 2016

rajatchopra commented Jul 6, 2016

ramr commented Jul 6, 2016

rajatchopra commented Jul 8, 2016

knobunc commented Jul 8, 2016

rajatchopra commented Jul 8, 2016

JacobTanenbaum commented Jul 12, 2016

knobunc commented Jul 12, 2016

rajatchopra commented Jul 12, 2016 • edited Loading

knobunc Jul 14, 2016

Choose a reason for hiding this comment

rajatchopra Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

knobunc Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

rajatchopra commented Jul 18, 2016

knobunc Jul 19, 2016

Choose a reason for hiding this comment

JacobTanenbaum commented Jul 19, 2016

knobunc Jul 19, 2016

Choose a reason for hiding this comment

knobunc commented Jul 20, 2016

openshift-bot commented Jul 20, 2016 • edited Loading

openshift-bot commented Jul 20, 2016

openshift-bot commented Jul 20, 2016

openshift-bot commented Jul 20, 2016

openshift-bot commented Jul 20, 2016

rajatchopra commented Jul 12, 2016 •

edited

Loading

rajatchopra Jul 14, 2016 •

edited

Loading

knobunc Jul 14, 2016 •

edited

Loading

openshift-bot commented Jul 20, 2016 •

edited

Loading