From 3d90c6db7663531890f98c614aee476e5becd32b Mon Sep 17 00:00:00 2001 From: Matthew Jones Date: Wed, 21 May 2014 16:08:51 -0400 Subject: [PATCH] Deref deleted group immediately and then pass off the children and hosts to be processed in the background. Get rid of the child relative delete mechanism and merge the old concept of delete on the /group/n endpoint with our new concept --- awx/api/urls.py | 2 - awx/api/views.py | 61 +++++++---------------------- awx/main/models/inventory.py | 37 ++++-------------- awx/main/tasks.py | 40 +++++++++++++++++++ awx/main/tests/inventory.py | 76 +++--------------------------------- 5 files changed, 66 insertions(+), 150 deletions(-) diff --git a/awx/api/urls.py b/awx/api/urls.py index fe38d61019..dd83eb98d8 100644 --- a/awx/api/urls.py +++ b/awx/api/urls.py @@ -65,7 +65,6 @@ inventory_urls = patterns('awx.api.views', url(r'^(?P[0-9]+)/$', 'inventory_detail'), url(r'^(?P[0-9]+)/hosts/$', 'inventory_hosts_list'), url(r'^(?P[0-9]+)/groups/$', 'inventory_groups_list'), - url(r'^(?P[0-9]+)/groups/(?P[0-9]+)/$', 'inventory_root_group_remove'), url(r'^(?P[0-9]+)/root_groups/$', 'inventory_root_groups_list'), url(r'^(?P[0-9]+)/variable_data/$', 'inventory_variable_data'), url(r'^(?P[0-9]+)/script/$', 'inventory_script_view'), @@ -90,7 +89,6 @@ group_urls = patterns('awx.api.views', url(r'^$', 'group_list'), url(r'^(?P[0-9]+)/$', 'group_detail'), url(r'^(?P[0-9]+)/children/$', 'group_children_list'), - url(r'^(?P[0-9]+)/children/(?P[0-9]+)/$', 'group_children_remove'), url(r'^(?P[0-9]+)/hosts/$', 'group_hosts_list'), url(r'^(?P[0-9]+)/all_hosts/$', 'group_all_hosts_list'), url(r'^(?P[0-9]+)/variable_data/$', 'group_variable_data'), diff --git a/awx/api/views.py b/awx/api/views.py index c701bf9ea6..1c5b6cf2af 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -788,42 +788,7 @@ class GroupChildrenList(SubListCreateAPIView): return Response(status=status.HTTP_204_NO_CONTENT) -class GroupChildrenRemove(DestroyAPIView): - - model = Group - serializer_class = GroupSerializer - view_name = 'Remove a subgroup recursively' - - def destroy(self, request, *args, **kwargs): - parent_group = self.get_object() - group = Group.objects.get(id=kwargs['subgroup_pk']) - if group not in parent_group.children.all(): - return Response(status=status.HTTP_404_NOT_FOUND) - group.mark_inactive_recursive(parent_group) - return Response() - - def post(self, request, *args, **kwargs): - if not isinstance(request.DATA, dict): - return Response('invalid type for post data', - status=status.HTTP_400_BAD_REQUEST) - if 'disassociate' in request.DATA: - parent_group = self.get_object() - group = Group.objects.get(id=kwargs['subgroup_pk']) - if group not in parent_group.children.all(): - return Response(status=status.HTTP_404_NOT_FOUND) - parent_group.children.remove(group) - for host in group.hosts.all(): - parent_group.hosts.add(host) - for subgroup in group.children.all(): - parent_group.children.add(subgroup) - if group.parents.count() < 1: - group.mark_inactive() - return Response() - else: - return Response() - - -class GroupPotentialChildrenList(SubListAPIView): +lass GroupPotentialChildrenList(SubListAPIView): model = Group serializer_class = GroupSerializer @@ -890,6 +855,19 @@ class GroupDetail(RetrieveUpdateDestroyAPIView): model = Group serializer_class = GroupSerializer + def destroy(self, request, *args, **kwargs): + obj = self.get_object() + # FIXME: Why isn't the active check being caught earlier by RBAC? + if getattr(obj, 'active', True) == False: + raise Http404() + if getattr(obj, 'is_active', True) == False: + raise Http404() + if not request.user.can_access(self.model, 'delete', obj): + raise PermissionDenied() + if hasattr(obj, 'mark_inactive'): + obj.mark_inactive_recursive() + return Response(status=status.HTTP_204_NO_CONTENT) + class InventoryGroupsList(SubListCreateAPIView): model = Group @@ -898,17 +876,6 @@ class InventoryGroupsList(SubListCreateAPIView): relationship = 'groups' parent_key = 'inventory' -class InventoryRootGroupRemove(DestroyAPIView): - - model = Group - serializer_class = GroupSerializer - view_name = 'Inventory Group Subgroup' - - def destroy(self, request, *args, **kwargs): - group = Group.objects.get(id=kwargs['group_pk']) - group.mark_inactive_recursive() - return Response() - class InventoryRootGroupsList(SubListCreateAPIView): model = Group diff --git a/awx/main/models/inventory.py b/awx/main/models/inventory.py index 45cef121a8..9aff59895c 100644 --- a/awx/main/models/inventory.py +++ b/awx/main/models/inventory.py @@ -530,36 +530,13 @@ class Group(CommonModelNameNotUnique): def get_absolute_url(self): return reverse('api:group_detail', args=(self.pk,)) - def mark_inactive_recursive(self, parent=None): - from awx.main.tasks import update_inventory_computed_fields - def mark_actual(parent=parent): - linked_children = [(parent, self)] - marked_groups = [] - marked_hosts = [] - for subgroup in linked_children: - parent, group = subgroup - if parent is not None: - group.parents.remove(parent) - if group.parents.count() > 0: - continue - for host in group.hosts.all(): - host.groups.remove(group) - host_inv_sources = host.inventory_sources.all() - for inv_source in group.inventory_sources.all(): - if inv_source in host_inv_sources: - host.inventory_sources.remove(inv_source) - if host.groups.count() < 1: - marked_hosts.append(host) - for childgroup in group.children.all(): - linked_children.append((group, childgroup)) - marked_groups.append(group) - for group in marked_groups: - group.mark_inactive() - for host in marked_hosts: - host.mark_inactive() - with ignore_inventory_computed_fields(): - mark_actual() - update_inventory_computed_fields.delay(self.id, True) + def mark_inactive_recursive(self): + from awx.main.tasks import update_inventory_computed_fields, bulk_inventory_element_delete + group_data = {'parent': self.id, 'inventory': self.inventory.id, + 'children': [{'id': c.id} for c in self.children.all()], + 'hosts': [{'id': h.id} for h in self.hosts.all()]} + self.mark_inactive() + bulk_inventory_element_delete.delay(group_data) def mark_inactive(self, save=True, recompute=True, from_inventory_import=False): ''' diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 85ed5f2d34..12f1fae5e1 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -47,6 +47,46 @@ logger = logging.getLogger('awx.main.tasks') # FIXME: Cleanly cancel task when celery worker is stopped. +@task() +def bulk_inventory_element_delete(group_details): + def remove_host_from_group(host, group): + host.groups.remove(group) + host_inv_sources = host.inventory_sources.all() + for inv_source in group.inventory_sources.all(): + if inv_source in host_inv_sources: + host.inventory_sources.remove(inv_source) + return host.groups.count() < 1 + def mark_actual(group_details): + overall_parent = Group.objects.get(id=group_details['parent']) + linked_children = [(overall_parent , Group.objects.get(id=g['id'])) for g in group_details['children']] + initial_hosts = [Host.objects.get(id=h['id']) for h in group_details['hosts']] + marked_hosts = [] + marked_groups = [] + for host in initial_hosts: + last_group = remove_host_from_group(host, overall_parent) + if last_group: + marked_hosts.append(host) + for subgroup in linked_children: + parent, group = subgroup + if parent is not None: + group.parents.remove(parent) + if group.parents.count() > 0: + continue + for host in group.hosts.all(): + last_group = remove_host_from_group(host, group) + if last_group: + marked_hosts.append(host) + for childgroup in group.children.all(): + linked_children.append((group, childgroup)) + marked_groups.append(group) + for group in marked_groups: + group.mark_inactive() + for host in marked_hosts: + host.mark_inactive() + with ignore_inventory_computed_fields(): + mark_actual(group_details) + update_inventory_computed_fields.delay(group_details['inventory'], True) + @task(bind=True) def tower_periodic_scheduler(self): def get_last_run(): diff --git a/awx/main/tests/inventory.py b/awx/main/tests/inventory.py index b8ab8b77b1..fc8f68eb9a 100644 --- a/awx/main/tests/inventory.py +++ b/awx/main/tests/inventory.py @@ -898,20 +898,12 @@ class InventoryTest(BaseTest): other_sub_group = self.inventory_a.groups.create(name='Sub2') other_low_group = self.inventory_a.groups.create(name='Low2') - # Third hierarchy - third_top_group = self.inventory_a.groups.create(name='Top3') - third_sub_group = self.inventory_a.groups.create(name='Sub3') - third_low_group = self.inventory_a.groups.create(name='Low3') - sub_group.parents.add(top_group) low_group.parents.add(sub_group) other_sub_group.parents.add(other_top_group) other_low_group.parents.add(other_sub_group) - third_sub_group.parents.add(third_top_group) - third_low_group.parents.add(third_sub_group) - t1 = self.inventory_a.hosts.create(name='t1') t1.groups.add(top_group) s1 = self.inventory_a.hosts.create(name='s1') @@ -926,75 +918,17 @@ class InventoryTest(BaseTest): l2 = self.inventory_a.hosts.create(name='l2') l2.groups.add(other_low_group) - t3 = self.inventory_a.hosts.create(name='t3') - t3.groups.add(third_top_group) - s3 = self.inventory_a.hosts.create(name='s3') - s3.groups.add(third_sub_group) - l3 = self.inventory_a.hosts.create(name='l3') - l3.groups.add(third_low_group) - # Copy second hierarchy subgroup under the first hierarchy subgroup other_sub_group.parents.add(sub_group) self.assertTrue(s2 in sub_group.all_hosts.all()) self.assertTrue(other_sub_group in sub_group.children.all()) - # Now recursively remove it, the references in other_top_group should remain - other_sub_group.mark_inactive_recursive(parent=sub_group) - self.assertFalse(s2 in sub_group.all_hosts.all()) - self.assertFalse(other_sub_group in sub_group.children.all()) - self.assertTrue(s2 in other_top_group.all_hosts.all()) - self.assertTrue(other_sub_group in other_top_group.children.all()) - - # Recursively remove the third hierarchy which has no links to others so should go inactive - third_top_group.mark_inactive_recursive(parent=None) - third_low_group = Group.objects.get(pk=third_low_group.pk) - third_sub_group = Group.objects.get(pk=third_sub_group.pk) - third_top_group = Group.objects.get(pk=third_top_group.pk) - t3 = Host.objects.get(pk=t3.pk) - s3 = Host.objects.get(pk=s3.pk) - l3 = Host.objects.get(pk=l3.pk) - self.assertFalse(third_low_group.active) - self.assertFalse(third_sub_group.active) - self.assertFalse(third_top_group.active) - self.assertFalse(l3.active) - self.assertFalse(s3.active) - self.assertFalse(t3.active) - - # Add second hierarchy low group under the first hierarchy subgroup - other_low_group.parents.add(sub_group) - - # Try to remove it with a regular user - with self.current_user(self.other_django_user): - url = reverse('api:group_children_remove', args=(sub_group.pk, other_low_group.pk)) - self.delete(url, expect=403) - - # Try to remove it with the admin user - with self.current_user(self.normal_django_user): - url = reverse('api:group_children_remove', args=(sub_group.pk, other_low_group.pk)) - self.delete(url, expect=200) - - # Admin user should have removed the reference - self.assertFalse(l2 in sub_group.all_hosts.all()) - self.assertFalse(other_low_group in sub_group.children.all()) - - # Add the second hierarchy low group under the first hierarchy subgroup again - other_low_group.parents.add(sub_group) - - with self.current_user(self.normal_django_user): - url = reverse('api:inventory_root_group_remove', args=(self.inventory_a.pk, other_top_group.pk,)) - self.delete(url, expect=200) - - # Entire hierarchy except for the low group should be gone - t2 = Host.objects.get(pk=t2.pk) - s2 = Host.objects.get(pk=s2.pk) - l2 = Host.objects.get(pk=l2.pk) + # Now recursively remove its parent and the reference from subgroup should remain + other_top_group.mark_inactive_recursive() other_top_group = Group.objects.get(pk=other_top_group.pk) - other_sub_group = Group.objects.get(pk=other_sub_group.pk) - other_low_group = Group.objects.get(pk=other_low_group.pk) - self.assertFalse(t2.active or s2.active) - self.assertFalse(other_top_group.active or other_sub_group.active) - self.assertTrue(l2.active) - self.assertTrue(other_low_group.active) + self.assertTrue(s2 in sub_group.all_hosts.all()) + self.assertTrue(other_sub_group in sub_group.children.all()) + self.assertFalse(other_top_group.active) def test_group_parents_and_children(self): # Test for various levels of group parent/child relations, with hosts,