added an enviroment variable for balance alg. and ability to disable route cookies #11984

JacobTanenbaum · 2016-11-21T18:28:48Z

added ROUTER_DEFAULT_BALANCE which allows the default balance algorithm
for edge and reencrypt routes to be set for the cluster

added annotation haproxy.router.openshift.io/use_cookies which when set
to false | FALSE omits the creation of a cookie so the balance algorithm
is used to determine endpoints everytime a request is made.

docs PR will follow once the new names are agreed on

Trello card https://trello.com/c/B56OdzdS

JacobTanenbaum · 2016-11-21T18:29:05Z

@knobunc

knobunc

LGTM @openshift/networking PTAL

knobunc · 2016-11-29T15:11:18Z

[test]

knobunc · 2016-11-29T20:02:25Z

We need to update the docs with the new ENV and with the annotation (and make sure all annotations are documented which may also resolve https://trello.com/c/hBWtixqA/299-add-tests-and-documentation-for-ddos-router-annotations-ingress and https://trello.com/c/62X44q90/276-document-the-router-template-format-and-how-to-use-annotations-ingress)

ramr · 2016-11-30T00:44:54Z

The code changes look good. A couple of thoughts though.

I think we should change the name to be a lil' more descriptive of what it is used for. Something like ROUTER_LOAD_BALANC{E,ING}_ALGO{,RE,RITHM} maybe?
There's an easter egg in there btw!! ;^)
haproxy.router.openshift.io/use_cookies ==> haproxy.router.openshift.io/disable_cookies
It might be better to use an inverted form of use_cookies - disable_cookies maybe? Set it to true
to make it so (didn't want to say enable that disabled functionality). And yeah, the false/true
checks for that annotation would need to be inverted.
I kind of like that as it means by default cookies are enabled unless you explicitly disable them for
a route.

JacobTanenbaum · 2016-11-30T16:02:53Z

Docs PR 3301

ramr · 2016-11-30T21:10:06Z

LGTM - tests flaked due yum/dnf cache sync.
re[test]

ramr

LGTM

ramr · 2016-11-30T21:12:47Z

@JacobTanenbaum squash the commits and this is good to go - thx

pecameron

Should there be oadm router options to create the environment variable and annotation?

pecameron · 2016-12-01T15:03:39Z

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

@@ -459,7 +465,11 @@ backend be_secure_{{$cfgIdx}}
  balance {{ $balanceAlgo }}
      {{ end }}
    {{ else }}
+      {{ if (matchPattern "roundrobin|leastconn|source" (env "ROUTER_LOAD_BALANCE_ALGORITHM" "")) }}
+  balance {{ env "ROUTER_LOAD_BALANCE_ALGORITHM" "leastconn"}}


Should this environment variable be added to the DC as well? It would make it easier for the user and make the doc easier as well.

We don't expose everything in the oadm. They can do 'oc set env dc/router ROUTER_LOAD_BALANCE_ALGORITHM=source' if they want to change it.

We need to make sure that we document the new environment variable and annotation. And we should provide a snippet showing how to set ENV and annotations from the cli (with a link to the full docs).

knobunc · 2016-12-01T15:17:02Z

Last flake was rpm pull error. re-[test]

route cookies added ROUTER_DEFAULT_BALANCE which allows the default balance algorithm for edge and reencrypt routes to be set for the cluster added annotation haproxy.router.openshift.io/use_cookies which when set to false | FALSE omits the creation of a cookie so the balance algorithm is used to determine endpoints everytime a request is made. changelog: - changed the name of the annotation haproxy.router.openshift.io/use_cookies to haproxy.router.openshift.io/disable_cookies - change env variable ROUTER_DEFAULT_LOAD_BALANCE to ROUTER_LOAD_BALANCE_ALGORITHM

openshift-bot · 2016-12-01T17:06:01Z

Evaluated for origin test up to 377284f

openshift-bot · 2016-12-01T18:39:09Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11871/) (Base Commit: a38151f)

JacobTanenbaum · 2016-12-06T13:06:16Z

@knobunc is this good to merge?

knobunc · 2016-12-06T13:14:43Z

[merge]

knobunc · 2016-12-07T15:18:46Z

[merge] failure was flake #11024 (logs: https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12074/)

openshift-bot · 2016-12-07T15:21:21Z

Evaluated for origin merge up to 377284f

openshift-bot · 2016-12-11T15:05:11Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11871/) (Base Commit: cb5c7e5) (Image: devenv-rhel7_5526)

smarterclayton · 2016-12-12T20:12:57Z

Starting since this merged, I've been seeing lots of TestRouter flakes. Can you help prove that this is not this PR?

smarterclayton · 2016-12-12T20:32:58Z

failed  TestRouter
I1212 13:52:16.448562     330 router_http_server.go:303] Started, serving at 172.18.3.29:8080
I1212 13:52:16.448644     330 router_http_server.go:303] Started, serving at 172.18.3.29:8888
I1212 13:52:16.448693     330 router_http_server.go:303] Started, serving at 172.18.3.29:8889
I1212 13:52:16.449294     330 router_http_server.go:335] Started, serving TLS at 172.18.3.29:8443
I1212 13:52:49.357958     330 router_http_server.go:208] Stopping listener at 172.18.3.29:8080
I1212 13:52:49.357987     330 router_http_server.go:208] Stopping listener at 172.18.3.29:8888
I1212 13:52:49.357996     330 router_http_server.go:208] Stopping listener at 172.18.3.29:8889
I1212 13:52:49.358010     330 router_http_server.go:208] Stopping listener at 172.18.3.29:8443
--- FAIL: TestRouter (32.91s)
	router_test.go:1229: timed out attempting to send to channel: {"type":"ADDED","object":{"kind":"Endpoints","apiVersion":"v1","metadata":{"name":"example","namespace":"rotorouter","creationTimestamp":null},"subsets":[{"addresses":[{"ip":"172.18.3.29"}],"ports":[{"port":8888}]}]}}
E1212 13:52:49.358048     330 router_http_server.go:309] HTTP server failed: accept tcp 172.18.3.29:8080: use of closed network connection
E1212 13:52:49.358048     330 router_http_server.go:309] HTTP server failed: accept tcp 172.18.3.29:8888: use of closed network connection
FAIL

knobunc · 2016-12-12T20:50:49Z

@JacobTanenbaum: Here's the link to the test queue:
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/buildTimeTrend

Unfortunately this PR is the only thing that touches code in roughly the right area before the failures start.

knobunc · 2016-12-13T14:43:45Z

@JacobTanenbaum you can ignore this (in case you haven't seen the discussion in #12236)

knobunc approved these changes Nov 29, 2016

View reviewed changes

ramr reviewed Nov 30, 2016

View reviewed changes

pecameron reviewed Dec 1, 2016

View reviewed changes

JacobTanenbaum force-pushed the stick-sessions branch from fdfd901 to 377284f Compare December 1, 2016 15:43

openshift-bot merged commit e40d2bf into openshift:master Dec 11, 2016

smarterclayton mentioned this pull request Dec 12, 2016

Revert sticky sessions #12232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added an enviroment variable for balance alg. and ability to disable route cookies #11984

added an enviroment variable for balance alg. and ability to disable route cookies #11984

JacobTanenbaum commented Nov 21, 2016 •

edited by knobunc

Loading

JacobTanenbaum commented Nov 21, 2016

knobunc left a comment

knobunc commented Nov 29, 2016

knobunc commented Nov 29, 2016

ramr commented Nov 30, 2016

JacobTanenbaum commented Nov 30, 2016

ramr commented Nov 30, 2016

ramr left a comment

ramr commented Nov 30, 2016

pecameron left a comment

pecameron Dec 1, 2016

knobunc Dec 1, 2016

knobunc commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

JacobTanenbaum commented Dec 6, 2016

knobunc commented Dec 6, 2016

knobunc commented Dec 7, 2016 •

edited

Loading

openshift-bot commented Dec 7, 2016

openshift-bot commented Dec 11, 2016 •

edited

Loading

smarterclayton commented Dec 12, 2016

smarterclayton commented Dec 12, 2016

knobunc commented Dec 12, 2016

knobunc commented Dec 13, 2016

added an enviroment variable for balance alg. and ability to disable route cookies #11984

added an enviroment variable for balance alg. and ability to disable route cookies #11984

Conversation

JacobTanenbaum commented Nov 21, 2016 • edited by knobunc Loading

JacobTanenbaum commented Nov 21, 2016

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented Nov 29, 2016

knobunc commented Nov 29, 2016

ramr commented Nov 30, 2016

JacobTanenbaum commented Nov 30, 2016

ramr commented Nov 30, 2016

ramr left a comment

Choose a reason for hiding this comment

ramr commented Nov 30, 2016

pecameron left a comment

Choose a reason for hiding this comment

pecameron Dec 1, 2016

Choose a reason for hiding this comment

knobunc Dec 1, 2016

Choose a reason for hiding this comment

knobunc commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

JacobTanenbaum commented Dec 6, 2016

knobunc commented Dec 6, 2016

knobunc commented Dec 7, 2016 • edited Loading

openshift-bot commented Dec 7, 2016

openshift-bot commented Dec 11, 2016 • edited Loading

smarterclayton commented Dec 12, 2016

smarterclayton commented Dec 12, 2016

knobunc commented Dec 12, 2016

knobunc commented Dec 13, 2016

JacobTanenbaum commented Nov 21, 2016 •

edited by knobunc

Loading

knobunc commented Dec 7, 2016 •

edited

Loading

openshift-bot commented Dec 11, 2016 •

edited

Loading