Skip to content

[Bug Fix] Properly Terminate Started but not yet Running NFs#88

Merged
koolzz merged 2 commits intosdnfv:developfrom
koolzz:bugfix_free_core_on_termination
Mar 30, 2019
Merged

[Bug Fix] Properly Terminate Started but not yet Running NFs#88
koolzz merged 2 commits intosdnfv:developfrom
koolzz:bugfix_free_core_on_termination

Conversation

@koolzz
Copy link
Copy Markdown
Member

@koolzz koolzz commented Mar 21, 2019

This pr cleans up the NF termination cleanup and resolves a STARTED NF termination which has not entered the RUNNING state.

Summary:

  • Check which state a NF is in, determine cleanup based on that
  • Add comments for the people who are debugging this in the future

Usage:
Tested with running 1 NF core for the manager, f.e.:
./go.sh 0,1,2 0 0x60
Run the NF with bad args, causing it to quit before entering the RUNNING state:

cd basic_monitor
./go.sh 2 -d 1

The above should fail, now run basic monitor with
./go.sh 2
Before this pr it would fail, after this pr it should run fine.

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:

Try to break this as hard as you can by running different NFs, with valid/invalid args and stopping them whenever your heart desires.

Review:

Review checklist:

  • Sanity checks, assigned to @kevindweb @Arjun-Vijay

    • Run make in /onvm and /examples directories
    • Test the bug fix
  • Code style, assigned to @kevindweb @Arjun-Vijay

    • Run linter
    • Check everything style related
  • Code design, assigned to @kevindweb @Arjun-Vijay

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

    • Run Speed Tester NF, report performance.
  • Documentation, assigned to @kevindweb @Arjun-Vijay

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

@onvm
Copy link
Copy Markdown

onvm commented Mar 21, 2019

In response to PR creation

CI Message

Your results will arrive shortly

@onvm

This comment has been minimized.

@koolzz
Copy link
Copy Markdown
Member Author

koolzz commented Mar 21, 2019

PR review assigned to @kevindweb and @Arjun-Vijay

Copy link
Copy Markdown
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 basic_monitor. Previously, exiting nf that has not started running messes with the manager, so that it has to exit those non-running NFs.

APP: Core 0: Notifying NF 1 to shut down
APP: Core 0: Notifying NF 3 to shut down

This PR works in that these issues do not happen when exiting before nf->status is set to RUNNING.

@koolzz
Copy link
Copy Markdown
Member Author

koolzz commented Mar 23, 2019

@onvm how does it look now

@onvm
Copy link
Copy Markdown

onvm commented Mar 23, 2019

@onvm how does it look now

CI Message

Your results will arrive shortly

@onvm
Copy link
Copy Markdown

onvm commented Mar 23, 2019

@onvm how does it look now

CI Message

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

Linter Passed

@koolzz koolzz merged commit 5928fef into sdnfv:develop Mar 30, 2019
@koolzz koolzz added this to the ONVM 19.05 Release milestone May 26, 2019
@koolzz koolzz deleted the bugfix_free_core_on_termination branch June 7, 2019 23:17
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