Преглед изворни кода

Fixes #22812: Batch child-script job deletion when deleting a ScriptModule

Deleting a Script via the UI is only possible by deleting its parent ScriptModule
(no Script delete view exists). That cascades to the child Script rows, and the
collector materialized every one of those Scripts' jobs — the memory blowup, which
scales with jobs-per-script. JobsMixin.delete() only batched the deleted object's own
jobs, and a ScriptModule has none; the jobs live on its child Scripts.

Extract the chunked job-deletion loop from JobsMixin.delete() into a shared
batch_delete_jobs() helper, and add a ScriptModule.delete() override that batch-deletes
its child Scripts' jobs (in a single queryset keyed on the script PKs, no per-script
loop) before delegating to the cascade. This bounds peak memory to one batch regardless
of how many jobs the module's scripts hold.
Jason Novinger пре 2 недеља
родитељ
комит
f2923f4ce4

+ 25 - 2
netbox/extras/models/scripts.py

@@ -3,7 +3,8 @@ import logging
 from functools import cached_property
 
 from django.contrib.contenttypes.fields import GenericRelation
-from django.db import models
+from django.contrib.contenttypes.models import ContentType
+from django.db import models, router, transaction
 from django.db.models import Q
 from django.urls import reverse
 from django.utils.translation import gettext_lazy as _
@@ -11,7 +12,7 @@ from django.utils.translation import gettext_lazy as _
 from core.choices import ManagedFileRootPathChoices
 from core.models import ManagedFile
 from extras.utils import is_script
-from netbox.models.features import EventRulesMixin, JobsMixin
+from netbox.models.features import EventRulesMixin, JobsMixin, batch_delete_jobs
 from utilities.querysets import RestrictedQuerySet
 
 from .mixins import PythonModuleMixin
@@ -119,6 +120,28 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile):
     def __str__(self):
         return self.python_name
 
+    def delete(self, *args, **kwargs):
+        # Job is imported here rather than at module level to avoid a circular import
+        # (core.models.jobs -> core.signals -> extras.events -> extras.models -> this module).
+        from core.models import Job
+
+        # Deleting a ScriptModule cascades (via the Script.module FK) to its child Scripts, and
+        # Django's collector would materialize every one of those Scripts' Jobs to delete them.
+        # A module's scripts can accumulate thousands of jobs, exhausting memory. Batch-delete
+        # the child Scripts' jobs up front, in a single queryset (no per-script loop), before
+        # delegating to the cascade. Wrapped in a transaction so a failure in the parent delete
+        # rolls these deletions back as well. See #22812.
+        using = router.db_for_write(self.__class__, instance=self)
+        with transaction.atomic(using=using):
+            script_type = ContentType.objects.get_for_model(Script, for_concrete_model=False)
+            child_jobs = Job.objects.filter(
+                object_type=script_type,
+                object_id__in=self.scripts.values_list('pk', flat=True),
+            )
+            batch_delete_jobs(child_jobs)
+            return super().delete(*args, **kwargs)
+    delete.alters_data = True
+
     @property
     def ordered_scripts(self):
         script_objects = {s.name: s for s in self.scripts.all()}

+ 24 - 0
netbox/extras/tests/test_scripts_deletion.py

@@ -90,6 +90,30 @@ class ScriptDeletionTestCase(TestCase):
         self.assertFalse(Script.objects.filter(pk=script.pk).exists())
         self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0)
 
+    def test_delete_scriptmodule_batches_child_script_jobs(self):
+        # The reporter's actual path: a script is only removable via the UI by deleting its
+        # ScriptModule. The module's delete must batch the child Script's jobs.
+        module, script = self._create_script()
+        self._add_jobs(script, 5)
+
+        job_delete_calls = []
+        original_delete = QuerySet.delete
+
+        def counting_delete(qs, *args, **kwargs):
+            if qs.model is Job:
+                job_delete_calls.append(len(qs))
+            return original_delete(qs, *args, **kwargs)
+
+        with mock.patch('netbox.models.features.JOB_DELETE_BATCH_SIZE', 2):
+            with mock.patch.object(QuerySet, 'delete', counting_delete):
+                module.delete()
+
+        # 5 child-script jobs at a batch size of 2 => three batched deletes (2, 2, 1). The module
+        # has no jobs of its own, so JobsMixin.delete adds no further Job deletes.
+        self.assertEqual(job_delete_calls, [2, 2, 1])
+        self.assertFalse(Script.objects.filter(pk=script.pk).exists())
+        self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0)
+
     def test_delete_datasource_deletes_jobs(self):
         datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test')
         self._add_jobs(datasource, 100)

+ 22 - 12
netbox/netbox/models/features.py

@@ -43,6 +43,7 @@ __all__ = (
     'NotificationsMixin',
     'SyncedDataMixin',
     'TagsMixin',
+    'batch_delete_jobs',
     'get_model_features',
     'has_feature',
     'model_is_public',
@@ -449,6 +450,23 @@ class NotificationsMixin(models.Model):
         abstract = True
 
 
+def batch_delete_jobs(job_queryset):
+    """
+    Delete the Jobs in `job_queryset` in JOB_DELETE_BATCH_SIZE chunks, so the caller never has
+    to load thousands of Job rows (each carrying potentially large data/log_entries payloads)
+    into memory at once. Callers are responsible for wrapping this in a transaction. As with the
+    prior cascade behavior, this bulk delete does not invoke Job.delete() and therefore does not
+    cancel the backing RQ job. See #22812.
+    """
+    from core.models import Job
+
+    job_pks = job_queryset.order_by('pk').values_list('pk', flat=True)
+    # Re-slice the queryset each iteration: it re-queries after each batch delete, so the
+    # remaining set shrinks and the loop terminates (do not hoist this into a cursor).
+    while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]):
+        Job.objects.filter(pk__in=pks).delete()
+
+
 class JobsMixin(models.Model):
     """
     Enables support for job results.
@@ -464,20 +482,12 @@ class JobsMixin(models.Model):
         abstract = True
 
     def delete(self, *args, **kwargs):
-        from core.models import Job
-
-        # Delete associated jobs in batches so the cascade never has to load thousands of
-        # Job rows (each carrying potentially large data/log_entries payloads) into memory
-        # at once. Wrapped in a transaction so that a failure in the parent delete rolls the
-        # job deletions back as well. As with the prior cascade behavior, this bulk delete does
-        # not invoke Job.delete() and therefore does not cancel the backing RQ job. See #22812.
+        # Delete associated jobs in batches so the cascade never has to load thousands of Job
+        # rows into memory at once. Wrapped in a transaction so that a failure in the parent
+        # delete rolls the job deletions back as well. See #22812.
         using = router.db_for_write(self.__class__, instance=self)
         with transaction.atomic(using=using):
-            job_pks = self.jobs.order_by('pk').values_list('pk', flat=True)
-            # Re-slice the queryset each iteration: it re-queries after each batch delete, so the
-            # remaining set shrinks and the loop terminates (do not hoist this into a cursor).
-            while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]):
-                Job.objects.filter(pk__in=pks).delete()
+            batch_delete_jobs(self.jobs)
             return super().delete(*args, **kwargs)
     delete.alters_data = True