Router template optimization #12882

louyihua · 2017-02-09T08:43:36Z

This PR introduces some optimizations to router template files, which simplify the structure of the template file, improve compatibility for future HAPROXY versions, and improve maintainancibility of template-related codes. Major optimizations include:

Move template helper functions, as well as the mapping structure, into a new .go file named template_helper.go
Replace the non-standard form listen stats :<port> which is forbidden by HAPROXY 1.7.0+ with the standard form listen stats and bind :<port>.
Introduce two new helper functions: firstMatch and isTrue. firstMatch matches the regexp given in the first argument with the following string arguments and returns the first matched string, while isTrue is a proxy for Go's strconv.ParseBool.
Change the env function to take variable args: env(name, def...). It accepts multiple default values, but only the first non-empty default value takes effect if the environment variable is not defined. If no default value is given, or all default values are empty, then it treats "" as the default value.
Eliminate many unnecessary {{if}}s from the template file through the introduction of firstMatch function. The result is the size of the template file is significantly reduced.

Final result of this PR is that the size of the template file is significantly reduced, and there are much fewer duplicating items which improves the maintaincibility.

knobunc · 2017-02-09T18:14:02Z

@ramr @JacobTanenbaum PTAL

louyihua · 2017-02-24T02:50:00Z

Anyone take a look?

knobunc · 2017-05-24T13:53:49Z

@louyihua This is really good stuff. We had talked about similar changes but you beat us to the implementation. If you can rebase this @JacobTanenbaum can help shepherd it in.

knobunc

This is great stuff. We need to think about what functions we expose in the template a bit since they are an API contract. But I really like the cleanliness of the resulting template.

knobunc · 2017-05-24T13:57:11Z

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

-{{ else }}
-  timeout http-keep-alive 300s
-{{ end }}
+  timeout connect {{firstMatch "[1-9][0-9]*(us|ms|s|m|h|d)?" (env "ROUTER_DEFAULT_CONNECT_TIMEOUT" "") "5s"}}


I was wondering if we should change env to take variable args, and if called without a default, assume "".

We can do that as a follow-on PR, but I wanted to get your thoughts. Alternatively, we can add a new env function that takes one arg... but I don't like that as much.

knobunc · 2017-05-24T14:02:11Z

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


-{{ if (matchPattern "true|TRUE" (env "ROUTER_ENABLE_COMPRESSION" "false")) }}
+{{ if isTrue (env "ROUTER_ENABLE_COMPRESSION" "false") }}


Should we make an "is" function, and have the first arg govern "true" vs "false"?

I don't really want to add isTrue and isFalse and whatever else people think of...

But I guess we can say not isTrue ...

Would we need to change the documentation because of the way isTrue is implemented using strconv? The amount of acceptable values increases 1, t, T, TRUE, true, True

knobunc · 2017-05-24T14:08:36Z

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

-{{ else }}
-  timeout http-keep-alive 300s
-{{ end }}
+  timeout connect {{firstMatch "[1-9][0-9]*(us|ms|s|m|h|d)?" (env "ROUTER_DEFAULT_CONNECT_TIMEOUT" "") "5s"}}


I was thinking that we should define a template variable at the top that holds this regex (and make other ones for common regexes.

e.g.:

{{with $timeSpecRE := "[1-9][0-9]*(us|ms|s|m|h|d)?"}}

timeout connect {{firstMatch $timeSpecRE (env ...) "5s"}}

JacobTanenbaum · 2017-05-24T15:18:13Z

pkg/router/template/template_helper.go

+	return v
+}
+
+func matchValues(s string, allowedValues ...string) bool {


Could you add the logging to matchValues similar to that of the helper functions you added

louyihua · 2017-05-25T07:26:13Z

@knobunc @JacobTanenbaum
This PR is rebased on current master. And the env function is re-designed to take zero or more default values, with the first non-empty default value string being used as the actual default value.

JacobTanenbaum · 2017-06-14T14:01:42Z

This is looking good, besides rebase I don't see anything specific that I have a problem with. The env function looks good. I will take a another look after rebase

louyihua · 2017-08-10T05:39:21Z

Fixed the template broken.

pecameron · 2017-08-10T18:15:42Z

/retest

louyihua · 2017-08-11T02:04:28Z

Seems all tests are unexpectly stopped

pecameron · 2017-08-11T20:04:35Z

@louyihua I am on PTO next week. @rajatchopra can help you.

louyihua · 2017-08-14T06:50:45Z

@rajatchopra Seems tests are failed strangely, maybe rerun the tests can solve the problem?

pecameron · 2017-08-21T17:49:09Z

/retest

pecameron · 2017-08-21T17:51:18Z

@louyihua I am back from PTO and will continue to help with this.

pecameron · 2017-08-21T17:51:56Z

@louyihua do the tests work on your local machine?

pecameron · 2017-08-21T19:06:41Z

@louyihua The end to end test has a regression that needs to be fixed:
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_end_to_end/1042/testReport/junit/github/com_openshift_origin_test_integration%20%20%20/TestRouter/

E0821 18:29:06.281218       1 ratelimiter.go:52] error executing template for file /var/lib/haproxy/conf/cert_config.map: template: haproxy-config.template:593:51: executing "/var/lib/haproxy/conf/cert_config.map" at <$cfg.TLSTermination>: wrong type for value; expected string; got route.TLSTerminationType
--- FAIL: TestRouter (31.73s)
	router_test.go:353: TC non-secure: url 0.0.0.0 alias unsecure.example.com protocol http
	router_test.go:355: TC non-secure failed: unavailable the entire time: endpoint not available
FAIL

This PR introduces some optimizations to router template files, which simplify the structure of the template file, improve compatibility for future HAPROXY versions, and improve maintainancibility of template-related codes. Major optimizations include: 1. Move template helper functions, as well as the mapping structure, into a new .go file named `template_helper.go` 2. Replace the non-standard form `listen stats :<port>` which is forbidden by HAPROXY 1.7.0+ with the standard form `listen stats` and `bind :<port>`. 3. Introduce two new helper functions: `firstMatch` and `isTrue`. `firstMatch` matches the regexp given in the first argument with the following string arguments and returns the first matched string, while `isTrue` is a proxy for Go's `strconv.ParseBool`. 4. Change the `env` function to take variable args: `env(name, def...)`. It accepts multiple default values, but only the first non-empty default value takes effect if the environment variable is not defined. If no default value is given, or all default values are empty, then it treats `""` as the default value. 5. Eliminate many unnecessary `{{if}}`s from the template file through the introduction of `firstMatch` function. The result is the size of the template file is significantly reduced.

pecameron · 2017-08-22T13:04:13Z

/retest

pecameron · 2017-08-22T13:58:23Z

@stevekuznetsov
extended_conformance_install_update is failing with:
ansible/playbooks/byo/config.retry'. [Errno 13] Permission denied:

Do you have any idea what we need to do to get past this? Thanks!

stevekuznetsov · 2017-08-22T14:11:59Z

That is a red herring. Looks like you hit #15874 which should be fixed now

/retest

pecameron

/lgtm

openshift-merge-robot · 2017-08-22T17:00:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: louyihua, pecameron, rajatchopra

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~images/router/OWNERS~~ [rajatchopra]
~~pkg/router/OWNERS~~ [rajatchopra]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

pecameron · 2017-08-22T18:45:36Z

@stevekuznetsov The ci/openshift-jenkins/extended_conformance_gce test previously passed. Actually all the tests passed. Where do we go from here?

stevekuznetsov · 2017-08-22T18:59:23Z

@pecameron GCE provision flake,

/retest

openshift-bot · 2017-08-23T08:43:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-08-23T10:22:00Z

/test all [submit-queue is verifying that this PR is safe to merge]

pecameron · 2017-08-23T13:03:34Z

@stevekuznetsov the ci/openshift-jenkins/extended_conformance_gce test seems to be stuck. How do we get it going?

stevekuznetsov · 2017-08-23T13:08:36Z

From my end I see it successful:

 ci/openshift-jenkins/extended_conformance_gce — Jenkins job succeeded.

pecameron · 2017-08-23T14:32:03Z

@stevekuznetsov Oops! My bad. I needed to refresh the page. All tests passed and then it did a /retest with everything.

