Skip to content

[WIP] Perf config variants#1863

Closed
grantr wants to merge 4 commits into
knative:masterfrom
grantr:perf-config-variants
Closed

[WIP] Perf config variants#1863
grantr wants to merge 4 commits into
knative:masterfrom
grantr:perf-config-variants

Conversation

@grantr
Copy link
Copy Markdown
Contributor

@grantr grantr commented Sep 11, 2019 •
edited
Loading

We only have one config for running perf tests today. That's suboptimal because there are at least two variants in use:

  • Single runs that don't push to Mako
  • Scheduled, continuous runs that do push to Mako

This PR adds support for config variants.

Proposed Changes

  • Move the yaml configs for the broker-latency test to subdirectories continuous and single.
  • Continuous is for the case where the developer is not directly watching the results. It uses a CronJob to run the test continuously and reports results to Mako.
  • Single is for the case where the developer is directly watching the results. It uses a Job and the Mako stub sidecar to report results as CSV.
  • The scripts for managing CI clusters use the continuous variant.

/area performance
/project perf/measurement

@grantr
Move single-run broker-latency config to subdir
a9b4a4c 
This allows the configs for continuous runs and single runs to be
separate.

The continuous configs are the same as single right now, but later will
use CronJobs and the real mako sidecar.

Remove namespace so namespace can be set by kubectl.
@grantr
Use the continuous config variant in CI clusters
571e52d
@knative-prow-robot knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/performance labels Sep 11, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 11, 2019
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Sep 11, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Sep 11, 2019

[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:

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 Sep 11, 2019
srinivashegde86
srinivashegde86 reviewed Sep 11, 2019
View reviewed changes
Comment thread test/performance/tools/common.sh
function install_mako_resources() {
echo ">> Setting up config-mako ConfigMap"
cat <<EOF | kubectl apply -f -
cat <<EOF | kubectl apply -n ${TEST_NAMESPACE}-f -
Copy link
Copy Markdown
Contributor

@srinivashegde86 srinivashegde86 Sep 11, 2019

Choose a reason for hiding this comment

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

Suggested change
cat <<EOF | kubectl apply -n ${TEST_NAMESPACE}-f -
cat <<EOF | kubectl apply -n ${TEST_NAMESPACE} -f -

@srinivashegde86
Copy link
Copy Markdown
Contributor

srinivashegde86 commented Sep 11, 2019

overall LGTM. I have one comment to fix a typo.

/lgtm
/hold

@knative-prow-robot 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 11, 2019
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2019
@grantr
Merge branch 'master' into perf-config-variants
f0f26f8
@grantr
Add CronJobs for running broker-latency regularly
9478129 
ko apply -f continuous/ to create CronJobs that will run the
broker-latency test every half hour.

The continuous config runs in default namespace because we expect prod
to run one benchmark per cluster.
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Sep 12, 2019

New changes are detected. LGTM label has been removed.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2019
mattmoor-sockpuppet
mattmoor-sockpuppet reviewed Sep 12, 2019
View reviewed changes
Comment thread test/performance/broker-latency/continuous/broker-latency.yaml
name: config-mako
- name: service-account
secret:
secretName: service-account No newline at end of file
Copy link
Copy Markdown
Contributor

@mattmoor-sockpuppet mattmoor-sockpuppet Sep 12, 2019

Choose a reason for hiding this comment

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

Add trailing newline:

Suggested change
secretName: service-account
secretName: service-account

Comment thread test/test_images/latencymako/prod.config
# Human owners for manual benchmark adjustments.
owner_list: "grantrodgers@google.com"
owner_list: "chizhg@google.com"
owner_list: "srinivashegde@google.com"
Copy link
Copy Markdown
Contributor

@mattmoor-sockpuppet mattmoor-sockpuppet Sep 12, 2019

Choose a reason for hiding this comment

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

Remove trailing whitespace:

Suggested change
owner_list: "srinivashegde@google.com"
owner_list: "srinivashegde@google.com"

srinivashegde86
srinivashegde86 reviewed Sep 12, 2019
View reviewed changes
Comment thread test/performance/broker-latency/continuous/broker-latency.yaml
- name: config-mako
mountPath: /etc/config-mako
terminationMessagePolicy: FallbackToLogsOnError
- name: mako
Copy link
Copy Markdown
Contributor

@srinivashegde86 srinivashegde86 Sep 12, 2019

Choose a reason for hiding this comment

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

I am guessing only this job needs mako creds?

@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented Sep 17, 2019

Fixes #1700

@chizhg
Copy link
Copy Markdown
Contributor

chizhg commented Sep 19, 2019

@grantr @srinivashegde86 is this PR ready to be merged? It generally looks good to me.

@matzew
Copy link
Copy Markdown
Member

matzew commented Sep 23, 2019

@Fredy-Z @grantr let's get the comments addressed (including the suggestions from the sockpuppet)

Otherswise, /lgtm

@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented Sep 26, 2019

I'll create a new PR shortly that's up to date with the latest perf test image.

@grantr grantr closed this Sep 26, 2019
@grantr grantr mentioned this pull request Sep 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chizhg chizhg Awaiting requested review from chizhg

2 more reviewers

@srinivashegde86 srinivashegde86 srinivashegde86 left review comments

@mattmoor-sockpuppet mattmoor-sockpuppet mattmoor-sockpuppet left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@srinivashegde86 srinivashegde86

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@grantr @knative-prow-robot @srinivashegde86 @chizhg @matzew @mattmoor-sockpuppet @googlebot