Add CronJobs for running broker-imc regularly by grantr · Pull Request #1969 · knative/eventing

grantr · 2019-09-26T23:17:29Z

Redo of #1863. This PR drops the single variant, leaving that config in the parent of continuous.

Run ko apply -f test/performance/broker-imc/continuous/ to create CronJobs that will run the broker-imc test every 15 minutes.

We expect CI to run one benchmark per cluster and have access to mako credentials, so the continuous config runs in default namespace and uses the real mako sidecar instead of the stub.

Fixes #1700.

Proposed Changes

Add continuous config for the broker-imc performance test.
The perf cluster creation tools now expect performance testing directories to have a continuous directory containing the yaml config to apply to the perf cluster.

/area performance
/project Perf/Measurement To do

ko apply -f continuous/ to create CronJobs that will run the broker-imc test every 15 minutes. The continuous config runs in default namespace because we expect prod to run one benchmark per cluster.

knative-prow-robot · 2019-09-26T23:17:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr

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:

~~test/OWNERS~~ [grantr]

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

grantr · 2019-09-26T23:22:05Z

Caveat: we can only use this to run the broker-imc perf test right now. mako.Setup only allows us to use one mako config in the performance image, and for now that's the "Broker Latency and Throughput" config. We need to do some refactoring of either mako.Setup or the shared performance image to fix that.

The 3000rps pacer takes slightly longer than 9 minutes

chizhg · 2019-09-27T00:40:50Z

@@ -0,0 +1,182 @@
+# Copyright 2019 The Knative Authors


Is this setup file the same as the existing one under broker-imc?

same here, @grantr why did you duplicated configs?

chizhg · 2019-09-27T00:50:33Z

Caveat: we can only use this to run the broker-imc perf test right now. mako.Setup only allows us to use one mako config in the performance image, and for now that's the "Broker Latency and Throughput" config. We need to do some refactoring of either mako.Setup or the shared performance image to fix that.

Hmm, it seems I missed something, why are the mako config files under the shared test image, instead of the benchmark folder?

slinkydeveloper · 2019-09-27T07:44:51Z

Caveat: we can only use this to run the broker-imc perf test right now. mako.Setup only allows us to use one mako config in the performance image, and for now that's the "Broker Latency and Throughput" config. We need to do some refactoring of either mako.Setup or the shared performance image to fix that.

This could be solved by splitting the aggregator and sender-receiver in two different images?

matzew · 2019-09-27T07:55:55Z

+kind: Broker
+metadata:
+  name: imc
+  namespace: default


@chizhg @slinkydeveloper well, it DIFFs in name/namespace.

But I agree, it's good to potentially reuse "common" setup yaml files...

matzew · 2019-09-27T07:57:27Z

+    ref:
+      apiVersion: v1
+      kind: Service
+      name: broker-imc-receiver


same... slight diff: https://github.com/knative/eventing/blob/master/test/performance/broker-imc/100-broker-perf-setup.yaml#L82

matzew · 2019-09-27T07:58:17Z

 export TEST_ROOT_PATH="$GOPATH/src/knative.dev/eventing/test/performance"
 export KO_DOCKER_REPO="gcr.io/knative-eventing-performance"
+export TEST_CONFIG_VARIANT="continuous"
+export TEST_NAMESPACE="default"


how about perf-eventing ?

matzew · 2019-09-27T15:49:39Z

/hold

perhaps let's see to not duplicate configs?

grantr · 2019-09-27T17:53:47Z

Hmm, it seems I missed something, why are the mako config files under the shared test image, instead of the benchmark folder?

