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

Pktgen for CI #148

Merged
merged 52 commits into from
Aug 6, 2019
Merged

Pktgen for CI #148

merged 52 commits into from
Aug 6, 2019

Conversation

kevindweb
Copy link
Contributor

Finally have Pktgen running for our Continuous Integration in the nimbus cluster!

Summary:

Long awaited, and tested recently in another PR, the default run mode is now to run Pktgen for our base performance testing. Right now here's the basic steps, ci sends our worker node to reboot, then fires the worker script. This script, depending on the worker-config, runs a certain mode (right now just pktgen or speed_test). If pktgen mode (MODE="0"), then we run a script that calls the Pktgen openNetVM-Scripts/run-pktgen.sh script in the other node through paramiko's SSHCLient. That uses our new lua script that sends packets for 30 seconds. We retrieve the data from basic monitor, send it back to CI for analysis. The new worker script also allows for multiple run modes, (MODE="0 1" for example). This way, we can potentially run pktgen then speed_tester and get all the results back to back after reboot.

Usage:

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes
New functionality 👍
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

  • Dependencies: None
    Please merge Ci busy no more #143 because I merged that code into this one, and all those commits are cluttering up this commit log as you see below (27 commits). There should only be like 3 commits for this PR so far.

TODO before merging :

  • PR is ready for review

Test Plan:

We have to figure out how we want to do different modes, from Github comment parsing or something. This way, we can stress test that this works. We should also test that this works on nn30 with nn33 (pktgen) so we know it's scalable to new nodes we want to use.

Review:

@koolzz @dennisafa

@kevindweb kevindweb added the CI/Testing 🤖 Continuous Integration and Testing related label Jun 24, 2019
@kevindweb
Copy link
Contributor Author

@onvm you there now?

@onvm
Copy link

onvm commented Jun 24, 2019

@onvm you there now?

CI Message

Your results will arrive shortly

@kevindweb
Copy link
Contributor Author

@koolzz I added information about benchmarks to the README and changed the name of the helper script symlink

@koolzz
Copy link
Member

koolzz commented Aug 4, 2019

@onvm
Olá

@onvm
Copy link

onvm commented Aug 4, 2019

@onvm
Olá

CI Message

Your results will arrive shortly

Copy link
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

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

Just some small things, mostly everything lgtm

ci/manager.sh Outdated
# put all files in one temporary folder for one scp
cp -r ./$worker_ip temp
cp -r ./repository temp/
cp -r ./worker_files/* temp/
Copy link
Member

Choose a reason for hiding this comment

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

Can we just copy all of these at once, will look cleaner cp -r ./$worker_ip ./repository temp/ ./worker_files/* temp/ k


# run pktgen
log "Collecting Pktgen Statistics"
run_pktgen
Copy link
Member

Choose a reason for hiding this comment

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

run_pktgen doesn't tell us that it also fetches stats, also the function serves little purpose. Lets just extract the function here

client.set_missing_host_key_policy(AutoAddPolicy())
client.connect(worker_ip, timeout = 30, username=worker_user, pkey = key)

(stdin, stdout, stderr) = client.exec_command("sudo ~/repository/tools/Pktgen/openNetVM-Scripts/run-pktgen.sh 1", get_pty=True)
Copy link
Member

Choose a reason for hiding this comment

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

Drop a comment saying get_pty is required so pktgen runs (I remember we had issues with this)

@@ -0,0 +1,8 @@
WORKER_MODE="0"
PKT_IFACE="p2p1"
Copy link
Member

Choose a reason for hiding this comment

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

Wait why do you have a different iface for pktgen and for mtcp? I'm pretty sure they can just share one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a hard time configuring the two interfaces separately. When one runs, it's ok, but configuring two interfaces is much easier than checking to see if the server was lying, before each check.

Copy link
Member

Choose a reason for hiding this comment

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

For now maybe we let it slide, but I'm not convinced that this is better. What's the topology you're using(which node is connected where), 2 NICs instead of one doesn't make much sense as it's using more resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tricky one, on the manager side (nimbnode17 for example) we only need 1 NIC, true, but on the client, we don't have to do anything if we can send Pktgen as one IP, and mTCP as another. This allows us to basically not touch the client server, because it's already configured. Your choice, but this makes it easy to we can run just mTCP, or just Pktgen, without any issues. This makes it faster, but I can definitely work a solution if necessary.

sudo rm -rf repository

git clone https://github.com/sdnfv/openNetVM.git repository
check_exit_code "ERROR: Failed installing onvm"
Copy link
Member

Choose a reason for hiding this comment

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

Failed cloning


cd repository

print_header "Beginning Execution of Workload"
Copy link
Member

Choose a reason for hiding this comment

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

rm

local idx = 0;
while( idx < waitTime ) do

-- Write port stats to output file separated by line
Copy link
Member

Choose a reason for hiding this comment

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

add 2 spaces so comment lines up and remove the newline above

onvm
onvm previously approved these changes Aug 4, 2019
Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

@onvm
Olá

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode17]

  • Median TX pps for Pktgen: 10205705
    Performance rating - 102.06% (compared to 10000000 average)

  • Median TX pps for Speed Tester: 40185939
    Performance rating - 100.46% (compared to 40000000 average)

@kevindweb
Copy link
Contributor Author

@onvm did the changes mess things up?

@onvm
Copy link

onvm commented Aug 6, 2019

@onvm did the changes mess things up?

CI Message

Your results will arrive shortly

@onvm
Copy link

onvm commented Aug 6, 2019

@onvm did the changes mess things up?

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

@onvm
Copy link

onvm commented Aug 6, 2019

Testing

CI Message

Your results will arrive shortly

@kevindweb
Copy link
Contributor Author

@onvm that was my fault

@onvm
Copy link

onvm commented Aug 6, 2019

@onvm that was my fault

CI Message

Your results will arrive shortly

@onvm
Copy link

onvm commented Aug 6, 2019

@onvm that was my fault

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

@onvm
Copy link

onvm commented Aug 6, 2019

Let us see

CI Message

Your results will arrive shortly

onvm
onvm previously approved these changes Aug 6, 2019
Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

Let us see

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode17]

  • Median TX pps for Pktgen: 10448860
    Performance rating - 104.49% (compared to 10000000 average)

  • Median TX pps for Speed Tester: 40189750
    Performance rating - 100.47% (compared to 40000000 average)

@kevindweb
Copy link
Contributor Author

@koolzz I will figure out the mTCP side of it in that PR, but as of now, there is only one interface being used, identified by CLIENT_IFACE in the worker-config

@koolzz
Copy link
Member

koolzz commented Aug 6, 2019

@onvm we good?

@onvm
Copy link

onvm commented Aug 6, 2019

@onvm we good?

CI Message

Your results will arrive shortly

Copy link

@onvm onvm left a comment

Choose a reason for hiding this comment

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

@onvm we good?

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode17]

  • Median TX pps for Pktgen: 10432160
    Performance rating - 104.32% (compared to 10000000 average)

  • Median TX pps for Speed Tester: 40192276
    Performance rating - 100.48% (compared to 40000000 average)

Copy link
Member

@koolzz koolzz left a comment

Choose a reason for hiding this comment

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

Great work man, happy to see Pktgen as a reliable CI check

@koolzz koolzz merged commit ca145c6 into sdnfv:develop Aug 6, 2019
@kevindweb kevindweb deleted the ci-pktgen branch March 21, 2020 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/Testing 🤖 Continuous Integration and Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants