Bash Script Clean-Up #183

kevindweb · 2020-01-19T22:01:49Z

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 :

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

This PR releases OpenNetVM v19.05

This PR adds the following bug fix to the master branch: [Bug Fix] Fix Typo in Console Stats Header (sdnfv#142)

[Bug Fix] Fix Stats Header in Release Notes (sdnfv#145)

This PR releases OpenNetVM v19.07

Continuous Integration now runs on Ubuntu 18.04 flawlessly. We also started using Personal Access Tokens in github3.py instead of raw username and password.

Shellcheck, the bash linter, has many suggestions for cleaner, safer bash code that have been implemented.

onvm · 2020-01-19T22:01:51Z

In response to PR creation

CI Message

Your results will arrive shortly

onvm

In response to PR creation

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode17]

Median TX pps for Pktgen: 10746272
Performance rating - 107.46% (compared to 10000000 average)
Median TX pps for Speed Tester: 40109885
Performance rating - 100.27% (compared to 40000000 average)

kevindweb · 2020-01-19T22:22:54Z

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.

twood02

This is a good improvement. Can you put an example in the PR comments to show what it will look like if there is a linter failure? I assume CI will post the error?

Please verify the line I highlighted is correct.

ci/worker_files/speed-tester-worker.sh

kevindweb · 2020-01-20T17:42:51Z

@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 · 2020-01-20T17:46:10Z

@onvm check the new scripts

kevindweb · 2020-01-20T17:47:35Z

@onvm my bad

onvm · 2020-01-20T17:47:37Z

@onvm my bad

CI Message

Your results will arrive shortly

onvm · 2020-01-20T17:50:31Z

@onvm my bad

CI Message

Error: ERROR: Failed to copy ONVM files to nimbnode17

kevindweb · 2020-01-20T18:03:20Z

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 · 2020-01-20T18:06:13Z

@onvm work please

onvm · 2020-01-20T18:06:14Z

@onvm work please

CI Message

Your results will arrive shortly

onvm · 2020-01-20T18:08:23Z

@onvm work please

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

onvm · 2020-01-20T18:17:43Z

Checking

CI Message

Your results will arrive shortly

onvm · 2020-01-20T18:19:50Z

Checking

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

onvm · 2020-01-20T18:45:05Z

Checking

CI Message

Your results will arrive shortly

onvm · 2020-01-20T18:45:35Z

Checking

CI Message

Error: ERROR: Failed to fetch results from nimbnode17

onvm · 2020-03-11T20:27:42Z

@onvm pls

CI Message

Your results will arrive shortly

onvm

@onvm pls

CI Message

Run successful see results:
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7736974
Performance rating - 100.48% (compared to 7700000 average)
Median TX pps for Speed Tester: 42102927
Performance rating - 100.25% (compared to 42000000 average)

dennisafa · 2020-03-14T21:34:50Z

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

kevindweb · 2020-03-14T21:38:18Z

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 · 2020-04-05T22:16:05Z

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 · 2020-04-14T11:29:24Z

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?
Can't disable SC2086 when on an elif branch koalaman/shellcheck#744
In NFD/go.sh should it be ../start_nf.sh "$NF_DIR" instead of ../../?

kevindweb · 2020-04-14T17:53:12Z

@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 GitHub actions #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 · 2020-04-15T23:13:26Z

@bdevierno1 and @EthanBaron14 will look through this again

EthanBaron14

Reviewed and individually ran each script. No issues. Ran each of the scripts on Nimbus nodes on a fresh ONVM installation. Observed that each script would produce expected result.

bdevierno1

Looked through each individual file again.

kevindweb · 2020-05-03T14:38:19Z

@onvm approve

onvm · 2020-05-03T14:38:21Z

@onvm approve

CI Message

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

onvm · 2020-05-03T14:42:29Z

@onvm approve

CI Message

Your results will arrive shortly

onvm

@onvm approve

CI Message

Run successful see results:
Test took: 7 minutes, 43 seconds
✔️ PR submitted to develop branch
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed
✔️ Linter passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7718023
Performance rating - 100.23% (compared to 7700000 average)
Median TX pps for Speed Tester: 42171723
Performance rating - 100.41% (compared to 42000000 average)

koolzz and others added 9 commits June 4, 2019 14:45

Merge pull request sdnfv#134 from sdnfv/develop

46bbc96

This PR releases OpenNetVM v19.05

Merge pull request sdnfv#144 from sdnfv/develop

ea0bf0c

This PR adds the following bug fix to the master branch: [Bug Fix] Fix Typo in Console Stats Header (sdnfv#142)

Merge pull request sdnfv#146 from sdnfv/develop

c12d26d

[Bug Fix] Fix Stats Header in Release Notes (sdnfv#145)

Merge pull request sdnfv#156 from sdnfv/develop

30cf49a

This PR releases OpenNetVM v19.07

Updates for CI to run on ubuntu 18, as well as PAT

c2a0d34

Continuous Integration now runs on Ubuntu 18.04 flawlessly. We also started using Personal Access Tokens in github3.py instead of raw username and password.

Exit upon python creds length failure

0635626

Fix spacing and comment out mtcp

ce9daaf

Lot of updates to shell scripts for Shellcheck

c1368a9

Shellcheck, the bash linter, has many suggestions for cleaner, safer bash code that have been implemented.

Fix parameter expansion with disables statements

be07690

onvm previously approved these changes Jan 19, 2020

View reviewed changes

twood02 requested changes Jan 20, 2020

View reviewed changes

ci/worker_files/speed-tester-worker.sh Outdated Show resolved Hide resolved

Minor conditional updates

3fe58d7

kevindweb dismissed onvm’s stale review via 3fe58d7 January 20, 2020 17:24

Merge ubuntu18_tokens (sdnfv#176) into shellcheck_updates

aa21978

Fix early termination issue

ff9d5d1

onvm previously approved these changes Mar 11, 2020

View reviewed changes

Fix docker script for shellcheck

be7c5d9

kevindweb dismissed onvm’s stale review via be7c5d9 April 5, 2020 22:11

kevindweb mentioned this pull request Apr 7, 2020

Update Docker to Ubuntu 18 #202

Merged

2 tasks

twood02 added this to the ONVM 20.05 milestone Apr 9, 2020

dennisafa mentioned this pull request Apr 10, 2020

Release 20.05 #206

Closed

3 tasks

Update for SC2086

0f26a3c

EthanBaron14 approved these changes Apr 29, 2020

View reviewed changes

bdevierno1 approved these changes Apr 29, 2020

View reviewed changes

twood02 approved these changes Apr 29, 2020

View reviewed changes

onvm approved these changes May 3, 2020

View reviewed changes

kevindweb added the ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge label May 3, 2020

dennisafa approved these changes May 5, 2020

View reviewed changes

dennisafa merged commit fe5b253 into sdnfv:develop May 5, 2020

twood02 added NeedsReleaseNote Needs updated release note info and removed NeedsReleaseNote Needs updated release note info labels May 22, 2020

kevindweb deleted the shellcheck_updates branch June 1, 2020 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bash Script Clean-Up #183

Bash Script Clean-Up #183

kevindweb commented Jan 19, 2020 •

edited

Loading

onvm commented Jan 19, 2020

onvm left a comment

kevindweb commented Jan 19, 2020

twood02 left a comment

kevindweb commented Jan 20, 2020

kevindweb commented Jan 20, 2020

kevindweb commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Jan 20, 2020

kevindweb commented Jan 20, 2020

kevindweb commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Jan 20, 2020

onvm commented Mar 11, 2020

onvm left a comment

dennisafa commented Mar 14, 2020

kevindweb commented Mar 14, 2020

kevindweb commented Apr 5, 2020

bdevierno1 commented Apr 14, 2020

kevindweb commented Apr 14, 2020

twood02 commented Apr 15, 2020

EthanBaron14 left a comment •

edited

Loading

bdevierno1 left a comment

kevindweb commented May 3, 2020

onvm commented May 3, 2020

onvm commented May 3, 2020

onvm left a comment

Bash Script Clean-Up #183

Bash Script Clean-Up #183

Conversation

kevindweb commented Jan 19, 2020 • edited Loading

Summary:

Merging notes:

Test Plan:

Review:

onvm commented Jan 19, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

kevindweb commented Jan 19, 2020

twood02 left a comment

Choose a reason for hiding this comment

kevindweb commented Jan 20, 2020

kevindweb commented Jan 20, 2020

kevindweb commented Jan 20, 2020

onvm commented Jan 20, 2020

CI Message

onvm commented Jan 20, 2020

CI Message

kevindweb commented Jan 20, 2020

kevindweb commented Jan 20, 2020

onvm commented Jan 20, 2020

CI Message

onvm commented Jan 20, 2020

CI Message

onvm commented Jan 20, 2020

CI Message

onvm commented Jan 20, 2020

CI Message

onvm commented Jan 20, 2020

CI Message

onvm commented Jan 20, 2020

CI Message

onvm commented Mar 11, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

dennisafa commented Mar 14, 2020

kevindweb commented Mar 14, 2020

kevindweb commented Apr 5, 2020

bdevierno1 commented Apr 14, 2020

kevindweb commented Apr 14, 2020

twood02 commented Apr 15, 2020

EthanBaron14 left a comment • edited Loading

Choose a reason for hiding this comment

bdevierno1 left a comment

Choose a reason for hiding this comment

kevindweb commented May 3, 2020

onvm commented May 3, 2020

CI Message

onvm commented May 3, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

kevindweb commented Jan 19, 2020 •

edited

Loading

EthanBaron14 left a comment •

edited

Loading