[fix] Fixed perennial config modified state #1113

Fixes #1113

Author: pandafy

openwisp_controller/config/base/config.py

@@ -3,6 +3,7 @@
 import re
 from collections import defaultdict
 
+from cache_memoize import cache_memoize
 from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError
 from django.db import models, transaction
 from django.utils.translation import gettext_lazy as _
@@ -100,8 +101,15 @@ class AbstractConfig(ChecksumCacheMixin, BaseConfig):
         load_kwargs={"object_pairs_hook": collections.OrderedDict},
         dump_kwargs={"indent": 4},
     )
+    checksum_db = models.CharField(
+        _("configuration checksum"),
+        max_length=32,
+        blank=True,
+        null=True,
+        help_text=_("Checksum of the generated configuration."),
+    )
 
-    _CHECKSUM_CACHE_TIMEOUT = 60 * 60 * 24 * 30  # 10 days
+    _CHECKSUM_CACHE_TIMEOUT = ChecksumCacheMixin._CHECKSUM_CACHE_TIMEOUT
     _config_context_functions = list()
     _old_backend = None
 
@@ -151,6 +159,32 @@ def key(self):
         """
         return self.device.key
 
+    @cache_memoize(
+        timeout=ChecksumCacheMixin._CHECKSUM_CACHE_TIMEOUT,
+        args_rewrite=get_cached_args_rewrite,
+    )
+    def get_cached_checksum(self):
+        """
+        Returns the cached configuration checksum.
+
+        Unlike `ChecksumCacheMixin.get_cached_checksum`, this returns the
+        value from the `checksum_db` field instead of recalculating it.
+        """
+        self.refresh_from_db(fields=["checksum_db"])
+        return self.checksum_db
+
+    @classmethod
+    def bulk_invalidate_get_cached_checksum(cls, query_params):
+        """
+        Bulk invalidates cached configuration checksums for matching instances
+
+        Sets status to modified if the configuration of the instance has changed.
+        """
+        for instance in cls.objects.only("id").filter(**query_params).iterator():
+            has_changed = instance.update_status_if_checksum_changed()
+            if has_changed:
+                instance.get_cached_checksum.invalidate(instance)
+
     @classmethod
     def get_template_model(cls):
         return cls.templates.rel.model
@@ -276,9 +310,9 @@ def templates_changed(cls, action, instance, **kwargs):
             if not instance._just_created:
                 # sends only config modified signal
                 instance._send_config_modified_signal(action="m2m_templates_changed")
-            if instance.status != "modified":
-                # sends both status modified and config modified signals
-                instance.set_status_modified(send_config_modified_signal=False)
+            instance.update_status_if_checksum_changed(
+                send_config_modified_signal=False
+            )
 
     @classmethod
     def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
@@ -443,7 +477,7 @@ def certificate_updated(cls, instance, created, **kwargs):
         except ObjectDoesNotExist:
             return
         else:
-            transaction.on_commit(config.set_status_modified)
+            transaction.on_commit(config.update_status_if_checksum_changed)
 
     @classmethod
     def register_context_function(cls, func):
@@ -541,6 +575,8 @@ def save(self, *args, **kwargs):
         # check if config has been modified (so we can emit signals)
         if not created:
             self._check_changes()
+        self._invalidate_backend_instance_cache()
+        self.checksum_db = self.checksum
         self._just_created = created
         result = super().save(*args, **kwargs)
         # add default templates if config has just been created
@@ -584,8 +620,7 @@ def _check_changes(self):
         if self.backend != current.backend:
             # storing old backend to send backend change signal after save
             self._old_backend = current.backend
-        if hasattr(self, "backend_instance"):
-            del self.backend_instance
+        self._invalidate_backend_instance_cache()
         if self.checksum != current.checksum:
             if self.status != "modified":
                 self.set_status_modified(save=False)
@@ -594,6 +629,52 @@ def _check_changes(self):
                 # regardless of the current status
                 self._send_config_modified_after_save = True
 
+    def update_status_if_checksum_changed(
+        self, save=True, send_config_modified_signal=True
+    ):
+        """
+        Updates the instance status if its checksum has changed.
+
+        Returns:
+            bool: True if the checksum changed and an update was applied,
+            False otherwise.
+        """
+        checksum_changed = self._should_update_status_based_on_checksum()
+        if checksum_changed:
+            self.checksum_db = self.checksum
+            if self.status != "modified":
+                self.set_status_modified(
+                    save=save,
+                    send_config_modified_signal=send_config_modified_signal,
+                    extra_update_fields=["checksum_db"],
+                )
+            else:
+                # Instead of calling the "save()" method, which would
+                # trigger various signals and checks, we directly update
+                # the "checksum_db" field in the database.
+                self._meta.model.objects.filter(pk=self.pk).update(
+                    checksum_db=self.checksum_db
+                )
+        return checksum_changed
+
+    def _should_update_status_based_on_checksum(self):
+        """
+        Determines whether the config status should be updated based on
+        checksum comparison.
+
+        Returns True if:
+        - No checksum_db exists (first time)
+        - Current checksum differs from checksum_db
+
+        Returns False if:
+        - Current checksum is the same as checksum_db
+        """
+        if self.checksum_db is None:
+            # First time or no database checksum, should update
+            return True
+        self._invalidate_backend_instance_cache()
+        return self.checksum_db != self.checksum
+
     def _send_config_modified_signal(self, action):
         """
         Emits ``config_modified`` signal.
