From 082ff730fef20fac8fce50d3ea237c05f52fbbf5 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 1 Mar 2022 09:22:01 -0800 Subject: [PATCH] Dequeue items that can no longer merge We do not allow items to be enqueued into dependent pipelines if they do not meet the approval/mergability requirements of the code review system. For example, an "Approved" review in gerrit, or a required status check in github. However, we do not check those once the item has been enqueued. This means that if someone revokes approval, the item may remain in the queue until it finishes, at which point it will fail to merge and be dequeued. To avoid this, perform the canMerge check (which operates on local data) each time we process the item. Note: this does not perform Zuul pipeline requirement checks, but that would be a natural follow-on from this change. Change-Id: Iaf0ff530a9cb70052bf6a0908b28e2794dd110ae --- doc/source/developer/model-changelog.rst | 8 +++ ...ueue-on-missing-reqs-897210de746b3585.yaml | 6 +++ tests/unit/test_github_driver.py | 49 +++++++++++++++++ tests/unit/test_model_upgrade.py | 52 +++++++++++++++++++ tests/unit/test_scheduler.py | 29 +++++++++++ zuul/manager/__init__.py | 21 +++++++- zuul/model.py | 14 ++++- zuul/reporter/__init__.py | 3 ++ 8 files changed, 179 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/dequeue-on-missing-reqs-897210de746b3585.yaml diff --git a/doc/source/developer/model-changelog.rst b/doc/source/developer/model-changelog.rst index 3a471fd55d..bf638a6625 100644 --- a/doc/source/developer/model-changelog.rst +++ b/doc/source/developer/model-changelog.rst @@ -46,3 +46,11 @@ Version 3 :Description: Add a new `SupercedeEvent` and use that for dequeuing of superceded items from other pipelines. This only affects the schedulers. + +Version 4 +--------- + +:Prior Zuul version: 5.1.0 +:Description: Adds QueueItem.dequeued_missing_requirements and sets it to True + if a change no longer meets merge requirements in dependent + pipelines. This only affects schedulers. diff --git a/releasenotes/notes/dequeue-on-missing-reqs-897210de746b3585.yaml b/releasenotes/notes/dequeue-on-missing-reqs-897210de746b3585.yaml new file mode 100644 index 0000000000..93e2e7f610 --- /dev/null +++ b/releasenotes/notes/dequeue-on-missing-reqs-897210de746b3585.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Changes in dependent queues are now automatically dequeued if they + no longer meet the mergability requirements set by the upstream + code review system. diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 80e99cbc90..15f3489782 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -1109,6 +1109,55 @@ class TestGithubDriver(ZuulTestCase): # the change should have entered the gate self.assertEqual(2, len(self.history)) + @simple_layout('layouts/gate-github.yaml', driver='github') + def test_status_checks_removal(self): + github = self.fake_github.getGithubClient() + repo = github.repo_from_project('org/project') + repo._set_branch_protection( + 'master', contexts=['something/check', 'tenant-one/gate']) + + A = self.fake_github.openFakePullRequest('org/project', 'master', 'A') + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = True + # Since the required status 'something/check' is not fulfilled, + # no job is expected + self.assertEqual(0, len(self.builds)) + self.assertEqual(0, len(self.history)) + + # Set the required status 'something/check' + repo.create_status(A.head_sha, 'success', 'example.com', 'description', + 'something/check') + + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + self.assertEqual(2, len(self.builds)) + self.assertEqual(0, len(self.history)) + + # Remove it and verify the change is dequeued. + repo.create_status(A.head_sha, 'failed', 'example.com', 'description', + 'something/check') + self.fake_github.emitEvent(A.getCommitStatusEvent('something/check', + state='failed', + user='foo')) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + # The change should be dequeued. + self.assertHistory([ + dict(name='project-test1', result='ABORTED'), + dict(name='project-test2', result='ABORTED'), + ], ordered=False) + self.assertEqual(1, len(A.comments)) + self.assertFalse(A.is_merged) + self.assertIn('This change is unable to merge ' + 'due to a missing requirement.', + A.comments[0]) + # This test case verifies that no reconfiguration happens if a branch was # deleted that didn't contain configuration. @simple_layout('layouts/basic-github.yaml', driver='github') diff --git a/tests/unit/test_model_upgrade.py b/tests/unit/test_model_upgrade.py index 9bbaf12f70..eae37ff089 100644 --- a/tests/unit/test_model_upgrade.py +++ b/tests/unit/test_model_upgrade.py @@ -193,3 +193,55 @@ class TestSemaphoreModelUpgrade(ZuulTestCase): self.assertEqual( len(tenant.semaphore_handler.semaphoreHolders("test-semaphore")), 0) + + +class TestGithubModelUpgrade(ZuulTestCase): + config_file = 'zuul-github-driver.conf' + scheduler_count = 1 + + @model_version(3) + @simple_layout('layouts/gate-github.yaml', driver='github') + def test_status_checks_removal(self): + # This tests the old behavior -- that changes are not dequeued + # once their required status checks are removed -- since the + # new behavior requires a flag in ZK. + # Contrast with test_status_checks_removal. + github = self.fake_github.getGithubClient() + repo = github.repo_from_project('org/project') + repo._set_branch_protection( + 'master', contexts=['something/check', 'tenant-one/gate']) + + A = self.fake_github.openFakePullRequest('org/project', 'master', 'A') + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = True + # Since the required status 'something/check' is not fulfilled, + # no job is expected + self.assertEqual(0, len(self.history)) + + # Set the required status 'something/check' + repo.create_status(A.head_sha, 'success', 'example.com', 'description', + 'something/check') + + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + # Remove it and verify the change is not dequeued (old behavior). + repo.create_status(A.head_sha, 'failed', 'example.com', 'description', + 'something/check') + self.fake_github.emitEvent(A.getCommitStatusEvent('something/check', + state='failed', + user='foo')) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + # the change should have entered the gate + self.assertHistory([ + dict(name='project-test1', result='SUCCESS'), + dict(name='project-test2', result='SUCCESS'), + ], ordered=False) + self.assertTrue(A.is_merged) diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 69597401b8..ebeff53180 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -1879,6 +1879,35 @@ class TestScheduler(ZuulTestCase): self.assertEqual(B.reported, 2) self.assertEqual(C.reported, 2) + def test_approval_removal(self): + # Test that we dequeue a change when it can not merge + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + A.addApproval('Code-Review', 2) + self.fake_gerrit.addEvent(A.addApproval('Approved', 1)) + + self.waitUntilSettled() + self.assertEqual(1, len(self.builds)) + self.assertEqual(0, len(self.history)) + + # Remove the approval + self.fake_gerrit.addEvent(A.addApproval('Approved', 0)) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + # The change should be dequeued. + self.assertHistory([ + dict(name='project-merge', result='ABORTED'), + ], ordered=False) + self.assertEqual(2, len(A.messages)) + self.assertEqual(A.data['status'], 'NEW') + self.assertIn('This change is unable to merge ' + 'due to a missing requirement.', + A.messages[1]) + @simple_layout('layouts/nonvoting-job-approval.yaml') def test_nonvoting_job_approval(self): "Test that non-voting jobs don't vote but leave approval" diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index afaec4e25d..22ecb79411 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -459,6 +459,8 @@ class PipelineManager(metaclass=ABCMeta): item.setConfigErrors(item.getConfigErrors()) if item.dequeued_needing_change: item.setDequeuedNeedingChange() + if item.dequeued_missing_requirements: + item.setDequeuedMissingRequirements() # It can happen that all in-flight builds have been removed # which would lead to paused parent jobs not being resumed. @@ -1399,14 +1401,24 @@ class PipelineManager(metaclass=ABCMeta): item_ahead = None change_queue = item.queue - if self.checkForChangesNeededBy(item.change, change_queue, - item.event) is not True: + if COMPONENT_REGISTRY.model_api > 3: + # This sets a QueueItem flag which is only understood by + # api 4. + meets_reqs = self.isChangeReadyToBeEnqueued( + item.change, item.event) + else: + meets_reqs = True + needs_met = self.checkForChangesNeededBy(item.change, change_queue, + item.event) is True + if not (meets_reqs and needs_met): # It's not okay to enqueue this change, we should remove it. log.info("Dequeuing change %s because " "it can no longer merge" % item.change) self.cancelJobs(item) if item.isBundleFailing(): item.setDequeuedBundleFailing() + elif not meets_reqs: + item.setDequeuedMissingRequirements() else: item.setDequeuedNeedingChange() if item.live: @@ -1875,6 +1887,11 @@ class PipelineManager(metaclass=ABCMeta): action = 'failure' actions = self.pipeline.failure_actions item.setReportedResult('FAILURE') + elif item.wasDequeuedMissingRequirements(): + log.debug("Dequeued missing requirements") + action = 'failure' + actions = self.pipeline.failure_actions + item.setReportedResult('FAILURE') elif not item.getJobs(): # We don't send empty reports with +1 log.debug("No jobs for change %s", item.change) diff --git a/zuul/model.py b/zuul/model.py index 9a3334d201..df210e353f 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -50,7 +50,7 @@ from zuul.zk.change_cache import ChangeKey # When making ZK schema changes, increment this and add a record to # docs/developer/model-changelog.rst -MODEL_API = 3 +MODEL_API = 4 MERGER_MERGE = 1 # "git merge" MERGER_MERGE_RESOLVE = 2 # "git merge -s resolve" @@ -4031,6 +4031,7 @@ class QueueItem(zkobject.ZKObject): queue=None, change=None, # a ref dequeued_needing_change=False, + dequeued_missing_requirements=False, current_build_set=None, item_ahead=None, items_behind=[], @@ -4090,6 +4091,8 @@ class QueueItem(zkobject.ZKObject): # This needs change cache and the API to resolve change by key. "change": self.change.cache_key, "dequeued_needing_change": self.dequeued_needing_change, + "dequeued_missing_requirements": + self.dequeued_missing_requirements, "current_build_set": (self.current_build_set and self.current_build_set.getPath()), "item_ahead": self.item_ahead and self.item_ahead.getPath(), @@ -4387,6 +4390,9 @@ class QueueItem(zkobject.ZKObject): def wasDequeuedNeedingChange(self): return self.dequeued_needing_change + def wasDequeuedMissingRequirements(self): + return self.dequeued_missing_requirements + def includesConfigUpdates(self): includes_trusted = False includes_untrusted = False @@ -4816,6 +4822,12 @@ class QueueItem(zkobject.ZKObject): dequeued_needing_change=True) self._setAllJobsSkipped() + def setDequeuedMissingRequirements(self): + self.updateAttributes( + self.pipeline.manager.current_context, + dequeued_missing_requirements=True) + self._setAllJobsSkipped() + def setDequeuedBundleFailing(self): self.updateAttributes( self.pipeline.manager.current_context, diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py index 1749f8a63e..718c90db5d 100644 --- a/zuul/reporter/__init__.py +++ b/zuul/reporter/__init__.py @@ -179,6 +179,9 @@ class BaseReporter(object, metaclass=abc.ABCMeta): msg = 'This change is part of a bundle that failed to merge.\n' elif item.dequeued_needing_change: msg = 'This change depends on a change that failed to merge.\n' + elif item.dequeued_missing_requirements: + msg = ('This change is unable to merge ' + 'due to a missing requirement.\n') elif item.isBundleFailing(): msg = 'This change is part of a bundle that failed.\n' if with_jobs: