[change] Changes updated #576

Fixes #576

openwisp_monitoring/device/apps.py

@@ -32,8 +32,6 @@
     manage_short_retention_policy,
 )
 
-Check = load_model('check', 'Check')
-
 
 class DeviceMonitoringConfig(AppConfig):
     name = 'openwisp_monitoring.device'
@@ -56,16 +54,21 @@ def ready(self):
         self.connect_check_signals()
 
     def connect_check_signals(self):
+        from swapper import load_model
+
+        Check = load_model('check', 'Check')
+        DeviceData = load_model('device_monitoring', 'DeviceData')
+
+        @receiver(post_save, sender=Check)
+        def check_post_save_receiver(sender, instance, **kwargs):
+            if not instance.is_active:
+                handle_critical_check_change(instance)
+
         post_save.connect(
-            check_post_save_receiver,
+            DeviceData.handle_unknown_status_change,
             sender=Check,
             dispatch_uid='check_post_save_receiver',
         )
-        post_delete.connect(
-            check_post_delete_receiver,
-            sender=Check,
-            dispatch_uid='check_post_delete_receiver',
-        )
 
     def connect_device_signals(self):
         from .api.views import DeviceMetricView
@@ -472,17 +475,3 @@ def add_connection_ignore_notification_reasons(self):
         ConnectionConfig._ignore_connection_notification_reasons.extend(
             ['timed out', 'Unable to connect']
         )
-
-
-@receiver(post_save, sender=Check)
-def check_post_save_receiver(sender, instance, **kwargs):
-    if instance.is_active:
-        return
-    handle_critical_check_change(instance)
-
-
-@receiver(post_delete, sender=Check)
-def check_post_delete_receiver(sender, instance, **kwargs):
-    critical_metrics = settings.OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS
-    if instance.metric.name in critical_metrics:
-        handle_critical_check_change(instance)

openwisp_monitoring/device/migrations/0009_update_device_status_for_disabled_critical_checks.py

@@ -8,9 +8,16 @@
 
 
 def update_device_status(apps, schema_editor):
-    critical_metrics = settings.OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS
-    for check in Check.objects.filter(is_active=False):
-        if check.object_id and check.metric.name in critical_metrics:
+    Check = load_model('check', 'Check')
+    Device = load_model('config', 'Device')
+    critical_metrics_keys = [
+        metric['key'] for metric in settings.OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS
+    ]
+
+    for check in Check.objects.filter(
+        is_active=False, metric__key__in=critical_metrics_keys
+    ).iterator():
+        if check.object_id:
             device = Device.objects.get(pk=check.object_id)
             device.monitoring.update_status('unknown')
 

openwisp_monitoring/device/models.py

@@ -1,3 +1,4 @@
+from django.conf import settings
 from django.contrib.contenttypes.fields import GenericRelation
 from swapper import get_model_name, load_model, swappable_setting
 
@@ -19,6 +20,21 @@ class Meta:
         proxy = True
         swappable = swappable_setting('device_monitoring', 'DeviceData')
 
+    def handle_unknown_status_change(self, instance, **kwargs):
+        critical_metrics = settings.OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS
+        if instance.metric.name in critical_metrics:
+            if kwargs.get('created', False):
+                self.update_status('unknown')
+            elif kwargs.get('sender', None).__name__ == 'Check':
+                self.update_status('unknown')
+
+    def handle_critical_check_change(cls, check):
+        critical_metrics = settings.OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS
+        device_data_instances = DeviceData.objects.filter(pk=check.object_id)
+        for instance in device_data_instances:
+            if check.metric.name in critical_metrics:
+                instance.update_status('unknown')
+
 
 class DeviceMonitoring(AbstractDeviceMonitoring):
     class Meta(AbstractDeviceMonitoring.Meta):

openwisp_monitoring/device/signals.py

@@ -8,7 +8,3 @@
 device_metrics_received.__doc__ = """
 Providing arguments: ['instance', 'request', 'time', 'current']
 """
-device_status_unknown = Signal()
-device_status_unknown.__doc__ = """
-Providing arguments: ['instance']
-"""

openwisp_monitoring/device/tests/test_apps.py

@@ -1,6 +1,9 @@
 from django.test import TestCase
 
+from openwisp_monitoring.device.models import CHECK_CLASSES, Check
+from openwisp_monitoring.device.signals import health_status_changed
 from openwisp_utils.admin_theme.dashboard import DASHBOARD_CHARTS, DASHBOARD_TEMPLATES
+from openwisp_utils.tests import catch_signal
 
 
 class TestApps(TestCase):
@@ -37,3 +40,54 @@ def test_device_map_template_registered(self):
                 'monitoring/js/device-map.js',
             ),
         )
+
+
+class TestCheckSignals(TestCase):
+    def setUp(self):
+        self.device = self._create_device(organization=self._create_org())
+        self.ping_check = Check.objects.create(
+            name='Ping Check',
+            check_type=CHECK_CLASSES[0][0],
+            content_object=self.device,
+            params={},
+        )
+        self.device.monitoring.update_status('ok')
+
+    def test_check_signals(self):
+        with self.subTest('Test disabling a critical check'):
+            self.assertEqual(self.device.monitoring.status, 'ok')
+            with catch_signal(health_status_changed) as handler:
+                self.ping_check.is_active = False
+                self.ping_check.save()
+            self.assertEqual(handler.call_count, 1)
+            call_args = handler.call_args[0]
+            self.assertEqual(call_args[0], self.device.monitoring)
+            self.assertEqual(call_args[1], 'unknown')
+            self.device.refresh_from_db()
+            self.assertEqual(self.device.monitoring.status, 'unknown')
+            self.device.monitoring.update_status('ok')
+
+        with self.subTest('Test saving an active critical check'):
+            self.assertEqual(self.device.monitoring.status, 'ok')
+            self.ping_check.is_active = True
+            self.ping_check.save()
+            self.device.refresh_from_db()
+            self.assertEqual(self.device.monitoring.status, 'ok')
+
+        with self.subTest('Test saving a non-critical check'):
+            self.assertEqual(self.device.monitoring.status, 'ok')
+            non_critical_check = Check.objects.create(
+                name='Configuration Applied',
+                check_type=CHECK_CLASSES[1][0],
+                content_object=self.device,
+                params={},
+            )
+            non_critical_check.save()
+            self.device.refresh_from_db()
+            self.assertEqual(self.device.monitoring.status, 'ok')
+
+        with self.subTest('Test deleting a critical check'):
+            self.device.monitoring.update_status('ok')
+            self.ping_check.delete()
+            self.device.refresh_from_db()
+            self.assertEqual(self.device.monitoring.status, 'unknown')

openwisp_monitoring/device/utils.py

@@ -1,23 +1,14 @@
-from django.conf import settings
 from swapper import load_model
 
 from ..db import timeseries_db
 from . import settings as app_settings
-from .signals import device_status_unknown
 
 SHORT_RP = 'short'
 DEFAULT_RP = 'autogen'
 
 Device = load_model('config', 'Device')
 DeviceMonitoring = load_model('device_monitoring', 'DeviceMonitoring')
-
-
-def handle_critical_check_change(check):
-    critical_metrics = settings.OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS
-    if check.object_id and check.metric.name in critical_metrics:
-        device = Device.objects.get(pk=check.object_id)
-        device.monitoring.update_status('unknown')
-        device_status_unknown.send(sender=DeviceMonitoring, instance=device.monitoring)
+DeviceData = load_model('device_monitoring', 'DeviceData')
 
 
 def get_device_cache_key(device, context='react-to-updates'):