[change/fix] Recovery detection must trigger only critical checks #642

Closes #642

Author: pandafy

docs/user/settings.rst

@@ -619,21 +619,32 @@ Automatically created charts.
 ``OPENWISP_MONITORING_CRITICAL_DEVICE_METRICS``
 -----------------------------------------------
 
-============ ==================================================================
+============ ===================================================================================
 **type**:    ``list`` of ``dict`` objects
-**default**: .. code-block::
+**default**: .. code-block:: python
 
                  [
-                     {"key": "ping", "field_name": "reachable"},
-                     {"key": "data_collected", "field_name": "data_collected"},
+                     {
+                         "key": "ping",
+                         "field_name": "reachable",
+                         "check": "openwisp_monitoring.check.classes.Ping",  # optional
+                     },
+                     {
+                         "key": "data_collected",
+                         "field_name": "data_collected",
+                         "check": "openwisp_monitoring.check.classes.DataCollected",  # optional
+                     },
                  ]
-============ ==================================================================
+============ ===================================================================================
 
 Device metrics that are considered critical.
 
 When all critical metrics have values outside their expected range, the
 device's health status changes to ``CRITICAL``.
 
+The ``check`` field is optional for each metric. These checks are executed
+when attempting to detect the recovery of a device.
+
 .. _openwisp_monitoring_health_status_labels:
 
 ``OPENWISP_MONITORING_HEALTH_STATUS_LABELS``

openwisp_monitoring/check/base/models.py

@@ -49,6 +49,12 @@ class AbstractCheck(TimeStampedEditableModel):
     class Meta:
         abstract = True
         unique_together = ('name', 'object_id', 'content_type')
+        indexes = [
+            models.Index(
+                fields=['content_type', 'object_id', 'is_active'],
+                name='active_object_checks_idx',
+            )
+        ]
 
         permissions = (
             ('add_check_inline', 'Can add check inline'),

openwisp_monitoring/check/migrations/0011_check_active_object_checks_idx.py

@@ -0,0 +1,19 @@
+# Generated by Django 4.2.17 on 2025-03-25 12:00
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('check', '0010_create_data_collected_check'),
+    ]
+
+    operations = [
+        migrations.AddIndex(
+            model_name='check',
+            index=models.Index(
+                fields=['content_type', 'object_id', 'is_active'],
+                name='active_object_checks_idx',
+            ),
+        ),
+    ]

openwisp_monitoring/device/apps.py

@@ -224,11 +224,13 @@ def manage_device_recovery_cache_key(cls, instance, status, **kwargs):
 
     @classmethod
     def trigger_device_recovery_checks(cls, instance, **kwargs):
-        from .tasks import trigger_device_checks
+        from .tasks import trigger_device_critical_checks
 
         # Cache is managed by "manage_device_recovery_cache_key".
         if cache.get(get_device_cache_key(device=instance), False):
-            transaction_on_commit(lambda: trigger_device_checks.delay(pk=instance.pk))
+            transaction_on_commit(
+                lambda: trigger_device_critical_checks.delay(pk=instance.pk)
+            )
 
     @classmethod
     def connect_is_working_changed(cls):
@@ -242,7 +244,7 @@ def connect_is_working_changed(cls):
     def is_working_changed_receiver(
         cls, instance, is_working, old_is_working, failure_reason, **kwargs
     ):
-        from .tasks import trigger_device_checks
+        from .tasks import trigger_device_critical_checks
 
         Check = load_model('check', 'Check')
         device = instance.device
