Fix override variables in zuul_return

When a job is retried, the variables set by the zuul_return are not
overwritten.

Change-Id: Ia78eac82a978740766ef5a3f5e5e32a02a8d4479

tests/fixtures/config/data-return/git/common-config/playbooks/paused-data-return-vars.yaml

@@ -0,0 +1,7 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          build_id: "{{ zuul.build }}"
+          zuul:
+            pause: true

tests/fixtures/config/data-return/git/common-config/playbooks/print-data-return-vars.yaml

@@ -0,0 +1,4 @@
+- hosts: localhost
+  tasks:
+    - debug:
+        var: build_id

tests/fixtures/config/data-return/git/common-config/zuul.yaml

@@ -48,6 +48,15 @@
     run:
       - playbooks/paused-data-return-child-jobs.yaml
 
+- job:
+    name: paused-data-return-vars
+    run:
+      - playbooks/paused-data-return-vars.yaml
+
+- job:
+    name: print-data-return-vars
+    run: playbooks/print-data-return-vars.yaml
+
 - job:
     name: data-return-a
     run: playbooks/data-return-a.yaml
@@ -164,6 +173,14 @@
             dependencies:
               - paused-data-return-child-jobs
 
+- project:
+    name: org/project7
+    check:
+      jobs:
+        - paused-data-return-vars
+        - print-data-return-vars:
+            dependencies:
+              - paused-data-return-vars
 
 - project:
     name: org/project-soft
@@ -185,4 +202,3 @@
                 soft: true
               - name: data-return-c
                 soft: true
-

tests/fixtures/config/data-return/git/org_project7/README

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

tests/fixtures/config/data-return/main.yaml

@@ -12,4 +12,5 @@
           - org/project4
           - org/project5
           - org/project6
+          - org/project7
           - org/project-soft

tests/unit/test_v3.py

@@ -3757,6 +3757,68 @@ class TestDataReturn(AnsibleZuulTestCase):
                  result='SUCCESS', changes='1,1'),
         ])
 
+    def test_data_return_child_from_retried_paused_job(self):
+        """
+        Tests that the data returned to the child job is overwritten if the
+        paused job is lost and gets retried (e.g.: executor restart or node
+        unreachable).
+        """
+
+        def _get_file(path):
+            with open(path) as f:
+                return f.read()
+
+        self.wait_timeout = 120
+        self.executor_server.hold_jobs_in_build = True
+        self.executor_server.keep_jobdir = True
+
+        A = self.fake_gerrit.addFakeChange('org/project7', 'master', 'A')
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled("patchset uploaded")
+
+        self.executor_server.release('paused-data-return-vars')
+        self.waitUntilSettled("till job is paused")
+
+        paused_job = self.builds[0]
+        self.assertTrue(paused_job.paused)
+
+        # zuul_return data is set correct
+        j = json.loads(_get_file(paused_job.jobdir.result_data_file))
+        self.assertEqual(j["build_id"], paused_job.uuid)
+
+        # Stop the job worker to simulate an executor restart
+        for job_worker in self.executor_server.job_workers.values():
+            if job_worker.job.unique == paused_job.uuid:
+                job_worker.stop()
+        self.waitUntilSettled("stop job worker")
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release('print-data-return-vars')
+        self.waitUntilSettled("all jobs are done")
+        # The "pause" job might be paused during the waitUntilSettled
+        # call and appear settled; it should automatically resume
+        # though, so just wait for it.
+        for x in iterate_timeout(60, 'paused job'):
+            if not self.builds:
+                break
+        self.waitUntilSettled()
+
+        # First build of paused job (gets retried)
+        first_build = self.history[0]
+        # Second build of the paused job (the retried one)
+        retried_build = self.history[3]
+        # The successful child job (second build)
+        print_build = self.history[2]
+
+        # zuul_return data is set correct to new build id
+        j = json.loads(_get_file(retried_build.jobdir.result_data_file))
+        self.assertEqual(j["build_id"], retried_build.uuid)
+
+        self.assertNotIn(first_build.uuid,
+                         _get_file(print_build.jobdir.job_output_file))
+        self.assertIn(retried_build.uuid,
+                      _get_file(print_build.jobdir.job_output_file))
+
 
 class TestDiskAccounting(AnsibleZuulTestCase):
     config_file = 'zuul-disk-accounting.conf'

zuul/executor/client.py

@@ -248,7 +248,7 @@ class ExecutorClient(object):
             params['ssh_keys'].append(dict(
                 name='%s project key' % item.change.project.canonical_name,
                 key=item.change.project.private_ssh_key))
-        params['vars'] = job.variables
+        params['vars'] = job.combined_variables
         params['extra_vars'] = job.extra_variables
         params['host_vars'] = job.host_variables
         params['group_vars'] = job.group_variables

zuul/model.py

@@ -1231,6 +1231,14 @@ class Job(ConfigObject):
 
         self.name = name
 
+    @property
+    def combined_variables(self):
+        """
+        Combines the data that has been returned by parent jobs with the
+        job variables where job variables have priority over parent data.
+        """
+        return Job._deepUpdate(self.parent_data or {}, self.variables)
+
     def toDict(self, tenant):
         '''
         Convert a Job object's attributes to a dictionary.
@@ -1449,9 +1457,9 @@ class Job(ConfigObject):
                 self.group_variables, other_group_vars)
 
     def updateParentData(self, other_build):
-        # Update variables, but give the current values priority (used
-        # for job return data which is lower precedence than defined
-        # job vars).
+        # Update variables, but give the new values priority. If more than one
+        # parent job returns the same variable, the value from the later job
+        # in the job graph will take precedence.
         other_vars = other_build.result_data
         v = self.parent_data or {}
         v = Job._deepUpdate(v, other_vars)
@@ -1460,7 +1468,6 @@ class Job(ConfigObject):
         if 'zuul' in v:
             del v['zuul']
         self.parent_data = v
-        self.variables = Job._deepUpdate(self.parent_data, self.variables)
 
         artifact_data = self.artifact_data or []
         artifacts = get_artifacts_from_result_data(other_vars)
@@ -2541,11 +2548,12 @@ class QueueItem(object):
                     break
 
             if all_parent_jobs_successful:
-                # Iterate in reverse order over all jobs of the graph (which is
+                # Iterate over all jobs of the graph (which is
                 # in sorted config order) and apply parent data of the jobs we
                 # already found.
                 if len(parent_builds_with_data) > 0:
-                    for parent_job in reversed(self.job_graph.getJobs()):
+                    job.parent_data = {}
+                    for parent_job in self.job_graph.getJobs():
                         parent_build = parent_builds_with_data.get(
                             parent_job.name)
                         if parent_build: