Fix dynamic dependent pipeline failure

When a change that altered a dependent pipeline failed and was removed
from that pipeline, changes behind it did not have their configuration
reset; instead, the alteration introduced by the failed change lingered.
This was largely because the way we tested that a project participated
in a pipeline was incorrect -- we only checked that it was in the layout,
not the pipeline.  That is corrected.

Additionally, the job graph for the second change was not reset.  That's
because it was only created once, and was not cleared out when the
change's buildset is reset.  That is corrected as well (though a future
enhancement would be to move item.job_graph into the BuildSet class to
better reflect its lifecycle).

Change-Id: Icdbc17bb21887a96f4118bb1a49ed7b00e1304e6

tests/fixtures/config/in-repo-join/git/common-config/playbooks/common-config-test.yaml

@@ -0,0 +1,2 @@
+- hosts: all
+  tasks: []

tests/fixtures/config/in-repo-join/git/common-config/zuul.yaml

@@ -0,0 +1,46 @@
+- pipeline:
+    name: check
+    manager: independent
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- pipeline:
+    name: gate
+    manager: dependent
+    success-message: Build succeeded (tenant-one-gate).
+    trigger:
+      gerrit:
+        - event: comment-added
+          approval:
+            - Approved: 1
+    success:
+      gerrit:
+        Verified: 2
+        submit: true
+    failure:
+      gerrit:
+        Verified: -2
+    start:
+      gerrit:
+        Verified: 0
+    precedence: high
+
+- job:
+    name: base
+    parent: null
+
+- job:
+    name: common-config-test
+
+- project:
+    name: org/project
+    check:
+      jobs:
+        - common-config-test

tests/fixtures/config/in-repo-join/git/org_project/.zuul.yaml

@@ -0,0 +1,2 @@
+- job:
+    name: project-test1

tests/fixtures/config/in-repo-join/git/org_project/README

@@ -0,0 +1 @@
+test

tests/fixtures/config/in-repo-join/git/org_project/playbooks/project-test1.yaml

@@ -0,0 +1,2 @@
+- hosts: all
+  tasks: []

tests/fixtures/config/in-repo-join/main.yaml

@@ -0,0 +1,8 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project

tests/fixtures/config/in-repo/git/common-config/zuul.yaml

@@ -78,6 +78,8 @@
 
 - project:
     name: common-config
+    check:
+      jobs: []
     tenant-one-gate:
       jobs:
         - common-config-test

tests/fixtures/config/in-repo/git/org_project/.zuul.yaml

@@ -3,6 +3,8 @@
 
 - project:
     name: org/project
+    check:
+      jobs: []
     tenant-one-gate:
       jobs:
         - project-test1

tests/unit/test_v3.py

@@ -371,55 +371,6 @@ class TestInRepoConfig(ZuulTestCase):
             dict(name='project-test1', result='SUCCESS', changes='1,2'),
             dict(name='project-test2', result='SUCCESS', changes='1,2')])
 
