Limit search scope of getChangesDependingOn to tenant
In GitHub with many app installations the getChangesDependingOn currently iterates over all installations within the system and fires up a search query. In larger deployments this can sum up to hundreds of queries for a single parent-change-enqueued event. At least for multi-tenant deployments this can be greatly improved when limiting the scope just to the installations related to the tenant. With this improvement in most tenants this can be accomplished with a handful of requests then. Change-Id: Ibfad750a685d2ec58f3e452bfe2098bbdc294e37
This commit is contained in:
parent
124ada7dc8
commit
619e2fc904
0
tests/fixtures/config/zuultrigger/parent-change-enqueued-github/git/org2_project/README
vendored
Normal file
0
tests/fixtures/config/zuultrigger/parent-change-enqueued-github/git/org2_project/README
vendored
Normal file
7
tests/fixtures/config/zuultrigger/parent-change-enqueued-github/git/org2_project/zuul.yaml
vendored
Normal file
7
tests/fixtures/config/zuultrigger/parent-change-enqueued-github/git/org2_project/zuul.yaml
vendored
Normal file
@ -0,0 +1,7 @@
|
||||
- project:
|
||||
check:
|
||||
jobs:
|
||||
- project-check
|
||||
gate:
|
||||
jobs:
|
||||
- project-gate
|
@ -61,12 +61,3 @@
|
||||
- job:
|
||||
name: project-gate
|
||||
run: playbooks/project-gate.yaml
|
||||
|
||||
- project:
|
||||
name: org/project
|
||||
check:
|
||||
jobs:
|
||||
- project-check
|
||||
gate:
|
||||
jobs:
|
||||
- project-gate
|
||||
|
7
tests/fixtures/config/zuultrigger/parent-change-enqueued-github/git/org_project/zuul.yaml
vendored
Normal file
7
tests/fixtures/config/zuultrigger/parent-change-enqueued-github/git/org_project/zuul.yaml
vendored
Normal file
@ -0,0 +1,7 @@
|
||||
- project:
|
||||
check:
|
||||
jobs:
|
||||
- project-check
|
||||
gate:
|
||||
jobs:
|
||||
- project-gate
|
@ -6,3 +6,12 @@
|
||||
- org/common-config
|
||||
untrusted-projects:
|
||||
- org/project
|
||||
|
||||
- tenant:
|
||||
name: tenant-two
|
||||
source:
|
||||
github:
|
||||
config-projects:
|
||||
- org/common-config
|
||||
untrusted-projects:
|
||||
- org2/project
|
||||
|
@ -115,6 +115,13 @@ class TestZuulTriggerParentChangeEnqueuedGithub(ZuulGithubAppTestCase):
|
||||
B1.addLabel('for-check') # should go to check
|
||||
B2.addLabel('for-gate') # should go to gate
|
||||
|
||||
# In this case we have two installations
|
||||
# 1: org/common-config, org/project (used by tenant-one and tenant-two)
|
||||
# 2: org2/project (only used by tenant-two)
|
||||
# In order to track accesses to the installations enable client
|
||||
# recording in the fake github.
|
||||
self.fake_github.record_clients = True
|
||||
|
||||
self.fake_github.emitEvent(A.getReviewAddedEvent('approved'))
|
||||
# Jobs are being held in build to make sure that 3,1 has time
|
||||
# to enqueue behind 1,1 so that the test is more
|
||||
@ -138,6 +145,7 @@ class TestZuulTriggerParentChangeEnqueuedGithub(ZuulGithubAppTestCase):
|
||||
# Now directly enqueue a change into the check. As no pipeline reacts
|
||||
# on parent-change-enqueued from pipeline check no
|
||||
# parent-change-enqueued event is expected.
|
||||
self.waitUntilSettled()
|
||||
zuultrigger_event_count = 0
|
||||
|
||||
def counting_put(*args, **kwargs):
|
||||
@ -163,6 +171,13 @@ class TestZuulTriggerParentChangeEnqueuedGithub(ZuulGithubAppTestCase):
|
||||
self.assertEqual(len(self.history), 4)
|
||||
self.assertEqual(zuultrigger_event_count, 0)
|
||||
|
||||
# After starting recording installation containing org2/project
|
||||
# should not be contacted
|
||||
inst_id_to_check = self.fake_github.installation_map['org2/project']
|
||||
inst_clients = [x for x in self.fake_github.recorded_clients
|
||||
if x._inst_id == inst_id_to_check]
|
||||
self.assertEqual(len(inst_clients), 0)
|
||||
|
||||
|
||||
class TestZuulTriggerProjectChangeMerged(ZuulTestCase):
|
||||
|
||||
|
@ -72,7 +72,7 @@ class GerritSource(BaseSource):
|
||||
results[0]['number'], results[0]['currentPatchSet']['number'])
|
||||
return change
|
||||
|
||||
def getChangesDependingOn(self, change, projects):
|
||||
def getChangesDependingOn(self, change, projects, tenant):
|
||||
changes = []
|
||||
if not change.uris:
|
||||
return changes
|
||||
|
@ -41,7 +41,7 @@ class GitSource(BaseSource):
|
||||
def getChangeByURL(self, url):
|
||||
return None
|
||||
|
||||
def getChangesDependingOn(self, change, projects):
|
||||
def getChangesDependingOn(self, change, projects, tenant):
|
||||
return []
|
||||
|
||||
def getCachedChanges(self):
|
||||
|
@ -749,7 +749,7 @@ class GithubConnection(BaseConnection):
|
||||
raise
|
||||
return change
|
||||
|
||||
def getChangesDependingOn(self, change, projects):
|
||||
def getChangesDependingOn(self, change, projects, tenant):
|
||||
changes = []
|
||||
if not change.uris:
|
||||
return changes
|
||||
@ -774,13 +774,17 @@ class GithubConnection(BaseConnection):
|
||||
installation_projects.add(project.name)
|
||||
else:
|
||||
# We aren't in the context of a change queue and we just
|
||||
# need to query all installations. This currently only
|
||||
# happens if certain features of the zuul trigger are
|
||||
# need to query all installations of this tenant. This currently
|
||||
# only happens if certain features of the zuul trigger are
|
||||
# used; generally it should be avoided.
|
||||
for project, installation_id in self.installation_map.items():
|
||||
for project_name, installation_id in self.installation_map.items():
|
||||
trusted, project = tenant.getProject(project_name)
|
||||
# ignore projects from different tenants
|
||||
if not project:
|
||||
continue
|
||||
if installation_id not in installation_ids:
|
||||
installation_ids.add(installation_id)
|
||||
installation_projects.add(project)
|
||||
installation_projects.add(project_name)
|
||||
|
||||
keys = set()
|
||||
pattern = ' OR '.join(change.uris)
|
||||
|
@ -91,8 +91,8 @@ class GithubSource(BaseSource):
|
||||
patchset=pull.get('head').get('sha'))
|
||||
return change
|
||||
|
||||
def getChangesDependingOn(self, change, projects):
|
||||
return self.connection.getChangesDependingOn(change, projects)
|
||||
def getChangesDependingOn(self, change, projects, tenant):
|
||||
return self.connection.getChangesDependingOn(change, projects, tenant)
|
||||
|
||||
def getCachedChanges(self):
|
||||
return self.connection._change_cache.values()
|
||||
|
@ -65,7 +65,8 @@ class ZuulDriver(Driver, TriggerInterface):
|
||||
self.log.debug("onChangeEnqueued %s", tenant_events)
|
||||
if tenant_events:
|
||||
try:
|
||||
self._createParentChangeEnqueuedEvents(change, pipeline)
|
||||
self._createParentChangeEnqueuedEvents(
|
||||
change, pipeline, tenant)
|
||||
except Exception:
|
||||
self.log.exception(
|
||||
"Unable to create parent-change-enqueued events for "
|
||||
@ -90,7 +91,7 @@ class ZuulDriver(Driver, TriggerInterface):
|
||||
event.ref = change.ref
|
||||
self.sched.addEvent(event)
|
||||
|
||||
def _createParentChangeEnqueuedEvents(self, change, pipeline):
|
||||
def _createParentChangeEnqueuedEvents(self, change, pipeline, tenant):
|
||||
self.log.debug("Checking for changes needing %s:" % change)
|
||||
if not hasattr(change, 'needed_by_changes'):
|
||||
self.log.debug(" %s does not support dependencies" % type(change))
|
||||
@ -103,7 +104,7 @@ class ZuulDriver(Driver, TriggerInterface):
|
||||
for source in self.sched.connections.getSources():
|
||||
self.log.debug(" Checking source: %s", source)
|
||||
needed_by_changes.update(
|
||||
source.getChangesDependingOn(change, None))
|
||||
source.getChangesDependingOn(change, None, tenant))
|
||||
self.log.debug(" Following changes: %s", needed_by_changes)
|
||||
|
||||
for needs in needed_by_changes:
|
||||
|
@ -109,7 +109,8 @@ class DependentPipelineManager(PipelineManager):
|
||||
for source in sources:
|
||||
self.log.debug(" Checking source: %s", source)
|
||||
for c in source.getChangesDependingOn(change,
|
||||
change_queue.projects):
|
||||
change_queue.projects,
|
||||
self.pipeline.layout.tenant):
|
||||
if c not in seen:
|
||||
seen.add(c)
|
||||
needed_by_changes.append(c)
|
||||
|
@ -61,7 +61,7 @@ class BaseSource(object, metaclass=abc.ABCMeta):
|
||||
"""
|
||||
|
||||
@abc.abstractmethod
|
||||
def getChangesDependingOn(self, change, projects):
|
||||
def getChangesDependingOn(self, change, projects, tenant):
|
||||
"""Return changes which depend on changes at the supplied URIs.
|
||||
|
||||
Search this source for changes which depend on the supplied
|
||||
@ -72,6 +72,9 @@ class BaseSource(object, metaclass=abc.ABCMeta):
|
||||
If the projects argument is None, search across all known
|
||||
projects. If it is supplied, the search may optionally be
|
||||
restricted to only those projects.
|
||||
|
||||
The tenant argument can be used by the source to limit the
|
||||
search scope.
|
||||
"""
|
||||
|
||||
@abc.abstractmethod
|
||||
|
Loading…
Reference in New Issue
Block a user