-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Vegeta rates / targets to SLA in performance tests #14429
Conversation
Hi @xiangpingjiang. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
/test performance-tests |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14429 +/- ##
==========================================
- Coverage 86.05% 86.02% -0.04%
==========================================
Files 197 197
Lines 14937 14945 +8
==========================================
+ Hits 12854 12856 +2
- Misses 1774 1778 +4
- Partials 309 311 +2 ☔ View full report in Codecov by Sentry. |
SLAs seem not to be respected (previous SLAs unrelated to the PR as well, see bellow).
|
/ok-to-test |
/test performance-tests cc @ReToCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor things, other than that it looks good.
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: xiangpingjiang 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 |
@skonto done |
/test performance-tests |
Maybe we are a bit too restrictive at some tests:
|
Looking at the errors I see:
This one should fail because I suspect that 3 out 100 services were not ready, given that we manually add the data point. I think this one needs further debugging as it seems we got stuck here:
Here I suspect that instead of:
it may help to do:
rollout-probe-queue-direct
rollout-probe-activator-direct
We have more like the above: rollout-probe-activator-direct-lin
Wrt rate comparison and for the constant pacers we have:
I think here just rounding the observed rate is enough, eg. 1000.001162 -> 1000 |
Signed-off-by: pingjiang <[email protected]>
/test performance-tests |
It seems we are still getting the round errors. Re-running to make sure the run was the latest: |
/test performance-tests |
Yep, we still have issues:
@xiangpingjiang can you add a threshold in that test? |
hello @ReToCode |
Yeah, or maybe in %, like we accept a deviation of 0.1% or something like this. |
Signed-off-by: pingjiang <[email protected]>
/test performance-tests |
Signed-off-by: pingjiang <[email protected]>
/test performance-tests |
@ReToCode I still see SLA failures in the performance test - but fixing them seems out of scope for this PR. (unless the SLA is becoming computed incorrectly) Another thing that would be useful is if we fail the performance test to surface the SLA failures have happened |
The SLAs were not constantly stable from the beginning, but this is a separate topic that we need to look into. So let's get this in, as it's better than before.
Yeah, but probably after we make them stable, otherwise we only have red builds and/or partial test results in influxdb. /lgtm Thanks @xiangpingjiang for doing this! |
@ReToCode @dprotaso Rounding errors or inaccurate conditions should not be fixed in this PR? For example for
|
I partially agree. The SLAs were never really stable to begin with (not even before my refactoring PR). We should look into that topic separately, aside from this specific PR (or phrased differently, I would not revert it for that). I created this issue: #14793 to follow up on this. |
Fixes #14403
Proposed Changes
Release Note