Switch to fortio for RateLimit testing by douglas-reid · Pull Request #517 · istio/istio

douglas-reid · 2017-07-31T23:04:31Z

Running with WRK seems to cause issues in local runs, related to amount of traffic generated. It also is yet another dep for the framework to establish.

This PR removes these concerns (hopefully) and reduces wait times inside of TestRateLimit, also reducing the amount of time running the load test down from 2m to 30s.

Fixes: #494

Use fortio instead of wrk in e2e tests

ldemailly · 2017-07-31T23:44:09Z

tests/e2e/tests/mixer/mixer_test.go

any chance to use a context or per thread/goroutine for that instead of atomic ?

like https://github.com/istio/istio/blob/master/devel/fortio/cmd/fortio/main.go#L48

(maybe I should add stuff to make it easier to reuse and aggregate without copy pasting)

I'm going to have a layer to make http+periodic be better integrated so you don't have to copy that code

the layer etc is done as part of #519

istio-testing · 2017-08-01T01:20:09Z

Jenkins job istio/presubmit passed

So #517 can be better/easier

ldemailly · 2017-08-01T02:23:28Z

tests/e2e/tests/mixer/mixer_test.go

ldemailly · 2017-08-01T02:25:24Z

tests/e2e/tests/mixer/mixer_test.go

let's switch this to #519 's :

res, err := HTTPRunner(&opts) // then use res.RetCodes[http.StatusOK] and res.DurationHistogram.Count

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

ldemailly

let's switch to fortio.HTTPRunner()

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!

yutongz · 2017-08-22T17:24:00Z

Please add release note by either adding message with pr template or comment "/release-note XXX" or "/release-note-none"

ldemailly · 2017-08-22T20:55:48Z

I updated the initial comment

douglas-reid · 2017-09-02T05:25:37Z

updated to HTTPRunner. PTAL.

ldemailly

I'm not sure I fully understand the (original) test but lgtm otherwise - seem simpler now

ldemailly · 2017-09-02T05:36:13Z

tests/e2e/tests/mixer/mixer_test.go

 }

+func promDump(client v1.API, metric string) string {
+	if value, err := client.Query(context.Background(), fmt.Sprintf("%s{}", metric), time.Now()); err == nil {


this is a bit mysterious to me - maybe a comment ?

ldemailly · 2017-09-02T05:40:53Z

tests/e2e/tests/mixer/mixer_test.go

@@ -323,53 +293,37 @@ func TestRateLimit(t *testing.T) {
 		}
 	}()



might be worth commenting what's in rateLimitRule / what we expect to happen here

ldemailly · 2017-09-02T05:41:20Z

tests/e2e/tests/mixer/mixer_test.go


-	// Allow full processing of metrics, etc.
-	time.Sleep(1 * time.Minute)
+	// consider only successful full requests


what is a "full request" ?

ldemailly · 2017-09-02T05:41:39Z

tests/e2e/tests/mixer/mixer_test.go

-	// Allow full processing of metrics, etc.
-	time.Sleep(1 * time.Minute)
+	// consider only successful full requests
+	perSvc := float64(succReqs) / 3


dividing by 3 because?

ldemailly · 2017-09-02T05:42:22Z

tests/e2e/tests/mixer/mixer_test.go

-	// allow some leeway for rejections
-	if got <= (want * .9) {
+	// establish some baseline (40% of expected counts)
+	want := math.Floor(perSvc * .4)


40% of 1/3rd because?

ldemailly · 2017-09-02T05:43:55Z

/lgtm
(though a couple more comments could help)

istio-merge-robot · 2017-09-02T05:45:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

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

Details

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

~~tests/OWNERS~~ [ldemailly]

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-09-02T08:00:59Z

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

istio-merge-robot · 2017-09-02T08:06:09Z

Automatic merge from submit-queue

istio-testing · 2017-09-02T08:52:54Z

@douglas-reid: The following tests failed, say /retest to rerun them all:

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

Details

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.

* Introduce attributes aspect kind to the mixer. This PR adds a new aspect kind (and associated manager and config) to the istio mixer. The new aspect kind is meant to be used exclusively in the preprocess phase of mixer request handling. It takes a set of inputs (attributes mapped via expression into "labels") and produces a set of generated attributes. To configure the new aspect kind, the operator will specify the labels mapping and attribute descriptors for the generated attributes. Any key-value pairs produced by implementing adapters that are not included in the list of generated attribute descriptors will be dropped on the floor at the moment. NOTE: the new aspect kind is named 'attributes' to match the existing style of the aspect kind names. That is, we don't call the access logging aspect kind 'access-logs-generator', nor do we call the metrics reporting aspect kind 'metrics-reporter'. To match, we call the attribute generation aspect kind simply 'attributes'. However, this could be slightly confusing in the implementing code (too many things called Attributes), so the manager and aspect interface are in terms of AttributesGenerator. Former-commit-id: 06f431a82805d5949c916be18acfb3d2df5ef961

@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

Automatic merge from submit-queue Switch to fortio for RateLimit testing Running with WRK seems to cause issues in local runs, related to amount of traffic generated. It also is yet another dep for the framework to establish. This PR removes these concerns (hopefully) and reduces wait times inside of `TestRateLimit`, also reducing the amount of time running the load test down from `2m` to `30s`. Fixes: #494 ```release-note Use fortio instead of wrk in e2e tests ``` Former-commit-id: 33cc5e5

* Introduce attributes aspect kind to the mixer. This PR adds a new aspect kind (and associated manager and config) to the istio mixer. The new aspect kind is meant to be used exclusively in the preprocess phase of mixer request handling. It takes a set of inputs (attributes mapped via expression into "labels") and produces a set of generated attributes. To configure the new aspect kind, the operator will specify the labels mapping and attribute descriptors for the generated attributes. Any key-value pairs produced by implementing adapters that are not included in the list of generated attribute descriptors will be dropped on the floor at the moment. NOTE: the new aspect kind is named 'attributes' to match the existing style of the aspect kind names. That is, we don't call the access logging aspect kind 'access-logs-generator', nor do we call the metrics reporting aspect kind 'metrics-reporter'. To match, we call the attribute generation aspect kind simply 'attributes'. However, this could be slightly confusing in the implementing code (too many things called Attributes), so the manager and aspect interface are in terms of AttributesGenerator. Former-commit-id: 3fe979ad9918014644ff082f2e9c6d880036ae69

@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

Automatic merge from submit-queue Switch to fortio for RateLimit testing Running with WRK seems to cause issues in local runs, related to amount of traffic generated. It also is yet another dep for the framework to establish. This PR removes these concerns (hopefully) and reduces wait times inside of `TestRateLimit`, also reducing the amount of time running the load test down from `2m` to `30s`. Fixes: istio#494 ```release-note Use fortio instead of wrk in e2e tests ``` Former-commit-id: 33cc5e5

@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

Automatic merge from submit-queue Switch to fortio for RateLimit testing Running with WRK seems to cause issues in local runs, related to amount of traffic generated. It also is yet another dep for the framework to establish. This PR removes these concerns (hopefully) and reduces wait times inside of `TestRateLimit`, also reducing the amount of time running the load test down from `2m` to `30s`. Fixes: #494 ```release-note Use fortio instead of wrk in e2e tests ``` Former-commit-id: 33cc5e5

* Add reminder message if user fail with values schema * Address comment

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

Co-authored-by: maistra-bot <null>

* bump jaeger version to latest Signed-off-by: Pierre Tessier <pierre@pierretessier.com> * bump jaeger-operator version to latest Signed-off-by: Pierre Tessier <pierre@pierretessier.com> --------- Signed-off-by: Pierre Tessier <pierre@pierretessier.com>

Update go to 1.22.4

douglas-reid requested a review from ldemailly July 31, 2017 23:04

googlebot added the cla: yes label Jul 31, 2017

ldemailly reviewed Jul 31, 2017

View reviewed changes

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

Make http runner accessible/reusable as library

8aa9d2c

So #517 can be better/easier

ldemailly mentioned this pull request Aug 1, 2017

grpc support in fortio (grpc load testing) #519

Merged

ldemailly reviewed Aug 1, 2017

View reviewed changes

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

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

ldemailly suggested changes Aug 1, 2017

View reviewed changes

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

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

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

istio-merge-robot added release-note and removed release-note-label-needed labels Aug 22, 2017

douglas-reid added 4 commits September 1, 2017 22:20

Switch to fortio for RateLimit testing

3def9ad

Update to latest fortio

7841675

simplify test condition

3142f09

Update to HTTPRunner

63f3281

prefer 8 to 10

2988c9e

ldemailly approved these changes Sep 2, 2017

View reviewed changes

istio-testing assigned ldemailly Sep 2, 2017

istio-testing added the lgtm label Sep 2, 2017

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

istio-merge-robot merged commit 33cc5e5 into istio:master Sep 2, 2017

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

Only using tag when explicity asked (#517)

cd38a85

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

make referenced attributes optional (istio#517)

a29d69c

howardjohn pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020

Try add a hint to use force flag (istio#517)

99cdf5d

* Add reminder message if user fail with values schema * Address comment

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

Fix FIPS build timeout. (istio#517)

c1b35e7

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

luksa pushed a commit to luksa/istio that referenced this pull request Sep 20, 2022

Automator: Update proxy (istio#517)

095da62

Co-authored-by: maistra-bot <null>

vikaschoudhary16 pushed a commit to vikaschoudhary16/istio that referenced this pull request Aug 12, 2024

Merge pull request istio#517 from tetrateio/update-go-1.22.4

159cf5e

Update go to 1.22.4

		@@ -323,53 +293,37 @@ func TestRateLimit(t *testing.T) {
		}
		}()

Conversation

douglas-reid commented Jul 31, 2017 • edited by ldemailly 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldemailly left a comment

Choose a reason for hiding this comment

Uh oh!

yutongz commented Aug 22, 2017

Uh oh!

ldemailly commented Aug 22, 2017

Uh oh!

douglas-reid commented Sep 2, 2017

Uh oh!

ldemailly 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!

ldemailly commented Sep 2, 2017

Uh oh!

istio-merge-robot commented Sep 2, 2017

Uh oh!

istio-merge-robot commented Sep 2, 2017

Uh oh!

istio-merge-robot commented Sep 2, 2017

Uh oh!

istio-testing commented Sep 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

douglas-reid commented Jul 31, 2017 •

edited by ldemailly

Loading