Add mysql db to ratings service in bookinfo app. #574

saumoh · 2017-08-17T16:19:34Z

Tracked in #573 and discussed in #473, adding mysql db access to the ratings service.
cc @rshriram @AnthonyAmanse @ldemailly

Added ratings-v2

googlebot · 2017-08-17T16:19:38Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

istio-testing · 2017-08-17T16:19:41Z

Hi @saumoh. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

AnthonyAmanse · 2017-08-17T20:13:03Z

Confirmed

rshriram · 2017-08-17T20:19:07Z

/ok-to-test

rshriram · 2017-08-17T20:19:22Z

Please sync with master

rshriram

Why do we need another rule for reviews?

rshriram · 2017-08-17T20:20:36Z

samples/apps/bookinfo/BUILD

@@ -11,6 +11,7 @@ filegroup(
        "route-rule-reviews-50-v3.yaml",
        "route-rule-reviews-test-v2.yaml",
        "route-rule-reviews-v2-v3.yaml",
+	"route-rule-ratings-test-v2.yaml"


Rename to ...ratings-mysql.yaml for clarity ?

rshriram · 2017-08-17T20:22:20Z

samples/apps/bookinfo/route-rule-ratings-test-v2.yaml

+---
+type: route-rule
+spec:
+  name: reviews-test-v2


What if this for?

The reviews version v1 does not query the micro service ratings only version 2 and 3 of reviews services do that.

right.. But to keep in line with the tutorials and story flow, I suggest picking up from last step of request routing task (all traffic goes to reviews v3). You wont even need a rule here for reviews

rshriram · 2017-08-18T02:51:01Z

@saumoh CLA please

rshriram · 2017-08-18T02:51:13Z

cc @frankbu

frankbu · 2017-08-18T16:02:07Z

samples/apps/bookinfo/route-rule-ratings-test-v2.yaml

@@ -0,0 +1,17 @@
+type: route-rule
+spec:
+  name: ratings-test-v2


Since the rule has no specific (test related) match criteria and just sends 100% of traffic to v2, I think it might be better to just name this rule ratings-default and use istioctl replace rather than creating a second rule with higher precedence. I guess it depends on how we integrate this with the tasks and story flow. If we plan to run it and then delete before moving on to the next task, then I guess it's fine as is.

The main goal of this rule was to be able to write a e2e test where I can use rule(s) to direct traffic to v2/v3 of reviews and ratings(v2).
I can write a new route-rule file which is specific to the tasks and story flow. My preference is to separate that out to a different pr if thats ok with you folks. wdyt?

frankbu · 2017-08-18T16:14:36Z

samples/apps/bookinfo/bookinfo.yaml

@@ -82,6 +82,34 @@ spec:
        ports:
        - containerPort: 9080
 ---
+apiVersion: extensions/v1beta1
+kind: Deployment


Should we move this deployment to a separate yaml file (e.g., bookinfo-ratings-v2.yaml)? Starting all versions of all services up front with one yaml file is not such a good idea, especially as we continue to add more versions with additional prereqs.

Will e2e testing be able to load multiple services, at init time. if so then i can move it over these new services to a different file.

frankbu · 2017-08-18T16:18:39Z

samples/apps/bookinfo/bookinfo.yaml

+    app: mysqldb
+---
+apiVersion: extensions/v1beta1
+kind: Deployment


Would it be better to launch mysql outside of the mesh? A more realistic scenario? I wonder if it even works, since several users have been noticing issues with accessing non-istiofied TCP and/or headless services. Would be a good test anyway.

afaik today there is no easy way to bypass the sidecar at reviews(v3) specific to a version of ratings(v2). Right off the bat there wouldn't be a way to test(e2e) this out right now?

implementation wise there would be 2 versions of the bookinfo-ratings-(x|y).yaml. One where the mysql is loaded with annotation to skip sidecar and one with sidecar enabled.

What I meant is to run the mysqldb service and deployment outside of the mesh, that is, put them into a separate yaml file and apply without kube-inject. Ratings v3 will still use the sidecar, it will just be calling an ordinary mysqldb service instead of an istiofied one.

@douglas-reid

* make http runner available as library Moved a lot of the code from fortio main to the library so #517 can be better/easier (This is same as #519 with attempt at preserving history and without the grpc wip) * Added test, moved echosrv code to lib, ... Added test, moved echosrv code to lib, created DynamicHTTP for test, renames per @douglas-reid code review comments * Better initialization syntax * Add a negative testing

* Upgrade mixer to 0.2.0-c570a67 * Fix linter issue in devel/fortio/httprunner_test.go

* Upgrade rules_go to 0.5.2 * Fix bazel_to_go.py to vendor go_repository

Automatic merge from submit-queue Add PR template PR template to help adding release-note

…fNotPresent/g' (#532) * git ls-files|xargs sed -i .bak -e 's/PullPolicy: Always/PullPolicy: IfNotPresent/g' git ls-files|xargs sed -i .bak -e 's/PullPolicy: Always/PullPolicy: IfNotPresent/g' Fixes #441 * review comment

* Fix devel linters issue * Move TestDefaultRouting to Setup * Increase retries on mixer test

rshriram · 2017-08-28T21:30:15Z

/release-note-none

rshriram · 2017-08-28T21:31:00Z

/lgtm

ldemailly · 2017-08-28T22:44:05Z

can we fix
I0828 21:28:33.781] tests/e2e/framework/testInfo.go:95:2:warning: ineffectual assignment to tmpDir (ineffassign)

yutongz · 2017-08-28T23:03:24Z

/test all

istio-merge-robot · 2017-08-28T23:09:09Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @rshriram @saumoh

ldemailly · 2017-08-28T23:15:27Z

/lgtm

istio-merge-robot · 2017-08-28T23:16:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly, rshriram

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

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

~~OWNERS~~ [ldemailly,rshriram]

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

istio-merge-robot · 2017-08-28T23:32:18Z

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

istio-merge-robot · 2017-08-28T23:37:57Z

Automatic merge from submit-queue

ldemailly · 2017-08-28T23:59:29Z

I just noticed a bunch of

   image: quay.io/saurabh/...

can we change that to istio ? @saumoh would you mind filling a followup - tia!

istio-testing · 2017-08-29T00:06:56Z

@saumoh: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-suite-no_rbac-auth.sh	`b8e9f97`	link	`/test e2e-suite-no_rbac-auth`
prow/e2e-suite-rbac-auth.sh	`b8e9f97`	link	`/test e2e-suite-rbac-auth`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rshriram

Automatic merge from submit-queue Add mysql db to ratings service in bookinfo app. Tracked in #573 and discussed in #473, adding mysql db access to the ratings service. cc @rshriram @AnthonyAmanse @ldemailly Former-commit-id: 7b5271d

@rshriram

Automatic merge from submit-queue Add mysql db to ratings service in bookinfo app. Tracked in istio#573 and discussed in istio#473, adding mysql db access to the ratings service. cc @rshriram @AnthonyAmanse @ldemailly Former-commit-id: 7b5271d

@rshriram

Automatic merge from submit-queue Add mysql db to ratings service in bookinfo app. Tracked in #573 and discussed in #473, adding mysql db access to the ratings service. cc @rshriram @AnthonyAmanse @ldemailly Former-commit-id: 7b5271d

* Update common files * Update installer SHA * Remove protoc_gen_k8s_support_plugins It is causing errors like this: ``` istio.io/operator/pkg/apis/istio/v1alpha1 imports github.com/gogo/protobuf/protobuf/google/protobuf: module github.com/gogo/protobuf@latest found (v1.3.1), but does not contain package github.com/gogo/protobuf/protobuf/google/protobuf common/Makefile.common.mk:67: recipe for target 'tidy-go' failed make: *** [tidy-go] Error 1 make: *** [Makefile:111: gen-check] Error 2 ``` * make gen-check * update golden files

saumohos and others added 5 commits August 1, 2017 11:34

Starting point for adding mysql db service to the bookinfo application.

6d95f26

provide password for mysqldb as an env var.

664ae66

Added ratings-v2

58667c0

Merge pull request #1 from AnthonyAmanse/bookinfo-db

ed21c43

Added ratings-v2

Ratings service mysql db access e2e test

ff83622

