onvm
commented
4 years ago In response to PR creation
CI Message
Your results will arrive shortly
Closed EthanBaron14 closed 4 years ago
onvm
commented
4 years ago In response to PR creation
Your results will arrive shortly
EthanBaron14
commented
4 years ago Since this is a high priority bug fix and corrects a major issue with release 20.05, I'm attempting to merge this PR onto master, per @twood02's request. This would be why the first GitHub Action is failing. If I change the branch to develop, it would succeed. Should I PR to develop first to fix the workflow error, or just ignore it?
dennisafa
commented
4 years ago Since this is a high priority bug fix and corrects a major issue with release 20.05, I'm attempting to merge this PR onto master, per @twood02's request. This would be why the first GitHub Action is failing. If I change the branch to develop, it would succeed. Should I PR to develop first to fix the workflow error, or just ignore it?
you can ignore it
twood02
commented
4 years ago To follow our normal procedure let’s still merge this first into develop and then into master. I just meant we won’t wait for a full release.
EthanBaron14
commented
4 years ago @onvm thoughts?
onvm
commented
4 years ago @onvm thoughts?
Your results will arrive shortly
EthanBaron14
commented
4 years ago Improved the bc dependency check as suggested by @kevindweb. @sreya519 thanks so much for reviewing this already. Would you be able to review this again at your earliest convenience? This should be the final version now.
EthanBaron14
commented
4 years ago @onvm help a brother out
onvm
commented
4 years ago @onvm help a brother out
Your results will arrive shortly
EthanBaron14
commented
4 years ago @onvm test
onvm
commented
4 years ago @onvm test
Your results will arrive shortly
EthanBaron14
commented
4 years ago Looks like we should be all good to go. If I could get some fresh reviews from @sreya519 and @kevindweb, we should be good to merge as soon as they're done. @twood02 let me know if there's any other changes you think I should make.
EthanBaron14
commented
4 years ago @onvm last time
onvm
commented
4 years ago @onvm last time
Your results will arrive shortly
EthanBaron14
commented
4 years ago @onvm ok last last time
onvm
commented
4 years ago @onvm ok last last time
Your results will arrive shortly
twood02
commented
4 years ago Ready for @dennisafa to merge into develop and master!
Closes #228
Summary:
Updated the manager go.sh script to properly count the number of ports referenced in the port mask argument and compares it to the number of ports available for the manager to run on. If the number of ports specified in the hexadecimal port mask is more than the number available, the manager will not start. This is the anticipated behavior.
The bug was the script not converting the hexadecimal mask to binary and counting the digits, but instead taking the hexadecimal port mask and assuming it was the number of ports the manager should attempt to bind to. This has been fixed, since I have converted the hexadecimal port mask to binary and performed modulo operations to determine the number of digits that are "1", which signifies a port in use. This still allows the check to work, by alerting the user when the number of ports they specify in the port mask is higher than the number of ports available to DPDK.
Usage:
Run the manager the same as before:
where
<port mask>is the hexadecimal port mask for the ports you want the manager to run on.Merging notes:
Similar to PR #219, bc is a new dependency for openNetVM. It can be installed with
sudo apt-get install bc
TODO before merging :
Test Plan:
Tested running the manager on CloudLab, with 2 NICs bound and default arguments, but varied numbers for the hexadecimal port mask. Tried 0, 1, 2, 3, 4, and 7. This ensured that the port number comparison worked. I used 7 because it would have 3 "1's" in the binary number 111, which would equate to ports 0, 1 and 2. This, as it should, caused an error. With 4, though, we notice something interesting. Since 4 is technically 100, there is only one port that would be bound to. The port mask check does not catch this, but the manager does. The manager gives a warning that since port 2 is not configured, the manager will skip attempting to bind to it.
Review:
@sreya519 - you initially brought this up as a bug @kevindweb - you worked with the go.sh scripts frequently