AC-984 Prevent signal handlers from being run unnecessarily when deleting inventory, remove unnecessary extra queries, use update_fields when possible.

awx/api/generics.py

@@ -6,7 +6,7 @@ import inspect
 import json
 
 # Django
-from django.http import HttpResponse, Http404
+from django.http import Http404
 from django.contrib.auth.models import User
 from django.shortcuts import get_object_or_404
 from django.template.loader import render_to_string
@@ -428,4 +428,4 @@ class RetrieveUpdateDestroyAPIView(RetrieveUpdateAPIView, generics.RetrieveUpdat
             obj.mark_inactive()
         else:
             raise NotImplementedError('destroy() not implemented yet for %s' % obj)
-        return HttpResponse(status=204)
+        return Response(status=status.HTTP_204_NO_CONTENT)

awx/api/views.py

@@ -34,6 +34,7 @@ from awx.main.licenses import LicenseReader
 from awx.main.models import *
 from awx.main.utils import *
 from awx.main.access import get_user_queryset
+from awx.main.signals import ignore_inventory_computed_fields, ignore_inventory_group_removal
 from awx.api.authentication import JobTaskAuthentication
 from awx.api.permissions import *
 from awx.api.serializers import *
@@ -619,6 +620,11 @@ class InventoryDetail(RetrieveUpdateDestroyAPIView):
     model = Inventory
     serializer_class = InventorySerializer
 
+    def destroy(self, request, *args, **kwargs):
+        with ignore_inventory_computed_fields():
+            with ignore_inventory_group_removal():
+                return super(InventoryDetail, self).destroy(request, *args, **kwargs)
+
 class InventoryActivityStreamList(SubListAPIView):
 
     model = ActivityStream

awx/main/middleware.py

@@ -8,17 +8,18 @@ from django.db import IntegrityError
 from django.utils.functional import curry
 from awx.main.models import ActivityStream, AuthToken
 import json
+import threading
 import uuid
 import urllib2
 
 import logging
 logger = logging.getLogger('awx.main.middleware')
 
-class ActivityStreamMiddleware(object):
+class ActivityStreamMiddleware(threading.local):
 
     def __init__(self):
         self.disp_uid = None
-        self.instances = []
+        self.instance_ids = []
 
     def process_request(self, request):
         if hasattr(request, 'user') and hasattr(request.user, 'is_authenticated') and request.user.is_authenticated():
@@ -28,6 +29,7 @@ class ActivityStreamMiddleware(object):
 
         set_actor = curry(self.set_actor, user)
         self.disp_uid = str(uuid.uuid1())
+        self.instance_ids = []
         post_save.connect(set_actor, sender=ActivityStream, dispatch_uid=self.disp_uid, weak=False)
 
     def process_response(self, request, response):
@@ -35,31 +37,27 @@ class ActivityStreamMiddleware(object):
         drf_user = getattr(drf_request, 'user', None)
         if self.disp_uid is not None:
             post_save.disconnect(dispatch_uid=self.disp_uid)
-        for instance_id in self.instances:
-            instance = ActivityStream.objects.filter(id=instance_id)
-            if instance.exists():
-                instance = instance[0]
-            else:
-                logger.debug("Failed to look up Activity Stream instance for id : " + str(instance_id))
-                continue
 
-            if drf_user is not None and drf_user.__class__ != AnonymousUser:
+        for instance in ActivityStream.objects.filter(id__in=self.instance_ids):
+            if drf_user and drf_user.pk:
                 instance.actor = drf_user
                 try:
-                    instance.save()
+                    instance.save(update_fields=['actor'])
                 except IntegrityError, e:
-                    logger.debug("Integrity Error saving Activity Stream instance for id : " + str(instance_id))
+                    logger.debug("Integrity Error saving Activity Stream instance for id : " + str(instance.id))
             # else:
             #     obj1_type_actual = instance.object1_type.split(".")[-1]
             #     if obj1_type_actual in ("InventoryUpdate", "ProjectUpdate", "Job") and instance.id is not None:
             #         instance.delete()
+
+        self.instance_ids = []
         return response
 
     def set_actor(self, user, sender, instance, **kwargs):
         if sender == ActivityStream:
-            if isinstance(user, User) and instance.user is None:
+            if isinstance(user, User) and instance.actor is None:
                 instance.actor = user
-                instance.save()
+                instance.save(update_fields=['actor'])
             else:
-                if instance.id not in self.instances:
-                    self.instances.append(instance.id)
+                if instance.id not in self.instance_ids:
+                    self.instance_ids.append(instance.id)

awx/main/models/activity_stream.py

@@ -51,3 +51,14 @@ class ActivityStream(models.Model):
 
     def get_absolute_url(self):
         return reverse('api:activity_stream_detail', args=(self.pk,))
+
+    def save(self, *args, **kwargs):
+        # For compatibility with Django 1.4.x, attempt to handle any calls to
+        # save that pass update_fields.
+        try:
+            super(ActivityStream, self).save(*args, **kwargs)
+        except TypeError:
+            if 'update_fields' not in kwargs:
+                raise
+            kwargs.pop('update_fields')
+            super(ActivityStream, self).save(*args, **kwargs)

awx/main/models/base.py

@@ -193,14 +193,20 @@ class PrimordialModel(BaseModel):
 
     tags = TaggableManager(blank=True)
 
-    def mark_inactive(self, save=True):
+    def mark_inactive(self, save=True, update_fields=None):
         '''Use instead of delete to rename and mark inactive.'''
+        update_fields = update_fields or []
         if self.active:
             if 'name' in self._meta.get_all_field_names():
                 self.name   = "_deleted_%s_%s" % (now().isoformat(), self.name)
+                if 'name' not in update_fields:
+                    update_fields.append('name')
             self.active = False
+            if 'active' not in update_fields:
+                update_fields.append('active')
             if save:
-                self.save()
+                self.save(update_fields=update_fields)
+        return update_fields
 
     def clean_description(self):
         # Description should always be empty string, never null.

awx/main/models/inventory.py

@@ -100,14 +100,15 @@ class Inventory(CommonModel):
         '''
         When marking inventory inactive, also mark hosts and groups inactive.
         '''
+        from awx.main.signals import ignore_inventory_computed_fields
+        with ignore_inventory_computed_fields():
+            for host in self.hosts.filter(active=True):
+                host.mark_inactive()
+            for group in self.groups.filter(active=True):
+                group.mark_inactive()
+            for inventory_source in self.inventory_sources.filter(active=True):
+                inventory_source.mark_inactive()
         super(Inventory, self).mark_inactive(save=save)
-        for host in self.hosts.filter(active=True):
-            host.mark_inactive()
-        for group in self.groups.filter(active=True):
-            group.mark_inactive()
-            group.inventory_source.mark_inactive()
-        for inventory_source in self.inventory_sources.filter(active=True):
-            inventory_source.mark_inactive()
 
     variables_dict = VarsDictProperty('variables')
 

awx/main/signals.py

@@ -24,7 +24,7 @@ logger = logging.getLogger('awx.main.signals')
 # or marked inactive, when a Host-Group or Group-Group relationship is updated,
 # or when a Job is deleted or marked inactive.
 
-_inventory_updating = threading.local()
+_inventory_updates = threading.local()
 
 @contextlib.contextmanager
 def ignore_inventory_computed_fields():
@@ -32,49 +32,62 @@ def ignore_inventory_computed_fields():
     Context manager to ignore updating inventory computed fields.
     '''
     try:
-        previous_value = getattr(_inventory_updating, 'is_updating', False)
-        _inventory_updating.is_updating = True
+        previous_value = getattr(_inventory_updates, 'is_updating', False)
+        _inventory_updates.is_updating = True
         yield
     finally:
-        _inventory_updating.is_updating = previous_value
+        _inventory_updates.is_updating = previous_value
+
+@contextlib.contextmanager
+def ignore_inventory_group_removal():
+    '''
+    Context manager to ignore moving groups/hosts when group is deleted.
+    '''
+    try:
+        previous_value = getattr(_inventory_updates, 'is_removing', False)
+        _inventory_updates.is_removing = True
+        yield
+    finally:
+        _inventory_updates.is_removing = previous_value
 
 def update_inventory_computed_fields(sender, **kwargs):
     '''
     Signal handler and wrapper around inventory.update_computed_fields to
     prevent unnecessary recursive calls.
     '''
-    if not getattr(_inventory_updating, 'is_updating', False):
-        instance = kwargs['instance']
-        if sender == Group.hosts.through:
-            sender_name = 'group.hosts'
-        elif sender == Group.parents.through:
-            sender_name = 'group.parents'
-        elif sender == Host.inventory_sources.through:
-            sender_name = 'host.inventory_sources'
-        elif sender == Group.inventory_sources.through:
-            sender_name = 'group.inventory_sources'
-        else:
-            sender_name = unicode(sender._meta.verbose_name)
-        if kwargs['signal'] == post_save:
-            if sender == Job and instance.active:
-                return
-            sender_action = 'saved'
-        elif kwargs['signal'] == post_delete:
-            sender_action = 'deleted'
-        elif kwargs['signal'] == m2m_changed and kwargs['action'] in ('post_add', 'post_remove', 'post_clear'):
-            sender_action = 'changed'
-        else:
+    if getattr(_inventory_updates, 'is_updating', False):
+        return
+    instance = kwargs['instance']
+    if sender == Group.hosts.through:
+        sender_name = 'group.hosts'
+    elif sender == Group.parents.through:
+        sender_name = 'group.parents'
+    elif sender == Host.inventory_sources.through:
+        sender_name = 'host.inventory_sources'
+    elif sender == Group.inventory_sources.through:
+        sender_name = 'group.inventory_sources'
+    else:
+        sender_name = unicode(sender._meta.verbose_name)
+    if kwargs['signal'] == post_save:
+        if sender == Job and instance.active:
             return
-        logger.debug('%s %s, updating inventory computed fields: %r %r',
-                     sender_name, sender_action, sender, kwargs)
-        with ignore_inventory_computed_fields():
-            try:
-                inventory = instance.inventory
-            except Inventory.DoesNotExist:
-                pass
-            else:
-                update_hosts = issubclass(sender, Job)
-                inventory.update_computed_fields(update_hosts=update_hosts)
+        sender_action = 'saved'
+    elif kwargs['signal'] == post_delete:
+        sender_action = 'deleted'
+    elif kwargs['signal'] == m2m_changed and kwargs['action'] in ('post_add', 'post_remove', 'post_clear'):
+        sender_action = 'changed'
+    else:
+        return
+    logger.debug('%s %s, updating inventory computed fields: %r %r',
+                 sender_name, sender_action, sender, kwargs)
+    with ignore_inventory_computed_fields():
+        try:
+            inventory = instance.inventory
+        except Inventory.DoesNotExist:
+            pass
+        else:
+            update_hosts = issubclass(sender, Job)
+            inventory.update_computed_fields(update_hosts=update_hosts)
 
 post_save.connect(update_inventory_computed_fields, sender=Host)
 post_delete.connect(update_inventory_computed_fields, sender=Host)
@@ -94,32 +107,50 @@ post_delete.connect(update_inventory_computed_fields, sender=InventorySource)
 
 @receiver(pre_delete, sender=Group)
 def save_related_pks_before_group_delete(sender, **kwargs):
+    if getattr(_inventory_updates, 'is_removing', False):
+        return
     instance = kwargs['instance']
+    instance._saved_inventory_pk = instance.inventory.pk
     instance._saved_parents_pks = set(instance.parents.values_list('pk', flat=True))
     instance._saved_hosts_pks = set(instance.hosts.values_list('pk', flat=True))
     instance._saved_children_pks = set(instance.children.values_list('pk', flat=True))
 
 @receiver(post_delete, sender=Group)
 def migrate_children_from_deleted_group_to_parent_groups(sender, **kwargs):
+    if getattr(_inventory_updates, 'is_removing', False):
+        return
     instance = kwargs['instance']
     parents_pks = getattr(instance, '_saved_parents_pks', [])
     hosts_pks = getattr(instance, '_saved_hosts_pks', [])
     children_pks = getattr(instance, '_saved_children_pks', [])
-    for parent_group in Group.objects.filter(pk__in=parents_pks):
-        for child_host in Host.objects.filter(pk__in=hosts_pks):
-            logger.debug('adding host %s to parent %s after group deletion',
-                         child_host, parent_group)
-            parent_group.hosts.add(child_host)
-        for child_group in Group.objects.filter(pk__in=children_pks):
-            logger.debug('adding group %s to parent %s after group deletion',
-                         child_group, parent_group)
-            parent_group.children.add(child_group)
+    with ignore_inventory_group_removal():
+        with ignore_inventory_computed_fields():
+            if parents_pks:
+                for parent_group in Group.objects.filter(pk__in=parents_pks, active=True):
+                    for child_host in Host.objects.filter(pk__in=hosts_pks, active=True):
+                        logger.debug('adding host %s to parent %s after group deletion',
+                                     child_host, parent_group)
+                        parent_group.hosts.add(child_host)
+                    for child_group in Group.objects.filter(pk__in=children_pks, active=True):
+                        logger.debug('adding group %s to parent %s after group deletion',
+                                     child_group, parent_group)
+                        parent_group.children.add(child_group)
+                inventory_pk = getattr(instance, '_saved_inventory_pk', None)
+                if inventory_pk:
+                    try:
+                        inventory = Inventory.objects.get(pk=inventory_pk, active=True)
+                        inventory.update_computed_fields()
+                    except Inventory.DoesNotExist:
+                        pass
 
 @receiver(pre_save, sender=Group)
 def save_related_pks_before_group_marked_inactive(sender, **kwargs):
+    if getattr(_inventory_updates, 'is_removing', False):
+        return
     instance = kwargs['instance']
     if not instance.pk or instance.active:
         return
+    instance._saved_inventory_pk = instance.inventory.pk
     instance._saved_parents_pks = set(instance.parents.values_list('pk', flat=True))
     instance._saved_hosts_pks = set(instance.hosts.values_list('pk', flat=True))
     instance._saved_children_pks = set(instance.children.values_list('pk', flat=True))
@@ -127,26 +158,41 @@ def save_related_pks_before_group_marked_inactive(sender, **kwargs):
 
 @receiver(post_save, sender=Group)
 def migrate_children_from_inactive_group_to_parent_groups(sender, **kwargs):
+    if getattr(_inventory_updates, 'is_removing', False):
+        return
     instance = kwargs['instance']
     if instance.active:
         return
     parents_pks = getattr(instance, '_saved_parents_pks', [])
     hosts_pks = getattr(instance, '_saved_hosts_pks', [])
     children_pks = getattr(instance, '_saved_children_pks', [])
-    for parent_group in Group.objects.filter(pk__in=parents_pks):
-        for child_host in Host.objects.filter(pk__in=hosts_pks):
-            logger.debug('moving host %s to parent %s after marking group %s inactive',
-                         child_host, parent_group, instance)
-            parent_group.hosts.add(child_host)
-        for child_group in Group.objects.filter(pk__in=children_pks):
-            logger.debug('moving group %s to parent %s after marking group %s inactive',
-                         child_group, parent_group, instance)
-            parent_group.children.add(child_group)
-        parent_group.children.remove(instance)
-    inventory_source_pk = getattr(instance, '_saved_inventory_source_pk', None)
-    if inventory_source_pk:
-        inventory_source = InventorySource.objects.get(pk=inventory_source_pk)
-        inventory_source.mark_inactive()
+    with ignore_inventory_group_removal():
+        with ignore_inventory_computed_fields():
+            if parents_pks:
+                for parent_group in Group.objects.filter(pk__in=parents_pks, active=True):
+                    for child_host in Host.objects.filter(pk__in=hosts_pks, active=True):
+                        logger.debug('moving host %s to parent %s after marking group %s inactive',
+                                     child_host, parent_group, instance)
+                        parent_group.hosts.add(child_host)
+                    for child_group in Group.objects.filter(pk__in=children_pks, active=True):
+                        logger.debug('moving group %s to parent %s after marking group %s inactive',
+                                     child_group, parent_group, instance)
+                        parent_group.children.add(child_group)
+                    parent_group.children.remove(instance)
+            inventory_source_pk = getattr(instance, '_saved_inventory_source_pk', None)
+            if inventory_source_pk:
+                try:
+                    inventory_source = InventorySource.objects.get(pk=inventory_source_pk, active=True)
+                    inventory_source.mark_inactive()
+                except InventorySource.DoesNotExist:
+                    pass
+            inventory_pk = getattr(instance, '_saved_inventory_pk', None)
+            if inventory_pk:
+                try:
+                    inventory = Inventory.objects.get(pk=inventory_pk, active=True)
+                    inventory.update_computed_fields()
+                except Inventory.DoesNotExist:
+                    pass
 
 # Update host pointers to last_job and last_job_host_summary when a job is
 # marked inactive or deleted.