Utilize buildCache in a smarter way to prevent pathological disk I/O storms.

Utilize buildCache in a smarter way to prevent pathological disk I/O storms.

This should reduce latency spikes seen during periods of moderate to low tryserver usage.

Background for this patch is discussed in https://codereview.chromium.org/18429003.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=209732

[Mike Stip (use stip instead), 2013-07-02 00:43:54]

Please take a look at the above patch. Note that due to review churn, I will add
the final README.chromium diff once the patch is in final form.

Background: https://codereview.chromium.org/13619004/ was an attempt to
drastically reduce latency and CPU usage of the tryserver's /json endpoint.
While local tests showed great gains, when deployed to the tryserver results
were mixed: latencies were reduced during peak hours, but during off-peak hours
they remained staggeringly high. After profiling and log analysis, there seemed
to be an abnormally high amount of disk I/O from unpickling builds. Further
analysis showed that the builds being loaded were *already* in the buildCache.
This observation prompted the creation of this CL.

The following sequence of actions will cause 90 builds to be loaded from disk,
no matter the state of the tryserver's linux_rel buildCache (the actual build #s
don't matter as long as they exist on the server and are > 90 builds old):

curl
build.chromium.org/p/tryserver.chromium/json/builders/linux_rel/builds/111742 >
/dev/null
curl
build.chromium.org/p/tryserver.chromium/json/builders/linux_rel/builds/111741 >
/dev/null
curl
build.chromium.org/p/tryserver.chromium/json/builders/linux_rel/builds/111740 >
/dev/null
curl build.chromium.org/p/tryserver.chromium/json > /dev/null

On an unloaded Z620, this adds 1.5s of latency to the root json endpoint (2s vs
0.5s), and this is just touching linux_rel. A full load of archived tryserver
data takes approximately 15s, which is in-line with production tryserver data.

What is happening here is a bad interaction between the root json's cache access
pattern and the design of buildbot's SyncLRUCache. The root json code actually
populates the buildCache with the last len(buildCache) builds. It does this by
sequentially calling getBuild(), which uses SyncLRUCache's logic to evict the
oldest entry on a cache miss.

This works fine until additional builds outside the last len(buildCache) builds
are introduced. The pathological condition happens when a certain number of
builds are accessed that are outside the last len(buildCache) entries. When
those entries are accessed, they evict the oldest entry in the cache -- which
would be the latest build. When /json is called, it starts its loop and
populates the cache with the latest build -- which evicts the second latest
build. It moves to loading the second latest build -- evicting the third. The
process continues until the entire cache is reloaded.

This also explains why the latency was good under high load -- CQ is constantly
polling and keeping the caches fresh. It increases the chance that the
monitoring script will land on a 'good' cache instead of a 'pathological' one. 

This patch introduces getBuilds(). getBuilds() provides a way to load in-cache
builds before cache misses, intelligently sidestepping this problem. It also
randomizes its access pattern to make the LRU pattern non-sequential. This is
designed to alleviate the same condition caused by external scripts (CQ,
tryserver monitoring) from triggering the same cascade by repeatedly accessing
sequential buildnumbers.

As we are increasingly relying on monitoring scripts crawling tryserver data,
three options present themselves for future scalability: replace SyncLRUCache
with a cache that selectively evicts based on historical data, provide a
getBuilds() url endpoint, or move build data off the tryserver onto the cloud
where scripts can crawl with wanton abandon. The third option seems like the
natural course, but that discussion is for another CL.

As a side effect of the new LRU pattern, the builder asDict() method also
populates its buildCache. This ensures that the builds presented are sequential
and don't have "holes" due to randomly evicted entries.

[iannucci, 2013-07-02 01:15:27]

looks pretty good to me :)

https://chromiumcodereview.appspot.com/18429003/diff/1/third_party/buildbot_8...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://chromiumcodereview.appspot.com/18429003/diff/1/third_party/buildbot_8...
third_party/buildbot_8_4p1/buildbot/status/builder.py:363: (i, n) for i, n in
enumerate(numbers) if n is not None]
Seems like this filtering could be done in the for loop below, and it would be
clearer (and would avoid looping over numbers several times)?

https://chromiumcodereview.appspot.com/18429003/diff/1/third_party/buildbot_8...
third_party/buildbot_8_4p1/buildbot/status/builder.py:368:
random.shuffle(valid_numbers)
comment is redundant w/ docstring

https://chromiumcodereview.appspot.com/18429003/diff/1/third_party/buildbot_8...
third_party/buildbot_8_4p1/buildbot/status/builder.py:668: buildnums = [-i for i
in buildnums]
Would it be worth having a helper function for this 'get last N' pattern?

https://chromiumcodereview.appspot.com/18429003/diff/1/third_party/buildbot_8...
File third_party/buildbot_8_4p1/buildbot/status/web/status_json.py (right):

https://chromiumcodereview.appspot.com/18429003/diff/1/third_party/buildbot_8...
third_party/buildbot_8_4p1/buildbot/status/web/status_json.py:646: buildnums =
[-i for i in buildnums]
range(-1, -(size - 1), -1)?

[Mike Stip (use stip instead), 2013-07-02 02:18:34]

updated. ptal

https://codereview.chromium.org/18429003/diff/1/third_party/buildbot_8_4p1/bu...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://codereview.chromium.org/18429003/diff/1/third_party/buildbot_8_4p1/bu...
third_party/buildbot_8_4p1/buildbot/status/builder.py:363: (i, n) for i, n in
enumerate(numbers) if n is not None]
On 2013/07/02 01:15:27, iannucci wrote:
> Seems like this filtering could be done in the for loop below, and it would be
> clearer (and would avoid looping over numbers several times)?

Done.

https://codereview.chromium.org/18429003/diff/1/third_party/buildbot_8_4p1/bu...
third_party/buildbot_8_4p1/buildbot/status/builder.py:368:
random.shuffle(valid_numbers)
On 2013/07/02 01:15:27, iannucci wrote:
> comment is redundant w/ docstring

Done.

https://codereview.chromium.org/18429003/diff/1/third_party/buildbot_8_4p1/bu...
third_party/buildbot_8_4p1/buildbot/status/builder.py:668: buildnums = [-i for i
in buildnums]
On 2013/07/02 01:15:27, iannucci wrote:
> Would it be worth having a helper function for this 'get last N' pattern?

well now it's one line...

https://codereview.chromium.org/18429003/diff/1/third_party/buildbot_8_4p1/bu...
File third_party/buildbot_8_4p1/buildbot/status/web/status_json.py (right):

https://codereview.chromium.org/18429003/diff/1/third_party/buildbot_8_4p1/bu...
third_party/buildbot_8_4p1/buildbot/status/web/status_json.py:646: buildnums =
[-i for i in buildnums]
On 2013/07/02 01:15:27, iannucci wrote:
> range(-1, -(size - 1), -1)?

Done.

[Isaac (away), 2013-07-02 02:38:25]

nice!

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
third_party/buildbot_8_4p1/buildbot/status/builder.py:334: def
_safeGetBuild(self, number):
maybe name this var 'build_number'

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
third_party/buildbot_8_4p1/buildbot/status/builder.py:366: for idx, number in
numbers:
better variable name for number, perhaps build_number?

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
File third_party/buildbot_8_4p1/buildbot/status/web/status_json.py (right):

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
third_party/buildbot_8_4p1/buildbot/status/web/status_json.py:646: builds =
builder_status.getBuilds(buildnums)
nice.

[Mike Stip (use stip instead), 2013-07-02 05:43:52]

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
third_party/buildbot_8_4p1/buildbot/status/builder.py:334: def
_safeGetBuild(self, number):
On 2013/07/02 02:38:25, Isaac wrote:
> maybe name this var 'build_number'

Done.

https://codereview.chromium.org/18429003/diff/6001/third_party/buildbot_8_4p1...
third_party/buildbot_8_4p1/buildbot/status/builder.py:366: for idx, number in
numbers:
On 2013/07/02 02:38:25, Isaac wrote:
> better variable name for number, perhaps build_number?

Done.

[Isaac (away), 2013-07-02 06:00:47]

this LGTM -- but wait for robbie or maruel.  I don't quite understand why the
shuffle is necessary after you fix the json fetch ordering, but it can't hurt
since you're pulling cached entries first.

[Isaac (away), 2013-07-02 06:01:42]

https://codereview.chromium.org/18429003/diff/12001/third_party/buildbot_8_4p...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://codereview.chromium.org/18429003/diff/12001/third_party/buildbot_8_4p...
third_party/buildbot_8_4p1/buildbot/status/builder.py:327: def
_resolveBuildNumber(self, build_number):
and this one should be number.

https://codereview.chromium.org/18429003/diff/12001/third_party/buildbot_8_4p...
third_party/buildbot_8_4p1/buildbot/status/builder.py:334: def
_safeGetBuild(self, number):
this variable should be build_number

[iannucci, 2013-07-02 18:24:26]

LGTM. The shuffle is because we:
  1) Can't /really/ fix the cache behavior without much more work; and
  2) Can't control the query order from external sources.

Randomizing will make us hit the average runtime as opposed to the worst runtime
(which we're hitting now). Unless they can guess the order we're getting from
shuffle, and they intentionally try to DOS the master.

[commit-bot: I haz the power, 2013-07-02 18:31:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xusydoc@chromium.org/18429003/23001

[commit-bot: I haz the power, 2013-07-02 18:32:28]

Change committed as 209732