[change/fix] Store checksum in DB (fixed perennial modified status) #1113

The config checksum is now stored in the database
and the config status is changed to modified only
when the checksum changes.

Closes #1113

Author: pandafy

openwisp_controller/config/apps.py

@@ -23,8 +23,6 @@
 from .signals import (
     config_backend_changed,
     config_deactivated,
-    config_deactivating,
-    config_modified,
     device_group_changed,
     device_name_changed,
     group_templates_changed,
@@ -285,14 +283,6 @@ def enable_cache_invalidation(self):
             DeviceChecksumView.invalidate_get_device_cache_on_config_deactivated,
             dispatch_uid="config_deactivated_invalidate_get_device_cache",
         )
-        config_deactivating.connect(
-            DeviceChecksumView.invalidate_checksum_cache,
-            dispatch_uid="config_deactivated_invalidate_get_device_cache",
-        )
-        config_modified.connect(
-            DeviceChecksumView.invalidate_checksum_cache,
-            dispatch_uid="invalidate_checksum_cache",
-        )
         # VPN cache invalidation
         post_save.connect(
             GetVpnView.invalidate_get_vpn_cache,

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.invalidate_checksum_cache()
+
     @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):
@@ -539,7 +573,9 @@ def clean(self):
     def save(self, *args, **kwargs):
         created = self._state.adding
         # check if config has been modified (so we can emit signals)
-        if not created:
+        if created:
+            self.checksum_db = self.checksum
+        else:
             self._check_changes()
         self._just_created = created
         result = super().save(*args, **kwargs)
@@ -584,15 +620,63 @@ 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
-        if self.checksum != current.checksum:
+        self.update_status_if_checksum_changed(
+            save=False,
+        )
+
+    def update_status_if_checksum_changed(
+        self, save=True, update_checksum_db=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._has_configuration_checksum_changed()
+        if checksum_changed:
+            self.checksum_db = self.checksum
             if self.status != "modified":
-                self.set_status_modified(save=False)
+                self.set_status_modified(
+                    save=save,
+                    send_config_modified_signal=send_config_modified_signal,
+                    extra_update_fields=["checksum_db"],
+                )
             else:
-                # config modified signal is always sent
-                # regardless of the current status
-                self._send_config_modified_after_save = True
+                if update_checksum_db:
+                    self._update_checksum_db(new_checksum=self.checksum_db)
+                if send_config_modified_signal:
+                    self._send_config_modified_after_save = True
+            self.invalidate_checksum_cache()
+        return checksum_changed
+
+    def _has_configuration_checksum_changed(self):
+        """
+        Determines whether the config checksum has changed
+
+        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 _update_checksum_db(self, new_checksum=None):
+        """
+        Updates checksum_db field in the database
+
+        It does not call save() to avoid sending signals
+        and updating other fields.
+        """
+        new_checksum = new_checksum or self.checksum
+        self._meta.model.objects.filter(pk=self.pk).update(checksum_db=new_checksum)
 
     def _send_config_modified_signal(self, action):
         """
@@ -668,10 +752,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=extra_update_fields)
 
     def set_status_applied(self, save=True):
         self._set_status("applied", save)
@@ -697,12 +783,22 @@ def deactivate(self):
         """
         # Invalidate cached property before checking checksum.
         self._invalidate_backend_instance_cache()
-        old_checksum = self.checksum
+        old_checksum = self.checksum_db
+        # Don't alter the order of the following steps.
+        # We need to set the status to deactivating before clearing the templates
+        # otherwise, the "enforce_required_templates" and "add_default_templates"
+        # methods would re-add required/default templates.
+        # The "templates_changed" receiver skips post_clear action. Thus,
+        # we need to update the checksum_db field manually and invalidate
+        # the cache.
         self.config = {}
-        self.set_status_deactivating()
+        self.set_status_deactivating(save=False)
         self.templates.clear()
-        del self.backend_instance
-        if old_checksum == self.checksum:
+        self._invalidate_backend_instance_cache()
+        self.checksum_db = self.checksum
+        self.invalidate_checksum_cache()
+        self.save()
+        if old_checksum == self.checksum_db:
             # Accelerate deactivation if the configuration remains
             # unchanged (i.e. empty configuration)
             self.set_status_deactivated()

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/controller/views.py

@@ -190,14 +190,6 @@ def invalidate_get_device_cache_on_config_deactivated(cls, instance, **kwargs):
         """
         cls.invalidate_get_device_cache(instance=instance.device, **kwargs)
 
-    @classmethod
-    def invalidate_checksum_cache(cls, instance, device, **kwargs):
-        """
-        Called from signal receiver which performs cache invalidation
-        """
-        instance.get_cached_checksum.invalidate(instance)
-        logger.debug(f"invalidated checksum cache for device ID {device.pk}")
-
 
 class DeviceDownloadConfigView(GetDeviceView):
     """

openwisp_controller/config/migrations/0061_config_checksum_db.py

@@ -0,0 +1,44 @@
+# Generated by Django for issue #1113 optimization
+
+from django.db import migrations, models
+from swapper import load_model
+
+
+def populate_checksum_db(apps, schema_editor):
+    """
+    Populate checksum_db field with current checksum values
+    for existing Config objects.
+    """
+    Config = load_model("config", "Config")
+    qs = (
+        Config.objects.prefetch_related("vpnclient_set", "templates")
+        .select_related("device", "device__organization__config_settings")
+        .filter(checksum_db__isnull=True)
+        .iterator(chunk_size=100)
+    )
+    for config in qs:
+        config.update_status_if_checksum_changed()
+
+
+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"
@@ -262,6 +262,15 @@ def test_get_cached_checksum(self):
                 self.assertEqual(c.get_cached_checksum(), c.checksum)
                 mocked_debug.assert_called_once()
 
+        with self.subTest("cache invalidation works when config is deactivated"):
+            with patch.object(base_config_logger, "debug") as mocked_debug:
+                old_checksum = c.checksum
+                c.deactivate()
+                del c.backend_instance
+                self.assertNotEqual(c.checksum, old_checksum)
+                self.assertEqual(c.get_cached_checksum(), c.checksum)
+                mocked_debug.assert_called_once()
+
     def test_backend_import_error(self):
         """
         see issue #5
@@ -828,8 +837,15 @@ def test_config_modified_sent(self):
 
     def test_check_changes_query(self):
         config = self._create_config(organization=self._get_org())
-        with self.assertNumQueries(10):
-            config._check_changes()
+        with self.subTest("No changes made to the config object"):
+            with self.assertNumQueries(3):
+                config._check_changes()
+
+        with self.subTest("Changes made to the config object"):
+            config.templates.add(self._create_template())
+            config.config = {"general": {"description": "test"}}
+            with self.assertNumQueries(4):
+                config._check_changes()
 
     def test_config_get_system_context(self):
         config = self._create_config(