From cb3cc6bd4b8a7f33bde81e8fb3088279afc03dfa Mon Sep 17 00:00:00 2001 From: Simon Westphahl Date: Fri, 25 Nov 2022 13:19:24 +0100 Subject: [PATCH] Consider queue settings for topic dependencies Most of a change's attributes are tenant-independent. This however is different for topic dependencies, which should only be considered in tenants where the dependencies-by-topic feature is enabled. This is mainly a problem when a project is part of multiple tenants as the dependencies-by-topic setting might be different for each tenant. To fix this we will only return the topic dependencies for a change in tenants where the feature has been activated. Since the `needs_changes` property is now a method called `getNeedsChanges()`, we also changed `needed_by_changes` to `getNeededByChanges()` so they match. Change-Id: I343306db0abbe2fbf98ddb3f81b6d509eaf4a2bf --- .../git/common-config/zuul.yaml | 19 ++++ .../git/common-tenant-two/playbooks/run.yaml | 1 + .../git/common-tenant-two/zuul.yaml | 96 +++++++++++++++++++ .../git/org_project5/README | 1 + .../git/org_project6/README | 1 + .../config/circular-dependencies/main.yaml | 6 ++ tests/unit/test_circular_dependencies.py | 49 ++++++++++ zuul/driver/zuul/__init__.py | 6 +- zuul/manager/__init__.py | 20 ++-- zuul/manager/dependent.py | 20 ++-- zuul/manager/independent.py | 8 +- zuul/model.py | 14 +-- zuul/reporter/__init__.py | 4 +- 13 files changed, 218 insertions(+), 27 deletions(-) create mode 100644 tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml create mode 100644 tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml create mode 100644 tests/fixtures/config/circular-dependencies/git/org_project5/README create mode 100644 tests/fixtures/config/circular-dependencies/git/org_project6/README diff --git a/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml index 44f26411ce..b36f71cc72 100644 --- a/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml +++ b/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml @@ -89,6 +89,12 @@ - job: name: project3-job +- job: + name: project5-job-t1 + +- job: + name: project6-job-t1 + - project: name: common-config queue: integrated @@ -144,3 +150,16 @@ gate: jobs: - project3-job + +- project: + name: org/project5 + queue: integrated + check: + jobs: + - project5-job-t1 + +- project: + name: org/project6 + check: + jobs: + - project6-job-t1 diff --git a/tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/playbooks/run.yaml @@ -0,0 +1 @@ +--- diff --git a/tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml new file mode 100644 index 0000000000..be30f91d52 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/common-tenant-two/zuul.yaml @@ -0,0 +1,96 @@ +- queue: + name: integrated + allow-circular-dependencies: true + dependencies-by-topic: true + +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + github: + - event: pull_request + action: + - opened + - changed + - reopened + - edited + success: + gerrit: + Verified: 1 + github: + status: success + failure: + gerrit: + Verified: -1 + github: + status: failure + +- pipeline: + name: gate + manager: dependent + success-message: Build succeeded (gate). + require: + gerrit: + approval: + - Approved: 1 + github: + label: approved + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + github: + - event: pull_request + action: edited + - event: pull_request + action: labeled + label: approved + success: + gerrit: + Verified: 2 + submit: true + github: + merge: true + failure: + gerrit: + Verified: -2 + github: {} + start: + gerrit: + Verified: 0 + github: {} + precedence: high + +- job: + name: base + parent: null + run: playbooks/run.yaml + +- job: + name: project5-job-t2 + +- job: + name: project6-job-t2 + +- project: + name: org/project5 + queue: integrated + check: + jobs: + - project5-job-t2 + gate: + jobs: + - project5-job-t2 + +- project: + name: org/project6 + queue: integrated + check: + jobs: + - project6-job-t2 + gate: + jobs: + - project6-job-t2 diff --git a/tests/fixtures/config/circular-dependencies/git/org_project5/README b/tests/fixtures/config/circular-dependencies/git/org_project5/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project5/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/org_project6/README b/tests/fixtures/config/circular-dependencies/git/org_project6/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project6/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/main.yaml b/tests/fixtures/config/circular-dependencies/main.yaml index 8083a582b3..d03b087f9f 100644 --- a/tests/fixtures/config/circular-dependencies/main.yaml +++ b/tests/fixtures/config/circular-dependencies/main.yaml @@ -10,6 +10,8 @@ - org/project1 - org/project2 - org/project3 + - org/project5 + - org/project6 github: untrusted-projects: - gh/project @@ -21,5 +23,9 @@ name: tenant-two source: gerrit: + config-projects: + - common-tenant-two untrusted-projects: - org/project4 + - org/project5 + - org/project6 diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py index 31fb4fd285..74d2cedf06 100644 --- a/tests/unit/test_circular_dependencies.py +++ b/tests/unit/test_circular_dependencies.py @@ -2283,6 +2283,55 @@ class TestGerritCircularDependencies(ZuulTestCase): dict(name="test-job", result="SUCCESS", changes="2,1 1,2"), ], ordered=False) + def test_deps_by_topic_multi_tenant(self): + A = self.fake_gerrit.addFakeChange('org/project5', "master", "A", + topic='test-topic') + B = self.fake_gerrit.addFakeChange('org/project6', "master", "B", + topic='test-topic') + + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueuing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([], max_age=0) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 4) + self.assertEqual(B.reported, 4) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + + self.assertHistory([ + # Check + dict(name="project5-job-t1", result="SUCCESS", changes="1,1"), + dict(name="project6-job-t1", result="SUCCESS", changes="2,1"), + dict(name="project5-job-t2", result="SUCCESS", changes="2,1 1,1"), + dict(name="project6-job-t2", result="SUCCESS", changes="1,1 2,1"), + # Gate + dict(name="project5-job-t2", result="SUCCESS", changes="1,1 2,1"), + dict(name="project6-job-t2", result="SUCCESS", changes="1,1 2,1"), + ], ordered=False) + class TestGithubCircularDependencies(ZuulTestCase): config_file = "zuul-gerrit-github.conf" diff --git a/zuul/driver/zuul/__init__.py b/zuul/driver/zuul/__init__.py index 6dd6ff1b94..5051f99e5b 100644 --- a/zuul/driver/zuul/__init__.py +++ b/zuul/driver/zuul/__init__.py @@ -23,6 +23,7 @@ from zuul.driver.zuul.zuulmodel import ZuulTriggerEvent from zuul.driver.zuul import zuulmodel from zuul.driver.zuul import zuultrigger from zuul.lib.logutil import get_annotated_logger +from zuul.model import Change PARENT_CHANGE_ENQUEUED = 'parent-change-enqueued' PROJECT_CHANGE_MERGED = 'project-change-merged' @@ -121,7 +122,7 @@ class ZuulDriver(Driver, TriggerInterface): log = get_annotated_logger(self.log, event) log.debug("Checking for changes needing %s:" % change) - if not hasattr(change, 'needed_by_changes'): + if not isinstance(change, Change): log.debug(" %s does not support dependencies" % type(change)) return @@ -129,7 +130,8 @@ class ZuulDriver(Driver, TriggerInterface): # numbers of github installations. This can be improved later # with persistent storage of dependency information. needed_by_changes = set( - pipeline.manager.resolveChangeReferences(change.needed_by_changes)) + pipeline.manager.resolveChangeReferences( + change.getNeededByChanges())) for source in self.sched.connections.getSources(): log.debug(" Checking source: %s", source.connection.connection_name) diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 538da2c104..c948997826 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -228,9 +228,16 @@ class PipelineManager(metaclass=ABCMeta): change = source.getChange(key) if change is None: self.log.error("Unable to resolve change from key %s", key) - if (isinstance(change, model.Change) - and change.commit_needs_changes is None): - self.updateCommitDependencies(change, None, event=None) + + if isinstance(change, model.Change): + update_commit_dependencies = ( + change.commit_needs_changes is None) + update_topic_dependencies = ( + change.topic_needs_changes is None + and self.useDependenciesByTopic(change.project)) + if (update_commit_dependencies + or update_topic_dependencies): + self.updateCommitDependencies(change, None, event=None) self._change_cache[change.cache_key] = change resolved_changes.append(change) return resolved_changes @@ -246,8 +253,9 @@ class PipelineManager(metaclass=ABCMeta): active_layout_uuids.add(item.layout_uuid) if isinstance(item.change, model.Change): - referenced_change_keys.update(item.change.needs_changes) - referenced_change_keys.update(item.change.needed_by_changes) + referenced_change_keys.update(item.change.getNeedsChanges( + self.useDependenciesByTopic(item.change.project))) + referenced_change_keys.update(item.change.getNeededByChanges()) # Clean up unused layouts in the cache unused_layouts = set(self._layout_cache.keys()) - active_layout_uuids @@ -574,7 +582,7 @@ class PipelineManager(metaclass=ABCMeta): return True cycle = [] - if hasattr(change, "needs_changes"): + if isinstance(change, model.Change): cycle = self.cycleForChange(change, dependency_graph, event) if cycle and not self.canProcessCycle(change.project): log.info("Dequeing change %s since at least one project " diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index f84f52dad5..4af541dfd7 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -80,7 +80,7 @@ class DependentPipelineManager(SharedQueuePipelineManager): history = history if history is not None else [] log.debug("Checking for changes needing %s:" % change) - if not hasattr(change, 'needed_by_changes'): + if not isinstance(change, model.Change): log.debug(" %s does not support dependencies" % type(change)) return @@ -90,7 +90,7 @@ class DependentPipelineManager(SharedQueuePipelineManager): sources = {p.source for p in projects} needed_by_changes = self.resolveChangeReferences( - change.needed_by_changes) + change.getNeededByChanges()) log.debug(" Previously known following changes: %s", needed_by_changes) seen = set(needed_by_changes) @@ -184,10 +184,11 @@ class DependentPipelineManager(SharedQueuePipelineManager): # Return true if okay to proceed enqueing this change, # false if the change should not be enqueued. log.debug("Checking for changes needed by %s:" % change) - if not hasattr(change, 'needs_changes'): + if not isinstance(change, model.Change): log.debug(" %s does not support dependencies", type(change)) return False, [] - if not change.needs_changes: + if not change.getNeedsChanges( + self.useDependenciesByTopic(change.project)): log.debug(" No changes needed") return False, [] changes_needed = [] @@ -195,7 +196,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): # Ignore supplied change_queue with self.getChangeQueue(change, event) as change_queue: for needed_change in self.resolveChangeReferences( - change.needs_changes): + change.getNeedsChanges( + self.useDependenciesByTopic(change.project))): log.debug(" Change %s needs change %s:" % ( change, needed_change)) if needed_change.is_merged: @@ -245,13 +247,15 @@ class DependentPipelineManager(SharedQueuePipelineManager): return abort, changes_needed def getFailingDependentItems(self, item, nnfi): - if not hasattr(item.change, 'needs_changes'): + if not isinstance(item.change, model.Change): return None - if not item.change.needs_changes: + if not item.change.getNeedsChanges( + self.useDependenciesByTopic(item.change.project)): return None failing_items = set() for needed_change in self.resolveChangeReferences( - item.change.needs_changes): + item.change.getNeedsChanges( + self.useDependenciesByTopic(item.change.project))): needed_item = self.getItemForChange(needed_change) if not needed_item: continue diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index b70e9184b7..aeeff5adcc 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -82,16 +82,18 @@ class IndependentPipelineManager(PipelineManager): log.debug("Checking for changes needed by %s:" % change) # Return true if okay to proceed enqueing this change, # false if the change should not be enqueued. - if not hasattr(change, 'needs_changes'): + if not isinstance(change, model.Change): log.debug(" %s does not support dependencies" % type(change)) return False, [] - if not change.needs_changes: + if not change.getNeedsChanges( + self.useDependenciesByTopic(change.project)): log.debug(" No changes needed") return False, [] changes_needed = [] abort = False for needed_change in self.resolveChangeReferences( - change.needs_changes): + change.getNeedsChanges( + self.useDependenciesByTopic(change.project))): log.debug(" Change %s needs change %s:" % ( change, needed_change)) if needed_change.is_merged: diff --git a/zuul/model.py b/zuul/model.py index c2da4f65cf..3658a02897 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -6162,18 +6162,17 @@ class Change(Branch): return True return False - @property - def needs_changes(self): + def getNeedsChanges(self, include_topic_needs=False): commit_needs_changes = self.commit_needs_changes or [] - topic_needs_changes = self.topic_needs_changes or [] + topic_needs_changes = ( + include_topic_needs and self.topic_needs_changes) or [] r = OrderedDict() for x in (self.git_needs_changes + self.compat_needs_changes + commit_needs_changes + topic_needs_changes): r[x] = None return tuple(r.keys()) - @property - def needed_by_changes(self): + def getNeededByChanges(self): r = OrderedDict() for x in (self.git_needed_by_changes + self.compat_needed_by_changes): r[x] = None @@ -6197,8 +6196,9 @@ class Change(Branch): so far. Will be updated with additional changes by this method. """ related.add(self.cache_stat.key) - for reference in itertools.chain(self.needs_changes, - self.needed_by_changes): + for reference in itertools.chain( + self.getNeedsChanges(include_topic_needs=True), + self.getNeededByChanges()): key = ChangeKey.fromReference(reference) if key not in related: source = sched.connections.getSource(key.connection_name) diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py index 9a868eac41..090bbfbb8d 100644 --- a/zuul/reporter/__init__.py +++ b/zuul/reporter/__init__.py @@ -261,7 +261,9 @@ class BaseReporter(object, metaclass=abc.ABCMeta): def _formatItemReportOtherBundleItems(self, item): related_changes = item.pipeline.manager.resolveChangeReferences( - item.change.needs_changes) + item.change.getNeedsChanges( + item.pipeline.manager.useDependenciesByTopic( + item.change.project))) return "Related changes:\n{}\n".format("\n".join( f' - {c.url}' for c in related_changes if c is not item.change))