[change] Requested changes

README.rst

@@ -94,6 +94,7 @@ Available Features
 * Extensible metrics and charts: it's possible to define new metrics and new charts
 * API to retrieve the chart metrics and status information of each device
   based on `NetJSON DeviceMonitoring <http://netjson.org/docs/what.html#devicemonitoring>`_
+* Collection of monitoring information via `SNMP`_
 
 ------------
 
@@ -503,15 +504,29 @@ configuration status of a device changes, this ensures the check reacts
 quickly to events happening in the network and informs the user promptly
 if there's anything that is not working as intended.
 
-SNMP DeviceMonitoring
-~~~~~~~~~~~~~~~~~~~~~
+SNMP
+~~~~
 
-This check provides an alternative protocol to collect monitoring information from devices that don't have
-the ability to use `openwisp-config agent <https://github.com/openwisp/openwisp-config/>`_.
+This check provides an alternative way to collect monitoring information from devices that don't have
+the ability to use `openwisp-monitoring agent <https://github.com/openwisp/openwrt-openwisp-monitoring>`_.
 The information is collected via SNMP using `netengine <https://github.com/openwisp/netengine/>`_. The devices
 need to have an SNMP daemon in order for this to work.
 This check is disabled by default, you may choose to enable auto creation of this check by setting
-`OPENWISP_MONITORING_AUTO_SNMP_DEVICEMONITORING <#OPENWISP_MONITORING_AUTO_SNMP_DEVICEMONITORING>`_ to ``True``.
+`OPENWISP_MONITORING_AUTO_SNMP <#OPENWISP_MONITORING_AUTO_SNMP>`_ to ``True``.
+
+Instructions to use this feature:
+
+1. Register your device to OpenWISP via admin or `api <https://github.com/openwisp/openwisp-controller/#create-device>`_
+2. Create SNMP access credentials and add them to your device:
+
+   In the admin, go to ``Access Credentials > Add Access Credentials`` and select a type suitable for your device's backend.
+   Then either enable ``Auto Add``, or add it to your device manually.
+   Alternatively, use the `api endpoint <https://github.com/openwisp/openwisp-controller/#create-credential>`_
+3. Create an SNMP check:
+
+   In the admin, go to ``Check > Add Check``. Choose ``content type: config | Device``, ``Check type: SNMP Device Monitoring``
+   and write your device's UUID in the ``Object id`` section.
+   You can alternatively use the setting `OPENWISP_MONITORING_AUTO_SNMP <#OPENWISP_MONITORING_AUTO_SNMP>`_
 
 Settings
 --------
@@ -554,8 +569,8 @@ Whether ping checks are created automatically for devices.
 This setting allows you to choose whether `config_applied <#configuration-applied>`_ checks should be
 created automatically for newly registered devices. It's enabled by default.
 
-``OPENWISP_MONITORING_AUTO_SNMP_DEVICEMONITORING``
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+``OPENWISP_MONITORING_AUTO_SNMP``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 +--------------+-------------+
 | **type**:    | ``bool``    |

openwisp_monitoring/check/apps.py

@@ -33,11 +33,11 @@ def _connect_signals(self):
                 dispatch_uid='auto_config_check',
             )
 
-        if app_settings.AUTO_SNMP_DEVICEMONITORING:
-            from .base.models import auto_snmp_devicemonitoring_receiver
+        if app_settings.AUTO_SNMP:
+            from .base.models import auto_snmp_receiver
 
             post_save.connect(
-                auto_snmp_devicemonitoring_receiver,
+                auto_snmp_receiver,
                 sender=load_model('config', 'Device'),
-                dispatch_uid='auto_snmp_devicemonitoring',
+                dispatch_uid='auto_snmp',
             )

openwisp_monitoring/check/base/models.py

@@ -122,7 +122,7 @@ def auto_config_check_receiver(sender, instance, created, **kwargs):
     )
 
 