istio-merge-robot added the release-note-label-needed label Aug 17, 2017

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Aug 17, 2017

istio-testing added the needs-ok-to-test label Aug 17, 2017

istio-testing removed the needs-ok-to-test label Aug 17, 2017

rshriram suggested changes Aug 17, 2017

View reviewed changes

Merge branch 'master' into bookinfo-db

f32cde1

rshriram requested a review from frankbu August 18, 2017 02:51

rshriram added this to the Istio 0.2 milestone Aug 18, 2017

frankbu reviewed Aug 18, 2017

View reviewed changes

ldemailly and others added 7 commits August 18, 2017 10:06

Upgrade mixer to 0.2.0-c570a67 (#524)

be92f3f

* Upgrade mixer to 0.2.0-c570a67 * Fix linter issue in devel/fortio/httprunner_test.go

Upgrade rules_go to 0.5.2 (#508)

8db4891

* Upgrade rules_go to 0.5.2 * Fix bazel_to_go.py to vendor go_repository

Update templates (#530)

f71c3fc

Add pr template (#525)

9570c78

Automatic merge from submit-queue Add PR template PR template to help adding release-note

Update e2e test to be less flaky (#533)

2b28ae4

* Fix devel linters issue * Move TestDefaultRouting to Setup * Increase retries on mixer test

rshriram removed the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Aug 28, 2017

istio-testing added release-note-none and removed release-note labels Aug 28, 2017

rshriram removed the do-not-merge Block automatic merging of a PR. label Aug 28, 2017

istio-testing added the lgtm label Aug 28, 2017

Merge branch 'master' into bookinfo-db

b8e9f97

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Aug 28, 2017

istio-merge-robot removed the lgtm label Aug 28, 2017

istio-testing assigned ldemailly Aug 28, 2017

istio-testing added the lgtm label Aug 28, 2017

rshriram added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Aug 28, 2017

istio-merge-robot merged commit 7b5271d into istio:master Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mysql db to ratings service in bookinfo app. #574

Add mysql db to ratings service in bookinfo app. #574

saumoh commented Aug 17, 2017 •

edited by rshriram

Loading

googlebot commented Aug 17, 2017

istio-testing commented Aug 17, 2017

AnthonyAmanse commented Aug 17, 2017 •

edited

Loading

rshriram commented Aug 17, 2017

rshriram commented Aug 17, 2017

rshriram left a comment

rshriram Aug 17, 2017

saumoh Aug 17, 2017

rshriram Aug 17, 2017

saumoh Aug 17, 2017

rshriram Aug 18, 2017

rshriram commented Aug 18, 2017

rshriram commented Aug 18, 2017

frankbu Aug 18, 2017

saumoh Aug 18, 2017

frankbu Aug 18, 2017

saumoh Aug 18, 2017

frankbu Aug 18, 2017

saumoh Aug 18, 2017

frankbu Aug 18, 2017

rshriram commented Aug 28, 2017

rshriram commented Aug 28, 2017

ldemailly commented Aug 28, 2017

yutongz commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

ldemailly commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

ldemailly commented Aug 28, 2017 •

edited

Loading

istio-testing commented Aug 29, 2017

Add mysql db to ratings service in bookinfo app. #574

Add mysql db to ratings service in bookinfo app. #574

Conversation

saumoh commented Aug 17, 2017 • edited by rshriram Loading

googlebot commented Aug 17, 2017

istio-testing commented Aug 17, 2017

AnthonyAmanse commented Aug 17, 2017 • edited Loading

rshriram commented Aug 17, 2017

rshriram commented Aug 17, 2017

rshriram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Aug 18, 2017

rshriram commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Aug 28, 2017

rshriram commented Aug 28, 2017

ldemailly commented Aug 28, 2017

yutongz commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

ldemailly commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

istio-merge-robot commented Aug 28, 2017

ldemailly commented Aug 28, 2017 • edited Loading

istio-testing commented Aug 29, 2017

saumoh commented Aug 17, 2017 •

edited by rshriram

Loading

AnthonyAmanse commented Aug 17, 2017 •

edited

Loading

ldemailly commented Aug 28, 2017 •

edited

Loading