@@ -668,10 +749,12 @@ def _set_status(self, status, save=True, reason=None, extra_update_fields=None):
         if save:
             self.save(update_fields=update_fields)
 
-    def set_status_modified(self, save=True, send_config_modified_signal=True):
+    def set_status_modified(
+        self, save=True, send_config_modified_signal=True, extra_update_fields=None
+    ):
         if send_config_modified_signal:
             self._send_config_modified_after_save = True
-        self._set_status("modified", save)
+        self._set_status("modified", save, extra_update_fields)
 
     def set_status_applied(self, save=True):
         self._set_status("applied", save)

openwisp_controller/config/base/template.py

@@ -162,20 +162,18 @@ def _update_related_config_status(self):
         # be executed via transaction.on_commit
         # is executed after the whole block
         with transaction.atomic():
-            changing_status = list(
-                self.config_relations.exclude(status="modified").values_list(
-                    "pk", flat=True
+            for config in (
+                self.config_relations.prefetch_related(
+                    "vpnclient_set",
+                    "templates",
+                )
+                .select_related("device", "device__organization__config_settings")
+                .iterator(chunk_size=1000)
+            ):
+                config.update_status_if_checksum_changed(
+                    send_config_modified_signal=False
                 )
-            )
-            # flag all related configs as modified with 1 update query
-            self.config_relations.exclude(status="modified").update(status="modified")
-            # the following loop should not take too long
-            for config in self.config_relations.select_related("device").iterator():
-                # config modified signal sent regardless
                 config._send_config_modified_signal(action="related_template_changed")
-                # config status changed signal sent only if status changed
-                if config.pk in changing_status:
-                    config._send_config_status_changed_signal()
 
     def _auto_add_to_existing_configs(self):
         """

openwisp_controller/config/migrations/0061_config_checksum_db.py

@@ -0,0 +1,46 @@
+# Generated by Django for issue #1113 optimization
+
+from django.db import migrations, models
+
+
+def populate_checksum_db(apps, schema_editor):
+    """
+    Populate checksum_db field with current checksum values
+    for existing Config objects.
+    """
+    Config = apps.get_model("config", "Config")
+    batch = []
+    batch_size = 500
+    qs = Config.objects.filter(checksum_db__isnull=True).iterator()
+    for config in qs:
+        config.checksum_db = config.checksum
+        batch.append(config)
+        if len(batch) >= batch_size:
+            Config.objects.bulk_update(batch, ["checksum_db"])
+            batch = []
+    if batch:
+        Config.objects.bulk_update(batch, ["checksum_db"])
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("config", "0060_cleanup_api_task_notification_types"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="config",
+            name="checksum_db",
+            field=models.CharField(
+                blank=True,
+                help_text=("Checksum of the generated configuration."),
+                max_length=32,
+                null=True,
+                verbose_name="configuration checksum",
+            ),
+        ),
+        migrations.RunPython(
+            populate_checksum_db,
+            reverse_code=migrations.RunPython.noop,
+        ),
+    ]

openwisp_controller/config/tests/test_config.py

@@ -232,7 +232,7 @@ def test_get_cached_checksum(self):
             with patch.object(base_config_logger, "debug") as mocked_debug:
                 c.get_cached_checksum.invalidate(c)
                 self.assertEqual(len(c.get_cached_checksum()), 32)
-                mocked_debug.assert_called_once()
+                mocked_debug.assert_not_called()
 
         with self.subTest(
             "ensure fresh checksum is NOT calculated when cache is present"
@@ -250,7 +250,7 @@ def test_get_cached_checksum(self):
                 del c.backend_instance
                 self.assertNotEqual(c.checksum, old_checksum)
                 self.assertEqual(c.get_cached_checksum(), c.checksum)
-                mocked_debug.assert_called_once()
+                mocked_debug.assert_not_called()
 
         with self.subTest("test cache invalidation when config templates are changed"):
             with patch.object(base_config_logger, "debug") as mocked_debug:
@@ -260,7 +260,7 @@ def test_get_cached_checksum(self):
                 del c.backend_instance
                 self.assertNotEqual(c.checksum, old_checksum)
                 self.assertEqual(c.get_cached_checksum(), c.checksum)
-                mocked_debug.assert_called_once()
+                mocked_debug.assert_not_called()
 
     def test_backend_import_error(self):
         """
@@ -962,7 +962,7 @@ def test_certificate_renew_invalidates_checksum_cache(self):
             vpnclient_cert.renew()
             # An additional call from cache invalidation of
             # DeviceGroupCommonName View
-            self.assertEqual(mocked_delete.call_count, 3)
+            self.assertEqual(mocked_delete.call_count, 2)
             del config.backend_instance
             self.assertNotEqual(config.get_cached_checksum(), old_checksum)
             config.refresh_from_db()

openwisp_controller/config/tests/test_controller.py

@@ -181,7 +181,7 @@ def test_get_cached_checksum(self):
         url = reverse("controller:device_checksum", args=[d.pk])
 
         with self.subTest("first request does not return value from cache"):
-            with self.assertNumQueries(4):
+            with self.assertNumQueries(3):
                 with patch.object(
                     controller_views_logger, "debug"
                 ) as mocked_view_debug:
@@ -191,7 +191,7 @@ def test_get_cached_checksum(self):
                         self.client.get(
                             url, {"key": d.key, "management_ip": "10.0.0.2"}
                         )
-                        self.assertEqual(mocked_config_debug.call_count, 1)
+                        self.assertEqual(mocked_config_debug.call_count, 0)
                     self.assertEqual(mocked_view_debug.call_count, 1)
 
         with self.subTest("update_last_ip updates the cache"):
@@ -240,7 +240,7 @@ def test_get_cached_checksum(self):
                 del d.config.backend_instance
                 self.assertNotEqual(d.config.checksum, old_checksum)
                 self.assertEqual(d.config.get_cached_checksum(), d.config.checksum)
-                mocked_debug.assert_called_once()
+                mocked_debug.assert_not_called()
 
     def test_device_checksum_requested_signal_is_emitted(self):
         d = self._create_device_config()
@@ -1394,12 +1394,12 @@ def test_ip_fields_not_duplicated(self):
         c2 = self._create_config(device=d2)
         org2 = self._create_org(name="org2", shared_secret="123456")
         c3 = self._create_config(organization=org2)
-        with self.assertNumQueries(7):
+        with self.assertNumQueries(6):
             self.client.get(
                 reverse("controller:device_checksum", args=[c3.device.pk]),
                 {"key": c3.device.key, "management_ip": "192.168.1.99"},
             )
-        with self.assertNumQueries(7):
+        with self.assertNumQueries(6):
             self.client.get(
                 reverse("controller:device_checksum", args=[c1.device.pk]),
                 {"key": c1.device.key, "management_ip": "192.168.1.99"},
@@ -1441,14 +1441,14 @@ def test_organization_shares_management_ip_address_space(self):
         org1_config = self._create_config(organization=org1)
         org2 = self._create_org(name="org2", shared_secret="org2")
         org2_config = self._create_config(organization=org2)
-        with self.assertNumQueries(7):
+        with self.assertNumQueries(6):
             self.client.get(
                 reverse("controller:device_checksum", args=[org1_config.device_id]),
                 {"key": org1_config.device.key, "management_ip": "192.168.1.99"},
             )
         # Device from another organization sends conflicting management IP
         # Extra queries due to conflict resolution
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(8):
             self.client.get(
                 reverse("controller:device_checksum", args=[org2_config.device_id]),
                 {"key": org2_config.device.key, "management_ip": "192.168.1.99"},