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

MAC address fix #223

Merged
merged 5 commits into from
Jun 23, 2020
Merged

MAC address fix #223

merged 5 commits into from
Jun 23, 2020

Conversation

catherinemeadows
Copy link
Contributor

@catherinemeadows catherinemeadows commented Jun 9, 2020

Fixes the load_generator bug mentioned in #222

Summary:

In order to resolve the issue where the "Failed to obtain MAC address" error is raised in the load_generator NF, the function onvm_get_fake_macaddr() is used to obtain a fake MAC address for testing or in the case that the provided MAC address of the port is not bound to DPDK.

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:

Ran the load_generator NF with the MAC address of the node I am testing with. I had @sreya519 run this fix for the load_generator as well since she experienced this error.

Review:

@sreya519 --> tested the load_generator fixes already

@onvm
Copy link

onvm commented Jun 9, 2020

In response to PR creation

CI Message

Your results will arrive shortly

@catherinemeadows catherinemeadows changed the base branch from master to develop June 9, 2020 18:18
@onvm
Copy link

onvm commented Jun 9, 2020

In response to PR creation

CI Message

Error: ERROR: Failed to post results to GitHub

@EthanBaron14
Copy link
Contributor

@onvm try again?

@onvm
Copy link

onvm commented Jun 9, 2020

@onvm try again?

CI Message

Your results will arrive shortly

@onvm
Copy link

onvm commented Jun 9, 2020

@onvm try again?

CI Message

Error: ERROR: Failed to post results to GitHub

EthanBaron14
EthanBaron14 previously approved these changes Jun 9, 2020
Copy link
Contributor

@EthanBaron14 EthanBaron14 left a comment

Choose a reason for hiding this comment

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

This worked for me. Made sure to checkout on the PR branch, ran the manager with the default syntax and ran the load generator NF with ./go.sh 1 -d 2 -m <Nimbus node MAC address> and had no issues. Did not see the "Failed to obtain MAC address" error.

@catherinemeadows
Copy link
Contributor Author

@onvm is this working now?

@onvm
Copy link

onvm commented Jun 9, 2020

@onvm is this working now?

CI Message

Your results will arrive shortly

onvm
onvm previously approved these changes Jun 9, 2020
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 is this working now?

CI Message

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

[Results from nimbnode23]

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

  • Median TX pps for Speed Tester: 42227047
    Performance rating - 100.54% (compared to 42000000 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.

Good fix, remove it from lb and I'll approve

examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
examples/load_balancer/load_balancer.c Outdated Show resolved Hide resolved
@catherinemeadows catherinemeadows dismissed stale reviews from onvm and EthanBaron14 via 63191da June 10, 2020 13:36
koolzz
koolzz previously approved these changes Jun 10, 2020
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.

lgtm

Copy link
Member

@dennisafa dennisafa left a comment

Choose a reason for hiding this comment

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

Looks good - lets add a RTE_LOG(INFO, APP, "") (dpdk's print statement) somewhere to let the user know we're using a fake mac addr (as discussed on slack)

@catherinemeadows
Copy link
Contributor Author

speed_tester has the same setup. Should we add the same print to it as well? @dennisafa

EthanBaron14
EthanBaron14 previously approved these changes Jun 12, 2020
Copy link
Contributor

@EthanBaron14 EthanBaron14 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested again using same configuration as above. No problems.

@kevindweb
Copy link
Contributor

kevindweb commented Jun 12, 2020

@catherinemeadows I think it would be useful to have a print statement like this one in speed_tester

@dennisafa
Copy link
Member

Agreed - sorry I missed this comment. A comment wherever we fake it is a good idea.

@dennisafa dennisafa merged commit cc2c0bf into sdnfv:develop Jun 23, 2020
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.

7 participants