Skip to content

Performance test image refactor#1979

Merged
knative-prow-robot merged 20 commits into
knative:masterfrom
slinkydeveloper:performance-image-refactor
Oct 15, 2019
Merged

Performance test image refactor#1979
knative-prow-robot merged 20 commits into
knative:masterfrom
slinkydeveloper:performance-image-refactor

Conversation

@slinkydeveloper
Copy link
Copy Markdown
Contributor

Proposed Changes

  • Decoupling between sender and receiver
    • Splitted into two separate roles
    • Now between paces a gc event is sent, to ask to the receiver to perform a gc cycle
    • At the end of the test an end event is sent
    • Now an image instance can fulfil multiple roles (sender, receiver, aggregator)
  • Pluggable load generator (for testing different sources components like Test KafkaSource thpt and latency eventing-contrib#607)
  • Removed --verbose flag (not really useful since we always need a basic logging)
  • Renamed --role to --roles
  • Now you need to expect one record for sender and one for receiver, e.g. in a single sender/single receiver configuration the value of the aggregator flag --expect-record must be 2

/area performance
/assign grantr
/assign antoineco

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 30, 2019
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 30, 2019
@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Sep 30, 2019
@chaodaiG
Copy link
Copy Markdown
Contributor

/unassign

@chaodaiG
Copy link
Copy Markdown
Contributor

/uncc

@knative-prow-robot knative-prow-robot removed the request for review from chaodaiG September 30, 2019 18:53
@slinkydeveloper slinkydeveloper force-pushed the performance-image-refactor branch from f21b841 to 51ae437 Compare October 1, 2019 06:58
@slinkydeveloper slinkydeveloper force-pushed the performance-image-refactor branch from 51ae437 to feeb14f Compare October 1, 2019 07:01
@matzew
Copy link
Copy Markdown
Member

matzew commented Oct 1, 2019

/test pull-knative-eventing-unit-tests

@slinkydeveloper slinkydeveloper force-pushed the performance-image-refactor branch 2 times, most recently from 5ab5185 to c0b7d07 Compare October 9, 2019 07:13
@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

slinkydeveloper commented Oct 9, 2019

@grantr couple of questions after merge with master changes:

These flags: https://github.com/knative/eventing/pull/1979/files#diff-8658fa4c6c07318887e574ee745825d3R126 must be optional or mandatory?

And what their values should be in configs needed to run test manually? (like here https://github.com/knative/eventing/pull/1979/files?file-filters%5B%5D=.yaml#diff-f26b9b92ae11ebd49be7b5be576eda8d)

@grantr
Copy link
Copy Markdown
Contributor

grantr commented Oct 9, 2019

These flags: https://github.com/knative/eventing/pull/1979/files#diff-8658fa4c6c07318887e574ee745825d3R126 must be optional or mandatory?

I think we can make these flags optional with a default value of TODO. The stub sidecar ignores them, and the mako sidecar will fail to send data to mako if they are not set.

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper force-pushed the performance-image-refactor branch from 031a125 to e69dfc0 Compare October 10, 2019 08:49
Comment thread test/common/performance/receiver/id_extractor.go
Comment thread test/common/performance/receiver/type_extractor.go
Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
@antoineco
Copy link
Copy Markdown
Contributor

Good stuff, sorry for taking so long to review!
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2019
@grantr
Copy link
Copy Markdown
Contributor

grantr commented Oct 10, 2019

Ran this locally and got a grpc error:

broker-perf-aggregator aggregator 2019/10/09 22:58:01 Failed to store data: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (12908044 vs. 4194304)

So I think we need to increase the max rpc size allowed by the aggregator. The mako sidecar sets it to maxint.

@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

@grantr what was your pace configuration?

@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

slinkydeveloper commented Oct 10, 2019

Am I wrong or the actual maxRcvMsgSize is 100Mb?

s := grpc.NewServer(grpc.MaxRecvMsgSize(maxRcvMsgSize))
In your opinion is not big enough?

@grantr
Copy link
Copy Markdown
Contributor

grantr commented Oct 10, 2019

broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:53:51 Starting pace 2° at 600 rps for 30s seconds
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:54:25 Triggering GC
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:54:26 Starting pace 3° at 700 rps for 30s seconds
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:55:04 Triggering GC
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:55:08 Starting pace 4° at 800 rps for 30s seconds
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:55:41 Triggering GC
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:55:42 Starting pace 5° at 900 rps for 30s seconds
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:57:04 Triggering GC
broker-perf-send-receive-67qdz sender-receiver 2019/10/09 22:57:06 Starting pace 6° at 1000 rps for 30s seconds

It looks like grpc thinks the max receive size is 4MB. Maybe there's something wrong with my local setup. I'll try again.

@grantr
Copy link
Copy Markdown
Contributor

grantr commented Oct 10, 2019

On the second try I don't get that error, but the aggregator hangs:

broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:08 ---- END WARMUP ----
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:08 --- BEGIN BENCHMARK ---
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:08 Starting events processor
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:08 Starting benchmark
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:08 Starting pace 1° at 500 rps for 30s seconds
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:39 Triggering GC
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:30:40 Starting pace 2° at 600 rps for 30s seconds
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:31:21 Triggering GC
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:31:22 Starting pace 3° at 700 rps for 30s seconds
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:31:53 Triggering GC
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:31:54 Starting pace 4° at 800 rps for 30s seconds
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:32:47 Triggering GC
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:32:48 Starting pace 5° at 900 rps for 30s seconds
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:33:42 Triggering GC
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:33:43 Starting pace 6° at 1000 rps for 30s seconds
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:50 Triggering GC
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 Benchmark completed in 4m44.731775742s
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 All requests sent
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 All channels closed
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 ---- END BENCHMARK ----
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 Sending collected data to the aggregator
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 Sent count     : 123233
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 Accepted count : 81428
broker-perf-send-receive-99f6c sender-receiver 2019/10/10 17:34:53 Failed count   : 41805
broker-perf-aggregator aggregator 2019/10/10 17:34:55 -> Recording 123233 SENT events
broker-perf-aggregator aggregator 2019/10/10 17:34:55 -> Recording 81428 ACCEPTED events
broker-perf-aggregator aggregator 2019/10/10 17:34:56 -> Recording 41805 FAILED events

Looks like it's waiting for a report from the receiver, but it's never sent.

@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

And can you check if you expect 2 records more than one?

@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

slinkydeveloper commented Oct 10, 2019

Ok I get the problem, the end message is dropped at some point. So perhaps or I set a timeout when I don't receive any message after N seconds, or I send end events at low pace for N seconds... Or maybe both, just in case 😄 Wdyt?

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2019
@slinkydeveloper
Copy link
Copy Markdown
Contributor Author

slinkydeveloper commented Oct 11, 2019

I've added a timeout timer calculated from pace configuration, I did a couple of test runs and looks good

@matzew
Copy link
Copy Markdown
Member

matzew commented Oct 15, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2019
Copy link
Copy Markdown
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

One minor comment comment, but that can be fixed in a followup. Thanks @slinkydeveloper!

@@ -39,9 +39,8 @@ const (

// Those two depends on the maximum tolerated latency. If latency is higher than 1 sec, increase these.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this comment didn't get moved with the waitFor* constants.

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, slinkydeveloper

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019
@grantr
Copy link
Copy Markdown
Contributor

grantr commented Oct 15, 2019

/retest

@knative-prow-robot knative-prow-robot merged commit 03b42e7 into knative:master Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/performance area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants