Closed dennisafa closed 4 years ago
In response to PR creation
Your results will arrive shortly
Code looks clean and testing worked as usual! Good job working on this @dennisafa, an important feature added. Not entirely sure what NF next is, but you didn't make any new stats that didn't already exist.
Thanks for the review. ONVM_NF_ACTION_NEXT is used for a flow-table lookup action, as described in the docs
Added more descriptive output as per your suggestion @koolzz should be good to review
@onvm .
@onvm .
Your results will arrive shortly
@onvm .
Error: ERROR: Failed to post results to GitHub
@onvm .
CI Message
Error: ERROR: Failed to post results to GitHub
🤔
Ya so idk why it happened. I just looked at the logs. HTTPSConnectionPool(host='api.github.com', port=443): Read timed out.
It seems to have worked itself out though?
@onvm .
CI Message
Error: ERROR: Failed to post results to GitHub
🤔
Ya so idk why it happened. I just looked at the logs.
HTTPSConnectionPool(host='api.github.com', port=443): Read timed out.
It seems to have worked itself out though?
sounds like a github problem anyway
@onvm .
CI Message
Error: ERROR: Failed to post results to GitHub
🤔
Ya so idk why it happened. I just looked at the logs.
HTTPSConnectionPool(host='api.github.com', port=443): Read timed out.
It seems to have worked itself out though?sounds like a github problem anyway
You're right, somehow connecting to Github API took more than 10 seconds? But then somehow we actually approved the change in the end anyways?
@onvm .
CI Message
Error: ERROR: Failed to post results to GitHub
🤔
Ya so idk why it happened. I just looked at the logs.
HTTPSConnectionPool(host='api.github.com', port=443): Read timed out.
It seems to have worked itself out though?sounds like a github problem anyway
You're right, somehow connecting to Github API took more than 10 seconds? But then somehow we actually approved the change in the end anyways?
Ok ya Github is having problems. It just posted my comment twice in a row
@dennisafa think about how this should print out if we are in the CSV format output mode -- maybe just print one line with these stats in CSV format with a header listing the fields
@dennisafa, I agree with the above, implement and I'll merge.
@dennisafa is this still in progress or ready for review?
@dennisafa is this still in progress or ready for review?
still tuning it, it will be done by this weekend.
in addition to printing to the terminal, a csv file is created in the nf's directory with its stats. i thought this might be better than setting a flag, since it would be an arg change for each nf have a few small changes to make, should be ready to review by sunday
good to review @sdnfv/active_maintainers
@onvm test this stale review
@onvm test this stale review
Your results will arrive shortly
So much cleaner 👍
@onvm check this?
@onvm check this?
Your results will arrive shortly
Tested and got the correct desired output. Looks good. I decided to double check the output of the file with the linter on my terminal. python --version Python 2.7.16 python style/gwclint.py --verbose=4 onvm/onvm_nflib/onvm_nflib.c onvm/onvm_nflib/onvm_nflib.c:1349: Almost always, snprintf is better than strcpy [runtime/printf] [4] onvm/onvm_nflib/onvm_nflib.c:1354: Almost always, snprintf is better than strcat [runtime/printf] [4] I understand from line 1343 you are getting the total length of the string plus the null character therefore, this will not make undefined behavior unlikely due the source size being greater than the destination. However, will using strncat may be a bit safer and will help remove these errors from the linter?
Tested and got the correct desired output. Looks good. I decided to double check the output of the file with the linter on my terminal. _python --version Python 2.7.16 python style/gwclint.py --verbose=4 onvm/onvm_nflib/onvm_nflib.c onvm/onvm_nflib/onvm_nflib.c:1349: Almost always, snprintf is better than strcpy [runtime/printf] [4] onvm/onvm_nflib/onvmnflib.c:1354: Almost always, snprintf is better than strcat [runtime/printf] [4] I understand from line 1343 you are getting the total length of the string plus the null character therefore, this will not make undefined behavior unlikely due the source size being greater than the destination. However, will using strncat may be a bit safer and will help remove these errors from the linter?
Good catch, fixed
We now print NF specific stats on shutdown, such as total RX, dropped, etc.
Summary:
Adds the statistics print call before we exit the NF, giving a summary of its activity. An example of speed_tester + simple_forward is shown:
Usage:
Merging notes:
TODO before merging :
Test Plan:
Chained various NF's and exited to verify the final output was correct.
Review:
@koolzz @kevindweb let me know if we should add/remove any stats.