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

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

Merged
merged 60 commits into from
Aug 31, 2021
Merged

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

merged 60 commits into from
Aug 31, 2021

Conversation

NoahChinitz
Copy link
Contributor

@NoahChinitz NoahChinitz commented Jun 18, 2021

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:

  • Dependencies: None

TODO before merging :

  • [ X ] PR is ready for review

Test Plan:

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

Review:

@onvm
Copy link

onvm commented Jun 18, 2021

In response to PR creation

CI Message

Aborting, need an authorized user to run CI

examples/basic_msg_test/basic_msg_test.c Outdated Show resolved Hide resolved
examples/basic_msg_test/basic_msg_test.c Outdated Show resolved Hide resolved
@twood02 twood02 changed the title [Update] Fixed Issue #293 [Update] Fixed Issue #293 Message Memory Leak Jun 18, 2021
dennisafa
dennisafa previously approved these changes Jun 19, 2021
Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

I'll add another comment about making a full test suite

examples/basic_msg_test/basic_msg_test.c Outdated Show resolved Hide resolved
onvm/onvm_nflib/onvm_nflib.c Outdated Show resolved Hide resolved
onvm/onvm_nflib/onvm_nflib.c Outdated Show resolved Hide resolved
examples/basic_msg_test/basic_msg_test.c Outdated Show resolved Hide resolved
examples/basic_msg_test/basic_msg_test.c Outdated Show resolved Hide resolved
@twood02
Copy link
Member

twood02 commented Jun 21, 2021

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

  • 1. Send and receive a message, verifying that the correct data was sent
  • 2. Send multiple messages (but not enough to overflow the ring) and make sure the right number are received
  • 3. Send too many messages so that the ring gets full and some messages fail. Check the count of free objects in the message memory pool after all messages are received to verify that it goes back to 0 (the memory leak code should either crash or not go back to zero)

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 NoahChinitz requested review from twood02 and dennisafa June 29, 2021 16:36
@twood02
Copy link
Member

twood02 commented Aug 23, 2021

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)

@twood02 twood02 added this to the ONVM 21 Summer Release milestone Aug 23, 2021
Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

Overall looks good. A few final nits.

onvm/onvm_nflib/onvm_nflib.c Outdated Show resolved Hide resolved
examples/test_messaging/test_messaging.c Outdated Show resolved Hide resolved
examples/test_messaging/test_messaging.c Outdated Show resolved Hide resolved
examples/test_messaging/test_messaging.c Outdated Show resolved Hide resolved
examples/test_messaging/test_messaging.c Outdated Show resolved Hide resolved
examples/test_messaging/test_messaging.c Outdated Show resolved Hide resolved
examples/test_messaging/test_messaging.c Show resolved Hide resolved
examples/test_messaging/test_messaging.c Show resolved Hide resolved
examples/test_messaging/test_messaging.c Outdated Show resolved Hide resolved
NoahChinitz and others added 6 commits August 24, 2021 12:12
Co-authored-by: Tim Wood <timwood@gwu.edu>
Co-authored-by: Tim Wood <timwood@gwu.edu>
Co-authored-by: Tim Wood <timwood@gwu.edu>
Co-authored-by: Tim Wood <timwood@gwu.edu>
Co-authored-by: Tim Wood <timwood@gwu.edu>
Co-authored-by: Tim Wood <timwood@gwu.edu>
@twood02 twood02 added the CI/Testing 🤖 Continuous Integration and Testing related label Aug 24, 2021
twood02
twood02 previously approved these changes Aug 27, 2021
Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

I added a commit suggestion so the readme will describe the test cases. Otherwise this is ready to go.

examples/test_messaging/README.md Show resolved Hide resolved
Co-authored-by: Tim Wood <timwood@gwu.edu>
@twood02 twood02 changed the title [Update] Fixed Issue #293 Message Memory Leak [Update] Add Messaging Unit Tests and Fix Issue #293 Message Memory Leak Aug 30, 2021
Copy link
Member

@twood02 twood02 left a comment

Choose a reason for hiding this comment

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

Ready to go!

@twood02 twood02 added the ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge label Aug 30, 2021
@dennisafa dennisafa merged commit 07fc800 into sdnfv:develop Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/Testing 🤖 Continuous Integration and Testing related ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants