Add check for validating ports #205

rohit-mp · 2020-04-09T06:53:43Z

Added a simple check in onvm/go.sh to validate the given port mask.

Closes #139

Summary:

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

TODO before merging :

PR is ready for review

Test Plan:

Review:

@kevindweb

(optional) Subscribers: << @-mention people who probably care about these changes >>

This PR releases OpenNetVM v19.05

This PR adds the following bug fix to the master branch: [Bug Fix] Fix Typo in Console Stats Header (sdnfv#142)

[Bug Fix] Fix Stats Header in Release Notes (sdnfv#145)

This PR releases OpenNetVM v19.07

This PR adds the a few bugfixes and features, this is not a full release.

onvm · 2020-04-09T06:53:45Z

In response to PR creation

CI Message

Your results will arrive shortly

onvm

In response to PR creation

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Linter passed

rohit-mp · 2020-04-09T06:56:23Z

The script stops the user only when they specify a port value which is greater than the number of NICs bound. Additionally, gives a warning when the port value of 0 is used.
I tested this against 0, 1, 2 port values.

Does this look good? Let me know if any changes are required.

…elop

rohit-mp · 2020-04-09T07:00:35Z

I seem to have incorrectly based my branch off of master instead of develop. I'll create another PR if that's an issue.

kevindweb · 2020-04-09T07:33:36Z

Thanks for making this so quickly. ONVM_NIC_PCI is not always filled. In scripts/setup_environment.sh, we check if that variable is empty, and request user response, but we don't actually set the variable. There's also the case that I don't set the NIC in setup, but I do it manually while testing through DPDK (which I've done many times). In those cases as well, we can't ensure the user has them set just through that variable.

rohit-mp · 2020-04-09T07:45:56Z

Ah forgot about that. I could modify it to check through all network devices instead of just those in the variable but that doesn't seem elegant. Do you have anything in mind?

kevindweb · 2020-04-09T15:57:40Z

Here is a command I think would work

./dpdk-devbind.py --status-dev net | sed '/Network devices using kernel driver/q'

This will only output the interfaces bound to dpdk, and stops outputting when we get to the kernel drivers. We can just grep to find a specific device, or count how many devices are bound.

rohit-mp · 2020-04-09T16:24:55Z

That's neat. Modifying the same to just count the number of times 'drv' appears would give us our required output.

./dpdk-devbind.py --status-dev net | sed '/Network devices using kernel driver/q' | grep -c "drv"

This should now give the number of network devices using dpdk compatible driver and can be directly compared with the portmask given. Does that sound good?

rohit-mp · 2020-04-11T16:58:12Z

Sorry for the delay. I've updated the checking condition. I hope the messages displayed are fine.

kevindweb · 2020-04-11T17:15:44Z

Thanks for updating. dpdk_drivers is probably unnecessary now. Can you make sure your tabs/spaces are correct (lines 64 and 65 specifically)? Also, we are trying to use [[ over [ single brace conditions in the future.

rohit-mp · 2020-04-11T17:21:08Z

Thanks for pointing that out. I'll remove dpdk_drivers.
I'm not sure why the tabs were misaligned (looks proper in vim). Replaced tabs by spaces now.

Should I update [ to [[ in other parts of the script as well?

kevindweb · 2020-04-11T17:36:20Z

I would leave other parts of the script. It's not horrible, we just want to do it for the future updates, and to reduce the amount of testing you should just update your code if you can.

kevindweb · 2020-04-11T17:51:42Z

Looks good, I will have to update CI before we test this, because CI runs go.sh without the correct number of ports I think.

rohit-mp · 2020-04-11T18:04:33Z

Okay. Thanks for immediate responses!

kevindweb · 2020-04-14T18:41:57Z

Let's try this @onvm

onvm · 2020-04-14T18:41:59Z

Let's try this @onvm

CI Message

Your results will arrive shortly

rohit-mp · 2020-04-17T20:25:36Z

Does this look good?

onvm/onvm_nflib/onvm_common.h

rohit-mp · 2020-04-18T04:50:03Z

I've also updated the function name to make it more readable. I'd used onvm_macaddr_get before to make it similar to rte_eth_macaddr_get.

dennisafa · 2020-04-18T17:51:11Z

onvm/onvm_nflib/onvm_common.h

+                rte_eth_macaddr_get(port_id, mac_addr);
+                return 0;
+        } else {
+                onvm_get_fake_macaddr(mac_addr);


I think that we can remove this part of the function. My idea is that onvm_get_macaddr is just checking for a valid port and fetching the macaddr for that port on success, return -1 otherwise. I don't think we should call onvm_get_fake_macaddr here. We should modify the NF's to decide whether or not they need a fake mac addr on their own.

Oh okay got it. So, speed_tester and scaling should use onvm_get_fake_macaddr? But they do take in a dst MAC in which case it would be good to try to get a real address for the source but can settle for a fake address. Or do we go along the lines that if a fake address is sufficient, then we needn't bother to fetch the real address?

I think the qn is whether returning the fake MAC address is an extra burden to be vary of. Otherwise, the return value denotes the status and the user can decide what to do with the address they received. Let me know what you guys think. I hope I'm not missing something.

kevindweb

Dennis' comment and mine too, after that I think it looks good otherwise.

examples/speed_tester/speed_tester.c

kevindweb · 2020-04-19T17:31:38Z

Hate to be that guy again but can you remove the speed_tester port extern line? I'll test and approve once you do.

Co-Authored-By: Kevin Deems <kevin8deems@gmail.com>

rohit-mp · 2020-04-19T17:46:22Z

Nothing to say for missing that a third time 😅

dennisafa

looking good, just a few small fixes and should be good to merge

examples/scaling_example/scaling.c

onvm/onvm_nflib/onvm_common.h

dennisafa

Looks good to me, thanks Rohit.

rohit-mp · 2020-04-20T15:55:59Z

Thanks for the careful reviews!

kevindweb

Just tested with multiple speed_tester, load_generator, and scaling on shared core mode as well as without. The port check on go.sh works well, tested on two different machines with and without NICs bound. Thanks for getting these changes updated so quickly, great work!

kevindweb · 2020-04-20T16:00:35Z

@onvm sanity check?

onvm · 2020-04-20T16:00:36Z

@onvm sanity check?

CI Message

Your results will arrive shortly

onvm

@onvm sanity check?

CI Message

Run successful see results:
Test took: 5 minutes, 42 seconds
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7729371
Performance rating - 100.38% (compared to 7700000 average)
Median TX pps for Speed Tester: 42215643
Performance rating - 100.51% (compared to 42000000 average)

pcodes

Also tested the new changes, looks good!

Vivek-anand-jain · 2020-06-18T01:56:24Z

onvm/go.sh

@@ -52,6 +52,13 @@ then
    usage
 fi

+ports_detected=$("$RTE_SDK"/usertools/dpdk-devbind.py --status-dev net | sed '/Network devices using kernel driver/q' | grep -c "drv")
+if [[ $ports_detected -lt $ports ]]


@rohit-mp @kevindweb I think this might be a bug since $ports represents the port mask while $ports_detected is the number of network devices using DPDK-compatible driver. Please correct me if I am wrong.

Yes, you are right. Sorry for the mistake on my part. This bug is being tracked in issue #228 at the moment.

koolzz and others added 6 commits June 4, 2019 14:45

Merge pull request sdnfv#134 from sdnfv/develop

46bbc96

This PR releases OpenNetVM v19.05

Merge pull request sdnfv#144 from sdnfv/develop

ea0bf0c

This PR adds the following bug fix to the master branch: [Bug Fix] Fix Typo in Console Stats Header (sdnfv#142)

Merge pull request sdnfv#146 from sdnfv/develop

c12d26d

[Bug Fix] Fix Stats Header in Release Notes (sdnfv#145)

Merge pull request sdnfv#156 from sdnfv/develop

30cf49a

This PR releases OpenNetVM v19.07

Merge pull request sdnfv#177 from sdnfv/develop

440ac6b

This PR adds the a few bugfixes and features, this is not a full release.

Add check for invalid port

67040d9

onvm previously approved these changes Apr 9, 2020

View reviewed changes

rohit-mp added 2 commits April 8, 2020 23:58

Merge branch 'develop' of https://github.com/sdnfv/openNetVM into dev…

3516e33

…elop

Merge branch 'develop' into 139_validate_ports

8976a43

kevindweb linked an issue Apr 9, 2020 that may be closed by this pull request

Invalid Port Prints when running with no ports enabled #139

Closed

dennisafa added the bug fix 🐛 label Apr 9, 2020

dennisafa mentioned this pull request Apr 10, 2020

Release 20.05 #206

Closed

3 tasks

Update bound ports detection

972beb9

rohit-mp dismissed onvm’s stale review via 972beb9 April 11, 2020 16:57

Format code

9f03e13

rohit-mp added 2 commits April 17, 2020 13:22

Update onvm_macaddr_get macro to function

400c149

Update onvm_macaddr_get usage

1c0a480

dennisafa requested changes Apr 17, 2020

View reviewed changes

onvm/onvm_nflib/onvm_common.h Outdated Show resolved Hide resolved

onvm/onvm_nflib/onvm_common.h Outdated Show resolved Hide resolved

rohit-mp added 2 commits April 17, 2020 21:47

Add onvm_get_fake_macaddr

279a9bd

Update onvm_get_macaddr usage

73975b1

dennisafa requested changes Apr 18, 2020

View reviewed changes

kevindweb reviewed Apr 18, 2020

View reviewed changes

examples/speed_tester/speed_tester.c Outdated Show resolved Hide resolved

Seperate onvm_get_macaddr and onvm_get_fake_macaddr

b903994

Update examples/speed_tester/speed_tester.c

f3ed7f5

Co-Authored-By: Kevin Deems <kevin8deems@gmail.com>

dennisafa requested changes Apr 19, 2020

View reviewed changes

examples/scaling_example/scaling.c Outdated Show resolved Hide resolved

onvm/onvm_nflib/onvm_common.h Outdated Show resolved Hide resolved

rohit-mp added 2 commits April 19, 2020 12:25

Update style

fa3f784

Try to get real MAC address before using a fake

f3c5b39

dennisafa reviewed Apr 19, 2020

View reviewed changes

onvm/onvm_nflib/onvm_common.h Outdated Show resolved Hide resolved

dennisafa reviewed Apr 19, 2020

View reviewed changes

onvm/onvm_nflib/onvm_common.h Outdated Show resolved Hide resolved

Remove redundant else statement

5b55418

dennisafa approved these changes Apr 20, 2020

View reviewed changes

kevindweb approved these changes Apr 20, 2020

View reviewed changes

onvm approved these changes Apr 20, 2020

View reviewed changes

pcodes approved these changes Apr 22, 2020

View reviewed changes

kevindweb added the ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge label Apr 22, 2020

dennisafa merged commit 3858c27 into sdnfv:develop Apr 23, 2020

Vivek-anand-jain reviewed Jun 18, 2020

View reviewed changes

rohit-mp deleted the 139_validate_ports branch August 18, 2020 05:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for validating ports #205

Add check for validating ports #205

rohit-mp commented Apr 9, 2020

onvm commented Apr 9, 2020

onvm left a comment

rohit-mp commented Apr 9, 2020

rohit-mp commented Apr 9, 2020

kevindweb commented Apr 9, 2020

rohit-mp commented Apr 9, 2020 •

edited

Loading

kevindweb commented Apr 9, 2020

rohit-mp commented Apr 9, 2020

rohit-mp commented Apr 11, 2020

kevindweb commented Apr 11, 2020

rohit-mp commented Apr 11, 2020

kevindweb commented Apr 11, 2020

kevindweb commented Apr 11, 2020

rohit-mp commented Apr 11, 2020

kevindweb commented Apr 14, 2020

onvm commented Apr 14, 2020

rohit-mp commented Apr 17, 2020

rohit-mp commented Apr 18, 2020

dennisafa Apr 18, 2020

rohit-mp Apr 18, 2020

rohit-mp Apr 18, 2020 •

edited

Loading

kevindweb left a comment

kevindweb commented Apr 19, 2020

rohit-mp commented Apr 19, 2020

dennisafa left a comment

dennisafa left a comment

rohit-mp commented Apr 20, 2020

kevindweb left a comment

kevindweb commented Apr 20, 2020

onvm commented Apr 20, 2020

onvm left a comment

pcodes left a comment

Vivek-anand-jain Jun 18, 2020

rohit-mp Jun 18, 2020

Add check for validating ports #205

Add check for validating ports #205

Conversation

rohit-mp commented Apr 9, 2020

Summary:

Merging notes:

Test Plan:

Review:

onvm commented Apr 9, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

rohit-mp commented Apr 9, 2020

rohit-mp commented Apr 9, 2020

kevindweb commented Apr 9, 2020

rohit-mp commented Apr 9, 2020 • edited Loading

kevindweb commented Apr 9, 2020

rohit-mp commented Apr 9, 2020

rohit-mp commented Apr 11, 2020

kevindweb commented Apr 11, 2020

rohit-mp commented Apr 11, 2020

kevindweb commented Apr 11, 2020

kevindweb commented Apr 11, 2020

rohit-mp commented Apr 11, 2020

kevindweb commented Apr 14, 2020

onvm commented Apr 14, 2020

CI Message

rohit-mp commented Apr 17, 2020

rohit-mp commented Apr 18, 2020

dennisafa Apr 18, 2020

Choose a reason for hiding this comment

rohit-mp Apr 18, 2020

Choose a reason for hiding this comment

rohit-mp Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

kevindweb left a comment

Choose a reason for hiding this comment

kevindweb commented Apr 19, 2020

rohit-mp commented Apr 19, 2020

dennisafa left a comment

Choose a reason for hiding this comment

dennisafa left a comment

Choose a reason for hiding this comment

rohit-mp commented Apr 20, 2020

kevindweb left a comment

Choose a reason for hiding this comment

kevindweb commented Apr 20, 2020

onvm commented Apr 20, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

pcodes left a comment

Choose a reason for hiding this comment

Vivek-anand-jain Jun 18, 2020

Choose a reason for hiding this comment

rohit-mp Jun 18, 2020

Choose a reason for hiding this comment

rohit-mp commented Apr 9, 2020 •

edited

Loading

rohit-mp Apr 18, 2020 •

edited

Loading