Skip to content
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 rc 200 and 0 to touchstone config for router-perf and modifying default number of clients #518

Merged

Conversation

qiliRedHat
Copy link
Collaborator

@qiliRedHat qiliRedHat commented Nov 28, 2022

  1. The current touchstone config router-perf is using is https://github.com/cloud-bulldozer/e2e-benchmarking/blob/master/utils/touchstone-configs/mb-touchstone.json, it gets latency_95pctl. While avg_latency is also important, to get a feeling of the overall latency. In the real test, avg_latency is smaller than latency_95pctl. For example:
11-28 15:16:35.289      "avg_latency": 20011,
11-28 15:16:35.289      "latency_95pctl": 52734,
11-28 15:16:35.289      "latency_99pctl": 59614,
  1. Getting the number of response code is useful to evaluate if the test load is too big for a cluster, for example when the total connection number is bigger than the cluster's capacity, more '0' response code is seen. This is useful when testing with connection related features, like https://docs.google.com/spreadsheets/d/1AvpLioEZB-HigjaKMNRa_GYiffVBiyP8arqvlm0BAZg/edit#gid=313345561. I used to manually collect those data from output of the router-perf test.

This PR is to add a touchstone config file to generate results for the 200 and 0 rc codes.
I didn't include all kinds of rc code, because other rc codes are rarely seen, getting all of them can make the results sheet not easy to read.

@comet-perf-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@qiliRedHat
Copy link
Collaborator Author

qiliRedHat commented Dec 2, 2022

Test result in this result sheet https://docs.google.com/spreadsheets/d/118stkJ5uzEhYCo8k9F6J0CfkEmepoE1EvCzVxmlhuxM/edit#gid=831215729
@rsevilla87 Could you help to review this?

@rsevilla87
Copy link
Member

Hey @qiliRedHat , rather than adding another config file, why don't you add the new comparisons to the original?

@rsevilla87 rsevilla87 self-requested a review December 4, 2022 20:27
@qiliRedHat
Copy link
Collaborator Author

@rsevilla87 If adding to the origin don't impact others, I'm more than happy to do that.

@qiliRedHat qiliRedHat force-pushed the routerperf-touchstone-add-rc branch from 5fbbab1 to 893ee4c Compare December 6, 2022 09:06
@qiliRedHat
Copy link
Collaborator Author

@rsevilla87 PTAL again

@qiliRedHat
Copy link
Collaborator Author

@rsevilla87 Please help to review again.

@@ -24,13 +24,13 @@ get_scenario(){
if [[ ${NUM_NODES} -ge ${LARGE_SCALE_THRESHOLD} ]]; then
log "Large scale scenario detected: #workers >= ${LARGE_SCALE_THRESHOLD}"
export NUMBER_OF_ROUTES=${LARGE_SCALE_ROUTES:-500}
CLIENTS=${LARGE_SCALE_CLIENTS:-"1 20 80"}
CLIENTS_MIX=${LARGE_SCALE_CLIENTS_MIX:-"1 10 20"}
CLIENTS=${LARGE_SCALE_CLIENTS:-"1 80"}
Copy link
Member

@rsevilla87 rsevilla87 Jan 9, 2023

Choose a reason for hiding this comment

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

cc: @mohit-sheth @jtaleric @dry923, changing default number of clients, as per 4.12 maxconns has been upgraded from 20K to 50K. Some context -> #519 (comment)

Copy link
Member

@rsevilla87 rsevilla87 left a comment

Choose a reason for hiding this comment

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

lgtm!

@rsevilla87 rsevilla87 changed the title add rc 200 and 0 to touchstone config for router-perf add rc 200 and 0 to touchstone config for router-perf and modifying default number of clients Jan 9, 2023
Copy link
Member

@jtaleric jtaleric left a comment

Choose a reason for hiding this comment

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

lgtm

@mohit-sheth mohit-sheth merged commit 3839ba2 into cloud-bulldozer:master Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants