From e53250c80d8959dcbe63780f05558b451fa88ca1 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Wed, 1 Mar 2017 14:34:36 -0800 Subject: [PATCH] Report dynamic layout config errors If Zuul encounters a syntax error when loading an in-repo layout, report that as a failure on the change. Try to provide as much readable information as possible to allow the user to diagnose the error. All top-level configuration objects are updated to support a hidden source context variable to aid in creating a useful error message. Item.areAllJobsComplete is updated so that it returns true in the case of a merge failure or config error. In Zuulv2, we knew what jobs we would run even in those cases, but in Zuulv3, we don't, so the item does not have a job tree, so in those cases, the item does not report that all jobs are complete. Returining True in this case allows the NNFI algorithm to continue and expel the item from the queue, avoiding an inifinite loop. The merge failure accessor method is simplified. Change-Id: I9cf19b87c6af5926b5e8bb403b81ce0470e3592d --- tests/unit/test_v3.py | 21 ++++++++++++++++ zuul/configloader.py | 51 ++++++++++++++++++++++++++++++++++----- zuul/manager/__init__.py | 28 ++++++++++++++++++--- zuul/model.py | 18 ++++++++++---- zuul/reporter/__init__.py | 2 ++ 5 files changed, 106 insertions(+), 14 deletions(-) diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index f69ffe61be..cf88265c55 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -185,6 +185,27 @@ class TestInRepoConfig(ZuulTestCase): dict(name='project-test1', result='SUCCESS', changes='2,1'), dict(name='project-test2', result='SUCCESS', changes='3,1')]) + def test_dynamic_syntax_error(self): + in_repo_conf = textwrap.dedent( + """ + - job: + name: project-test2 + foo: error + """) + + file_dict = {'.zuul.yaml': in_repo_conf} + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A', + files=file_dict) + A.addApproval('code-review', 2) + self.fake_gerrit.addEvent(A.addApproval('approved', 1)) + self.waitUntilSettled() + + self.assertEqual(A.data['status'], 'NEW') + self.assertEqual(A.reported, 2, + "A should report start and failure") + self.assertIn('syntax error', A.messages[1], + "A should have a syntax error reported") + class TestAnsible(AnsibleZuulTestCase): # A temporary class to hold new tests while others are disabled diff --git a/zuul/configloader.py b/zuul/configloader.py index 5a132fea7c..42616a8a5a 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -10,11 +10,13 @@ # License for the specific language governing permissions and limitations # under the License. +from contextlib import contextmanager import copy import os import logging import six import yaml +import pprint import voluptuous as vs @@ -38,6 +40,35 @@ def as_list(item): return [item] +class ConfigurationSyntaxError(Exception): + pass + + +@contextmanager +def configuration_exceptions(stanza, conf): + try: + yield + except vs.Invalid as e: + conf = copy.deepcopy(conf) + context = conf.pop('_source_context') + m = """ +Zuul encountered a syntax error while parsing its configuration in the +repo {repo} on branch {branch}. The error was: + + {error} + +The offending content was a {stanza} stanza with the content: + +{content} +""" + m = m.format(repo=context.project.name, + branch=context.branch, + error=str(e), + stanza=stanza, + content=pprint.pformat(conf)) + raise ConfigurationSyntaxError(m) + + class NodeSetParser(object): @staticmethod def getSchema(): @@ -47,13 +78,15 @@ class NodeSetParser(object): nodeset = {vs.Required('name'): str, vs.Required('nodes'): [node], + '_source_context': model.SourceContext, } return vs.Schema(nodeset) @staticmethod def fromYaml(layout, conf): - NodeSetParser.getSchema()(conf) + with configuration_exceptions('nodeset', conf): + NodeSetParser.getSchema()(conf) ns = model.NodeSet(conf['name']) for conf_node in as_list(conf['nodes']): node = model.Node(conf_node['name'], conf_node['image']) @@ -136,7 +169,8 @@ class JobParser(object): @staticmethod def fromYaml(tenant, layout, conf): - JobParser.getSchema()(conf) + with configuration_exceptions('job', conf): + JobParser.getSchema()(conf) # NB: The default detection system in the Job class requires # that we always assign values directly rather than modifying @@ -272,7 +306,8 @@ class ProjectTemplateParser(object): @staticmethod def fromYaml(tenant, layout, conf): - ProjectTemplateParser.getSchema(layout)(conf) + with configuration_exceptions('project or project-template', conf): + ProjectTemplateParser.getSchema(layout)(conf) # Make a copy since we modify this later via pop conf = copy.deepcopy(conf) project_template = model.ProjectConfig(conf['name']) @@ -338,11 +373,13 @@ class ProjectParser(object): for p in layout.pipelines.values(): project[p.name] = {'queue': str, 'jobs': [vs.Any(str, dict)]} - return vs.Schema([project]) + return vs.Schema(project) @staticmethod def fromYaml(tenant, layout, conf_list): - ProjectParser.getSchema(layout)(conf_list) + for conf in conf_list: + with configuration_exceptions('project', conf): + ProjectParser.getSchema(layout)(conf) project = model.ProjectConfig(conf_list[0]['name']) mode = conf_list[0].get('merge-mode', 'merge-resolve') project.merge_mode = model.MERGER_MAP[mode] @@ -461,6 +498,7 @@ class PipelineParser(object): 'window-increase-factor': window_factor, 'window-decrease-type': window_type, 'window-decrease-factor': window_factor, + '_source_context': model.SourceContext, } pipeline['trigger'] = vs.Required( PipelineParser.getDriverSchema('trigger', connections)) @@ -472,7 +510,8 @@ class PipelineParser(object): @staticmethod def fromYaml(layout, connections, scheduler, conf): - PipelineParser.getSchema(layout, connections)(conf) + with configuration_exceptions('pipeline', conf): + PipelineParser.getSchema(layout, connections)(conf) pipeline = model.Pipeline(conf['name'], layout) pipeline.description = conf.get('description') diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 18cf11b59a..4447615f65 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -265,6 +265,8 @@ class PipelineManager(object): # Similarly, reset the item state. if item.current_build_set.unable_to_merge: item.setUnableToMerge() + if item.current_build_set.config_error: + item.setConfigError(item.current_build_set.config_error) if item.dequeued_needing_change: item.setDequeuedNeedingChange() @@ -482,8 +484,21 @@ class PipelineManager(object): import zuul.configloader loader = zuul.configloader.ConfigLoader() self.log.debug("Load dynamic layout with %s" % build_set.files) - layout = loader.createDynamicLayout(item.pipeline.layout.tenant, - build_set.files) + try: + layout = loader.createDynamicLayout( + item.pipeline.layout.tenant, + build_set.files) + except zuul.configloader.ConfigurationSyntaxError as e: + self.log.info("Configuration syntax error " + "in dynamic layout %s" % + build_set.files) + item.setConfigError(str(e)) + return None + except Exception: + self.log.exception("Error in dynamic layout %s" % + build_set.files) + item.setConfigError("Unknown configuration error") + return None return layout build_set.merge_state = build_set.PENDING self.log.debug("Preparing dynamic layout for: %s" % item.change) @@ -556,6 +571,8 @@ class PipelineManager(object): ready = self.prepareLayout(item) if item.current_build_set.unable_to_merge: failing_reasons.append("it has a merge conflict") + if item.current_build_set.config_error: + failing_reasons.append("it has an invalid configuration") if ready and self.provisionNodes(item): changed = True if actionable and ready and self.launchJobs(item): @@ -693,7 +710,12 @@ class PipelineManager(object): def _reportItem(self, item): self.log.debug("Reporting change %s" % item.change) ret = True # Means error as returned by trigger.report - if not item.getJobs(): + if item.getConfigError(): + self.log.debug("Invalid config for change %s" % item.change) + # TODOv3(jeblair): consider a new reporter action for this + actions = self.pipeline.merge_failure_actions + item.setReportedResult('CONFIG_ERROR') + elif not item.getJobs(): # We don't send empty reports with +1, # and the same for -1's (merge failures or transient errors) # as they cannot be followed by +1's diff --git a/zuul/model.py b/zuul/model.py index 96414be4f1..19931ea798 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -1012,6 +1012,7 @@ class BuildSet(object): self.commit = None self.zuul_url = None self.unable_to_merge = False + self.config_error = None # None or an error message string. self.failing_reasons = [] self.merge_state = self.NEW self.nodesets = {} # job -> nodeset @@ -1170,6 +1171,9 @@ class QueueItem(object): return True def areAllJobsComplete(self): + if (self.current_build_set.config_error or + self.current_build_set.unable_to_merge): + return True if not self.hasJobTree(): return False for job in self.getJobs(): @@ -1203,9 +1207,10 @@ class QueueItem(object): return False def didMergerFail(self): - if self.current_build_set.unable_to_merge: - return True - return False + return self.current_build_set.unable_to_merge + + def getConfigError(self): + return self.current_build_set.config_error def isHoldingFollowingChanges(self): if not self.live: @@ -1325,6 +1330,10 @@ class QueueItem(object): self.current_build_set.unable_to_merge = True self._setAllJobsSkipped() + def setConfigError(self, error): + self.current_build_set.config_error = error + self._setAllJobsSkipped() + def _setAllJobsSkipped(self): for job in self.getJobs(): fakebuild = Build(job, None) @@ -2083,8 +2092,7 @@ class UnparsedTenantConfig(object): "a single key (when parsing %s)" % (conf,)) key, value = item.items()[0] - if key in ['project', 'project-template', 'job']: - value['_source_context'] = source_context + value['_source_context'] = source_context if key == 'project': name = value['name'] self.projects.setdefault(name, []).append(value) diff --git a/zuul/reporter/__init__.py b/zuul/reporter/__init__.py index 9ed659949f..541f259f2b 100644 --- a/zuul/reporter/__init__.py +++ b/zuul/reporter/__init__.py @@ -85,6 +85,8 @@ class BaseReporter(object): msg = 'This change depends on a change that failed to merge.\n' elif item.didMergerFail(): msg = pipeline.merge_failure_message + elif item.getConfigError(): + msg = item.getConfigError() else: msg = (pipeline.failure_message + '\n\n' + self._formatItemReportJobs(pipeline, item))