-def auto_snmp_devicemonitoring_receiver(sender, instance, created, **kwargs):
+def auto_snmp_receiver(sender, instance, created, **kwargs):
     """
     Implements OPENWISP_MONITORING_AUTO_DEVICE_SNMP_DEVICEMONITORING
     The creation step is executed in the background

openwisp_monitoring/check/classes/__init__.py

@@ -1,3 +1,3 @@
 from .config_applied import ConfigApplied  # noqa
 from .ping import Ping  # noqa
-from .snmp_devicemonitoring import SnmpDeviceMonitoring  # noqa
+from .snmp_devicemonitoring import Snmp  # noqa

openwisp_monitoring/check/classes/snmp_devicemonitoring.py

@@ -15,7 +15,7 @@
 AlertSettings = load_model('monitoring', 'AlertSettings')
 
 
-class SnmpDeviceMonitoring(BaseCheck, MetricChartsMixin):
+class Snmp(BaseCheck, MetricChartsMixin):
     def check(self, store=True):
         result = self.netengine_instance.to_dict()
         self._init_previous_data(data=getattr(self.related_object, 'data', {}))

openwisp_monitoring/check/settings.py

@@ -5,13 +5,10 @@
     (
         ('openwisp_monitoring.check.classes.Ping', 'Ping'),
         ('openwisp_monitoring.check.classes.ConfigApplied', 'Configuration Applied'),
-        (
-            'openwisp_monitoring.check.classes.SnmpDeviceMonitoring',
-            'SNMP Device Monitoring',
-        ),
+        ('openwisp_monitoring.check.classes.Snmp', 'SNMP Device Monitoring',),
     ),
 )
 AUTO_PING = get_settings_value('AUTO_PING', True)
 AUTO_CONFIG_CHECK = get_settings_value('AUTO_DEVICE_CONFIG_CHECK', True)
-AUTO_SNMP_DEVICEMONITORING = get_settings_value('AUTO_SNMP_DEVICEMONITORING', False)
+AUTO_SNMP = get_settings_value('AUTO_SNMP', False)
 MANAGEMENT_IP_ONLY = get_settings_value('MANAGEMENT_IP_ONLY', True)

openwisp_monitoring/check/tasks.py

@@ -105,10 +105,10 @@ def auto_create_snmp_devicemonitoring(
     model, app_label, object_id, check_model=None, content_type_model=None
 ):
     """
-    Called by openwisp_monitoring.check.models.auto_snmp_devicemonitoring_receiver
+    Called by openwisp_monitoring.check.models.auto_snmp_receiver
     """
     Check = check_model or get_check_model()
-    devicemonitoring_path = 'openwisp_monitoring.check.classes.SnmpDeviceMonitoring'
+    devicemonitoring_path = 'openwisp_monitoring.check.classes.Snmp'
     has_check = Check.objects.filter(
         object_id=object_id, content_type__model='device', check=devicemonitoring_path
     ).exists()

openwisp_monitoring/check/tests/test_models.py

@@ -9,7 +9,7 @@
 
 from ...device.tests import TestDeviceMonitoringMixin
 from .. import settings as app_settings
-from ..classes import ConfigApplied, Ping, SnmpDeviceMonitoring
+from ..classes import ConfigApplied, Ping, Snmp
 from ..tasks import (
     auto_create_config_check,
     auto_create_ping,
@@ -53,11 +53,8 @@ def test_check_class(self):
             )
             self.assertEqual(c.check_class, ConfigApplied)
         with self.subTest('Test DeviceMonitoring check Class'):
-            c = Check(
-                name='SnmpDeviceMonitoring class check',
-                check=self._SNMP_DEVICEMONITORING,
-            )
-            self.assertEqual(c.check_class, SnmpDeviceMonitoring)
+            c = Check(name='Snmp class check', check=self._SNMP_DEVICEMONITORING,)
+            self.assertEqual(c.check_class, Snmp)
 
     def test_base_check_class(self):
         path = 'openwisp_monitoring.check.classes.base.BaseCheck'
@@ -96,7 +93,7 @@ def test_check_instance(self):
                 params={},
             )
             i = c.check_instance
-            self.assertIsInstance(i, SnmpDeviceMonitoring)
+            self.assertIsInstance(i, Snmp)
             self.assertEqual(i.related_object, obj)
             self.assertEqual(i.params, c.params)
 
@@ -121,7 +118,7 @@ def test_validation(self):
     def test_auto_check_creation(self):
         self.assertEqual(Check.objects.count(), 0)
         d = self._create_device(organization=self._create_org())
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         with self.subTest('Test AUTO_PING'):
             c1 = Check.objects.filter(check=self._PING).first()
             self.assertEqual(c1.content_object, d)
@@ -134,7 +131,7 @@ def test_auto_check_creation(self):
     def test_device_deleted(self):
         self.assertEqual(Check.objects.count(), 0)
         d = self._create_device(organization=self._create_org())
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         d.delete()
         self.assertEqual(Check.objects.count(), 0)
 
@@ -145,7 +142,7 @@ def test_config_modified_device_problem(self):
         self._create_config(status='modified', organization=self._create_org())
         d = Device.objects.first()
         d.monitoring.update_status('ok')
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         self.assertEqual(Metric.objects.count(), 0)
         self.assertEqual(AlertSettings.objects.count(), 0)
         check = Check.objects.filter(check=self._CONFIG_APPLIED).first()
@@ -174,7 +171,7 @@ def test_config_error(self):
         self._create_config(status='error', organization=self._create_org())
         dm = Device.objects.first().monitoring
         dm.update_status('ok')
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         self.assertEqual(Metric.objects.count(), 0)
         self.assertEqual(AlertSettings.objects.count(), 0)
         check = Check.objects.filter(check=self._CONFIG_APPLIED).first()
@@ -207,7 +204,7 @@ def test_config_error(self):
     @patch('openwisp_monitoring.check.settings.AUTO_PING', False)
     def test_config_check_critical_metric(self):
         self._create_config(status='modified', organization=self._create_org())
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         d = Device.objects.first()
         dm = d.monitoring
         dm.update_status('ok')
@@ -226,7 +223,7 @@ def test_config_check_critical_metric(self):
 
     def test_no_duplicate_check_created(self):
         self._create_config(organization=self._create_org())
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         d = Device.objects.first()
         auto_create_config_check.delay(
             model=Device.__name__.lower(),
@@ -249,7 +246,7 @@ def test_device_unreachable_no_config_check(self):
         self._create_config(status='modified', organization=self._create_org())
         d = self.device_model.objects.first()
         d.monitoring.update_status('critical')
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         c2 = Check.objects.filter(check=self._CONFIG_APPLIED).first()
         c2.perform_check()
         self.assertEqual(Metric.objects.count(), 0)
@@ -260,7 +257,7 @@ def test_device_unknown_no_config_check(self):
         self._create_config(status='modified', organization=self._create_org())
         d = self.device_model.objects.first()
         d.monitoring.update_status('unknown')
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         c2 = Check.objects.filter(check=self._CONFIG_APPLIED).first()
         c2.perform_check()
         self.assertEqual(Metric.objects.count(), 0)

openwisp_monitoring/check/tests/test_ping.py

@@ -201,7 +201,7 @@ def test_store_result(self, mocked_method):
         device.management_ip = '10.40.0.1'
         device.save()
         # check created automatically by autoping
-        self.assertEqual(Check.objects.count(), 3)
+        self.assertEqual(Check.objects.count(), 2)
         self.assertEqual(Metric.objects.count(), 0)
         self.assertEqual(Chart.objects.count(), 0)
         self.assertEqual(AlertSettings.objects.count(), 0)

openwisp_monitoring/check/tests/test_utils.py

@@ -6,7 +6,7 @@
 
 from ...check.tests.utils import MockOpenWRT
 from ...device.tests import TestDeviceMonitoringMixin
-from ..classes import Ping, SnmpDeviceMonitoring
+from ..classes import Ping, Snmp
 from ..settings import CHECK_CLASSES
 from ..tasks import perform_check
 from ..utils import run_checks_async
@@ -25,12 +25,12 @@ def _create_check(self):
         # check is automatically created via django signal
 
     @patch.object(Ping, '_command', return_value=_FPING_REACHABLE)
-    @patch.object(SnmpDeviceMonitoring, 'netengine_instance', MockOpenWRT)
+    @patch.object(Snmp, 'netengine_instance', MockOpenWRT)
     def test_run_checks_async_success(self, mocked_method):
         self._create_check()
         run_checks_async()
 
-    @patch.object(SnmpDeviceMonitoring, 'netengine_instance', MockOpenWRT)
+    @patch.object(Snmp, 'netengine_instance', MockOpenWRT)
     @patch.object(Ping, '_command', return_value=_FPING_REACHABLE)
     def test_management_command(self, mocked_method):
         self._create_check()

openwisp_monitoring/device/tests/test_transactions.py

@@ -8,9 +8,8 @@
 from openwisp_controller.connection.tests.base import CreateConnectionsMixin
 from openwisp_utils.tests import catch_signal
 
-from ...check.classes import Ping, SnmpDeviceMonitoring
+from ...check.classes import Ping
 from ...check.tests import _FPING_REACHABLE, _FPING_UNREACHABLE
-from ...check.tests.utils import MockOpenWRT
 from ..tasks import trigger_device_checks
 from . import DeviceMonitoringTransactionTestcase
 
@@ -55,8 +54,6 @@ def test_trigger_device_recovery_task(self, mocked_method):
             self._post_data(d.id, d.key, data)
             mock.assert_called_once()
 
-    @patch.object(SnmpDeviceMonitoring, 'netengine_instance', MockOpenWRT)
-    @patch('openwisp_monitoring.check.settings.AUTO_SNMP_DEVICEMONITORING', False)
     @patch.object(Ping, '_command', return_value=_FPING_UNREACHABLE)
     @patch.object(DeviceMonitoring, 'update_status')
     def test_trigger_device_recovery_task_regression(
@@ -69,7 +66,7 @@ def test_trigger_device_recovery_task_regression(
         self.assertTrue(Check.objects.exists())
         # we expect update_status() to be called once (by the check)
         # and not a second time directly by our code
-        self.assertEqual(mocked_update_status.call_count, 3)
+        self.assertEqual(mocked_update_status.call_count, 1)
 
     @patch.object(Check, 'perform_check')
     def test_is_working_false_true(self, perform_check):

tests/openwisp2/settings.py

@@ -192,7 +192,6 @@
     OPENWISP_MONITORING_MAC_VENDOR_DETECTION = False
     OPENWISP_MONITORING_API_URLCONF = 'openwisp_monitoring.urls'
     OPENWISP_MONITORING_API_BASEURL = 'http://testserver'
-    OPENWISP_MONITORING_AUTO_SNMP_DEVICEMONITORING = True
 
 # Temporarily added to identify slow tests
 TEST_RUNNER = 'openwisp_utils.tests.TimeLoggingTestRunner'