Skip to content
This repository has been archived by the owner on May 2, 2023. It is now read-only.

Add performance test #156

Merged
merged 1 commit into from
Oct 7, 2022

Conversation

jwcesign
Copy link
Member

@jwcesign jwcesign commented Aug 3, 2022

Fix #151

@knative-prow
Copy link

knative-prow bot commented Aug 3, 2022

Hi @jwcesign. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@knative-prow knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 3, 2022
@jwcesign jwcesign changed the title Add performance test [WIP] Add performance test Aug 3, 2022
@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2022
@jwcesign jwcesign force-pushed the performance-test branch 2 times, most recently from 20722b8 to cca74f0 Compare August 3, 2022 15:48
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #156 (37a14f7) into main (8451375) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #156   +/-   ##
=======================================
  Coverage   67.35%   67.35%           
=======================================
  Files           5        5           
  Lines         193      193           
=======================================
  Hits          130      130           
  Misses         44       44           
  Partials       19       19           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jwcesign
Copy link
Member Author

jwcesign commented Aug 3, 2022

/test pull-knative-sandbox-container-freezer-build-tests

@knative-prow
Copy link

knative-prow bot commented Aug 3, 2022

@jwcesign: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-knative-sandbox-container-freezer-build-tests

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.

@jwcesign
Copy link
Member Author

jwcesign commented Aug 3, 2022

cc @psschwei give one /ok-to-test,I will do tests here

@jwcesign
Copy link
Member Author

jwcesign commented Aug 3, 2022

So for the useage like:

/test pull-knative-sandbox-container-freezer-performance-tests-hey

First, I should set here test-infra

Than add a shell file in this PR.
finally trigger it with /test pull-knative-sandbox-container-freezer-performance-tests-hey ?

@knative-prow
Copy link

knative-prow bot commented Aug 3, 2022

@jwcesign: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

So for the useage like:

/test pull-knative-sandbox-container-freezer-performance-tests-hey

First, I should set for config

Than I can add a test file and trigger it with /test pull-knative-sandbox-container-freezer-performance-tests-hey ?

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.

@psschwei
Copy link
Contributor

psschwei commented Aug 3, 2022

/ok-to-test

@knative-prow knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2022
@jwcesign
Copy link
Member Author

jwcesign commented Aug 4, 2022

/test pull-knative-sandbox-container-freezer-build-tests

@knative-prow
Copy link

knative-prow bot commented Aug 4, 2022

@jwcesign: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test build-tests_container-freezer_main
  • /test integration-tests_container-freezer_main
  • /test unit-tests_container-freezer_main

Use /test all to run all jobs.

In response to this:

/test pull-knative-sandbox-container-freezer-build-tests

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.

@jwcesign jwcesign force-pushed the performance-test branch 3 times, most recently from 8c13182 to 16409e9 Compare October 6, 2022 13:23
@jwcesign jwcesign changed the title [WIP] Add performance test Add performance test Oct 6, 2022
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2022
@jwcesign
Copy link
Member Author

jwcesign commented Oct 6, 2022

cc @nader-ziada @psschwei , when you are available, please check it. It works on my local cluster when comment initialize --min-nodes=2 --max-nodes=2 --cluster-version=1.23 "$@"

# Copyright 2022 The Knative Authors
#!/usr/bin/env bash

# Copyright 2018 The Knative Authors
#
Copy link
Member Author

Choose a reason for hiding this comment

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

copy from serving repo

Copy link
Contributor

@psschwei psschwei Oct 6, 2022

Choose a reason for hiding this comment

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

Let's keep the dates in both scripts as 2022 since that's when they were added to this repo

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,132 @@
#!/usr/bin/env bash
Copy link
Member Author

Choose a reason for hiding this comment

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

copy from serving repo

Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

Couple nits:

  • when I pulled the PR, none of the scripts were marked executable, I think they'll need a chmod +x in order to run via prow
  • test currently requires istio in order to generate results, as get_gateway_ip() checks the istio namespace
  • there's a lot of bits in the copied over tests scripts that we aren't using and could probably be cut

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 7, 2022
@jwcesign jwcesign force-pushed the performance-test branch 2 times, most recently from 5c997a9 to 413ce6b Compare October 7, 2022 04:27
@jwcesign
Copy link
Member Author

jwcesign commented Oct 7, 2022

test currently requires istio in order to generate results, as get_gateway_ip() checks the istio namespace.

I use setup_ingress_env_vars to set the gateway env then use it directly(all the logic is from serving repo)

Signed-off-by: jiang wei <jwcesign@gmail.com>
Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

We might want to add a README for the performance test with a note on how to use an alternative ingress, but can do that in a separate PR.

Thanks for adding this!

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2022
@knative-prow
Copy link

knative-prow bot commented Oct 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwcesign, psschwei

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

The pull request process is described here

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 knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2022
@knative-prow knative-prow bot merged commit 1980ff5 into knative-extensions:main Oct 7, 2022
@psschwei psschwei mentioned this pull request Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for Beta
2 participants