pecameron · 2017-08-23T15:15:37Z

/retest

openshift-merge-robot · 2017-08-23T17:41:35Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-23T19:25:08Z

Automatic merge from submit-queue

pecameron · 2017-08-24T15:13:17Z

Documentation changes:
openshift/openshift-docs#5068

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2017

pweil- assigned knobunc Feb 9, 2017

pweil- added the priority/P2 label Feb 9, 2017

louyihua force-pushed the router-template-optimize branch 2 times, most recently from f78a53c to 07ad61a Compare February 10, 2017 01:38

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2017

louyihua force-pushed the router-template-optimize branch from 07ad61a to 5104d00 Compare February 10, 2017 09:24

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2017

louyihua force-pushed the router-template-optimize branch from 5104d00 to f161234 Compare March 7, 2017 08:33

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 7, 2017

louyihua force-pushed the router-template-optimize branch from f161234 to d6cf2ed Compare March 8, 2017 00:57

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 14, 2017

knobunc requested a review from JacobTanenbaum May 24, 2017 13:52

knobunc reviewed May 24, 2017

View reviewed changes

JacobTanenbaum reviewed May 24, 2017

View reviewed changes

louyihua force-pushed the router-template-optimize branch from d6cf2ed to 07f978a Compare May 25, 2017 07:22

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

louyihua force-pushed the router-template-optimize branch from 07f978a to c917b69 Compare May 31, 2017 07:13

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2017

louyihua force-pushed the router-template-optimize branch from 2d47da0 to 2ed8547 Compare August 22, 2017 06:39

pecameron approved these changes Aug 22, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 22, 2017

openshift-merge-robot merged commit 5be9974 into openshift:master Aug 23, 2017

louyihua deleted the router-template-optimize branch August 30, 2017 01:19


		{{ if (matchPattern "true\|TRUE" (env "ROUTER_ENABLE_COMPRESSION" "false")) }}
		{{ if isTrue (env "ROUTER_ENABLE_COMPRESSION" "false") }}

Router template optimization #12882

Router template optimization #12882

Conversation

louyihua commented Feb 9, 2017 • edited Loading

knobunc commented Feb 9, 2017

louyihua commented Feb 24, 2017

knobunc commented May 24, 2017

knobunc left a comment

Choose a reason for hiding this comment

knobunc May 24, 2017

Choose a reason for hiding this comment

knobunc May 24, 2017

Choose a reason for hiding this comment

knobunc May 24, 2017

Choose a reason for hiding this comment

JacobTanenbaum May 24, 2017

Choose a reason for hiding this comment

knobunc May 24, 2017

Choose a reason for hiding this comment

JacobTanenbaum May 24, 2017

Choose a reason for hiding this comment

louyihua commented May 25, 2017

JacobTanenbaum commented Jun 14, 2017

louyihua commented Aug 10, 2017

pecameron commented Aug 10, 2017

louyihua commented Aug 11, 2017

pecameron commented Aug 11, 2017

louyihua commented Aug 14, 2017

pecameron commented Aug 21, 2017

pecameron commented Aug 21, 2017

pecameron commented Aug 21, 2017

pecameron commented Aug 21, 2017 • edited by stevekuznetsov Loading

pecameron commented Aug 22, 2017

pecameron commented Aug 22, 2017

stevekuznetsov commented Aug 22, 2017

pecameron left a comment

Choose a reason for hiding this comment

openshift-merge-robot commented Aug 22, 2017

pecameron commented Aug 22, 2017 • edited Loading

stevekuznetsov commented Aug 22, 2017

openshift-bot commented Aug 23, 2017

openshift-merge-robot commented Aug 23, 2017

pecameron commented Aug 23, 2017

stevekuznetsov commented Aug 23, 2017

pecameron commented Aug 23, 2017

pecameron commented Aug 23, 2017

openshift-merge-robot commented Aug 23, 2017

openshift-merge-robot commented Aug 23, 2017

pecameron commented Aug 24, 2017

louyihua commented Feb 9, 2017 •

edited

Loading

pecameron commented Aug 21, 2017 •

edited by stevekuznetsov

Loading

pecameron commented Aug 22, 2017 •

edited

Loading