Ab testing redone #9119

rajatchopra · 2016-06-01T20:28:40Z

Re-implementing A/B testing work with weights as an API field. Ref: #9070

smarterclayton · 2016-06-01T21:03:41Z

pkg/route/registry/route/strategy.go

@@ -45,7 +45,10 @@ func (s routeStrategy) PrepareForCreate(obj runtime.Object) {
 	route.Status = api.RouteStatus{}
 	// Limit to kind/name
 	// TODO: convert to LocalObjectReference
-	route.Spec.To = kapi.ObjectReference{Kind: route.Spec.To.Kind, Name: route.Spec.To.Name}
+	route.Spec.To = api.RouteTargetReference{Kind: route.Spec.To.Kind, Name: route.Spec.To.Name, Weight: 1}


This block is no longer necessary - you can remove it and the block below because we no longer have fields to strip.

pravisankar · 2016-06-02T20:37:10Z

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

  mode http
  option redispatch
  option forwardfor
+    {{ with $balanceAlgo := index $cfg.Annotations "haproxy.balance" }}


Namespace the annotation just like other existing k8s/openshift annotations, router.openshift.io/haproxy.balance?

rajatchopra · 2016-06-02T23:47:45Z

All comments taken care of. Testcases still need to be added. Review please. DO NOT MERGE YET - some cosmetics still to go in.
Thanks.

smarterclayton · 2016-06-02T23:50:45Z

Will look, thanks

rajatchopra · 2016-06-08T21:39:33Z

@smarterclayton Any inputs here. Working on the cli in a separate PR.

smarterclayton · 2016-06-08T21:55:59Z

Cli for viewing or changing? If latter a quick proposal write up would be
best.

On Jun 8, 2016, at 5:39 PM, Rajat Chopra [email protected] wrote:

@smarterclayton https://github.com/smarterclayton Any inputs here.
Working on the cli in a separate PR.

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

knobunc · 2016-06-24T13:57:40Z

[test]

jorgemoralespou · 2016-06-25T15:17:44Z

@rajatchopra @smarterclayton @knobunc Looking at the PR looks like weights would be first class citizen of Routing for AB testing. I just wonder how this will scale for next requests to support AB testing based on HTTP headers and cookies?
I understand that our routing will not support AB testing logic based on L7 but these 2 will come and be pretty ovbious should be supported by default routing and not additional router implementation. Also if weights are API, so should be others.

smarterclayton · 2016-06-27T00:12:50Z

We should definitely discuss those use cases.

On Sat, Jun 25, 2016 at 11:17 AM, Jorge Morales Pou <
[email protected]> wrote:

@rajatchopra https://github.com/rajatchopra @smarterclayton
https://github.com/smarterclayton @knobunc https://github.com/knobunc
Looking at the PR looks like weights would be first class citizen of
Routing for AB testing. I just wonder how this will scale for next requests
to support AB testing based on HTTP headers and cookies?
I understand that our routing will not support AB testing logic based on
L7 but these 2 will come and be pretty ovbious should be supported by
default routing and not additional router implementation. Also if weights
are API, so should be others.

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

smarterclayton · 2016-06-27T00:49:52Z

pkg/route/api/validation/validation.go

+	}
+
+	if len(route.Spec.AlternateBackends) > 3 {
+		result = append(result, field.Required(specPath.Child("to"), "cannot specify more than 3 additional services"))


this is wrong, it should be specPath.Child("alternateBackends")

rajatchopra · 2016-06-27T23:20:42Z

redo aborted [test]
@jorgemoralespou Annotations are exposed through the router template now. Any reasonable demands on changing the router behaviour should be possible through that. The current weight as an API field is the most straightforward way of specifying multiple backends/services. The template can be changed to ignore these fields and choose anything specified in the 'route' annotations.
Any specific use-case that you have should definitely be discussed.

@smarterclayton The fixes suggested by you have been put in. The tests flake randomly still but they are unrelated. If you could mark this PR for merge if everything else is okay. Thanks.

smarterclayton · 2016-06-28T04:19:11Z

[test] so failures are unrelated to router changes?

rajatchopra · 2016-06-28T14:11:58Z

[test]

rajatchopra · 2016-06-28T17:37:43Z

[test]
Again. Previous test aborted on this flake

openshift-bot · 2016-06-29T13:35:37Z

Evaluated for origin test up to 5a19e50

openshift-bot · 2016-06-29T14:56:50Z

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

smarterclayton · 2016-06-29T15:57:53Z

LGTM [merge]

rajatchopra · 2016-06-29T18:28:00Z

Unrelated failures. Retry [merge]

openshift-bot · 2016-06-29T19:55:45Z

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

rajatchopra · 2016-06-29T22:59:29Z

[merge] once more

openshift-bot · 2016-06-29T23:00:37Z

Evaluated for origin merge up to 5a19e50

JacobTanenbaum · 2016-07-19T14:09:11Z

pkg/router/template/router.go

+	return false
+}
+
+func matchString(pattern, s string) bool {


In #9671 changing matchString to matchPattern in order to change default behavior to anchor on the beginning and end of the line. To avoid confusion with regexp.MatchString() the name was changed

jorgemoralespou · 2016-07-20T09:16:03Z

@rajatchopra @smarterclayton @knobunc Where would be the best place to discuss those other use cases we have left that makes a better ab testing?

smarterclayton · 2016-07-20T15:58:16Z

Issue probably in github that lays out what we want to achieve and the use
cases - we'll summarize that into a card as necessary, but issues are the
broadest contributor place.

On Wed, Jul 20, 2016 at 5:16 AM, Jorge Morales Pou <[email protected]

wrote:

@rajatchopra https://github.com/rajatchopra @smarterclayton
https://github.com/smarterclayton @knobunc https://github.com/knobunc
Where would be the best place to discuss those other use cases we have left
that makes a better ab testing?

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

rajatchopra mentioned this pull request Jun 1, 2016

ab testing #9070

Closed

smarterclayton reviewed Jun 1, 2016
View reviewed changes

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

pravisankar reviewed Jun 2, 2016
View reviewed changes

rajatchopra force-pushed the AB branch from fadae3d to b934053 Compare June 2, 2016 23:45

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2016

rajatchopra force-pushed the AB branch from b934053 to 02405f3 Compare June 4, 2016 00:19

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

rajatchopra force-pushed the AB branch 2 times, most recently from 45dc612 to 2adf06a Compare June 6, 2016 18:18

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 6, 2016

rajatchopra force-pushed the AB branch 6 times, most recently from c32436e to 2155506 Compare June 6, 2016 23:58

rajatchopra force-pushed the AB branch 4 times, most recently from 5003f24 to 234c89c Compare June 23, 2016 22:43

rajatchopra force-pushed the AB branch from 234c89c to 89bc90b Compare June 24, 2016 21:52

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

smarterclayton reviewed Jun 27, 2016
View reviewed changes

ab testing

5a19e50

rajatchopra force-pushed the AB branch from 89bc90b to 5a19e50 Compare June 27, 2016 19:44

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2016

openshift-bot merged commit 4b195a7 into openshift:master Jun 30, 2016

JacobTanenbaum reviewed Jul 19, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ab testing redone #9119

Ab testing redone #9119

rajatchopra commented Jun 1, 2016 •

edited

Loading

smarterclayton Jun 1, 2016

pravisankar Jun 2, 2016

rajatchopra commented Jun 2, 2016 •

edited

Loading

smarterclayton commented Jun 2, 2016

rajatchopra commented Jun 8, 2016

smarterclayton commented Jun 8, 2016

knobunc commented Jun 24, 2016

jorgemoralespou commented Jun 25, 2016

smarterclayton commented Jun 27, 2016

smarterclayton Jun 27, 2016

rajatchopra commented Jun 27, 2016

smarterclayton commented Jun 28, 2016

rajatchopra commented Jun 28, 2016

rajatchopra commented Jun 28, 2016 •

edited

Loading

openshift-bot commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

smarterclayton commented Jun 29, 2016

rajatchopra commented Jun 29, 2016

openshift-bot commented Jun 29, 2016 •

edited

Loading

rajatchopra commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

JacobTanenbaum Jul 19, 2016

jorgemoralespou commented Jul 20, 2016

smarterclayton commented Jul 20, 2016

Ab testing redone #9119

Ab testing redone #9119

Conversation

rajatchopra commented Jun 1, 2016 • edited Loading

smarterclayton Jun 1, 2016

Choose a reason for hiding this comment

pravisankar Jun 2, 2016

Choose a reason for hiding this comment

rajatchopra commented Jun 2, 2016 • edited Loading

smarterclayton commented Jun 2, 2016

rajatchopra commented Jun 8, 2016

smarterclayton commented Jun 8, 2016

knobunc commented Jun 24, 2016

jorgemoralespou commented Jun 25, 2016

smarterclayton commented Jun 27, 2016

smarterclayton Jun 27, 2016

Choose a reason for hiding this comment

rajatchopra commented Jun 27, 2016

smarterclayton commented Jun 28, 2016

rajatchopra commented Jun 28, 2016

rajatchopra commented Jun 28, 2016 • edited Loading

openshift-bot commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

smarterclayton commented Jun 29, 2016

rajatchopra commented Jun 29, 2016

openshift-bot commented Jun 29, 2016 • edited Loading

rajatchopra commented Jun 29, 2016

openshift-bot commented Jun 29, 2016

JacobTanenbaum Jul 19, 2016

Choose a reason for hiding this comment

jorgemoralespou commented Jul 20, 2016

smarterclayton commented Jul 20, 2016

rajatchopra commented Jun 1, 2016 •

edited

Loading

rajatchopra commented Jun 2, 2016 •

edited

Loading

rajatchopra commented Jun 28, 2016 •

edited

Loading

openshift-bot commented Jun 29, 2016 •

edited

Loading