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

Fix json configuration and duplicate instance id bugs #272

Merged
merged 4 commits into from
Mar 7, 2021

Conversation

kevindweb
Copy link
Contributor

Users can now re-launch NFs with the same instance ID, and memory bugs were fixed allowing JSON configs to work again.

Summary:

This PR fixes 2 bugs that were referenced heavily in Slack and in a different issue (#233). I made a strlenn function which is an adaptation on string.h's strlen std library function. Normally, strlen does not include the null terminator when calculating the length of the string. This is a memory leak problem for our situation because we memcpy the string and need the '\0' to denote when to stop reading the config.

The second issue was the re-launch of instance IDs. This required a ring cleanup, but also on the stats side in onvm_stats.c, we needed to update nf_rx_last (a static function variable) after cleaning up the rx stats on the nfs table. This part is probably messy with the unlikely, but it's purpose is to avoid underflow with subtracting unsigned integers.

Usage:

This PR includes
Resolves issues #233
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:

If you notice with the current version of develop, running (with the bridge NF) ./go.sh -F ../example_config.json does not work. Also, if you try to run speed_tester for example twice with the same instance ID
(./go.sh -l 5 -- -m -r 3 -n 2 -- 3 -d 3), there will be an rte_exit from the manager because the rings weren't freed. Those are the two main tests to run and see that they are now fixed.

Review:

@twood02

(optional) Subscribers: << @-mention people who probably care about these changes >>
anyone else (maybe @catherinemeadows because you made the previous issue report)

@onvm
Copy link

onvm commented Dec 17, 2020

In response to PR creation

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.

In response to PR creation

CI Message

Run successful see results:
Test took: 14 minutes, 28 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed

[Results from nimbnode23]

  • Median TX pps for Pktgen: 7701733
    Performance rating - 100.02% (compared to 7700000 average)

  • Median TX pps for Speed Tester: 42013641
    Performance rating - 100.03% (compared to 42000000 average)

@dennisafa dennisafa merged commit 0cb551d into sdnfv:develop Mar 7, 2021
@twood02 twood02 added this to the ONVM 21 Summer Release milestone Aug 23, 2021
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.

4 participants