-    def test_dynamic_dependent_pipeline(self):
-        # Test dynamically adding a project to a
-        # dependent pipeline for the first time
-        self.executor_server.hold_jobs_in_build = True
-
-        tenant = self.sched.abide.tenants.get('tenant-one')
-        gate_pipeline = tenant.layout.pipelines['gate']
-
-        in_repo_conf = textwrap.dedent(
-            """
-            - job:
-                name: project-test1
-
-            - job:
-                name: project-test2
-
-            - project:
-                name: org/project
-                gate:
-                  jobs:
-                    - project-test2
-            """)
-
-        in_repo_playbook = textwrap.dedent(
-            """
-            - hosts: all
-              tasks: []
-            """)
-
-        file_dict = {'.zuul.yaml': in_repo_conf,
-                     'playbooks/project-test2.yaml': in_repo_playbook}
-        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
-                                           files=file_dict)
-        A.addApproval('Approved', 1)
-        self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2))
-        self.waitUntilSettled()
-
-        items = gate_pipeline.getAllItems()
-        self.assertEqual(items[0].change.number, '1')
-        self.assertEqual(items[0].change.patchset, '1')
-        self.assertTrue(items[0].live)
-
-        self.executor_server.hold_jobs_in_build = False
-        self.executor_server.release()
-        self.waitUntilSettled()
-
-        # Make sure the dynamic queue got cleaned up
-        self.assertEqual(gate_pipeline.queues, [])
-
     def test_in_repo_branch(self):
         in_repo_conf = textwrap.dedent(
             """
@@ -846,6 +797,105 @@ class TestInRepoConfig(ZuulTestCase):
                       "C should have an error reported")
 
 
+class TestInRepoJoin(ZuulTestCase):
+    # In this config, org/project is not a member of any pipelines, so
+    # that we may test the changes that cause it to join them.
+
+    tenant_config_file = 'config/in-repo-join/main.yaml'
+
+    def test_dynamic_dependent_pipeline(self):
+        # Test dynamically adding a project to a
+        # dependent pipeline for the first time
+        self.executor_server.hold_jobs_in_build = True
+
+        tenant = self.sched.abide.tenants.get('tenant-one')
+        gate_pipeline = tenant.layout.pipelines['gate']
+
+        in_repo_conf = textwrap.dedent(
+            """
+            - job:
+                name: project-test1
+
+            - job:
+                name: project-test2
+
+            - project:
+                name: org/project
+                gate:
+                  jobs:
+                    - project-test2
+            """)
+
+        in_repo_playbook = textwrap.dedent(
+            """
+            - hosts: all
+              tasks: []
+            """)
+
+        file_dict = {'.zuul.yaml': in_repo_conf,
+                     'playbooks/project-test2.yaml': in_repo_playbook}
+        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()
+
+        items = gate_pipeline.getAllItems()
+        self.assertEqual(items[0].change.number, '1')
+        self.assertEqual(items[0].change.patchset, '1')
+        self.assertTrue(items[0].live)
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        # Make sure the dynamic queue got cleaned up
+        self.assertEqual(gate_pipeline.queues, [])
+
+    def test_dynamic_dependent_pipeline_failure(self):
+        # Test that a change behind a failing change adding a project
+        # to a dependent pipeline is dequeued.
+        self.executor_server.hold_jobs_in_build = True
+
+        in_repo_conf = textwrap.dedent(
+            """
+            - job:
+                name: project-test1
+
+            - project:
+                name: org/project
+                gate:
+                  jobs:
+                    - project-test1
+            """)
+
+        file_dict = {'.zuul.yaml': in_repo_conf}
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
+                                           files=file_dict)
+        self.executor_server.failJob('project-test1', A)
+        A.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
+        B.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(B.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+        self.assertEqual(A.reported, 2,
+                         "A should report start and failure")
+        self.assertEqual(A.data['status'], 'NEW')
+        self.assertEqual(B.reported, 1,
+                         "B should report start")
+        self.assertHistory([
+            dict(name='project-test1', result='FAILURE', changes='1,1'),
+            dict(name='project-test1', result='FAILURE', changes='1,1 2,1'),
+        ], ordered=False)
+
+
 class TestAnsible(AnsibleZuulTestCase):
     # A temporary class to hold new tests while others are disabled
 

zuul/manager/__init__.py

@@ -529,11 +529,12 @@ class PipelineManager(object):
 
         if not item.job_graph:
             try:
+                self.log.debug("Freezing job graph for %s" % (item,))
                 item.freezeJobGraph()
             except Exception as e:
                 # TODOv3(jeblair): nicify this exception as it will be reported
                 self.log.exception("Error freezing job graph for %s" %
-                                   item)
+                                   (item,))
                 item.setConfigError("Unable to freeze job graph: %s" %
                                     (str(e)))
                 return False
@@ -752,9 +753,12 @@ class PipelineManager(object):
         layout = (item.current_build_set.layout or
                   self.pipeline.layout)
 
-        if not layout.hasProject(item.change.project):
+        project_in_pipeline = True
+        if not layout.getProjectPipelineConfig(item.change.project,
+                                               self.pipeline):
             self.log.debug("Project %s not in pipeline %s for change %s" % (
                 item.change.project, self.pipeline, item.change))
+            project_in_pipeline = False
             actions = []
         elif item.getConfigError():
             self.log.debug("Invalid config for change %s" % item.change)
@@ -780,7 +784,7 @@ class PipelineManager(object):
             actions = self.pipeline.failure_actions
             item.setReportedResult('FAILURE')
             self.pipeline._consecutive_failures += 1
-        if layout.hasProject(item.change.project) and self.pipeline._disabled:
+        if project_in_pipeline and self.pipeline._disabled:
             actions = self.pipeline.disabled_actions
         # Check here if we should disable so that we only use the disabled
         # reporters /after/ the last disable_at failure is still reported as

zuul/model.py

@@ -1374,6 +1374,7 @@ class QueueItem(object):
         self.quiet = False
         self.active = False  # Whether an item is within an active window
         self.live = True  # Whether an item is intended to be processed at all
+        # TODO(jeblair): move job_graph to buildset
         self.job_graph = None
 
     def __repr__(self):
@@ -1391,6 +1392,7 @@ class QueueItem(object):
         old.next_build_set = self.current_build_set
         self.current_build_set.previous_build_set = old
         self.build_sets.append(self.current_build_set)
+        self.job_graph = None
 
     def addBuild(self, build):
         self.current_build_set.addBuild(build)
@@ -2331,19 +2333,21 @@ class Layout(object):
             job_graph.addJob(frozen_job)
 
     def createJobGraph(self, item):
-        project_config = self.project_configs.get(
-            item.change.project.canonical_name, None)
-        ret = JobGraph()
         # NOTE(pabelanger): It is possible for a foreign project not to have a
         # configured pipeline, if so return an empty JobGraph.
-        if project_config and item.pipeline.name in project_config.pipelines:
-            project_job_list = \
-                project_config.pipelines[item.pipeline.name].job_list
-            self._createJobGraph(item, project_job_list, ret)
+        ret = JobGraph()
+        ppc = self.getProjectPipelineConfig(item.change.project,
+                                            item.pipeline)
+        if ppc:
+            self._createJobGraph(item, ppc.job_list, ret)
         return ret
 
-    def hasProject(self, project):
-        return project.canonical_name in self.project_configs
+    def getProjectPipelineConfig(self, project, pipeline):
+        project_config = self.project_configs.get(
+            project.canonical_name, None)
+        if not project_config:
+            return None
+        return project_config.pipelines.get(pipeline.name, None)
 
 
 class Semaphore(object):