launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:builder-set-preload-processors-robustness into launchpad:master.

Commit message:
Make BuilderSet.preloadProcessors more robust

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/395781

We occasionally see buildd-manager log entries like this:

  Failure while updating builders:
  Traceback (most recent call last):
    File ".../lib/lp/buildmaster/manager.py", line 755, in scanBuilders
      self.builder_factory.update()
    File ".../lib/lp/buildmaster/manager.py", line 274, in update
      [b for b, _ in builders_and_current_bqs])
    File ".../lib/lp/buildmaster/model/builder.py", line 287, in preloadProcessors
      cache._processors_cache.append(store.get(Processor, pid))
  AttributeError: 'DefaultPropertyCache' object has no attribute '_processors_cache'

The oldest occurrence of this on dogfood was 2020-10-01, and the oldest occurrence on production was on 2020-10-10 (though there may have been some older occurrences on production that have expired from logs).  The frequency is quite low: 11 occurrences on production including the first
on 2020-10-10.

It seems theoretically possible for the Storm cache to return a different object from `store.get(Builder, bid)` than the Builder object with the same ID that was passed into `BuilderSet.preloadProcessors`, although it's not quite clear exactly how and I haven't been able to reproduce it in the test suite.  However, it's straightforward enough to guarantee to update the processors cache on the objects that were passed in regardless of whatever Storm cache shenanigans may occur, so do so.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:builder-set-preload-processors-robustness into launchpad:master.

diff --git a/lib/lp/buildmaster/model/builder.py b/lib/lp/buildmaster/model/builder.py
index 5e66baf..a5f23bd 100644
--- a/lib/lp/buildmaster/model/builder.py
+++ b/lib/lp/buildmaster/model/builder.py
@@ -273,17 +273,17 @@ class BuilderSet(object):
         # Grab (Builder.id, Processor.id) pairs and stuff them into the
         # Builders' processor caches.
         store = IStore(BuilderProcessor)
-        builder_ids = [b.id for b in builders]
+        builders_by_id = {b.id: b for b in builders}
         pairs = list(store.using(BuilderProcessor, Processor).find(
             (BuilderProcessor.builder_id, BuilderProcessor.processor_id),
             BuilderProcessor.processor_id == Processor.id,
-            BuilderProcessor.builder_id.is_in(builder_ids)).order_by(
+            BuilderProcessor.builder_id.is_in(builders_by_id)).order_by(
                 BuilderProcessor.builder_id, Processor.name))
         load(Processor, [pid for bid, pid in pairs])
         for builder in builders:
             get_property_cache(builder)._processors_cache = []
         for bid, pid in pairs:
-            cache = get_property_cache(store.get(Builder, bid))
+            cache = get_property_cache(builders_by_id[bid])
             cache._processors_cache.append(store.get(Processor, pid))
 
     def getBuilders(self):