Currently there is no benchmark-specific main. All the benchmark execution code is in the shared test image. Due to the way mako.Setup works, the mako config files must be in the kodata for that image. This is a known issue that will require either mako.Setup to change or the eventing test image(s) to change (knative/pkg#721). @grac3gao is actively looking into this.

For now, this PR puts config files under the shared test image to allow Mako to run one test instead of zero tests.

grantr · 2019-09-27T19:20:49Z

perhaps let's see to not duplicate configs?

I'm fine with making continuous/broker-imc-setup.yaml a symlink to ./broker-imc-setup.yaml.

We should consider this: continuous/broker-imc.yaml is tightly coupled to continuous/broker-imc-setup.yaml, but ./broker-imc.yaml file has different content than continuous/broker-imc.yaml. If we have a shared setup file, and someone makes a change to that shared file and ./broker-imc.yaml, we need to make sure they also update continuous/broker-imc.yaml. So the developer must understand the coupling between the configurations and update the continuous config on every change to the shared setup config.

matzew · 2019-09-27T19:36:35Z

thanks for clarifying! feel free to cancel the hold /lgtm

On Fri 27. Sep 2019 at 21:20, Grant Rodgers ***@***.***> wrote: perhaps let's see to not duplicate configs? I'm fine with making continuous/broker-imc-setup.yaml a symlink to ./broker-imc-setup.yaml. We should consider this: continuous/broker-imc.yaml is tightly coupled to continuous/broker-imc-setup.yaml, *but ./broker-imc.yaml file has different content than continuous/broker-imc.yaml.* If we have a shared setup file, and someone makes a change to that shared file and ./broker-imc.yaml, we need to make sure they also update continuous/broker-imc.yaml. So the developer must understand the coupling between the configurations and update the continuous config on every change to the shared setup config. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1969?email_source=notifications&email_token=AABGPTU3WDPZXOVOVVEGPILQLZMJDA5CNFSM4I3ABFP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7Z3SMA#issuecomment-536066352>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABGPTSYPMQGCP6GPF5MQTLQLZMJDANCNFSM4I3ABFPQ> .

-- Sent from Gmail Mobile

grantr · 2019-09-27T21:18:46Z

Thanks @matzew! I expect all of this to be refactored several times before we're happy with it, but this PR at least makes some progress.

/hold cancel

grantr · 2019-09-27T21:24:00Z

Recent mako runs: https://mako.dev/benchmark?benchmark_key=5036156928393216&tseconds=604800

Add CronJobs for running broker-imc regularly

602a170

ko apply -f continuous/ to create CronJobs that will run the broker-imc test every 15 minutes. The continuous config runs in default namespace because we expect prod to run one benchmark per cluster.

grantr requested review from chizhg, slinkydeveloper and srinivashegde86 September 26, 2019 23:17

knative-prow-robot added area/performance size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2019

knative-prow-robot requested a review from yt3liu September 26, 2019 23:17

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 26, 2019

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Sep 26, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2019

mattmoor-sockpuppet reviewed Sep 26, 2019

View reviewed changes

grantr added 2 commits September 26, 2019 16:22

clean up line endings

7669b97

increase time allowed to finish

fc9edcf

The 3000rps pacer takes slightly longer than 9 minutes

grantr mentioned this pull request Sep 26, 2019

Run perf tests continuously for IMC channel #1970

Closed

chizhg reviewed Sep 27, 2019

View reviewed changes

matzew reviewed Sep 27, 2019

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2019

knative-prow-robot assigned matzew Sep 27, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2019

knative-prow-robot merged commit 95edd04 into knative:master Sep 27, 2019

grantr deleted the perf-config-variants-3 branch September 27, 2019 23:31

slinkydeveloper mentioned this pull request May 6, 2020

Added slinkydeveloper as approver #3091

Merged

Conversation

grantr commented Sep 26, 2019

Proposed Changes

Uh oh!

knative-prow-robot commented Sep 26, 2019

Uh oh!

grantr commented Sep 26, 2019

Uh oh!

chizhg Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

slinkydeveloper Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

chizhg commented Sep 27, 2019

Uh oh!

slinkydeveloper commented Sep 27, 2019

Uh oh!

matzew Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

matzew Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

matzew Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

matzew commented Sep 27, 2019

Uh oh!

grantr commented Sep 27, 2019

Uh oh!

grantr commented Sep 27, 2019

Uh oh!

matzew commented Sep 27, 2019 via email

Uh oh!

grantr commented Sep 27, 2019

Uh oh!

grantr commented Sep 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants