Bash Script Clean-Up - Githubissues

kevindweb commented 4 years ago

Update all bash scripts using a comprehensive linter Shellcheck.

Summary:

Using ShellCheck, a comprehensive Bash linter, I have updated all the bash scripts in the repository. I have talked with @koolzz in the past about allowing CI to lint not only *.c but also *.py and *.sh files. This is the first step, because I will do this for all python files as well before programming this into CI.

This is necessary because some of the updates improve our bash script performance (in terms of usability and speed) and safety. The biggest change was placing quotes around variables to ignore bash expansion of the variable contents. This is an unintended thing that we don't want bash to do, as it can cause issues and slows down processing or parameters.

I understand this is a huge undertaking, but will help the scripts (bash and python) a great deal in the future for maintainability and script programming. All future scripts changes can be checked with CI just as .c updates are.

Usage: Install shellcheck with sudo apt install shellcheck. I have tested this on Ubuntu 18.04 and the most recent develop branch. There is a required shellcheck configuration file to disable some checks. Below is the file. You can place this in the home directory and call it .shellcheckrc. It is automatically sourced by shellcheck each run. I am on version 0.7.0, which I found from the tarball. This is the version that allows for the config file below.

The ~/.shellcheckrc file

# Expanding Sourcing Files
disable=SC1090
# Source File Doesn't Exist
disable=SC1091

# Exit On Fail CD
disable=SC2164

# Using $? For Exit Code
disable=SC2181

# Splitting Array Into Tuple
disable=SC2206

This PR includes
Resolves issues
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: None

TODO before merging :

[x] PR is ready for review
[X] Onvm scripts
[X] Example scripts
[X] Onvm_web scripts
[x] /scripts bash scripts

Test Plan:

The check boxes above are for each of the major scripts in the repository. They will need to be tested individually. Some haven't been tested in a while (/scripts/docker.sh), which will be good to verify anyways.

Review:

@koolzz @dennisafa

onvm commented 4 years ago

In response to PR creation

CI Message

Your results will arrive shortly

kevindweb commented 4 years ago

One thing to note about the updates is that above some lines a shellcheck disable=SCXXXX. I use this to disable some errors because we need parameter expansion in go scripts for example. Shellcheck isn't perfect for every situation, but is good at giving suggestions a lot of the time.

kevindweb commented 4 years ago

@twood02 thanks for seeing that. When doing these changes, I unfortunately did not know how to use the shellcheck "auto-fix", so I made them by myself. You were right, I made a mistake. I will update CI and check if the updated scripts still work there. I think it's necessary to add a section in the style/styleguide.md for people to check their bash scripts in the future. I will make a separate file on installing, and checking bash scripts with shellcheck, and how to auto-fix small lint errors.

Right now I've found a good function to run to check all of onvm:

for name in $(find . -name "*.sh"); do shellcheck $name; done

kevindweb commented 4 years ago

@onvm check the new scripts

kevindweb commented 4 years ago

@onvm my bad

onvm commented 4 years ago

@onvm my bad

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm my bad

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

kevindweb commented 4 years ago

Ok the issue was that for CI to work, I need #176 merged into here. We will need to merge that one first, so that the changes are already placed into develop

kevindweb commented 4 years ago

@onvm work please

onvm commented 4 years ago

@onvm work please

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm work please

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

onvm commented 4 years ago

Checking

CI Message

Your results will arrive shortly

onvm commented 4 years ago

Checking

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

onvm commented 4 years ago

Checking

CI Message

Your results will arrive shortly

onvm commented 4 years ago

Checking

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

onvm commented 4 years ago

Checking

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

kevindweb commented 4 years ago

@onvm fixes work?

onvm commented 4 years ago

@onvm fixes work?

CI Message

Your results will arrive shortly

twood02 commented 4 years ago

@kevindweb can you add a few key scripts for us to test and verify everything still works here?

twood02 commented 4 years ago

@kevindweb is this ready for a final review / merge?

kevindweb commented 4 years ago

@twood02 Sorry I haven't been working on this lately. I will test this soon to make sure everything works. I do need people to give me edge cases though. For example, some NFs use config files, does testing still work with those? I will keep on this and set it to ready_for_gatekeeper when done.

kevindweb commented 4 years ago

Cleaned up the code a lot. Removed all the changes to CI (because CI will be removed from this repo). I tested all the go scripts, and speed_tester, scaling, and firewall very thoroughly. I didn't test every example because I didn't need to change any NF-specific scripts (only start_nf.sh and go.sh). I tested ONVM web and it seems to be working. I also tested the environment scripts, but I didn't run install.sh again, that might have issues we need to test. I don't really know how to test docker.sh though.

Overall, the changes seem like a lot but are mostly turning a variable from $myvariable to "$myvariable" because that can cause globbing and word splitting in bash. I disabled some unnecessary errors in Shellcheck and tested that all the major files pass the linter now.

dennisafa commented 4 years ago

Is there anyway for the linter to add double square brackets to the if statements? We should be consistent with that if we are to use it.

kevindweb commented 4 years ago

Not sure, I don't think so because shellcheck thinks that [ is totally valid. I'll look into that though.

kevindweb commented 4 years ago

@onvm Test

kevindweb commented 4 years ago

@onvm test again 23

onvm commented 4 years ago

@onvm test again 23

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm test again 23

CI Message

Error: Failed to parse Speed Tester stats

kevindweb commented 4 years ago

@onvm 23 test again

kevindweb commented 4 years ago

@onvm final test

onvm commented 4 years ago

@onvm final test

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm final test

CI Message

Error: ERROR: Failed to fetch results from nimbnode23

kevindweb commented 4 years ago

@onvm You got this!

onvm commented 4 years ago

@onvm You got this!

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm You got this!

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode23

kevindweb commented 4 years ago

@onvm try again

onvm commented 4 years ago

@onvm try again

CI Message

Your results will arrive shortly

onvm commented 4 years ago

@onvm try again

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode23

onvm commented 4 years ago

Message

CI Message

Your results will arrive shortly

kevindweb commented 4 years ago

@onvm pls

onvm commented 4 years ago

@onvm pls

CI Message

Your results will arrive shortly

dennisafa commented 4 years ago

Is this good to go @kevindweb or do we need more testing?

kevindweb commented 4 years ago

I think it's good, but I don't know how to test the docker.sh scripts or no_hyperthread.sh. All others have either been run by me specifically and/or CI during testing multiple times.

kevindweb commented 4 years ago

Just fixed the docker script. I've now tested all the different sections of bash scripts, from installation, go scripts, onvm web, docker, Pktgen, etc. CI tests most of these scripts, which is why I'm more confident, but I've individually tested all the scripts I previously mentioned for assurance.

bdevierno1 commented 4 years ago

Individually tested most of the scripts and used your branch during installation of ONVM on CloudLab. No issues.

For docker.sh I ran shellcheck and I was still getting error 2086 for line 82 and 96. I was using version 0.7.0. Is ${DIR} # disable=SC2086 supposed to disable the error for that one line? I noticed you have multiple occurrences of this throughout the if. Perhaps you can use
shellcheck disable=SC2086 at the beginning of the if statement instead?

https://github.com/koalaman/shellcheck/issues/744
In NFD/go.sh should it be ../start_nf.sh "$NF_DIR" instead of ../../?

kevindweb commented 4 years ago

@bdevierno1 thanks for commenting.

I messed up for the # disable=SC2086. In our .shellcheckrc, we have that line already. You can see this in your PR #196 . If you run with shellcheck 0.7.0, with .shellcheck in the root openNetVM directory, it will ignore that error. I'll fix in my code.
NFD/<exampleNFD>/go.sh are all symlinks to ../go.sh. We actually want to run each go script from the NFD example directory, for example openNetVM/examples/NFD/stateful_firewall/go.sh. Therefore the start_nf.sh script actually exists in ../../start_nf.sh inside openNetVM/examples (two directories back). I will test NFD again to make sure though.

twood02 commented 4 years ago

@bdevierno1 and @ethanbaron14 will look through this again

kevindweb commented 4 years ago

@onvm approve

onvm commented 4 years ago

@onvm approve

CI Message

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

onvm commented 4 years ago

@onvm approve

CI Message

Your results will arrive shortly

sdnfv / openNetVM

Bash Script Clean-Up #183

Summary:

Merging notes:

Test Plan:

Review:

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

CI Message

shellcheck disable=SC2086 at the beginning of the if statement instead?

CI Message

CI Message