From bf4aed3d8cc24ec116a7a58b0f096b7ee48d4cb1 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 21 Nov 2024 14:25:38 -0800 Subject: [PATCH] Allow dependent jobs to force dependencies to run If a two jobs (let's call them "parent" and "child") have file matchers and child has a hard dependency on parent, if parent does not match the files in the change, an exception will be raised while freezing the job graph. The current best mitigation for this is to ensure that the file matches for such parent-child job pairs match in such a way that parent will always run if child does. This fix does not work if a user changes the definition of child without changing the definition of parent. In this case, the file matchers for child will be disabled, but not parent, and the error will be raised. The only way to push through such a change would be to also make a no-op change to parent. This is inconvenient, and as long as we're thinking along those lines, the ordinary situation with file matchers is also inconevient. Make both situations easier by ignoring the results of file matchers for hard dependencies. This behavior should be more intuitive, and will simplify file matchers (in many cases, it may now be possible to configure a job to effectively never match a change, and it will then only run if there is a dependent job that does run). This does not apply to soft dependencies. Change-Id: I7df2262881cb952b46b8a99ad5ccbe912c2e070b --- .../run-dependencies-81844e46fc295843.yaml | 15 +++ tests/unit/test_model.py | 24 +++-- tests/unit/test_scheduler.py | 36 +++++++ zuul/model.py | 96 +++++++++++++++---- 4 files changed, 141 insertions(+), 30 deletions(-) create mode 100644 releasenotes/notes/run-dependencies-81844e46fc295843.yaml diff --git a/releasenotes/notes/run-dependencies-81844e46fc295843.yaml b/releasenotes/notes/run-dependencies-81844e46fc295843.yaml new file mode 100644 index 0000000000..f0eef37ea3 --- /dev/null +++ b/releasenotes/notes/run-dependencies-81844e46fc295843.yaml @@ -0,0 +1,15 @@ +--- +features: + - | + Zuul will now run any job that is a hard dependency of a job that + is set to run. + + Previously if job B had a hard dependency on job A, and job A did + not run due to a file matcher, Zuul would report an error. Now it + will instead ignore the file matcher on job A as long as job B is + to be run. + + This will allow for simpler file matchers, more intuitive + behavior, and enable a new pattern for job graphs where job A + might have a file matcher that never matches, but job B will cause + it to run regardless. diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index ddcc083657..b3eb9da90b 100644 --- a/tests/unit/test_model.py +++ b/tests/unit/test_model.py @@ -912,6 +912,10 @@ class FakeFrozenJob(model.Job): self.uuid = uuid.uuid4().hex self.ref = 'fake reference' self.all_refs = [self.ref] + self.matches_change = True + + def _set(self, **kw): + self.__dict__.update(kw) class TestGraph(BaseTestCase): @@ -943,7 +947,7 @@ class TestGraph(BaseTestCase): with testtools.ExpectedException( Exception, "Dependency cycle detected in job jobX"): - graph.freezeDependencies() + graph.freezeDependencies(self.log) # Disallow circular dependencies graph = setup_graph() @@ -954,7 +958,7 @@ class TestGraph(BaseTestCase): with testtools.ExpectedException( Exception, "Dependency cycle detected in job job3"): - graph.freezeDependencies() + graph.freezeDependencies(self.log) graph = setup_graph() jobs[3].dependencies = frozenset([model.JobDependency(jobs[5].name)]) @@ -967,7 +971,7 @@ class TestGraph(BaseTestCase): with testtools.ExpectedException( Exception, "Dependency cycle detected in job job3"): - graph.freezeDependencies() + graph.freezeDependencies(self.log) graph = setup_graph() jobs[3].dependencies = frozenset([model.JobDependency(jobs[2].name)]) @@ -978,7 +982,7 @@ class TestGraph(BaseTestCase): graph.addJob(jobs[5]) jobs[6].dependencies = frozenset([model.JobDependency(jobs[2].name)]) graph.addJob(jobs[6]) - graph.freezeDependencies() + graph.freezeDependencies(self.log) def test_job_graph_allows_soft_dependencies(self): parent = FakeFrozenJob('parent') @@ -990,14 +994,14 @@ class TestGraph(BaseTestCase): graph = model.JobGraph({}) graph.addJob(parent) graph.addJob(child) - graph.freezeDependencies() + graph.freezeDependencies(self.log) self.assertEqual(graph.getParentJobsRecursively(child), [parent]) # Skip the parent graph = model.JobGraph({}) graph.addJob(child) - graph.freezeDependencies() + graph.freezeDependencies(self.log) self.assertEqual(graph.getParentJobsRecursively(child), []) def test_job_graph_allows_soft_dependencies4(self): @@ -1018,7 +1022,7 @@ class TestGraph(BaseTestCase): for j in parents: graph.addJob(j) graph.addJob(child) - graph.freezeDependencies() + graph.freezeDependencies(self.log) self.assertEqual(set(graph.getParentJobsRecursively(child)), set(parents)) @@ -1028,7 +1032,7 @@ class TestGraph(BaseTestCase): if j is not parents[0]: graph.addJob(j) graph.addJob(child) - graph.freezeDependencies() + graph.freezeDependencies(self.log) self.assertEqual(set(graph.getParentJobsRecursively(child)), set(parents) - set([parents[0], parents[2], parents[3]])) @@ -1039,7 +1043,7 @@ class TestGraph(BaseTestCase): if j is not parents[3]: graph.addJob(j) graph.addJob(child) - graph.freezeDependencies() + graph.freezeDependencies(self.log) self.assertEqual(set(graph.getParentJobsRecursively(child)), set(parents) - set([parents[3]])) @@ -1068,7 +1072,7 @@ class TestGraph(BaseTestCase): graph.addJob(child) # We don't expect this to raise an exception - graph.freezeDependencies() + graph.freezeDependencies(self.log) class TestTenant(BaseTestCase): diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 54be60c56d..2aeb4a1cb8 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -7100,6 +7100,42 @@ class TestJobUpdateFileMatcher(ZuulTestCase): self.assertHistory([]) + def test_job_dependencies_update(self): + "Test that a dependent job will run when depending job updates" + in_repo_conf = textwrap.dedent( + """ + # Same + - job: + name: existing-files + files: + - README.txt + + # Changed + - job: + name: existing-irr + files: + - README.txt + + - project: + name: org/project + check: + jobs: + - existing-files + - existing-irr: + dependencies: [existing-files] + """) + + file_dict = {'zuul.d/existing.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A', + files=file_dict) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertHistory([ + dict(name='existing-files', result='SUCCESS', changes='1,1'), + dict(name='existing-irr', result='SUCCESS', changes='1,1'), + ], ordered=False) + def test_new_job(self): "Test matchers are overridden when creating a new job" in_repo_conf = textwrap.dedent( diff --git a/zuul/model.py b/zuul/model.py index 832260dcf4..548814a8a1 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -3107,6 +3107,8 @@ class FrozenJob(zkobject.ZKObject): ref=None, other_refs=[], image_build_name=None, + # Not serialized + matches_change=True, ) def __repr__(self): @@ -3648,8 +3650,12 @@ class Job(ConfigObject): """ project_canonical_names = set() project_canonical_names.update(self.required_projects.keys()) + if self.run: + run = [self.run[0]] + else: + run = [] project_canonical_names.update(self._projectsFromPlaybooks( - itertools.chain(self.pre_run, [self.run[0]], self.post_run, + itertools.chain(self.pre_run, run, self.post_run, self.cleanup_run), with_implicit=True)) project_canonical_names.update({ iv.project_canonical_name @@ -4623,7 +4629,7 @@ class JobGraph(object): uuids_to_iterate.add((u, current_dependent_uuids[u]['soft'])) return [self.getJobFromUuid(u) for u in all_dependent_uuids] - def freezeDependencies(self, layout=None): + def freezeDependencies(self, log, layout=None): for dependent_uuid, parents in self._dependencies.items(): dependencies = self.job_dependencies.setdefault(dependent_uuid, {}) for parent_name, parent_soft in parents.items(): @@ -4645,6 +4651,14 @@ class JobGraph(object): raise JobConfigurationError( "Job %s depends on %s which was not run." % (dependent_job.name, parent_name)) + if not parent_soft: + # If this is a hard dependency, then tell the + # parent to ignore file matchers. + if not parent_job.matches_change: + log.debug( + "Forcing non-matching hard dependency " + "%s to run for %s", parent_job, dependent_job) + parent_job._set(matches_change=True) dependencies[parent_job.uuid] = dict(soft=parent_soft) dependents = self.job_dependents.setdefault( parent_job.uuid, {}) @@ -4685,6 +4699,35 @@ class JobGraph(object): return self.project_metadata[name] return None + def removeNonMatchingJobs(self, log): + # This is similar to what we do in freezeDependencies + for dependent_uuid, parents in self._dependencies.items(): + for parent_name, parent_soft in parents.items(): + dependent_job = self._job_map[dependent_uuid] + # We typically depend on jobs with the same ref, but + # if we have been deduplicated, then we depend on + # every job-ref for the given parent job. + for ref in dependent_job.all_refs: + parent_job = self.getJob(parent_name, ref) + if parent_job is None: + continue + if not parent_soft: + # If this is a hard dependency, then tell the + # parent to ignore file matchers. + if not parent_job.matches_change: + log.debug( + "Forcing non-matching hard dependency " + "%s to run for %s", parent_job, dependent_job) + parent_job._set(matches_change=True) + + # Afer removing duplicates and walking the dependency graph, + # remove any jobs that we shouldn't run because of file + # matchers. + for job in list(self._job_map.values()): + if not job.matches_change: + log.debug("Removing non-matching job %s", job) + self._removeJob(job) + def deduplicateJobs(self, log, item): # Jobs are deduplicated before they start, so returned data # are not considered at all. @@ -9254,7 +9297,7 @@ class Layout(object): def extendJobGraph(self, context, item, change, ppc, job_graph, skip_file_matcher, redact_secrets_and_keys, - debug_messages): + debug_messages, pending_errors): log = item.annotateLogger(self.log) semaphore_handler = item.pipeline.tenant.semaphore_handler job_list = ppc.job_list @@ -9353,6 +9396,7 @@ class Layout(object): final_job, change, self) else: matched_files = True + matches_change = True if not matched_files: if updates_job_config: # Log the reason we're ignoring the file matcher @@ -9369,31 +9413,37 @@ class Layout(object): add_debug_line(debug_messages, "Job {jobname} did not match files". format(jobname=jobname), indent=2) - continue + # A decision not to run based on a file matcher + # can be overridden later, so we just note our + # initial decision here. + matches_change = False + frozen_job = final_job.freezeJob( + context, self.tenant, self, item, change, + redact_secrets_and_keys) + frozen_job._set(matches_change=matches_change) + job_graph.addJob(frozen_job) + + # These are only errors if we actually decide to run the job if final_job.abstract: - raise JobConfigurationError( + pending_errors[frozen_job.uuid] = JobConfigurationError( "Job %s is abstract and may not be directly run" % (final_job.name,)) - if (not final_job.ignore_allowed_projects and - final_job.allowed_projects is not None and - change.project.name not in final_job.allowed_projects): - raise JobConfigurationError( + elif (not final_job.ignore_allowed_projects and + final_job.allowed_projects is not None and + change.project.name not in final_job.allowed_projects): + pending_errors[frozen_job.uuid] = JobConfigurationError( "Project %s is not allowed to run job %s" % (change.project.name, final_job.name)) - if ((not pipeline.post_review) and final_job.post_review): - raise JobConfigurationError( + elif ((not pipeline.post_review) and final_job.post_review): + pending_errors[frozen_job.uuid] = JobConfigurationError( "Pre-review pipeline %s does not allow " "post-review job %s" % ( pipeline.name, final_job.name)) - if not final_job.run: - raise JobConfigurationError( + elif not final_job.run: + pending_errors[frozen_job.uuid] = JobConfigurationError( "Job %s does not specify a run playbook" % ( final_job.name,)) - job_graph.addJob(final_job.freezeJob( - context, self.tenant, self, item, change, - redact_secrets_and_keys)) - def createJobGraph(self, context, item, skip_file_matcher, redact_secrets_and_keys, @@ -9412,6 +9462,7 @@ class Layout(object): else: job_map = item.current_build_set.jobs job_graph = JobGraph(job_map) + pending_errors = {} for change in item.changes: ppc = self.getProjectPipelineConfig(item, change) if not ppc: @@ -9422,14 +9473,19 @@ class Layout(object): debug_messages.extend(ppc.debug_messages) self.extendJobGraph( context, item, change, ppc, job_graph, skip_file_matcher, - redact_secrets_and_keys, debug_messages) + redact_secrets_and_keys, debug_messages, pending_errors) if ppc.fail_fast is not None: # Any explicit setting of fail_fast takes effect, # last one wins. fail_fast = ppc.fail_fast - job_graph.deduplicateJobs(self.log, item) - job_graph.freezeDependencies(self) + log = item.annotateLogger(self.log) + job_graph.deduplicateJobs(log, item) + job_graph.removeNonMatchingJobs(log) + job_graph.freezeDependencies(log, self) + for job in job_map.values(): + if job.uuid in pending_errors: + raise pending_errors[job.uuid] # Copy project metadata to job_graph since this must be independent # of the layout as we need it in order to prepare the context for