@@ -266,11 +268,15 @@ def is_working_changed_receiver(
             return
         if not is_working:
             if initial_status == 'ok':
-                transaction_on_commit(lambda: trigger_device_checks.delay(pk=device.pk))
+                transaction_on_commit(
+                    lambda: trigger_device_critical_checks.delay(pk=device.pk)
+                )
         else:
             # if checks exist trigger them else, set status as 'ok'
             if Check.objects.filter(object_id=instance.device.pk).exists():
-                transaction_on_commit(lambda: trigger_device_checks.delay(pk=device.pk))
+                transaction_on_commit(
+                    lambda: trigger_device_critical_checks.delay(pk=device.pk)
+                )
             else:
                 device_monitoring.update_status(status)
 

openwisp_monitoring/device/base/models.py

@@ -398,6 +398,19 @@ def get_active_metrics(cls):
             cls._active_metrics = active_metrics
         return cls._active_metrics
 
+    @classmethod
+    def get_critical_checks(cls):
+        """
+        Returns list of critical checks.
+        """
+        if not hasattr(cls, '_critical_checks'):
+            critical_checks = []
+            for metric in app_settings.CRITICAL_DEVICE_METRICS:
+                if metric.get('check'):
+                    critical_checks.append(metric['check'])
+            cls._critical_checks = critical_checks
+        return cls._critical_checks
+
     @staticmethod
     @receiver(threshold_crossed, dispatch_uid='threshold_crossed_receiver')
     def threshold_crossed(sender, metric, alert_settings, target, first_time, **kwargs):

openwisp_monitoring/device/settings.py

@@ -5,9 +5,21 @@
 
 
 def get_critical_device_metrics():
-    default = [{'key': 'ping', 'field_name': 'reachable'}]
+    default = [
+        {
+            'key': 'ping',
+            'field_name': 'reachable',
+            'check': 'openwisp_monitoring.check.classes.Ping',
+        }
+    ]
     if AUTO_DATA_COLLECTED_CHECK:
-        default.append({'key': 'data_collected', 'field_name': 'data_collected'})
+        default.append(
+            {
+                'key': 'data_collected',
+                'field_name': 'data_collected',
+                'check': 'openwisp_monitoring.check.classes.DataCollected',
+            }
+        )
     critical_metrics = get_settings_value(
         'CRITICAL_DEVICE_METRICS',
         default,

openwisp_monitoring/device/tasks.py

@@ -1,4 +1,5 @@
 import logging
+import warnings
 
 from celery import shared_task
 from django.core.exceptions import ObjectDoesNotExist
@@ -13,7 +14,7 @@
 
 
 @shared_task(base=OpenwispCeleryTask)
-def trigger_device_checks(pk, recovery=True):
+def trigger_device_critical_checks(pk, recovery=True):
     """Triggers the monitoring checks for the specified device pk.
 
     Retrieves all related checks to the passed ``device`` and calls the
@@ -28,7 +29,11 @@ def trigger_device_checks(pk, recovery=True):
     except ObjectDoesNotExist:
         logger.warning(f'The device with uuid {pk} has been deleted')
         return
-    check_ids = list(device.checks.filter(is_active=True).values_list('id', flat=True))
+    check_ids = list(
+        device.checks.filter(
+            is_active=True, check_type__in=device.monitoring.get_critical_checks()
+        ).values_list('id', flat=True)
+    )
     if not check_ids:
         status = 'ok' if recovery else 'critical'
         device.monitoring.update_status(status)
@@ -39,6 +44,19 @@ def trigger_device_checks(pk, recovery=True):
         perform_check.delay(check_id)
 
 
+@shared_task(base=OpenwispCeleryTask)
+def trigger_device_checks(pk, recovery=True):
+    """
+    Deprecated, use trigger_device_critical_checks instead.
+    """
+    warnings.warn(
+        'trigger_device_checks is deprecated, use trigger_device_critical_checks instead.',
+        DeprecationWarning,
+        stacklevel=2,
+    )
+    trigger_device_critical_checks(pk, recovery)
+
+
 @shared_task(base=OpenwispCeleryTask)
 def delete_wifi_clients_and_sessions(days=6 * 30):
     WifiClient = load_model('device_monitoring', 'WifiClient')

openwisp_monitoring/device/tests/test_models.py

@@ -16,7 +16,7 @@
 from ...db import timeseries_db
 from .. import settings as app_settings
 from ..signals import health_status_changed
-from ..tasks import delete_wifi_clients_and_sessions, trigger_device_checks
+from ..tasks import delete_wifi_clients_and_sessions, trigger_device_critical_checks
 from ..utils import get_device_cache_key
 from . import (
     DeviceMonitoringTestCase,
@@ -537,9 +537,9 @@ def test_resources_no_key(self):
             dd.validate_data()
 
     @patch('logging.Logger.warning')
-    def test_trigger_device_checks_task_resiliency(self, mock):
+    def test_trigger_device_critical_checks_task_resiliency(self, mock):
         dd = DeviceData(name='Test Device')
-        trigger_device_checks.delay(dd.pk)
+        trigger_device_critical_checks.delay(dd.pk)
         mock.assert_called_with(f'The device with uuid {dd.pk} has been deleted')
 
     def test_device_data_cache_set(self):

openwisp_monitoring/device/tests/test_recovery.py

@@ -5,13 +5,14 @@
 from swapper import load_model
 
 from ..signals import health_status_changed
-from ..tasks import trigger_device_checks
+from ..tasks import trigger_device_checks, trigger_device_critical_checks
 from ..utils import get_device_cache_key
 from . import DeviceMonitoringTestCase, DeviceMonitoringTransactionTestcase
 
 DeviceMonitoring = load_model('device_monitoring', 'DeviceMonitoring')
 DeviceData = load_model('device_monitoring', 'DeviceData')
 Device = load_model('config', 'Device')
+Check = load_model('check', 'Check')
 
 
 class TestRecovery(DeviceMonitoringTestCase):
@@ -57,18 +58,24 @@ def test_status_set_ok(self):
         """Tests device status is set to ok if no related checks present"""
         dm = self._create_device_monitoring()
         dm.update_status('critical')
-        trigger_device_checks.delay(dm.device.pk)
+        trigger_device_critical_checks.delay(dm.device.pk)
         dm.refresh_from_db()
         self.assertEqual(dm.status, 'ok')
 
     def test_status_set_critical(self):
         """Tests device status is set to critical if no related checks present and recovery=False is passed"""
         dm = self._create_device_monitoring()
         dm.update_status('critical')
-        trigger_device_checks.delay(dm.device.pk, recovery=False)
+        trigger_device_critical_checks.delay(dm.device.pk, recovery=False)
         dm.refresh_from_db()
         self.assertEqual(dm.status, 'critical')
 
+    @patch('openwisp_monitoring.device.tasks.trigger_device_critical_checks')
+    def test_trigger_device_checks(self, mocked_task):
+        dm = self._create_device_monitoring()
+        trigger_device_checks.delay(dm.device.pk)
+        mocked_task.assert_called_once_with(dm.device.pk, True)
+
 
 class TestRecoveryTransaction(DeviceMonitoringTransactionTestcase):
     @patch('openwisp_monitoring.device.tasks.perform_check.delay')
@@ -111,6 +118,15 @@ def test_metrics_received_trigger_device_recovery_checks(self, mocked_task):
                 content_type='application/json',
             )
             self.assertEqual(response.status_code, 200)
-            self.assertEqual(mocked_task.call_count, 4)
+            critical_checks = device_monitoring.get_critical_checks()
+            self.assertEqual(mocked_task.call_count, len(critical_checks))
+            # Verify that only critical checks are triggered
+            # Get the check types from the mocked task calls
+            triggered_check_types = list(
+                Check.objects.filter(
+                    id__in=[call[0][0] for call in mocked_task.call_args_list]
+                ).values_list('check_type', flat=True)
+            )
+            self.assertListEqual(sorted(triggered_check_types), sorted(critical_checks))
             device_monitoring.refresh_from_db()
             self.assertEqual(device_monitoring.status, 'problem')

openwisp_monitoring/device/tests/test_transactions.py

@@ -10,7 +10,7 @@
 
 from ...check.classes import Ping
 from ...check.tests import _FPING_REACHABLE, _FPING_UNREACHABLE
-from ..tasks import trigger_device_checks
+from ..tasks import trigger_device_critical_checks
 from . import DeviceMonitoringTransactionTestcase
 
 Check = load_model('check', 'Check')
@@ -62,7 +62,7 @@ def test_trigger_device_recovery_task_regression(
         dm = self._create_device_monitoring()
         dm.device.management_ip = None
         dm.device.save()
-        trigger_device_checks.delay(dm.device.pk)
+        trigger_device_critical_checks.delay(dm.device.pk)
         self.assertTrue(Check.objects.exists())
         # we expect update_status() to be called twice (by the check)
         # and not a third time directly by our code