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
This commit is contained in:
parent
b70acfcef5
commit
082ff730fe
@ -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.
|
||||
|
@ -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.
|
@ -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')
|
||||
|
@ -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)
|
||||
|
@ -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"
|
||||
|
@ -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)
|
||||
|
@ -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,
|
||||
|
@ -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:
|
||||
|
Loading…
x
Reference in New Issue
Block a user