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

Manager go.sh port mask incorrectly counting ports available #228

Closed
EthanBaron14 opened this issue Jun 11, 2020 · 3 comments
Closed

Manager go.sh port mask incorrectly counting ports available #228

EthanBaron14 opened this issue Jun 11, 2020 · 3 comments

Comments

@EthanBaron14
Copy link
Contributor

Bug Report

Current Behavior
When running go.sh for the manager, the check on line 57 compares the raw hexadecimal port mask passed in to the number of NICs available to run the manager on.

Expected behavior/code
When running go.sh for the manager, the check on line 57 should convert the hexadecimal port mask passed in to binary then compare the number of '1' digits to the number of NICs available to run the manager on.

Steps to reproduce
Run go.sh with similar syntax to the following:

./go.sh 0,1,2 3 0xF8 -s stdout

While ensuring that the third positional argument represents more than one port, e.g. 3.

Environment

  • OS: Ubuntu 18.04
  • onvm version: 20.05

Possible Solution

Will convert the hexadecimal port mask to binary then count the number of '1' digits to determine the number of ports that we compare to.

@EthanBaron14
Copy link
Contributor Author

This is a high priority issue as it dramatically impact the performance of the manager in release 20.05. Accordingly, I'll try to get this done within the next few hours so it can be tested and merged into 20.05 before EoW.

@EthanBaron14 EthanBaron14 self-assigned this Jun 11, 2020
EthanBaron14 added a commit to EthanBaron14/openNetVM that referenced this issue Jun 11, 2020
@BuzzRage
Copy link

Hi, I think I had the same problem.
This line ( onvm/go.sh line 56 ) probably don't have the expected behavior:

ports_detected=$("$RTE_SDK"/usertools/dpdk-devbind.py --status-dev net | sed '/Network devices using kernel driver/q' | grep -c "drv")

when you remove the sed command, everything works fine.

dennisafa pushed a commit that referenced this issue Jun 18, 2020
Fixes an issue where port mask was being calculated 
incorrectly.

Commit log:

* Fixed bug in #228

* Changed dependency checking to be more OS-agnostic

* Forcing workflow rerun

* Forcing workflow rerun

* Improved dependency check

* Simplified port counting

* Added comments for port conversions

* Moved port check to prevent future merge conflict
dennisafa added a commit that referenced this issue Jun 18, 2020
This PR updates our install guide to include Python,
needed for many of our scripts.

Commit log:

* scripts/*.py files updated to use python3

* hashbang reflects use of python3 in .py files

* Python dependency noted in Installation guide

* Install script edits

* syntax errors fixed

* scripts/*.py files updated to use python3

* hashbang reflects use of python3 in .py files

* Python dependency noted in Installation guide

* Install script edits

* syntax errors fixed

* fixed syntax errors

* spacing errors fixed

* addressing pylint errors

* switched exit(1) to sys.exit(1) given pylint suggestion

* cleaned up print statements

* [Bug Fix[ Fix for Manager go.sh Incorrectly Comparing Ports Available

Fixes an issue where port mask was being calculated 
incorrectly.

Commit log:

* Fixed bug in #228

* Changed dependency checking to be more OS-agnostic

* Forcing workflow rerun

* Forcing workflow rerun

* Improved dependency check

* Simplified port counting

* Added comments for port conversions

* Moved port check to prevent future merge conflict
@EthanBaron14
Copy link
Contributor Author

This is fixed in PR #229.

dennisafa pushed a commit that referenced this issue Jul 22, 2020
This PR improves command line argument functionality for our
manager go script. 

Commit log:

* Added improvements in #190

* User can now use 1-8 cores with input checking. Updated usage instructions.

* Changed message for using default cores from WARNING to INFO

* Updated documentation for new manager syntax

* Updated dependency information in docs

* Flag syntax implemented. Checks for dependency bc. Syntax checking

* Updated documentation for new syntax

* Ensures flags can be entered out of order in new syntax

* Fixed bug in #228

* Changed dependency checking to be more OS-agnostic

* Forcing workflow rerun

* Forcing workflow rerun

* Improved dependency check and stored regex match result in variables

* Just learned Bash does not assume false if variable does not exist

* Improved dependency check

* Simplified port counting

* Added comments for port conversions

* Moved port check to prevent future merge conflict

* Added usage on error and fixed merge conflicts

* Added usage on error and fixed merge conflicts

* Improved if statement logic, information printing

* Fixed syntax checking expression to match switching positions

* Fixed logic

* Last documentation updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants