[change] Set device status to unknown if critical check is disaled/deleted #576

Closes #576

Author: praptisharma28

openwisp_monitoring/device/apps.py

@@ -44,12 +44,31 @@ def ready(self):
         self.connect_config_status_changed()
         self.connect_wifi_client_signals()
         self.connect_offline_device_close_wifisession()
+        self.connect_check_signals()
         self.device_recovery_detection()
         self.set_update_config_model()
         self.register_dashboard_items()
         self.register_menu_groups()
         self.add_connection_ignore_notification_reasons()
 
+    def connect_check_signals(self):
+        from django.db.models.signals import post_delete, post_save
+        from swapper import load_model
+
+        Check = load_model('check', 'Check')
+        DeviceMonitoring = load_model('device_monitoring', 'DeviceMonitoring')
+
+        post_save.connect(
+            DeviceMonitoring.handle_critical_metric,
+            sender=Check,
+            dispatch_uid='check_post_save_receiver',
+        )
+        post_delete.connect(
+            DeviceMonitoring.handle_critical_metric,
+            sender=Check,
+            dispatch_uid='check_post_delete_receiver',
+        )
+
     def connect_device_signals(self):
         from .api.views import DeviceMetricView
 

openwisp_monitoring/device/base/models.py

@@ -9,6 +9,7 @@
 from django.core.cache import cache
 from django.core.exceptions import ValidationError
 from django.db import models
+from django.db.models.signals import post_delete
 from django.dispatch import receiver
 from django.utils.timezone import now
 from django.utils.translation import gettext_lazy as _
@@ -21,6 +22,7 @@
 from swapper import load_model
 
 from openwisp_controller.config.validators import mac_address_validator
+from openwisp_monitoring.device.settings import get_critical_device_metrics
 from openwisp_utils.base import TimeStampedEditableModel
 
 from ...db import device_data_query, timeseries_db
@@ -454,6 +456,21 @@ def handle_disabled_organization(cls, organization_id):
             status='unknown'
         )
 
+    @classmethod
+    def _get_critical_metric_keys(cls):
+        return [metric['key'] for metric in get_critical_device_metrics()]
+
+    @classmethod
+    def handle_critical_metric(cls, instance, **kwargs):
+        critical_metrics = cls._get_critical_metric_keys()
+        if instance.check_type in critical_metrics:
+            try:
+                device_monitoring = cls.objects.get(device=instance.content_object)
+                if not instance.is_active or kwargs.get('signal') == post_delete:
+                    device_monitoring.update_status('unknown')
+            except cls.DoesNotExist:
+                pass
+
 
 class AbstractWifiClient(TimeStampedEditableModel):
     id = None

openwisp_monitoring/device/migrations/0009_update_device_status_for_disabled_critical_checks.py

@@ -0,0 +1,27 @@
+from django.db import migrations
+from swapper import load_model
+
+
+def update_critical_device_metric_status(apps, schema_editor):
+    Check = apps.get_model('check', 'Check')
+    # We need to load the real concrete model here, because we
+    # will be calling one of its class methods below
+    DeviceMonitoring = load_model('device_monitoring', 'DeviceMonitoring')
+    critical_metrics_keys = DeviceMonitoring._get_critical_metric_keys()
+
+    for check in Check.objects.filter(
+        is_active=False, check_type__in=critical_metrics_keys
+    ).iterator():
+        DeviceMonitoring.handle_critical_metric(check)
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('device_monitoring', '0008_alter_wificlient_options'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            update_critical_device_metric_status, reverse_code=migrations.RunPython.noop
+        ),
+    ]

openwisp_monitoring/device/tests/test_models.py

@@ -578,6 +578,73 @@ def _create_env(self):
         )
         return dm, ping, load, process_count
 
+    def test_disabling_critical_check(self):
+        Check = load_model('check', 'Check')
+        dm, ping, load, process_count = self._create_env()
+        ping_check_instance = Check.objects.create(
+            name='Ping Check',
+            check_type='ping',
+            content_object=dm.device,
+            params={},
+        )
+        dm.update_status('ok')
+        with catch_signal(health_status_changed) as handler:
+            ping_check_instance.is_active = False
+            ping_check_instance.save()
+        self.assertEqual(handler.call_count, 1)
+        call_args = handler.call_args[1]
+        self.assertEqual(call_args['instance'], dm)
+        self.assertEqual(call_args['status'], 'unknown')
+        dm.refresh_from_db()
+        self.assertEqual(dm.status, 'unknown')
+        with self.subTest(
+            'Ensure status does not change on saving active critical check'
+        ):
+            ping_check_instance.is_active = True
+            ping_check_instance.save()
+            dm.refresh_from_db()
+            self.assertEqual(dm.status, 'unknown')
+
+    def test_saving_non_critical_check(self):
+        Check = load_model('check', 'Check')
+        dm, ping, load, process_count = self._create_env()
+        # Ensure initial status is 'ok'
+        dm.update_status('ok')
+        # Created a non-critical check
+        non_critical_check = Check.objects.create(
+            name='Configuration Applied',
+            check_type='non_critical',
+            content_object=dm.device,
+            params={},
+        )
+        with catch_signal(health_status_changed) as handler:
+            non_critical_check.is_active = False
+            non_critical_check.save()
+        self.assertEqual(
+            handler.call_count, 0, 'Signal should not be fired for non-critical check'
+        )
+        dm.refresh_from_db()
+        self.assertEqual(dm.status, 'ok')
+
+    def test_deleting_critical_check(self):
+        Check = load_model('check', 'Check')
+        dm, ping, load, process_m = self._create_env()
+        ping_check_instance = Check.objects.create(
+            name='Ping Check',
+            check_type='ping',
+            content_object=dm.device,
+            params={},
+        )
+        dm.update_status('ok')
+        with catch_signal(health_status_changed) as handler:
+            ping_check_instance.delete()
+        self.assertEqual(handler.call_count, 1)
+        call_args = handler.call_args[1]
+        self.assertEqual(call_args['instance'], dm)
+        self.assertEqual(call_args['status'], 'unknown')
+        dm.refresh_from_db()
+        self.assertEqual(dm.status, 'unknown')
+
     def test_status_changed(self):
         dm, ping, load, process_count = self._create_env()
         # check signal