Build layout of non-live items with config updates
The recent change to diff job configs in order to run modified jobs even if file matchers don't match compares the job config in the change under test to the most recent layout ahead in the queue, or the running tenant layout if none, in order to determine if a job's config has changed. In an independent pipeline with multiple dependent changes, we did not generate layouts for non-live items, therefore the only layout available to diff was the running tenant layout. This would cause changes to run too many jobs in that situation (while they would run the correct jobs in a dependent pipeline since layouts of items ahead were available). To correct this, always generate layouts for items with config updates regardless of whether they are live, and ensure that every item in the queue has a pointer to a layout (whether that is its own layout or the layout of the item ahead, or the tenant layout) so that the next item can diff against it. This means we are potentially running more merge operations than before. The TestNonLiveMerges test is a poor match for the new system, since every change in it contained a configuration update. So a test which previously was used to verify that we did not perform a merge for every item now correctly performs a merge for every item. Since that is no longer a useful test, it is split into two simpler tests; one which verifies that non-live items without config updates do not perform merges, and one that repeats the same scenario with config updates and verifies that we do perform merges for non-live items in that case (as a sort of control for the other test). This also changes how errors are reported for a series of changes with a config error. Previously in the case of a non-live item with a config error followed by a live item without a config error, we would report the config error of the non-live item on the live item, because Zuul was unable to determine which change was responsible for the error (since only the live item got a merge and layout). Now that both items have layouts, Zuul can determine that it does not need to report the non-live item's error on the live item. However, we still report an error on the live item since it can not merge as-is. We now report an abbreviated error: "This change depends on a change with an invalid configuration." Change-Id: Id533772f35ebbc76910398e0e0fa50a3abfceb52
This commit is contained in:
James E. Blair
parent
41927a38db
commit
de1a8372a8
|
|
@ -182,7 +182,8 @@ class TestGerritWeb(ZuulTestCase):
|
|||
self.waitUntilSettled()
|
||||
|
||||
self.assertEqual(B.patchsets[0]['approvals'][0]['value'], "-1")
|
||||
self.assertIn('Zuul encountered a syntax error',
|
||||
self.assertIn('This change depends on a change '
|
||||
'with an invalid configuration',
|
||||
B.messages[0])
|
||||
self.assertEqual(B.comments, [])
|
||||
|
||||
|
|
|
|||
|
|
@ -5781,6 +5781,47 @@ class TestJobUpdateFileMatcher(ZuulTestCase):
|
|||
dict(name='new-files', result='SUCCESS', changes='1,1'),
|
||||
])
|
||||
|
||||
def test_patch_series(self):
|
||||
"Test that we diff to the nearest layout in a patch series"
|
||||
in_repo_conf = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: new-files1
|
||||
parent: existing-files
|
||||
|
||||
- project:
|
||||
check:
|
||||
jobs:
|
||||
- new-files1
|
||||
""")
|
||||
|
||||
file_dict = {'zuul.d/new1.yaml': in_repo_conf}
|
||||
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
|
||||
files=file_dict)
|
||||
|
||||
in_repo_conf = textwrap.dedent(
|
||||
"""
|
||||
- job:
|
||||
name: new-files2
|
||||
parent: existing-files
|
||||
|
||||
- project:
|
||||
check:
|
||||
jobs:
|
||||
- new-files2
|
||||
""")
|
||||
|
||||
file_dict = {'zuul.d/new2.yaml': in_repo_conf}
|
||||
B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B',
|
||||
files=file_dict)
|
||||
B.setDependsOn(A, 1)
|
||||
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
dict(name='new-files2', result='SUCCESS', changes='1,1 2,1'),
|
||||
])
|
||||
|
||||
def test_disable_match(self):
|
||||
"Test matchers are not overridden if we say so"
|
||||
in_repo_conf = textwrap.dedent(
|
||||
|
|
|
|||
|
|
@ -2178,8 +2178,8 @@ class TestInRepoConfig(ZuulTestCase):
|
|||
|
||||
self.assertEqual(C.reported, 1,
|
||||
"C should report failure")
|
||||
self.assertIn('Zuul encountered a syntax error while parsing '
|
||||
'its configuration',
|
||||
self.assertIn('This change depends on a change '
|
||||
'with an invalid configuration.',
|
||||
C.messages[0],
|
||||
"C should have an error reported")
|
||||
|
||||
|
|
@ -2215,20 +2215,14 @@ class TestNonLiveMerges(ZuulTestCase):
|
|||
config_file = 'zuul-connections-gerrit-and-github.conf'
|
||||
tenant_config_file = 'config/in-repo/main.yaml'
|
||||
|
||||
def test_non_live_merges(self):
|
||||
def test_non_live_merges_with_config_updates(self):
|
||||
"""
|
||||
This test checks that we don't do merges for non-live queue items.
|
||||
|
||||
The scenario tests three scenarios:
|
||||
This test checks that we do merges for non-live queue items with
|
||||
config updates.
|
||||
|
||||
* Simple dependency chain:
|
||||
A -> B -> C
|
||||
|
||||
* Dependency chain that includes a merge conflict:
|
||||
A -> B -> B_conflict (conflicts with A) -> C_conflict
|
||||
|
||||
* Dependency chain with broken config in the middls:
|
||||
A_invalid (broken config) -> B_after_invalid (repairs broken config(
|
||||
"""
|
||||
|
||||
in_repo_conf_a = textwrap.dedent(
|
||||
|
|
@ -2302,55 +2296,18 @@ class TestNonLiveMerges(ZuulTestCase):
|
|||
parent=B.patchsets[0]['ref'])
|
||||
C.setDependsOn(B, 1)
|
||||
|
||||
B_conflict = self.fake_gerrit.addFakeChange(
|
||||
'org/project', 'master', 'B_conflict', files=file_dict_a)
|
||||
B_conflict.setDependsOn(B, 1)
|
||||
C_conflict = self.fake_gerrit.addFakeChange(
|
||||
'org/project', 'master', 'C_conflict')
|
||||
C_conflict.setDependsOn(B_conflict, 1)
|
||||
|
||||
in_repo_conf_a_invalid = textwrap.dedent(
|
||||
"""
|
||||
- project:
|
||||
name: org/project
|
||||
check:
|
||||
jobs:
|
||||
- project-test1
|
||||
""")
|
||||
|
||||
file_dict_a_invalid = {'.zuul.yaml': in_repo_conf_a_invalid,
|
||||
'playbooks/project-test.yaml': in_repo_playbook}
|
||||
A_invalid = self.fake_gerrit.addFakeChange(
|
||||
'org/project', 'master', 'A_invalid', files=file_dict_a_invalid)
|
||||
B_after_invalid = self.fake_gerrit.addFakeChange(
|
||||
'org/project', 'master', 'B', files=file_dict_b,
|
||||
parent=A_invalid.patchsets[0]['ref'])
|
||||
B_after_invalid.setDependsOn(A_invalid, 1)
|
||||
|
||||
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(B_conflict.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(C_conflict.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(A_invalid.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(B_after_invalid.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertEqual(A.reported, 1, "A should report")
|
||||
self.assertEqual(B.reported, 1, "B should report")
|
||||
self.assertEqual(C.reported, 1, "C should report")
|
||||
self.assertEqual(B_conflict.reported, 1, "B_conflict should report")
|
||||
self.assertEqual(C_conflict.reported, 1, "C_conflict should report")
|
||||
self.assertEqual(B_after_invalid.reported, 1,
|
||||
"B_after_invalid should report")
|
||||
|
||||
self.assertIn('Build succeeded', A.messages[0])
|
||||
self.assertIn('Build succeeded', B.messages[0])
|
||||
self.assertIn('Build succeeded', C.messages[0])
|
||||
self.assertIn('Merge Failed', B_conflict.messages[0])
|
||||
self.assertIn('Merge Failed', C_conflict.messages[0])
|
||||
self.assertIn('Zuul encountered a syntax error', A_invalid.messages[0])
|
||||
self.assertIn('Build succeeded', B_after_invalid.messages[0])
|
||||
|
||||
self.assertHistory([
|
||||
# Change A
|
||||
|
|
@ -2367,14 +2324,34 @@ class TestNonLiveMerges(ZuulTestCase):
|
|||
changes='1,1 2,1 3,1'),
|
||||
dict(name='project-test3', result='SUCCESS',
|
||||
changes='1,1 2,1 3,1'),
|
||||
|
||||
# Change B_after_invalid
|
||||
dict(name='project-test1', result='SUCCESS', changes='6,1 7,1'),
|
||||
dict(name='project-test2', result='SUCCESS', changes='6,1 7,1'),
|
||||
], ordered=False)
|
||||
|
||||
# We expect one merge call per change
|
||||
self.assertEqual(len(self.merge_client.history['merger:merge']), 7)
|
||||
# We expect one merge call per live change, plus one call for
|
||||
# each non-live change with a config update (which is all of them).
|
||||
self.assertEqual(len(self.merge_client.history['merger:merge']), 6)
|
||||
|
||||
def test_non_live_merges(self):
|
||||
"""
|
||||
This test checks that we don't do merges for non-live queue items.
|
||||
|
||||
* Simple dependency chain:
|
||||
A -> B -> C
|
||||
"""
|
||||
|
||||
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
|
||||
B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
|
||||
B.setDependsOn(A, 1)
|
||||
|
||||
C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C')
|
||||
C.setDependsOn(B, 1)
|
||||
|
||||
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
|
||||
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# We expect one merge call per live change.
|
||||
self.assertEqual(len(self.merge_client.history['merger:merge']), 3)
|
||||
|
||||
|
||||
class TestJobContamination(AnsibleZuulTestCase):
|
||||
|
|
|
|||
|
|
@ -250,8 +250,7 @@ class PipelineManager(object):
|
|||
item.job_graph = None
|
||||
item.layout = None
|
||||
if item.active:
|
||||
if self.prepareItem(item):
|
||||
self.prepareJobs(item)
|
||||
self.prepareItem(item)
|
||||
|
||||
# Re-set build results in case any new jobs have been
|
||||
# added to the tree.
|
||||
|
|
@ -458,12 +457,9 @@ class PipelineManager(object):
|
|||
return canceled
|
||||
|
||||
def _findRelevantErrors(self, item, layout):
|
||||
parent_layout = None
|
||||
parent = item.item_ahead
|
||||
while not parent_layout and parent:
|
||||
parent_layout = parent.layout
|
||||
parent = parent.item_ahead
|
||||
if not parent_layout:
|
||||
if item.item_ahead:
|
||||
parent_layout = item.item_ahead.layout
|
||||
else:
|
||||
parent_layout = item.pipeline.tenant.layout
|
||||
|
||||
relevant_errors = []
|
||||
|
|
@ -604,24 +600,22 @@ class PipelineManager(object):
|
|||
return False
|
||||
|
||||
def getLayout(self, item):
|
||||
if not self._queueUpdatesConfig(item):
|
||||
# No config updates in queue. Use existing pipeline layout
|
||||
return item.queue.pipeline.tenant.layout
|
||||
elif (not item.change.updatesConfig(item.pipeline.tenant) and
|
||||
item.item_ahead and item.item_ahead.live):
|
||||
# Current change does not update layout, use its parent if parent
|
||||
# has a layout.
|
||||
return item.item_ahead.layout
|
||||
# Else this item or a non live parent updates the config,
|
||||
if item.item_ahead:
|
||||
fallback_layout = item.item_ahead.layout
|
||||
else:
|
||||
fallback_layout = item.pipeline.tenant.layout
|
||||
if not item.change.updatesConfig(item.pipeline.tenant):
|
||||
# Current change does not update layout, use its parent.
|
||||
return fallback_layout
|
||||
# Else this item updates the config,
|
||||
# ask the merger for the result.
|
||||
build_set = item.current_build_set
|
||||
if build_set.merge_state == build_set.PENDING:
|
||||
if build_set.merge_state != build_set.COMPLETE:
|
||||
return None
|
||||
if build_set.merge_state == build_set.COMPLETE:
|
||||
if build_set.unable_to_merge:
|
||||
return None
|
||||
self.log.debug("Preparing dynamic layout for: %s" % item.change)
|
||||
return self._loadDynamicLayout(item)
|
||||
if build_set.unable_to_merge:
|
||||
return fallback_layout
|
||||
self.log.debug("Preparing dynamic layout for: %s" % item.change)
|
||||
return self._loadDynamicLayout(item)
|
||||
|
||||
def scheduleMerge(self, item, files=None, dirs=None):
|
||||
log = item.annotateLogger(self.log)
|
||||
|
|
@ -681,47 +675,76 @@ class PipelineManager(object):
|
|||
return False
|
||||
|
||||
def prepareItem(self, item):
|
||||
# This runs on every iteration of _processOneItem
|
||||
# Returns True if the item is ready, false otherwise
|
||||
ready = True
|
||||
build_set = item.current_build_set
|
||||
tenant = item.pipeline.tenant
|
||||
# We always need to set the configuration of the item if it
|
||||
# isn't already set.
|
||||
tpc = tenant.project_configs.get(item.change.project.canonical_name)
|
||||
if not build_set.ref:
|
||||
build_set.setConfiguration()
|
||||
|
||||
# Next, if a change ahead has a broken config, then so does
|
||||
# this one. Record that and don't do anything else.
|
||||
if (item.item_ahead and item.item_ahead.current_build_set and
|
||||
item.item_ahead.current_build_set.config_errors):
|
||||
msg = "This change depends on a change "\
|
||||
"with an invalid configuration.\n"
|
||||
item.setConfigError(msg)
|
||||
return False
|
||||
|
||||
# The next section starts between 0 and 2 remote merger
|
||||
# operations in parallel as needed.
|
||||
ready = True
|
||||
# If the project is in this tenant, fetch missing files so we
|
||||
# know if it updates the config.
|
||||
if tpc:
|
||||
if build_set.files_state == build_set.NEW:
|
||||
ready = self.scheduleFilesChanges(item)
|
||||
if build_set.files_state == build_set.PENDING:
|
||||
ready = False
|
||||
# If this change alters config or is live, schedule merge and
|
||||
# build a layout.
|
||||
if build_set.merge_state == build_set.NEW:
|
||||
tenant = item.pipeline.tenant
|
||||
tpc = tenant.project_configs[item.change.project.canonical_name]
|
||||
ready = self.scheduleMerge(
|
||||
item,
|
||||
files=(['zuul.yaml', '.zuul.yaml'] +
|
||||
list(tpc.extra_config_files)),
|
||||
dirs=(['zuul.d', '.zuul.d'] +
|
||||
list(tpc.extra_config_dirs)))
|
||||
if build_set.files_state == build_set.NEW:
|
||||
ready = self.scheduleFilesChanges(item)
|
||||
if build_set.files_state == build_set.PENDING:
|
||||
ready = False
|
||||
if item.live or item.change.updatesConfig(tenant):
|
||||
ready = self.scheduleMerge(
|
||||
item,
|
||||
files=(['zuul.yaml', '.zuul.yaml'] +
|
||||
list(tpc.extra_config_files)),
|
||||
dirs=(['zuul.d', '.zuul.d'] +
|
||||
list(tpc.extra_config_dirs)))
|
||||
if build_set.merge_state == build_set.PENDING:
|
||||
ready = False
|
||||
if build_set.unable_to_merge:
|
||||
ready = False
|
||||
if build_set.config_errors:
|
||||
ready = False
|
||||
return ready
|
||||
|
||||
def prepareJobs(self, item):
|
||||
log = item.annotateLogger(self.log)
|
||||
# This only runs once the item is in the pipeline's action window
|
||||
# Returns True if the item is ready, false otherwise
|
||||
if not item.live:
|
||||
# Short circuit as non live items don't need layouts.
|
||||
# We also don't need to take further ready actions in
|
||||
# _processOneItem() so we return false.
|
||||
# If a merger op is outstanding, we're not ready.
|
||||
if not ready:
|
||||
return False
|
||||
elif not item.layout:
|
||||
|
||||
# With the merges done, we have the info needed to get a
|
||||
# layout. This may return the pipeline layout, a layout from
|
||||
# a change ahead, a newly generated layout for this change, or
|
||||
# None if there was an error that makes the layout unusable.
|
||||
# In the last case, it will have set the config_errors on this
|
||||
# item, which may be picked up by the next itme.
|
||||
if not item.layout:
|
||||
item.layout = self.getLayout(item)
|
||||
if not item.layout:
|
||||
return False
|
||||
|
||||
# If the change can not be merged or has config errors, don't
|
||||
# run jobs.
|
||||
if build_set.unable_to_merge:
|
||||
return False
|
||||
if build_set.config_errors:
|
||||
return False
|
||||
|
||||
# We don't need to build a job graph for a non-live item, we
|
||||
# just need the layout.
|
||||
if not item.live:
|
||||
return False
|
||||
|
||||
# At this point we have a layout for the item, and it's live,
|
||||
# so freeze the job graph.
|
||||
log = item.annotateLogger(self.log)
|
||||
if not item.job_graph:
|
||||
try:
|
||||
log.debug("Freezing job graph for %s" % (item,))
|
||||
|
|
@ -784,8 +807,8 @@ class PipelineManager(object):
|
|||
change_queue.moveItem(item, nnfi)
|
||||
changed = True
|
||||
self.cancelJobs(item)
|
||||
if actionable and item.live:
|
||||
ready = self.prepareItem(item) and self.prepareJobs(item)
|
||||
if actionable:
|
||||
ready = self.prepareItem(item)
|
||||
# Starting jobs reporting should only be done once if there are
|
||||
# jobs to run for this item.
|
||||
if ready and len(self.pipeline.start_actions) > 0 \
|
||||
|
|
|
|||
Loading…
Reference in New Issue