Skip to content

Launch script#114

Merged
koolzz merged 4 commits intosdnfv:developfrom
kevindweb:launchScript
May 23, 2019
Merged

Launch script#114
koolzz merged 4 commits intosdnfv:developfrom
kevindweb:launchScript

Conversation

@kevindweb
Copy link
Copy Markdown
Contributor

@kevindweb kevindweb commented May 12, 2019

From Issue 107, go script couldn't handle multi-word string arguments

Summary:

Specifically in the payload_scan NF, running with the required ./go.sh 1 -d 1 -s "My String Here" would not work with multiple words. While ./go.sh 1 -d 1 -s computer or "computer" works well, bash strips the quotes with variable assignment, so "computer here" would become computer here. Thus the c getopt would not recognize the word here.

Usage:

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

Run multiple NFs with tons of arguments to ensure the go script doesn't fail. Make sure to run with DPDK arguments as well, to check that errors are handled correctly still. Only adding DPDK without ONVM args should fail, from previous edits to go scripts.

  • Sanity checks, assigned to @koolzz @dennisafa

    • Run make in /onvm and /examples directories
    • Code style, assigned to @koolzz @dennisafa
  • Run linter

  • Check for scripting errors

    • Check that code is reusable
    • Code is clean, functions are concise
    • Verify that edge cases properly handled
  • Performance, assigned to @koolzz @dennisafa

    • Run Speed Tester NF, report performance.

@onvm
Copy link
Copy Markdown

onvm commented May 12, 2019

In response to PR creation

CI Message

Your results will arrive shortly

Copy link
Copy Markdown

@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
✔️ Speed tester performance check passed

[Results from nimbnode30]

  • Median TX pps for Speed Tester: 35192022
  • Performance rating - 100.55% (compred to 35000000 average)

Linter Failed

examples/arp_response/arp_response.c:284: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
examples/flow_table/openflow.h:50: Using deprecated casting style. Use static_cast(...) instead [readability/casting] [4]
examples/flow_table/openflow.h:569: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:634: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:771: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:804: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:865: Extra space before ( in function call [whitespace/parens] [4]
examples/flow_table/openflow.h:926: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 7
examples/nf_router/nf_router.c:173: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 1
examples/payload_scan/payload_scan.c:95: Lines should be <= 120 characters long [whitespace/line_length] [5]
examples/payload_scan/payload_scan.c:114: Almost always, snprintf is better than strcpy [runtime/printf] [4]
examples/payload_scan/payload_scan.c:126: Lines should be <= 120 characters long [whitespace/line_length] [5]
Total errors found: 3
onvm/onvm_mgr/onvm_pkt.c:68: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1
onvm/onvm_mgr/onvm_stats.c:306: If an else has a brace on one side, it should have it on both [readability/braces] [5]
Total errors found: 1
onvm/onvm_nflib/onvm_pkt_common.c:98: Are you taking an address of a cast? This is dangerous: could be a temp var. Take the address before doing the cast, rather than after [runtime/casting] [4]
Total errors found: 1

@koolzz
Copy link
Copy Markdown
Member

koolzz commented May 12, 2019

@onvm do your thing 😉

@onvm
Copy link
Copy Markdown

onvm commented May 12, 2019

@onvm do your thing 😉

CI Message

Your results will arrive shortly

Copy link
Copy Markdown

@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 do your thing 😉

CI Message

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

[Results from nimbnode30]

  • Median TX pps for Speed Tester: 35209607
  • Performance rating - 100.60% (compred to 35000000 average)

Copy link
Copy Markdown
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.

Above there is a exec sudo $BINARY -F $config $CONFIG_ARGS for config file launch, lets also fix that. Otherwise looks good, tested with a few speed_tester & paylaod_scan cmds.

Comment thread examples/start_nf.sh Outdated
dash_dash_cnt=0
arg_cnt=0
for i in "$@" ; do
if [[ dash_dash_cnt -le 1 ]] ; then
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we make it -lt 2 (seems to me a bit more intuitive)

Comment thread examples/start_nf.sh Outdated

# Check if -- is present, if so parse dpdk/onvm specific args
dash_dash_cnt=0
arg_cnt=0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe something like non_nf_arg_cnt to be more specific?

Comment thread examples/start_nf.sh Outdated
DPDK_ARGS="$DPDK_BASE_ARGS $(echo " ""$@" | awk -F "--" '{print $1;}')"
ONVM_ARGS="$(echo " ""$@" | awk -F "--" '{print $2;}')"
NF_ARGS="$(echo " ""$@" | awk -F "--" '{print $3;}')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

extra space

@koolzz
Copy link
Copy Markdown
Member

koolzz commented May 23, 2019

This one is next @onvm

@onvm
Copy link
Copy Markdown

onvm commented May 23, 2019

This one is next @onvm

CI Message

Your results will arrive shortly

Copy link
Copy Markdown

@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.

This one is next @onvm

CI Message

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

[Results from nimbnode30]

  • Median TX pps for Speed Tester: 35205648
  • Performance rating - 100.59% (compared to 35000000 average)

@koolzz koolzz merged commit dc3f430 into sdnfv:develop May 23, 2019
@koolzz koolzz added this to the ONVM 19.05 Release milestone May 26, 2019
@kevindweb kevindweb deleted the launchScript branch June 4, 2019 23:46
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.

3 participants