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

Standard SecNetPerf Scenarios #4607

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Standard SecNetPerf Scenarios #4607

wants to merge 6 commits into from

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Oct 10, 2024

Description

Adds a new secnetperf cmd line arg -scenario: which standardizes a few common test cases. This then simplifies the secnetperf scripts for netperf to just use these scenarios.

Testing

CI/CD

Documentation

Updated help text

$allTests["hps-conns-100"] = "-exec:maxtput -rconn:1 -share:1 -conns:100 -run:12s -prate:1"
$allTests["rps-up-512-down-4000"] = "-exec:lowlat -rstream:1 -up:512 -down:4000 -run:20s -plat:1"
$allTests["max-rps-up-512-down-4000"] = "-exec:lowlat -conns:16cpu -streams:10 -rstream:1 -up:512 -down:4000 -run:20s -plat:1"
$allTests = @("upload", "download", "hps", "rps-single", "rps-multi", "latency")
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is effectively a breaking change, from an identifier tput-up to upload (and so on), and I might need to keep the old ID and map it to the scenario. @ProjectsByJackHe please let me know what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ProjectsByJackHe as we discussed in our meeting yesterday, we need to ensure we're Ok with this change, and then update the dashboard and other netperf files as necessary. Please take a look. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.87%. Comparing base (c8d66e8) to head (410e492).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4607      +/-   ##
==========================================
- Coverage   86.98%   85.87%   -1.12%     
==========================================
  Files          56       56              
  Lines       17354    17354              
==========================================
- Hits        15096    14903     -193     
- Misses       2258     2451     +193     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nibanks
Copy link
Member Author

nibanks commented Nov 6, 2024

@ProjectsByJackHe where are we on this? Is netperf ready to merge this?

@ProjectsByJackHe
Copy link
Contributor

@ProjectsByJackHe where are we on this? Is netperf ready to merge this?

Doing some local dry runs to ensure these changes do not break compatibility.

Tracking: microsoft/netperf#401


$hasFailures = $false
$json["run_args"] = $allTests
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, if netperf will always use the default options (stream count, connection count...) for each scenario, then this is fine. But otherwise, we might still need this.

@ProjectsByJackHe
Copy link
Contributor

@ProjectsByJackHe where are we on this? Is netperf ready to merge this?

Doing some local dry runs to ensure these changes do not break compatibility.

Tracking: microsoft/netperf#401

Almost ready for merge! Once microsoft/netperf#414 gets merged first, validation passes, then this PR is ready for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants