Skip to content

[Bug Fix] NF Advanced Ring Thread Process NF Shutdown Messages#111

Merged
nks5295 merged 9 commits intosdnfv:developfrom
dennisafa:adv_rings_bug
May 16, 2019
Merged

[Bug Fix] NF Advanced Ring Thread Process NF Shutdown Messages#111
nks5295 merged 9 commits intosdnfv:developfrom
dennisafa:adv_rings_bug

Conversation

@dennisafa
Copy link
Copy Markdown
Member

@dennisafa dennisafa commented Apr 30, 2019

When running the advanced rings mode in speed tester and scaling, closing the manager would not kill the NF running. (referencing #105 )

Summary:

This adds a message handler in the advanced rings loop that listens for the MSG_STOP macro sent by the manager.

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:

Run the manager, then run speed tester (or scaling) with advanced ring mode: ./go.sh 1 -d 1 -a
Close the manager, assure that the NF is stopping.

Review:

@koolzz @kevindweb I was thinking for sanity checks maybe try running several of the same NF and speed tester + scaling then closing manager.

  • Sanity checks, assigned to @koolzz @kevindweb

    • Run make in /onvm and /examples directories
    • Test new functionality or bug fix from the pr (if needed)
  • Code style, assigned to @koolzz @kevindweb

    • Run linter
    • Check everything style related
  • Code design, assigned to @koolzz @kevindweb

    • Check for memory leaks
    • Check that code is reusable
    • Code is clean, functions are concise
    • Verify that edge cases properly handled
  • Documentation, assigned to @koolzz @kevindweb

    • Check if the new changes are well documented, in both code and READMEs

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

@onvm
Copy link
Copy Markdown

onvm commented Apr 30, 2019

In response to PR creation

CI Message

Your results will arrive shortly

@onvm
Copy link
Copy Markdown

onvm commented Apr 30, 2019

In response to PR creation

CI Message

Run successful see results:
[Results from nimbnode30]
Median TX pps for Speed Tester: 35184761

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

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.

Good work, added a few code comments. Once you update I'll review again

Comment thread examples/scaling_example/scaling.c Outdated
Comment thread examples/scaling_example/scaling.c
Comment thread examples/scaling_example/scaling.c
Comment thread examples/scaling_example/scaling.c
Comment thread examples/scaling_example/scaling.c Outdated
Comment thread examples/speed_tester/speed_tester.c
Comment thread examples/scaling_example/scaling.c Outdated
Comment thread examples/scaling_example/scaling.c Outdated
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.

@dennisafa overall good changes, it works and I approve, although please fix the 2 style changes I commented on (in both files) and I'll merge.

Comment thread examples/scaling_example/scaling.c Outdated
Comment thread examples/scaling_example/scaling.c Outdated
@koolzz
Copy link
Copy Markdown
Member

koolzz commented May 12, 2019

@onvm I need a pr with linter conflicts

@onvm
Copy link
Copy Markdown

onvm commented May 12, 2019

@onvm I need a pr with linter conflicts

CI Message

Your results will arrive shortly

@onvm
Copy link
Copy Markdown

onvm commented May 12, 2019

@onvm I need a pr with linter conflicts

CI Message

Error: ERROR: Script failed on nimbnode30

@koolzz
Copy link
Copy Markdown
Member

koolzz commented May 12, 2019

@onvm should be good to go

@onvm
Copy link
Copy Markdown

onvm commented May 12, 2019

@onvm should be good to go

CI Message

Your results will arrive shortly

@koolzz
Copy link
Copy Markdown
Member

koolzz commented May 12, 2019

@onvm jk, but now for sure

@onvm
Copy link
Copy Markdown

onvm commented May 12, 2019

@onvm jk, but now for sure

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 jk, but now for sure

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Speed tester performance check passed
❌ Linter Failed (please fix style errors)

[Results from nimbnode30]

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

Linter Output

examples/scaling_example/scaling.c:303: An else should appear on the same line as the preceding } [whitespace/newline] [4]
examples/scaling_example/scaling.c:303: If an else has a brace on one side, it should have it on both [readability/braces] [5]
Total errors found: 2
examples/speed_tester/speed_tester.c:347: An else should appear on the same line as the preceding } [whitespace/newline] [4]
examples/speed_tester/speed_tester.c:347: If an else has a brace on one side, it should have it on both [readability/braces] [5]
Total errors found: 2

@koolzz
Copy link
Copy Markdown
Member

koolzz commented May 13, 2019

@onvm is the linter pleased

@onvm
Copy link
Copy Markdown

onvm commented May 13, 2019

@onvm is the linter pleased

CI Message

Your results will arrive shortly

@dennisafa
Copy link
Copy Markdown
Member Author

@onvm how's your weekend been?

@onvm
Copy link
Copy Markdown

onvm commented May 13, 2019

@onvm how's your weekend been?

CI Message

Another CI run in progress, please try again in 15 minutes

Comment thread examples/speed_tester/speed_tester.c
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 is the linter pleased

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: 35250185
  • Performance rating - 100.71% (compred to 35000000 average)

@nks5295 nks5295 changed the title Advanced rings bug fix [Bug Fix] NF Advanced Ring Thread Process NF Shutdown Messages May 16, 2019
@nks5295 nks5295 merged commit 20941c6 into sdnfv:develop May 16, 2019
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