search

sdnfv / openNetVM

A high performance container-based NFV platform from GW and UCR.
http://sdnfv.github.io/onvm/
Other
262 stars 136 forks source link

[Update] Add Messaging Unit Tests and Fix Issue #293 Message Memory Leak #296

Closed NoahChinitz closed 3 years ago

NoahChinitz commented 3 years ago

Summary:

This PR includes a new NF called Basic Message Test that tests the functionality of an NF sending messages to itself. This NF was created in response to Issue #293 where a memory leak was found. The memory leak occurred when a message was trying to be sent and the message queue had been filled; the message was never freed and put back into the shared memory pool. Three tests are ran: the first making sure that one message (with the correct data) can be sent to an NF, next it tests if 10 messages can be sent to an NF, and finally it tests to make sure that even if the ring buffer is overflowed that the appropriate messages are received by the NF. Moreover, there were changes made to onvm_sc_common.c in which if there was no packet sent to the function then we would return the first instance ID associated with the given service ID.

Without the fix, test_messaging.c would output:

TEST MESSAGING STARTED
---------------------------
APP: Received MSG from other NF
TEST 1: Send/Receive One Message...
---------------------------
PASSED TEST 1
---------------------------
TEST 2: Send/Receive Multiple Messages...
---------------------------
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
PASSED TEST 2
---------------------------
TEST 3: Message Ring Overflow...
---------------------------
(MORE "Received MSG" MESSAGES)
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
PASSED TEST 3
---------------------------
4 messages have not been deallocated back to the memory pool.
Passed 2/3 Tests
---------------------------

This makes sense since Test 4 is testing to make sure that all messages have been deallocated back to the memory pool. Since these overloaded messages were never put back into the memory pool correctly (which was the memory leak), we see that there is a memory leak.

With the fix, test_messaging.c will output:

TEST MESSAGING STARTED
---------------------------
APP: Received MSG from other NF
TEST 1: Send/Receive One Message...
---------------------------
PASSED TEST 1
---------------------------
TEST 2: Send/Receive Multiple Messages...
---------------------------
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
PASSED TEST 2
---------------------------
TEST 3: Message Ring Overflow...
---------------------------
APP: Destination NF ring is full! Unable to enqueue msg to ring
APP: Destination NF ring is full! Unable to enqueue msg to ring
APP: Destination NF ring is full! Unable to enqueue msg to ring
---------------------------
APP: Received MSG from other NF
---------------------------
(MORE "Received MSG" MESSAGES)
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
---------------------------
APP: Received MSG from other NF
PASSED TEST 3
---------------------------
Passed 3/3 Tests
---------------------------

Now that the fix has been implemented, all 4 tests pass and the memory leak is plugged.

Usage: Usage is described in README.md

This PR includes
Resolves issues Issue #293
Breaking API changes
Internal API changes
Usability improvements
Bug fixes X
New functionality X
New NF/onvm_mgr args X
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

TODO before merging :

Test Plan:

The NF was run extensively to ensure no segmentation faults occurred, as well as proper deallocation of memory was maintained.

Review:

onvm commented 3 years ago

In response to PR creation

CI Message

Aborting, need an authorized user to run CI

twood02 commented 3 years ago

Here are some basic tests we will want for the messaging system:

Right now your test also relies on a human to watch it and judge whether it works correctly or not -- let's try to automate that. Later we might integrate with a proper unit testing framework which can provide a standardized way to define tests and the expected output for them. For now you can do this manually. I'm imaging something that would display output like this when run:

Test Messaging started
Test 1: Send/Receive message...
PASSED
Test 2: Send/Receive multiple messages...
PASSED
Test 3: Message ring overflow...
FAILED

Passed 2/3 tests

Making the logic for this will be a bit awkward since the only place you can put code is in the nflib defined callbacks (setup, message, packet, etc). Probably the easiest way is to have all of the testing logic inside the message_handler function you define. The setup function might create the first message for Test 1 which would initiate the whole process.

NoahChinitz commented 3 years ago

After a code review I am going to make some changes to the way the tests are ran. I will request reviews when the final changes have been made.

catherinemeadows commented 3 years ago

Great work! A few recommendations:

twood02 commented 3 years ago

FYI other reviewers -- this includes the files needed to run ONVM in the VS Code debugger which will be merged with @elliotthenne 's PR #297 first.

@NoahChinitzGWU - can you add a comment for the library function you updated in its header file: https://github.com/sdnfv/openNetVM/blob/5379bc031dbd6f6487d5ebf88d176a45f0240ac7/onvm/onvm_nflib/onvm_sc_common.h#L55-L57 (and remove the old extraneous commented out code at the end of the line)

You should format the function comment based on those in nflib.h

It should explain the difference between passing a pkt and NULL as the second argument

catherinemeadows commented 3 years ago

@onvm check please

twood02 commented 3 years ago

@onvm run CI

twood02 commented 3 years ago

296 https://github.com/sdnfv/openNetVM/pull/296

twood02 commented 3 years ago

To finalize this can you add sample output from running your tests in these cases:

Put the output for these two cases in the body of your PR's descriptions to illustrate what the test NF does and how it can catch the bug from the original system

twood02 commented 3 years ago

We will want to make your socket information into a separate Pull request, so for now please rollback those commits. You can move them to a separate branch and do a different PR for that.

twood02 commented 3 years ago

great work @NoahChinitzGWU and @Lhahn01 on Friday revising this to follow a better testing structure. I think we still had a couple minor issues to resolve when we ended. Let me know when the PR is fully updated and I’ll do a final review

Be sure to add a comment to the test_handler function and also remove any TODO comments that we have completed (hopefully all of them now)