[fix] Fixed perennial config modified state #1113

Fixes #1113

Author: pandafy

openwisp_controller/config/base/config.py

@@ -100,6 +100,15 @@ class AbstractConfig(ChecksumCacheMixin, BaseConfig):
         load_kwargs={"object_pairs_hook": collections.OrderedDict},
         dump_kwargs={"indent": 4},
     )
+    checksum_db = models.CharField(
+        _("database checksum"),
+        max_length=32,
+        blank=True,
+        null=True,
+        help_text=_(
+            "Last known checksum stored in database for optimization purposes."
+        ),
+    )
 
     _CHECKSUM_CACHE_TIMEOUT = 60 * 60 * 24 * 30  # 10 days
     _config_context_functions = list()
@@ -265,7 +274,15 @@ def templates_changed(cls, action, instance, **kwargs):
         there fore we need to ignore it to avoid emitting signals twice
         """
         # execute only after a config has been saved or deleted
-        if action not in ["post_add", "post_remove"] or instance._state.adding:
+        if (
+            action not in ["post_add", "post_remove", "post_clear"]
+            or instance._state.adding
+        ):
+            return
+        if action == "post_clear":
+            if hasattr(instance, "backend_instance"):
+                del instance.backend_instance
+            cls.objects.filter(pk=instance.pk).update(checksum_db=instance.checksum)
             return
         # use atomic to ensure any code bound to
         # be executed via transaction.on_commit
@@ -276,9 +293,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.check_and_update_status_if_changed(
+                send_config_modified_signal=False
+            )
 
     @classmethod
     def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
@@ -538,9 +555,11 @@ 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:
             self._check_changes()
+        if hasattr(self, "backend_instance"):
+            del self.backend_instance
+        self.checksum_db = self.checksum
         self._just_created = created
         result = super().save(*args, **kwargs)
         # add default templates if config has just been created
@@ -579,7 +598,10 @@ def is_deactivated(self):
 
     def _check_changes(self):
         current = self._meta.model.objects.only(
-            "backend", "config", "context", "status"
+            "backend",
+            "config",
+            "context",
+            "status",
         ).get(pk=self.pk)
         if self.backend != current.backend:
             # storing old backend to send backend change signal after save
@@ -594,6 +616,48 @@ def _check_changes(self):
                 # regardless of the current status
                 self._send_config_modified_after_save = True
 
+    def check_and_update_status_if_changed(
+        self, save=True, send_config_modified_signal=True
+    ):
+        """
+        Helper method to check if configuration has changed based on checksum
+        comparison and update status accordingly.
+        """
+
+        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:
+                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
+        if hasattr(self, "backend_instance"):
+            delattr(self, "backend_instance")
+        return self.checksum_db != self.checksum
+
     def _send_config_modified_signal(self, action):
         """
         Emits ``config_modified`` signal.
@@ -668,10 +732,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.check_and_update_status_if_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,49 @@
+# 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=(
+                    "Last known checksum stored in database for "
+                    "optimization purposes."
+                ),
+                max_length=32,
+                null=True,
+                verbose_name="database checksum",
+            ),
+        ),
+        migrations.RunPython(
+            populate_checksum_db,
+            reverse_code=migrations.RunPython.noop,
+        ),
+    ]

openwisp_controller/config/tests/test_template.py

@@ -586,7 +586,7 @@ def test_config_status_modified_after_change(self):
             with catch_signal(config_status_changed) as handler:
                 t.config["interfaces"][0]["name"] = "eth2"
                 t.full_clean()
-                with self.assertNumQueries(10):
+                with self.assertNumQueries(13):
                     t.save()
                 c.refresh_from_db()
                 handler.assert_not_called()
@@ -596,7 +596,9 @@ def test_config_modified_signal(self):
         conf = self._create_config(device=self._create_device(name="test-status"))
         template1 = self._create_template(name="t1")
         template2 = self._create_template(
-            name="t2", organization=conf.device.organization
+            name="t2",
+            organization=conf.device.organization,
+            config={"interfaces": [{"name": "eth1", "type": "ethernet"}]},
         )
         self.assertEqual(conf.status, "modified")
         # refresh instance to reset _just_created attribute
@@ -710,6 +712,33 @@ def test_config_modified_signal(self):
                 )
                 self.assertEqual(conf.status, "modified")
 
+    def test_config_status_after_template_variables_change(self):
+        template = self._create_template(
+            default_values={"type": "ethernet"},
+            config={"interfaces": [{"name": "eth0", "type": "{{ type }}"}]},
+            default=True,
+        )
+        config = self._create_config(
+            device=self._create_device(name="test-status"), context={"type": "virtual"}
+        )
+        config.set_status_applied()
+        config.refresh_from_db()
+        self.assertEqual(config.status, "applied")
+
+        if hasattr(config, "backend_instance"):
+            del config.backend_instance
+
+        template.default_values["type"] = "virtual"
+        template.full_clean()
+        template.save()
+
+        # The configuration status should not change because the device
+        # is overriding Template.default_value
+        config.refresh_from_db()
+        if hasattr(config, "backend_instance"):
+            del config.backend_instance
+        self.assertEqual(config.status, "applied")
+
     @mock.patch.object(update_template_related_config_status, "delay")
     def test_task_called(self, mocked_task):
         with self.subTest("task not called when template is created"):

tests/openwisp2/sample_config/migrations/0001_initial.py

@@ -129,6 +129,19 @@ class Migration(migrations.Migration):
                     ),
                 ),
                 ("details", models.CharField(blank=True, max_length=64, null=True)),
+                (
+                    "checksum_db",
+                    models.CharField(
+                        blank=True,
+                        help_text=(
+                            "Last known checksum stored in database for "
+                            "optimization purposes."
+                        ),
+                        max_length=32,
+                        null=True,
+                        verbose_name="database checksum",
+                    ),
+                ),
             ],
             options={
                 "verbose_name": "configuration",