Default CPU Cores for Manager #219
Conversation
CI MessageAborting, need an authorized user to run CI |
|
@onvm test this. I know you didn't mean to reject me |
CI MessageAborting, need an authorized user to run CI |
|
@onvm test pls |
CI MessageAborting, need an authorized user to run CI |
|
@onvm i'm gonna cry please test this |
CI MessageYour results will arrive shortly |
CI MessageError: Failed to parse Speed Tester stats |
|
That probably makes sense considering CI most likely is running the old go.sh syntax for the manager. This would need to be updated to run CI to test this. @kevindweb not sure how you want to proceed with this; we'll need to update it eventually regardless since the manager syntax is changing, I suppose. |
|
The current script used in CI will not work as the current usage will not function with your changes. Could you list the new usage so we can test this? Such as how do we configure what cores the manager will run? If this is the case you will also have to update the readme as the current guideline to run the manager will not work. |
That's all correct, like I said. Here's some of the notes I have, based on the new syntax:
I think varying the number of arguments might cause inconsistency, but would love to get more opinions on this. Also difficult to know because bash requires us to know the number of arguments to shift the arguments to parse the flags inputted. Any input would be appreciated. |
|
I think I'm going to implement a version of Ben's solution with the goal of having something pushed by end-of-day. We can determine whether the first argument is the set of cores to use and shift the arguments accordingly. Might keep the |
|
Going to go a little further with this. The old syntax will still be allowed, e.g. ./go.sh -k <port mask> -n <NF cores> [OPTIONS]Note that the -m flag for setting the manager cores will remain optional and we will assume default cores 0,1,2 unless the flag is specified. The Users will have the option to either continue to use the old syntax (though this won't be supported or reflected anywhere in the docs except maybe a note for the manager), or adopt the new syntax where they are required to use The motivation for changing my approach to do it this way is so we can guarantee a level of backwards compatibility, but encourage proper practices going forward. Depending on whether users transition from the old to new syntax will dictate whether we phase out the old syntax at some point. |
|
Implemented flag syntax as mentioned above. Current syntax should now be referred to as legacy. New syntax for running manager go.sh script with cores 0,1,2, ports 0 and 1, and NFs on cores 3,4,5,6,7 should be: ./go.sh -k 3 -n 0xF8For specifying manager cores, simply add the ./go.sh -k 3 -n 0xF8 -m 0,1,2Manager allows any number of cores greater than or equal than three. The manager checks the syntax of flag inputs as well as the legacy syntax. As stated above, users will have the option to either continue to use the old syntax (though this won't be supported or reflected anywhere in the docs except maybe a note for the manager), or adopt the new syntax where they are required to use Documentation has been updated to reflect the new syntax for the manager shell script as well as the added dependency of bc. |
|
@onvm test this |
CI MessageYour results will arrive shortly |
CI MessageError: Failed to parse Speed Tester stats |
|
Was able to test successfully using legacy and new syntaxes. Tested with manager and speed test NF. Will continue to investigate CI failures with the goal of rectification within the next 24 hrs. |
|
|
Yup, that's working as intended. As we spoke about on Friday, bc is now a dependency. This is reflected in the documentation update commit on this branch as well as the error message you're seeing. If that's an issue on your machine, it could be an issue with CI. I'll check with Kevin in the morning. Thanks for bringing that up. Also, regarding that readlink error you're seeing, that's likely a Mac-specific issue. Since we recommend running ONVM on Linux, that's not a problem. More information: http://biercoff.com/readlink-f-unrecognized-option-problem-solution-on-mac/ |
Yeap sorry my bad I just wanted to show you the command CI uses. Also me just being dumb and not realizing my nimbnode lost connection.. |
CI MessageYour results will arrive shortly |
CI MessageYour results will arrive shortly |
|
Added some final changes that @kevindweb suggested. Ready for final review. |
CI MessageError: Failed to parse Speed Tester stats |
|
@onvm now |
CI MessageYour results will arrive shortly |
There was a problem hiding this comment.
@onvm now
CI Message
Run successful see results:
Test took: 3 minutes, 56 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
[Results from nimbnode23]
-
Median TX pps for Pktgen: 7739866
Performance rating - 100.52% (compared to 7700000 average) -
Median TX pps for Speed Tester: 42254584
Performance rating - 100.61% (compared to 42000000 average)
|
@onvm last time |
CI MessageYour results will arrive shortly |
There was a problem hiding this comment.
@onvm last time
CI Message
Run successful see results:
Test took: 3 minutes, 55 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
[Results from nimbnode23]
-
Median TX pps for Pktgen: 7741782
Performance rating - 100.54% (compared to 7700000 average) -
Median TX pps for Speed Tester: 42105410
Performance rating - 100.25% (compared to 42000000 average)
There was a problem hiding this comment.
This all looks pretty good now, I think it's ready to merge if @kevindweb also approves this
There was a problem hiding this comment.
@EthanBaron14 this looks solid to me, tons of necessary updates here!
EthanBaron14
dismissed stale reviews from kevindweb, WilliamMaa, and onvm
via
13e871b
|
@onvm verify I didn't break anything by updating readme's |
CI MessageYour results will arrive shortly |
There was a problem hiding this comment.
@onvm verify I didn't break anything by updating readme's
CI Message
Run successful see results:
Test took: 4 minutes, 4 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
[Results from nimbnode23]
-
Median TX pps for Pktgen: 7737192
Performance rating - 100.48% (compared to 7700000 average) -
Median TX pps for Speed Tester: 42242943
Performance rating - 100.58% (compared to 42000000 average)
|
@kevindweb @WilliamMaa sorry I forgot to update one of the README.md files; can you guys review again? |
New syntax for running the manager script.
Summary:
Implemented the improvements outlined in Issue #190.
When running the manager, users no longer have to input manager cores, a port mask, and a set of NF cores using positional arguments in the go.sh script. This functionality was retained, but from here on out we'll be suggesting that users use a new syntax with flags. In this PR, I added three flags:
-m,-k, and-n, which set the cores for the manager to run on, the hexadecimal port mask, and the cores for the NF to run on in a hexadecimal port mask, respectively. The new go.sh syntax is as follows:Users have the option to manually set at least three cores to use with the manager, using either
-mor the legacy positional option syntax. There is no upper bound on the number of cores that the manager can use, but we require at least three.Users will also be warned if their manager and NF cores overlap, regardless of the script running syntax they choose to use, because overlapping cores could cause some issues. Users will also see a message if they are using the default cores 0, 1 and 2 if they do not set the
-mflag.When a user opts to use the new syntax, they are required to specify
-kand-nflags to set the port and NF core masks. This is because there is no intuitive default for these values. If a user instead decides to use a combination of the legacy and new methods and includes one of the new flags, the manager will not run, because we would have conflicting information specified in multiple places.For any input arguments now, the script will do minor syntax checking with regular expressions. This will give more intuitive error messages if the user mistakenly enters bad input arguments.
Usage:
-m <cores, separated by commas>,-k <port mask>,-n <NF core mask>./go.sh -k <port mask> -n <NF core mask> [OPTIONS]Merging notes:
bc is a new dependency for openNetVM. It can be installed with
TODO before merging :
Test Plan:
Tested running the manager with default cores, manually set cores, and various numbers of cores. Tested both legacy and new manager running syntax. Verified that user cannot use a combination of the syntax types. Verified that manager does not recognize new syntax as an attempt at running legacy syntax with improper arguments. Verified that regex correctly parses the input. Tested to see that warnings and dialogues are printed in the appropriate situations. Verified that changes to usage make sense and are formatted correctly.
Review:
@kevindweb - you wrote the bash script, I believe.