From 25f0d65c5ff639bf51eeb6dc613cb928beb58d73 Mon Sep 17 00:00:00 2001 From: Akita Noek Date: Thu, 14 Apr 2016 21:56:10 -0400 Subject: [PATCH] Ancestor rebuild optimization progress --- .../management/commands/inventory_import.py | 73 +++--- awx/main/models/rbac.py | 225 +++++++++++++++++- awx/main/tests/functional/test_rbac_core.py | 98 +++++++- .../tests/old/commands/commands_monolithic.py | 2 +- config/awx-munin.conf | 1 - 5 files changed, 349 insertions(+), 50 deletions(-) diff --git a/awx/main/management/commands/inventory_import.py b/awx/main/management/commands/inventory_import.py index 91b3a0a544..47d20cc1dd 100644 --- a/awx/main/management/commands/inventory_import.py +++ b/awx/main/management/commands/inventory_import.py @@ -780,10 +780,10 @@ class Command(NoArgsCommand): if settings.SQL_DEBUG: queries_before = len(connection.queries) if self.inventory_source.group: - groups_qs = self.inventory_source.group.all_children + groups_qs = self.inventory_source.group.all_children.all() # FIXME: Also include groups from inventory_source.managed_groups? else: - groups_qs = self.inventory.groups + groups_qs = self.inventory.groups.all() # Build list of all group pks, remove those that should not be deleted. del_group_pks = set(groups_qs.values_list('pk', flat=True)) all_group_names = self.all_group.all_groups.keys() @@ -1273,42 +1273,43 @@ class Command(NoArgsCommand): self.is_custom) self.all_group.debug_tree() - # Ensure that this is managed as an atomic SQL transaction, - # and thus properly rolled back if there is an issue. - with transaction.atomic(): - # Merge/overwrite inventory into database. - if settings.SQL_DEBUG: - self.logger.warning('loading into database...') - with ignore_inventory_computed_fields(): - if getattr(settings, 'ACTIVITY_STREAM_ENABLED_FOR_INVENTORY_SYNC', True): - self.load_into_database() - else: - with disable_activity_stream(): + with batch_role_ancestor_rebuilding(): + # Ensure that this is managed as an atomic SQL transaction, + # and thus properly rolled back if there is an issue. + with transaction.atomic(): + # Merge/overwrite inventory into database. + if settings.SQL_DEBUG: + self.logger.warning('loading into database...') + with ignore_inventory_computed_fields(): + if getattr(settings, 'ACTIVITY_STREAM_ENABLED_FOR_INVENTORY_SYNC', True): self.load_into_database() - if settings.SQL_DEBUG: - queries_before2 = len(connection.queries) - self.inventory.update_computed_fields() - if settings.SQL_DEBUG: - self.logger.warning('update computed fields took %d queries', - len(connection.queries) - queries_before2) - try: - self.check_license() - except CommandError as e: - self.mark_license_failure(save=True) - raise e + else: + with disable_activity_stream(): + self.load_into_database() + if settings.SQL_DEBUG: + queries_before2 = len(connection.queries) + self.inventory.update_computed_fields() + if settings.SQL_DEBUG: + self.logger.warning('update computed fields took %d queries', + len(connection.queries) - queries_before2) + try: + self.check_license() + except CommandError as e: + self.mark_license_failure(save=True) + raise e - if self.inventory_source.group: - inv_name = 'group "%s"' % (self.inventory_source.group.name) - else: - inv_name = '"%s" (id=%s)' % (self.inventory.name, - self.inventory.id) - if settings.SQL_DEBUG: - self.logger.warning('Inventory import completed for %s in %0.1fs', - inv_name, time.time() - begin) - else: - self.logger.info('Inventory import completed for %s in %0.1fs', - inv_name, time.time() - begin) - status = 'successful' + if self.inventory_source.group: + inv_name = 'group "%s"' % (self.inventory_source.group.name) + else: + inv_name = '"%s" (id=%s)' % (self.inventory.name, + self.inventory.id) + if settings.SQL_DEBUG: + self.logger.warning('Inventory import completed for %s in %0.1fs', + inv_name, time.time() - begin) + else: + self.logger.info('Inventory import completed for %s in %0.1fs', + inv_name, time.time() - begin) + status = 'successful' # If we're in debug mode, then log the queries and time # used to do the operation. diff --git a/awx/main/models/rbac.py b/awx/main/models/rbac.py index 91e055f6eb..f2ff1956ca 100644 --- a/awx/main/models/rbac.py +++ b/awx/main/models/rbac.py @@ -7,7 +7,7 @@ import threading import contextlib # Django -from django.db import models, transaction +from django.db import models, transaction, connection from django.db.models import Q from django.db.models.aggregates import Max from django.core.urlresolvers import reverse @@ -65,9 +65,11 @@ def batch_role_ancestor_rebuilding(allow_nesting=False): if not batch_role_rebuilding: rebuild_set = getattr(tls, 'roles_needing_rebuilding') with transaction.atomic(): - for role in Role.objects.filter(id__in=list(rebuild_set)).all(): - # TODO: We can reduce this to one rebuild call with our new upcoming rebuild method.. do this - role.rebuild_role_ancestor_list() + Role._simultaneous_ancestry_rebuild(rebuild_set) + + #for role in Role.objects.filter(id__in=list(rebuild_set)).all(): + # # TODO: We can reduce this to one rebuild call with our new upcoming rebuild method.. do this + # role.rebuild_role_ancestor_list() delattr(tls, 'roles_needing_rebuilding') @@ -97,7 +99,6 @@ class Role(CommonModelNameNotUnique): def get_absolute_url(self): return reverse('api:role_detail', args=(self.pk,)) - def rebuild_role_ancestor_list(self): ''' Updates our `ancestors` map to accurately reflect all of the ancestors for a role @@ -115,12 +116,13 @@ class Role(CommonModelNameNotUnique): roles_needing_rebuilding.add(self.id) return - actual_ancestors = set(Role.objects.filter(id=self.id).values_list('parents__ancestors__id', flat=True)) - actual_ancestors.add(self.id) - if None in actual_ancestors: - actual_ancestors.remove(None) - stored_ancestors = set(self.ancestors.all().values_list('id', flat=True)) + #actual_ancestors = set(Role.objects.filter(id=self.id).values_list('parents__ancestors__id', flat=True)) + #actual_ancestors.add(self.id) + #if None in actual_ancestors: + # actual_ancestors.remove(None) + #stored_ancestors = set(self.ancestors.all().values_list('id', flat=True)) + ''' # If it differs, update, and then update all of our children if actual_ancestors != stored_ancestors: for id in actual_ancestors - stored_ancestors: @@ -130,6 +132,209 @@ class Role(CommonModelNameNotUnique): for child in self.children.all(): child.rebuild_role_ancestor_list() + ''' + + # If our role heirarchy hasn't actually changed, don't do anything + #if actual_ancestors == stored_ancestors: + # return + + Role._simultaneous_ancestry_rebuild([self.id]) + + + @staticmethod + def _simultaneous_ancestry_rebuild2(role_ids_to_rebuild): + #all_parents = Role.parents.through.values_list('to_role_id', 'from_role_id') + + ''' + sql_params = { + 'ancestors_table': Role.ancestors.through._meta.db_table, + 'parents_table': Role.parents.through._meta.db_table, + 'roles_table': Role._meta.db_table, + 'ids': ','.join(str(x) for x in role_ids_to_rebuild) + } + ''' + #Expand our parent list + #Expand our child list + #Construct our ancestor list for + #apply diff + + + + @staticmethod + def _simultaneous_ancestry_rebuild(role_ids_to_rebuild): + # + # The simple version of what this function is doing + # ================================================= + # + # When something changes in our role "hierarchy", we need to update + # the `Role.ancestors` mapping to reflect these changes. The basic + # idea, which the code in this method is modeled after, is to do + # this: When a change happens to a role's parents list, we update + # that role's ancestry list, then we recursively update any child + # roles ancestry lists. Because our role relationships are not + # strictly hierarchical, and can even have loops, this process may + # necessarily visit the same nodes more than once. To handle this + # without having to keep track of what should be updated (again) and + # in what order, we simply use the termination condition of stopping + # when our stored ancestry list matches what our list should be, eg, + # when nothing changes. This can be simply implemented: + # + # if actual_ancestors != stored_ancestors: + # for id in actual_ancestors - stored_ancestors: + # self.ancestors.add(id) + # for id in stored_ancestors - actual_ancestors: + # self.ancestors.remove(id) + # + # for child in self.children.all(): + # child.rebuild_role_ancestor_list() + # + # However this results in a lot of calls to the database, so the + # optimized implementation below effectively does this same thing, + # but we update all children at once, so effectively we sweep down + # through our hierarchy one layer at a time instead of one node at a + # time. Because of how this method works, we can also start from many + # roots at once and sweep down a large set of roles, which we take + # advantage of when performing bulk operations. + # + # + # SQL Breakdown + # ============= + # The Role ancestors has three columns, (id, from_role_id, to_role_id) + # + # id: Unqiue row ID + # from_role_id: Descendent role ID + # to_role_id: Ancestor role ID + # + # *NOTE* In addition to mapping roles to parents, there also + # always exists must exist an entry where + # + # from_role_id == role_id == to_role_id + # + # this makes our joins simple when we go to derive permissions or + # accessible objects. + # + # + # We operate under the assumption that our parent's ancestor list is + # correct, thus we can always compute what our ancestor list should + # be by taking the union of our parent's ancestor lists and adding + # our self reference entry from_role_id == role_id == to_role_id + # + # The inner query for the two SQL statements compute this union, + # the union of the parent's ancestors and the self referncing entry, + # for all roles in the current set of roles to rebuild. + # + # The DELETE query uses this to select all entries on disk for the + # roles we're dealing with, and removes the entries that are not in + # this list. + # + # The INSERT query uses this to select all entries in the list that + # are not in the database yet, and inserts all of the missing + # records. + # + # Once complete, we select all of the children for the roles we are + # working with, this list becomes the new role list we are working + # with. + # + # When our delete or insert query return that they have not performed + # any work, then we know that our children will also not need to be + # updated, and so we can terminate our loop. + # + # + # *NOTE* Keen reader may realize that there are many instances where + # fuck. cycles will never shake parents. + # + # + + cursor = connection.cursor() + loop_ct = 0 + + sql_params = { + 'ancestors_table': Role.ancestors.through._meta.db_table, + 'parents_table': Role.parents.through._meta.db_table, + 'roles_table': Role._meta.db_table, + 'ids': ','.join(str(x) for x in role_ids_to_rebuild) + } + + # This is our solution for dealing with updates to parents of nodes + # that are in cycles. It seems like we should be able to find a more + # clever way of just dealing with the issue in another way, but it's + # surefire and I'm not seeing the easy solution to dealing with that + # problem that's not this. + + # TODO: Test to see if not deleting any entry that has a direct + # correponding entry in the parents table helps reduce the processing + # time significantly + cursor.execute(''' + DELETE FROM %(ancestors_table)s + WHERE to_role_id IN (%(ids)s) + AND from_role_id != to_role_id + ''' % sql_params) + + + while role_ids_to_rebuild: + if loop_ct > 1000: + raise Exception('Ancestry role rebuilding error: infinite loop detected') + loop_ct += 1 + + sql_params = { + 'ancestors_table': Role.ancestors.through._meta.db_table, + 'parents_table': Role.parents.through._meta.db_table, + 'roles_table': Role._meta.db_table, + 'ids': ','.join(str(x) for x in role_ids_to_rebuild) + } + + delete_ct = 0 + + cursor.execute(''' + DELETE FROM %(ancestors_table)s + WHERE from_role_id IN (%(ids)s) + AND + id NOT IN ( + SELECT %(ancestors_table)s.id FROM ( + SELECT parents.from_role_id from_id, ancestors.to_role_id to_id + FROM %(parents_table)s as parents + LEFT JOIN %(ancestors_table)s as ancestors + ON (parents.to_role_id = ancestors.from_role_id) + WHERE parents.from_role_id IN (%(ids)s) AND ancestors.to_role_id IS NOT NULL + + UNION + + SELECT id from_id, id to_id from %(roles_table)s WHERE id IN (%(ids)s) + ) new_ancestry_list + LEFT JOIN %(ancestors_table)s ON (new_ancestry_list.from_id = %(ancestors_table)s.from_role_id + AND new_ancestry_list.to_id = %(ancestors_table)s.to_role_id) + WHERE %(ancestors_table)s.id IS NOT NULL + ) + ''' % sql_params) + delete_ct = cursor.rowcount + + cursor.execute(''' + INSERT INTO %(ancestors_table)s (from_role_id, to_role_id) + SELECT from_id, to_id FROM ( + SELECT parents.from_role_id from_id, ancestors.to_role_id to_id + FROM %(parents_table)s as parents + LEFT JOIN %(ancestors_table)s as ancestors + ON (parents.to_role_id = ancestors.from_role_id) + WHERE parents.from_role_id IN (%(ids)s) AND ancestors.to_role_id IS NOT NULL + + UNION + + SELECT id from_id, id to_id from %(roles_table)s WHERE id IN (%(ids)s) + ) new_ancestry_list + LEFT JOIN %(ancestors_table)s ON (new_ancestry_list.from_id = %(ancestors_table)s.from_role_id + AND new_ancestry_list.to_id = %(ancestors_table)s.to_role_id) + WHERE %(ancestors_table)s.id IS NULL + ''' % sql_params) + insert_ct = cursor.rowcount + + if insert_ct == 0 and delete_ct == 0: + break + + role_ids_to_rebuild = Role.objects.distinct() \ + .filter(id__in=role_ids_to_rebuild, children__id__isnull=False) \ + .values_list('children__id', flat=True) + + @staticmethod def visible_roles(user): diff --git a/awx/main/tests/functional/test_rbac_core.py b/awx/main/tests/functional/test_rbac_core.py index 2ad1250f81..3ef1bd477d 100644 --- a/awx/main/tests/functional/test_rbac_core.py +++ b/awx/main/tests/functional/test_rbac_core.py @@ -168,8 +168,8 @@ def test_content_object(user): assert org.admin_role.content_object.id == org.id @pytest.mark.django_db -def test_hierarchy_rebuilding(): - 'Tests some subdtle cases around role hierarchy rebuilding' +def test_hierarchy_rebuilding_multi_path(): + 'Tests a subdtle cases around role hierarchy rebuilding when you have multiple paths to the same role of different length' X = Role.objects.create(name='X') A = Role.objects.create(name='A') @@ -196,6 +196,100 @@ def test_hierarchy_rebuilding(): assert X.is_ancestor_of(D) is False +@pytest.mark.django_db +def test_hierarchy_rebuilding_loops1(organization, team): + 'Tests ancestry rebuilding loops are involved' + + + assert team.admin_role.is_ancestor_of(organization.admin_role) is False + assert organization.admin_role.is_ancestor_of(team.admin_role) + + team.admin_role.children.add(organization.admin_role) + + assert team.admin_role.is_ancestor_of(organization.admin_role) + assert organization.admin_role.is_ancestor_of(team.admin_role) + + team.admin_role.children.remove(organization.admin_role) + + assert team.admin_role.is_ancestor_of(organization.admin_role) is False + assert organization.admin_role.is_ancestor_of(team.admin_role) + + team.admin_role.children.add(organization.admin_role) + + + X = Role.objects.create(name='X') + X.children.add(organization.admin_role) + assert X.is_ancestor_of(team.admin_role) + assert X.is_ancestor_of(organization.admin_role) + assert organization.admin_role.is_ancestor_of(X) is False + assert team.admin_role.is_ancestor_of(X) is False + + #print(X.descendents.filter(id=organization.admin_role.id).count()) + #print(X.children.filter(id=organization.admin_role.id).count()) + X.children.remove(organization.admin_role) + X.rebuild_role_ancestor_list() + #print(X.descendents.filter(id=organization.admin_role.id).count()) + #print(X.children.filter(id=organization.admin_role.id).count()) + + assert X.is_ancestor_of(team.admin_role) is False + assert X.is_ancestor_of(organization.admin_role) is False + + + +@pytest.mark.django_db +def test_hierarchy_rebuilding_loops(): + 'Tests ancestry rebuilding loops are involved' + + X = Role.objects.create(name='X') + A = Role.objects.create(name='A') + B = Role.objects.create(name='B') + C = Role.objects.create(name='C') + + A.children.add(B) + B.children.add(C) + C.children.add(A) + X.children.add(A) + + assert X.is_ancestor_of(A) + assert X.is_ancestor_of(B) + assert X.is_ancestor_of(C) + + assert A.is_ancestor_of(B) + assert A.is_ancestor_of(C) + assert B.is_ancestor_of(C) + assert B.is_ancestor_of(A) + assert C.is_ancestor_of(A) + assert C.is_ancestor_of(B) + + X.children.remove(A) + X.rebuild_role_ancestor_list() + + assert X.is_ancestor_of(A) is False + assert X.is_ancestor_of(B) is False + assert X.is_ancestor_of(C) is False + + X.children.add(A) + + assert X.is_ancestor_of(A) + assert X.is_ancestor_of(B) + assert X.is_ancestor_of(C) + + C.children.remove(A) + + assert A.is_ancestor_of(B) + assert A.is_ancestor_of(C) + assert B.is_ancestor_of(C) + assert B.is_ancestor_of(A) is False + assert C.is_ancestor_of(A) is False + assert C.is_ancestor_of(B) is False + + assert X.is_ancestor_of(A) + assert X.is_ancestor_of(B) + assert X.is_ancestor_of(C) + + + + @pytest.mark.django_db def test_auto_parenting(): org1 = Organization.objects.create(name='org1') diff --git a/awx/main/tests/old/commands/commands_monolithic.py b/awx/main/tests/old/commands/commands_monolithic.py index 869b530ac9..153d1be27d 100644 --- a/awx/main/tests/old/commands/commands_monolithic.py +++ b/awx/main/tests/old/commands/commands_monolithic.py @@ -986,7 +986,7 @@ class InventoryImportTest(BaseCommandMixin, BaseLiveServerTest): self.assertEqual(new_inv.groups.count(), ngroups) self.assertEqual(new_inv.total_hosts, nhosts) self.assertEqual(new_inv.total_groups, ngroups) - self.assertElapsedLessThan(1200) # FIXME: This should be < 120, will drop back down next sprint during our performance tuning work - anoek 2016-03-22 + self.assertElapsedLessThan(120) @unittest.skipIf(getattr(settings, 'LOCAL_DEVELOPMENT', False), 'Skip this test in local development environments, ' diff --git a/config/awx-munin.conf b/config/awx-munin.conf index 833a6f36bf..ca4f4c39ad 100644 --- a/config/awx-munin.conf +++ b/config/awx-munin.conf @@ -9,4 +9,3 @@ Alias /munin /var/www/html/munin/ require valid-user -ScriptAlias /munin-cgi/munin-cgi-graph /var/www/cgi-bin/munin-cgi-graph \ No newline at end of file