Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

purhan
Copy link
Contributor

@purhan purhan commented Jun 29, 2021

Closes #297

Checks:

  • I have manually tested the proposed changes
  • I have written new test cases to avoid regressions (if necessary)
  • I have updated the documentation (e.g. README.rst)

@purhan purhan added this to In progress in [GSoC 2021] Netengine & add SNMP to OpenWISP via automation Jun 29, 2021
@pandafy pandafy moved this from In progress to To do in [GSoC 2021] Netengine & add SNMP to OpenWISP Jul 8, 2021
@purhan purhan moved this from To do to In progress in [GSoC 2021] Netengine & add SNMP to OpenWISP Jul 28, 2021
[GSoC 2021] Netengine & add SNMP to OpenWISP automation moved this from In progress to Needs Review Jul 31, 2021
openwisp_monitoring/check/settings.py Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a 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.

@purhan purhan moved this from Needs Review to In progress in [GSoC 2021] Netengine & add SNMP to OpenWISP Jul 31, 2021
openwisp_monitoring/check/settings.py Outdated Show resolved Hide resolved
openwisp_monitoring/check/tasks.py Outdated Show resolved Hide resolved
openwisp_monitoring/check/tasks.py Outdated Show resolved Hide resolved
[GSoC 2021] Netengine & add SNMP to OpenWISP automation moved this from In progress to Needs Review Aug 3, 2021
Copy link
Member

@pandafy pandafy left a 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?

openwisp_monitoring/check/classes/snmp_devicemonitoring.py Outdated Show resolved Hide resolved
openwisp_monitoring/check/classes/snmp_devicemonitoring.py Outdated Show resolved Hide resolved
openwisp_monitoring/check/classes/snmp_devicemonitoring.py Outdated Show resolved Hide resolved
openwisp_monitoring/device/api/views.py Outdated Show resolved Hide resolved
@purhan purhan force-pushed the issues/297-snmp-check branch 2 times, most recently from 34a196a to fb6423f Compare August 6, 2021 13:21
@coveralls
Copy link

coveralls commented Aug 6, 2021

Coverage Status

Coverage increased (+0.03%) to 98.936% when pulling aaad59a on issues/297-snmp-check into abd9165 on master.

@purhan purhan force-pushed the issues/297-snmp-check branch 2 times, most recently from 8591465 to 2ad8688 Compare August 6, 2021 14:54
@purhan purhan marked this pull request as ready for review August 6, 2021 16:17
Copy link
Member

@pandafy pandafy left a 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:

# 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

@purhan
Copy link
Contributor Author

purhan commented Aug 19, 2021

Interface information of the device is not shown on in device status page. I think the culprit is the following code:

# 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

Copy link
Member

@nemesifier nemesifier left a 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.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
openwisp_monitoring/check/apps.py Outdated Show resolved Hide resolved
openwisp_monitoring/check/classes/snmp_devicemonitoring.py Outdated Show resolved Hide resolved
openwisp_monitoring/check/base/models.py Outdated Show resolved Hide resolved
tests/openwisp2/settings.py Outdated Show resolved Hide resolved
def _get_connnector(self):
connectors = {
'openwisp_controller.connection.connectors.snmp.Snmp': OpenWRT,
'openwisp_controller.connection.connectors.airos.snmp.Snmp': AirOS,
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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?

README.rst Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a 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?

@purhan
Copy link
Contributor Author

purhan commented Aug 24, 2021

During our testing we noticed that the CPU load chart is not laoded for AirOS, can you please fix this?

@nemesisdesign This was done in openwisp/netengine@5424532

openwisp_monitoring/check/base/models.py Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
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.

@pandafy pandafy moved this from Needs Review to In progress in [GSoC 2021] Netengine & add SNMP to OpenWISP Aug 25, 2021
@purhan purhan moved this from In progress to Needs Review in [GSoC 2021] Netengine & add SNMP to OpenWISP Aug 30, 2021
@nemesifier nemesifier added this to In progress in OpenWISP Priorities for next releases via automation Feb 17, 2022
@nemesifier nemesifier moved this from In progress to To do in OpenWISP Priorities for next releases Feb 17, 2022
- 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.
@Aryamanz29
Copy link
Member

I need to increase coverage.

@Aryamanz29
Copy link
Member

Aryamanz29 commented May 19, 2022

@nemesisdesign The coverage is reduced due to absence of test_auto_check_creation -> SNMP auto subtest (which I later added), So I tried various methods to set AUTO_SNMP = True in check/tests/test_models.py using @patch() / @patch_objects() /@override_settings() , They indeed set AUTO_SNMP to True but before calling post_save.connect() (checks/apps.py) AUTO_SNMP always False due to which check never created.

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 :

  • Set AUTO_SNMP directly in openwisp-monitoring/tests/openwisp2/settings.py
  • Made required changes in broken tests aaad59a.
# settings.py 
if TESTING:
    ...
    ...
    # for testing AUTO_SNMP
    OPENWISP_MONITORING_AUTO_SNMP = True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

[feature] Add an SNMP check
5 participants