Refresh builds even less

Change I3824af6149bf27c41a8d895fc682236bd0d91f6b intended to refresh
builds from ZK only when necessary.  Before that change we would
refresh them only if they did not have a result (because once they
have a result they won't change).  That change should have reduced
that so that in the cases where we don't have a result yet, we
still only refresh if the build has changed.

In other words, that change should have used an "and" instead of
an "or".

Logically, if builds can't change after they have a result, then
the check of whether we have a result is not necessary.  So rather
that change the operator, we can just drop the build.result check
altogether and rely on the object update check.  This has the effect
of making the code more future proof as well, in that we remove
the assumption that the build will never change after receiving a
result.

This change also surfaced a bug in the original implementation:
because refreshing the Build objects happens inside the deserialize
method of the BuildSet object, the BuildSet has not actually updated
its build_versions variable from ZK yet, which means our comparisons
in shouldRefreshBuild were using outdated data.  To correct, we now
pass in the newly deserialized value.  And the same for Jobs.

Change-Id: Ie688f2ee0343cab5d82776ccfc7b0f2edc5f91e5

tests/unit/test_scheduler.py

@@ -6467,8 +6467,8 @@ For CI problems and help debugging, contact ci@example.org"""
         # they don't they may be a good way for us to catch unintended
         # extra read operations.  If they change too much, they may
         # not be worth keeping and we can just remove them.
-        self.assertEqual(6, ctx.cumulative_read_objects)
-        self.assertEqual(6, ctx.cumulative_read_znodes)
+        self.assertEqual(5, ctx.cumulative_read_objects)
+        self.assertEqual(5, ctx.cumulative_read_znodes)
         self.assertEqual(0, ctx.cumulative_write_objects)
         self.assertEqual(0, ctx.cumulative_write_znodes)
 
@@ -6490,8 +6490,8 @@ For CI problems and help debugging, contact ci@example.org"""
         self.assertIs(old_check_build_results, new_check_build_results)
 
         # Again check the object read counts
-        self.assertEqual(6, ctx.cumulative_read_objects)
-        self.assertEqual(6, ctx.cumulative_read_znodes)
+        self.assertEqual(4, ctx.cumulative_read_objects)
+        self.assertEqual(4, ctx.cumulative_read_znodes)
         self.assertEqual(0, ctx.cumulative_write_objects)
         self.assertEqual(0, ctx.cumulative_write_znodes)
 

zuul/model.py

@@ -4364,6 +4364,8 @@ class BuildSet(zkobject.ZKObject):
         # order.
         tpe_jobs = []
         tpe = context.executor[BuildSet]
+        job_versions = data.get('job_versions', {})
+        build_versions = data.get('build_versions', {})
         # jobs (deserialize as separate objects)
         if data['job_graph']:
             for job_name in data['job_graph'].jobs:
@@ -4378,7 +4380,7 @@ class BuildSet(zkobject.ZKObject):
                 if job_name in self.jobs:
                     job = self.jobs[job_name]
                     if ((not old_build_exists) or
-                        self.shouldRefreshJob(job)):
+                        self.shouldRefreshJob(job, job_versions)):
                         tpe_jobs.append((None, job_name,
                                          tpe.submit(job.refresh, context)))
                 else:
@@ -4390,8 +4392,7 @@ class BuildSet(zkobject.ZKObject):
                     build = self.builds.get(job_name)
                     builds[job_name] = build
                     if build and build.getPath() == build_path:
-                        if ((not build.result) or
-                            self.shouldRefreshBuild(build)):
+                        if self.shouldRefreshBuild(build, build_versions):
                             tpe_jobs.append((
                                 None, job_name, tpe.submit(
                                     build.refresh, context)))
@@ -4472,20 +4473,20 @@ class BuildSet(zkobject.ZKObject):
             self.job_versions[job.name] = version + 1
             self.updateAttributes(context, job_versions=self.job_versions)
 
-    def shouldRefreshBuild(self, build):
+    def shouldRefreshBuild(self, build, build_versions):
         # Unless all schedulers are updating versions, we can't trust
         # the data.
         if (COMPONENT_REGISTRY.model_api < 12):
             return True
         current = build.getZKVersion()
-        expected = self.build_versions.get(build.uuid, 0)
+        expected = build_versions.get(build.uuid, 0)
         return expected != current
 
-    def shouldRefreshJob(self, job):
+    def shouldRefreshJob(self, job, job_versions):
         if (COMPONENT_REGISTRY.model_api < 12):
             return True
         current = job.getZKVersion()
-        expected = self.job_versions.get(job.name, 0)
+        expected = job_versions.get(job.name, 0)
         return expected != current
 
     @property