grpc support in fortio (grpc load testing) by ldemailly · Pull Request #519 · istio/istio

ldemailly · 2017-08-01T01:54:52Z

fortio support for grpc
fixes #451

fortio now supports grpc testing as well
docker images are created for fortio, grpcping, echosrv

Gazelle doesn’t seem to generate build rule for proto

So #517 can be better/easier

…or programatic calls Also auto set the testing function for http by default

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)

@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

Used master for conflicts

…h services) For #451

sebastienvas · 2017-08-04T21:46:16Z

prow/istio-presubmit.sh

 bazel test //...

+echo 'Pushing Images'
+(cd devel/fortio make TAG="${GIT_SHA}")


don't you need a ; ?
(cd devel/fortio; make TAG="${GIT_SHA}")

sebastienvas · 2017-08-04T21:52:04Z

prow/istio-presubmit.sh and makefile look legit beside the typo

ldemailly · 2017-08-04T22:39:59Z

the presubmit part needs istio/test-infra#366

+ reduce the verbosity of http

douglas-reid

Only small nits. I don't need to review again (unless there are major changes).

douglas-reid · 2017-08-06T22:14:56Z

devel/fortio/cmd/grpcping/pingsrv.go

+// GODEBUG="http2debug=2" GRPC_GO_LOG_VERBOSITY_LEVEL=99 GRPC_GO_LOG_SEVERITY_LEVEL=info grpcping -loglevel debug
+
+var (
+	countFlag     = flag.Int("n", 1, "how many ping the client will send")


super nit: "how many ping (s) the client will send"

douglas-reid · 2017-08-06T22:18:03Z

devel/fortio/cmd/grpcping/pingsrv.go

+
+func main() {
+	flag.Parse()
+	if *hostFlag != "" {


in the future, you may want to consider https://github.com/spf13/cobra to allow subcommands that may be better suited for this purpose (instead of grpcping -host ..., you would have something like grpcping server ... which might be easier to grok (and allow file separation of client/server code)).

douglas-reid · 2017-08-06T22:20:39Z

devel/fortio/cmd/grpcping/pingsrv.go

+	"istio.io/istio/devel/fortio"
+
+	context "golang.org/x/net/context"
+


nit: unnecessary separation of import blocks.

other nit: here and elsewhere, you are using import blocks of the order: stdlib, istio, external. elsewhere in istio, it is typically: stdlib, external, istio.

my IDE or gofmt seems to do the ordering but I'll see if a change sticks

douglas-reid · 2017-08-06T22:22:15Z

devel/fortio/fortiogrpc/grpcrunner.go

+
+// TestGrpc exercises Grpc health check at the target QPS.
+// To be set as the Function in RunnerOptions.
+func TestGrpc(t int) {


food for thought: i know this matches other bits, but this always makes me do a double-take to ensure that I'm not in a *_test.go file.

yeah, on the other hand fortio is all about testing but I'll see if I can come up with something

douglas-reid · 2017-08-06T22:24:00Z

devel/fortio/fortiogrpc/grpcrunner_test.go

+
+// DynamicGrpcHealthServer starts and returns the port where a Grpc Health
+// server is running. It runs until error or program exit (separate go routine)
+func DynamicGrpcHealthServer() int {


small nit: initialisms, like GRPC, are supposed to be non-mixed-case (https://github.com/golang/go/wiki/CodeReviewComments#initialisms).

I was happy the linter didn't know about GRPC but you do :-)

changing it but for the record I think it's less readable... but conventions are conventions so ok

Thx doug!

ldemailly · 2017-08-07T05:32:53Z

Thanks a lot @douglas-reid for all the reviews!

@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 Former-commit-id: 7b83910

fortio now supports grpc testing as well fixes #451 docker images are created for fortio, grpcping, echosrv * Simple grpc cli/server Gazelle doesn’t seem to generate build rule for proto * Added payload flag * User fortio logger, temp check in of .pb. file * Renamed to grpcping * Allow grpc_cli ls to work * ping and clock skew calculations * Add histograms to grpcping * Make http runner accessible/reusable as library So #517 can be better/easier * Return actual QPS and indicate default runner options aren't needed for programatic calls Also auto set the testing function for http by default * Comment on the provenance of the code * Adding gprc std health svc check (through -health) * Grpc runner as library * Fortio now has a -grpc flag to exercise grpc servers (that have health services) For #451 * Minor fixes/updates * Added comment per code review * Opsa spell check * Fix Dockerfile Builds 3 binaries which is kind of big Todo: consider 1 binary * Move grpc in separate package to keep echosrv binary small Move grpc in separate package to keep echosrv binary small and dependencies smaller when not using grpc (so echosrv which doesn’t use grpc stays 4mb instead of becoming 6mb) (Seemingly a side effect of go’s init() that not using still pulls it in) Individual images fortio.fortio fortio.echosrv fortio.grpcping as well as a combo images having all 3 * Adding images push * Adding missing GIT_SHA * Typo galore * Debug docker version * Got push failure, trying authorize + reduce the verbosity of http * Code review comments Thx doug! Former-commit-id: 13ced35

@douglas-reid

* make http runner available as library Moved a lot of the code from fortio main to the library so istio#517 can be better/easier (This is same as istio#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 Former-commit-id: 7b83910

fortio now supports grpc testing as well fixes istio#451 docker images are created for fortio, grpcping, echosrv * Simple grpc cli/server Gazelle doesn’t seem to generate build rule for proto * Added payload flag * User fortio logger, temp check in of .pb. file * Renamed to grpcping * Allow grpc_cli ls to work * ping and clock skew calculations * Add histograms to grpcping * Make http runner accessible/reusable as library So istio#517 can be better/easier * Return actual QPS and indicate default runner options aren't needed for programatic calls Also auto set the testing function for http by default * Comment on the provenance of the code * Adding gprc std health svc check (through -health) * Grpc runner as library * Fortio now has a -grpc flag to exercise grpc servers (that have health services) For istio#451 * Minor fixes/updates * Added comment per code review * Opsa spell check * Fix Dockerfile Builds 3 binaries which is kind of big Todo: consider 1 binary * Move grpc in separate package to keep echosrv binary small Move grpc in separate package to keep echosrv binary small and dependencies smaller when not using grpc (so echosrv which doesn’t use grpc stays 4mb instead of becoming 6mb) (Seemingly a side effect of go’s init() that not using still pulls it in) Individual images fortio.fortio fortio.echosrv fortio.grpcping as well as a combo images having all 3 * Adding images push * Adding missing GIT_SHA * Typo galore * Debug docker version * Got push failure, trying authorize + reduce the verbosity of http * Code review comments Thx doug! Former-commit-id: 13ced35

@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 Former-commit-id: 7b83910

fortio now supports grpc testing as well fixes #451 docker images are created for fortio, grpcping, echosrv * Simple grpc cli/server Gazelle doesn’t seem to generate build rule for proto * Added payload flag * User fortio logger, temp check in of .pb. file * Renamed to grpcping * Allow grpc_cli ls to work * ping and clock skew calculations * Add histograms to grpcping * Make http runner accessible/reusable as library So #517 can be better/easier * Return actual QPS and indicate default runner options aren't needed for programatic calls Also auto set the testing function for http by default * Comment on the provenance of the code * Adding gprc std health svc check (through -health) * Grpc runner as library * Fortio now has a -grpc flag to exercise grpc servers (that have health services) For #451 * Minor fixes/updates * Added comment per code review * Opsa spell check * Fix Dockerfile Builds 3 binaries which is kind of big Todo: consider 1 binary * Move grpc in separate package to keep echosrv binary small Move grpc in separate package to keep echosrv binary small and dependencies smaller when not using grpc (so echosrv which doesn’t use grpc stays 4mb instead of becoming 6mb) (Seemingly a side effect of go’s init() that not using still pulls it in) Individual images fortio.fortio fortio.echosrv fortio.grpcping as well as a combo images having all 3 * Adding images push * Adding missing GIT_SHA * Typo galore * Debug docker version * Got push failure, trying authorize + reduce the verbosity of http * Code review comments Thx doug! Former-commit-id: 13ced35

Signed-off-by: Kuat Yessenov <kuat@google.com>

* [kiali] issue-18798 fix some wrong yaml. Another (trivial) change is that this alphabetizes a couple settings to make comparison with the old helm charts easier. * gen check results

…cified (istio#519) Annotations for ServiceAccounts can now be specified with helm values Signed-off-by: Nabeel Sadiq <nabeel.sadiq@paytrix.io> Signed-off-by: Pavel Nikolov <pavelnikolov@users.noreply.github.com> Co-authored-by: Pavel Nikolov <pavelnikolov@users.noreply.github.com>

ldemailly added 10 commits July 26, 2017 19:41

Wip/test simple grace cli/server

efc39e9

Gazelle doesn’t seem to generate build rule for proto

Added payload flag

079788d

Merge branch 'master' into rawvm-register-1

5d1690f

User fortio logger, temp check in of .pb. file

95e0b28

Renamed to grpcping

d1c3fe4

Allow grpc_cli ls to work

0093fd0

Wip ping and clock skew calculations

6987972

Add histograms to grpcping

b6f0439

Merge branch 'master' into rawvm-register-1

e62c09c

Make http runner accessible/reusable as library

8aa9d2c

So #517 can be better/easier

googlebot added the cla: yes label Aug 1, 2017

ldemailly requested a review from douglas-reid August 1, 2017 01:55

Return actual QPS and indicate default runner options aren't needed f…

a444f56

…or programatic calls Also auto set the testing function for http by default

ldemailly mentioned this pull request Aug 1, 2017

Switch to fortio for RateLimit testing #517

Merged

Comment on the provenance of the code

5b11f83

ldemailly added a commit that referenced this pull request Aug 1, 2017

make http runner available as library

a1f87a3

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)

ldemailly mentioned this pull request Aug 1, 2017

make http runner available as library, take 2 #520

Merged

ldemailly removed the request for review from douglas-reid August 1, 2017 03:02

istio-merge-robot added the approved label Aug 1, 2017

Merge branch 'master' into rawvm-register-1

e8cf949

Used master for conflicts

ldemailly changed the title ~~make http runner available as library~~ wip grpc fortio Aug 1, 2017

ldemailly added 2 commits August 1, 2017 17:33

Adding gprc std health svc check (through -health)

7668c5b

Merge branch 'master' into rawvm-register-1

2019f6d

istio-merge-robot removed the approved label Aug 2, 2017

ldemailly added 2 commits August 1, 2017 19:05

Grpc runner as library

0e0d5fb

Fortio now has a -grpc flag to exercise grpc servers (that have healt…

b9f6879

…h services) For #451

ldemailly changed the title ~~wip grpc fortio~~ grpc support in fortio (grpc load testing) Aug 2, 2017

istio-merge-robot added the approved label Aug 2, 2017

ldemailly requested a review from douglas-reid August 2, 2017 03:38

ldemailly added 2 commits August 4, 2017 14:35

Adding images push

69e9c6f

Adding missing GIT_SHA

9f0cdca

istio deleted a comment from istio-testing Aug 4, 2017

sebastienvas reviewed Aug 4, 2017

View reviewed changes

Typo galore

34711c5

ldemailly added 2 commits August 4, 2017 16:58

Merge branch 'master' into rawvm-register-1

a1257f8

Debug docker version

93915da

istio deleted a comment from istio-testing Aug 5, 2017

Got push failure, trying authorize

1f03365

+ reduce the verbosity of http

douglas-reid approved these changes Aug 6, 2017

View reviewed changes

Code review comments

49a69a5

Thx doug!

ldemailly merged commit 13ced35 into master Aug 7, 2017

ldemailly deleted the rawvm-register-1 branch August 7, 2017 05:32

mandarjog pushed a commit that referenced this pull request Oct 31, 2017

Use same date label for both hubs (#519)

0910cd2

guptasu pushed a commit to guptasu/istio that referenced this pull request Jun 11, 2018

grpc attributes (istio#519)

35efff0

Signed-off-by: Kuat Yessenov <kuat@google.com>

danehans pushed a commit to danehans/istio that referenced this pull request Nov 2, 2021

update gke version matrix (istio#519)

ac75465

		"istio.io/istio/devel/fortio"

		context "golang.org/x/net/context"

Conversation

ldemailly commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastienvas commented Aug 4, 2017

Uh oh!

ldemailly commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Aug 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ldemailly commented Aug 1, 2017 •

edited

Loading

ldemailly commented Aug 4, 2017 •

edited

Loading