-
-
Notifications
You must be signed in to change notification settings - Fork 120
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 SNMP check #297 #309
base: master
Are you sure you want to change the base?
Conversation
ac73075
to
dc4887f
Compare
0680828
to
c81fbfe
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.
There are conflicts with the base branch as well.
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.
Good work @purhan! Can you please answer to my following queries?
34a196a
to
fb6423f
Compare
8591465
to
2ad8688
Compare
7fca813
to
3d094ac
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.
Interface information of the device is not shown on in device status page. I think the culprit is the following code:
openwisp-monitoring/openwisp_monitoring/device/base/models.py
Lines 80 to 82 in 3d094ac
# don't show interfaces if they don't have any useful info | |
if len(interface.keys()) <= 2: | |
continue |
@purhan please ensure that information about interfaces is displayed
@pandafy Thanks! I found out the problem in netengine and fixed it in this commit. It should be working now, we don't need to change anything here |
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.
Good progress! See my comments below.
def _get_connnector(self): | ||
connectors = { | ||
'openwisp_controller.connection.connectors.snmp.Snmp': OpenWRT, | ||
'openwisp_controller.connection.connectors.airos.snmp.Snmp': AirOS, |
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.
It's not very clear to me how OpenWrt or AirOS is chosen
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 We are selecting a class from Netengine based on the type of connector. It's like a switch-case statement.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
@pandafy we can still add a setting later if we feel it can really be useful, at this stage it's not so urgent, right?
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.
During our testing we noticed that the CPU load chart is not laoded for AirOS, can you please fix this?
c339816
to
9b977a5
Compare
@nemesisdesign This was done in openwisp/netengine@5424532 |
def _get_connnector(self): | ||
connectors = { | ||
'openwisp_controller.connection.connectors.snmp.Snmp': OpenWRT, | ||
'openwisp_controller.connection.connectors.airos.snmp.Snmp': AirOS, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
c0972be
to
a08651c
Compare
6c4eb6d
to
b8056d8
Compare
- Completed rebase. - Fixed failing tests to make sure it works on current version of openwisp_monitoring. - Updated settings of snmp_check acc to new openwisp_controller snmp credentials. - Formatted code and fixed all QA warnings.
658aaa0
to
7c4cfda
Compare
I need to increase coverage. |
0af2dff
to
aaad59a
Compare
@nemesisdesign The coverage is reduced due to absence of See below: # check/tests/test_models.py
@patch('openwisp_monitoring.check.settings.AUTO_SNMP', True)
def test_auto_check_creation(self):
self.assertEqual(Check.objects.count(), 0)
d = self._create_device(organization=self._create_org())
print(app_settings.__dict__)
print(Check.objects.all())
# check is automatically created via django signal
self.assertEqual(Check.objects.count(), 3) # It should have Ping, Config Applied & SNMP
... ...
... ...
with self.subTest('Test AUTO_SNMP'):
c3 = Check.objects.filter(check_type=self._SNMP_DEVICEMONITORING).first()
self.assertEqual(c3.content_object, d)
self.assertEqual(self._SNMP_DEVICEMONITORING, c3.check_type) print(f'Value of AUTO_SNMP = {app_settings.AUTO_SNMP}')
if app_settings.AUTO_SNMP:
...
...
print(app_settings.__dict__)
print(Check.objects.all())
# output
Value of AUTO_SNMP = False # Because of this snmp signal not called and check not created
... ... ... 'AUTO_PING': True, 'AUTO_CONFIG_CHECK': True, 'AUTO_SNMP': True, 'MANAGEMENT_IP_ONLY': False, 'PING_CHECK_CONFIG': {}}
<QuerySet [<Check: Ping (Device: default.test.device)>, <Check: Configuration Applied (Device: default.test.device)>]>
AssertionError: 2 != 3
----------------------------------------------------------------------
Ran 1 test in 0.092s
FAILED (failures=1) Current possible solution :
# settings.py
if TESTING:
...
...
# for testing AUTO_SNMP
OPENWISP_MONITORING_AUTO_SNMP = True |
Closes #297
Checks: