Merge pull request #22867 from netbox-community/22812-script-delete-memory-exhaustion
Closes #22812: Avoid loading all jobs into memory when deleting a JobsMixin object
This commit is contained in:
bctiemann
GitHub
commit
a08d9f13fc
|
|
@ -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,29 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile):
|
|||
def __str__(self):
|
||||
return self.python_name
|
||||
|
||||
def delete(self, using=None, *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. The transaction rolls the job deletions back if the parent
|
||||
# delete fails; note it does not cover ManagedFile.delete removing the file from disk,
|
||||
# which happens before the DB delete and is not transactional. See #22812.
|
||||
using = using or 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.using(using).filter(
|
||||
object_type=script_type,
|
||||
object_id__in=self.scripts.values_list('pk', flat=True),
|
||||
)
|
||||
batch_delete_jobs(child_jobs)
|
||||
return super().delete(using, *args, **kwargs)
|
||||
delete.alters_data = True
|
||||
|
||||
@property
|
||||
def ordered_scripts(self):
|
||||
script_objects = {s.name: s for s in self.scripts.all()}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,306 @@
|
|||
import uuid
|
||||
from unittest import mock
|
||||
|
||||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.db import router
|
||||
from django.db.models import QuerySet
|
||||
from django.test import TestCase, override_settings
|
||||
from django.urls import reverse
|
||||
|
||||
from core.choices import JobStatusChoices, ManagedFileRootPathChoices
|
||||
from core.models import DataSource, Job
|
||||
from extras.models import Script, ScriptModule
|
||||
from extras.validators import CustomValidator
|
||||
from netbox.models.deletion import ConfirmCollector, CountOnly
|
||||
from utilities.exceptions import AbortRequest
|
||||
from utilities.testing import TestCase as ViewTestCase
|
||||
|
||||
|
||||
class ScriptDeletionTestCase(TestCase):
|
||||
"""
|
||||
Regression tests for #22812: deleting a JobsMixin object (Script, ScriptModule, DataSource)
|
||||
with many associated Jobs must not load every Job into memory at once.
|
||||
"""
|
||||
@classmethod
|
||||
def setUpTestData(cls):
|
||||
cls.script_ct = ContentType.objects.get_for_model(Script, for_concrete_model=False)
|
||||
|
||||
def _create_module(self):
|
||||
return ScriptModule.objects.create(
|
||||
file_root=ManagedFileRootPathChoices.SCRIPTS,
|
||||
file_path=f'test_{uuid.uuid4().hex[:8]}.py',
|
||||
)
|
||||
|
||||
def _create_script(self, module=None):
|
||||
module = module or self._create_module()
|
||||
script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}')
|
||||
return module, script
|
||||
|
||||
def _add_jobs(self, obj, count, object_type=None):
|
||||
object_type = object_type or ContentType.objects.get_for_model(type(obj), for_concrete_model=False)
|
||||
Job.objects.bulk_create([
|
||||
Job(
|
||||
object_type=object_type,
|
||||
object_id=obj.pk,
|
||||
name='testjob',
|
||||
status=JobStatusChoices.STATUS_COMPLETED,
|
||||
job_id=uuid.uuid4(),
|
||||
data={'output': 'x' * 50},
|
||||
)
|
||||
for _ in range(count)
|
||||
])
|
||||
|
||||
def test_delete_script_deletes_all_jobs(self):
|
||||
_, script = self._create_script()
|
||||
self._add_jobs(script, 2500)
|
||||
self.assertEqual(script.jobs.count(), 2500)
|
||||
|
||||
script.delete()
|
||||
|
||||
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_script_batches_jobs(self):
|
||||
_, 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):
|
||||
script.delete()
|
||||
|
||||
# 5 jobs at a batch size of 2 => three batched deletes (2, 2, 1)
|
||||
self.assertEqual(job_delete_calls, [2, 2, 1])
|
||||
self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0)
|
||||
|
||||
def test_delete_scriptmodule_cascades_to_scripts_and_jobs(self):
|
||||
module, script = self._create_script()
|
||||
self._add_jobs(script, 100)
|
||||
|
||||
module.delete()
|
||||
|
||||
self.assertFalse(ScriptModule.objects.filter(pk=module.pk).exists())
|
||||
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)
|
||||
ds_ct = ContentType.objects.get_for_model(DataSource, for_concrete_model=False)
|
||||
self.assertEqual(Job.objects.filter(object_type=ds_ct, object_id=datasource.pk).count(), 100)
|
||||
|
||||
datasource.delete()
|
||||
|
||||
self.assertFalse(DataSource.objects.filter(pk=datasource.pk).exists())
|
||||
self.assertEqual(Job.objects.filter(object_type=ds_ct, object_id=datasource.pk).count(), 0)
|
||||
|
||||
def test_soft_delete_preserves_jobs(self):
|
||||
_, script = self._create_script()
|
||||
self._add_jobs(script, 10)
|
||||
|
||||
script.delete(soft_delete=True)
|
||||
|
||||
script.refresh_from_db()
|
||||
self.assertFalse(script.is_executable)
|
||||
self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10)
|
||||
|
||||
@override_settings(PROTECTION_RULES={'extras.script': [CustomValidator({'name': {'eq': ''}})]})
|
||||
def test_delete_rolls_back_jobs_on_parent_failure(self):
|
||||
# A protection rule that no real script can satisfy (name must be empty) makes the
|
||||
# cascade's pre_delete handler raise AbortRequest *after* JobsMixin.delete has already
|
||||
# batch-deleted the jobs. JobsMixin.delete wraps the batch loop and super().delete() in a
|
||||
# transaction, so the job deletions must roll back, leaving no orphaned partial state.
|
||||
# This exercises the real deletion-abort path rather than mocking Django internals.
|
||||
_, script = self._create_script()
|
||||
self._add_jobs(script, 10)
|
||||
|
||||
with self.assertRaises(AbortRequest):
|
||||
script.delete()
|
||||
|
||||
self.assertTrue(Script.objects.filter(pk=script.pk).exists())
|
||||
self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10)
|
||||
|
||||
@override_settings(PROTECTION_RULES={'extras.script': [CustomValidator({'name': {'eq': ''}})]})
|
||||
def test_delete_scriptmodule_rolls_back_child_jobs_on_failure(self):
|
||||
# Same abort path via the module: the protection rule fires when the cascade pre_deletes
|
||||
# the child Script, after ScriptModule.delete has already batch-deleted that script's jobs.
|
||||
# The transaction must roll those job deletions back, leaving no orphaned partial state.
|
||||
module, script = self._create_script()
|
||||
self._add_jobs(script, 10)
|
||||
|
||||
with self.assertRaises(AbortRequest):
|
||||
module.delete()
|
||||
|
||||
self.assertTrue(ScriptModule.objects.filter(pk=module.pk).exists())
|
||||
self.assertTrue(Script.objects.filter(pk=script.pk).exists())
|
||||
self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10)
|
||||
|
||||
|
||||
class ConfirmCollectorTestCase(TestCase):
|
||||
"""
|
||||
#22812: the delete-confirmation page must not materialize every dependent Job.
|
||||
"""
|
||||
def _create_script_with_jobs(self, count):
|
||||
module = ScriptModule.objects.create(
|
||||
file_root=ManagedFileRootPathChoices.SCRIPTS,
|
||||
file_path=f'test_{uuid.uuid4().hex[:8]}.py',
|
||||
)
|
||||
script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}')
|
||||
ct = ContentType.objects.get_for_model(Script, for_concrete_model=False)
|
||||
Job.objects.bulk_create([
|
||||
Job(object_type=ct, object_id=script.pk, name='j', status=JobStatusChoices.STATUS_COMPLETED,
|
||||
job_id=uuid.uuid4(), data={'output': 'x' * 50})
|
||||
for _ in range(count)
|
||||
])
|
||||
return script
|
||||
|
||||
def test_confirm_collector_counts_jobs_without_instantiating(self):
|
||||
script = self._create_script_with_jobs(500)
|
||||
|
||||
init_calls = []
|
||||
original_init = Job.__init__
|
||||
|
||||
def counting_init(self, *args, **kwargs):
|
||||
init_calls.append(1)
|
||||
original_init(self, *args, **kwargs)
|
||||
|
||||
with mock.patch.object(Job, '__init__', counting_init):
|
||||
collector = ConfirmCollector(using=router.db_for_write(Script))
|
||||
collector.collect([script])
|
||||
|
||||
# No Job rows were instantiated; the relation was counted instead.
|
||||
self.assertEqual(len(init_calls), 0)
|
||||
self.assertNotIn(Job, collector.data)
|
||||
self.assertEqual(collector.generic_relation_counts.get(Job), 500)
|
||||
# The non-job cascade (the Script itself) is still collected.
|
||||
self.assertIn(Script, collector.data)
|
||||
|
||||
def test_count_only_wrapper(self):
|
||||
# CountOnly reports its count via len() but iterates empty, so it slots into the
|
||||
# dependent-objects mapping as a non-expandable, non-materializing row.
|
||||
wrapper = CountOnly(3000)
|
||||
self.assertEqual(len(wrapper), 3000)
|
||||
self.assertEqual(list(wrapper), [])
|
||||
self.assertTrue(wrapper.count_only)
|
||||
|
||||
def test_confirm_collector_omits_jobs_when_none(self):
|
||||
# A jobless object must not record a zero count, or the confirmation page would show a
|
||||
# spurious "0 jobs" row (#22812 regression).
|
||||
datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test')
|
||||
|
||||
collector = ConfirmCollector(using=router.db_for_write(DataSource))
|
||||
collector.collect([datasource])
|
||||
|
||||
self.assertNotIn(Job, collector.generic_relation_counts)
|
||||
|
||||
|
||||
class ObjectDeleteViewCountsTestCase(ViewTestCase):
|
||||
"""
|
||||
#22812: the delete-confirmation view must report a JobsMixin object's jobs as a count
|
||||
(via CountOnly) without materializing them, and _get_dependent_objects must keep returning
|
||||
a single dict.
|
||||
"""
|
||||
def test_get_dependent_objects_returns_count_only_for_jobs(self):
|
||||
from netbox.views.generic.object_views import ObjectDeleteView
|
||||
|
||||
module = ScriptModule.objects.create(
|
||||
file_root=ManagedFileRootPathChoices.SCRIPTS,
|
||||
file_path=f'test_{uuid.uuid4().hex[:8]}.py',
|
||||
)
|
||||
script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}')
|
||||
ct = ContentType.objects.get_for_model(Script, for_concrete_model=False)
|
||||
Job.objects.bulk_create([
|
||||
Job(
|
||||
object_type=ct, object_id=script.pk, name='j',
|
||||
status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(),
|
||||
)
|
||||
for _ in range(50)
|
||||
])
|
||||
|
||||
view = ObjectDeleteView()
|
||||
view.queryset = ScriptModule.objects.all()
|
||||
dependent_objects = view._get_dependent_objects(module)
|
||||
|
||||
# Single dict returned (not a tuple); jobs represented as a CountOnly.
|
||||
self.assertIsInstance(dependent_objects, dict)
|
||||
self.assertIn(Job, dependent_objects)
|
||||
self.assertIsInstance(dependent_objects[Job], CountOnly)
|
||||
self.assertEqual(len(dependent_objects[Job]), 50)
|
||||
|
||||
@override_settings(EXEMPT_VIEW_PERMISSIONS=['*'])
|
||||
def test_confirm_page_renders_job_count(self):
|
||||
module = ScriptModule.objects.create(
|
||||
file_root=ManagedFileRootPathChoices.SCRIPTS,
|
||||
file_path=f'test_{uuid.uuid4().hex[:8]}.py',
|
||||
)
|
||||
script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}')
|
||||
ct = ContentType.objects.get_for_model(Script, for_concrete_model=False)
|
||||
Job.objects.bulk_create([
|
||||
Job(
|
||||
object_type=ct, object_id=script.pk, name='j',
|
||||
status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(),
|
||||
)
|
||||
for _ in range(50)
|
||||
])
|
||||
|
||||
# ScriptModule is a proxy over core.ManagedFile, so the delete view requires the
|
||||
# concrete model's permission (core.delete_managedfile), not extras.delete_scriptmodule.
|
||||
self.add_permissions('core.delete_managedfile')
|
||||
url = reverse('extras:scriptmodule_delete', kwargs={'pk': module.pk})
|
||||
response = self.client.get(url)
|
||||
self.assertEqual(response.status_code, 200)
|
||||
|
||||
# Assert on the rendered context, not brittle HTML substrings: Job is present in
|
||||
# dependent_objects as a CountOnly reporting the true count, so the confirmation page
|
||||
# renders it as a summarized (non-expandable) row without materializing 50 Job rows.
|
||||
dependent_objects = response.context['dependent_objects']
|
||||
self.assertIn(Job, dependent_objects)
|
||||
self.assertIsInstance(dependent_objects[Job], CountOnly)
|
||||
self.assertEqual(len(dependent_objects[Job]), 50)
|
||||
self.assertTrue(dependent_objects[Job].count_only)
|
||||
|
||||
def test_get_dependent_objects_omits_jobs_when_none(self):
|
||||
from netbox.views.generic.object_views import ObjectDeleteView
|
||||
|
||||
# A module with no jobs must not produce a CountOnly(0) entry (#22812 regression).
|
||||
module = ScriptModule.objects.create(
|
||||
file_root=ManagedFileRootPathChoices.SCRIPTS,
|
||||
file_path=f'test_{uuid.uuid4().hex[:8]}.py',
|
||||
)
|
||||
|
||||
view = ObjectDeleteView()
|
||||
view.queryset = ScriptModule.objects.all()
|
||||
dependent_objects = view._get_dependent_objects(module)
|
||||
|
||||
self.assertNotIn(Job, dependent_objects)
|
||||
|
|
@ -74,3 +74,10 @@ CENSOR_TOKEN_CHANGED = '***CHANGED***'
|
|||
|
||||
# Placeholder text for empty tables
|
||||
EMPTY_TABLE_TEXT = 'No results found'
|
||||
|
||||
# Batch size for deleting a JobsMixin object's associated jobs during cascade deletion. Job
|
||||
# cannot be fast-deleted (a global pre_delete receiver forces per-instance signals), so deleting
|
||||
# in chunks bounds the work per delete cycle rather than building one huge collection and running
|
||||
# one long DELETE. 1000 matches EXPORT_CHUNK_SIZE and, in benchmarking a 200k-job deletion, was
|
||||
# the fastest of 100/1000/5000 while keeping peak memory flat. See #22812.
|
||||
JOB_DELETE_BATCH_SIZE = 1000
|
||||
|
|
|
|||
|
|
@ -8,6 +8,72 @@ from django.utils.translation import gettext as _
|
|||
logger = logging.getLogger("netbox.models.deletion")
|
||||
|
||||
|
||||
class CountOnly:
|
||||
"""
|
||||
A stand-in for a list of dependent instances that reports a count without holding any
|
||||
instances. Used on the delete-confirmation page for high-cardinality relations (e.g. a
|
||||
JobsMixin object's jobs) which we deliberately do not materialize (see #22812). It is a
|
||||
lenient, empty iterable: `len()` returns the true row count, but iterating yields nothing,
|
||||
so it slots into the same `{model: <iterable>}` mapping as real instance lists and renders
|
||||
as a non-expandable row.
|
||||
"""
|
||||
# Template flag: distinguishes a count-only entry (no instances to list) from a real list,
|
||||
# so the confirmation page can render it without an expand/collapse affordance.
|
||||
count_only = True
|
||||
|
||||
def __init__(self, count):
|
||||
self.count = count
|
||||
|
||||
def __len__(self):
|
||||
return self.count
|
||||
|
||||
def __iter__(self):
|
||||
return iter(())
|
||||
|
||||
|
||||
class ConfirmCollector(Collector):
|
||||
"""
|
||||
A display-only Collector used to enumerate the objects that would be deleted along with a
|
||||
given object, for rendering the delete confirmation page. It behaves like Django's stock
|
||||
Collector (preserving the full FK cascade graph and its ProtectedError/RestrictedError
|
||||
behavior) except that it does not descend into the `jobs` GenericRelation. A JobsMixin
|
||||
object can accumulate thousands of Jobs, each carrying large data/log_entries payloads;
|
||||
materializing them all just to render a confirmation page can exhaust memory (see #22812).
|
||||
Instead, the related Jobs are counted and recorded in `generic_relation_counts`.
|
||||
|
||||
This is intentionally specific to Job, the only high-cardinality GenericRelation in the
|
||||
data model; it is not a general count-out over every GenericRelation. If another relation
|
||||
ever needs the same treatment, extend the check in collect() (and the matching write-path
|
||||
batching in JobsMixin/ScriptModule.delete) rather than assuming this already handles it.
|
||||
"""
|
||||
def __init__(self, *args, **kwargs):
|
||||
super().__init__(*args, **kwargs)
|
||||
self.generic_relation_counts = {}
|
||||
|
||||
def collect(self, objs, source=None, *args, **kwargs):
|
||||
"""
|
||||
Override collect() to count the `jobs` GenericRelation rather than descend into it.
|
||||
|
||||
Django's Collector offers no per-relation skip hook, so we intercept the one call it
|
||||
makes when cascading into a GenericRelation: collect(sub_objs, source=model, ...), where
|
||||
`sub_objs` is a queryset of the related model. When that model is Job, we count the rows
|
||||
instead of collecting (and thus instantiating) them, and forward every other call to the
|
||||
stock implementation untouched. A directly-deleted Job (top-level call, source=None)
|
||||
still collects normally.
|
||||
"""
|
||||
from core.models import Job
|
||||
|
||||
if source is not None and getattr(objs, 'model', None) is Job:
|
||||
# Django calls this branch for the jobs relation even when there are none; only record
|
||||
# a count when there are actually jobs, so jobless objects don't get a spurious
|
||||
# "0 jobs" row on the delete-confirmation page.
|
||||
count = objs.count()
|
||||
if count:
|
||||
self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + count
|
||||
return None
|
||||
return super().collect(objs, source=source, *args, **kwargs)
|
||||
|
||||
|
||||
class CustomCollector(Collector):
|
||||
"""
|
||||
Override Django's stock Collector to handle GenericRelations and ensure proper ordering of cascading deletions.
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ from functools import cached_property
|
|||
from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
|
||||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.core.validators import ValidationError
|
||||
from django.db import models
|
||||
from django.db import models, router, transaction
|
||||
from django.db.models import Q
|
||||
from django.utils import timezone
|
||||
from django.utils.translation import gettext_lazy as _
|
||||
|
|
@ -17,7 +17,7 @@ from extras.constants import CUSTOMFIELD_EMPTY_VALUES
|
|||
from extras.managers import NetBoxTaggableManager, NetBoxTaggableManagerField
|
||||
from extras.utils import is_taggable
|
||||
from netbox.config import get_config
|
||||
from netbox.constants import CORE_APPS
|
||||
from netbox.constants import CORE_APPS, JOB_DELETE_BATCH_SIZE
|
||||
from netbox.models.deletion import DeleteMixin
|
||||
from netbox.plugins import PluginConfig
|
||||
from netbox.registry import registry
|
||||
|
|
@ -43,6 +43,7 @@ __all__ = (
|
|||
'NotificationsMixin',
|
||||
'SyncedDataMixin',
|
||||
'TagsMixin',
|
||||
'batch_delete_jobs',
|
||||
'get_model_features',
|
||||
'has_feature',
|
||||
'model_is_public',
|
||||
|
|
@ -449,9 +450,39 @@ class NotificationsMixin(models.Model):
|
|||
abstract = True
|
||||
|
||||
|
||||
def batch_delete_jobs(job_queryset):
|
||||
"""
|
||||
Delete the Jobs in `job_queryset` in JOB_DELETE_BATCH_SIZE chunks. Job cannot be fast-deleted
|
||||
(a global pre_delete receiver forces per-instance signals), so a single delete would build one
|
||||
huge collection of Job instances and run one very long DELETE; batching bounds the per-cycle
|
||||
work. 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
|
||||
|
||||
# Route writes to the same database the queryset reads from. In JobsMixin.delete the queryset
|
||||
# is bound to the instance's DB while Job.objects would otherwise use the router default; if
|
||||
# those diverge the deleted rows never leave the read side and the loop below never terminates.
|
||||
jobs = Job.objects.using(job_queryset.db)
|
||||
|
||||
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]):
|
||||
# only('pk'): the batch still can't fast-delete, so each Job in the batch is instantiated;
|
||||
# loading just the PK avoids pulling the large data/log_entries payloads into memory.
|
||||
jobs.filter(pk__in=pks).only('pk').delete()
|
||||
|
||||
|
||||
class JobsMixin(models.Model):
|
||||
"""
|
||||
Enables support for job results.
|
||||
|
||||
Note: for the job-batching in delete() to run, JobsMixin must precede DeleteMixin in a
|
||||
model's MRO. DeleteMixin.delete() drives its own collector and does not call super(), so a
|
||||
model declared as e.g. `class Foo(NetBoxModel, JobsMixin)` would reach DeleteMixin first and
|
||||
bypass the batching. Core models that combine both (e.g. DataSource) list JobsMixin first.
|
||||
"""
|
||||
jobs = GenericRelation(
|
||||
to='core.Job',
|
||||
|
|
@ -463,6 +494,16 @@ class JobsMixin(models.Model):
|
|||
class Meta:
|
||||
abstract = True
|
||||
|
||||
def delete(self, using=None, *args, **kwargs):
|
||||
# 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 = using or router.db_for_write(self.__class__, instance=self)
|
||||
with transaction.atomic(using=using):
|
||||
batch_delete_jobs(self.jobs.using(using))
|
||||
return super().delete(using, *args, **kwargs)
|
||||
delete.alters_data = True
|
||||
|
||||
def get_latest_jobs(self):
|
||||
"""
|
||||
Return a list of the most recent jobs for this instance.
|
||||
|
|
|
|||
|
|
@ -4,7 +4,6 @@ from collections import defaultdict
|
|||
from django.contrib import messages
|
||||
from django.db import router, transaction
|
||||
from django.db.models import ProtectedError, RestrictedError
|
||||
from django.db.models.deletion import Collector
|
||||
from django.http import HttpResponse
|
||||
from django.shortcuts import redirect, render
|
||||
from django.urls import reverse
|
||||
|
|
@ -13,6 +12,7 @@ from django.utils.safestring import mark_safe
|
|||
from django.utils.translation import gettext as _
|
||||
|
||||
from core.signals import clear_events
|
||||
from netbox.models.deletion import ConfirmCollector, CountOnly
|
||||
from netbox.object_actions import BulkDelete, BulkEdit, CloneObject, DeleteObject, EditObject
|
||||
from utilities.error_handlers import handle_protectederror
|
||||
from utilities.exceptions import AbortRequest, PermissionsViolation
|
||||
|
|
@ -385,14 +385,19 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView):
|
|||
|
||||
def _get_dependent_objects(self, obj):
|
||||
"""
|
||||
Returns a dictionary mapping of dependent objects (organized by model) which will be deleted as a result of
|
||||
deleting the requested object.
|
||||
Returns a dictionary mapping each dependent model to the objects (of that model) which will
|
||||
be deleted as a result of deleting the requested object.
|
||||
|
||||
Values are normally a list of instances. For high-cardinality relations that we do not
|
||||
materialize to avoid excessive memory use (currently a JobsMixin object's jobs, see
|
||||
#22812), the value is a `CountOnly` — a lenient empty iterable whose `len()` is the true
|
||||
row count, so it renders as a non-expandable row alongside the itemized relations.
|
||||
|
||||
Args:
|
||||
obj: The object to return dependent objects for
|
||||
"""
|
||||
using = router.db_for_write(obj._meta.model)
|
||||
collector = Collector(using=using)
|
||||
collector = ConfirmCollector(using=using)
|
||||
collector.collect([obj])
|
||||
|
||||
# Compile a mapping of models to instances
|
||||
|
|
@ -406,7 +411,13 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView):
|
|||
continue
|
||||
dependent_objects[model].append(instances)
|
||||
|
||||
return dict(dependent_objects)
|
||||
# Add count-only entries for relations the collector enumerated by count rather than by
|
||||
# instance (e.g. jobs), so they render as non-expandable rows in the same mapping.
|
||||
dependent_objects = dict(dependent_objects)
|
||||
for model, count in collector.generic_relation_counts.items():
|
||||
dependent_objects[model] = CountOnly(count)
|
||||
|
||||
return dependent_objects
|
||||
|
||||
def _handle_protected_objects(self, obj, protected_objects, request, exc):
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -24,31 +24,38 @@
|
|||
</p>
|
||||
<div class="accordion mb-3" id="deleteAccordion">
|
||||
{% for model, instances in dependent_objects.items %}
|
||||
<div class="accordion-item">
|
||||
<h2 class="accordion-header h4" id="deleteheading{{ forloop.counter }}">
|
||||
<button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" data-bs-target="#collapse{{ forloop.counter }}" aria-expanded="false" aria-controls="collapse{{ forloop.counter }}">
|
||||
{% with object_count=instances|length %}
|
||||
{{ object_count }}
|
||||
{% if object_count == 1 %}
|
||||
{{ model|meta:"verbose_name" }}
|
||||
{% else %}
|
||||
{{ model|meta:"verbose_name_plural" }}
|
||||
{% endif %}
|
||||
{% endwith %}
|
||||
</button>
|
||||
</h2>
|
||||
<div id="collapse{{ forloop.counter }}" class="accordion-collapse collapse" aria-labelledby="deleteheading{{ forloop.counter }}" data-bs-parent="#deleteAccordion">
|
||||
<div class="accordion-body p-0">
|
||||
<div class="list-group list-group-flush">
|
||||
{% for instance in instances %}
|
||||
{% with url=instance.get_absolute_url %}
|
||||
<a {% if url %}href="{{ url }}" {% endif %}class="list-group-item list-group-item-action">{{ instance }}</a>
|
||||
{% endwith %}
|
||||
{% endfor %}
|
||||
{% with object_count=instances|length %}
|
||||
<div class="accordion-item">
|
||||
<h2 class="accordion-header h4" id="deleteheading{{ forloop.counter }}">
|
||||
{# High-cardinality relations (e.g. jobs) are summarized by count and are not #}
|
||||
{# expandable, since their instances are intentionally not loaded (see #22812). #}
|
||||
{% if instances.count_only %}
|
||||
<div class="accordion-button collapsed" style="cursor: default;">
|
||||
{{ object_count }}
|
||||
{% if object_count == 1 %}{{ model|meta:"verbose_name" }}{% else %}{{ model|meta:"verbose_name_plural" }}{% endif %}
|
||||
</div>
|
||||
{% else %}
|
||||
<button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" data-bs-target="#collapse{{ forloop.counter }}" aria-expanded="false" aria-controls="collapse{{ forloop.counter }}">
|
||||
{{ object_count }}
|
||||
{% if object_count == 1 %}{{ model|meta:"verbose_name" }}{% else %}{{ model|meta:"verbose_name_plural" }}{% endif %}
|
||||
</button>
|
||||
{% endif %}
|
||||
</h2>
|
||||
{% if not instances.count_only %}
|
||||
<div id="collapse{{ forloop.counter }}" class="accordion-collapse collapse" aria-labelledby="deleteheading{{ forloop.counter }}" data-bs-parent="#deleteAccordion">
|
||||
<div class="accordion-body p-0">
|
||||
<div class="list-group list-group-flush">
|
||||
{% for instance in instances %}
|
||||
{% with url=instance.get_absolute_url %}
|
||||
<a {% if url %}href="{{ url }}" {% endif %}class="list-group-item list-group-item-action">{{ instance }}</a>
|
||||
{% endwith %}
|
||||
{% endfor %}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
{% endif %}
|
||||
</div>
|
||||
</div>
|
||||
{% endwith %}
|
||||
{% endfor %}
|
||||
</div>
|
||||
{% endif %}
|
||||
|
|
|
|||
Loading…
Reference in New Issue