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

1 files changed, 77 insertions, 54 deletions

diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index 16ef787e0..eee2b8dfb 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -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 \