Default CPU Cores for Manager

[EthanBaron14]

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:

./go.sh -k <port mask> -n <NF core mask> [OPTIONS]

Users have the option to manually set at least three cores to use with the manager, using either -m or 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 -m flag.

When a user opts to use the new syntax, they are required to specify -k and -n flags 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:

• Adjusting required parameters for manager to enable default manager cores 0,1,2
• New flags: -m <cores, separated by commas>, -k <port mask>, -n <NF core mask>
• New syntax: ./go.sh -k <port mask> -n <NF core mask> [OPTIONS]

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

Merging notes:

• Dependencies: bc

bc is a new dependency for openNetVM. It can be installed with

sudo apt-get install bc

TODO before merging :

• [x] PR is ready for review

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.

[onvm]

In response to PR creation

CI Message

Aborting, need an authorized user to run CI

[EthanBaron14]

@onvm test this. I know you didn't mean to reject me

[onvm]

@onvm test this. I know you didn't mean to reject me

CI Message

Aborting, need an authorized user to run CI

[EthanBaron14]

@onvm test pls

[onvm]

@onvm test pls

CI Message

Aborting, need an authorized user to run CI

[EthanBaron14]

@onvm i'm gonna cry please test this

[onvm]

@onvm i'm gonna cry please test this

CI Message

Your results will arrive shortly

[onvm]

@onvm i'm gonna cry please test this

CI Message

Error: Failed to parse Speed Tester stats

[EthanBaron14]

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.

[bdevierno1]

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.

[EthanBaron14]

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:

• Adjusting required parameters for manager to enable default manager cores 0,1,2
• New flag: -m <cores, separated by commas>
• New syntax: ./go.sh <port> <NF mask> [OPTIONS]

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.

[EthanBaron14]

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 -m flag as optional just to make it more intuitive for the user. This solution would preserve existing functionality but also simplify for new users or those who want to transition.

[EthanBaron14]

Going to go a little further with this. The old syntax will still be allowed, e.g. ./go.sh <cores> <port mask> <NF cores> [OPTIONS] but a new syntax will be encouraged across the board:

./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 -k and -n flags for the port mask and NF cores will be required flags, since there isn't an easy default for those. The script will not run unless these flags are specified.

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 -k and -n flags and optionally -m. Users will not be able to use both, though. Either a user uses the old syntax in its entirety, or uses the new syntax in its entirety. The implementation I have now with an optional set of manager cores will remain, but it won't be integrated into the old syntax. This will be exclusively used in the new syntax. Furthermore, attempting to use the old syntax but including the flags only used in the new syntax will raise an error and the script will not run.

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.

[EthanBaron14]

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 0xF8

For specifying manager cores, simply add the -m flag:

./go.sh -k 3 -n 0xF8 -m 0,1,2

Manager 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 -k and -n flags and optionally -m. Users will not be able to use both, though. Either a user uses the old syntax in its entirety, or uses the new syntax in its entirety. Attempting to use the old syntax but including the flags only used in the new syntax will raise an error and the script will not run.

Documentation has been updated to reflect the new syntax for the manager shell script as well as the added dependency of bc.

[EthanBaron14]

@onvm test this

[onvm]

@onvm test this

CI Message

Your results will arrive shortly

[onvm]

@onvm test this

CI Message

Error: Failed to parse Speed Tester stats

[EthanBaron14]

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.

[bdevierno1]

This is the command CI runs

[EthanBaron14]

This is the command CI runs

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/

[bdevierno1]

This is the command CI runs

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..

[onvm]

Test code from ethan

CI Message

Your results will arrive shortly

[onvm]

Test code from ethan

CI Message

Error: ERROR: Failed to fetch results from nimbnode23

[onvm]

Test code from ethan with reboot

CI Message

Your results will arrive shortly

[kevindweb]

I'll look at Ethan's code, but CI likes us now, I had to add apt install bc but that's basically it

[kevindweb]

Because of how much go script code this changes, we'll need to test every argument possible to make sure it all works, @EthanBaron14 have you been testing different stats, shared core and other onvm arguments that might not be exactly relevant, but could be altered by go.sh? Btw great job Ethan, code looks pretty clean so far!

[EthanBaron14]

Because of how much go script code this changes, we'll need to test every argument possible to make sure it all works, @EthanBaron14 have you been testing different stats, shared core and other onvm arguments that might not be exactly relevant, but could be altered by go.sh? Btw great job Ethan, code looks pretty clean so far!

I was just thinking about this as I wrote it up. I think right now, the script will freak out if -k or -n aren't the first argument, just because I test with those specific flags to determine if the user is using the legacy syntax. I might mess with the logic a little bit so it accepts any flag so users can use them out of order like ./go.sh -s stdout -n 0xF8 -k 1 instead of requiring that one of the new flags be first. Otherwise, I haven't tested too much with the other arguments, but currently, as long as they're after one of the required ones, there should be no problem.

[EthanBaron14]

@onvm how's this?

[onvm]

@onvm how's this?

CI Message

Your results will arrive shortly

[EthanBaron14]

@WilliamMaa @kevindweb I believe that everything I planned to add has been implemented at this point. Feel free to test and give feedback as necessary.

[kevindweb]

@onvm want to test for real?

[onvm]

@onvm want to test for real?

CI Message

Your results will arrive shortly

[onvm]

@onvm want to test for real?

CI Message

Error: Failed to post results to GitHub

[onvm]

@onvm want to test for real?

CI Message

Error: ERROR: Failed to clean up and restart nimbnode23

[kevindweb]

@onvm one last time?

[kevindweb]

@onvm now?

[onvm]

@onvm now?

CI Message

Your results will arrive shortly

[EthanBaron14]

Just pushed new commits with a number of changes suggested by @kevindweb. Tested using the same testing procedure outlined in the PR description.

[kevindweb]

Awesome! The changes look good to me, I’ll test a bit more later but solid work here

[EthanBaron14]

@onvm hey

[onvm]

@onvm hey

CI Message

Another CI run in progress, adding request to the end of the list

[onvm]

@onvm hey

CI Message

Your results will arrive shortly

[EthanBaron14]

@onvm now this one please

[onvm]

@onvm now this one please

CI Message

Another CI run in progress, adding request to the end of the list

[onvm]

@onvm now this one please

CI Message

Your results will arrive shortly

[kevindweb]

@EthanBaron14 I just ran your PR and the first thing I would say is to use the usage function more often. Printing error messages specifically is helpful, but when I run ./go.sh normally, with no arguments, I should see the usage statement giving me a few examples of how to run the manager.

Also, watch out for the merge conflicts with go.sh. I think it's a result of your other PR, might just want to merge develop back into this branch to fix those.

[EthanBaron14]

@EthanBaron14 I just ran your PR and the first thing I would say is to use the usage function more often. Printing error messages specifically is helpful, but when I run ./go.sh normally, with no arguments, I should see the usage statement giving me a few examples of how to run the manager.

Also, watch out for the merge conflicts with go.sh. I think it's a result of your other PR, might just want to merge develop back into this branch to fix those.

Hey Kevin! Thanks for taking a look at this. I’ll definitely add the usage function more often. I thought I tried to fix the merge conflicts by merging my other PR into this one but i’ll double check. Should have that done by end of day today.

[EthanBaron14]

@onvm actually the last time

[onvm]

@onvm actually the last time

CI Message

Your results will arrive shortly

[EthanBaron14]

@kevindweb I took your advice and called the usage function as much as possible where it would be helpful to do so. Also ensured that the PR merges with develop nicely. Should be good for a final review whenever you and @WilliamMaa get a few minutes.