-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] Implement Iperf3 check #385 #394
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the target branch to gsoc22-iperf. Please let's use this from now on.
335cf4f
to
bad881d
Compare
f968f79
to
2b474ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @Aryamanz29! 👏🏼 👏🏼
@nemesisdesign @okraits things that I have tested manually:
- The
iperf
check is created automatically when a new device registers - The
iperf
check is not created ifOPENWISP_MONITORING_AUTO_IPERF
is set toFalse
. - The
iperf
check execute for a device having an active SSH connection. - The
iperf
check does not execute for a device whose SSH connection is innot_working
state - The charts are shown for the gathered data.
Edge cases to handle
- The database reports that
DeviceConnection
is working, but the management tunnel goes down just beforeiperf
check was executed. The code should be able to gracefully handle this and also notify users that SSH connection is not working. We already have a mechanism for sending notifications. @Aryamanz29 please check other checks for reference and ask us your doubts.
Things to discuss in next weekly meeting
- What parameter should trigger alert for
iperf
check? - Which data should be shown to users with charts?
b535521
to
016ea9f
Compare
@pandafy In 8823581 I have fixed #399, Now you can see below that error is longer produced because earlier we're executing command without considering the One more thing, do we really need to notify users for this event? I mean when I'm testing it locally, the "SSH not working" alert already popped out & it already fulfilled our purpose, what do you think? Although, I have added |
016ea9f
to
8823581
Compare
As the user is already getting one notification for SSH not working, we don't need to create another notification. Logging a warning is sufficient. I will do manual testing later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aryamanz29 I did a manual test of the changes and I don't see an error for #399. I will do an in-depth code review tomorrow.
Good work @Aryamanz29! 👏🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nemesisdesign @okraits I need some clarity on how we are planning to store the data in the InfluxDB. The current implementation ignores the bitrate from the UDP test, which seems wrong to me.
Thinking out loud
A good argument could be "because UDP bitrate would be similar to TCP bitrate so we can ignore that". If so, then why bother doing a UDP test? Is it only for getting jitter and packet loss?
4bf1fd9
to
1d94482
Compare
- Added ping config checks in celery beat configuration. - If args is None executes all checks.
- Added username, password, rsa-public-key-path parameter to DEFAULT_IPERF_CHECK_CONFIG. Closes #414
- Changed iperf auth logic (RSA key in params instead of path). - Added IPERF_CHECK_RSA_KEY_DELETE (If true RSA will be delete from the device). - Added IPERF_CHECK_RSA_KEY_PATH (To set default iperf RSA public key). - Impoved existing tests as per review suggestion.
- Connected data points with new bar+lines chart type. - Added fill(0) to the chart query to fill blank data points(ie.N/A). Closes #417
- Combined all iperf settings into one setting. - Removed default from OPENWISP_MONITORING_IPERF_CHECK_CONFIG - The order of getting params for iperf check is: check_params(through admin panel)-> OPENWISP_MONITORING_IPERF_CHECK_CONFIG-> DEFAULT_IPERF_CHECK_CONFIG - Updated iperf tests. Closes #418
- Added bitrate param to tcp and udp mode. - Improved iperf tests. - Improved alert_on_related_field tests.
- Found that options.mode = 'lines+markers' for the stackedbar+lines charts which streches charts x-axis to some backward time we can use options.mode = 'lines' for iperf charts to avoid this. Fixes #422
- Combined transfer and bandwidth charts. - improved docs.
- Removed alert_on_related feature from this PR.
5f4c51f
to
9213aaf
Compare
Steps to test
tests/openwisp2/settings.py
configure:NOTE : The host can be specified by hostname, IPv4 literal, or IPv6 literal
python3 manage.py run_checks
(To run all checks manually).Demo
iperf_demo.mp4
TCP Charts
UDP Charts
Closes #385