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

Warn user when not specifying -m flag in start_nf.sh #158

Merged
merged 5 commits into from
Aug 25, 2019

Conversation

dennisafa
Copy link
Member

Warn the user if -m isn't specified in start_nf.sh

Summary:

You can bind a core manually using -l and -m in the start_nf script. If -m isn't specified, no binding will occur. This change will let the user know that they should supply this flag if they want the binding to occur.
Usage:
Run any NF using the ./start_nf.sh NF-NAME DPDK_ARGS -- ONVM_ARGS -- NF_ARGS format
(i.e, ./start_nf.sh speed_tester -l 5 -- -m -r 3 -- -d 2 where is -l is the core # to assign, and -m is the binding flag)

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements X
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:

Run the start_nf script without and with the -m flag.

Review:

@koolzz @kevindweb

@onvm
Copy link

onvm commented Aug 15, 2019

In response to PR creation

CI Message

Your results will arrive shortly

@dennisafa dennisafa requested review from koolzz and kevindweb August 15, 2019 19:23
onvm
onvm previously approved these changes Aug 15, 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.

In response to PR creation

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: 10488087
    Performance rating - 104.88% (compared to 10000000 average)

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

Copy link
Contributor

@kevindweb kevindweb left a comment

Choose a reason for hiding this comment

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

I think it looks good, really just a bash nit, and Nick can dispute, but personally I like combining those conditions, as well as using built-in regex.
[[ $DPDK_ARGS =~ "-l" && ! $ONVM_ARGS =~ "-m" ]]

@koolzz
Copy link
Member

koolzz commented Aug 18, 2019

Lgtm, testing is on you though. I like @kevindweb nit looks elegant

@dennisafa
Copy link
Member Author

I think it looks good, really just a bash nit, and Nick can dispute, but personally I like combining those conditions, as well as using built-in regex.
[[ $DPDK_ARGS =~ "-l" && ! $ONVM_ARGS =~ "-m" ]]

looks clean!

@kevindweb
Copy link
Contributor

@onvm I will test, but please work correctly

@onvm
Copy link

onvm commented Aug 22, 2019

@onvm I will test, but please work correctly

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 I will test, but please work correctly

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: 10324681
    Performance rating - 103.25% (compared to 10000000 average)

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

Copy link
Contributor

@kevindweb kevindweb left a comment

Choose a reason for hiding this comment

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

Tested with CI and cloudlab, all performance looks good, as well as Pktgen, and multiple speed testers running.

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.

Seen the warning so approving

@koolzz koolzz merged commit 79af9c7 into sdnfv:develop Aug 25, 2019
@twood02 twood02 added this to the ONVM 20.05 milestone Apr 9, 2020
@twood02 twood02 added the NeedsReleaseNote Needs updated release note info label May 22, 2020
@dennisafa dennisafa deleted the start_nf_core branch May 23, 2020 18:44
@twood02 twood02 removed the NeedsReleaseNote Needs updated release note info label May 26, 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.

5 participants